Counting quiz for children I made a game for children. This game tests the ability to count (i.e. compare numbers based on magnitude). First you must select the age of the child in the upper menu, then you need to select blocks with numbers in ascending order.

JSFIDDLE

document.addEventListener("DOMContentLoaded", ready);

let currNum;
let step;
let startContent = 'Это простейший тренажёр для детей, он проверяет способность считать. Предполагается, что: \

дети от 1 до 3 лет должны уметь считать до 5 \

дети от 4 до 5 лет должны уметь считать до 10 \

дети от 6 до 7 лет должны уметь считать до 30 \

дети от 7 до 9 лет должны уметь считать до 100 \

Чтобы проверить знания участнику предлагается последовательно в порядке возрастания выбрать кубики с цифрами \
';

function init() {
 currNum = -1;
 step = 0
 let startBtn = document.getElementById('start');
 startBtn.click();
};

function clearContent() {
 let contentInner = document.getElementById('contentInner');
 contentInner.innerHTML = '';
};

function deletePoint(id) {
 let pointEl = document.getElementById(id);
 pointEl.parentNode.removeChild(pointEl);
};

function createPoints(count) {
 let contentInner = document.getElementById('contentInner');

 for(let i = 0; i <= 2; i++) {
  let num_ = Math.random() * count;
  let num = Math.ceil(num_);

  let pointEl = document.createElement('div');
  pointEl.className = 'point';
  pointEl.id = 'point_' + i;
  pointEl.setAttribute('data-num', num);
  pointEl.innerHTML = num;

  contentInner.appendChild(pointEl);
 };
};

function ready() {
 let ulEl = document.getElementById('ul');
 let contentInnerEl = document.getElementById('contentInner');

 let menu = Rx.Observable.fromEvent(ulEl, 'click');
 let contentInner = Rx.Observable.fromEvent(contentInnerEl, 'click');

 let subscription = menu
 .map(e => e.target)
 .filter(t => t.className === 'start' || t.className === 'li')
 .subscribe(function (t) {
  clearContent();
    switch (t.className) {
      case 'start':
    document.getElementById('contentInner').innerHTML = startContent;
        break;
      case 'li':
    createPoints(t.getAttribute('data-count'));
        break;
    }
 });

 let subscription3 = contentInner
 .map(e => e.target)
 .filter(t => t.className === 'point')
 .subscribe(function (t) {
  let val = +document.getElementById(t.id).innerHTML;

  ++step;
  deletePoint(t.id);

  if(currNum > val) {
   alert('Not correct');
   init();
  } else {
   currNum = val;
   if(step > 2) {
    alert('Correct');
    init();
   }
  }
 });

 init();
};

1-3

4-5

6-7

7-9

start

First off I noticed a problem in how the quiz chooses the numbers:

It can easily happen that two (or more) numbers are the same. Especially when picking number between 1..5. In that case it can be confusing to the user, which of the two equal numbers should he click.

The main problems with the code resides in tight coupling between UI logic and business logic. For example the createPoints() function:

function createPoints(count) {
    let contentInner = document.getElementById('contentInner');

    for(let i = 0; i <= 2; i++) {
        let num_ = Math.random() * count;
        let num = Math.ceil(num_);

        let pointEl = document.createElement('div');
        pointEl.className = 'point';
        pointEl.id = 'point_' + i;
        pointEl.setAttribute('data-num', num);
        pointEl.innerHTML = num;

        contentInner.appendChild(pointEl);
    };
};

This function both:

generates three random numbers and
displays the numbers on page.

Would be better two have two functions that separately perform the tasks, something that could be used like so:

displayNumbers(randomNumbersBelow(10))

Also note the naming: the original name createPoints() seems to be talking about some coordinates, which doesn't sound correct to me.

By separating the two, it should be also easier to improve randomNumbersBelow() function so that it would not return duplicates, without having to modify anything inside displayNumbers().

Additionally:

num_ variable can be easily eliminated by inlining the expression: Math.ceil(Math.random() * count).
Instead of modifying the contentInner element by itself, it could just return the new contents (e.g. as a DocumentFragment). That would eliminate the need for the existence of clearContent() function.


SICP Exercise 1.3: Sum of squares of two largest numbers out of three, Haskell Version The exercise 1.3 of the book Structure and Interpretation of Computer Programs asks the following:

  Exercise 1.3.  Define a procedure that takes three numbers as arguments and returns the sum of the squares of the two larger numbers.

My answer is this:

    import Data.List(sort)

    sumSquareLargest :: (Ord n, Floating n) => n -> n -> n -> n
    sumSquareLargest x y z = b ** 2 + c ** 2 where
        [a, b, c] = sort[x, y, z]

Am I doing unnecessary work here? How could I improve this answer, even if only stylistically-speaking?

If you don't need a value, don't bind it, use a wildcard instead.

[a, b, c] = sort [x, y, z] # and btw, add a space after the function name

becomes

[_, b, c] = sort [x, y, z]

