Faster solution for data grouping to determine highest frequency than iteration with for loops The code below allows me to determine what the most common main dish and the most common method of preparation for the most common main dish, for each US Region. It uses data obtained from 'thanksgiving-2015-poll-data.csv' which can be found on (GitHub).

I believe that a pivot_table might offer a more efficient method of getting the same information, but I can not figure out how to do so. Can anyone offer any insight? Here's the code I used to get this information which works but I feel is not the best (fastest) method for doing so.

import pandas as pd

data = pd.read_csv('thanksgiving-2015-poll-data.csv', encoding="Latin-1")
regions = data['US Region'].value_counts().keys()
main_dish = data['What is typically the main dish at your Thanksgiving dinner?']
main_dish_prep = data['How is the main dish typically cooked?']
regional_entire_meal_data_rows = []

for region in regions:
    is_in_region = data['US Region'] == region
    most_common_regional_dish = main_dish[is_in_region].value_counts().keys().tolist()[0]
    is_region_and_most_common_dish = (is_in_region) & (main_dish == most_common_regional_dish)
    most_common_regional_dish_prep_type = main_dish_prep[is_region_and_most_common_dish].value_counts().keys().tolist()[0]
    regional_entire_meal_data_rows.append((region, most_common_regional_dish, most_common_regional_dish_prep_type))

labels = ['US Region', 'Most Common Main Dish', 'Most Common Prep Type for Main Dish']
regional_main_dish_data = pd.DataFrame(regional_entire_meal_data_rows, columns=labels)

full_meal_message = '''\n\nThe table below shows a breakdown of the most common
full Thanksgiving meal broken down by region.\n'''
print(full_meal_message)
print(regional_main_dish_data)

I have recast your loop, and the code is below.  I will discuss a couple of points.

pandas.Dataframe.groupby() allows working with specific groups at a time

Your current code is working with the entire dataframe for each region.  Pandas has the groupby to allow you to work with a specific regions data at one time.  I don't know if it is any faster, but to my eye is easier to read.

desired_cols = [region_col, main_dish_col, main_dish_prep_col]
for region, group in df[desired_cols].groupby('US Region'):
    ....

Using pandas.Series

A pandas.Series is a data structure that is basically two vectors.  One vector is the data, the other is the Index. In this code:

main_dish[is_in_region].value_counts().keys().tolist()[0]

.value_counts() returns a Series.  You then ask for the keys(), turn that into a list and the take the first element.  This is more naturally done by just taking the first elment of the index like:

.value_counts().index[0]

Main Loop Code:

df = pd.read_csv('thanksgiving-2015-poll-data.csv', encoding="Latin-1")
region_col = 'US Region'
main_dish_col = 'What is typically the main dish at your Thanksgiving dinner?'
main_dish_prep_col = 'How is the main dish typically cooked?'
desired_cols = [region_col, main_dish_col, main_dish_prep_col]

regional_entire_meal_data_rows = []
for region, group in df[desired_cols].groupby('US Region'):
    main_dish = group[main_dish_col]
    main_dish_prep = group[main_dish_prep_col]

    most_common_dish = main_dish.value_counts().index[0]
    prep_types = main_dish_prep[main_dish == most_common_dish]
    most_common_prep_type = prep_types.value_counts().index[0]
    regional_entire_meal_data_rows.append(
        (region, most_common_dish, most_common_prep_type))


Mastermind game in Kotlin I am trying to create an evaluator in Kotlin for the Mastermind game which compares two strings and tells how many letters and letters on the right position they share.

I was hoping to get some comments on how I can improve the code below in any way. Many thanks in advance!

fun String.masterCompare(otherString:String): Pair<Int, Int>? {

    // If the two strings are not equal in size, return null
    if (length != otherString.length)
        return null

    var letters : Int = 0  // The number of letters the two strings have in common
    var positions: Int = 0 // The number of shared letters that are in the same index
    var other = otherString // A mutable string copy of the argument string

    // Get number of shared indexes (positions)
    for ((index, c) in withIndex()){
        if (c == otherString[index]) {
            positions++
        }
    }

    // Check if a char in this string exists in the other. If so, increment letters and delete the char from the other string (so that it's not rechecked)
    for (c in toList()){
        if (other.contains(c)){
            letters++
            other = other.replace(c.toString(), "")
        }
    }

    return letters to positions
}

fun main(args: Array<String>) {
    println("ABCD".masterCompare("AFCH"))    // 2,2
    println("AAAA".masterCompare("AFCH"))    // 1,1
    println("ABCD".masterCompare("ABCD"))    // 4,4
    println("ABCD".masterCompare("DCBA"))    // 4,0
    println("ABCDP".masterCompare("DCBA"))   // null
}

You could go for a more functional (rather than imperative) approach.

Such as:

fun String.masterCompare(otherString: String): Pair<Int, Int>? =
        when {
            length != otherString.length -> null
            else -> {
                // this part is really easy
                val commonAtSameIndex = zip(otherString).count {
                    (one, another) -> one == another
                }

                /* calculates the frequency map of characters in a given string.
                 * not the best solution in terms of performance - if you were to expect
                 * really large strings, you might be better off keeping the imperative approach.
                 * possibly with a bitmask instead of reassigning the mutable string. */
                fun String.toFrequency() = groupBy { it }
                        .mapValues { it.value.size }
                        .toMap()

                val thisFrequencyMap = toFrequency()
                val otherFrequencyMap = otherString.toFrequency()

                val commonOverall = thisFrequencyMap
                        .map { (letter, count) ->
                            min(count, otherFrequencyMap[letter] ?: 0)
                        }
                        .sum()
                commonOverall to commonAtSameIndex
            }
        }

I would also suggest avoiding nulls out of principle, and possibly returning (0, 0) if strings aren't of the same length.

If you do need to differentiate between this case and strings of the same length that don't share any characters, I'd consider coming up with dedicated types instead of a nullable Pair, which is too generic (and makes it easy to confuse which field stands for what).

Eg. the result type might be:

sealed class MasterComparisonResult {
    object DifferentLength : MasterComparisonResult()

    data class SameLength(val commonAtSameIndex: Int, val commonOverall: Int) : MasterComparisonResult()
}

And then the function returns a MasterComparisonResult instance.

Here's a version that combines the functional way - in my opinion much clearer - of calculating the number of identical characters in the same positions with the loop that calculates the other value.

A bit array is used for better performance.

fun String.masterCompare(otherString: String): Pair<Int, Int>? =
        when {
            length != otherString.length -> null
            else -> {
                // this part is really easy
                val commonAtSameIndex = zip(otherString).count {
                    (one, another) -> one == another
                }

                var commonOverall = 0
                val countedAlready = BooleanArray(length)

                /* this is yet another detail: there's no need for toList() allocation.
                 * you can iterate over this (String) straight away. */
                for (c in this) {
                    /* find the first occurrence of the c character in otherString
                     * that wasn't counted already */
                    val index = countedAlready
                            .asSequence()
                            .withIndex()
                            .filterNot { it.value }
                            .indexOfFirst { otherString[it.index] == c }
                    if (index >= 0) {
                        countedAlready[index] = true
                        commonOverall++
                    }
                }
                commonOverall to commonAtSameIndex
            }
        }


Building array keys using a span of numbers for like values I am calculating discounts based on a quantity after 6.

Is there a cleaner way to do this:

$leg_directory_discount = array(
    6   =>  .50,
    7   =>  .50,
    8   =>  .50,
    9   =>  .50,
    10  =>  .50,
    11  =>  1,
    12  =>  1,
    13  =>  1,
    14  =>  1,
    15  =>  1,
    16  =>  1,
    17  =>  1,
    18  =>  1,
    19  =>  1,
    20  =>  1,
    21  =>  1.5,
    22  =>  1.5,
    23  =>  1.5,
    24  =>  1.5,
    25  =>  1.5,
    26  =>  1.5,
    27  =>  1.5,
    28  =>  1.5,
    29  =>  1.5,
    30  =>  1.5,
    31  =>  1.5,
    32  =>  1.5,
    33  =>  1.5,
    34  =>  1.5,
    35  =>  1.5,
    36  =>  1.5,
    37  =>  1.5,
    38  =>  1.5,
);

edit: The max and min values of the discounts never change. Here is how I'm calculating the discount and arriving at the final price:

if($v[0]['leg_discount'] == '1' && $v[0]['qty'] >= 6){
    $discount = $qty * $leg_directory_discount[$qty];
    $price = $price - $discount;
}

I like the idea of putting the discount steps into an array, but stating a discount for every possible quantity seems too much.

$leg_directory_discount = array(
    0 => 0,
    6   =>  .50,
    11  =>  1,
    21  =>  1.5,
);

This is the discount configuration - can later be stored in a config file or database.

This function gets the discount to be used:

function getDiscount($qty, $leg_directory_discount) {
  $return = 0;
  foreach ($leg_directory_discount as $amount => $discount) {
    if ($qty >= $amount) {
      $return = $discount;
    }
  }
  return $discount;
}


Memoization of Fibonacci using generic Int => Int helper I'm trying to understand memoization using Scala. I took the Fibonacci sequence as an example because of the high cost of recursively computing numbers in the sequence without memoization.

class memoize private (val f: Int => Int) {
  import scala.collection.mutable
  val cache = new mutable.HashMap[Int, Int]()
  def memoized_f(x : Int) : Int =
    cache.getOrElseUpdate(x, f(x))
}
object memoize {
  def apply(f: Int => Int) : Int => Int = new memoize(f).memoized_f
}

val fib : Int=>Int = memoize((n:Int) => {
  if (n <= 1) n else fib(n-1) + fib(n-2)
})

With memoization the function fib is called 10 times compared to 276 without it. I discovered that val fib is required: def fib results in the memo object being created on each call to fib!

I realize that this could eventually be generalized by parameterising memoize with a type T (or key K and value V) to replace Int. But I wanted to start somewhere simple to understand how the technique works. Given that I'm looking at memoizing only Int => Int functions, could what I've done be improved in terms of function or style?

I've also discovered a couple of related items on the web: Is there a generic way to memoize in Scala? and Memo in ScalaZ (see also this tutorial), but I'm not entirely sure how what I've done relates to them.

For generic memoization I pretty much always use scalaz Memo but when I compute sequences, I personally prefer using the standard Scala Stream.

Stream naturally memoize every computed value. Your solution could be refactored as:

lazy val fib = {
  def f(a: Int, b: Int): Stream[Int] = a #:: f(b, a + b)
  f(0, 1)
}

scala> fib.take(10) foreach println
0
1
1
2
3
5
8
13
21
34

scala> fib(45)
res1: Int = 1134903170


Recode algorithm for merged cells on HTML table Is there any way to improve this algorithm maybe making it so faster avoid using foreach? Maybe with a recursion?
The output of my algorithm is a table with 3 merged columns, each row will have its own rowspan.

