Counting consonants and vowels I'm new to Python and want to develop good habits at the beginning. This class takes a string, and then counts the number of vowels and consonants in the word(s).  Any advice on how to improve?

class CntVowels:

    def __init__(self,string):
        self.string = string
        self.vowels = [ "a", "e", "i", "o", "u"]

    def vowel_count(self):
        strings = self.string.lower()
        vowels = self.vowels

        counter = 0
        for letter in strings:
            for vowel in vowels:
                if letter == vowel:
                    counter += 1
        print("There are {} vowels in this string.".format(counter))

    def constant_count(self):
        strings = self.string.lower()
        vowels = self.vowels
        length = len(strings)
        counter = 0
        for letter in self.string:
            for vowel in vowels:
                if letter == vowel:
                    counter += 1

        results = length - counter
        print("There are {} constants in this string.".format(results))

a = input("enter a string")

CntVowels(a).vowel_count()

CntVowels(a).constant_count()

Welcome to Python, and welcome to Code Review.

Classes

An instance of a class, where only one method is ever called, is not a good class.  Instead, use functions.

a = input("Enter a string: ")
vowel_count(a)
constant_count(a)

Class Constants & Members

If you do create a class, and that class has some global constant data, store that on the class itself, not on the instances of the class.

Constants should have UPPERCASE_NAMES, to distinguish them from non-constant data.  Moreover, if the data is constant, where possible use non-mutable objects, such as tuple (eg, (..., ..., ...)) over list (eg, [..., ..., ...]).

Non-public members of a class should begin with a single underscore.

class CntVowels:

    VOWELS = ("a", "e", "i", "o", "u")

    def __init__(self, string):
        self._string = string

    # ...

Eschew .lower()

When attempting to compare strings (or characters) case-insensitively, use .casefold() instead of .lower().  This properly handles many additional oddities, such as the German ß which is equivalent to ss.

x in y

Testing for existence of an item in a container is easily done with the in operator.  For example, this code:

for vowel in vowels:
    if letter == vowel:
        counter += 1

can be re-written more efficiently as:

if letter in vowels:
    count += 1

A String is a Container

It could even be re-written as:

if letter in "aeiou":
    count += 1

since a string is simply a container of letters.

Non-Vowels are not necessarily Consonants

Many characters which are not vowels are also not consonants, such as 4, &,  and . to name a few.

Avoid Name Shadowing

string is not a good variable name, because a module named string exists, and can be imported, and doing so results in the meaning of string changing depending on what is in scope.

It is even reasonable that you’d import string in this program, to get convenient access to all of the string.ascii_lowercase letters.


Determine value in range, inject to HTML to be used in CSS I want to show users what package they are, based on how many items they have in their cart. The code works, but I'm wondering if anything can be improved.

function new_thing (){

//decleare variables
global $x_subs;
$lim01 = get_option(woomps_limit01);
$lim02 = get_option(woomps_limit02);
$lim03 = get_option(woomps_limit03);
$x = $x_subs;
$plugin_url =(plugins_url("/", __FILE__ ));

//set CSS class based on value of $x
if  ($x >= 1 && $x < $lim02) {
    $active1 = "fb-sub-active";
    $active2 = "fb-sub-deactive";
    $active3 = "fb-sub-deactive";
} else if ($x >= $lim02 && $x < $lim03) {
    $active1 = "fb-sub-deactive";
    $active2 = "fb-sub-active";
    $active3 = "fb-sub-deactive";
} else if ($x >= $lim03) {
    $active1 = "fb-sub-deactive";
    $active2 = "fb-sub-deactive";
    $active3 = "fb-sub-active";
} else {
    $active1 = "fb-sub-deactive";
    $active2 = "fb-sub-deactive";
    $active3 = "fb-sub-deactive";
}

//generate the content that will be displayed
$content = <<<EOD

{$x_subs}

EOD;

return $content;
}

.fb-sub-img-holder {
    width: 95%;
    margin: 0 auto;
}