Perhaps its just personal preference (I don't use much Haskell myself), but I usually see the where clause in the next line, with an extra tab, like:

sumSquareLargest x y z = b ** 2 + c ** 2
    where [_, b, c] = sort [x, y, z]

Moreover, the b and c values are not obvious at first glance, and you need to read the second line before the first. In these cases, a let expression can be a more nice alternative:

sumSquareLargest x y z =
    let [_, b, c] = sort [x, y, z]
    in b ** 2 + c ** 2

This is a very small function, but consider having more meaningful names for your bindings:

sumSquareLargest x y z =
    let [_, medium, maxim] = sort [x, y, z]
    in medium ** 2 + maxim ** 2

Also, consider writing some unit tests in a library like QuickCheck.


Prioritizing the most important value in an array I have an array of names where I need to find the most important person.

This is my array. I cannot edit this array unless I create a copy of it

$data = [
    'Jack',
    'Josh',
    'Boris',
    'Bob',
    'Boris',
    'Jack',
    'Josh'
];

The most important person is number 1. After number 1 the most improtant person is number 2 and so on. These are the priorities.

Jack
Bob
Boris
Josh

The best way of tackling this issue I could think of is like the following:

$i = 5;
foreach ($data as $val) {
    if ($val == 'Jack' && $i > 1) {
        $myValue = $val;
        $i = 1;
        break;
    } else if ($val == 'Bob' && $i > 2) {
        $myValue = $val;
        $i = 2;
    } else if ($val == 'Boris' && $i > 3) {
        $myValue = $val;
        $i = 3;
    } else if ($val == 'Josh' && $i > 4) {
        $myValue = $val;
        $i = 4;
    }
}

It works great. But it just doesn't feel right.

Is prioritizing it like this common practice?

In most circumstances it is better to do sorting in your SQL layer

<?php
$sorted_data = $data;
$importance = array_flip(['Jack', 'Bob', 'Boris', 'Josh']);
usort($sorted_data, function ($a, $b) use ($importance) {
  return $importance[$a] <=> $importance[$b];
});
$important_person = reset($sorted_data);

If you need to reuse your code then I suggest:

<?php
function sort_users(array $importance) : Closure {
  return function (string $user_a, string $user_b) use ($importance) : int {
      $a_priority = $importance[$user_a] ?? 0;
      $b_priority = $importance[$user_b] ?? 0;
      return $a_priority <=> $b_priority;
  };
}

$sorted_data = $data;
$importance = array_flip(['Jack', 'Bob', 'Boris', 'Josh']);
usort($sorted_data, sort_users($importance));
$important_person = reset($sorted_data);

Some of the above code can be removed if you can sort the original array, and if $data always contains elements of $importance

I would suggest thinking of a better variable name for $data because it's not obvious what data is, is $first_names better?


Hackerrank - "Sherlock and the Beast" I solved the problem but for two test cases, it's giving me timeout error. It's taking longer than 2 seconds on their servers. I tried using <ctime> to calculate the run time of the program and on my system, it's taking between 1.3-1.6 seconds. It's only giving me error for TestCases #9 and #10.

The question is to find the largest N-digit "decent" number, where a decent number consists only of the digits '3' and '5', and the number of threes is divisible by 5, and the number of fives is divisible by 3.

#include <iostream>
#include <string>

std::string decent_number(int, std::string);

int main() {
  clock_t start = clock();
  int numOfCases;

  std::cin >> numOfCases;
  int n;
  for (int i = 0; i < numOfCases; ++i) {
    std::string dn("-1");
    std::cin >> n;
    for (int j = n; j >= 0; --j) {
      if (j % 3 == 0 && (n-j) % 5 == 0) {
        dn = decent_number(j, "5");
        dn += decent_number(n-j, "3");
        break;
      }
    }
    std::cout << dn << std::endl;
  }
  clock_t ends = clock();
  std::cout << "Time: " << static_cast<double>(ends - start)/ CLOCKS_PER_SEC
            << std::endl;
  return 0;
}

std::string decent_number(int repeatNum, std::string num) {
  std::string dn;
  for (int i = 0; i < repeatNum; ++i)
    dn = dn + num;
  return dn;
}

The code is simple, I check for the number of 3s and 5s which make the number and call a string builder function which builds the string.

Your problem is clearly caused by that decent_number function of yours. Once you have the number of '5' digits and  the number of '3' digits, just dump the number to the console - building an std::string to hold it is totally redundant:

Your program currently takes Time: 1.71979 on my system when running Test 9 (which fails in the HackerRank test environment).
If instead of building that string you simply print the digits it takes: Time: 0.026608.

Alternatively you could simply build the whole strings from the start (instead of using the loop to append one character at a time) - this works even faster: Time: 0.000818.

dn = std::string(j, '5');
if (n-j) {
    dn += std::string(n-j, '3');
}


Find the first non-recurring character in string I wrote a solution to first-unique-character-in-a-string:

  Given a string, find the first non-repeating character in it and return its index. If it doesn't exist, return -1.

If there is better way for me to build the dictionary seems? and find the first key when value is 1 and index is lowest?

def firstUniqChar(string) -> int:
    seems = dict()
    index = len(string)
    c = ''
    for i, char in string:
        if char not in seems:
            seems.setdefault(char, (i,1))
        else:
            seems[char] = (seems[char][0], seems[char][1] + 1)

    for k, value in seems.items():
        if value[1] == 1 and value[0] < index:
            index = value[0]
            c = k
    return  index if c else -1

The code is too convoluted and unreadable for such a simple task

seems is a dictionary where the values are (first index, occurrences) pairs.  That's an unconventional data structure.  You'd be better off with two separate dictionaries.
seems is a weird variable name.  What does it mean, anyway?
Why is the index = len(string) statement necessary?

This solution is less cluttered, and more clearly expresses the idea that you are counting the characters, then finding the first unique one.

from collections import Counter

def first_uniq_char(s):
    counts = Counter(s)
    return next((i for i, c in enumerate(s) if counts[c] == 1), -1)


Smallest job first algorithm implementation I have implemented smallest job first algorithm using javascript. It was part of a coding challenge. I would like to ask for evaluation of my script. I would like to get suggestions to improve my code performance. Which parts of the code I can improve. Is there an alternative to for loops?

Function takes jobs array with transaction time and index. Returns the clock cycles of the process/job at specific index. Function Call with sample input, sjf([10, 3, 15, 8, 7], 3).

Here is the code.

function sjf(jobs, index) {

// declare waiting time array
var waitingTime = [];
// set waiting time for first task to zero
waitingTime[0] = 0;

// declare total time array
var totalTimeArray = [];

var processArray = [];
// add job value and index as an object to process array
for (i=0; i < jobs.length; i++) {
    processArray.push({
        id: i,
        value: jobs[i]
    })
}

// sort the array in ascending order
function sortArray(a, b) {
    return a.value - b.value;
}

// calculate waiting time for each process except first process
// waiting time for first process is zero so we start to calculate from index 1
function calculateWaitingTime(sortedArray, waitingTime) {
    for (i=1; i < sortedArray.length; i++) {
        waitingTime.push(sortedArray[i-1].value + waitingTime[i-1]);
    }
}

// total time taken to complete each task
function calculateTimeForEachTask(sortedArray, totalTimeArray) {
    for (i=0; i < sortedArray.length; i++) {
        totalTimeArray.push({
            id: sortedArray[i].id,
            time: sortedArray[i].value + waitingTime[i]
        });
    }
}

// find clock cycles
function findClockCycles(totalTimeArray, index) {
    for (i = 0; i < totalTimeArray.length; i++) {
        if (totalTimeArray[i].id === index) {
            return totalTimeArray[i].time;
        }
    }
}

// First of all sort the process array in ascending order of their value
var sortedArray = processArray.sort(sortArray);

// calculate waiting time for the rest of the processes
calculateWaitingTime(sortedArray, waitingTime);

// calculate total time for each task
calculateTimeForEachTask(sortedArray, totalTimeArray);

// return clock cycles for the task
return findClockCycles(totalTimeArray, index);
}

I think this can be done with far less code. Like this:

function calculateTotalClockCycles(jobs, index) {
  jobs.sort((a, b) => a - b);
  return jobs.slice(0, index).reduce((a, b) => a + b, 0);
}

alert(calculateTotalClockCycles([10, 3, 15, 8, 7], 3));

In the first line of code inside the function, the jobs are sorted, small to large. The second line is more complicated. The reduce() method is used to sum all the values of the array it applies to. That array is the wanted part (slice) of the whole sorted jobs array.

So here reduce() is used as an alternative for a 'for' loop.


Recursive calculation of second order derivative I am writing code to do some numerical task using the routines of the book Numerical Recipes. One of my objectives is to calculate the second derivative of a function and have a routine that calculates the first derivative of a function in a nice manner within a specified accuracy. However, I would like to generalize this method and use this function recursively to find the second derivatives or higher order derivatives if needed. I have made some changes in my code and defined a reduced function which takes one argument and achieved my aim more or less. The function that calculates the first derivatives are declared as follows:

float dfridr(float (*func)(float), float x, float h, float *err);

My aim is to set the h value by calling another function for both of the nested dfridr functions at once. For clarity I have enclosed my code. I would appreciate it if you could comment on my method and provide suggestions for improvement and general feedback. As far as I've checked, it works better than the finite difference algorithms, but I think it may be refined.

// The compilation command used is given below
// gcc Q3.c nrutil.c DFRIDR.c -lm -o Q3

#include <stdio.h>
#include <math.h>
#include <stdlib.h>
#include "nr.h"

#define LIM1 20.0
#define a -5.0
#define b 5.0
#define pre 100.0 // This defines the precision
/* This file calculates the func at given points, makes a
 * plot. It also calculates the maximum and minimum of the func
 * at given points and its first and second numerical derivative.
*/
float func(float x)
{
    return exp(x / 2) / pow(x, 2);
}

// We define the following functions to aid in the calculation of the
// second derivative
float reddfridr(float x)
{
    float err;
    return dfridr(func, x, 0.1, &err);
}
float dfridr2(float x, float h)
{
    float err;
    return dfridr(reddfridr, x, h, &err);
}
int main(void)
{
    FILE *fp = fopen("Q3data.dat", "w+"), *fp2 = fopen("Q3results.dat", "w+");
    int i; // Declaring our loop variable
    float min, max, err, nd1, nd2, x, y;
    // Define the initial value of the func to be the minimum
    min = func(0);
    // Initialize x
    x = 0;
    for(i = 0; x < LIM1 ; i++)
    {
        x = i / pre; // There is a singularity at x = 0
        y = func(x);
        if(y < min)
            min = y;
        fprintf(fp, "%f \t %f \n", x, y);
    }
    fprintf(fp, "\n\n");
    max = 0;
    // Never forget to initialize x again
    x = a;
    // Since i is incremented at the end of the loop
    for(i = 0; x < b  ; i++)
    {
        x = a + i / pre;
        y = func(x);
        nd1 = dfridr(func, x, 0.1, &err);
        nd2 = dfridr2(x, 0.1);
        fprintf(fp, "%f \t %f \t %f \t %f\n", x, y, nd1, nd2);
        if(y > max)
            max = y;
    }
    fprintf(fp2, "The minimum value of f(x) is %f when x is between 0 and 20. \n", min);
    fprintf(fp2, "The maximum value of f(x) is %f when x is between -5 and 5. \n", max);
    fclose(fp);
    fclose(fp2);
    return 0;
}

Recommendations:

Your output files are delimited by both spaces and tabs, which is weird.  You probably want to use tabs only.
There are a couple of points related to floating-point handling that I think deserve commenting:

Evaluating func(0.0) has a side effect of setting some floating-point error flags.
You actually intend to step x by Δx=0.01 between some lower bound and upper bound.  However, repeated addition of 0.01 would result in accumulation of round-off errors, so a workaround is used.

pow(x, 2) could be simplified to x * x.
Naming could be better.  I suggest:

pre → inv_delta_x (to emphasize its relationship to x)
fp → table, and fp2 → extrema (to explain the contents of the files)

Since you are already using C99-style comments, you should also take advantage of C99-style variable declarations, which allow you to declare variables closer to the point of use to enhance readability.

I've defined a FOR_RANGE() macro to make the loops in main() more readable, but that may be a controversial improvement.  I've also initialized min and max to INFINITY and -INFINITY, respectively, as a personal preference.

I agree that using recursion will probably yield unsatisfactory results, though I am unable to advise you further.  One of the comments above links to a related math question.  You will probably get a better answer about numerical methods on Computational Science SE.

#include <math.h>
#include <stdio.h>

// Singularity at x=0.0 raises exception FE_DIVBYZERO and/or sets errno to EDOM
float func(float x)
{
    return exp(x / 2) / (x * x);
}

#define FOR_RANGE(type, var, lower_bound, upper_bound, inv_delta) \
        for (type iteration_dummy = (lower_bound) * (inv_delta), var; \
             (var = iteration_dummy / (inv_delta)) <= (upper_bound); \
             iteration_dummy++)

int main(void)
{
    // We want to increment x in steps of 0.01, but repeatedly adding 0.01
    // would cause round-off errors to accumulate.  Therefore, we will divide
    // successive floating-point integers by 100 instead.
    const float inv_delta_x = 100.0;

    FILE *table   = fopen("Q3data.dat", "w+"),
         *extrema = fopen("Q3results.dat", "w+");

    float min = INFINITY;
    FOR_RANGE(float, x, 0.0, 20.0, inv_delta_x)
    {
        float y = func(x);
        if (y < min) min = y;
        fprintf(table, "%f\t%f\n", x, y);
    }

    fprintf(table, "\n\n");

    float max = -INFINITY;
    FOR_RANGE(float, x, -5.0, 5.0, inv_delta_x)
    {
        float y = func(x);
        float err;
        float nd1 = dfridr(func, x, 0.1, &err);
        float nd2 = dfridr2(x, 0.1);
        if (y > max) max = y;
        fprintf(table, "%f\t%f\t%f\t%f\n", x, y, nd1, nd2);
    }

    fprintf(extrema, "The minimum value of f(x) is %f when x is between 0 and 20.\n", min);
    fprintf(extrema, "The maximum value of f(x) is %f when x is between -5 and 5.\n", max);
    fclose(table);
    fclose(extrema);
    return 0;
}


Postponed Prime Sequence in Kotlin Following my previous unbounded prime generator and a followup by Martin R, I've tested the waters in Kotlin by making an unbounded sieve.

To quote Martin R's wonderful explanation of the base algorithm,

  The interesting thing about that algorithm is that the prime generator needs a list of "base primes" which are created using another instance of itself. This works because

  the base prime generator is created "delayed", after producing some fixed primes, and
  in order to generate primes up to \$ N \$, base primes are needed only up to \$ \sqrt N \$

  so that the creation of nested prime generators terminates eventually.

  The sieving is done using a dictionary which holds the "next composite numbers" which are divisible by any prime less than the current base prime. As explained [on SO], the required memory to produce \$ n \$ primes is \$ O(\sqrt n). \$

class PostponedPrimeSequence() : AbstractIterator<Int>(), Sequence<Int> {
    override fun iterator() = this

    private var basePrimes: PostponedPrimeSequence? = null
    private var basePrime = 0

    private var sieve = mutableMapOf<Int, Int>()
    private var initialPrimes = mutableListOf(2, 3, 5, 7)
    private var candidate = 9

    override fun computeNext() {
        if (!initialPrimes.isEmpty()) {
            setNext(initialPrimes.removeAt(0))
        } else {
            if (candidate == 9) {
                basePrimes = PostponedPrimeSequence()
                basePrimes!!.next()
                basePrime = basePrimes!!.next()
                assert(candidate == basePrime * basePrime)
            }

            while (candidate > 0) {
                val factor = sieve.remove(candidate) ?:
                        if (candidate == basePrime * basePrime) {
                            val temp = basePrime
                            basePrime = basePrimes!!.next()
                            temp
                        } else {
                            assert(candidate < basePrime * basePrime)
                            setNext(candidate)
                            break
                        }
                var j = candidate + 2 * factor
                while (sieve.containsKey(j)) {
                    j += 2 * factor
                }
                sieve[j] = factor
                candidate += 2
            }
            candidate += 2
        }
    }
}

Usage:

PostponedPrimeSequence().take(20).forEach(::println)

All feedback welcome, though reviews are encouraged to look at:

Naming: As I'm new to Kotlin, I don't know the naming conventions yet
Performance: If there are simple, provable improvements without drastically changing the algorithm (such as adding a large wheel)
Iteration organization: As Kotlin lacks a true yielding generator and I wanted a true Iterator/Sequence, rather than a callback based "poor man's yield", the iteration isn't exactly the prettiest matter. The candidate += 2 } candidate += 2 bit especially feels like there should be a better way to do this, and the break (which is related) breaks ha simple control flow, which is not ideal.
!!: Even on the line immediately following assignment to basePrimes, I have to use !! to potentially fail on accessing the next method (Smart cast to 'PostponedPrimeSequence' is impossible, because 'basePrimes' is a mutable property that could have been changed by this time). I know that, due to reflection, any variable on the JVM could change at any time, but this seems like a smelly smell that smells smelly. basePrimes has to be mutable and start out null, though, because otherwise the class would recursively take infinite space. In this case, though, I want to initialize it once, late, and from then on it should act as an immutable (non-nullable) val reference. If this is possible in Kotlin, I don't know how to do so yet.

You can make basePrimes a lateinit non-null var to avoid !!.
sieve and initialPrimes are only assigned once so they can be marked with val instead of var
sequenceOf(2, 3, 4, 7).iterator() can be used along with Iterator.hasNext and Iterator.next instead of mutableListOf(2, 3, 5, 7) with somewhat difficult to read !initialPrimes.isEmpty() and initialPrimes.removeAt(0) (although if you really prefer to use a collection instead of an iterator then you can use isNotEmpty instead of !collection.isEmpty).
Instead of explicitly declaring a temp variable you can use apply to take the current value of a variable, use/change it, and return the initial value.
As an alternative to sieve.containsKey(j) you can use j in sieve.
As an alternative to candidate += 2 } candidate += 2 you can move the latter += 2 to relatively the beginning of computeNext (requires a few other changes, see below). The name candidate would no longer really fit though, but I think a generic property name like value is sufficient enough (we know what "value" we care about so we know what we're computing as we go, etc.).
You probably shouldn't implement Iterator<Int> and Sequence<Int> in the same object. e.g. When invoking with(PostponedPrimeSequence()) { repeat(2) { take(20).forEach(::println) } } I would expect it as a Sequence<Int> to print the first 20 primes twice (or throw "IllegalStateException: This sequence can be consumed only once.") but as an Iterator<Int> I would expect it to print the first 40 primes. These are conflicting expectations. I recommend only implementing one. A Sequence is understood to possibly be constrained to be iterated only once so I would implement the base Iterator and then wrap it as a Sequence in a top-level function for convenience.

e.g.

fun generatePrimeSequence() = PostponedPrimeIterator().asSequence()

class PostponedPrimeIterator() : AbstractIterator<Int>() {
    private lateinit var basePrimes: PostponedPrimeIterator
    private var basePrime = 0

    private val sieve = mutableMapOf<Int, Int>()
    private val initialPrimes = sequenceOf(2, 3, 5, 7).iterator()
    private var value = 0

    override fun computeNext() {
        if (initialPrimes.hasNext()) {
            value = initialPrimes.next()
            setNext(value)
        } else {
            value += 2

            if (value == 9) {
                basePrimes = PostponedPrimeIterator()
                basePrimes.next()
                basePrime = basePrimes.next()
                assert(value == basePrime * basePrime)
            }

            while (value > 0) {
                val factor = sieve.remove(value) ?:
                        if (value == basePrime * basePrime) {
                            basePrime.apply {
                                basePrime = basePrimes.next()
                            }
                        } else {
                            assert(value < basePrime * basePrime)
                            setNext(value)
                            break
                        }
                var j = value + 2 * factor
                while (j in sieve) {
                    j += 2 * factor
                }
                sieve[j] = factor
                value += 2
            }
        }
    }
}

Usage:

generatePrimeSequence().take(20).forEach(::println)


How to avoid code duplication in ordering system Here is the piece of order system.
And this is hierarchy of order classes in this system

Public MustInherit Class BaseItem
    Public Property Id() As Guid
    Public Property Sku() As String
    Public Property Quantity() As String
    Public Property TransactionId() As Integer?
End Class

Public Class SoOrderItem
    Inherits BaseItem

    Public Property BatchNumber() As String
    Public Property Lottable1() As String
    Public Property Lottable2() As String
    Public Property Lottable3() As String
    Public Property Lottable4() As String

End Class

Public Class AsnOrderItem
    Inherits BaseItem

   Public Property AdditionalInfo() As String

End Class

Public Class BaseOrder
    Public Property OrderReference() As String
    Public Property PoNumber() As String
    Public Property CustomerVat() As String
    Public Property CustomerReference() As String
    Public Property TransactionType() As String
End Class

Public Class SoOrder
    Inherits BaseOrder

    Public Property ConsigneeAddress() As String
    Public Property ConsigneeContact() As String
    Public Property ConsigneeName() As String
    Public Property OrderItems() As New List(Of SoOrderItem)
End Class

Public Class AsnOrder
    Inherits BaseOrder

    Public Property Account() As Account
    Public Property SupplierName() As String
    Public Property OrderItems() As New List(Of AsnOrderItem)
End Class

And here is confirm classes for them

Public Class SoConfirmation
     Private _itransactionService As ITransactionService

     Private Sub AssignNewTransactionsToOrderItems(soOrders As IEnumerable(Of SoOrder))

        For Each order In soOrders
            For Each item In order.OrderItems
                If (Not item.TransactionId.HasValue) Then
                    Dim newTransactionId =
_itransactionService.GenerateTransactionId()
   _itransactionService.AssignTransactionToOrderItem(order.OrderReference, item.Sku, item.Id, newTransactionId)
                    item.TransactionId = newTransactionId
                End If
            Next
        Next
    End Sub

    Private Sub FilterForConfirmation(soOrders As IEnumerable(Of SoOrder))

        For Each order In soOrders
            order.OrderItems.RemoveAll(Function(i) _itransactionService.IsTransactionProcessedSuccesfully(order.OrderReference, i.Id))
        Next
    End Sub
End Class

Public Class AsnConfirmation
     Private _itransactionService As ITransactionService

  Private Sub AssignNewTransactionsToOrderItems(asnOrders As IEnumerable(Of AsnOrder))

        For Each asn In asnOrders
            For Each item In asn.OrderItems
                If (Not item.TransactionId.HasValue) Then
                    Dim newTransactionId = _itransactionService.GenerateTransactionId()

_itransactionService.AssignTransactionToOrderItem(asn.OrderReference, item.Sku, item.Id, newTransactionId)
                    item.TransactionId = newTransactionId
                End If
            Next
        Next
    End Sub

    Private Sub FilterForConfirmation(asnOrders As IEnumerable(Of AsnOrder))

        For Each order In asnOrders
            order.OrderItems.RemoveAll(Function(i) _itransactionService.IsTransactionProcessedSuccesfully(order.OrderReference, i.Id))
        Next
    End Sub
End Class

As you can see confirmation classes have a lot of duplcited code and i don't know how to avoid it ...
Will be appreciate for any help.

You are right that you have code duplication where no duplication should be. As far as I can see from the code you have posted, nothing really changes if you would write a class that would handle it as a BaseOrder, since all the properties you are referring to are available on the BaseOrder (except for OrderItems, however this could be retrieved through an extra method implementing it)

So you could write your class more like

Public MustInherit Class BaseOrderConfirmation
    Private _itransactionService As ITransactionService

    Protected MustOverride Function GetOrderItems(order as BaseOrder) As IEnumerable(Of BaseItem)

    Protected Sub AssignNewTransactionsToOrderItems(orders As List(Of BaseOrder))
        For Each order In orders
            For Each item In GetOrderItems(order)
                If (Not item.TransactionId.HasValue) Then
                    Dim newTransactionId = _itransactionService.GenerateTransactionId()
                    _itransactionService.AssignTransactionToOrderItem(order.OrderReference, item.Sku, item.Id, newTransactionId)
                    item.TransactionId = newTransactionId
                End If
            Next
        Next
    End Sub

    Protected Sub FilterForConfirmation(orders As IEnumerable(Of BaseOrder))
        For Each order In orders
            GetOrderItems(order).RemoveAll(Function(i) _itransactionService.IsTransactionProcessedSuccesfully(order.OrderReference, i.Id))
        Next
    End Sub
End Class

This would now be an abstract class (ie: a class that cannot be instantiated), from which the GetOrderItems method has to be implemented still.

Also note, that this change means that all methods now changed from Private modifier to the Protected modifier

In the implementing classes you can then implement something like:

Public Class SoOrderConfirmation
    Inherits BaseOrderConfirmation

    Protected Overrides Function GetOrderItems(order as BaseOrder) as List(Of BaseItem)
        return DirectCast(order, SoOrder).OrderItems
    End Function
End Class

Preferably though, the GetOrderItems method shouldn't return anything that can be modified. I wish I could create it as an IEnumerable(Of BaseItem) instead, however, that would not work with your FilterForConfirmation method.

As I do not know how you intend to filter, I chose to keep the List(Of BaseItem). The filter also doesn't seem to filter, but rather seems to remove items from that list, so without knowing how you are using it, I chose to stick with List(Of BaseItem).


Advent of Code 2020 Day 9 in Python This is a solution for part 1 & 2 of Advent of Code 2020 Day 9.
Given the example input sample.txt:
35
20
15
25
47
40
62
55
65
95
102
117
150
182
127
219
299
277
309
576

Part 1: find the first number in the list (after the preamble) which is not the sum of two of the n numbers before it. n is the preamble.
For example, after the 5-number preamble, almost every number is the sum of two of the previous 5 numbers; the only number that does not follow this rule is 127.
Part 2: find a contiguous set of at least two numbers in your list which sum to the invalid number from part 1.
For example, in this list, adding up all of the numbers from 15 through 40 produces the invalid number from part 1, 127. Add together the smallest and largest number in this contiguous range; in this example, these are 15 and 47, producing 62.
Code:
def not_in_preamble(nums, preamble):
    diff = set()
    for i in range(preamble, len(nums)):
        current = nums[i]
        found = False
        for j in range(i - preamble, i):
            if nums[j] in diff:
                found = True
                break
            diff.add(current - nums[j])
        if not found:
            return current
        diff.clear()

def sub_array_with_sum(nums, n):
    i, j = 0, 1
    partial_sum = nums[i] + nums[j]
    while j < len(nums):
        if partial_sum == n:
            return nums[i: j + 1]
        if partial_sum < n or i == j - 1:
            j += 1
            partial_sum += nums[j]
        else:
            partial_sum -= nums[i]
            i += 1

# Part 1
preamble = 5
with open('sample.txt') as f:
    nums = [int(l) for l in f.read().splitlines()]
pt1_result = not_in_preamble(nums, preamble)
print(f'PT 1 result: {pt1_result}')

# Part 2
n = pt1_result
sub_array = sub_array_with_sum(nums, n)
print(f'PT 2 result {min(sub_array) + max(sub_array)}')

The code outputs:
PT 1 result: 127
PT 2 result 62

Which is the correct result for the sample input.
Changing the sample.txt to the puzzle input and setting preamble=25, will give the correct result for the challenge.
Can the code be made more compact and efficient? Any other feedback or alternative solutions are welcome.

Part 1
For not_in_preamble, I'd use a for-else instead of your found variable, and not re-use the same set (I don't think it's worth it):
def not_in_preamble(nums, preamble):
    for i in range(preamble, len(nums)):
        current = nums[i]
        diff = set()
        for j in range(i - preamble, i):
            if nums[j] in diff:
                break
            diff.add(current - nums[j])
        else:
            return current

Since your "Any other feedback or alternative solutions are welcome" also allows less efficient solutions, here's how I actually solved it myself :-)
from itertools import combinations

def not_in_preamble(nums, preamble):
    for i in range(preamble, len(nums)):
        if nums[i] not in map(sum, combinations(nums[i-preamble : i], 2)):
            return nums[i]

Preamble size 25 and nums size 1000 are easily small enough for that to run quickly.
Part 2
Your sub_array_with_sum is great, although it does rely on the input being non-negative numbers, which isn't guaranteed in the problem statement (but is the case for the fixed input file).
I'd just go with PEP 8 and change nums[i: j + 1] to nums[i : j + 1] or nums[i : j+1]. And perhaps I'd go with while True:, relying on the existence of an answer.
Your algorithm almost only takes O(1) space. The only reason it takes more is copying out the sub-array (or rather sub-list, as this is Python and the problem never says "array", so there's really no need to call it array). Could be done in O(1) space to make the algorithm not just time-optimal (O(n)) but also space-optimal. Though that would require more code, not worth it (unless you have an interviewer who insists).
Input
I'd change [int(l) for l in f.read().splitlines()] to list(map(int, f)), or at least [int(line) for line in f].
Output
I'd change
print(f'PT 1 result: {pt1_result}') to
print('PT 1 result:', pt1_result). Bit shorter, and it works in older versions of Python. And at least in IDLE, the f-string is styled only as string, the whole thing green, including the variable. I don't like that.


Ruby infixed math parser It only does expressions with 2 operands yet, but I'm wondering if there are any ways I can improve this:

# infix.rb: parse infix-operated math expressions

class String
  def is_number?
    true if Float(self) rescue false
  end
end

class InfixParser
  @operations = [ '/', '*', '+', '-' ]

  testString = '(24 + 6) / 10 * 3'
  shouldEqual = 9

  def parseExpression(expr) #TODO: implement parsing of entire expressions

  end

  def self.parseChunk(chunk)
    chunk = chunk.gsub ' ', ''

    if not (chunk.include?('/') or chunk.include?('*') \
        or chunk.include?('+') or chunk.include?('-'))
        puts "error: no operations in chunk: #{chunk}"
        exit
    end

    firstNumber = ''
    secondNumber = ''

    i = 0
    currentChar = ''
    while not @operations.include? currentChar
      currentChar = chunk[i]
      if not currentChar.is_number? and not @operations.include? currentChar \
         and currentChar != '.'
        puts "error: non-numerical digit in chunk: #{currentChar}"
        exit
      end
      firstNumber += currentChar
      i += 1
    end
    firstNumber = firstNumber[0 .. -2]

    for c in i-1 .. chunk.length-1
      if not chunk[c].is_number? and not @operations.include? currentChar \
         and currentChar != '.'
        puts "error: non-numerical digit in chunk: #{currentChar}"
        exit
      end
      secondNumber += chunk[c]
    end
    secondNumber = secondNumber[1 .. secondNumber.length - 1]

    if chunk[i - 1] == '/'
      return Float(firstNumber) / Float(secondNumber)
    elsif chunk[i - 1] == '*'
      return Float(firstNumber) * Float(secondNumber)
    elsif chunk[i - 1] == '+'
      return Float(firstNumber) + Float(secondNumber)
    elsif chunk[i - 1] == '-'
      return Float(firstNumber) - Float(secondNumber)
    else
      puts "error: invalid operator in chunk: #{chunk}"
    end

  end

end

Your parseChunk function is not really parsing an expression — it is also evaluating it.  Its naming style is also inconsistent with is_number?.  I think that eval_binary_expr would be a more appropriate name.

Augmenting the String class with an is_number? method is something to avoid doing.  That kind of patching increases the chances of breakage when your code coexists with other code in the same program.

In functions that are meant to be used by other code, calling exit is not an appropriate way to handle errors.  You should raise exceptions instead, so that the caller can handle the error in whatever way it chooses.

You appear to be working much too hard to parse the binary expression.  I think that this would accomplish more or less the same thing:

def eval_binary_expr(expr)
  l_operand, op, r_operand = expr.partition(%r{[/*+-]})

  if op.empty?
    raise ArgumentError, "Invalid operation or no operation in expression: #{expr}"
  end
  l_operand, r_operand = Float(l_operand), Float(r_operand)

  case op
    when '/' then l_operand / r_operand
    when '*' then l_operand * r_operand
    when '+' then l_operand + r_operand
    when '-' then l_operand - r_operand
  end
end

The solution above makes no attempt to handle negative numbers correctly.  As with your original solution, it fails when the first operand is negative.


Text-based adventure game I'm new to programming, and I would like to know if this is the right way to layout code. If anyone has any tips, please share them.

import random
import time

def displayIntro():
    print("You are in a land full of dragons. In front of")
    print("you are two caves. In one cave the dragon is")
    print("friendly and will give you his treasure. In the")
    print("other, the dragon is greedy and hungry and will")
    print("eat you on sight")
    print()

def chooseCave():
    cave = ""
    while cave != "1" and cave != "2":
        print("Which cave will you choose? (1 or 2)")
        cave = input()
   return cave

def checkCave(chosenCave):
    print("You approach the cave...")
    time.sleep(2)
    print("It is dark and spooky...")
    time.sleep(2)
    print("A large dragon jumps out in front of you. He opens his jaws and...")
    print()
    time.sleep(2)

    friendlyCave = random.randint(1,2)

    if chosenCave == str(friendlyCave):
        print("gives you his treasure")
    else:
        print("gobbles you down in one bite")

playAgain = "yes"
while playAgain =="yes" or playAgain == "y":

    displayIntro()

    caveNumber = chooseCave()

    checkCave(caveNumber)

    print("Would you like to play again? (yes or no)")
    playAgain = input()

(1) For variable and function names, check_cave (underscore) rather than checkCave (camelcase) are standard in python.

(2) The line def chooseCave needs to be unindented

(3) Your main game loop could probably be neatened up:

while True:
    display_intro()
    check_cave(choose_cave())
    print("Would you like to play again? (yes or no)")
    if input() not in ('yes', 'y'):
        break

(4) conventionally you should put the main game loop in a function called main() and add

if __name__ == "__main__":
    main()

at the end.

(5) More a matter of taste, but I'd find it more elegant to write choose_cave like this:

def choose_cave():
    while True:
        print("Which cave will you choose? (1 or 2)")
        cave = input()
        if cave in ('1', '2'):
            return cave


Simple tkinter application that choose a random string I am really new to programming, and I made this simple tkinter app in
python. Please tell me what can be improved and how to use classes
correctly in combination with tkinter.

Github to clone if you want

#!/usr/bin/env python

# ---------------------------------
# @author: apoc
# @version: 0.1
# ---------------------------------

# importing
from tkinter import *
import csv
from random import randint

class LRG(object):

    def __init__(self,master):

        # variables
        with open('data/champs.csv','r') as f:
            reader = csv.reader(f)
            self.champ_list = list(reader)

        # layout
        self.randombutton = Button(master,text='Random',command=self.scan)
        self.infofield = Text(master,height=20,width=50)

        # layout
        self.randombutton.grid(row=0,column=1,sticky=E,pady=2,padx=5)
        self.infofield.grid(row=1,columnspan=4,sticky=W,column=0,pady=4)

    def scan(self):

        self.infofield.delete('1.0',END)
        self.infofield.insert(END,self.champ_list[
            randint(0,len(self.champ_list))
            ])

if __name__ == "__main__":
    master = Tk()
    LRG(master)
    master.title("LRG")
    master.mainloop()

Standards

Follow the PEP8 coding standard.  One violation that immediately is obvious is the lack of a space following a comma.  For example, (row=0, column=1, sticky=E, pady=2, padx=5) is much easier to read.  Use pylint (or similar) to check your code style against the recommended standard.

Import grouping

My personal preference is to keep import ... statements together, and from ... import ... statements together, not interleave them.

Private class members

"Private" members should be prefixed with an underscore.  Such as self._champ_list and def _scan(self):.  This doesn't actually make them private, but some tools will use the leading underscore as a key to skip generating documentation, not offer the member name in autocomplete, etc.

Remove useless members

self.randombutton is only used in the constructor; it could just be a local variable, instead of a member.

Use correct comments

The # layout comment above the Button and Text construction is misleading; it is not doing any layout.

Self documenting names

Use understandable names.  I have no idea what LRG is.  Good variable names go a long way towards creating self-documenting code.

Use Doc-Strings

Add """doc-strings""" for files, public classes, public members, and public functions.  Using LRG as a class name could be fine (in may be a common acronym at your company), but adding a doc-string for the class could spell out what the acronym stands for at least once.

Avoid hard-coded information

You have hard-coded reading the data/champs.csv file into the constructor.  Consider making it more flexible, such as passing in a filename with that as a default.

class LRG:
    """Literal Random Generator"""

    def __init__(self, master, csv_filename="data/champs.csv"):
        # ...etc...

Or, move the reading of the data outside of the class, and pass the champ_list in as a constructor argument.


A doubly linked list implementation using C++ smart pointers I am new to "modern" C++ and decided to learn about smart pointers by implementing a doubly linked list.

I have a few problems with this code: Should the tail pointer be a weak pointer? Should the head pointer be a unique pointer (If yes, then the node after head will have a weak_ptr to the head node, causing a unique_ptr and a weak pointer pointing to the same object)?

#include <iostream>
#include <memory>

template <typename T>
class LinkedList {
 public:
  LinkedList();
  int GetLength() const;
  void Print() const;
  // TODO: Implement and return an iterator.
  void Append(T data);
  // TODO: Implement and return an iterator.
  void Prepend(T data);
  bool Delete(T data);

 private:
  struct ListNode {
    T data;
    std::shared_ptr<ListNode> next;
    std::weak_ptr<ListNode> prev;

    ListNode(T data) : data(data) {}
  };
  int length;
  std::shared_ptr<ListNode> head;
  std::shared_ptr<ListNode> tail;

  void Delete(std::shared_ptr<ListNode> ptr);
};

template <typename T>
LinkedList<T>::LinkedList()
    : length(0) {}

template <typename T>
int LinkedList<T>::GetLength() const {
  return length;
}

template <typename T>
void LinkedList<T>::Print() const {
  std::shared_ptr<ListNode> h(head);
  std::cout << "List (length = " << length << "): ";
  while (h) {
    std::cout << h->data << " ";
    h = h->next;
  }
  std::cout << std::endl;
}

template <typename T>
void LinkedList<T>::Append(T data) {
  std::shared_ptr<ListNode> node_ptr(std::make_shared<ListNode>(data));
  node_ptr->prev = tail;

  if (tail) {
    tail->next = node_ptr;
    tail = std::move(node_ptr);
  } else {
    tail = std::move(node_ptr);
    head = tail;
  }

  length++;
}

template <typename T>
void LinkedList<T>::Prepend(T data) {
  std::shared_ptr<ListNode> node_ptr(std::make_shared<ListNode>(data));
  node_ptr->next = head;

  if (head) {
    head->prev = node_ptr;
    head = std::move(node_ptr);
  } else {
    head = std::move(node_ptr);
    tail = head;
  }

  length++;
}

template <typename T>
bool LinkedList<T>::Delete(T data) {
  std::shared_ptr<ListNode> ptr = head;
  while (ptr) {
    if (ptr->data == data) {
      Delete(ptr);
      return true;
    }

    ptr = ptr->next;
  }

  return false;
}

template <typename T>
void LinkedList<T>::Delete(std::shared_ptr<ListNode> ptr) {
  auto shared_prev = (ptr->prev).lock();

  if (head == ptr) {
    head = ptr->next;
  }
  if (tail == ptr) {
    tail = shared_prev;
  }

  if (shared_prev) {
    shared_prev->next = ptr->next;
  }
  if (ptr->next) {
    ptr->next->prev = ptr->prev;
  }

  length--;
}

This review will focus on ListNode.
ListNode
The constructor takes its argument by copy, whether the argument is a char or a 1'000'000 element std::vector<>. Additionally, there is no exception specification.
Before:
ListNode(T data) : data(data) {}
//                 ^^^^^^^^^^ another copy is made here
//       ^^^^^^ a copy is made here

In order to prevent the first copy, you should pass it as T const&.
Use type traits to add an exception specification.

After:
#include <type_traits> // for the std::is_nothrow_xxx family of template traits

ListNode(T const& data) // the argument is now taken by const&
noexcept(std::is_nothrow_copy_constructible<T>::value) // correct exception specification
    : data(data) // now, only one copy is made: here.
{}

Additional remarks:

Consider adding a constructor that takes a T&& for efficiency.
Consider having only one constructor that directly initializes data by using a variadic template and perfect forwarding.

The std::unique_ptr<> and std::shared_ptr<> types model ownership semantics. Meaning that the smart pointer instance itself owns the memory it points to. In a linked list, the list owns the nodes and their values.
Currently, when ListNode's destructor is called, it will start a recursive chain of calls: freeing a node requires freeing its next data member, which requires freeing the next data member's next data member, and so on.
This is inefficient and limits the number of elements that your list can contain to the recursive limit that your stack allows; it is reason enough to remove ownership semantics by using raw pointers over std::shared_ptr<>.
To deal with this change, you simply define a destructor for LinkedList:
~LinkedList() noexcept(std::is_nothrow_destructible<T>::value)
{
    for (ListNode* current = head, *next; current;)
    {
        next = current->next;
        delete current;
        current = next;
    }
}

Note that this will require you to provide the proper move and copy operations for your LinkedList type by hand as well.

From looking at your declaration (not your implementation), here's some points for:
LinkedList<T>

Provide a size_type alias for your container. Assuming I want to fill the linked list up to its theoretical maximum size, I need to know how many elements it can support. This is determined by the maximum value of the length data type. The only way to guess at its type is by checking GetLength()'s return type.
The Append(), Prepend() and Delete() member functions all take their arguments by copy just like ListNode's old constructor. Consider similar fixes without forgetting to adapt the exception specification according to the operations you do.
Your data members comply with the requirements needed to mark your default constructor as constexpr and as noexcept.
Printing should not be a member function. Instead, provide operator<< as a free function.
As Loki Astari mentioned, consider using sentinel nodes in order to avoid having to check for nullptr values.
Provide an exception specification for non-throwing functions such as GetLength().


GitHub repo tree generator I have built a generator that, using the GitHub API, creates a dictionary containing a tree of all the resources in any GitHub repo. It uses the function git_tree, which takes one argument, a repo-string (repo-author/repo-name). For example, this repo only contains one file, a README. If we pass in the repo-string, and format the end result, this is what we get:

>> json.dumps(git_tree("githubtraining/github-move"), indent=3)
{
   "github-move": {
      "files": [
         "README.md"
      ],
      "dirs": {}
   }
}

You can see that it returns a dict with a key/value pair of {repo_name: tree}. So the github_move item contains a list of all files in that directory, and a dict with more nested directories. Obviously, in this repository there aren't any other directories, so that dict is just blank.

For sake of purpose, here is the tree of this repo (it was too long to put in the post). You can see each directory and subdirectory has its own files list and dirs dict.

Here's the code (repl.it online program for testing):

import requests
from pprint import pprint
from functools import reduce
import operator
import json
from itertools import chain, repeat, islice

class GitError(Exception): pass

def intersperse(delimiter, seq):
  return list(islice(chain.from_iterable(zip(repeat(delimiter), seq)), 1, None))

def _get_from_dict(dataDict, mapList):
  return reduce(operator.getitem, mapList, dataDict)

def _append_in_dict(dataDict, mapList, value):
  _get_from_dict(dataDict, mapList[:-1]).append(value)

def _get_sha(author, repo):
  try:
    return requests.get('https://api.github.com/repos/{}/{}/branches/master'.format(author, repo)).json()['commit']['commit']['tree']['sha']
  except KeyError as ex:
    raise GitError("Invalid author or repo name") from ex

def _get_git_tree(author, repo):
  return requests.get("https://api.github.com/repos/{}/{}/git/trees/{}?recursive=1".format(author, repo, _get_sha(author, repo))).json()["tree"]

def git_tree(repostring):
  author, repo = repostring.split("/")
  tree = {repo: {"files": [], "dirs": {}}}
  for token in _get_git_tree(author, repo):
    if token["type"] == "tree" and "/" not in token["path"]:
      tree[repo]["dirs"].update({token["path"]: {}})
      tree[repo]["dirs"][token["path"]].update({"files": [], "dirs": {}})
    elif token["type"] == "tree" and "/" in token["path"]:
      temp_dict = {}
      a = list(reversed(token["path"].split("/")))
      for k in a[:-1]:
        temp_dict = {k: {"files": [], "dirs": temp_dict}}
      tree[repo]["dirs"][a[-1]]["dirs"] = temp_dict
    elif token["type"] == "blob":
      path = token["path"].split("/")
      if len(path) == 1:
        tree[repo]["files"].append(path[0])
      else:
        dict_path = [repo, "dirs"] + intersperse("dirs", path[:-1]) + ["files", path[-1]]
        _append_in_dict(tree, dict_path, dict_path[-1])
  return tree

print(json.dumps(git_tree("githubtraining/caption-this"), indent=3))

(The json.dumps is just there for easy viewing, it can be ommited).

My questions:

Is it too messy? I look back at this function and it looks a bit cluttered and all over the place. Is that the case, or am I just going crazy?
Do I have any unnecessary code in there?
Is there anything else you deem wrong with the program?

I see nothing wrong with the code itself, but you're in dire need of docstrings. Arcane functional one-liners like the one in intersperse are impenetrable unless they're well-documented. You'd also benefit from splitting that one into multiple lines.

For strings like this:

    return requests.get('https://api.github.com/repos/{}/{}/branches/master'.format(author, repo)).json()['commit']['commit']['tree']['sha']

consider rewriting your format call as an f-string; i.e.

f'https://api.github.com/repos/{author}/{repo}/branches/master'


A flexible menu function in python This is a question about some code I've written that works, but I'm not sure it's good code.

I would like to get some comments on this or if possible hints to build a better solution.

Background:

This is a small program to calculate some data, connect these to a participant and store it in a JSON file.

Here the already working solution:

menu_entries = (
                    ('Order Pizza', order_pizza),
                    ('Order Burger', order_burger),
                    ('Order Coke', order_coke, '0.5')
               )

def menu(menu_title, menu_entries):

    while True:
        mpoint = 0

        print(menu_title)

        # Build the menu entries plus one to close
        for entry in menu_entries:
            mpoint += 1
            print(str(mpoint) + '.', entry[0])

        print(str(mpoint + 1) + '.', 'Ende')

        # Get value and validate
        try:
            task = int(input('> '))
        except ValueError:
            task = None

        if task is None or task > mpoint + 1:
            task = None
            continue

        if task == mpoint + 1:
            print('Done')
            break

        # Build a function call
        get_task = menu_entries[task - 1][1]
        try:
            get_task(menu_entries[task - 1][2])
        except IndexError:
            get_task()

def order_pizza(size=None):
    if size is None:
        size = ''
    print('Bring me a Pizza!')

def order_burger(size=None):
    if size is None:
        size = ''
    print('One {} Burger please.'.format(size))

def order_coke(size=None):
    if size is None:
        size = ''
    print('A {} Coke please.'.format(size))

menu('Main menu', menu_entries)

As you can see the menu function gets a main title and a tuple of menu entries.

In these menu entries I could define the menu point title, a function name and one parameter.

All these other functions are only examples and are of no interest (they are ugly, I know).

Please have a look, criticize and if possible show me a better solution.

You have an actual error in your code:

$ python test.py
Traceback (most recent call last):
  File "test.py", line 2, in <module>
    ('Order Pizza', order_pizza),
NameError: name 'order_pizza' is not defined

You need to define menu_entries after creating the things it refers to.

Some notes:

All Python code could benefit from being run through at least one linter such as flake8 or pycodestyle. Nothing springs out at me except possibly the indentation of menu_entries, but it's worth a check.
Generally, names don't need to repeat anything which is understood from context. So def menu(menu_title, menu_entries): could be easier written as def menu(title, entries):
You always add one to mpoint - a cleaner solution would be to mpoint = 1 and continue from there. If mpoint was used for indexing into an array that would be different, but it's really only for presentation.
When reading the code top to bottom I should be able to understand what a name means the first time I encounter it. mpoint should be renamed to what it is (maybe menu_index?).
For consistency (and to write more idiomatic code) the strings in all your print statements should be formatted.
This is a prime example for code which could be made object oriented - obvious classes include Menu, MenuEntry and Order.
Setting task = None and then checking if it's None to check whether you should continue is redundant - you can just continue immediately.
size is unused in order_pizza, so it should be removed.


Drawing an Archimedean spiral using Pillow From Rosetta Code:

  The Archimedean spiral is a spiral named after the Greek mathematician Archimedes. It can be described by the equation:
  $$r=a+b\theta$$
  with real numbers \$a\$ and \$b\$.

Here is my attempt to draw it in Python (using Pillow):

"""This module creates an Archimdean Spiral."""

from math import cos, sin, pi
from PIL import Image, ImageDraw

def translate(point, screen_size):
    """
    Takes a point and converts it to the appropriate coordinate system.
    Note that PIL uses upper left as 0, we want the center.
    Args:
        point (real, real): A point in space.
        screen_size (int): Size of an N x N screen.
    Returns:
        (real, real): Translated point for Pillow coordinate system.
    """
    return point[0] + screen_size / 2, point[1] + screen_size / 2

def draw_spiral(a, b, img, step=0.5, loops=5):
    """
    Draw the Archimdean spiral defined by:
    r = a + b*theta
    Args:
        a (real): First parameter
        b (real): Second parameter
        img (Image): Image to write spiral to.
        step (real): How much theta should increment by. (default: 0.5)
        loops (int): How many times theta should loop around. (default: 5)
    """
    draw = ImageDraw.Draw(img)
    theta = 0.0
    r = a
    prev_x = int(r*cos(theta))
    prev_y = int(r*sin(theta))
    while theta < 2 * loops * pi:
        theta += step
        r = a + b*theta
        # Draw pixels, but remember to convert to Cartesian:
        x = int(r*cos(theta))
        y = int(r*sin(theta))
        draw.line(translate((prev_x, prev_y), img.size[0]) +
                  translate((x, y), img.size[0]), fill=1)
        prev_x = x
        prev_y = y

if __name__ == '__main__':
    IMAGE_SIZE = 300, 300
    img = Image.new('1', IMAGE_SIZE)
    draw_spiral(1, 2, img)
    img.save('spiral.png')

The program outputs this image:

With the default values, the spiral does not look very smooth: it's easy to see that it's been drawn using a series of straight-line segments. That's because the step in \$\theta\$ is quite large — 0.5 radians is more than 28°. Setting step=0.1 and loops=10 is an improvement:

But you can still see deviations from smoothness in various places. The problem here is that the step in \$\theta\$ is constant, but as \$r\$ gets bigger this becomes a bigger step in actual distance. What we would like is a (roughly) constant step in terms of distance along the curve, which we get by replacing

theta += step

with

theta += step / r

This makes step into an approximate distance along the curve in pixels (rather than a change in angle, as in the original code). Now, with step=1 and loops=10, we get this spiral:


Safe Reverse Polish Notation I implemented the following (what I believe to be) safe Reverse Polish Calculator.

Now I'm using foldM to take advantage of the power of Monads. Previously, I was not.

Can you please critique my code?

import Data.Maybe
import Text.Regex
import Control.Monad
import Network.CGI.Protocol

data Operator = Add | Subtract | Multiply

-- safe Reverse Polish Notation calculator
rpn :: String -> Maybe Double
rpn xs = maybeStringToMaybeDouble result :: Maybe Double
      where result = safeHead $ foldM foldingOp [] $ words xs

--foldM :: Monad m => (a -> b -> m a) -> a -> [b] -> m a

maybeStringToMaybeDouble :: Maybe String -> Maybe Double
maybeStringToMaybeDouble x = do
      y <- x
      maybeRead y

-- extract maybeHead of Maybe [a]
safeHead :: Maybe [a] -> Maybe a
safeHead m = do
    x <- m -- x has type [a]
    listToMaybe x

foldingOp :: [String] -> String -> Maybe [String]
foldingOp acc s
   | isNumber s        = Just (s : acc)
   | isPlusRegex s     = performOp Add acc
   | isMinusRegex s    = performOp Subtract acc
   | isMultiplyRegex s = performOp Multiply acc
   | otherwise         = Nothing

performOp :: Operator -> [String] -> Maybe [String]
performOp o (x:y:zs) = Just $ (doOp o x y) : zs
performOp _ _        = Nothing

doOp :: Operator -> String -> String -> String
doOp Add a b       = show $ (read b :: Double) + (read a :: Double)
doOp Subtract a b  = show $ (read b :: Double) - (read a :: Double)
doOp Multiply a b  = show $ (read b :: Double) * (read a :: Double)

isNumber :: String -> Bool
isNumber = matches' mkNumberRegex

mkNumberRegex :: Regex
mkNumberRegex = mkRegex "^[0-9]+[.]?[0-9]*$"

isPlusRegex :: String -> Bool
isPlusRegex = matches' mkPlusRegex

isMinusRegex :: String -> Bool
isMinusRegex = matches' mkMinusRegex

isMultiplyRegex :: String -> Bool
isMultiplyRegex = matches' mkMultiplyRegex

mkPlusRegex :: Regex
mkPlusRegex = mkRegex "^[+]$"

mkMinusRegex :: Regex
mkMinusRegex = mkRegex "^[-]$"

mkMultiplyRegex :: Regex
mkMultiplyRegex = mkRegex "^[*]$"

matches' :: Regex -> String -> Bool
matches' r x
   | matchRegex r x == Nothing = False
   | otherwise                 = True

This:

maybeStringToMaybeDouble x = do
      y <- x
      maybeRead y

Could be:

maybeStringToMaybeDouble x = x >>= maybeRead

Similarly,

safeHead m = do
    x <- m
    listToMaybe x

Could be

safeHead m = m >>= listToMaybe

Or we could remove these functions entirely and just write

rpn xs = foldM foldingOp [] (words xs) >>= listToMaybe >>= maybeRead

It seems strange that we're importing Network.CGI.Protocol for maybeRead. I would suggest using readMaybe from Text.Read instead.

There's a chance to use isJust here:

matches' :: Regex -> String -> Bool
matches' r x
   | matchRegex r x == Nothing = False
   | otherwise                 = True

matches' :: Regex -> String -> Bool
matches' r x = isJust $ matchRegex r x

We can do away with six functions by rewriting foldingOp like this:

foldingOp :: [String] -> String -> Maybe [String]
foldingOp acc s
   | isNumber s = Just (s : acc)
   | s == "+"   = performOp Add acc
   | s == "-"   = performOp Subtract acc
   | s == "*"   = performOp Multiply acc
   | otherwise  = Nothing

And we can simplify further by removing the Operator type:

foldingOp :: [String] -> String -> Maybe [String]
foldingOp acc s
   | isNumber s = Just (s : acc)
   | s == "+"   = performOp (+) acc
   | s == "-"   = performOp (-) acc
   | s == "*"   = performOp (*) acc
   | otherwise  = Nothing

performOp :: (Double -> Double -> Double) -> [String] -> Maybe [String]
performOp o (x:y:zs) = Just $ doOp o x y : zs
performOp _ _        = Nothing

doOp :: (Double -> Double -> Double) -> String -> String -> String
doOp op a b = show $ (read b :: Double) `op` (read a :: Double)


Return path between graph nodes using depth-first search This code is meant to implement a Graph class which has a method that returns a path between 2 nodes using depth-first search.

I'm doing this to improve my style and to improve my knowledge of fundamental algorithms/data structures for an upcoming coding interview.

from queue import PriorityQueue
from copy import deepcopy

class Graph:
    def __init__(self):
        self.verts = set()

    def add_vert(self, id):
        new_vert = Vertex(id)
        self.verts.add(new_vert)
        return new_vert

    def get_vert(self, id):
        for vert in self.verts:
            if vert.id == id:
                return vert
        return None

    def dfs(self, start, end):
        path_stack = []
        visited = set()
        path_stack.append([start])

        while path_stack:
            cur_path = path_stack.pop()
            last = cur_path[-1]
            visited.add(last)

            if last != end:
                path_stack = self.push_paths_in_order(path_stack, cur_path, visited)
            else:
                return cur_path
        return None

    def push_paths_in_order(self, path_stack, cur_path, visited):
        last = cur_path[-1]
        queue = PriorityQueue()

        for neighbour in last.neighbours:
            if neighbour not in visited:
                    # make queue like max heap so that we can push
                    # paths to path_stack in increasing order
                    dist_to_neighbour = last.neighbours[neighbour] * -1
                    queue.put((dist_to_neighbour, neighbour))

        while not queue.empty():
            new_path = deepcopy(cur_path)
            next_vert = queue.get(0)[1]
            new_path.append(next_vert)
            path_stack.append(new_path)
        return path_stack

class Vertex:
    def __init__(self, id):
        self.id = id
        self.neighbours = dict()

    def add_neighbour(self, to, weight):
        self.neighbours[to] = weight

The algorithm you've implemented is not a depth-first search. I'm not sure what it does exactly. It uses a priority queue for some reason (a depth-first search doesn't need a priority queue). It also stores the entire path in the stack and copies every time it discovers a new vertex, which makes it time and space inefficient (your's implementation needs O(n^2) space and time even on a sparse graph (for instance, a tree), while a correct implementation is always linear in number of vertices and edges).
A standard depth-first search goes like this:

stack = [start_vertex]
parent = empty dict
seen = set(start_vertex)
while not stack.empty():
     v = stack.pop()
     for u in neighbors of v:
          if not u in seen:
              seen.add(u)
              stack.push(u)
              parent[u] = v

The code is much simpler and more efficient. To find the path itself, we can just iterate starting from the destination vertex and going to the current vertex's parent until we reach the start vertex. This way, it requires a linear amount of time and space.
The code itself is rather obscure, in my opinion. Why do we multiply something by -1 here: last.neighbors[neighbor] * -1 (probably you do it to "reverse" the order, but it's not that clear when you read it for the first time)? The fact that neightbors[v] is actually a weight of the edge is also confusing (I would expect to see something like distance or weight in the name of the variable if it stores the length of the edge). By the way, depth-first search works with unweighted graph, so I don't see the point of storing the weights, anyway.


Binary reader and writer I recently implemented a binary reader/writer for my multiplayer game.

Now reading bytes looks like this:

static int32_t readInt32(std::vector<uint8_t>& msg, int *off)
{
    if ((*off) > msg.size()) return 0;
    int32_t result; int size = sizeof (result);
    memcpy(&result, msg.data() + (*off), size); (*off) += size;
    return result;
}

And writing bytes looks like this:

static void writeInt32(std::vector<uint8_t>* msg, int32_t value)
{
    uint8_t const * array = reinterpret_cast<uint8_t const *>(&value);

    for (std::size_t i = 0; i != sizeof(value); ++i)
    {
        (*msg).push_back(array[i]);
    }
}

How can I improve this?

I see a number of things that could help you improve your program.

Fix your formatting

Crowding multiple statements on a single line makes your program harder to read and understand.  So instead of this:

if ((*off) > msg.size()) return 0;
int32_t result; int size = sizeof (result);

Write this:

if ((*off) > msg.size()) {
    return 0;
}
int32_t result;
int size = sizeof (result);

Use the required #includes

The code uses std::vector which means that it should #include <vector>.  It was not difficult to infer, but it helps reviewers if the code is complete.  For this code, it appears that you need these:

#include <vector>
#include <cstring>
#include <cstdint>

Use C++-style includes

Instead of including string.h you should instead use #include <cstring>.  The difference is in namespaces as you can read about in this question. I infer that the code used that because the call to memcpy was not std::memcpy.

Prefer passing a reference to a raw pointer

The writeInt32() function takes a pointer to a vector, but what if the pointer is nullptr?  Clearly the function will fail, and probably crash the program if that happens.  Better, then, would be to pass a reference instead, which says that it really MUST be a std::vector<std::uint8_t> and nothing else.

Avoid portability problems

The endianness of computers is not fixed by the C++ language standard, so your memory copy or memory casting is not going to work the same way on all machines.  This portability problem can be solved by doing one of two things: either rely only on guarantees that are actually made by the standard or use conversion routines to put everything in the same order.  This often comes up in network communications, so some platforms have htonl() and friends to convert from "host order" which might be big-endian or little-endian, to network order which is always big-endian.  Note, however, that this has its own portability issues because those functions are not part of the C++ standard.  So the other way to do it is to do it yourself.

static void writeInt32(std::vector<std::uint8_t>& msg, std::int32_t value)
{
    for (auto i = sizeof(value); i; --i, value >>= 8) {
        msg.push_back(value & 0xff);
    }
}

This is shorter and also portable.  In this version, i is declared to be auto but declaring it to be of type std::size_t, as you had it, is another way to do it and even works if you're not using a C++11 compiler.

Be careful with signed and unsigned

In the readInt32 routine, the code compares an int to a size_t, but size_t is unsigned and int is signed.  In this case, your off variable should probably be declared as being of std::size_t * type.

Don't fail silently

In the readInt32 function, a returned value of 0 might mean that a value of zero was actually read or it might mean that the offset pointer was off the end of the message.  Unfortunately with the current code there's no way to differentiate these cases.  I'd suggest throwing an exception instead.

Be careful with memcpy

The code currently catches the case in which the offset is beyond the end of the vector, but what if it's pointing instead to the last byte of the vector?  In this case, memcpy will copy that last byte and whatever happens to be in memory beyond that.  This is not likely to be a useful value.  In this case, I'd recommend not using memcpy at all and rewriting the code to be portable as mentioned above.

Reconsider the interface

Right now, the code consists of two functions which deal with writing/reading a std::int32_t value to/from a message.  If your code frequently does things with such messages, it might be better to create a Message class and have member functions that read and write particular types of values, such as your std::int32_t values used here.  I'd also recommend that raw pointers such as off not be part of the interface at all.


Python vs Java performace: brute force equation solver I wrote a program in Python and in Java to search for the smallest integer solution of the equation:

$$a^5+b^5+c^5+d^5=e^5$$

(expected output is \$133^5 + 110^5 + 84^5 + 27^5 = 144^5\$)

Powers and roots are either computed directly ("direct calculation" method) or computed and stored in an array ("power lookup" method). Fifth powers are looked up like n5 = fifth_power[n]. Fifth power root is computed using a binary search in array 'fifth_power`.

I am running it on NetBeans if it matters. It takes:

30 s (Python, direct)
20 s (Python, lookup)
5.6 s (Java, direct)
0.8 s (Java, lookup)

Is there a way to boost Python performance? I am not looking for better math (sieving of some kind). I am looking for better implementation of "for each combination of a,b,c,d compute some of their powers, check if the sum is a perfect power. If it is - print the result".

Is it expected that Python runs some 20 times slower than Java?

Python 3.5

from array import *
import math
import time

#PYTHON, BRUTEFORCE : ~30 s
millis1 = int(round(time.time() * 1000))
keep_searching = True
a=1
result=""
while(keep_searching):
    a+=1
    for b in range(1,a+1):
        for c in range(1,b+1):
            for d in range(1,c+1):
                sum=math.pow(a,5)+math.pow(b,5)+math.pow(c,5)+math.pow(d,5)
                root = math.pow(sum,0.2)
                e = round(root)

                e5 = math.pow(e,5)

                if(e5==sum):
                    result="{}^5 + {}^5 + {}^5 + {}^5 = {}^5".format(int(a),int(b), int(c),int(d), int(e))
                    keep_searching = False
                    millis2 = int(round(time.time() * 1000))

print(result)
print("Found solution in {} ms".format(millis2-millis1))

#PYTHON, PRECOMPUTE POWERS: ~20 s
millis3 = int(round(time.time() * 1000))
#fifth_power #175 is enough
size=176
fifth_power = [None] * size
for i in range(size):
    fifth_power[i]=long(math.pow(i,5))

millis4 = int(round(time.time() * 1000))

#returns  value if it is a perfect power (32 returns 2)
#returns -1 if between perfect powers, -2 if greater than max value in array, -3 if smaller than min value in array

def check_perfect_power(number, min, max, fifth_power):

    current=int((min+max)/2)
    while(max>=min):
        if(number==fifth_power[current]):
            return current
        elif(number>fifth_power[current]):
            min=current+1
            current=int((max+min)/2)
        else:
            max=current-1
            current=int((max+min)/2)

    if(min>=len(fifth_power)):
        return -2
    if(max<0):
        return -3

    return -1

keep_searching = True
a=0
result=""
while(keep_searching):
    a+=1
    for b in range(1,a+1):
        for c in range(1,b+1):
            for d in range(1,c+1):
                mymax=min(int(a*1.32)+1, size-1)
                e=check_perfect_power(fifth_power[a]+fifth_power[b]+fifth_power[c]+fifth_power[d], a, mymax, fifth_power)
                if(e>0):
                    result="{}^5 + {}^5 + {}^5 + {}^5 = {}^5".format(int(a),int(b), int(c),int(d), int(e))
                    keep_searching = False
                    millis5 = int(round(time.time() * 1000))

print(result)
print("Populated in {} ms, find solution in {} ms".format(millis4-millis3,millis5-millis4))

An obvious possible improvement for both languages is storing the values to the fifth power (But I'm going to focus on python here).

In python, while is a keyword and does not need parenthesis. Also, global lookup of methods is slower than local  lookup, so I would define pow = math.pow

pow = math.pow
while keep_searching:
    a += 1
    a5 = pow(a, 5)
    for b in range(1, a+1):
        b5 = pow(b, 5)
        for c in range(1, b+1):
            sum_begin = a5 + b5 + pow(c, 5)
            for d in range(1, c+1):
                sum = sum_begin + pow(d, 5)
                ...

Also note that python has an official style-guide, PEP8, which recommends putting spaces around operators and after commas in argument lists (I applied those rules in the code above).

Note that str.format does not care what it's parameters are. It will just call str on them and use that. So no need to call int on your parameters (they are already ints anyway).


Checking if a random number exist within an interval efficiently I have a problem where you have a bunch of probabilities associated with a number.

In this case:

any number between 0 - 0.3 should return 0,
any number between 0.3 - 0.7 should return 1,
any number between 0.7 - 1.0 should return 2

Is there a better way I can achieve this?

import random

class ProbChecker(object):

    def __init__(self, nums, prob):
        self._nums = nums
        self._prob = prob

    def next_val(self):
        if sum(self._prob) != 1.0:
            raise ValueError("Probabilities does not sum up to 1.0")

        accumulate = [sum(self._prob[: idx+1]) for idx in range(len(self._prob))]
        randomVal = random.random()

        for idx, x in enumerate(accumulate):
            if randomVal < x :
                return randomVal, self._nums[idx]

if  __name__ =='__main__':
    random_nums = [0, 1, 2]
    probabilities = [0.3, 0.4, 0.3]
    gen = ProbChecker(random_nums, probabilities)
    for i in xrange(100):
        print gen.next_val()

# EXAMPLE OUTPUT
#(0.9771170760903612, 2)
#(0.738653579103415, 2)
#(0.33448331523062824, 1)
#(0.6458688675258738, 1)
#(0.7062504910283919, 2)
#(0.4159743894291702, 1)
#(0.2636825176117218, 0)

What you are basically asking is for a way to generate values with different weights.

In this case, you want 0 to be generated 30% of the time, 1 to be generated 40% of the time and 2 to be generated 30% of the time. Rather than checking intervals, it is much easier to generate a sequence that has elements that correspond to the probabilities.

For example, we could replicate the probabilities needed by having a 10-element list like so: [0, 0, 0, 1, 1, 1, 1, 2, 2, 2] and then pick 1 random element from it. How did I get 10? 10 is the minimal number of elements that evenly divides 30, 40 and 30 (the probabilities) multiplied by the sum of the weights (30 + 40 + 30)/10 = 10.

Another example is if we wanted 0 to occur 1% of the time and 1 to occur 99% of the time, then we could pick one element from [0, 1, ..., 1] where there are 99 1's. The 100 elements comes from (1 + 99 (sum of weights))/ 1 (divisor, or gcd to be precise) = 100.

Now assuming that all your probabilities are integral percentages (1%, 2%, ... 100%). Then you can always use 100 elements. Then for 5% you repeat the element 5x, 12x for 12% and so on.

Now let's do some code:

Lets say you build something like this: weighted_choices = [(0, 30), (1, 40), (2, 30)], where the 0, 1, 2 are the values and 30, 40, 30 are the percentage weights.

Then the code to select a item based on the weight is like so:

sample = [val for val, cnt in choices for i in range(cnt)]
random.choice(sample)

Note: This is code is taken straight from the random module documentation

If you want a simple, but slower version:

sample = []
for value, weight in choices:
    for i in range(0, weight):
        sample.append(value)
random.choice(sample)

If you want to read up on an even more general variation, go look at random's documentation


How can I better test whether the player won in my tic-tac-toe? I created a tic-tac-toe using java and I would like to know if there are any practices that I can improve and make the code cleaner and more performative
import java.util.Scanner;
public class Main {
    public static boolean test(String[] values){
        //Check diagonals
        if((values[0].equals("X") && values[4].equals("X") && values[8].equals("X")) || (values[0].equals("0") && values[4].equals("0") && values[8].equals("0"))){
            System.out.print(values[0] + " win");
            return true;
        }
        if((values[2].equals("X") && values[4].equals("X") && values[6].equals("X")) || (values[2].equals("0") && values[4].equals("0") && values[6].equals("0"))){
            System.out.print(values[2] + " win");
            return true;
        }
        //Check verticals
        for(int x = 0;x < 9;x+=3){
            if((values[0+x].equals("X") && values[1+x].equals("X") && values[2+x].equals("X")) || (values[0+x].equals("0") && values[1+x].equals("0") && values[2+x].equals("0"))){
                System.out.print(values[0+x] + " win");
                return true;
            }
        }
        //Checks horizontal
        for(int x = 0;x < 3;x++){
            if((values[0+x].equals("X") && values[3+x].equals("X") && values[6+x].equals("X")) || (values[0+x].equals("0") && values[3+x].equals("0") && values[6+x].equals("0"))){
                System.out.print(values[0+x] + " win");
                return true;
            }
        }
        return false;
    }
    public static void main(String[] args) {
        Scanner read = new Scanner(System.in);
        String[] game = {" ", " ", " ", " ", " ", " ", " ", " ", " "};
        String[] players = {"X", "0"};
        for(int x = 0;x < 9;x++){
            System.out.print(players[x%2] + ": ");
            int currentPlay = read.nextInt();
            while(currentPlay < 1 || currentPlay > 9 || game[currentPlay -1] != " "){
                System.out.print("Invalid, play again: ");
                currentPlay = read.nextInt();
            }
            if(x % 2 == 0){
                game[currentPlay -1]  = "X";
            }
            else{
                game[currentPlay -1] = "0";
            }
            System.out.printf("%s | %s | %s\n%s | %s | %s\n%s | %s | %s\n", game[0], game[1], game[2], game[3], game[4], game[5], game[6], game[7], game[8]);
            if(test(game)){
                break;
            }
            if(!test(game) && x == 8){
                System.out.print("Tie");
            }
        }
    }
}

I hate this sort of hard-coded testing - there is usually a better approach to represent our data.
A few thoughts come to mind.

You don't stop players playing into an already occupied cell.
Why not use a 2-d array (actually an array of arrays)? Then you can more naturally scan rows, columns, and even diagonals.
If you score X as 1 and O as -1, then a winning row, column or diagonal adds up to 3 (X wins) or -3 (O wins).
If you initialise the cells to an appropriate value, I think you can also detect a hung game early - for example if you initialise to 4, then any row, column or diagonal with a sum < 4 but not 3 or -3 is unwinnable. If they're all unwinnable, there's no point making further plays. (I need to think this through a bit more!)
I thought it through more carefully. If X is 1, O is -1 and an empty cell is 4 (for example), then a line summing to 3 is a win for X; summing to -3 is a win for O; 4 is an unwinnable line as it must have an X, an O and a blank cell; other values are undecided lines. If there are 8 unwinnable lines, then the match is a draw.]


Inheritance with Singletons The problem / background

I have a project, BusinessProject, that currently stores some static data in memory using the Singleton pattern - we'll call this class GlobalData. This project is used in multiple applications, with different requirements on what data should be loaded into memory. One application, lets call this SubProject, only needs a subset of the data. The other project, MasterProject, calls SubProject as well as other functions within BusinessProject which require other data in GlobalData to be available in memory. My goal is to use inheritance to avoid storing the overlapping data twice, as well as let the project interfacing with BusinessProject easily define the requirements needed by simply calling the desired class.

Simplified Code

public class GlobalData
{
    protected static readonly GlobalData _Instance = new GlobalData();
    protected DateTime _lastUpdateDate = DateTime.MinValue;

    public Dictionary<string, class> Lookup { get; protected set; }
    public IList<DateTime> Dates { get; set; }
    public Dictionary<string, List<double>> DataSeries { get; protected set; }

    static GlobalData() { }
    private GlobalData() { }

    // each time the Instance requested, make sure the data doesn't need to be refreshed
    public static GlobalData Instance
    {
        get
        {
            if (!DateTime.Today.Date.Equals(_Instance._lastUpdateDate.Date))
                _Instance.LoadData();
            return _Instance;
        }
    }
    protected virtual void LoadData()
    {
        ...
        _lastUpdateDate = DateTime.Today;
    }
}

public class MasterData : GlobalData
{
    protected static new readonly MasterData _Instance = new MasterData();
    ... //extra data properties

    static MasterData() { }
    private MasterData() :base() {}

    protected override void LoadData()
    {
        base.LoadData();
        ... // load extra data properties
    }
}

Conclusion

I'm looking for any feedback on my implementation of the problem ! Thank you.

If the objective is to avoid duplicating information in memory, the code you have given won't archive it.

The new keyword hides the original value. It doesn't replace it. You should try another approximations, like the Decorator pattern: https://en.wikipedia.org/wiki/Decorator_pattern

The MasterData inherits from the GlobalData class and stores internally the reference to that class. Whenever someone accesses to a property of the GlobalData class, you redirect the request to the instance stored internally.

The MasterData class stores only the values of the newly added properties.

The tricky part would be preventing other parts of your MasterApplication app calling to the GlobalData class and thus creating another instance.

Singletons are always a nightmare...


Lazy lists with transformations, using generators & iterators I built this very-basic lazy list (I'll add more methods as I need them). You provide it an array, a generator or any iterator. It creates a lazy list, which lets you run a pipeline of transformations in a lazy manner, meaning that they'll only be applied as you pull values out of the list.

Here's the class:

class List {
    static range(start, end) {
        return new List(function* () {
            while (start <= end) {
                yield start++;
            }
        });
    }

    constructor(source) {
        if (typeof source == 'function') {
            this.generator = source;
        } else {
            this.generator = function* () {
                yield* source;
            };
        }
    }

    filter(predicate) {
        return new List(function* () {
            for (const item of this) {
                if (predicate(item)) {
                    yield item;
                }
            }
        }.bind(this));
    }

    map(mapper) {
        return new List(function* () {
            for (const item of this) {
                yield mapper(item);
            }
        }.bind(this));
    }

    take(size) {
        return new List(function* () {
            for (const item of this) {
                if (size--) {
                    yield item;
                } else {
                    break;
                }
            }
        }.bind(this));
    }

    *[Symbol.iterator] () {
        yield* this.generator();
    }

    toArray() {
        return [...this];
    }
}

Here's a simple example of how to use it:

List.range(1, 10 ** 9)
    .filter(number => number % 10 == 0)
    .map(number => 'Item ' + number)
    .take(5)
    .toArray();

Trying the same with a regular array:

Array.from({ length: 10 ** 9 }, (v, i) => i + 1)
     .filter(number => number % 10 == 0)
     .map(number => 'Item ' + number)
     .slice(0, 5);

...will run out of memory before it completes.

I can not find fault with your code apart from the use of class of which i am not a fan.

So I will just present an alternative syntax that gives a little extra encapsulated protection. Its usage is slightly different. List is instantiated via a factory and closure holds the generator list. I also freeze each instance of List to further protect the state.

It also does not have to mess about with const list = this which from your side-note comment is a bit of an annoyance.

const lazyList = (() => {

    const range = (start, end) => List(function* () {
       while(start <= end) { yield start++ }
    });
    function List(source) {
        var list;
        if (typeof source === "function") { list = source }
        else { list = function* () { yield* source } }

        return Object.freeze({
            filter(pred){
                return List(function* () {
                    for (const item of list()) { pred(item) ? yield item : 0 }
                });
            },
            map(map) {
                return List(function* () {
                    for (const item of list()) { yield map(item) }
                });
            },
            take(count) {
                return List(function* () {
                    for (const item of list()) {
                        if (count--) { yield item }
                        else { break }
                    }
                });
            },
            toArray() { return [...list()] },
            *[Symbol.iterator] () { yield* list() }
        });
    }
    return Object.freeze({range, List});

})();


Increment each digit in a number to form a new number Today I've found /r/dailyprogrammer and I've solved an easy challenge. I'm new to coding and I'm not sure if this is good way to solve this kind of problems. Could you, please, give me some hints? How to make my code more clear and readable? Thanks!

  Challange description:

  A number is input in computer then a new no should get printed by
  adding one to each of its digit. If you encounter a 9, insert a 10
  (don't carry over, just shift things around).

  For example, 998 becomes 10109.

  Bonus

  This challenge is trivial to do if you map it to a string to iterate
  over the input, operate, and then cast it back. Instead, try doing it
  without casting it as a string at any point, keep it numeric (int,
  float if you need it) only.

private static int Challange(int number)
        {
            int digits = (int)Math.Log10(number); //this is number of digits -1
            int result = 0;
            for (int i=0; i <= digits; i++)
            {
                int tens = (int)(Math.Pow(10, digits - i));
                int currentDigit = ((number / tens) % 10);

                if (currentDigit == 9) result *= 10;
                result += (currentDigit + 1) * tens;
            }
            return result;
        }

It works, but it's a bit tricky to follow the logic and work out why. If you work from the other end then you can avoid the Log and Pow and shifting the partial result, and I find that easier to read.

Challange is not a descriptive name. What does the method do? IncrementDigits might be a better name, for example.


Best way to create SVG from database? (Using PHP) I've got a SQL database that keeps track of which services a client has.  I need to create an image using SVG that only displays the services that the client has.  The database stores the services in the format 1,2,3,6,7,9.  Here's what I have:

 <?php
    header("Content-type: image/svg+xml");
    $db = new PDO(**[redacted]**);
    $grabServices = $db->prepare("SELECT services FROM *** where name= :name");
    $grabServices->execute(array(":name" => "test" ));
    $services = $grabServices->fetchAll();
    $services = explode(",", $services[0][services]);
    var_dump($services);
?>

//snip...

<?php if(in_array(1, $services)){?>
    <line x1="820" y1="120" x2 ="790" y2="180" class="SaaS"/>
<?php }?>
<?php if(in_array(2, $services)){?>
    <line x1="905" y1="180" x2 ="700" y2="370" class="PM"/>
<?php }?>
//etc, etc,

This seems incredibly tedious to me.  The only other way I can think to do this would have all the SVG elements created in a PHP array, but that seems even more wasteful.  Is there a better way to do this?

<?php
$formats = array(
    '1' => array(
        'x1' => 820,
        'y1' => 120
    ),
    '2' => array(
        'x1' => 905,
        'y1' => 180
    )
);
?>

<?php foreach( $formats as $key => $format ) : ?>
    <?php if ( in_array( (int) $key, $services ) ) : ?>
        <line x1="<?php print $format['x1']; ?>" y1="<?php print $format['y1']; ?>" />
    <?php endif; ?>
<?php endforeach; ?>

I didn't do it for every field, but you get the idea.

This is tedious to write the array. But you have to write it somewhere. If there is no logic, the computer can't guess.

I think it's easier to read/write a simple array than all this XML though.


Python : adding a columns with count of missing values by row I have a big python data-frame and I am trying to add a column to it with average number of missing values by row. I have inherited some code that is working but I'd like to reduce memory usage by removing intermediary values.
Here is a toy exemple :
students = [ ('jack', np.NaN, 'Sydeny' , 'Australia') ,
                 ('Riti', np.NaN, 'Delhi' , 'India' ) ,
                 ('Vikas', 31, np.NaN , 'India' ) ,
                 ('Neelu', 32, 'Bangalore' , 'India' ) ,
                 ('John', 16, 'New York' , 'US') ,
                 ('John' , 11, np.NaN, np.NaN ) ,
                (np.NaN , np.NaN, np.NaN, np.NaN )
                 ]
dfObj = pd.DataFrame(students, columns = ['Name' , 'Age', 'City' , 'Country'])

And the code I inherited :
print('NanCounter -> transform')
nan_count = pd.DataFrame(data = np.mean(dfObj.isna().values, axis=1).astype('float32'), columns=['nan_count']).set_index(dfObj.index)
X_ = pd.concat([dfObj, nan_count], axis=1)
X_.set_index(dfObj.index, inplace=True)

It seems quite a convoluted way to just write :
print('NanCounter -> transform')
dfObj['nan_count'] = np.mean(dfObj.isna().values, axis=1).astype('float32')

Plus it seems to consume more memory. I am concerned I am missing something about calculcations. Are those expression equivalent ? Namely, what would be the interest with working with a supplementary variable ?

The difference between the two code snippets is that the first creates a new DataFrame while the second modifies the original DataFrame in-place.
The first snippet can be simplified to
X_ = dfObj.assign(nan_count=dfObj.isna().mean(axis=1).astype('float32'))


Show and hide elements in JQuery I have the following script to reveal sections on a page if certain text in an  is clicked. How do I collapse all other sections if any one of them is selected and make it scalable for 20 items?

The following solution looks like it needs refactoring, and also when clicking icn_1, for instance, all other sections might already be hidden and putting an if for all sections for each click function seems very redundant.

$(document).ready(function() {
  $('section#1').hide();
  $('section#2').hide();
  $('section#3').hide();
  $('section#4').hide();

  function reveal1() {
  $("section#1").show("slow");
  $("section#2").hide("fast");
  $("section#3").hide("fast");
  $("section#4").hide("fast");
  }
  $(".icn_1").click(reveal1);

  function reveal2() {
  $("section#2").show("slow");
  $("section#1").hide("fast");
  $("section#3").hide("fast");
  $("section#4").hide("fast");
  }
  $(".icn_2").click(reveal2);

  function reveal3() {
  $("section#3").show("slow");
  $("section#1").hide("fast");
  $("section#2").hide("fast");
  $("section#4").hide("fast");
  }
  $(".icn_3").click(reveal3);

  function reveal4() {
  $("section#4").show("slow");
  $("section#1").hide("fast");
  $("section#2").hide("fast");
  $("section#3").hide("fast");
  }
  $(".icn_4").click(reveal4);
});

First, you have multiple functions with the same name.  That's not valid.

Assuming these sections are divs, to hide all of them except one, take a look at the not function:

function reveal1() {
    $("div").not("#section1").hide("slow");
    $("#section1").show();
}

Or more generically:

function revealThisElement(elementId) {
    $("div").not("#" + elementId).hide("slow");
    $("#" + elementId).show();
}

$(".icn_1").click(function() { revealThisElement("section1"); });
$(".icn_2").click(function() { revealThisElement("section2"); });
$(".icn_3").click(function() { revealThisElement("section3"); });

Or better yet, why not try something like this:

$(".revealable").click(function() {
   revealMe(this);
});

function revealMe(element) {
    $("div.revealable").not("#" + element.id).hide("slow");
    $(element).show();
}