$matrix=array("person1"=>array("wifes"=>array("diana"=>array("born"=>1982,"last_name"=>"chapelle"),"carla"=>array("born"=>1983,"last_name"=>"michael"),"serena"=>array("born"=>1984,"last_name"=>"mickelson")
            ,"king"=>array("born"=>1980,"last_name"=>"jordan")))
    ,"person2"=>array("husbands"=>array("charles"=>array("born"=>1983,"last_name"=>"rambo")
    ,"minglen"=>array("born"=>1973,"last_name"=>"rian"),"serrano"=>array("born"=>1914,"last_name"=>"kingside"))));

$htmlx="
";
    $htmlx.="
";
    $htmlx.="object	type	names	detail_names	values_detail	";
    $html_p=NULL; $p=0;
    foreach ($matrix as $pkey => $pvalue) {
        $t=0;   $html_t=NULL;
        foreach ($pvalue as $tkey => $tvalue) {
            $n=0;   $html_n=NULL;   $rs_general=0;
            foreach ($tvalue as $nkey => $nvalue) {
                $d=0;   $html_d=NULL;   $rs_general+=count($nvalue);
                foreach ($nvalue as $dkey => $dvalue) {

                    $d++;
                    if($d==1){
                        $html_d="".$dkey."	".$dvalue."	";
                    }else{
                        $html_d.="
".$dkey."	".$dvalue."
";
                    }
                }
                $n++;
                if($n==1){
                    $html_n.="".$nkey."	".$html_d;
                }else{
                    $html_n.="
".$nkey."	".$html_d."
";
                }
            }
            $t++;
            if($t==1){
                $html_t="".$tkey."	".$html_n;
            }else{
                $html_t.="
".$tkey."	".$html_n."
";
            }
        }
        $p++;
        if($p==1){
            $html_p.="".$pkey."	".$html_t;
        }else{
            $html_p.="
".$pkey."	".$html_t."
";
        }

    }
    $htmlx.="
".$html_p;
$htmlx.="
";
echo $htmlx;

I have been thinking about recursion function but I don’t get the solution yet, so any help will be welcome.

Some tips to make it faster

Use references in foreach loops with &, like foreach ($matrix as $pkey => &$pvalue)
Don't use $htmlx variable, just output everything with echo
For some scecific purposes you may use ob_start(), ob_get_contents() and ob_end_clean() - it's much faster than concat like $html_d.=
Don't output html with php, just close php tags