.fb-img {
    width: 30%;

.fb-sub-active {
    width: 50%;
}
.fb-sub-deactive {
    width: 20%;
}

Why is $lim01 never used?  Is it always equal to 1?

Only one of the subscription packs applies, if at all.  You should first make it clear which subscription pack applies.

$subscription_pack = (($x >= $lim03) ? 3 :
                     (($x >= $lim02) ? 2 :
                     (($x >= $lim01) ? 1 : NULL)));
# Note: inactive rather than deactive
$active1 = $active2 = $active3 = 'fb-sub-inactive';
if ($subscription_pack == 1) { $active1 = 'fb-sub-active'; }
if ($subscription_pack == 2) { $active2 = 'fb-sub-active'; }
if ($subscription_pack == 3) { $active3 = 'fb-sub-active'; }

I also suggest reworking the CSS so that you can drop the fb-sub-inactive class altogether.


Writing strings to a file This opens a file and writes 5120 times a string to it, and if the file doesn't exist it creates a new file. The name of the file and the string are chosen randomly. I would like to know if there is a way to speed up this code. It takes about 10 seconds at the moment. I was wondering if I did a recursion in the escreve() function or if I create some space in memory with malloc() on the main and free it before each cycle. If it will speed the thing up.

#define linhas 1024
#define vezes 5120
char *ficheiros[5]={"SO2014-0.txt","SO2014-1.txt","SO2014-2.txt","SO2014-3.txt","SO2014-4.txt"};
char *cadeias[10]={"aaaaaaaaa\n","bbbbbbbbb\n","ccccccccc\n","ddddddddd\n","eeeeeeeee\n","fffffffff\n","ggggggggg\n","hhhhhhhhh\n","iiiiiiiii\n","jjjjjjjjj\n"};

int random(int range)
{
    int num;
    num = rand() % range;
    return num;
}

void escreve(char *fi,char *cad){
    int l;
    int file = open(fi, O_WRONLY | O_CREAT, S_IRWXU | S_IROTH ) ;
    for(l=0;l<linhas;l++)
        write(file,cad, strlen(cad));
    close(file);
}

int main(){
    int x,y,l,i;
    srand(time(NULL));
for(i=0; i<vezes; i++){
    x= random(5);
        y=random(10);
        escreve(ficheiros[x],cadeias[y]);

}
return 0;
}

This is a very instructive example of wrong judgement. I understand (correct me if I am wrong) that you went for a low-level write to avoid seemingly unnecessary overhead of FILE * functions offered by a stdio library. Not only it made your code less portable, it in fact made id slower!

The net effect is that your code invokes a system call for each 10 characters.

System call is an extremely expensive operation. It involves switching memory maps from a user to a kernel space, and on top of that it faces who knows how many cache misses. This is exactly the problem stdio solves by internal buffering.

This is how your original code behaves:

~/projects/play/cr/pedro $ time ./original

real    0m7.929s
user    0m0.484s
sys     0m7.432s

I changed opening a file to

FILE * file = fopen(fi, "w");

and writing to it to

    fputs(cad, file);

and the new timigs are

~/projects/play/cr/pedro $ time ./with-stdio

real    0m0.908s
user    0m0.276s
sys     0m0.584s

PS To make it a code review:

You shall #include at least <sys/types.h>, <sys/stat.h>, <fcntl.h> and <time.h>.


d3.js realtime streamgraph memory usage This page (full source & demo) displays a 2-channel horizontal d3.js Streamgraph that takes realtime mouse coordinates as data inputs for the graph.

However, it tends to gradually use up an increasing amount of memory until there's none left, both on Chrome and Firefox (on Ubuntu 12.04 x64).  Data is being collected but not released, yet the arrays that hold the incoming mouse coord data are being popped for every push, and I'm missing where the problem is occuring.

I'm working on profiling it, but wondering if any JS experts can eyeball it and see it.  The only dependencies are JQuery and D3.js v3.

I am happy your problem is solved. All in all, your code looks solid, my review is mostly nitpicking:

I am all for spartan coding, but you have far too many 1 and 2 character variables, you ought to use more meaningful variables.
Try to have 1 var block at the top. More specifically, you declare data0 after declaring the function stream_layers, there is no need for that.
Please use lowerCamelCase coding, so update_layers -> updateLayers
There is no need to call .mouseover(); after setting the listener, especially since you also call .mouseout(); afterwards for which there is also no need as you already set x and y to 1 with var currentMouseCoord = { x: 1, y: 1 };
You can drop curly braces after loops, please don't drop newlines.
vis.selectAll("path").data(data0).attr.... in transition() really should be broken out into multiple lines.


Reading input from file for bridge simulation program I am simulating a bridge where only one car can cross at a time. The bridge runs north south. The car arrival times and times taken to cross the bridge are given in an input file. The format is direction|arrival time,time taken to cross. The possible directions are north and south, represented by N/n and S/s respectively. An upper case letter means high priority and a lower case letter means low priority. The arrival times and time to cross are can have a value after the decimal e.g. 10.5
Examples:

N|6.0,10.0
n|4.5,5.5

Here is my function, which is pretty ugly but works:
void getInput(char * inFile)
{
    enum direction_t {NORTH, SOUTH};
    direction_t direction;
    enum priority_t {HIGH, LOW};
    priority_t priority;
    string line;
    ifstream inFile(inFile);
    if(!inFile.good())
    {
        cerr << "can't open input file";
        exit(1);
    }
    while(getline(inFile, line))
    {
        char first = line.at(0);//first letter signifies direction and priority
        if(first == 'n')
        {
            direction = NORTH;
            priority = LOW;
        }
        else if(first == 'N')
        {
            direction = NORTH;
            priority = HIGH;
        }
        else if(first == 's')
        {
            direction = SOUTH;
            priority = LOW;
        }
        else if(first == 'S')
        {
            direction = SOUTH;
            priority = HIGH;
        }
        else
        {
            cerr << "invalid direction given: " << first << endl;
            //exit(1);
        }
        size_t commaIndex = line.find(",", 2);//know first character is letter, second is |
        string firstNumber = line.substr(2, commaIndex-2);
        string secondNumber = line.substr(commaIndex+1, line.length());
        cout << "firstNumber: " << firstNumber << " secondNumber " << secondNumber << endl;
    }

I was wondering if the way I use enum is good. Any tips for making code more readable and more compact are appriaciated.

Parameter shadowing a local variable:

The first issue with your code that prevented in from compiling with Clang is the
inFile parameter of getInput() that shadows the ifstream instance of the same name.

Your compiler seems to be more forgiving with that problem. If you are using GCC, I recommend using the -Wshadow warning flag to enable warnings of this kind of variable name shadowing/redeclaration. This category of bugs can be particularly hard to track in a scenario where both variables are of the same type.

Reserved suffix:

The _t suffix is reserved by the POSIX standard (read this), so it is best if avoided.
Consider renaming direction_t and priority_t to Direction and Priority.

Const correctness:

The filename parameter of the function is a read-only string, so it should at least
be a const char *. But since this is C++ code, prefer a standard string and pass it by const reference:

void getInput(const std::string & filename)

Error handling:

You did right by using std::cerr to report errors, however, exit()ing after the error is logged
seems like a very crude way of handling it. Consider throwing an exception or even
making the function return a boolean false.

Using namespace:

You are probably using namespace std somewhere, judging by the lack of the std:: prefix in the library calls. This is best if avoided as it defeats the purpose of a namespace, which is allowing equal names to coexist without clashes. Read this thread for a more in-depth discussion.

Data driven solution:

There is a more data driven and flexible way of handling the conversion between the chars
read from file to the Direction/Priority enums than an if chain or a switch statement.

You could instead use a map keyed by a char. This way, expanding and adding more chars
is simpler and involves less code change. You could even externalize the map to a file
and load it dynamically at runtime if you wanted.

Following is the refactored code with the changes above mentioned applied to it plus
the use of a std::map as a lookup table. The map associates a char (the key)
with a pair of [Direction, Priority]. Making the conversion form the file data a lot more concise.

#include <iostream>
#include <fstream>
#include <string>
#include <map>

bool getInput(const std::string & filename)
{
    enum Direction { NORTH, SOUTH };
    enum Priority  { HIGH,  LOW   };

    const std::map<char, std::pair<Direction, Priority>> lookupTable = {
        { 'n', { NORTH, LOW  } },
        { 'N', { NORTH, HIGH } },
        { 's', { SOUTH, LOW  } },
        { 'S', { SOUTH, HIGH } }
    };

    Priority    priority;
    Direction   direction;
    std::string line;

    std::ifstream inFile(filename);
    if (!inFile.good())
    {
        std::cerr << "can't open input file";
        return false;
    }

    while (getline(inFile, line))
    {
        const char key = line.at(0); // first letter signifies direction and priority

        const auto entry = lookupTable.find(key);
        if (entry != lookupTable.end())
        {
            direction = entry->second.first;
            priority  = entry->second.second;
        }
        else
        {
            std::cerr << "invalid direction given: " << key << std::endl;
        }

        size_t commaIndex = line.find(",", 2); // know first character is letter, second is |
        std::string firstNumber  = line.substr(2, commaIndex - 2);
        std::string secondNumber = line.substr(commaIndex + 1, line.length());
        std::cout << "firstNumber: " << firstNumber << " secondNumber " << secondNumber << std::endl;
    }

    return true;
}

So it turn out that the OP's compiler is missing a few C++11 features that prevent the code provided above from compiling. It can be fairly easily adapted to support older compilers though, if we are willing to add a few not so elegant workarounds. Here is one way to adapt it using an aux function that keeps a static std::map:

enum Direction { NORTH, SOUTH, DIR_INVALID };
enum Priority  { HIGH,  LOW,   PRI_INVALID };

std::pair<Direction, Priority> lookup(char key)
{
    typedef std::map< char, std::pair<Direction, Priority> > TableType;

    static bool isTableInitialized = false;
    static TableType lookupTable;

    // One time initialization
    if (!isTableInitialized)
    {
        lookupTable.insert(std::make_pair( 'n', std::make_pair(NORTH, LOW)  ));
        lookupTable.insert(std::make_pair( 'N', std::make_pair(NORTH, HIGH) ));
        lookupTable.insert(std::make_pair( 's', std::make_pair(SOUTH, LOW)  ));
        lookupTable.insert(std::make_pair( 'S', std::make_pair(SOUTH, HIGH) ));
        isTableInitialized = true;
    }

    TableType::const_iterator entry = lookupTable.find(key);
    if (entry != lookupTable.end())
    {
        return (*entry).second;
    }

    // Not found / invalid key
    return std::make_pair(DIR_INVALID, PRI_INVALID);
}

bool getInput(const std::string & filename)
{
    Priority    priority;
    Direction   direction;
    std::string line;

    std::ifstream inFile(filename.c_str());
    if (!inFile.good())
    {
        std::cerr << "can't open input file";
        return false;
    }

    while (getline(inFile, line))
    {
        // first letter signifies direction and priority
        const char key = line.at(0);

        // Lookup the conversion table:
        const std::pair<Direction, Priority> entry = lookup(key);

        if ((entry.first != DIR_INVALID) && (entry.second != PRI_INVALID))
        {
            direction = entry.first;
            priority  = entry.second;
        }
        else
        {
            std::cerr << "invalid direction given: " << key << std::endl;
        }

        size_t commaIndex = line.find(",", 2); // know first character is letter, second is |
        std::string firstNumber  = line.substr(2, commaIndex - 2);
        std::string secondNumber = line.substr(commaIndex + 1, line.length());
        std::cout << "firstNumber: " << firstNumber << " secondNumber " << secondNumber << std::endl;
    }

    return true;
}

It has its drawback, but should work just the same. It is also quite a bit more code to get it done. One important thing to keep in mind is that initialization order of the lookup table is not thread safe, so don't try to use this in a concurrent program.


Delete all instances of "b" or "ac" from a string A recent question, inspired me to think about how far we've come, and how much easier tasks are in current, modern languages like C# and Java, compared to the low-level languages we were stuck with years ago.

Based on that, I decided to post some code for doing the same task, but in a language that (at least in most people's estimation) has been obsolete for decades. In this case, I chose an ancient language named SNOBOL 4 for the implementation.

Warning: this really is an old language (this year is its 50th anniversary), so try not to be terribly surprised at the length and complexity of the code compared to modern languages like C# and Java.

    * read a string from standard input
    input_string = input

* find "b" or "ac" in s, replace with nothing, and repeat if successful:
del_loop input_string ("b" | "ac") =     : s(del_loop)

    * write out resulting string
    output = input_string

I don't think anybody's posted any SNOBOL 4 here before, so please be gentle...

As best I can tell (with the limited info available on the language), it looks like you're violating just a couple language concerns.

First: all example SNOBOL4 code I've seen has the lines without labels indented past the horizontal extent of the longest label by at least 2 spaces. That is:

    input_string = input
del_loop input_string ("b" | "ac") =     : s(del_loop)
    output = input_string

Seems to be expected to be:

          input_string = input
del_loop  input_string ("b" | "ac") =     : s(del_loop)
          output = input_string

It also looks like SNOBOL4 labels are expected to be SHOUTYLABELS:

         input_string = input
DELLOOP  input_string ("b" | "ac") =     : s(DELLOOP)
         output = input_string

The next thing I seem to have seen is that output, input, and s are supposed to be SHOUTY.

         input_string = INPUT
DELLOOP  input_string ("b" | "ac") =     : S(DELLOOP)
         OUTPUT = input_string

The next comment is that it looks like SNOBOL4 code uses PascalCase subject (variable) names.

         InputString = INPUT
DELLOOP  InputString ("b" | "ac") =     : S(DELLOOP)
         OUTPUT = InputString

Next, it looks like the colons should line up vertically and be past the last horizontal character on any line. This doesn't apply to you, unless we include your comments:

         * read a string from standard input
         InputString = INPUT

* find "b" or "ac" in s, replace with nothing, and repeat if successful:
DELLOOP  InputString ("b" | "ac") =                                       : S(DELLOOP)

         * write out resulting string
         OUTPUT = InputString

Lastly, it looks like the transfer is supposed to directly follow the colon. (No spaces.)

         InputString = INPUT
DELLOOP  InputString ("b" | "ac") =     :S(DELLOOP)
         OUTPUT = InputString

Now, that looks like SNOBOL4. :) All-in-all, it looks quite good. Very nice to see an application of a very old language, that shows that sometimes the new ones just aren't the best suited for the task. (In fact, SNOBOL4 was designed to fulfill programmes with this in mind, it was designed to handle pattern-matching and string-processing very well.)


Synchronizing scrollbars, scrollTop and scroll event loops I've been playing around synchronizing two scrollbars on a web application using JavaScript and jQuery (1.11.3). So far what I've been doing is any time a scroll event happens on one div, I use scrollTop to adjust the position on the other div:

$left = $("#left")
$right = $("#right")

$left.scroll(function () {
    $right.scrollTop($left.scrollTop());
});

$right.scroll(function () {
    $left.scrollTop($right.scrollTop());
});

(jsFiddle)

The problem comes from scrollTop triggering a scroll event on the element it's called from, creating a loop and making it misbehave on some browsers.

JavaScript events are queued, so toggling a flag before and after a call to scrollTop is not working (and could possibly backfire). So I came up with this solution:

$left = $( "#left" )
scrollLeft = []

$right = $( "#right" )
scrollRight = []

$left.scroll( function () {
    if ( !scrollRight.pop() ) {
        scrollLeft.push( true );
        $right.scrollTop( $left.scrollTop() );
    }
} );

$right.scroll( function () {
    if ( !scrollLeft.pop() ) {
        scrollRight.push( true );
        $left.scrollTop( $right.scrollTop() );
    }
} );

(jsFiddle)

This actually works, even though the scroll event is triggered on the other div the code is not executed and the loop is broken. I'm using a queue with push and pop in case some browser calls event handlers in some weird way; possibly using a single boolean variable could suffice to guard scrollTop.

My question is, is there a better way of doing this?

As you never want to handle a triggered event, I can suggest making it simpler using a single sentinel flag:

$left = $("#left")
$right = $("#right")

// Use a simple sentinel to say we are busy handling an event
var scrolling = false;
var timer;
$left.scroll(function () {
    //console.log("left "  + $left.scrollTop() + " : " + $.now());
    if (!scrolling) {
        //console.log("SCROLLING LEFT");
        clearTimeout(timer);
        timer = setTimeout(function () {
            scrolling = true;
            $right.scrollTop($left.scrollTop());
            scrolling = false;
        }, 10);
    }
});

$right.scroll(function () {
    //console.log("Right " + $right.scrollTop() + " : " + $.now());
    if (!scrolling) {
        clearTimeout(timer);
        timer = setTimeout(function () {
            //console.log("SCROLLING RIGHT");
            scrolling = true;
            $left.scrollTop($right.scrollTop());
            scrolling = false;
        }, 10);
    }
});

Update:

With FireFox, it triggers multiple scroll events, in sequence, for a single mouse wheel move. The trick is to wait until it settles using a timer. I found 10ms seems to work fine.

JSFiddle: http://jsfiddle.net/8xaw2896/8/


Largest Prime Factor of Number in PHP or QBasic I was solving the 3rd problem at Project Euler which requires me to calculate the largest prime factor of 600851475143  which I think is a really large number (more than 6 billion).

QB64

At first, I tried solving this problem using QB64.

CLS
n = 600851475143
FOR x = 1 TO n
    IF n MOD x = 0 THEN
        FOR y = 1 TO (x / 2) - 1
            IF x MOD y = 0 THEN
                factor = y
            END IF
        NEXT y
        IF factor = 1 THEN
            ans = x
        END IF
    END IF
NEXT x
PRINT ans

However, on running the program, QB64 would appear busy executing the program, but no result would appear even after waiting for a long time.

QuickBasic 4.5

On getting no help from a compiler, I switched to QuickBasic 4.5 which is an interpreter. I executed the same code, but on trying to run the program, QuickBasic would trigger the following error on the line where I define the number.

Overflow

And, the program wouldn't execute at all.

Repl.it / QBasic

Getting no success, I tried using an online interpreter. View the live code at repl.it

On executing the same code on repl.it, the results were similar to what I got for QB64

PHP

I termed this as a limitation for the QBasic language and hence re-wrote the same logic in PHP.

$n = 600851475143;
for($x = 1; $x <= $n; $x++){
    if($n % $x == 0){
        for($y = 1; $y < $x/2; $y++){
            if($x % $y == 0){
                $factor = $y;
            }
        }
        if($factor == 1){
            $ans = $x;
        }
    }
}
echo $ans;

Localhost

On executing this code in my local server (http://localhost:8000) I got no result, instead, the following error:

Maximum execution time of 30 seconds exceeded -- at line 5

I increased the max_execution_time from the php.ini to a few hours. However, I got now result from the code as it never executed completely. I waited for more than 6 hours, but the code continued to execute indefinitely without giving any result.

Online PHP compilers

I then tried a number of online compilers like:

repl.it

Live code here.

WritePHPOnline
PHPFiddle

However, none of them could give me the result as they would either never stop executing or would show the maximum execution time error.

So, my question is, How can I SUCCESSFULLY execute this code to solve my problem? Do I need to:

Use a different, more efficient Compiler / Interpreter
Switch to a different language altogether
Make my logic even more concise to reduce execution time

Note: When I used the above codes with a smaller number like 13195, the result was exactly correct, i.e., 29.

Finding prime factor is a common problem. Because they are hard to find, prime numbers are used in cryptography. You can read more  here.

Now, in your question, you are looking for the largest prime factor of a huge number $n. Your approach is to test all numbers from 1 to $n. While this is accurate, is is not optimal, as you can see it takes more than 6 hours.

First loop : $n=600851475143 iterations

The first thing I would change is the loop logic : since you are looking for the largest prime factor, begin with $n instead of 1 and exit ($n-$ans iterations) :

for($x = $n; $x >= 1; $x--)
{
    if('$x is factor')
        if('$x is prime')
            break;
}

Then, because you are not looking for $n or 1 or 2 or 3, I would exclude them all ($n-$ans-4 iterations) :

for($x = $n-1; $x > 1; $x--)
{
    if('$x is factor')
        if('$x is prime')
            break;
}

Then, you are looking for a factor $x, such as $x1*$x2 = $n, which implies that if $x2 != 1 && $x2 <= sqrt($n) and $x2 is a factor, $x1 = $n/$x2 is a factor too that is larger than $x2 (sqrt($n)-$n/$ans-1 < 775146 iterations). Note that either or both $x1 and $x2 can be prime. Also note that floor() is because we are working with integers and (int) is for PHP7+ compatibility :

for($x1 = (int)floor(sqrt($n)); $x1 > 3; $x2++)
{
    if('$x1 is not a factor')
        continue;
    $x2 = $n/$x1;
    if('$x1 is prime')
        break;
    if('$x2 is prime')
        break;
}

There are ways to do better but since we switched from 600G to 775K it should be enough.

Second loop : $x/2 iterations

Loops in loops are the first thing to resolve when optimizing code. If you loop from 1 to 1000 in a loop from 1 to 1000, that is 1000000 iterations ; in your case it is a bit more complicated to evaluate but I think it is close to $n*$n/2 = 633457474756096856 iterations (!). with the first loop reviewed, is is close to 775146*775146/2 = 300425660658 iterations = 300G iterations : better but still huge !

First thing to do is to break on the first $y != 1 found, if $x1 is not a prime number you do not need to find all its factors (iterations are hard to evaluate but get in mind that all $x1 with factors of 2 and 3 gets their loop reduced from $x1/2 to 2 or 3) :

for($y = 1; $y < $x1/2; $y++)
{
    if($x1 % $y == 0)
    {
        $first_factor = $y;
        break;
    }
}

Then instead of doing $x1/2 do sqrt($x1) as in the loop 1 optimisations :

$first_factor = null;
for($y = (int)floor(sqrt($x)); $y > 1; $y--)
{
    if($x % $y == 0)
    {
        $first_factor = $y;
        break;
    }
}
if(is_null($first_factor))
{
    $ans = $x;
    break;
}

I personaly won't but you can do a faster prime check implementing first one hundred (or so) prime numbers and testing them for factor of $y first.

Code review

Please use functions, you will have a light performance loss due to internal PHP stack behavior but your code will be much cleaner and reusable (for the next Project Euler problems...).

Adjusting all the code gives you something running under 100ms : https://3v4l.org/jXd3E

function greatest_prime($n)
{
    for($x1 = (int)floor(sqrt($n)); $x1 > 3; $x1--)
    {
        if($n % $x1 != 0)
            continue;
        $x2 = $n/$x1;
        if(is_prime($x1))
            return $x1;
        if(is_prime($x2))
            return $x2;
    }
}
function is_prime($x)
{
    for($y = (int)floor(sqrt($x)); $y > 1; $y--)
        if($x % $y == 0)
            return false;
    return true;
}
var_dump(greatest_prime(600851475143));


Cryptographic range based random number generator class Recently had reason to design a range based random number generator that uses the RNGCryptoServiceProvider provided by the .net api.

The basic algorithm generates a random unsigned 64 bit integer and creates a floating point number between 0 and 1 by dividing it by UInt64.MaxValue+ 1.  This is then multiplied by the range and added to the minimum, to produce a 32 bit integer in the right range.

The class also includes a shuffle method.

using System;
using System.Collections.Generic;
using System.Security.Cryptography;
using System.Linq;
public static class CryptoRandom
{
    const double MAX_RANGE = (double)UInt64.MaxValue + 1;

    /// <summary>
    /// Get a cryptographic random integer in the range from
    /// min(inclusive) to max(exclusive)
    /// </summary>
    /// <param name="min"></param>
    /// <param name="max"></param>
    /// <returns></returns>
    public static int Next(int min, int max)
    {
        if (max < min)
        {
            throw new ArgumentException("max is less than min");
        }
        if(min < 0)
        {
            throw new ArgumentException("min and max must be positive integers");
        }
        if (min == max)
        {
            return min;
        }
        using (RNGCryptoServiceProvider rng = new RNGCryptoServiceProvider())
        {
            byte[] randomNumber = new byte[8];

            rng.GetBytes(randomNumber);
            double baseNum = BitConverter.ToUInt64(randomNumber, 0) / MAX_RANGE;

            int range = max - min;

            return (int)(baseNum * range) + min;
        }

    }
    /// <summary>
    /// Get a cryptographic random integer in the range from
    /// 0 to max(exclusive)
    /// </summary>
    /// <param name="max"></param>
    /// <returns></returns>
    public static int Next(int max)
    {
        return Next(0, max);
    }
    /// <summary>
    /// Get a cryptographic random 32-bit integer
    /// </summary>
    /// <returns></returns>
    public static int Next()
    {
        return Next(0, Int32.MaxValue);
    }
    /// <summary>
    /// A cryptographic random shuffle method
    /// </summary>
    /// <typeparam name="T"></typeparam>
    /// <param name="input"></param>
    /// <returns></returns>
    public static ICollection<T> Shuffle<T>(ICollection<T> input)
    {
        return input.OrderBy(x => Next()).ToArray();
    }
}

When dealing with fair randomness and bit manipulation code, you have to be a lot more careful than you currently are.

Your random number generator is not fair, i.e. not all numbers have exactly the same possibility of being returned. Have a look at Java's implementation of java.util.Random or the Python standard library, which are all open source and good learning resources.

When you do integer calculations, don't use floating point calculations internally. They are very tricky to do correctly. Think about rounding for every arithmetic operation.

Shuffling is not the same as sorting. Read the Wikipedia article on shuffling (or any other article) to get this right.

Your Next() function is documented to return a 32-bit integer, but that integer has a little less than 31 bits of randomness. Therefore the documentation is misleading. To properly implement this method, you should get 32 bits of randomness from the underlying random generator, and if you only want positive numbers, return rnd & Integer.MaxValue. This gives you exactly 31 bits of randomness, and this is also what the documentation should say.

In the Next(min, max) function, you must throw an exception if min == max. Your current code returns max in that case, but the documentation says that max is exclusive.

And finally, remove redundant documentation comments. Having an empty <param name=min></param> comment is worthless, since it doesn't add any information. The code already says clearly that there is a parameter called min, and the comment should not repeat this. If you had written some additional information in that comment (like you did in the summary), it would be ok.


Python function to map True, False, and None to symbols for display I have a use case in which I need to check whether a given value is explicitly True or False:

def stringify(value):
    """Returns the string representation of the value."""

    if value is None:
        return '-'

    if value is True:
        return '✓'

    if value is False:
        return '✗'

    return str(value)

I know, that in most cases, where you just need the truthiness, you'd just do if value:. But here this will not suffice, since I want zero ints and floats to be displayed in their decimal form as well as empty lists, sets dicts as such.
Is the above code acceptable or should I rather check using isinstance(value, bool) first and then do if value:?

What's the least surprising way to go?

Usage Context
The function is part of an API that converts database records to terminal output:
MariaDB → peewee → API → terminal

Example output:

$ termutil ls -T 1000
  ID  TID         CID  VID  OS                     IPv4 Address               Deployed  Testing  Tainted  Address                                           Annotation
 214    1        1000    -  HIDSL latest              XX.Y.Z.75                      -        ✓        ✗  Musterstraße 42, 123456 Irgendwo                      -
 215    2        1000    -  HIDSL latest              XX.Y.Z.76                      -        ✓        ✗  Musterstraße 42, 123456 Irgendwo                      -
 216    3        1000    -  HIDSL latest              XX.Y.Z.77                      -        ✓        ✗  Musterstraße 42, 123456 Irgendwo                      -
 217    4        1000    -  HIDSL latest              XX.Y.Z.78                      -        ✓        ✗  Musterstraße 42, 123456 Irgendwo                      -
 218    5        1000    -  HIDSL latest              XX.Y.Z.79                      -        ✓        ✗  Musterstraße 42, 123456 Irgendwo                      -
 219    6        1000    -  HIDSL latest              XX.Y.Z.80                      -        ✓        ✗  Musterstraße 42, 123456 Irgendwo                      -
 330    7        1000  399  HIDSL latest             XX.Y.Z.182                      -        ✗        ✗  Musterstraße 42, 123456 Irgendwo                      -
 508    8        1000    -  HIDSL latest             XX.Y.Z.189                      -        ✗        ✗  N/A                                                   -

Because the code snippet is so small and missing any other context, it's hard to identify any problems in scaling / that may arise elsewhere in the code. Thus if you want a more tailored answer, you should add more code.

Yes, this is fine.

Regarding the isinstance() check, no. That only adds an extra level of indentation without justification.


Java find minimum range Question Description:

Given a list of companies, eg {ABC, BBC} and a large list of data looks like below:

Timestamp Security
10000000  IBM
10000001  Apple
10000005  PEAR
10000008  ABC
10000012  BBC

Find minimum time range where you can find all companies from list above.

My solution works and is brute force. But it has 3 loops. Can someone point out some ways to improve this solution in terms of runtime?

import java.util.*;

public class MinimumTimeRange {
    public static int findMinimumTimeRange(String [] list, String [] input){
        int n = list.length;
        Set<String> mySet = new HashSet<String>(Arrays.asList(list));
        int result = Integer.MAX_VALUE;
        for(int i=0; i<=(input.length-n);i++){
            for(int idx =n; idx<=(input.length-i);idx++){
                int [] timeList = new int [idx];
                Set<String> compSet = new HashSet<String>();
                for(int j=0;j<idx;j++){

                    String[] tokens = input[i+j].split(" ");
                    String time = tokens[0];
                    String comp = tokens[1];
                    timeList[j]=Integer.parseInt(time);
                    compSet.add(comp);
                }
                if(compSet.containsAll(mySet)&& (timeList[idx-1]-timeList[0])<result){
                    result = timeList[idx-1]-timeList[0];
                }
            }

        }
        return result;
    }
    public static void main(String[] args){
        String [] listCompanies = {"IBM","GE","GOOGLE"};
        String [] input = {
                "10000000 IBM",
                "10000001 Apple",
                "10000005 GOOGLE",
                "10000012 IBM",
                "10000013 TEST",
                "10000014 GE"
        };
        System.out.println(findMinimumTimeRange(listCompanies, input));
    }
}

Above code output 9, which is correct.

Other test examples:

String [] listCompanies = {"ABC","APPLE"};
String [] input = {
                    "1 ABC",
                    "2 TEST",
                    "3 APPLE",
                    "4 BBC",
                    "12 APPLE",
                    "13 ABC"
            };

Output: 1

This can be reduced to a common problem : smallest range containing elements from k lists.

Assuming \$N\$ is the number of inputs and \$M\$ the number of companies in listCompanies.

In your problem, what you would do is make a set from your listCompanies (\$M \log M\$). Make a map where the keys are those companies and the values are lists (\$M \log M\$). Then, loop your input and if it's a company of interest (it has a key on the map), add that time to the list of that company.

So in your first example, IBM would be [10000000, 10000012]. After you have done this, you have now the common problem of finding the minimum range in which you can have one item from each of those lists (minimum range that has each of your companies in listCompanies).

You can then apply the algorithm linked and end up with a complexity of (\$N \log M\$ -> each element will enter and leave a heap of size \$M\$ only once). \$N\$ in this case will be the amount of inputs that are related to companies from your listCompanies.

Here is a C++ implementation.


Find count of alternating numbers What I am trying to do is to find count of alternating numbers such that it alternates with -ve and positive sign for e.g.: 1 -2 3 -4 would get me 4 3 2 1 as from 1 to -4 including the two numbers there are 4 numbers. Similarly, for 1 1 -3 2 would get me 1 3 2 1. Now I have the code but I cannot optimise it and it returns me a time limit exceeded error even though it works for moderate input stream.

Here, alternating means like first number is positive and second number is negative and next number is positive and so on. So if input is 1 -2 3 -4 then from number 1 maximum I can travel to -4 such that there is an alternating sequence. So the count =3 numbers + the number itself i.e 4. Again from -2 if I travel from I can go till -4 as it alternates from negative positive and negative. So it is 2 numbers +1=3 again. Similarly, for r=each position I get it as 4 3 2 1. Now suppose number is 1 1 -3 2 then there is no number for 1 to find the next alternating number immediately so 1.

So for 1 1 -3 2 for 1 since there is no immediate -ve number it is a 1.For the second 1 I can traverse to a -3 and 2 i.e 2 number so count=2+1=3.For -3 all I find is a single 2 so count is 1+1=2.For the last number it is 1.So the answer is 1 3 2 1

Sample input: 1 -2 1 -3 2

Output: 4 3 2 1

j=0
count=0
length=(raw_input())
st=map(int,raw_input().split())
while j+1 < len(st):
     k=j+1
     count=0
     temp=j
     while k<len(st) and ((st[k]<0 and st[j]>0) or (st[k]>0 and st[j]<0)):
       count+=1
       k+=1
       j+=1
     print count+1,
     j=temp+1
print 1

This problem is much easier looking it backwards: Instead of processing the input in the order given, process it in reverse order. You know the last number in your answer will be 1, because no numbers follow it. Now the previous one could be a 2 if the sign is different with the last one or a 1 if its the same sign.

So basically, each step back can either add 1 to the list of consecutive numbers with alternating signs or reset the count to 1.

The code:

    length=(raw_input())
    st = map(int,reversed(raw_input().split()))

    answ = []
    count = 0
    for i, num in enumerate(st):
         count += 1
         if i > 0 and (num < 0) == (st[i - 1] < 0):
            count = 1
        answ.append(str(count))

    print " ".join(reversed(answ))


"Deep length" of a list A list that contains one or more lists as elements is called a deep
  list. For example, [1, [2, 3], 4] is a deep list.

  Write a function deep_len that takes a list and returns its deep
  length. See the doctests for the function's behavior.

  Hint: you can check if something is a list by using the built-in
  type function. For example,

>>> type(3) == list
False
>>> type([1, 2, 3]) == list
True

My solution:

def deep_len(lst):
    """Returns the deep length of the list.

    >>> deep_len([1, 2, 3])     # normal list
    3
    >>> x = [1, [2, 3], 4]      # deep list
    >>> deep_len(x)
    4
    >>> x = [[1, [1, 1]], 1, [1, 1]] # deep list
    >>> deep_len(x)
    6
    """
    if not lst:
        return 0
    if type(lst[0]) != list:
        return 1 + deep_len(lst[1:])
    else:
        return deep_len(lst[0]) + deep_len(lst[1:])

Can we improve this recursive code?

I would write that simply:

def deep_len(lst):
    """Returns the deep length of the list.

    >>> deep_len([1, 2, 3])     # normal list
    3
    >>> x = [1, [2, 3], 4]      # deep list
    >>> deep_len(x)
    4
    >>> x = [[1, [1, 1]], 1, [1, 1]] # deep list
    >>> deep_len(x)
    6
    """
    return sum(deep_len(el) if isinstance(el, list) else 1 for el in lst)

Note that:

This doesn't create lots of new lists with slices, so I would expect it to be more efficient;
isinstance is a much more Pythonic way to check types than type, despite what the "hint" says, as it handles inheritance better; and
There's no need to special-case an empty lst, as the result is already zero in that case.


Handling COM exceptions / busy codes This code writes to Excel using the COM interface. The general issue is that any exception handling has to handle the "Excel is busy" exception. This occurs if information is sent to Excel quicker than it can handle it - eg. latency when a workbook is loaded/created, or the user is playing with the scrollbars (there are good reasons for letting this happen).

This is probably the only example I know of which is simpler and cleaner in VB6 than in C#! In VB6 an ON ERROR would be used. The error handler would then create an error for most cases. But if the error code is a "busy" then it will sleep a short period of time (typically half a second) and then try again with a "RESUME". Don't get me wrong, ON ERROR is generally messier than C#'s try...catch and it is easier to produce awful code; however, this is one example where the VB6 ON ERROR works better. A long sequence of Excel calls can be trapped with one handler. The "RESUME" will then send control back to the line where the 'busy' occurred - this avoids duplicate calls or skipped calls.

The solution I have in C# is to create a while loop with a flag. The flag indicates a repeat of the loop is required due to a 'busy' return from Excel. See the code below.

Is there a simpler, more elegant way of doing this?  The main problem is that this requires a method for each type of Excel call. To avoid too many duplicate Excel calls in the busy scenario, the contents of each method is atomic or close to atomic - eg. "write this formatted value"; "apply this formatting to this row". This results in lots of methods. And/or methods with lots of parameters (the example below is a short one with just one format option, but there could be more - colors, decimal points, etc).

private void WriteDoubleValue(Excel.Worksheet sh, int x, int y, double lfval, bool bBold)
{
    bool bNotSuccess = true;
    while (bNotSuccess)
    {
        try
        {
            ((Excel.Range)sh.Cells[y,x]).set_Value(Missing.Value, lfval);
            ((Excel.Range)sh.Cells[y, x]).Font.Bold = bBold;
            bNotSuccess = false;
        }

        catch (System.Runtime.InteropServices.COMException e)
        {
            if ((e.ErrorCode & 0xFFFF) == 0xC472)
            {   // Excel is busy
                Thread.Sleep(500); // Wait, and...
                bNotSuccess = true;  // ...try again
            }
            else
            {   // Re-throw!
                throw e;
            }
        }
    }
}

If I understood you correctly, you have a lot of methods which are identical to the one you've shown except for the parameters they take and the contents of the try-block. The rest is repeated code, which is bad.

To fix this I'd recommend to abstract the "repeat this action as long as Excel is busy" logic into its own method, which takes the action to be repeated as a parameter.

On a style note, I would argue against using Hungarian Notation. It's not really commonly used in .net and basically every style guide written in this century argues against it.

I'd also recommend making the bool variable positive (i.e. success instead of notSuccess). This way people don't have to perform double negation in their head when reading things like notSuccess = false (which would be changed to success = true).

With these suggestions the code could look like this:

private void TryUntilSuccess(Action action)
{
    bool success = false;
    while (!success)
    {
        try
        {
            action();
            success = true;
        }

        catch (System.Runtime.InteropServices.COMException e)
        {
            if ((e.ErrorCode & 0xFFFF) == 0xC472)
            {   // Excel is busy
                Thread.Sleep(500); // Wait, and...
                success = false;  // ...try again
            }
            else
            {   // Re-throw!
                throw e;
            }
        }
    }
}

You could then implement WriteDoubleValue and all the methods like it with a call to TryUntilSuccess like this:

TryUntilSuccess( () =>
{
    ((Excel.Range)sh.Cells[y,x]).set_Value(Missing.Value, lfval);
    ((Excel.Range)sh.Cells[y, x]).Font.Bold = bBold;
});


Cache implementation in Rust I've started learning Rust recently using the Rust book.

In one of the chapters, the authors encourage the reader to experiment with closures, and try to create a generic cache implementation.

I've managed to get it working as below. However, it irks me that I need to add Clone trait on the input argument in the get() method. I was wondering if there was a better way of doing this?

Any other comments are also welcome (apart from the main() method which is a sample driver).

use std::collections::HashMap;
use std::thread;
use std::time::Duration;
use std::hash::Hash;
use std::clone::Clone;

struct Cache<T, U, W>
    where T: Fn(U) -> W,
          U: Eq + Hash + Clone {
    calculator: T,
    values: HashMap<U, W>,
}

impl<T, U, W> Cache<T, U, W>
    where T: Fn(U) -> W,
          U: Eq + Hash + Clone {
    fn new(calculator: T) -> Cache<T, U, W> {
        Cache {
            calculator,
            values: HashMap::new(),
        }
    }

    /*
     * The ugliest part in this function is the constraint that we have to clone the input.
     * HashMap::entry() moves the key, which renders it unusable for subsequent calculations.
     * So we can either
     * - always perform the expensive computation
     * (OR)
     * - clone the input so that it can be re-used later
     *
     * Another problem is that defining a closure in or_insert_with borrows the Cache::calculator, and that creates a problem
     * when we attempt to borrow it again by invoking self.calculator. This is why the following line won't work:
     * self.values.entry(input.clone()).or_insert_with(|| { (self.calculator)(input) })
     */
    fn get(&mut self, input: U) -> &W {
        let calc = &self.calculator;
        self.values.entry(input.clone()).or_insert_with(|| { (calc)(input) })
    }
}

fn main() {
    println!("Hello, world!");
    let mut c = Cache::new(|x: &str| {
        println!("performing an expensive calculation with input: {}", x);
        thread::sleep(Duration::from_secs(2));
        format!("Original input: {}", x)
    });

    let x = c.get("10");
    println!("x: {}, cacher after getting {}", x, 10);

    let x = c.get("10");
    println!("x: {}, cacher after getting {}", x, 10);

    let x = c.get("20");
    println!("x: {}, cacher after getting {}", x, 20);

    let x = c.get("10");
    println!("x: {}, cacher after getting {}", x, 10);

    let x = c.get("10");
    println!("x: {}, cacher after getting {}", x, 10);

    let x = c.get("20");
    println!("x: {}, cacher after getting {}", x, 20);
}

You don't need to have the where clause on your struct. General, rust coders prefer to only restrict the impl blocks and not the structs.

You can avoid Clone, by unpacking the Entry enum, like so:

fn get(&mut self, input: U) -> &W {
    use std::collections::hash_map::Entry;
    match self.values.entry(input) {
        Entry::Occupied(entry) => entry.into_mut(),
        Entry::Vacant(entry) => {
            let value = (self.calculator)(entry.key());
            entry.insert(value)
        }
    }
}

This is, in fact, close to how or_insert_with is implemented.

Note that this calls the function with &U not U. Your calculator cannot take ownership of the key if you also plan to put into the hashmap. This is awkward for your code because it is using &str as the keys. Hence the type passed to the calculator is &&str. We can resolve this by using the borrow trait. The Borrow traits knows that it can convert &&str to &str, so we could implement it as follows.

// Leave the struct unconstrained
struct Cache<T, U, W> {
    calculator: T,
    values: HashMap<U, W>,
}

// The impl block only constrains for Eq + Hash of the keys.
impl<T, U, W> Cache<T, U, W>
    where
          U: Eq + Hash
         {

    fn new(calculator: T) -> Cache<T, U, W> {
        Cache {
            calculator,
            values: HashMap::new(),
        }
    }

    // We take a new type parameter, Y.
    // The calculation function takes &Y as a parameter
    // It does not need to be sized because we only use references to it.
    fn get<Y:?Sized>(&mut self, input: U) -> &W
        where
             // Our calculator must take a reference to Y.
             T: Fn(&Y) -> W,
             // The borrow trait will let us convert a &U to a &Y
             U: std::borrow::Borrow<Y> {
        use std::collections::hash_map::Entry;
        match self.values.entry(input) {
            Entry::Occupied(entry) => entry.into_mut(),
            Entry::Vacant(entry) => {
                let value = (self.calculator)(entry.key().borrow());
                entry.insert(value)
            }
        }
    }
}


ErrorManager management Recently I created a class that would manager errors, log them, and write details about them to files when the function logError was called. It would be done by having an error code and error message and then displaying the error to the console and writing it to the file if the user wanted to.

I just posted this here to see if there was any way I can improve the class.

using log4net;
using System;
using System.Collections.Generic;

namespace Kiwi.Application.Base.Error
{
    sealed class ErrorManager
    {
        private readonly Dictionary<string, string> errorCodes;
        private readonly ILog myLogger;

        public ErrorManager()
        {
            errorCodes = new Dictionary<string, string>();
            myLogger = LogManager.GetLogger(typeof(ErrorManager));

            // Add some errors and their messages to the dictionary
            errorCodes.Add("bn1x", "Error when beginning to listen on server socket.");
            errorCodes.Add("dm9e", "Unable to locate the error log file.");
        }

        public void logError(string errorCode, bool writeErrorToFile = false)
        {
            string errorMessage;
            if (errorCodes.TryGetValue(errorCode, out errorMessage))
            {
                myLogger.Warn(errorMessage);

                if (writeErrorToFile)
                    logToFile(errorCode);
            }
            else
            {
                logUnhandeldError("[Error code " + errorCode + "] " + errorCode);
            }
        }

        private void logUnhandeldError(string errorCode)
        {
            myLogger.Error("Unhandeld error " + errorCode);
        }

        private void logToFile(string errorCode = "", string fileName = "logs/error.log")
        {
            try
            {
                using (System.IO.StreamWriter file = new System.IO.StreamWriter(fileName, true))
                {
                    file.WriteLine("Error " + errorCode + " logged at " + getErrorTimestamp());
                    file.WriteLine("Message: " + tryGetErrorMessage(errorCode));
                }
            }
            catch (System.IO.FileNotFoundException)
            {
                logError("dm9e");
            }
        }

        private string getErrorTimestamp()
        {
            return DateTime.Now.ToLongDateString();
        }

        private string tryGetErrorMessage(string errorCode)
        {
            string errorMessage;

            if (errorCodes.TryGetValue(errorCode, out errorMessage))
                return errorMessage;

            else return "";
        }
    }
}

Generally in .NET (especially C#) public members are PascalCase.

On a more important note: don't use strings for the errorCode. Instead, use an enum.

public enum ErrorCode
{
    ServerSocketBeginListeningFailure, // Rename this to something shorter if desired.
    LogFileNotFound,
}

You could also, potentially, make this a Flags enum, which would mean that error codes could be easily combined and used together to indicate multiple errors occurring at a time.

This will allow you to make more certain that the user doesn't specify a bad error code.

Use braces even when not necessary, they won't generally prevent bugs, but they will help prevent bugs.

Just as well, do not inline an if/else if/else statement when not using braces. Typically, when I see one of those, my eyes immediately follow the next line. I didn't notice return "" on your else in tryGetErrorMessage until I realized there was not a statement below it.


Simple random generator Based on this question, I have made a simple program to print a sequence of random numbers. The program seems to run fine, but I would like to know if my implementation is correct and how I can improve it.

#include <random>
#include <array>
#include <algorithm>
#include <functional>
#include <iostream>

template<class T = std::mt19937, std::size_t N = T::state_size>
auto randomEngine() -> typename std::enable_if_t<!!N, T>
{
    std::array<typename T::result_type, N> seed_data;
    thread_local static std::random_device source;
    std::generate(std::begin(seed_data), std::end(seed_data), std::ref(source));
    std::seed_seq seeds(std::begin(seed_data), std::end(seed_data));
    thread_local static T seeded_engine(seeds);
    return seeded_engine;
}

template<typename T>
T random(T min, T max)
{
    static_assert(std::is_integral<T>::value || std::is_floating_point<T>::value, "!");

    using UniformInt = std::uniform_int_distribution<T>;
    using UniformReal = std::uniform_real_distribution<T>;
    using DistType = std::conditional_t<std::is_integral<T>::value, UniformInt, UniformReal>;

    static auto RandomEngine = randomEngine();

    DistType uniformDistribution(min, max);

    return uniformDistribution(RandomEngine);
}

int main()
{
    for (auto i = 0u; i < 16; ++i)
        std::cout << random(0u, 15u) << ' ';
}

Extra typenames

Every place you use typename, it's unnecessary. std::enable_if_t will take care of that for you.

thread_local

First of all, thread_local implies static, so writing both is redundant. Secondly, if you're making it thread_local, that's effectively like making a thread-specific singlelton - so you should return a reference to it, not a copy:

template<class T = std::mt19937, std::size_t N = T::state_size>
auto randomEngine() -> std::enable_if_t<!!N, T&>

And take it by reference:

static auto& RandomEngine = randomEngine();

random()

I'm not sure it's a good idea to create the distribution every time. The distribution object itself can keep state, so I think it'd be a better idea to take it in instead:

template <class Dist>
typename Dist::result_type random(Dist& dist)
{
    static auto& RandomEngine = randomEngine();
    return dist(RandomEngine);
}

And alter dist<T> to take arguments so that you could do:

auto d = dist<unsigned>(0, 15);
for (auto i = 0u; i < 16; ++i)
    std::cout << random(d) << ' ';


Counting intersections of wires between towers The "Rope Intranet" problem from Google Codejam 2010 asks us to count the number of intersections (black circles) in a planar straight-line graph like this:

That is, with each line segment having one endpoint with \$x=x_0\$ and the other with \$x=x_1\$.

  Input

  The first line of the input gives the number of test cases, \$T\$. \$T\$ test cases follow. Each case begins with a line containing an integer \$N\$, denoting the number of wires you see.

  The next \$N\$ lines each describe one wire with two integers \$A_i\$ and \$B_i\$. These describe the windows that this wire connects: \$A_i\$ is the height of the window on the left building, and \$B_i\$ is the height of the window on the right building.

  Output

  For each test case, output one line containing "Case #\$x\$: \$y\$", where \$x\$ is the case number (starting from 1) and \$y\$ is the number of intersection points you see.

  Sample input

2
3
1 10
5 5
7 7
2
1 1
2 2

  Sample output

Case #1: 2
Case #2: 0

I have a working solution, but the time complexity is awful. Here is my code:

def solve(wire_ints, test_case):
    answer_integer = 0
    for iterI in range(number_wires):
        for iterJ in range(iterI):
            holder = [wire_ints[iterI], wire_ints[iterJ]]
            holder.sort()
            if holder[0][1] > holder[1][1]:
                answer_integer = answer_integer + 1
    return("Case #" + str(test_case) + ":" + " " + str(answer_integer))

for test_case in range(1, int(input()) + 1):
  number_wires = int(input())
  wire_ints = []
  for count1 in range(number_wires):
    left_port,right_port = map(int, input().split())
    wire_ints.append((left_port,right_port))
  answer_string = solve(wire_ints, test_case)
  print(answer_string)

I was wondering if anyone here is able to suggest a way in which I can speed up my algorithm? After 4 hours I really can't see one and I'm losing my miinnnnddd.

There are two key observations to make about this problem.

The first (which you spotted), is that the exact heights of the left and right endpoints of each line segment don't matter, only their order. That is, a line \$A_i, B_i\$ intersects the line \$A_j, B_j\$ if either \$A_i < A_j\$ and \$B_i > B_j\$, or \$A_i > A_j\$ and \$B_i < B_j\$.

The second, is that each pair of intersecting lines is an "inversion". (An inversion is a pair of items that are out of order with respect to a permutation.) That is, if we start with the lines in order of their left heights \$A_i\$, and consider sorting them into the order of their right heights \$B_i\$, the number of intersections is the same as the number of inversions with respect to the sorted order.

The number of inversions in a sequence with respect to a sorted order can be counted in \$O(n \log n)\$ using an adaptation of the merge sort algorithm. This is a standard exercise in an algorithms course (for example, it's exercise 2–4d in Introduction To Algorithms by Cormen, Leiserson, Rivest, and Stein), so I won't spoil it for you here.


Counting the number of bits of a positive integer How can I improve this code for counting the number of bits of a positive integer n in Python?

def bitcount(n):
    a = 1
    while 1<<a <= n:
        a <<= 1

    s = 0
    while a>1:
        a >>= 1
        if n >= 1<<a:
            n >>= a
            s += a
    if n>0:
        s += 1

    return s

The very first thing you should do to improve it is comment it. I'm reading it for almost half an hour and still can't understand what it does. I tested it, and it indeed work as intended, but I have no idea why. What algorithm are you using?

I pointed below parts of the code that aren't clear to me. Since @blufox already presented a simpler way to count bits (that works for non-zero numbers), I won't bother to suggest an improvement myself.

def bitcount(n):
    a = 1
    while 1<<a <= n:
        a <<= 1

Why is a growing in powers of two, while you're comparing 1<<a to n? The sequence you're generating in binary is 10 100 10000 100000000 10000000000000000 ... Take n=101010, and notice that

10000 < 100000 < 101010 < 1000000 < 10000000 < 100000000

i.e. there is no relation between 1<<a and the number of bits in n. Choose a=1<<2, and 1<<a is too small. Choose a=1<<3 and 1<<a is too big. In the end, the only fact you know is that 1<<a is a power of two smaller than n, but I fail to see how this fact is relevant to the task.

    s = 0
    while a>1:
        a >>= 1
        if n >= 1<<a:
            n >>= a
            s += a

This removes a bits from n, while increasing the bit count by a. That is correct, but I fail to understand why the resulting n will still have fewer bits than the next 1<<a in the sequence (since they vary so wildly, by 2**(2**n)).

    if n>0:
        s += 1

    return s

I see that the result is off by 1 bit, and your code correctly adjust for that. Again, no idea why it does that.


Reading unlimited input strings in C The idea is to write a function that reads string of unknown size from stdin, keep it in dynamically allocated array of unknown size and return a copy of that array.

Is this the right approach? Could I be more clear? Would reading the input char-by-char be better than getting bigger chunks at once?

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

char * read_string(void)
{
    int buff_size = 11; /* size of temporary buffer */
    char temp[11]; /* temporary input buffer */
    char *output; /* pointer to allocated memory */
    char *copy; /* copy of input string to be returned */
    char *iterator; /* pointer to beginning of an array, used when searching for \n in input string */

    output = malloc(10*sizeof(char)); /* allocates 10 bytes for first part of input */

    if(output == NULL)
        return NULL;

    size_t size = 10*sizeof(char); /* keeps track of actual size of output */
    //*output = '\0'; /* places NUL at the beginning of output, to make it empty string */
    while(fgets(temp, buff_size,stdin) != NULL) /* read  max 10 chars from input to temp */
    {
        strcat(output,temp); /* append read input to output, without \0 char */
        if(strlen(temp) != 10 || temp[9] == '\n') /* if buffer wasn't full or last char was \n-stop because it reached the end of input string */
            break;

        size += buff_size - 1; /* if didn' reach end of input increase size of output*/
        output = realloc(output,size);

        if(output == NULL)
        {
            return NULL;
        }
    }

    iterator = output; /* search for \n in output and replace it with \0 */

    while(*iterator != '\n')
        iterator++;

    *iterator='\0';

    copy=malloc(strlen(output) + 1); /* allocate memory for copy of output +1 for \0 char */
    if(copy == NULL)
        return NULL;

    strcpy(copy, output); /* strcpy output to copy */
    free(output);  /* free memory taken by output */

    return copy;
}

Here are a few comments on your function:

Firstly, read_line seems a more accurate name, as you read up until the next \n.
Reading 10 chars at a time is an odd. Unless that is part of the requirement I think you should allocate a bigger buffer and read as many chars as fit, extending the buffer each time round the loop by doubling its size (instead of adding 11).
As you have it, your fgets call overwrites the char after the end of the buffer with \0.  You should pass the actual size of the buffer to fgets.
Your fgets/strcat/strlen sequence is inefficient.  You should be reading directly into the target buffer rather than reading and then copying. And the strcat call has to traverse the whole length of the accumulated string to concatenate the new part.

char *p = buf;
size_t len = 0;

while (fgets(p, size - len, stdin) != NULL) {
    len += strlen(p);
    if (buf[len-1] == '\n') {
        buf[len-1] = '\0';
        break;
    }
    ...
}

Your reallocation call leaks memory if it fails to allocate - you need a temporary variable to hold the return value from realloc:

size *= 2;
if ((p = realloc(buf, size)) == NULL) {
    break; // and then return an error if `p == NULL` or `ferror`
}
buf = p;
p += len;

Your loop exits when fgets returns NULL, but you do not check for an error.
You must call ferror to be sure that an error did not occur.
And your looping at the end to find the \n seems wasteful when you have already located the end of line/string in the main  loop.
From the task statement you posted, copying the dynamic string is not required.

Note that reading character at a time is not necessarily a slower solution.  As you are doing it (with all that the copying and string traversal), I would expect a well written character-at-a-time function to be faster.  I encourage you to try both and compare the execution speed (using a very big test file as input).


Leetcode 957. Prison Cells After N Days I am working on 957. Prison Cells After N Days
 but getting timeout error for my code. as I assume my code should be 14+O(n-14) about O(n) am I right since after 14 run pattern will be repeated? and how can I improve my code?
        from collections import defaultdict
        step_map = defaultdict(list)
        for k in range(N):
            if tuple(cells) in step_map:
                cells = step_map[tuple(cells)]
            else:
                tmp = list()
                for i in range(1, 7):
                    tmp += [int(cells[i - 1] == cells[i + 1])]
                tmp = [0] + tmp + [0]
                step_map[tuple(cells)] = tmp
                cells = tmp
        return cells

The important part
They want something below O(n). Using map is a good idea but actually you should find the cycles and return the right position on the cycle instead of computing line by line.
Spoiler (possible solution)
Change:
            if tuple(cells) in step_map:
                return  step_map[tuple(cells)]

to:
            if tuple(cells) in step_map:
                cycle = list()
                head = tuple(cells)
                cycle.append(head)
                previous = head
                while True:
                    next_node = tuple(step_map[previous])
                    if next_node == head:
                        return list(cycle[(N - k) % len(cycle)])
                    cycle.append(next_node)
                    previous = next_node

Old edit - Some small improvements
There are some O(m) operations multiple times...
For example:
 tmp = [0] + tmp + [0]

Python operation for that is O(m). Therefore, your solution is O(nm).
step_map[tuple(cells)] = tmp

this is also O(m).


Ruby Sudoku X-wing solution I have written a Sudoku solver, for the euler problem and also packaged it into a ruby gem.  In my solution strategies I do a lot of iteration (double/triple nested loops, I am coming from Java/C).  One method in particular has a lot of if checks and iteration.  Would it be better to package this iteration into an enumerator method or another approach all together?  I am trying to learn the ruby way.  To be clear all the code works but I fear it is written terribly.

All the code can be seen here.

This is from grid.rb

  def x_wing
    box_line_reduction
    remaining_points.each do |point|
      point.nums.each do |num|
        [:x, :y].each do |symbol|
          arr = @points.select{ |p| p.nums.include?(num) && p.send(flip(symbol)) == point.send(flip(symbol)) && p.value == 0  }
          if arr.count == 2 && @points.select { |p| p.value == num && p.send(flip(symbol)) == point.send(flip(symbol)) }.count == 0
            last = @points.select { |p| p.nums.include?(num) &&
                                    arr.map{ |a| a.send(symbol) }.include?(p.send(symbol)) &&
                                    (!arr.include?(p)) &&
                                    p.value == 0 && check_row(p.y,p,num,symbol) }
            if last.all? { |x| x.send(flip(symbol)) == last.first.send(flip(symbol)) } &&
               last.count == 2 &&
               @points.select { |p| p.value == num && p.send(flip(symbol)) == last.first.send(flip(symbol)) }.count == 0
                 final = arr + last
                 places = final.map { |m| m.send(symbol) }.uniq
                 remaining_points.select { |p| places.include?(p.send(symbol)) && (!final.include?(p)) }.each do |poi|
                 poi.nums = poi.nums - [num]
              end
            end
          end
        end
      end
    end

Your Ruby code is so dense as to be unreadable. What you want to do is separate each component into a separate method, and use descriptive names for the methods, which are a way of commenting your code. I can't figure out what the methods and tests do, but you want to replace second_test and such with clear multi-word underscored descriptions of that test, such as any_valid_points?.

Separately, you want to use the -= and zero? methods. Rather than using an if statement without an else, you want to use a guard clause. {} blocks should only be used when they fit on one (80 character!!) line. Finally, I really recommend the use of Rubocop, which will give you all these pointers, and also push you to reduce the Assignment Branch Condition complexity of the remaining methods.

def initialize
  @arr = []
end

def x_wing
  box_line_reduction
  remaining_points.each do |point|
    point.nums.each do |num|
      [:x, :y].each { |symbol| process(symbol, num) }
    end
  end
end

def process(symbol, num)
  @arr = first_method(symbol, num)
  next unless second_test?(symbol, num)
  last = second_method(symbol, num)
  third_method(last) if third_test?(last)
end

def first_method(symbol, num)
  @points.select do |p|
    p.nums.include?(num) &&
      p.send(flip(symbol)) == point.send(flip(symbol)) &&
      p.value == 0
  end
end

def second_test?(symbol, num)
  @arr.count == 2 &&
    @points.select do |p|
      p.value == num && p.send(flip(symbol)) == point.send(flip(symbol))
    end.zero?
end

def second_method(symbol, num)
  @points.select do |p|
    p.nums.include?(num) &&
      @arr.map { |a| a.send(symbol) }
        .include?(p.send(symbol)) &&
      (!arr.include?(p)) &&
      p.value == 0 && check_row(p.y, p, num, symbol)
  end
end

def third_test?(last)
  last.all? { |x| x.send(flip(symbol)) == last.first.send(flip(symbol)) } &&
    last.count == 2 &&
    @points.select do |p|
      p.value == num && p.send(flip(symbol)) == last.first.send(flip(symbol))
    end.zero?
end

def third_method(last)
  final = @arr + last
  places = final.map { |m| m.send(symbol) }.uniq
  remaining_points
    .select { |p| places.include?(p.send(symbol)) && (!final.include?(p)) }
    .each { |poi| poi.nums -= [num] }
end


Implementation of a new kind of smart pointer in C++ I'm tempted to write a new kind of smart pointer which, I believe, is less intrusive than boost::intrusive_ptr and has the same memory footprint.

The code for the smart pointer, which I called less_intrusive_ptr, is shown below (the boost version used is 1.57.0):

#include <utility>
#include <boost/intrusive_ptr.hpp>
#include <boost/smart_ptr/intrusive_ref_counter.hpp>

template <class T>
class less_intrusive_ref_counter :
    public boost::intrusive_ref_counter<less_intrusive_ref_counter<T>>, public T
{
    using T::T;
};

template <class T>
class less_intrusive_ptr :
    public boost::intrusive_ptr<less_intrusive_ref_counter<T>>
{
    using boost::intrusive_ptr<less_intrusive_ref_counter<T>>::intrusive_ptr;
};

template <class T, class... Args>
less_intrusive_ptr<T> make_less_intrusive(Args&&... args)
{
    return less_intrusive_ptr<T>(
        new less_intrusive_ref_counter<T>(std::forward<Args>(args)...));
}

The code used to test it is shown below:

#include <iostream>
#include <boost/smart_ptr/shared_ptr.hpp>
#include "less_intrusive_ptr.h"

class MyClass1 {};
class MyClass2 : public boost::intrusive_ref_counter<MyClass2> {};
class MyClass3 {};

int main()
{
    boost::shared_ptr<MyClass1> p1(new MyClass1);
    boost::intrusive_ptr<MyClass2> p2(new MyClass2);
    less_intrusive_ptr<MyClass3> p3(new less_intrusive_ref_counter<MyClass3>);
    // or, more friendly syntax:
    //auto p3 = make_less_intrusive<MyClass3>();

    std::cout << sizeof(p1) << std::endl;   // output: 8
    std::cout << sizeof(p2) << std::endl;   // output: 4
    std::cout << sizeof(p3) << std::endl;   // output: 4
    return 0;
}

The advantages I can see are:

less memory usage (compared to shared_ptr)
ref-counting is used only when necessary
no need to write boilerplate code (compared to intrusive_ptr)
no need for a common base class (as is common when using intrusive_ptr)

The disadvantages I can see are:

we cannot use an existing raw pointer and add ref-counting behavior to it, we must always create an object by means of less_intrusive_ref_counter when we need such behavior
we cannot up-cast the underlying raw pointer (which is of type less_intrusive_ref_counter<T> *) to T * and then try to delete it, unless T's destructor is declared virtual. If it's not, and we try to delete the object trough T's pointer, we get undefined behavior.
we must be able to derive from the passed in T, which excludes built-in and (maybe?) POD types, as well as classes marked as final. [pointed out by Filip Roséen and Morwenn]
we cannot make an assignment such as the following: less_intrusive_ptr<Base> p = make_less_intrusive<Derived>(), because less_intrusive_ref_counter<Derived> does not inherit from less_intrusive_ref_counter<Base>.

My question is: can you spot other disadvantages to this approach? In fact, do you think this design is flawed in some way? I really am not an experienced C++ programmer.

To answer my own question, in light of the disadvantages that were gathered, I guess that the usage of the proposed smart pointer would be severely limited. Hence, I would rather rely on the flexibility of the standard smart pointers, especially through the use of std::make_shared.


Get all possible subsets from a set of distinct integers using OOP An assignment at school required me to write a program for this task:

  Create a class and define two methods in the class.
  Method f1 is used to pass an empty list and the sorted list is taken from the user to method f2.
  Method f2 is used to compute all possible subsets of the list.
  Then the result is returned from the function and printed.
  Exit

Here is my solution to this task, using Python:

class sub:
    def f1(self, s1):
        return self.f2([], sorted(s1))

    def f2(self, current, s1):
        if s1:
            return self.f2(current, s1[1:]) + self.f2(current + [s1[0]], s1[1:])
        return [current]
a = []
n = int(input("Enter number of elements of list: "))
for i in range(0,n):
    b = int(input("Enter element: "))
    a.append(b)
print ("Subsets: ")
print (sub().f1(a))

NOTE - Method f2 is a recursive function.

Here are some example inputs and outputs:

Enter number of elements of list: 2
Enter element: 4
Enter element: 5
Subsets:
[[], [5], [4], [4, 5]]

Enter number of elements of list: 4
Enter element: 3
Enter element: 5
Enter element: 7
Enter element: 28
Subsets:
[[], [28], [7], [7, 28], [5], [5, 28], [5, 7], [5, 7, 28], [3], [3, 28], [3, 7], [3, 7, 28], [3, 5], [3, 5, 28], [3, 5, 7], [3, 5, 7, 28]

So, I would like to know whether I could make this program shorter and more efficient.

Any help would be highly appreciated.

I. This chunk of code:

a = []
n = int(input("Enter number of elements of list: "))
for i in range(0,n):
    b = int(input("Enter element: "))
    a.append(b)

is mostly C-like (or Pascal-like etc). You can replace it with more Pythonic:

a = input('Enter elements splitted by ":" ').split(' ')

II. Here:

print (sub().f1(a))

You are creating a sub instance within the print function. It is a bad idea because it will disappear after print. You should create it before you print something (and, yes, classes names in Python are in CamelCase):

waka = Sub()
print(waka.f1(a))

III. I prefer to create __init__() function each time I create a class. It is not really necessary but in most cases, it is the first one is creating in the new class:

class Sub(object):
    def __init__(self):
        pass

IV. If it is your homework you should do with recursion, the code is pretty OK. In another case, I recommend you to use Python itertools module (you should avoid recursion as much as possible):

    def f2(self, current, s1):
        return [
            e
            for e in itertools.chain.from_iterable([
                [sorted(l) for l in itertools.combinations(s1, length)]
                for length in range(1, len(s1))
            ])
        ]

So here is the result code for recursion version:

class Sub(object):
    def __init__(self):
        pass

    def f1(self, s1):
        return self.f2([], sorted(s1))

    def f2(self, current, s1):
        if s1:
            return self.f2(current, s1[1:]) + self.f2(current + [s1[0]], s1[1:])
        return [current]

a = input('Enter elements splitted by " ": ').split(' ')
waka = Sub()
print(waka.f1(a))


Drawing a planetary system in Pygame I am new to programming and mostly self-taught by following a few MOOCs and doing little projects. I have reached the point where I am trying to develop more complicated programs (numerical models at work and games at home) and I would like to be able to give a structure to my programs to enhance readability.

I am following an Edx course called Systematic Program Design which offers methods on how to design functions, data and world in a consistent manner.

However, I have come to notice that the method seems to work mostly with functional programs rather than the object-oriented(ish) code I tend to write (especially for games). This course made me aware that my code was full of methods with almost exclusively side effects changing attributes. Which makes it hard to follow the method and makes the code look really chunky. I was wondering if anyone knows of a course/book on how to increase readability of OO code. I use Python 2.7.1.

As I said, I am self-taught, so I apologies if the answer seems trivial to some of you who have done CS.

EDIT:

The github link to my game is Far and Away v1

And this is the picture of my  . The idea is that every object in the game has a game attribute linked to the Game() object which I use as a waypoint to access all objects in my game whilst keeping them in separate files.

EDIT END

To give you an example of what my code looks like and why I would like to have a method to improve it, here is one method of a game I've made:

    #Displays the map of planets and handles user interaction with the map
def view_solarsys(self,offset):
        planets_to_blit = []
        #checks for discovered planets and draws lines between them
        #as well as creating a list with them, that will be blitted to the screen
        for p in self.game.all_planets:
            if self.game.player.logbook[p.name].is_discovered == True:
                [pygame.draw.line(self.screen, (0,250,0), p.pos, p2.pos, 5) for p2 in p.planets_in_SOF if self.game.player.logbook[p2.name].is_explored and self.game.player.logbook[p.name].is_explored]
                planets_to_blit.append(p)
                #if mouse is over planet, the hoovered attribute is updated to the planet instance
                if p.rect.collidepoint(pygame.mouse.get_pos()) : self.hoovered = p
        #Blits discovered planets to screen. If they are not explored blits a red halo around them as well. If the player's location is on the planet blits a green halo around it.
        for p in planets_to_blit:
            if self.game.player.logbook[p.name].is_discovered == True:
                if self.game.player.logbook[p.name].is_explored == False:
                    pygame.draw.circle(self.screen, (255,0,0), p.pos, int(p.rect.w*0.6), 0)
                if self.game.player.location == p.name:
                    pygame.draw.circle(self.screen, (0,255,0), p.pos, int(p.rect.w*0.75), 0)
                fn.blitc(self.screen, Data.images_planets[p.img_ref], p.pos)

            '''Mouse interaction'''
            if p.rect.collidepoint(pygame.mouse.get_pos()) and self.game.map_active:
                if pygame.mouse.get_pressed()[0]:
                    if self.game.player.logbook[p.name].is_explored == False:
                        p.explore(self.game.player)
                    else:
                        p.visit(self.game.player)
                elif pygame.mouse.get_pressed()[2] and self.game.pressed_right_clic == True:
                    p.search_in_SOF(self.game.player,True,30)
                    self.game.pressed_right_clic = False

            '''blitting planet info of hoovered planet'''
            self.view_planet()

Your code is very procedural. Do this, do that, do another thing. There's nothing inherently wrong with this, but the only OOP in the code you've shown is that of pygame itself. There's a little bit hinted at by the game and player objects, and perhaps the logbook dictionary, which appears to hold some other kind of object. But the way you interact with all of these things is all very procedural.

So, how can you better leverage OOP?

The biggest change I would suggest is to avoid big long series of attribute accesses. Things like self.game.player.logbook[p.name].is_explored represent at best a misuse of OOP. What does this code try to do? It tries to answer if a player has explored a planet. How does it do so? By asking if the planet's name appears in the player's logbook, and if the resulting object has the right attribute set.

So how should this question look in code? It should mention as few of the other objects as possible. Ideally it would look like one of the following lines; choose one and stick with it:

player.has_explored(planet)
planet in player.explored_planets
planet.explored_by(player)
player in planet.explorers
game.planet_explored_by(planet, player)

Now, you might have to get your local planet or player variable from some other collection, but you don't want to have to know it's tracked in the game's player's logbook, indexed by planet's name, and stored in an an attribute called is_explored. Only the tiny function that answers that question should have to know any of that extra stuff, and then if you ever move the information, only that one function has to be updated.

Once you've chosen the new way to ask if a planet has been discovered and explored, update your code to use it. Then look for repetition, or otherwise unnecessary checks. They're distracting to read, and possibly slow your code down. For example, you currently create a list planets_to_blit that includes only planets that have been discovered. Then you iterate over this list to draw halos, but first check again whether the planet was discovered.

If you break the function into multiple pieces (say one helper that retrieves the planets, and another that draws halos), it's hard to know whether the check was performed before calling the second helper. However, instead of repeating the check as an if, consider adding it as an assert in the halo drawing helper. This documents to the reader of the code that the helper should only receive planets that have been discovered, and the resulting exception helps you to find callers that didn't make sure.

There are other small or cosmetic changes I would suggest:

Don't be explicit about comparing against True and False; prefer if x: ... or if not x: ...
Leverage some of the self-documenting features of python: in particular consider using docstrings instead of a comment before a function (on the flip side, consider avoiding the docstring-like string literals anywhere else in a function; use comments or extra functions if you need these)
Use whitespace a bit more liberally: tastes differ, but I like to group related lines into blocks and use empty lines before and after, and I like to have spaces after the # character
Be careful with spelling: in specific, hoover is a vacuum cleaner; hover is the term you're looking for

Now I want to circle back to your question about side effects. Once you've split up your code into smaller helper methods, a good split will often have some of the following properties for each method:

The method only relies on knowledge about one area - this is the first thing I talked about
The method will either only retrieve and filter data, or it will only modify data - for example, one method could retrieve all discovered planets, and another could draw lines for the planets that are passed to it
The method doesn't assume things that aren't in its name - for example a get_planets method wouldn't filter the results to just the discovered planets

If we look at your original code, the view_solarsys function violates all of these to some degree. It would be a lot nicer if it looked more like this:

def draw_solarsys(self):
    """Draw the discovered planets, and links between them."""

    discovered_planets = self.get_discovered_planets()
    for planet in discovered_planets:
        self.draw_SFO_for_planet(planet)

    mouse_pos = pygame.mouse.get_pos()

    for planet in discovered_planets:
        self.draw_planet_with_aura(planet)

        if planet.rect.collidepoint(mouse_pos):
            self.draw_planet_info(planet)
            if self.game.map_active:
                self.handle_clicks(planet)

Is that perfect? Of course not. But I think it helps.


Java Word Frequency Counter This is a simple word frequency counter in Java. I am just wondering if I can improve or compact this code better.  You choose a file to be counted with a JFileChooser, and the results get outputted into a file.  The biggest struggle I had was sorting the HashMap by values. I got it to work but I feel like it should be easier to sort.  I'm also wondering if I should break up my code in main into more methods.  I saw a Python script of this that does the same thing but in half the lines of code.  Is that just Java or does my code need touching up?

import java.io.BufferedWriter;
import java.io.File;
import java.io.FileReader;
import java.io.FileWriter;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;

import javax.swing.JFileChooser;

public class FrequencyCounter {

    public static void main(String[] args) throws IOException {
        JFileChooser fc = new JFileChooser();
        File file = null;
        fc.setCurrentDirectory(new File(System.getProperty("user.dir")));
        if (fc.showOpenDialog(null) == JFileChooser.APPROVE_OPTION) {
            file = fc.getSelectedFile();
        }
        FileReader is = new FileReader(file);
        char[] scriptBytes = new char[(int) file.length()];
        is.read(scriptBytes);
        is.close();
        String script = new String(scriptBytes);
        String[] words = script.split("[ \\r?\\n\\-]");
        List<String> wordList = new ArrayList<>(Arrays.asList(words));
        wordList = cleaner(wordList);
        HashMap<String, Integer> counter = new HashMap<>();
        for (String word : wordList) {
            if (counter.containsKey(word))
                counter.put(word, counter.get(word) + 1);
            else
                counter.put(word, 1);
        }
        counter = entriesSortedByValues(counter);
        BufferedWriter writer = new BufferedWriter(new FileWriter(new File(file.getAbsolutePath().replace(file.getName(), "") + "result_" + file.getName())));
        for (Entry<String, Integer> ent : counter.entrySet()) {
            writer.write(ent.getKey() + " " + ent.getValue());
            writer.newLine();
        }
        writer.close();
    }

    private static List<String> cleaner(List<String> words) {
        for (int i = 0; i < words.size(); i++) {
            words.set(i, words.get(i).replaceAll("[1234567—890!@#$%^&*()_+|\\-=~`{}\\[\\]:;\"<>,.?/]","").toLowerCase());
        }
        words.removeAll(Arrays.asList(""));
        return words;
    }

    private static <K,V extends Comparable<? super V>>
        HashMap<K, V> entriesSortedByValues(Map<K,V> map) {

        List<Entry<K,V>> sortedEntries = new ArrayList<Entry<K,V>>(map.entrySet());

        Collections.sort(sortedEntries,
            new Comparator<Entry<K,V>>() {
                @Override
                public int compare(Entry<K,V> e1, Entry<K,V> e2) {
                    return e2.getValue().compareTo(e1.getValue());
                }
            }
        );
        HashMap<K,V> result = new LinkedHashMap<K,V>();
        for (Entry<K,V> ent : sortedEntries)
            result.put(ent.getKey(), ent.getValue());

        return result;
    }

}

It's normal for Java programs to be longer. I wouldn't care about the number of lines per se.
The main method does too much stuff:

It asks the user to choose the file
It reads the content of the file
It counts the number of occurrences
It prints them to another file

One method should be responsible for one thing. There're at least 4 different (very different) things here.

In fact, I believe that the first thing (and probably the second and the last thing, too) doesn't belong in the FrequencyCounter class. Why? It makes sense for the counter to count occurrences and do just it. It'll make it more reusable. Think what happens if you want to change the output format, the way the user choose the file or just use it as part of your different project.
I would redesign the FrequencyCounter it so that it has one method that takes a string and returns a map with different words and the number of their occurrences.
The reading/writing stuff and the user dialog belong to a different class. These are different concerns. So I'd create another class to handle file I/O and one more class to interact with a user as these things have nothing to do together. They can change independently and they don't need each other to work. They're separate entities.
You don't handle the case when the file is null (that is, when the user doesn't choose a file). A NullPointerException isn't very user-friendly.
This piece of code:

 if (counter.containsKey(word))
     counter.put(word, counter.get(word) + 1);
 else
     counter.put(word, 1);

can be simplified to counter.put(counter.getOrDefault(word, 0) + 1); unless you're using an older Java version.
If you plan to work with large files (really large, such that their content doesn't fit into main memory), you'll need to get rid of reading the entire file into a String. A more space-efficient solution would be to read it in chunks and process them separately (the words are normally short, so it can save a lot of memory). Sure, it makes the code more complicated (for instance, you'll need to carefully check when the word ends), so it's worth doing only if you plan to work with large amounts of data.
A method name is normally a verb. cleaner doesn't sound very good to me. I'd rename it to something like cleanWords. I'd also put cleaning one word into a separate method to make it more manageable. You can go even further and put all cleaning stuff into a separate class to increase the flexibility (it also makes sense from a design point of view: cleaning and counting are different responsibilities and they are pretty much independent).
The way you work with the output file is potentially dangerous: if such a file already exists, it'll be overwritten. I don't think that doing it silently is a good idea. There're a few different ways to deal with it: generating a different unique name, aborting the program or letting the user choose the output file. Either of them is better than replacing the content of the file silently.


Javascript assignment as expression in fat-arrow one-liner I have a promise where I'm taking the eventual result from the callback and storing it in a value that's scoped outside of the current block in a before hook, all in one line, like this.

describe('mongo stuff', () => {
  let project;

  before(() => {
    return mongo.findFirst('projects').then(data => project = data);
  });

  it('should have the correct company name', () => {
      expect(project.projectName).to.equal('Company1');
  });

});

Note: Mocha, the test runner I'm using, understands promises. That means, placing a return statement before a promise in a before hook, or in an it test, will halt the execution of the next task in the mocha test runner until the promise has been resolved. In other words, data => project = data will always completely execute before any tests that evaluate the contents of project are run.

I use data as a temp variable just so I can assign it to the higher-scoped project variable. This lets me use it in some later test like in the example above.

I was told in a code review that using an assignment as an expression isn't as clear as writing it out.

return mongo.findFirst('projects').then(data => {
    project = data;
});

Which do you think is better? Do you have any solid justification for avoiding the one-liner?

The only thing I think is "bad" when using the one-liner arrow is the implicit return. Whatever the expression evaluates to becomes the return value of the arrow function. In the case of promises, you should be careful when returning from within then callbacks since the return value becomes the resolved value of the promise.

Now in your case, the return value of then isn't going anywhere since the promise is not being used. So it shouldn't matter which form you should use. Therefore it should be fine.

A bigger problem is the entire snippet. You should make sure that project has a value by the time the code using it uses it or ensure that the code using it only runs after you fetch the value of project. If possible, since you have to wait for project anyways, you can make the call to get project synchronous instead.