if($d==1){ ?>
   <?php echo $dkey ?>	<?php echo $dvalue ?>
<?php }else{ ?>


First n Happy Numbers I wrote a piece of code to generate a list of \$n\$ Happy Numbers, would love to see how I can write it better. More information below.

#a happy number is defined is found by taking a number, and adding the sum of its digits, and repeating the steps to add the sum of square
#of the resultant digits until we reach 1, if the result never reaches 1 then it is not a happy number

# example: number = 7
# 7^2 = 49
# 4^2 + 9^2 = 16+81 = 97
# 9^2 + 7^2 = 81 + 49 = 130
# 1^2 + 3^2 + 0 = 1+9 = 10
# 1^2 + 0 = 1 HAPPY number
#defining a emtpy list that will be populate as soon as we encounter a happy number
happy_numbers = []
sqdict={str(i):i**2 for i in range(10)}

#lets define our adder
def adder3(num):
    return sum(sqdict[i] for i in str(num))

#now we need to take a number and keep adding until we get a 1 or the loop keeps repeating
# lets define a function for that

def happynum (num, counter):
    i_sum = adder3(num) # storing the value from the adder fucntion to i_sum
    if i_sum == 1: # check for happy number
        happy_numbers.append(number)
        # print("The given number {} is a happy number.".format(number))
        return 1
    else: # we continue to keep splitting and adding until we reach 1 or attain infinite loop
        counter +=1
        if counter > 50:
             # print("The number {} isnt happy :(.".format(number))
            return False
        else:
            happynum(i_sum,counter)

counter = 0 # initializing a counter to keep keep track of the infinite loop if it does reach that.
# user_num = 7
for i in range (0, 1000):
    number = i
    happynum(number, counter)
print(happy_numbers[10])

The code above is working well for what I am trying to achieve. However, I tried defining the counter as a global variable before all the methods, but when called in the method I was not able to increment. It threw aa error "local variable referenced before assignment".

Additionally, I am not sure using a for loop in the end to generate 1000 Happy Numbers is an efficient way, it seems somewhat limiting. Would appreciate some help on this.

Coding style

There is too little white space at many places, for example

sqdict={str(i):i**2 for i in range(10)}

This and other PEP8 coding style
violations (most of them related to spacing and too long comment lines)
can be detected by checking your code at PEP8 online.

Unnecessary comments

There are some comments which add no information to the code and can safely be removed,
such as

#lets define our adder
def adder3(num):

or

i_sum = adder3(num) # storing the value from the adder fucntion to i_sum

Naming

def adder3(num):

Is is impossible to guess from the function name what its purpose might be.
I'd suggest something like square_digit_sum.

def happynum (num, counter):

Here we know what the function is about, what does it do? Compute a happy number,
check for happy number, ...?

Code structure:

The global variable counter = 0 is not used at all and can be removed.

A global variable number = i is used to "remember" the argument of
the inital call to the recursive happynum function, and the function
(as a "side effect") appends to the global happy_numbers array.
This is error-prone and not very elegant.

I'd suggest to define a function is_happy instead, which takes a number and
recursion limit, and returns True or False. The default recursion limit can be
defined as a default parameter value:

def is_happy(num, reclimit=50):
    if num == 1:
        return True
    if reclimit <= 0:
        return False
    return is_happy(square_digit_sum(num), reclimit - 1)

Note also that no else: block is needed if the if: block
has a return statement. This saves indenting levels.

Then you can use list comprehension

happy_numbers = [num for num in range(1000) if is_happy(num)]

or a filter

happy_numbers = filter(is_happy, range(1000))

to create an array with the first 1000 happy numbers.

Also it is a good habit to put add a "main guard" so that the source file can be
imported as a module:

if __name__ == "__main__":
    happy_numbers = filter(is_happy, range(1000))
    print(happy_numbers)

Performance improvements:

Computing the square digit sum can be made slightly faster by using integer
arithmetic only, without string conversions:

def square_digit_sum(num):
    sum = 0
    while num > 0:
        digit = num % 10
        sum += digit * digit
        num //= 10
    return sum

(This makes the sqdict hash obsolete.)

The recursion limit of 50 is somewhat arbitrary, and might not be sufficient for
large numbers. A possible alternative is to remember all numbers seen so far
in a set:

def is_happy(num):
    seen = set()
    while num > 1:
        if num in seen:
            return False
        seen.add(num)
        num = square_digit_sum(num)
    return True

More suggestions:

Add docstring comments to all functions.


Basic multithread server code I wrote a simple multithread server to use as a base class for other different types of multithread servers. I haven't had a lot of opportunities to get code critique from other people, so I wanted to post here to see how much coding style was. Any constructive feedback on how to improve my code would be appreciated! I'm personally looking more for code style and quality rather than compilability (since it does compile and work)

define.h

#pragma once

#define DEBUG
#ifndef DEBUG
#define _(x)
#else
#define _(x) x
#endif

Server.h

#pragma once

#include <iostream>
#include <netinet/in.h>
#include <unistd.h>
#include <string.h>
#include <thread>
#include <map>
#include "define.h"

class Server{
    private:
        virtual void handle_connection(int sockfd);
    public:
        Server() {}
        void initialize(int port);
        void start_listening(int socket);
        void authenticate(int sockfd);
        void sendMessage(int sockfd, std::string prompt);
        ~Server() {}
};

Server.cpp

#include "server.h"

void Server::initialize(int port){
    int sockfd;
    struct sockaddr_in serv_addr;

    //Create socket
    sockfd = socket(AF_INET, SOCK_STREAM, 0);
    if(sockfd < 0){
        perror("Error: Failed to initialize socket.\n");
        exit(1);
    }

    //Bind socket
    bzero((char *) &serv_addr, sizeof(serv_addr));
    serv_addr.sin_family        = AF_INET;
    serv_addr.sin_port      = htons(port);
    serv_addr.sin_addr.s_addr   = INADDR_ANY;
    if(bind(sockfd, (struct sockaddr *) &serv_addr, sizeof(serv_addr)) < 0){
        perror("Error: Failed to bind socket to address.\n");
        exit(1);
    }

    start_listening(sockfd);
    return;
}

void Server::start_listening(int socket){
    while(1){
        int newsockfd = -1;
        struct sockaddr_in cli_addr;
        socklen_t clilen = sizeof(cli_addr);

        listen(socket, 5);
        newsockfd = accept(socket, (struct sockaddr *) &cli_addr, &clilen);

        if(newsockfd < 0){
            perror("Error: Failed to connect to incoming connection.\n");
        }

        //Start new thread to handle request
        std::thread t1 (&Server::handle_connection, this, newsockfd);
        t1.detach();
    }
}

void Server::handle_connection(int sockfd){
    char buffer[256];
    std::string response = "Thank you for your response.\n";
    const char* responseChar = response.c_str();
    authenticate(sockfd);
    while(recv(sockfd, buffer, 255, 0) > 0){
        std::cout << buffer;
        send(sockfd, (void*)responseChar, strlen(responseChar), MSG_NOSIGNAL);
        bzero(buffer, 256);
    }
    close(sockfd);
    return;
}

void Server::sendMessage(int sockfd, std::string prompt){
    const char * promptChar = prompt.c_str();
    send(sockfd, (void*)promptChar, strlen(promptChar), MSG_NOSIGNAL);
}

Include (only) what you use
Users of Server.h don't need most of those headers; they can be reduced to
#include <string>

You might need some of the other headers in the implementation, but try to avoid polluting users' namespaces (and slowing their compilations).
Consider adding daemonize code
Most servers want to be able to daemonize before they start - provide a facility to do the forking and dissociation of controlling terminal and the rest.  Make sure it's optional, so users can run in foreground if they want (e.g. for debugging, or when controlled by a supervisor like systemd).
Report errors back to caller
Consider this code:
    if(newsockfd < 0){
        perror("Error: Failed to connect to incoming connection.\n");
    }

This prints a message to stderr, and then keeps going.  How does the server (subclass) know that we failed?  What if we want to syslog the failure?  It's better to communicate this information in return value or an exception, so the application can take the right action - here, in a library, we don't know what's appropriate, nor whether standard error stream is even available to us.


Avoiding the 'possible loss of data' compiler warning I'm writing a file I/O DLL in C++ which is going to be used from VB6. The interface is already fixed, and uses INT64/Currency as the general integer data type.

Now I have this function:

extern "C" LPWIN32_FIND_DATAW ZFILEIO_API GetFileInfoByIndex(INT64 index) {
    return total_size >= 0
        && index <= file_vec->size()
        && index >= 0
              ? &file_vec->at(index)
              : NULL;
}

where file_vec is of type std::vector<WIN32_FIND_DATAW>. Its at member function takes a size_t argument, which is in my case an unsigned 32-bit integer. Therefore, MSVC++2010 warns me of possible data loss. I think nothing can happen in my code, but please correct me if I'm wrong. Also, do you know a method to avoid the warning (without shutting it off for other places in my code)?

And then there's another question; I would like to return the WIN32_FIND_DATAW struct by value, not as a pointer, but that's not possible because then I can't return NULL for illegal requests. Can I return such a pointer to my internal data to a VB6 application?

Your types are not the same, and you're relying on implicit conversion. The compiler is warning you because not all implicit conversions in this case will be valid. If you know that the conversion is valid in your case, then you should do the conversion explicitly. It's sensible to check the condition you're relying on:

extern "C" LPWIN32_FIND_DATAW ZFILEIO_API GetFileInfoByIndex(INT64 index) {
    assert(index <= SIZE_MAX);

    return total_size >= 0
        && index <= file_vec->size()
        && index >= 0 ? &file_vec->at(static_cast<size_t>(index)) : NULL;
}

The check can be either an assert or throw an exception, depending on how confident you are that this condition cannot (even theoretically) be hit in your code. You may also want to make sure that the range of index is checked at a higher level in the code where it can throw a more meaningful exception, e.g. in the constructor of the corresponding type. Even if you do this, keeping an additional check in the code just prior to the cast is useful, because it verifies to a future maintainer that the cast is valid.

If the compiler still complains when you have the explicit cast in place, then I think your only option is to temporarily silence the warning. I still prefer the cast solution even in this case, since it's obvious to the reader that something potentially dangerous is being done.

Note that SIZE_MAX is not defined in the C++ standard AFAICT. It's likely to be available on your platform, but if not there's almost certainly another way of getting the same information, namely the maximum value of size_t.


Factorial in JavaScript I decided to write a simple factorial() function in JavaScript and ask here for opinions. The idea is to further improve my JavaScript style and learn the best practices. Is there something bad with this code? What can be improved? Any suggestions are welcome.

/**
 * Calculates the factorial of a given non-negative finite integer.
 * @param {Number} n
 * @return {Number} factorial
 * @throws TypeError if n is not a Number
 * @throws TypeError if n is a Number but not an integer
 * @throws RangeError if n is a negative integer
 */
function factorial(n) {
    if (typeof n !== "number") {
        throw new TypeError("factorial() expects a number.");
    }
    if (Math.floor(n) !== n) {
        throw new TypeError("factorial() expects an integer.");
    }
    if (n < 0) {
        throw new RangeError("factorial() expects a non-negative number.");
    }

    // 170 is the biggest integer whose factorial does not become Infinity in JavaScript.
    // Therefore, we can improve performance by checking directly.
    // This if statement also handles the important edge case of n === Infinity, which
    // would cause the function to be stuck forever.
    if (n > 170) {
        return Infinity;
    }

    var accumulator = 1;

    for (var i = 2; i <= n; i++) {
        accumulator *= i;
    }

    return accumulator;
}

if (typeof n !== "number") {
    throw new TypeError("factorial() expects a number.");
}
if (Math.floor(n) !== n) {
    throw new TypeError("factorial() expects an integer.");
}
if (n < 0) {
    throw new RangeError("factorial() expects a non-negative number.");
}

First, I discourage manual type and value checks on runtime. They're redundant since the JS engine will emit an error if you operate on the wrong data anyways. It's also runtime overhead as well as unnecessary code.

Often times, developers forward this task to the IDE when writing code. With the right plugins and annotations, the IDE can warn you of incompatible data types and operations. This only prevents writing the API incorrectly though. There's still risk of using the function incorrectly, but that's the consumer's fault, not the API.

var accumulator = 1;

JS has let, the block-scoped version of var. It's a good habit to use let since in most cases, you'll want block-scoped vars. It also prevents ambiguity of where the variable exists in the code, which usually causes problems (like var in for or if). If I remember correctly, it's because of hoisting that developers started defining vars explicitly on the top of the function body to signal that a variable of that name exists in the function.

if (n > 170) {
    return Infinity;
}

var accumulator = 1;

for (var i = 2; i <= n; i++) {
    accumulator *= i;
}

return accumulator;

In the end, the factorial code is just the one above. A shorter version factorials can be done using recursion.

function factorial(n) {
  return n > 170 ? Infinity    // Bail out on 170
    : n == 0 || n == 1 ? 1  // When the answer is just 1
    : n * factorial(n - 1)  // Multiply current with next smallest
}

console.log(factorial(1))
console.log(factorial(2))
console.log(factorial(3))
console.log(factorial(4))
console.log(factorial(5))


Material UI menu drop down component The reason I created this is because the Material UI way of doing a simple 'menu that opens below the button' is convoluted as hell (see the 'toggle menu grow' example, and check the code out. I've basically copied the code for that, and put it in one nice wrapper.

Codepen

Here's the usage of the component:

<NavDropdownMenu
  buttonProps={{
    variant: "contained",
    color: "secondary"
  }}
  buttonChildren={"foo bar"}
>
  <MenuItem onClick={this.handleA}>Profile</MenuItem>
  <MenuItem onClick={this.handleB}>My account</MenuItem>
  <MenuItem>Logout</MenuItem>
</NavDropdownMenu>

Notes:

We can pass through whatever material props to the button.
We can put whatever content we like inside the button. (e.g. icons, avatars).
We put whatever we like as menu items, and bind our own onclick events to them.

Under the hood, in order to both trigger the close menu functionality, as well as allow the user to bind their own click events to the menu items, I have this code that uses React.cloneElement:

   <MenuList>
    {children.map((child, i) => {
      return React.cloneElement(child, {
        ...child.props,
        ...{
          onClick: this.generateOnClick(child.props.onClick),
          key: `${menuId}${i}`
        }
      });
    })}
  </MenuList>

And that function:

  generateOnClick = fn => {
    if (fn)
      return () => {
        fn();
        this.handleMenuClose();
      };
    else {
      return this.handleMenuClose;
    }
  };

Now, I haven't put in PropTypes; I have to confess I'm not in the habit of using them. And there's probably some more extensibility I could add - (e.g. configuring the placement of the menu).

But other than that, are there glaring issue with this code that make it a bad idea?

As a general sentiment, I can't spot any glaring issues with the code just the one section and a few minor things (you did say you basically copied it; I'd imagine the Material UI team have some idea what they're doing)

{children.map((child, i) => {
  return React.cloneElement(child, {
    ...child.props,
    ...{
      onClick: this.generateOnClick(child.props.onClick),
      key: `${menuId}${i}`
    }
  });
})}

children cannot always be defined as an array, if you pass a single list item it will be given as a single element which will not have map as a property. You should be using React.Children.map instead.

Slipping in a small code style comment here, doing => { just to return something moments later seems like wasted code.

{children.map((child, i) => {
  return React.cloneElement(child, {

When using React.cloneElement, you don't need to manually specify the child props as that's the implication of cloning.

return React.cloneElement(child, {
  ...child.props,

Creating a separate object inside another object just to spread is redundant.

...{
  onClick: this.generateOnClick(child.props.onClick),
  key: `${menuId}${i}`
}

To finish things off here, using the index of the child as part of the key is an anti-pattern. Any time that someone wants to change the children, or reorder the list, React will assume it has already memoized the render during the previous reconcile and give you the wrong render.

There's a number of resources and in-depth explanations if you Google something along the lines of React index as key. Generally when building reusable components, I leave the key generating relative to each implementation, but, that's your decision, however, I would say that some developers want to use their own keys and manually overriding them is blocking that.

key: `${menuId}${i}`

A better implementation would be:

{React.Children.map(children, child =>
  React.cloneElement(child, {
    onClick: this.generateOnClick(child.props.onClick)
  })
)}

Even so, you're cloning all of your children inside each render of the component, with all the information used in that process being loaded in during the initialisation phase.

Everytime you act on the menu, you're performing the same element cloning on all your children again. If I had complex children or even a large amount, this could easily slow the render process.

A few minor points:

buttonChildren would better be named label or menuLabel
Using propTypes is very beneficial, especially if you're trying to create a reusable component/library, not only to the developer in charge of implementation to know what to expect but you (or whoever) in charge of fixing potential bugs in the library.
Usually the Popper component allows you to open and close a menu by clicking it... for whatever reason, you can't close the component by clicking it.
Stick with consistent bracketing around your if/else

if (fn)
  return () => {
    fn();
    this.handleMenuClose();
  };
else {
 return this.handleMenuClose;
}


Segmented Sieve of Eratosthenes in Scala I'm trying to solve PRIME1 in SPOJ with the following Scala snippet:

  Peter wants to generate some prime numbers for his cryptosystem. Help
  him! Your task is to generate all prime numbers between two given
  numbers!

  Input

  The input begins with the number \$t\$ of test cases in a single line
  (\$t \le 0\$). In each of the next t lines there are two numbers \$m\$ and \$n\$ (\$1 \le m \le n \le 1000000000, n-m \le 100000\$) separated by a space.

  Output

  For every test case print all prime numbers \$p\$ such that \$m <= p <= n\$,
  one number per line, test cases separated by an empty line.

object _002_PRIME1 extends App {

  import scala.io.StdIn._

  /// ***1***
  // def isPrime(num: Int) = (2 until num).view.takeWhile(i => i * i <= num).forall(i => i * (num / i) < num)

  def sieve(m: Int, n: Int, step: Int, primeArray: Array[Boolean]): Unit = {
    val start = {
      if (m <= step) {
        2 * step
      } else {
        val pre = (m / step) * step
        if (pre == m) {
          pre
        } else {
          pre + step
        }
      }
    }
    Range(start, n + 1, step).foreach(i => primeArray(i - m) = false)
  }

  def simpleSieve(num: Int): IndexedSeq[Int] = {
    val basicPrimes = Array.fill(num - 1)(true)
    for {
      i <- 2 to num
      if basicPrimes(i - 2)
      j <- i * 2 to num by i
    } basicPrimes(j - 2) = false
    for {
      j <- basicPrimes.indices
      if basicPrimes(j)
    } yield (j + 2)
  }

  def sieveAll(m: Int, n: Int): Unit = {
    val primeArray = Array.fill(n - m + 1)(true)

    val basicPrimes = simpleSieve(math.sqrt(n).toInt)
    /// for(p<- basicPrimes)sieve(m, n, p, primeArray)
    basicPrimes.foreach(p => sieve(m, n, p, primeArray))
    if (m == 1) {
      primeArray(0) = false
    }
    for (j <- primeArray.indices; if primeArray(j)) println(j + m)
    /// ***2***
    //    Array.tabulate(primeArray.length) { i => {
    //      if (primeArray(i)) {
    //        println(i + m)
    //      }
    //    }
    //    }
  }

  (1 to readInt()).foreach(_ => {
    val ss = readLine().split("\\s+").map(_.toInt)
    val (m, n) = (ss.head, ss.last)
    sieveAll(m, n)
  })
}

For PRIME1 itself, this is able to output the correct answer within 0.6s. However for another problem, PRINT, which is almost the same except that the input range is bigger: \$2 \le L < U \le 2147483647, U-L \le 1000000\$, and the time limit is more restricted (1.223s).

When I submit this code, it always tells that time limit exceeded. How can I tune this snippet to make it run faster?

Attempts I've tried (the time costs are based on the OJ from PRIME1):

Using isPrime for basic primes checking, this would increase the running time to 0.7s.
Using Array.tabulate instead of for comprehension for output, decreasing time a little, about 0.01s.

Additionally, from Daniel C. Sobral's answer in the other question, mutable.BitSet may be more memory efficient, however this answer mentions that as to Java, an array might be more CPU efficient. I therefore didn't use BitSet for Scala (the memory used by is about 314MB while the memory limited by SPOJ is 1536MB).

There is a lot of overlap between the concept of windowed Sieve of Eratosthenes and that of segmented one, which is why the two are often confuddled.

A windowed sieve is rigged such that it can sieve a window, that is a range of numbers in the interval [m, n] (in SPOJ PRIME1/PRINT parlance), without having to sieve all the numbers before the start of the interval as well. The main trick here is to use modulo operations to find where the crossing-off sequence for a prime first intersects with the sieve window.

You have already implemented this, but I'm showing it again in pseudo code that makes the logic clearer. It is for an odds-only sieve, which is why there is a division by 2 at the end of the offset calculation, which transforms the starting point from the realm of numbers into the realm of odds-only bit indices.

internal static ushort[] small_odd_primes; // up to sqrt(MAX_N)
internal static bool[] sieve = new bool[1 << 15];

internal static void sieve_primes_between (int m, int n)
{
    // ...

    int last_prime_index = Array.BinarySearch(small_odd_primes, (ushort)Math.Sqrt(n));
    if (last_prime_index < 0)
        last_prime_index = -last_prime_index - 2;

    for (int i = 0; i <= last_prime_index; ++i)
    {
        int prime = small_odd_primes[i];
        int start = prime * prime;
        int stride = prime << 1;

        if (start < m)
            start = (stride - 1) - (m - start - 1) % stride;
        else
            start -= m;

        offset[i] = start >> 1;
    }

    // ...

Of course, this not real pseudocode; it is my PRIME1/PRINT submission from my 'learn C#' tour of the world (fastest PRINT submission in C# and third fastest overall).

The modulo logic is a bit more complicated than what you did, for two reasons. First, I start crossing off at the square of a prime because all lower multiples will already have been crossed off during the runs for smaller primes. The effect minuscule, so don't worry about it. The second reason is that I wanted to minimise the number of branches necessary. Biassing the modulo operation by -1 makes it yield directly the desired result in the range 0 to stride-1, instead of giving 1 to stride where the last value has to be corrected to 0. Again, the effect is minor, but I wanted a clear, correct rendering of the windowed sieve offset logic on record.

Now, on to segmented operation. The purpose here is not to avoid the sieving of unwanted numbers as it is for a windowed sieve. The purpose is to sieve the desired range in small batches that do not exceed the size of the L1 cache of the target CPU (typically 32 KiB). For large sieve windows this can speed up things by orders of magnitude.

    for (int bits_to_sieve = ((n - m) >> 1) + 1; bits_to_sieve > 0; )
    {
        int window_bits = Math.Min(bits_to_sieve, sieve.Length);

        Array.Clear(sieve, 0, window_bits);

        for (int i = first_sieve_prime_index; i <= last_prime_index; ++i)
        {
            int p = small_odd_primes[i], j = offset[i];

            for ( ; j < window_bits; j += p)
                sieve[j] = true;

            offset[i] = j - window_bits;
        }

        append_sieved_numbers_to_result_vector(m, window_bits);

        m += window_bits << 1;
        bits_to_sieve -= window_bits;
    }

I think this makes the core idea of the segmented sieve very clear. In this case I'm reusing the sieve segment over and over, which is why I have to subtract window_bits before stashing an offset back in its slot. The same logic can also be used to process a bigger sieve in small, L1-sized stripes (but there's no advantage to that for PRIME1 and PRINT).

append_sieved_numbers_to_result_vector() just goes through the sieved range and pulls out the primes; I'm not showing it because it is actually twice as long as the sieve code (with mod 6 stepping and unrolling and all that) and still it takes twice as long as the actual sieving. It's core logic is quite simple, though:

    for (int i = 0; i < window_bits; ++i)
        if (!sieve[i])
            result[result_size++] = m + (i << 1);

In my submission I'm blasting a precomputed bit pattern over the sieve instead of calling Array.Clear(), in order to avoid the many iterations necessary for crossing off the smallest primes. However, the effect is not major and nothing to worry about.

What you do have to worry about is efficient extraction of the primes from the sieve, and I'd definitely recommend using an odds-only sieve in order to halve your workload in one fell swoop.

You'll also have to worry - a lot - about efficient output of the sieved primes. My submission used custom conversion code that rendered the numbers directly into the output buffer, digit by digit, in-place without any copying/moving. Using C#'s lame builtin I/O would have added at least a second to the timing, threatening TLE. See SPOJ's INOUTEST for the 'fast bulk output of numbers' thing. There are a lot of problems on SPOJ where fast bulk output of numbers comes in handy, and where the output time can dominate the total timing by a factor of 100 or so if you use the slow builtin I/O that comes with most languages. My first ETF submission got TLE, and after replacing the output code - not the actual processing related to the task - it clocked 0.030 vs. the time limit of 0.161, a nice big reserve.

I cannot comment on Scala's mutable.BitSet but I can assure you that its equivalents in C++ (std::vector<bool>) and in C# (System.Collections.BitArray) are slow as molasses and slow things down by an order of magnitude.

Custom code using a byte array and manual bit indexing (sieve[j>>3] |= 1 << (j & 7)) still takes twice as long as the plain bool[] sieve. It can be made about 33% faster than bool[] by copious unrolling, but the effort required is on the same order of magnitude as that for implementing a mod 30 wheel. To give you an impression, here is one of eight functions, each of which sieves a different residue class mod 8:

static void sieve_residue_class_1 ()
{
    var pao = primes_and_offsets[1];

    for (int i = 0, e = pao.Length; i < e; ++i)
    {
        int p = pao[i].prime, j = pao[i].offset, dj = p << 3;

        if (j + dj + dj <= window_bits)
        {
            int b = j >> 3, db = p >> 3;

            switch (j & 7)
            {
                case 4:  sieve[b] |= 1 << 4;  j += p;  b += db + 0;  goto case 7;
                case 7:  sieve[b] |= 1 << 7;  j += p;  b += db + 1;  goto case 2;
                case 2:  sieve[b] |= 1 << 2;  j += p;  b += db + 0;  goto case 5;
                case 5:  sieve[b] |= 1 << 5;  j += p;  b += db + 1;  goto case 0;
                case 0:  sieve[b] |= 1 << 0;  j += p;  b += db + 0;  goto case 3;
                case 3:  sieve[b] |= 1 << 3;  j += p;  b += db + 0;  goto case 6;
                case 6:  sieve[b] |= 1 << 6;  j += p;  b += db + 1;  break;
            }

            for (int next_j = j + dj; next_j <= window_bits; j = next_j, next_j += dj, b += p)
            {
                sieve[b + 0 * db + 0] |= 1 << 1;
                sieve[b + 1 * db + 0] |= 1 << 4;
                sieve[b + 2 * db + 0] |= 1 << 7;
                sieve[b + 3 * db + 1] |= 1 << 2;
                sieve[b + 4 * db + 1] |= 1 << 5;
                sieve[b + 5 * db + 2] |= 1 << 0;
                sieve[b + 6 * db + 2] |= 1 << 3;
                sieve[b + 7 * db + 2] |= 1 << 6;
            }
        }

        for ( ; j < window_bits; j += p)
            sieve[j >> 3] |= (byte)(1 << (j & 7));

        pao[i].offset = j - window_bits;
    }
}

The effort just isn't worth it in most cases, and it exceeds the source code size limits for most tasks involving primes. Without such gargantuan efforts, a simple bool[] always wins. At least in C#, C++ and Delphi.

In C# I realised significant performance gains from allocating buffers (sieve, presieve pattern and so on) once in the constructor and using them over and over, instead of allocating and discarding as needed. Of course, this goes against the grain of languages like Scala but that's the reality of tasks like PRIME1 and language runtimes.

Moral: keep things simple and your caches hot.


methods in the class so close in what they do I have this class that has these two methods that are so closely related to the each other. I do not want to pass the flags so I kept them separate. I was wondering if there is a way to rewrite it so that I do not have to repeat so closely!

  class Test extends Controller
    {
        public static function nonFormattedData($param)
        {
            $arr = array();
            if (is_array($param)) {
                $i = 0;
                $sql = "
                    select *
                    from table1
                    where
                    ";

                if (isset($param['startDate'])) {
                    $sql .= "   date_created between ? AND  ?";
                    $arr[] = $param['startDate'];
                    $arr[] = $param['endDate'];
                    $i++;
                }
                if (isset($param['amount']) && !empty($param['amount'])) {

                    if ($i > 0) $sql .= " AND ";
                    $sql .= " balance= ?";
                    $arr[] = $param['amount'];
                    $i++;
                }
                if (isset($param) && !empty($param['amount'])) {

                    if ($i > 0) $sql .= " AND ";
                    $sql .= " balance= ?";
                    $arr[] = $param['amount'];
                    $i++;
                }
                if (isset($param['createdBy']) && !empty($param['createdBy'])) {

                    if ($i > 0) $sql .= " AND ";
                    $sql .= " column2 like '%Created By: " . $param['createdBy'] . "%'";
                }
                $sql .= ' group by id.table1 ';
                $rs = Query::RunQuery($sql, $arr);
                foreach ($rs as $row) {
                    $records = new Account();
                    $results[] = $records;
                }
                return $results;
            }
        }

        public static function formattedData($serArray, $orderBy = "giftcardaccount_id desc", $offset = 0, $limit = 10)
        {
                $arr = array();
            if (is_array($param)) {
                $i = 0;
                $sql = "
                    select *
                    from table1
                    where
                    ";

                if (isset($param['startDate'])) {
                    $sql .= "   date_created between ? AND  ?";
                    $arr[] = $param['startDate'];
                    $arr[] = $param['endDate'];
                    $i++;
                }
                if (isset($param['amount']) && !empty($param['amount'])) {

                    if ($i > 0) $sql .= " AND ";
                    $sql .= " balance= ?";
                    $arr[] = $param['amount'];
                    $i++;
                }
                if (isset($param) && !empty($param['amount'])) {

                    if ($i > 0) $sql .= " AND ";
                    $sql .= " balance= ?";
                    $arr[] = $param['amount'];
                    $i++;
                }
                if (isset($param['createdBy']) && !empty($param['createdBy'])) {

                    if ($i > 0) $sql .= " AND ";
                    $sql .= " column2 like '%Created By: " . $param['createdBy'] . "%'";
                }
                $sql .= ' group by id.table1 ';
                $rs = Query::RunQuery($sql, $arr);

                return array("data" => $rs);
            }
        }

    }

Just judging by the size you can tell that this one method is doing entirely too much. The Single Responsibility Principle states that a method/function should be responsible for one thing to the exclusion of all others. It can request other methods/functions to help it, but it should only be concerned with its purpose, which should be evident by its name.

So, the first thing I would say to do is break this up into even more methods first. The how should be pretty obvious in most cases. In this case, however, it may not be, so let's take a look at what you are doing to get an idea of how we should break this up.

Each if statement sets up an additional part of this growing SQL statement. If you look closely at each of these blocks, you may begin to notice a pattern. When a pattern arises we know that we can apply the "Don't Repeat Yourself" (DRY) Principle to it so that it can be reused. So our pattern here is to append a SQL statement, add to an array, and finally increment a counter. Sounds simple enough.

private
    $sql = '',
    $arr = array()
;

//constructor, etc...

private function append( $sql, $params ) {
    if( strlen( $this->sql ) ) {
        $sql = ' AND ' . $sql;
    }

    $this->sql .= $sql;
    $this->arr = array_merge( $this->arr, $params );
}

So, you may notice that we are doing a few things differently here. First, we add some new properties to our class. The $sql and $arr properties take the place of their respective variables. This is so that our new methods can communicate with each other. Next, we removed the counter $i. The counter is pretty much useless, even in its current form. If we remove the initial value of $sql then we can just check the string's length. If it is empty then of course we don't prepend an AND, but if it isn't then we should. Don't worry, we'll prepend the initial value when we are done. The last thing we do is merge our arrays. Since we are using a standard sequential array there should be no problems here, we just have to be sure to append everything in the right order, which we've just done. So, now to use it.

public function nonFormattedData( $param ) {
    if( ! is_array( $param ) ) {
        return FALSE;
    }

    if( isset( $param[ 'startDate' ] ) ) {
        $this->append( ' date_created between ? AND ?', array(
            $param[ 'startDate' ],
            $param[ 'endDate' ]
        ) );
    }

    //etc...

    $this->sql = '
        select *
        from table1
        where
        ' . $this->sql . '
        group by id.table 1 '
    ;

    //etc...
}

So, there's a few new things to cover here as well. The first is the removal of the static keyword. Static methods and properties are counter to OOP. For the most part you will probably never need them. But specifically in this case they prevent us from using our new method and properties. There are ways around this, but again, that is counter to OOP. The next thing I did was reversed your initial if statement and returned early. This is a good habit to get in to. First it reduces the amount of indentation, meaning we are no longer violating the Arrow Anti-Pattern (even as slight as the violation was), and second it reduces the amount of processing necessary for PHP to determine that it doesn't need to do anything. Not much mind, but still more efficient. The next if statement should look pretty familiar, its exactly the same, but the inner block is different. Here we are using the new method we just created. This should be pretty self-explanatory. Add the custom SQL and pass in the appropriate parameters. Repeat for each if statement. Finally we come to our new SQL statement. I promised you we'd get back to that initial value. Well, here it is, combined with appending the final value, resulting in our complete SQL statement, ready to use. The rest is the same.

Now, we've cleaned up this first method and, as you've already mentioned, the second method is pretty similar. If we apply the above to it it will be better, but we can still do more. As sky py has pointed out, the only difference is that foreach loop in the first method. The second method returns the results the first uses to loop over, or something VERY similar. So the obvious move here would be to call the second method from the first, and use the return to loop from. So our new first method should look like this.

public function nonFormattedData( $param ) {
    $results = array();

    $rs = $this->formattedData( $param );
    foreach( $rs[ 'data' ] AS $row ) {
        $records = new Account();
        $results[] = $records;
    }

    return $results;
}

The only thing new here is that I initialized the $results array, which you should always do before trying to manipulate it. There's a couple of reasons for this. The first, and most obvious, is what happens if $rs[ 'data' ] is empty? The loop never happens and $results is never defined. Resulting in an undefined error and a potential crash. The not so obvious is what would happen if $results was accidentally set somewhere previously, or god forbid, is a global. If that variable happens to be an array you'll just get some funny results. But if it happens to be a string you will be modifying the string using string-array notation, which will definitely cause problems.

So this answers your question, but still isn't as efficient as it could be. To be the most efficient we will want to limit how many times we query the server. Assuming that we always call the second method before the first, then we can have the second method save its results as a new property and access that property in the first method instead of querying the server again. Following that logic we can do the same thing to the first method and then use getters to return the appropriate property instead of calling those methods again and again.

I hope this helps. If you have any questions or need some clarification, don't hesitate to ask.


How to remove duplicate matching? There are my shapes (for example):

public abstract class Shape
{
  protected int _id;
  protected string _description;

  public abstract string ToXml();
}

public sealed class Triangle : Shape
{
  public int TriangleId { get { return _id; } set { _id = value; } }
  public string TriangleDescription { get { return _description; } set { _description = value; } }
  public ConsoleColor color { get; set; }

  public override string ToXml()
  {
    // First implementation
    throw new NotImplementedException();
  }
}

public sealed class Circle : Shape
{
  public int CircleId { get { return _id; } set { _id = value; } }
  public string CircleDescription { get { return _description; } set { _description = value; } }
  public int Radius { get; set; }

  public override string ToXml()
  {
    // Second implementation
    throw new NotImplementedException();
  }
}

public sealed class Square : Shape
{
  public int SquareId { get { return _id; } set { _id = value; } }
  public string SquareDescription { get { return _description; } set { _description = value; } }
  public int Edge { get; set; }

  public override string ToXml()
  {
    // Third implementation
    throw new NotImplementedException();
  }
}

Client have to choose one kind of shape to get xml-view:

public enum ShapeType
{
  Triangle,
  Circle,
  Square
}

public class Module
{
  public string GetXml( ShapeType type, int id )
  {
    switch( type )
    {
      case ShapeType.Circle:
        return DB.GetEntity( id ).ToTypedEntity<Circle>().ToXml();
      case ShapeType.Square:
        return DB.GetEntity( id ).ToTypedEntity<Square>().ToXml();
      case ShapeType.Triangle:
        return DB.GetEntity( id ).ToTypedEntity<Triangle>().ToXml();
    }
  }
}

As you can see, there are too much the same code parts that are matching enumerations with shape's types. It is not good.

How can I avoid it?

Just expose the class types to the user (which they already appear to be) rather than forcing them to use a gawd-awful enum.

public class Module
{
  public string GetXml<TShape>( int id ) where TShape : Shape
  {
    return DB.GetEntity( id ).ToTypedEntity<TShape>().ToXml();
  }
}

The the user can just make a normal call like

new Module().GetXml<Triangle>(34);


Recursive method to print a descending then ascending integer sequence From a programming assignment:

  Write a method writeSequence that accepts an integer n as a
  parameter and prints a symmetric sequence of n numbers with
  descending integers ending in 1 followed by ascending integers
  beginning with 1, as in the table below:

Call                        Output

writeSequence(1);             1
writeSequence(2);            1 1
writeSequence(3);           2 1 2
writeSequence(4);          2 1 1 2
writeSequence(5);         3 2 1 2 3
writeSequence(6);        3 2 1 1 2 3
writeSequence(7);       4 3 2 1 2 3 4
writeSequence(8);      4 3 2 1 1 2 3 4
writeSequence(9);     5 4 3 2 1 2 3 4 5
writeSequence(10);   5 4 3 2 1 1 2 3 4 5

  Notice that for odd numbers the sequence has a single 1 in the middle
  while for even values it has two 1s in the middle.

  Your method should throw an IllegalArgumentException if passed a
  value less than 1. A client using this method would have to call
  println to complete the line of output.

I wrote code that is pretty ugly, but produces the correct output. I've put this up to see if anyone had a more efficient recursive algorithm and to put the example up. There are not many hits about more complex recursive methods in Java when searching.

public static void writeSequence(int n) {

    if( n < 1 )
        throw new IllegalArgumentException();

    if( n == 1 ) {
        System.out.print("1");

    } else if( n == 2 ) {
        System.out.print("1 1");

    } else if( n % 2 == 0 ){
        System.out.print((n / 2) + " ");
        writeSequence(n - 2);
        System.out.print(" " + (n / 2));

    } else if( n % 2 == 1 ) {
        System.out.print(((n + 1) / 2) + " ");
        writeSequence((n - 2));
        System.out.print(" " + ((n + 1) / 2));
    }
}

Assuming you can use a helper function, you could do this:

  public static void writeSequence(int n) {
    if (n < 1) throw new IllegalArgumentException();
    writeSequenceHelper(n);
  }

  public static void writeSequenceHelper(int n) {
    if (n <= 0) return;
    int m = (n+1) / 2;
    System.out.print(m + " ");
    writeSequenceHelper(n - 2);
    System.out.print(m + " ");
  }

If you can't use a helper function like this to workaround the strange IllegalArgumentException requirement, you could do this (which is similar to your solution, but a little more succinct):

  public static void writeSequence2(int n) {
    if (n < 1) throw new IllegalArgumentException();
    if (n == 1) {
      System.out.print("1");
    } else if (n == 2) {
      System.out.print("1 1");
    } else {
      int m = (n+1) / 2;
      System.out.print(m + " ");
      writeSequence2(n - 2);
      System.out.print(" " + m);
    }
  }


Counting inversions in C++ via mergesort and vectors I have a text file that contains 100,000 numbers from 1 ~ 100,000 in an unsorted manner (no duplicates).

The name of the file is "IntegerArray.txt"

My task is to count the number of inversions in the text file.  An inversion is a pair of elements a, b where a comes before b, but a > b.  Thus "1 2 3 6 4 5" contains two inversion (6 > 4 and 6 > 5).

I implemented it with the merge sorting method.

The following is my code:

long long mergeAndCount(vector<int>& vec, int p, int q, int r) {
    long long count = 0;

    vector<int> L, R;
    for (int i = p; i <= q; ++i) L.push_back(vec[i]);
    for (int j = q + 1; j <= r; ++j) R.push_back(vec[j]);

    L.push_back(numeric_limits<int>::max()); //sentinel element
    R.push_back(numeric_limits<int>::max());

    int i = 0, j = 0;

    for (int k = p; k <= r; ++k) {
        if (L[i] <= R[j]) {
            vec[k] = L[i];
            ++i;
        } else {
            vec[k] = R[j];
            ++j;
            if (L[i] != L.back() && R[j] != R.back())
                count += q - p + 1 - i
        }
    }
    return count;
}

long long inversion(vector<int>& vec, int p, int r) {
    long long count = 0;

    if (p < r) {
        int q = (p + r) / 2;
        count = inversion(vec, p, q);
        count += inversion(vec, q + 1, r);
        count += mergeAndCount(vec, p, q, r);
    }

    return count;
}

int main() {
    ifstream infile("IntegerArray.txt");
    int a;
    vector<int> vec;

    while (infile >> a)
        vec.push_back(a);

    cout << inversion(vec, 0, vec.size()-1);
    return 0;
}

The result from the above code is 2407905288, which is correct.

The answer by brute force with the following code is 2407905288, which is correct. The following is a brute force version (it took me more than 10 minutes to run this code):

long long inversion(vector<int>& vec, int p, int r) {
    long long count = 0;

    for (int i = 0; i < vec.size(); ++i)
        for (int j = i + 1; j < vec.size(); ++j)
            if (vec[i] > vec[j])
                ++count;

    return count;
}

Any improvements in runtime or efficiency would be much appreciated.

What kind of efficiency?

You ask for efficiency, but don't clarify whether you want programmer or processor efficiency. And, at least sometimes, the two can both be improved simultaneously by leveraging the right pre-existing tools, so it's worth considering programmer efficiency.

Programmer efficiency comes in at least three general categories:

Writing less code:

I would be strongly tempted to confirm whether this can be written to leverage std::sort with a custom type that implements a counter in its overload to std::swap. If you can transform the problem like this, you don't have to worry about coding a merge sort correctly, and should have less code.
Writing clearer code:

The arguments to mergeAndCount are named vec, p, q, and r, and have no comments that clarify the meaning of p, q, and r (inversion's arguments p and r suffer similarly). Furthermore mergeAndCount's local variables are named L and R. What do these mean? The parameters appear to be local indices for special spots in the vector; perhaps they could be named accordingly.

As an extension of "naming things is important", consider also renaming inversion to use a verb instead of a noun. Something like invert is typically better than inversion; something that indicates what it's doing could be better still. Perhaps sortAndCount?

Sometimes avoiding raw loops can help. For example, the loops that fill L and R could be written with std::copy. But better yet, you could construct them in place with the range constructor:

vector<int> L(&vec[p], &vec[q + 1]);
vector<int> R(&vec[q + 1], &vec[r + 1]);

Two side comments here: your use of a final index rather than one past final (closed range instead of open range) conflicts with the standard library. If main called inversion with .size() instead of .size() - 1, and everything else was updated to match, a number of + 1 and/or - 1 items should disappear, starting with the three + 1 instances in my little code excerpt.

Second, I'm uncertain what I think of the use of a sentinel as opposed to verifying the index location. Doesn't it have a side effect that would harm lists with that number? While the current problem definition excludes use of the sentinel, problems have a tendency of changing over time; attempts to reuse code may result in using code that isn't suited for the new problem.
Making it harder to screw up:

The indices you pass into inversion and mergeAndCount represent very important values that never change. You should consider marking them const in order to avoid losing the ends of your ranges.

On the flip side, processor efficiency comes from many things that often compete with programmer efficiency. Here are a couple that could be relevant.

Avoiding allocations:

Often allocating in a loop is a source of run-time inefficiency. Note, for example, that std::merge requires input and output ranges, rather than allocating its own helper ranges internally. This allows std::sort to allocate them once and reuse them as necessary. (This comment conflicts with the above suggestion to use the range constructor.)
Memory access patterns:

Scanning through memory linearly is often the fastest way to access it. Your code does a pretty good job of this already, so without profiling there's little I can offer here.


SQL + VBA Populating Data in Cell Range I currently have a code that is connecting to SQL database in VBA. The data is populating correctly. However, I was wondering if there is a way to condense the code below.

I have 4 different columns and 26 rows and I feel like if I go this route, I will be wasting a lot of time. I want the range to go from C20:C45 and the results to show from cells H20:H45.

Sub ConnectSqlServer()

    Dim conn As ADODB.Connection
    Dim rs As ADODB.Recordset
    Dim sConnString As String
    Dim wks As Worksheet, xRow As Long

    ' Create the connection string.
    sConnString = "Provider=SQLOLEDB;Data Source=chocnt-285;" & _
                  "Initial Catalog=trackerfy2015;" & _
                  "Integrated Security=SSPI;"

    ' Create the Connection and Recordset objects.
    Set conn = New ADODB.Connection
    Set rs = New ADODB.Recordset
' Open the connection and execute data for WFTEs.
   Set rs = conn.Execute("SELECT sum(Hours)/80 FROM payroll2015_rif WHERE DepartmentCode = '" & Range("$E$6") & "' AND payperiod = '" & Range("C20") & "' and paycode IN ('REG1', 'REG2');")
        ' Transfer result.
        Sheets(2).Range("$H20").CopyFromRecordset rs
    ' Close the recordset
        rs.Close

' Open the connection and execute data for WFTEs.
   Set rs = conn.Execute("SELECT sum(Hours)/80 FROM payroll2015_rif WHERE DepartmentCode = '" & Range("$E$6") & "' AND payperiod = '" & Range("C21") & "' and paycode IN ('REG1', 'REG2');")
        ' Transfer result.
         Sheets(2).Range("$H21").CopyFromRecordset rs
 ' Clean up
    If CBool(conn.State And adStateOpen) Then conn.Close
    Set conn = Nothing
    Set rs = Nothing

End Sub

What you need is an ADODB.Command. It's the same query with different parameters, so you should use a parameterized query. Honestly, you should be using a parameterized query anyway. You have no idea what the user has put into those cells. This is a SQL Injection disaster waiting to happen. Let's say I put '; DROP DATABASE trackerfy2015 -- into cell E6. Boom. There goes your entire database. Poof. Gone. Up in smoke.

xkcd: Exploits of a Mom

So, about that parameterized query..

Dim cmd as New ADODB.Command

Const sql As String = & _
    "SELECT sum(Hours)/80 " & _
    "FROM payroll2015_rif " & _
    "WHERE DepartmentCode = ? " & _
        "AND payperiod = ? " & _
        "AND paycode IN ('REG1', 'REG2');"

With cmd
    .CommandType = adCmdText
    .CommandText = sql
    .ActiveConnection = conn

    .Parameters.Append .CreateParameter(Name, Type, adParamInput, Size, Range("$E$6").Value)
    .Parameters.Append .CreateParameter(Name, Type, adParamInput, Size, Range("C20").Value)

    Set rs = .Execute
End With

' Transfer Result
Sheets(2).Range("$H20").CopyFromRecordset rs

' reset the value of the second parameter
cmd.Parameters(1).Value = Range("C21")

Set rs = cmd.Execute

Note that I didn't set all of the parameters for the CreateParameter function because I have no way of knowing the appropriate values.

I need to mention something that you did very very right. Using Worksheet.CopyFromRecordSet is by and far the most efficient way to transfer data from a Recordset to a worksheet. Well done. You have no idea how often I see people loop through a recordset to populate a worksheet. I'm very happy to see you do it the right way.

Some other notes:

You have unused variables wks and xrow. Put the first to good use by setting it to Sheets(2). This way if the destination sheet should ever need to change, you only have to do it in one place. Remove the other dead variable.
Add an error handler so you can make sure the connection and recordset always get closed no matter what happens.


Go cookie authentication system I'm working on a login system in Go (Golang). Previously, I was using Gorilla Sessions for this, but I wanted to see if I could reinvent the wheel and make it simpler.

Also, I don't need to store many user values—just one: whether a user is logged in or not. For this, I decided to use a map.

sessions := make(map[string]bool)

When a user tries to login, this function gets called.

func tryLogin(username, password string) (http.Cookie, error) {
    if exists := db.UserExists(username, password); !exists {
        return http.Cookie{},
            errors.New("The username or password you entered isn't correct.")
    }

    sid, err := randString(32)
    if err != nil {
         return http.Cookie{}, err
    }

    sessions[sid] = true

    loginCookie := http.Cookie{
        Name:     "id",
        Value:    sid,
        MaxAge:   int((time.Hour * 12).Seconds()),
        HttpOnly: true,
        Domain:   "mydomain.com",
        Path:     "/admin/",
    }

    return loginCookie, nil
}

My randString function, which I use to generate a session ID, just reads random bytes and base64 encodes them.

func randString(size int) (string, error) {
    buf := make([]byte, size)

    if _, err := rand.Read(buf); err != nil {
        log.Println(err)
        return "", errors.New("Couldn't generate random string")
    }

    return base64.URLEncoding.EncodeToString(buf)[:size], nil
}

If a user exists in my database, then they get redirected to an admin area. One of the first things I do on every admin page is call sessionExists to make sure they're still logged in.

func sessionExists(req *http.Request) bool {
    cookie, err := req.Cookie("id")
    if err == http.ErrNoCookie {
        return false
    } else if err != nil {
        log.Println(err)
        return false
    }

    if _, exists := sessions[cookie.Value]; !exists {
        return false
    }

    return true
}

If sessionExists returns false, then I redirect the user to the login page.

I don't currently have HTTPS working, so I can't turn on the secure flag on my login cookie. But besides that, would this design provide adequate protection against intruders?

Concurrent use of map

Quote from Golang FAQ:

  Map access is unsafe only when updates are occurring. As long as all goroutines are only reading—looking up elements in the map, including iterating through it using a for range loop—and not changing the map by assigning to elements or doing deletions, it is safe for them to access the map concurrently without synchronization.

Since your functions modify sessions and are running in different routines you need to introduce some locking. Take look at sync package.

Also see question at StackOverflow.

sessionExists function

if _, exists := sessions[cookie.Value]; !exists {
    return false
}

return true

exists already holds the desired bool.
Let's simply this part and return exists directly.

_, exists := sessions[cookie.Value]
return exixts

Also you may take advantage of named return values and simplify your function:

func sessionExists(req *http.Request) (exists bool) {
    cookie, err := req.Cookie("id")
    if err == nil {
        _, exists = sessions[cookie.Value]
    } else if err != http.ErrNoCookie {
        log.Println(err)
    }

    return
}

Now let's take a look at func (*Request) Cookie source code:

func (r *Request) Cookie(name string) (*Cookie, error) {
    for _, c := range readCookies(r.Header, name) {
        return c, nil
    }
    return nil, ErrNoCookie
}

The only possible results are c, nil and nil, ErrNoCookie so there is no point in logging errors:

func sessionExists(req *http.Request) (exists bool) {
    cookie, err := req.Cookie("id")
    if err == nil {
        _, exists = sessions[cookie.Value]
    }

    return
}


Calculate percentage based on dummy variable in Pandas Today I had a simple task: I have products, quantities and a dummy, and I needed to know which percentage of the total quantities of that product the dummy represented. My DataFrame looked like this:

Product    Qty    Dummy
   A        10      0
   B        15      0
   B        5       1
   C        5       0
   D        5       0
   D       20       1

And I needed to get there:

Product    Qty_pct
   B        0.25
   D        0.8

So, I only needed the percentage when the dummy takes value = 1

I managed to do it, like this:

df2=df.pivot_table(columns='Dummy',index='Product',aggfunc='sum',values=['Qty']).reset_index()
df2['Qty_pct']=df2['Qty'][1]/(df4['Qty'][1]+df2['Qty'][0])
df2.columns=df2.columns.get_level_values(0)

To me it seems like a very indirect way to achieve my goal and I feel this can be done in a way more elegant way. What would you do?

I think the better way is to use groupby. It looks more logical and "natural":

df = pd.DataFrame({
    'Product': ['A', 'B', 'B', 'C', 'D', 'D'],
    'Qty': [10, 15, 5, 5, 5, 20],
    'Dummy': [0, 0, 1, 0, 0, 1]
})

# Create new column = Dummy*Qty
df['DQty'] = df['Dummy'] * df['Qty']

# Groupby df by 'Product' and summarize columns
df2 = df.groupby('Product').sum()

# Create new column equal to percentage of the total quantities
df2['Q'] = df2['DQty'] / df2['Qty']

# Drop unnecessary columns
df2 = df2.drop(columns=['Dummy', 'Qty', 'DQty'])

# Drop rows equal to zero
df2 = df2.loc[df2['Q'] != 0]
df2

The result is:

        Q
Product
B       0.25
D       0.80


Simple age converter I want you to pick my code apart and give me some feedback on how I could make it better or more simple. In my opinion this is the best code I have written, being that I'm new to programming. The program is a basic "converter" that has the user input his/her age and it tells how many days, hours, and minutes the user has been alive. Let me know what you think!

import time
import sys

def name():
    name = input("What is your name? ")
    while name.isdigit():
        print("Make sure to enter an actual name.")
        name = input("What is your name? ")
    return name

def user_input():
    while True:
        age = input("What is your age? ")
        try:
            return int(age)
            break
        except ValueError:
            try:
                return float(age)
                break
            except ValueError:
                print("Make sure to enter a real age.")

def again():
    while True:
        again = input("Play again? Yes/No ").lower()
        if again == "no":
            sys.exit(0)
        elif again == "yes":
            break
        else:
            print("That was not an option.")

while True:
    the_name = name()
    age = user_input()
    Days = age * 365
    Hours = Days * 24
    Minutes = Hours * 60
    print("Ok {}, I have your results.".format(the_name))
    time.sleep(2)
    print("If you are {} years of age....".format(age))
    time.sleep(2)
    print("You are {} days old.".format(Days))
    time.sleep(2)
    print("You are {} hours old.".format(Hours))
    time.sleep(2)
    print("You are {} minutes old!".format(Minutes))

    again()

NOTE: If you make a change, please explain to me why and how it works in the most simple way you can, so I understand it better.

This:

def name():
    name = input("What is your name? ")
    while name.isdigit():
        print("Make sure to enter an actual name.")
        name = input("What is your name? ")
    return name

means that "1" is not a real name, but "a" and "17xy" are. I think it might be better to use some kind of a regular expression, or - better - just accept whatever nonempty string the user chooses to give you as his/her name.

In your user_input(), you try to convert the input to int, and - if that fails - to float. Why not straight to float? I don't see how is 17 better than 17.0.

Also, in the same function, your break statements will never execute, as the return statement terminates the function.

I think it'd be a much better program if the input was date of birth. Do you know your current age, as anything more precise than "between x and x+1 years old"?


Get The Base Name of an Elixir Template File I've got the following Elixir code:

defmodule T do
    @eex_extension "eex"
    @file_separator "."
    def get_base_file_name_from_template_name(template_file_name) do
        [base, extension, @eex_extension] = String.split(template_file_name,@file_separator)
        base <> @file_separator <> extension
    end
end

It works like this:

iex(3)> f = T.get_base_file_name_from_template_name("index.html.eex")
"index.html"

1.) Is there a built-in library function I'm missing that would do this?

2.) Is there a built-in attribute for the eex file extension?

3.) Is there an OS built-in value for the "." BTW, I think file separator is not a great name for that but I can't come up with a better name.

Thoughts, comments, suggestions would be greatly appreciated. Even better yet if this is a built-in function in one of the Elixir libs and I've missed it a pointer to it would be most welcome.

Since you seem to know what the file extension is, you can use Path.basename/2.

"foo.html.eex"
  |> Path.basename(".eex")
  |> IO.puts

If you don’t know the file extension, you can use Path.extname/1 in conjunction.

filename = "foo.html.eex"
filename
  |> Path.basename(Path.extname(filename))
  |> IO.puts


Temperature conversion table This is another solution for this challenge.

Problem statement:

  In this challenge, write a program that takes in three arguments, a
  start temperature (in Celsius), an end temperature (in Celsius) and a
  step size. Print out a table that goes from the start temperature to
  the end temperature, in steps of the step size; you do not actually
  need to print the final end temperature if the step size does not
  exactly match. You should perform input validation: do not accept
  start temperatures less than a lower limit (which your code should
  specify as a constant) or higher than an upper limit (which your code
  should also specify). You should not allow a step size greater than
  the difference in temperatures.

I want to learn more about C++, so if there is some cool C++ feature I could should have used, please comment (or include it in your answer).

#import <iostream>
#import <cmath>

#define COLUMN_SEPARATOR "\t| "
#define MAX_TEMP 500
#define MIN_TEMP -500

inline bool between(double x, double max, double min) {
    return max >= x && min <= x;
}

void getInput(double &lower, double &upper, double &step) {
    double temp1, temp2;
    std::cout << "Please enter consecutively the upper and lower limits, both between " <<  MIN_TEMP << " and " << MAX_TEMP << "." << std::endl;
    std::cin >> temp1;
    std::cin >> temp2;
    while (!between(temp1, MAX_TEMP, MIN_TEMP) || !between(temp2, MAX_TEMP, MIN_TEMP)) {
        std::cout << "At least one of the temperatures is out of bounds. Please reenter:" << std::endl;
        std::cin >> temp1;
        std::cin >> temp2;
    }
    upper = std::max(temp1, temp2);
    lower = std::min(temp1, temp2);
    std::cout << "Please enter a positive stepsize, smaller than the difference between the limits." << std::endl;
    std::cin >> step;
    while (step < 0 || step > upper - lower) {
        std::cout << "The stepsize is out of bounds. Please reenter:" << std::endl;
        std::cin >> step;
    }
}

double toFahrenheit(double celsius) {
    return celsius*(9/5) + 32;
}

void printTable(double start, double end, double step) {
    std::cout << "Celsius" << COLUMN_SEPARATOR << "Fahrenheit" << std::endl;
    std::cout << "=======" << COLUMN_SEPARATOR << "==========" << std::endl;
    for (double i = start; i < end; i += step) {
        std::cout << i << COLUMN_SEPARATOR << toFahrenheit(i) << std::endl;
    }
}

int main() {
    double start, end, step;
    getInput(start, end, step);
    printTable(start, end, step);
    return 0;
}

Sample run:

192:Challenges 11684$ ./a.out Please enter consecutively the upper and lower limits, both between -500 and 500.
3.692
65.937
Please enter a positive stepsize, smaller than the difference between the upper and lower limit.
5.3729
Celsius   | Fahrenheit
=======   | ==========
3.692 | 35.692
9.0649    | 41.0649
14.4378   | 46.4378
19.8107   | 51.8107
25.1836   | 57.1836
30.5565   | 62.5565
35.9294   | 67.9294
41.3023   | 73.3023
46.6752   | 78.6752
52.0481   | 84.0481
57.421    | 89.421
62.7939   | 94.7939

Looks like there are no Americans here. 30 degrees Celsius is about 90 degrees Fahrenheit, so something must be wrong in the conversion. Indeed, (9/5) is evaluated (at compile time) as an integer division, yielding 1. Change it to (9.0/5.0).


OOP implementation of Rock Paper Scissors game logic in Java What is your opinion on my solution for Rock Paper Scissors game logic in Java?
Round outcome enum class
public enum Winer
{
    DRAW,
    PLAYER,
    COMPUTER;
}

Played moves enum class
public enum PlayedMove
{
    ROCK,
    PAPER,
    SCISSORS;
}

Game logic class
public class GameLogic
{
    public static Winer getWiner(PlayedMove player, PlayedMove computer)
    {
        if (player == computer)
        {
            return Winer.DRAW;
        }
        else if (isPlayerWiner(player, computer))
        {
            return Winer.PLAYER;
        }
        else
        {
            return Winer.COMPUTER;
        }
    }

    private static boolean isPlayerWiner(PlayedMove player, PlayedMove computer)
    {
        return ((player == PlayedMove.ROCK && computer == PlayedMove.SCISSORS)
             || (player == PlayedMove.SCISSORS && computer == PlayedMove.PAPER)
             || (player == PlayedMove.PAPER && computer == PlayedMove.ROCK));
    }
}

I don't see anything object-oriented in it. A program without a single non-static method can hardly be called OO.
The main task of this piece of code is to compare PlayedMove instances, so to follow OO style, you should make that comparison a method of PlayedMove. This will make the enum a bit more complex (beyond beginner level), so you might want to switch away from PlayedMove being an enum (or, use it as an exercise on complex enums).


JavaScript function for to find a defined element within a nested array Following task:
You've got an array which contains parts. Parts can contain subparts. Subparts can contain further subparts and so on.

For example:

computer

   cpu

   hardDisc

   graphicsCard

      graphicsCardCpu

      graphicsCardRam

Let's suppose I have a correct structure where every part has a unique id. Such a structure would be assured.

I've made this function which retrieves a part with a given id from the above described structure.

// Example structure
var structure = [
  {
    id : 1,
    name : 'firstLevelOne',
    category : 'alpha',
    subParts : null,
  },
  {
    id : 2,
    name : 'firstLevelTwo',
    category : 'beta',
    subParts : [{
      id : 6,
      name : 'secondLevelOne',
      category : 'alpha',
      subParts : null
    }],
  },
  {
    id : 3,
    name : 'firstLevelThree',
    category : 'alpha',
    subParts : [{
      id : 7,
      name : 'secondLevelTwo',
      category : 'gamma',
      subParts : [{
        id : 8,
        name : 'thirdLevelOne',
        category : 'beta',
        subParts : [ {
          id: 11,
          name : 'fourthLevelOne',
          category : 'alpha',
          subParts : null
        }]
      },
                  {
                    id : 10,
                    name : 'secondLevelFour',
                    category : 'beta',
                    subParts : null
                  }]
    }],
  },
  {
    id : 4,
    name : 'firstLevelFour',
    category : 'gamma',
    subParts : [{
      id : 9,
      name : 'secondLevelThree',
      category : 'alpha',
      subParts : null
    }],
  },
  {
    id : 5,
    name : 'firstLevelFive',
    category : 'beta',
    subParts : null,
  }
];

// #### Let's try this out ...
var part = getPart(4, structure);

for (var i in part) {
  console.log(part[i]);
}

console.log('\n');

part = getPart(11, structure);

for (var i in part) {
  console.log(part[i]);
}

console.log('\n');

part = getPart(8, structure);

for (var i in part) {
  console.log(part[i]);
}
// ############################

// The actual function ...
// Retrieves the object from
// the storage-array.
function getPart(id, arr) {
  var ret = null;
  var i;

  if (!id || !arr) return ret;

  for (i = 0; i < arr.length; i++) {
    if (arr[i]['id'] === id) {
      return arr[i];
    } else {
      if (arr[i]['subParts']) {
        ret = getPart(id, arr[i]['subParts']);

        if (ret) return ret;
      }
    }
  };

  return ret;
}

Are there weak points? Could the function be improved?

Your function is fine for me according to the structure you mentioned. But it's slow because it has to run a full search every time it's called. The main reason is the structure. So I would suggest that build an index which will help you to search quicker.

Building an index

An index is simply a map associating each id to the corresponding Part. You will only need to fill it once then use it to access Parts by id more quickly.

var index = {};
fillIndexFrom(index, structure);

function buildIndexFrom (index, list) {
    list.forEach(function(part) {
        index[part.id] = part;
        if (part.subParts)
            fillIndexFrom(index, part.subParts);
    });
}

// Then to get the part of id 4 for example you simply do
var part = index[4];

Complexity comparison

If we assume that there are N parts and that you will do M searches then:

Your initial solution will cost O(M*N) (every search costs O(N) in worst case)
The index solution will cost O(N) + M * O(logN) (O(N) to fill the index and then O(logN) for every search)


String parsing and saving ipaddress to file in Java Following function will parse string resp, which will be like shown in following example. I will get an IP address in the form of a hexadecimal string like "C0A2AA32" and I need to convert that string and save it to a file.

Example of resp: "This is just a string with ip="2342A022" now I need to parse it"

void WritePublicIPAddressToFile(String resp)
{
    //find string in response
    String stringToFind = "ip=";

    //get index of stringToFind
    int indexOfIP = resp.indexOf(stringToFind);

    String newString = "";

    //find exact ip address which will be after = in stringToFind
    for(int i = indexOfIP; i<resp.length(); i++)
    {
        while(resp.charAt(i++) != '\"');

        while(resp.charAt(i) != '\"')
        {
            newString += resp.charAt(i++);
        }

        break;
    }

    int lenNewString = newString.length();
    String newIPAddress = "";

    //get two bytes from hex string and convert to ipaddress
    if(lenNewString == 8) //lenth of ipaddress raw data should be 8
    {
        long l;
        for(int i = 0;i<lenNewString;i+=2)
        {
             l = Long.parseLong(String.format("%c%c", newString.charAt(i),newString.charAt(i+1)),16);
             newIPAddress += String.format("%d.", l);
        }
    }

    StringBuilder sb = new StringBuilder(newIPAddress);
    sb.deleteCharAt(sb.length() - 1);//remvoe last '.' in ipaddress

    //final ipaddress to save in file
    newIPAddress = stringToFind + "\"" + sb.toString() + "\"";

    //write IP Address to file

    File file = new File("SomeFilePath.txt");
    FileWriter fileWriter;

    try {

        fileWriter = new FileWriter(file);
        fileWriter.write(newIPAddress);

        fileWriter.flush();
        fileWriter.close();

    } catch (IOException e2) {

        e2.printStackTrace();

    }
}

For starters you can simplify the first part by utilizing the fact that indexOf can accept a second character indicating start point. This would lead to something like the following:

// Find indices for IP-address
int startIndex = resp.indexOf('\"', resp.indexOf("ip=")) + 1;
int endIndex = resp.indexOf('\"', startIndex);

String ipString = resp.substring(startIndex, endIndex);

Secondly, you can simplify the conversion by utilizing the substring method a little, and improve a little on performance by using StringBuilder, like in the following:

StringBuilder ipAddress = new StringBuilder();
int lenIpString = ipString.length();

//get two and two bytes from hex string and append to ipaddress
if (lenIpString == 8) {
    long ipPart;
    for(int i = 0; i < lenIpString; i += 2)
    {
         ipPart = Long.parseLong(ipString.substring(i, i + 2), 16);
         ipAddress.append(String.format("%d.", ipPart));
    }

} else {
    throw new Exception("IP-string not long enough");
}

Note that this also does something if in the rare case the ipString is not 8 characters long. This also requires for the method to have an throws Exception added, and you should probably choose a better exception rather than Exception.

Finally your write to file can be enhanced to the following:

//write IP Address to file
File file = new File("SomeFilePath.txt");
FileWriter fileWriter;

try {
    fileWriter = new FileWriter(file);
    fileWriter.write(
        String.format("ip=\"%s\"",
                      ipAddress.substring(0, ipAddress.length() - 1)));

    fileWriter.flush();
    fileWriter.close();

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

Finally some general notes on coding style:

Consider splitting into three functions – These three code blocks could be extracted into (two or three) separate functions, where one serves as the output to the next. This would follow the single-responsibility principle of coding. One variant could change this function into:

void WritePublicIPAddressToFile(String resp, String filename)
{
   String ipAddress = findAndConvertIPAddress(resp);

   if (!ipAddress.isEmpty()) {
       writeIPAddressToFile(filename, ipAddress);
   }
}

Here I assume that the findAndConvertIPAddress() would return the empty string if the IP-address is not found as expected, and use that to bypass the file write if necessary. Now this function has a much clearer responsibility of finding and writing that IP-address to the file. No need to read lots of code to understand whats happening.
Use StringBuilder instead of String concatenation – In general try to avoid building new strings to a minimum, and use StringBuilder when building new strings, or a variation over String.format().
Consider having the filename as a parameter – Your code might be constructed specific for Code Review, but with a name like WritePublicIPAddressToFile I would expect that filename was one of the parameters.
Avoid using l as variable name – Especially in combination with i or 1, it can be really confusing to use lower-case L as a variable name. Use more descriptive names instead.


Compare ZIP codes, disregarding leading zeros I have a function that compares ZIP codes. It also has to handle cases where the input ZIP code be missing leading zeros, have extraneous leading zeros. This is what I've come up with. I don't have any issues with it. But since it's in Scala, I feel there must be a more functional way to write this.

My only functional thought (outside of the digit validation) is to insert them into a collection, then filter the collection by length != 5, and then attempt to fix them.

Here's my imperative, stateful version.

  def compareZip(first: String, second: String): Boolean = {
    var firstTmp: String = null
    var secondTmp: String = null
    if(!first.forall(_.isDigit) || !second.forall(_.isDigit)) false
    else if (first.length == ZIPCODE_LENGTH && first.length == second.length) {
      first equals second
    }
    else {
      if (first.length > ZIPCODE_LENGTH) {
        firstTmp = first takeRight ZIPCODE_LENGTH
      }
      else if (first.length < ZIPCODE_LENGTH) {
        firstTmp = ZERO_STR * (ZIPCODE_LENGTH - first.length) + first
      }
      else firstTmp = first

      if (second.length > ZIPCODE_LENGTH) {
        secondTmp = second takeRight ZIPCODE_LENGTH
      }
      else if (second.length < ZIPCODE_LENGTH) {
        secondTmp = ZERO_STR * (ZIPCODE_LENGTH - second.length) + second
      }
      else secondTmp = second
      firstTmp equals secondTmp
    }
  }

The title of your post is

  Compare ZIP codes, disregarding leading zeros

The solution is simple -- just drop the leading zeros.  Then you can compare the strings directly.

This leads us to something like this:

def compareZip(first: String, second: String): Boolean = {
  first.dropWhile(c => c == '0') == second.dropWhile(c => c =='0')
}

If you want, you can factor out the common lambdas.

There's no need to do anything more complicated than that.  "Fixing" the zipcode only makes things more complicated.  And, since the result never leaves the comparison function anyway, all that work is wasted.


MVVM pattern in Xamarin login form Please let me know if the MVVM pattern I followed in this is correct or not.

Here is the code for ViewModel

public class LoginViewModel
{
    public string Email { get; set; } = "";
    public string Password { get; set; } = "";

    public ProgressBar ProgressBar { get; set; }
    public Label StatusLabel { get; set; }

    public Command LoginCommand
    {
        get
        {
            return new Command(async()=> {
                ProgressBar.IsVisible = true;
                ProgressBar.Progress = 0;
                await ProgressBar.ProgressTo(0.2, 250, Easing.BounceOut);

                if(Email.Length>0 && Password.Length > 0)
                {
                    ApiServices apiServices = new ApiServices();
                    var isSuccess = await apiServices.LoginAsync(Email, Password);
                    if (isSuccess)
                    {
                        await ProgressBar.ProgressTo(1, 250, Easing.Linear);
                        Application.Current.MainPage = new FirstMasterDetailPage();
                    }
                    else
                    {
                        await ProgressBar.ProgressTo(1, 250, Easing.Linear);
                        StatusLabel.IsVisible = true;
                        StatusLabel.Text = "Failed to login.";
                    }
                }
                else
                {
                    await ProgressBar.ProgressTo(1, 250, Easing.Linear);
                    StatusLabel.IsVisible = true;
                    StatusLabel.Text = "Username and Password is required to login.";
                }
                ProgressBar.IsVisible = false;
            });
        }
    }

    public Command RegisterCommand
    {
        get
        {
            return new Command(()=> {
                Application.Current.MainPage = new RegisterPage();
            });
        }
    }
}

this is code behind page

public LoginPage ()
    {
        InitializeComponent ();

        var current = BindingContext as LoginViewModel;
        current.ProgressBar = progressBar;
        current.StatusLabel = lblStatus;
    }

No, it's not correct.

View-model shouldn't contain anything related to view. It means LoginViewModel shouldn't contain references on progress bar and label. ProgressBar and Label properties inside a view-model violate MVVM.
Any changes in UI elements should be done via bindings specified in XAML. Those bindings have to be linked to properties of a view-model. Changing these properties, UI get updated.
To make bindings work you should notify them about changes in view-model. For that purpose you need to implement INotifyPropertyChanged interface on the view-model and raise PropertyChanged event every time a property changed.

Let's see example of how you can remove ProgressBar from the LoginViewModel.

First of all, view -model should implement INotifyPropertyChanged interface which contains PropertyChanged event:

public class LoginViewModel : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler PropertyChanged;
}

Then remove ProgressBar property and add LoginInProgress:

private bool _loginInProgress;

public bool LoginInProgress
{
    get => _loginInProgress;
    set
    {
        if (value == _loginInProgress)
            return;

        _loginInProgress = value;
        PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(LoginInProgress)));
    }
}

Now in the LoginCommand body instead of

ProgressBar.IsVisible = true;

write

LoginInProgress = true;

And instead of

ProgressBar.IsVisible = false;

write

LoginInProgress = false;

Then in XAML bind Visibility of the progressBar to this property:

<ProgressBar x:Name="progressBar"
             Visibility="{Binding Path=LoginInProgress, Mode=OneWay, Converter="{StaticResource BooleanToVisibilityConverter}"}"
             .../>

Somewhere in resources you should define BooleanToVisibilityConverter:

<BooleanToVisibilityConverter x:Key="BooleanToVisibilityConverter"/>

And that's all. You need to eliminate all the view related stuff from the view model in such way. It is how MVVM have to be implemented.


Object to query string An object (POJO) holds query arguments and has two-binding to a form using some MVC framework. later the object needs to be converted to a query string that will be appended to an HTTP request.

function objectToQueryString(obj) {
        return Object.keys(obj)
            .filter(key => obj[key] !== '' && obj[key] !== null)
            .map((key, index) => {
            var startWith = index === 0 ? '?' : '&';
            return startWith + key + '=' + obj[key]
        }).join('');
    }

Any pitfalls?  IE9+ is what we support.

Example input:

{isExpensive:true, maxDistance:1000,ownerName:'Cindy',comment:''}

And its expected output:

?isExpensive=true&maxDistance=1000&ownerName=cindy

You have neglected to escape your keys and values. If any of the data contains a special character such as &, the generated URL will be wrong.

To perform the escaping, I recommend calling encodeURIComponent().