log4j 1.2 - own check for verbose logs? In my code's package, I have a flag such as:

public static final boolean LOGS_VERBOSE = false;

And in worker classes (multi threaded Runnable jobs), I have code like:

if(XYConstants.LOGS_VERBOSE){
    logger.debug("some state info var 1:" + var1 + ", var 2:" + var2);

Is this a good idea? Is there any performance gain? Developers say it avoids code going to the log4j level completely, and there is hardly any overhead to check the value of the constant.

We do plan to do performance testing, but that is a month away, and I need to show code to the client (who is a developer of 14 years) this Friday. I'm wondering if this should stay or not?

We have some debugs without the boolean check too, and those are wanted in all runs as long as log4j is configured in debug level.

This code is using log4j 1.2.16.

This type of practice is common, and works well.

The Java JIT compiler will identify the LOGS_VERBOSE as being a constant, and will know that it is always false, and thus will compile-out the logging entirely.

There is a slight performance hit on any code that has not (yet) been compiled.... but, once the compile has happened, there is no hit at all.

It is relatively common to use a System property to determine whether to log...

public static final boolean LOGS_VERBOSE = Boolean.getBoolean("LOGS_VERBOSE");

This behaves in the exact same way (because JIT happens after the property is set)... but, if you start your program as:

java -DLOGS_VERBOSE=true ......

then suddenly you have the fully-featured logging.

In many cases this sort of checking can improve performance because it can eliminate expensive 'setup' for log messages


Template function specialization with code duplication I'm working on a libcurl binding for NodeJS, and to get info from a curl handler, using curl_easy_getinfo, I've created a template function, the issue, is that the result of the call can be a null pointer.

To make sure this was correctly addressed, I've created a template specialization that handle the edge case:

template<typename ResultType, typename v8MappingType>
v8::Handle<v8::Value> Curl::GetInfoTmpl( const Curl &obj, int infoId )
{
    v8::HandleScope scope;

    ResultType result;

    CURLINFO info = (CURLINFO) infoId;
    CURLcode code = curl_easy_getinfo( obj.curl, info, &result );

    if ( code != CURLE_OK )
        return Curl::Raise( "curl_easy_getinfo failed!", curl_easy_strerror( code ) );

    return scope.Close( v8MappingType::New( result ) );
}

//The null pointer is only possible when the value returned is a c string
template<> //workaround for null pointer, aka, hack.
v8::Handle<v8::Value> Curl::GetInfoTmpl<char*,v8::String>( const Curl &obj, int infoId )
{
    //duplicated code
    v8::HandleScope scope;

    char *result;

    CURLINFO info = (CURLINFO) infoId;
    CURLcode code = curl_easy_getinfo( obj.curl, info, &result );

    if ( !result ) { //null pointer
        return v8::String::New( "" );
    }

    if ( code != CURLE_OK )
        return Curl::Raise( "curl_easy_getinfo failed!", curl_easy_strerror( code ) );

    return scope.Close( v8::String::New( result ) );
}

Is this the correct way to handle that, or are there better ways to do so?

The full code is on GitHub.

The check is better factored into a specialization on a traits class. This allows you to have one implementation of the overall logic flow, and just specialize the part that changes. I haven't fully researched the implications of this approach, but I expect that due to the always true/false nature of the check (for most given types), that it will be easily optimized away.

It would look something like this:

// traits class to determine whether to do the check
template <typename> struct ResultCanBeNull : std::false_type {};
template <> struct ResultCanBeNull<char*> : std::true_type {};

template<typename ResultType, typename v8MappingType>
v8::Handle<v8::Value> Curl::GetInfoTmpl( const Curl &obj, int infoId )
{
    v8::HandleScope scope;

    ResultType result;

    CURLINFO info = (CURLINFO) infoId;
    CURLcode code = curl_easy_getinfo( obj.curl, info, &result );

    // use the traits class to filter in or out the null pointer check
    if ( ResultCanBeNull<ResultType>::value && !result ) { //null pointer
        return v8::String::New( "" );
    }

    if ( code != CURLE_OK )
        return Curl::Raise( "curl_easy_getinfo failed!", curl_easy_strerror( code ) );

    return scope.Close( v8MappingType::New( result ) );
}

You could also implement the traits support as a method instead, in case it's not valid to call !result on instances of other types that are reasonable for ResultType.

template <typename ResultType>
struct ResultTraits {
    bool IsNull(const ResultType&) { return false; }
};
template<> bool ResultTraits<char const*&>::IsNull(char const*& pchar) {
    return !pchar;
};

: : :

    if (ResultTraits<ResultType>::IsNull(result)) {
        return v8::String::New( "" );
    }


ANOVA Using Continuous CDFs I have a procedure for conducting ANOVA, the details of which can be viewed in this white paper.

I would like to be able to make the routine scalable to handle any number of groups. Below is the R code for a 2 group example.  Any insights or comments are appreciated.

Partial Moments:

LPM<- function(degree,target,variable){
 sum((target - (variable[variable < target]))^degree)/length(variable)
}

UPM<- function(degree,target,variable){
 sum(((variable[variable > target]) - target)^degree)/length(variable)
}

ANOVA:

VN_ANOVA<- function(group1,group2){

  mean_of_means <- mean(c(mean(group1),mean(group2)))

#Continuous CDF for each group from Mean of Means
  LPM_ratio_1 <- LPM(1,mean_of_means,group1)/
    (LPM(1,mean_of_means,group1)+UPM(1,mean_of_means,group1))

  LPM_ratio_2 <- LPM(1,mean_of_means,group2)/
    (LPM(1,mean_of_means,group2)+UPM(1,mean_of_means,group2))

#Continuous CDF Deviation from 0.5
  MAD_CDF<- mean(c(abs(LPM_ratio_1 - 0.5),abs(LPM_ratio_2 - 0.5)))

#Certainty associated with samples
  VN_ANOVA_rho <- (0.5 - MAD_CDF)/0.5

#Graphs
  boxplot(list(group1,group2), las=2, names=c("Group 1","Group 2"),
      xlab= "Means", horizontal = TRUE,
      col=c("grey","white"), main="ANOVA")

#For ANOVA Visualization
  abline(v=mean_of_means,col="red",lwd=4)
  text(mean_of_means,pos=4, 2.5, "Mean of means", col = "red")

  return(c("Certainty of Same Population"=VN_ANOVA_rho))

}

Per the answer to this question, storing the variables into a matrix works.

VN_ANOVA<- function(A){

  mean_of_means <- mean(colMeans(A))
  n<- ncol(A)

  LPM_ratio = numeric(0L)
  MAD_CDF = numeric(0L)

#Continuous CDF for each variable from Mean of Means
  for (i in 1:n){
      LPM_ratio[i] <- LPM(1,mean_of_means,A[,i])/
                 (LPM(1,mean_of_means,A[,i])+UPM(1,mean_of_means,A[,i]))

#Continuous CDF Deviation from 0.5
      MAD_CDF[i]<- abs(LPM_ratio[i] - 0.5)
      }

  Mean_MAD_CDF <- mean(MAD_CDF)

#Certainty associated with samples
  VN_ANOVA_rho <- (0.5 - Mean_MAD_CDF)/0.5

#Graphs
  boxplot(A, las=2, xlab= "Means", horizontal = TRUE,
       main="ANOVA", col=rainbow(n))

#For ANOVA Visualization
  abline(v=mean_of_means,col="red",lwd=4)
  text(mean_of_means,pos=4, .25,"Mean of means", col = "red")

  return(c("Certainty of Same Population"=VN_ANOVA_rho))

}


Bresenham line drawing implementation I found myself having to implement Bresenham's line drawing algorithm.

I'm looking for feedback on effectivity and code style. Can I reduce the code somehow?

template<typename Callable>
void bresenham(Vec2i p0, Vec2i p1, Callable&& cb){
    const auto swap_xy = std::abs(p1.y - p0.y) > std::abs(p1.x - p0.x);
    if (swap_xy) {
        swap(p0.x, p0.y);
        swap(p1.x, p1.y);
    }

    auto mark = [swap_xy, &cb](Vec2i p) {
        if (swap_xy) {
            cb(Vec2i (p.y, p.x));
        }
        else {
            cb(p);
        }
    };

    const auto d = p1 - p0;
    const auto dx_abs = std::abs(d.x);
    const auto step_y = d.y < 0 ? -1 : 1;
    const auto step_x = d.x < 0 ? -1 : 1;
    auto pen = p0;
    while (pen.x != p1.x) {
        bool skip = false;
        auto error = std::abs(2 * d.y*(pen.x - p0.x) - 2 * d.x*(pen.y - p0.y));
        while (error > dx_abs) {
            pen.y += step_y;
            error -= 2 * dx_abs;
            mark(pen);
            skip = true;
        }
        if (!skip) {
            mark(pen);
        }
        pen.x += step_x;
    }
    mark(p1);
}

The class Vec2i is the equivalent of:

struct Vec2i{
   Vec2i(int ax = 0, int ay = 0) : x(ax), y(ay) {}
   int x,y;
};

with the expected copy constructor and arithmetic operators that one usually associates with a mathematical vector.

I have unit tested this extensively and found no issues. Although I'm a bit worried as most implementations I've seen are much longer (2-3x) and have special handling for a lot of cases, cases that just seem to work in my unit tests.

I accept any type of callback that will be called for each produced position. Some times I need to raster the line, some times I just need to iterate along the line and do things. This seemed appropriate to me.

That seems good. I really like the callback mechanism to do whatever we want with the marked positions and not one specific action. That makes for a generic tool; good design. I don't have much to say to improve the algorithm itself, but here are my two cents anyway:

Do you have a using std::swap; anywhere? Otherwise, I don't think that the unqualified calls to swap will work as intended.
While auto is a great tool, there are some times when I don't think that it adds to readability. For example, it took me more time that I would have wanted to see that swap_xy was a bool. That's prone to debate anyway, but I don't think that you will ever replace it by another type and an explicit bool would have highlighted the logic. That's still subjective though.
Still pretty subjective, but instead of this:

auto pen = p0;
while (pen.x != p1.x) {
    // ...
    pen.x += step_x;
}

I would have used a for loop since you explicitly have an initialization, a condition and a step. And everyone uses the same variable pen, which isn't used after the loop.

for (auto pen = p0; pen.x != p1.x; pen.x += step_x) {
    // ...
}

Nitpicking again, but I would have factored the multiplication by 2 out of the std::abs in the computation of error:

auto error = 2 * std::abs(d.y*(pen.x - p0.x) - d.x*(pen.y - p0.y));

By the way, this factorization highlights something else: you actually do not need to keep the multiplication by 2 in error; you only need it for the comparison:

auto error = std::abs(d.y*(pen.x - p0.x) - d.x*(pen.y - p0.y));
while (2 * error > dx_abs) {
    pen.y += step_y;
    error -= dx_abs;
    mark(pen);
    skip = true;
}

Not that it makes a big difference, but it still makes the code a little bit terser.


New to programming. How can I write this Rock, Paper, Scissors code better? I am practicing w/ Python and wanted to code a simple Rock, Paper, Scissors game w/o any user input. I used a list to hold the values and randomly pulled them with a function. I used the the return value from the first function to print the response in the second function, along with the printed results. What are some alternative methods to achieve this same functionality and make this more efficient. Thank you.

# Internal Rock, Paper Scissors Game  >>>>>>>>>>>>>>>>>>>>>>>>>

import random

# List string values for the game

r_p_s = [
    'Rock', 'Paper', 'Scissors'
]

# Function to return random values from List
def game():
    return random.choice(r_p_s) + " vs. " + random.choice(r_p_s)

result = game()  # Returned Value

# Function to print out the results of the returned value with a response

def response():
    print("Roll: " + "\n" + result)

    if "Scissors vs. Paper" == result or \
       "Paper vs. Scissors" == result:
        print("\nScissors beats Paper")

    elif "Scissors vs. Rock" == result or \
         "Rock vs. Scissors" == result:
        print("\nRock beats Scissors!")

    elif "Rock vs. Paper" == result or \
         "Paper vs. Rock" == result:
        print("\nPaper beats Rock!")

    else:
        print("\nDraw! Roll again.")

response()

Despite of very simple and "verbose" initial implementation it has a bunch of issues:

r_p_s = ['Rock', 'Paper', 'Scissors'] is better defined as constant of immutable values
result is accessed within response() function body as a global variable.Instead, it's better called within the function on demand:

def response():
    result = game()
    ...

instead of arbitrary strings concatenation ... + " vs. " + ..., "Roll: " + "\n" + result - use flexible f-string formatting.
For ex.: print(f"Roll: \n{result}")
the conditions like:

if "Scissors vs. Paper" == result or \
    "Paper vs. Scissors" == result:
    print("\nScissors beats Paper")

are too verbose and hard-coded to be manageable and reliable

Now, when you realize the bad things described above and wonder if there a manageable and advanced approach - take and consider the one below, which is OOP approach representing RPSGame class (as a main class) and powered by enum classes to represent the crucial shapes.
The new implementation itself is pretty descriptive (I hope). Enjoy!

import random
from enum import Enum

class RPSGame:
    class Shape(Enum):
        R = 'Rock'
        P = 'Paper'
        S = 'Scissors'

    # winning outcome bindings
    WIN_OUTCOMES = {
        (Shape.P, Shape.R), (Shape.R, Shape.S), (Shape.S, Shape.P),
    }

    @staticmethod
    def choice():
        shapes = list(RPSGame.Shape)
        return random.choice(shapes), random.choice(shapes)

    def play(self):
        shape1, shape2 = self.choice()
        print(f"Roll: \n{shape1.value} vs {shape2.value}\n")

        if shape1 == shape2:
            print("Draw! Roll again.")
        else:
            # check to swap shapes to get winning output order
            if (shape1, shape2) not in RPSGame.WIN_OUTCOMES:
                shape1, shape2 = shape2, shape1
            print(f"{shape1.value} beats {shape2.value}!")

if __name__ == "__main__":
    rps = RPSGame()
    rps.play()
    rps.play()
    rps.play()

Sample output (from 3 plays):

Roll:
Paper vs Scissors

Scissors beats Paper!
Roll:
Scissors vs Scissors

Draw! Roll again.
Roll:
Rock vs Paper

Paper beats Rock!


DOM building using DocumentFragment I want to build the markup below by iterating on an array of objects.  The structure of the object is also listed below.  I need some tips on how to make it clean.

Markup I want to build:

Crowne Plaza Hotel

                    <s>S$ 275</s>
                S$ 275

JavaScript:

//the object structure of each data
{
    name: "Hotel A",
    loc: "Prontera Rd",
    dist: 9,
    img: "img/rendezvous-hotel.jpg",
    price: 350,
    deal_price: 350,
    currency: "S$",
    star_rating: 4
}

function constructDOMFragment(data) {
    var fragment = document.createDocumentFragment();
    for (var i = data.length - 1; i >= 0; i--)(function(obj) {

        var div_container = document.createElement("div");
        div_container.className = "pull-left col-xs-4";

        var div_thumbnail = document.createElement("div");
        div_thumbnail.className = "thumbnail";

        div_container.appendChild(div_thumbnail);

        var div_caption_top = document.createElement("div");
        div_caption_top.className = "caption";

        var h5 = document.createElement("h5");
        h5["innerHTML" || "textContent"] = obj.name;
        div_caption_top.appendChild(h5);
        div_thumbnail.appendChild(div_caption_top);

        var img = document.createElement("img");
        img.className = "img-responsive";
        img.src = obj.img;
        img.alt = obj.name;

        div_thumbnail.appendChild(img);

        var div_caption_bottom = document.createElement("div");
        div_caption_bottom.className = "caption";

        var h3_deal_price = document.createElement("h3");
        h3_deal_price.className = "pull-right";
        if (obj.price !== undefined && obj.price && obj.price != obj.deal_price) {
            var price = document.createElement("small");
            var strikethrough = document.createElement("s");
            strikethrough["innerHTML" || "textContent"] = obj.currency + " " + obj.price;
            price.appendChild(strikethrough);
            h3_deal_price.appendChild(price);
        }
        h3_deal_price.appendChild(document.createTextNode("  " + obj.currency + " " + obj.price));

        div_caption_bottom.appendChild(h3_deal_price);

        var clearfix = document.createElement("div");
        clearfix.className = "clearfix";
        div_caption_bottom.appendChild(clearfix);

        div_thumbnail.appendChild(div_caption_bottom);
        fragment.appendChild(div_container);

    })(data[i]);

    var clearfix = document.createElement("div");
    clearfix.className = "clearfix";
    fragment.appendChild(clearfix);

    return fragment;
};

There are several problems in your code:

What is r.price?

h5["innerHTML" || "textContent"] = obj.name makes no sense. You could just a well write h5["innerHTML"] = obj.name. Your code won't test if such properties exist.

Even h5["innerHTML"] || h5["textContent"] won't do much good, since that just returns the value of either property, so you can't use it to set it.

You'd need to use something like

if (typeof h5.innerHTML == "undefined") { // no need for bracket notation here any more
  h5.innerHTML = obj.name
} else {
  h5.textContent = obj.name
}

However even that isn't very nice, since the element may have innerHTML set even if the browser doesn't actually use it.

Also innerHTML and textContent are "incompatible" to each other. For example, if you would assign the string "Example" to them, then using innerHTML will parse the HTML and display the text Example (in italic), while textContent will display the literal text Example.

The cleanest way would be to create and append explicit text nodes.

I don't see any benefit in using a function as the body of the for loop.

Generally building a fragment like this isn't very practical. For one it's terribly slow. For the other it's very difficult to read and makes changing the HTML structure a real bother, especially since it contains presentational information, such as Bootstraps grid classes and the clearfix elements (which IMHO are usually unnecessary nowadays).

Consider using a template mechanic of some kind. Either build something simple yourself, or use one of the plenty JavaScript template libraries, such as mustache.


Modifying database using POSTed variables in PHP I am writing PHP code to update a database, but this code is extremely repetitive and thus error prone. I am effectively typing the variable names six times each! Can this be improved without loosing functionality? I am certain there must be better way!

First, the definition of the demandArrayValue function:

function demandArrayValue($array, $key) {
  $value = isset($array[$key]) ? $array[$key] : null;
  if (is_null($value)) {
    throw new Exception("Failed to get value for '".$key."'.");
  }
  return $value;
}

Next an example of my extremely repetitive code:

try{
  if ($_POST) {
    $var1 = demandArrayValue($_POST, "Var1");
    $var2 = demandArrayValue($_POST, "Var2");
    $var3 = demandArrayValue($_POST, "Var3");
    [ ... and so on. ]

    $query = "UPDATE Table SET Var1 = :var1, Var2 = :var2, Var3 = :var3, [...] WHERE VarX = :varX";
    $stmt = $db->prepare($query);
    $stmt->bindParam(':var1', $Var1);
    $stmt->bindParam(':var2', $Var2);
    $stmt->bindParam(':var3', $Var3);
    [ ... and so on. ]
    $stmt->execute();
  }
}
catch [ ... ]

Can a better PHP programmer than I am help me out please? Many many thanks in advance!

Disclaimer: I asked this same question on stackoverflow and was pointed to this site as a potentially better location.  I have no idea how you move a question from one site to another, so I am sorry to duplicate this.

If I were you, I'd create a function that looked like this:

function getArrayValues( array $array, array $keys)
{
    $result = array();
    foreach($keys as $key)
    {
        if (!isset($array[$key]))
        {
            throw new RuntimeException($key.' is not set');
        }
        $result[$key] = $array[$key];
    }
    return $result;
}

This function will return an array, containing just those keys you were actually looking for. Then, in your try-catch block, I'd write this:

$values = getArrayValues($_POST, array('var1','var2','var3'));
$stmt = $db->prepare('your query here');
$stmt->execute($values);

And that's it.
Now I do have some worries/thoughts/suggestions about your code, but I'll add them in a future edit.

Edit:
You're throwing exceptions when an array-key does not exist. In this particular case, that might be desirable. You can't insert a row in your db, lest all data is at hand. I get that. But how often might you end up writing either:

try
{
    $var = demandArrayValue($someArray, 'isNotSet');
}
catch(Exception $e)
{
    $var = 'default value';
}

Or, to avoid having to write the try-catch too many times:

$var = isset($someArray['notSet'] ? $someArray['notSet'] : 'default value';

You could write 2 distinct functions: one throwing exceptions, one that doesn't, but that's just silly. getting array-keys shouldn't throw an exception. your code should be able to handle a missing parameter.
What's more, you place the demandArrayValue in the same try-catch block as all of your DB operations. If you're using PDO, and you've set the PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION, then you're actually handling 2 distinct exceptions in the same catch block:

catch(Exception $e)
{//you have to catch Exception, since that's what demandArrayValue throws
    if ($e instanceof PDOException)
    {
        $db->rollBack();
        echo $e->getMessage();//handle DB exception
    }
    else
    {
        echo $e->getMessage();//handle other Exception
    }
}

Now, that's not nice. Besides, like I said before: dealing with the request parameters is something that is, actually, quite a complex task, but most, if not all frameworks, implement a (series of) request objects. You can write your own, too.
Once you've done that, you can actually use the getKey or get or getAll methods you might implement:

class PostRequest
{
    private $raw = null;
    private $data = null;
    public function __construct(array $data = null)
    {
        $data = $data ?: $_POST;
        $this->raw = $data;
        $this->setData();
    }
    public function get($name, $default = null)
    {
        if (isset($this->data[$name]))
        {
            return $this->data[$name];
        }
        return $default;
    }
    private function setData()
    {
        $this->data = array();
        foreach ($this->raw as $k => $v)
        {//validate, sanitize, decode... do whatever
            $this->data[$k] = trim($v);
        }
        return $this;
    }
}

You could expand on this basic template, with custom email validation, or number formatting and what have you, but you could just  as well implement some form of constant-system, to set the get method's behaviour:

const POST_EXCEPTION = 1;
const POST_DEFAULT = 0;
private $exceptionMode = false;

public function setExceptionMode($exceptionMode = self::POST_DEFAULT)
{
    $this->exceptionMode = !!$exceptionMode;
    return $this;
}
public function get($name, $default = null)
{
    if (!isset($this->data[$name]))
    {
        if ($this->exceptionMode === true)
        {
            throw new Exception($name. ' key is not set');
        }
        return $default;
    }
    return $this->data[$name];
}

This get method does everything yours does, and more... but please, do look into the linked answer, and look into how other frameworks deal with the request. It's all been done before, and you're free to copy with pride


Split big file into separate files depending on column's distinct values I have a big file, which contains column names in the first row. The data is about 20GB and consists of 10s of millions of rows. Every other row contains data which I would like to append to other files depending on the distinct entries in a particular column (depending on index). When the program encounters a new distinct entry it creates the file other wise it should append the data. The following piece of code is my basic attempt to achieve this (I am Python newbie):

import re
import os

input_path_file1 = 'C:\InputData.txt'
output_path = r'C:\Output\Data'
unique_values = {}

# read in first line containing column names
with open(input_path_file1, 'r') as file:
    first_line = file.readline()

rows = 0
# 7 line_number
# 35 range
index = 35
with open(input_path_file1, mode='r') as file:

    # skip first row
    next(file)

    for line in file:
        splitted_data = re.split(r'\t+', line.rstrip('\t'))
        cell_data = splitted_data[index].replace('/', '')
        target = os.path.join(output_path, cell_data + '.txt')

        if cell_data in unique_values:
            unique_values[cell_data] += 1
        else:
            unique_values[cell_data] = 1
            with open(target, 'w') as outfile:
                outfile.write(first_line)

        with open(target, 'a') as outfile:
            outfile.write(line)

        rows += 1

Could this be made more efficient?

It would be more efficient to open each file only once,
instead of reopening every time you need it.
Two obvious approaches come to mind:

Accumulate all the lines you want to write to files into a dictionary of lists, and then write to the files one by one. Given that your content is very large, this alternative may consume too much memory and therefore not suitable for your case.
Keep a dictionary of open file handles to write to.
If the lines are ordered in such a way that the lines that should go to the same file are grouped together, then you don't even need a dictionary, you could just keep track of the previous filename, so that if the filename is the same then append to the currently open file, otherwise close the current file and open a new one.

And most certainly you don't want to open input_path_file1 twice to read the first line and then again to process the rest of the file.
Opening it once would suffice, and instead of next(file) to skip the first line,
you could store it with first_line = next(file).

The variable rows is not used, so it could be removed.

The variable unique_values is a dictionary of counts,
but the counts are not used for anything.
So this could have been a set instead of a dict.


If a message is there, but you cannot see it, is it really there? I have created a space encoder program, because it is fun to generate invisible messages. (This programme is not cryptographically secure, it is just for fun)

import doctest

def to_spaces(text):
    """
    Each char with ASCII code x gets converted to a
    line of x spaces.

    >>> to_spaces('a').count(' ')
    97
    >>> to_spaces("hello").count(' ')
    532
    """
    return '\n'.join((' '*ord(char) for char in text))

def from_spaces(spaces):
    """
    Each line of x spaces gets converted to the
    char of ASCII code x.

    >>> from_spaces(' '*97)
    'a'
    """
    return ''.join((chr(len(i)) for i in spaces.splitlines()))

def encode_and_print_file():
    filename = input("Where do you want to save the encrypted text? ")
    content = input("Enter your secret message: ")
    with open(filename,"w+") as f:
        f.write(to_spaces(content))

def decode_and_print_file():
    filename = input("Which file do you want to read decoded? ")
    with open(filename) as f:
        print(from_spaces(f.read()))

def user_interface():
    while True:
        choice = input("\nDo you want to encode or decode? ")
        encode_and_print_file() if 'en' in choice.lower() \
                                else decode_and_print_file()

if __name__ == "__main__":
    doctest.testmod()
    user_interface()

Coding style looks good except for one thing. You are using a conditional expression where an if statement would do.

encode_and_print_file() if 'en' in choice.lower() \
                        else decode_and_print_file()

As the value of the expression is not used, I would prefer the statement version as it is clearer:

if 'en' in choice.lower():
    encode_and_print_file()
else:
    decode_and_print_file()

I don't see why you use if 'en' in choice.lower() to recognize the word "encode", allowing "en" anywhere. if choice.strip().lower().startswith('en') would be more to the point.


Using a function to find the average for number of letters and words This is my code:

#include <stdio.h>
#include <ctype.h>
int average_of_let_wor();
int main(void)
{
    double answer;
    answer = average_of_let_wor();

    printf("%.2lf", answer);
    return 0;
}

int average_of_let_wor()

{
    double numberOfLetters = 0;
    double numberOfWords = 0;
    int userInput;
    double answer;
    printf("please enter your input:\n");
    while ((userInput = getchar()) != EOF)
    {

        if (userInput == ' ' || userInput == '\t' || userInput == '\n')
        {
            numberOfWords++;
            continue;
        }

        else
           numberOfLetters++;

    }
    answer = numberOfLetters/numberOfWords;
    return answer;
}

First of all, be careful with types: to compute an average something, you will need a floating point number. However, the number of letters and words are countable and can therefore use the type unsigned int which are generally faster than floating point numbers.

Moreover, to detect a "space" character, you can use the standard function isspace from the header ctype.h.

Also, the variable answer is useless. You can get rid of it and simply return the answer the same line you're computing it.

Therefore, your function average_of_let_wor can be reduced to:

double average_of_let_wor()
{
    unsigned int numberOfLetters = 0;
    unsigned int numberOfWords = 0;
    int userInput;

    printf("please enter your input:\n");
    while ((userInput = getchar()) != EOF)
    {
        if (isspace(userInput))
        {
            numberOfWords++;
        }
        else
        {
            numberOfLetters++;
        }
    }

    return (double)numberOfLetters / (double)numberOfWords;
}


Find smallest subset prefixes Here is my code for this problem. Any bugs, performance in terms of algorithm time complexity, code style advice are appreciated.

The areas which I want to improve, but do not know how to improve are:

I sort the strings by length and I'm not sure if it's a smarter idea not using sorting at all.
I used a few set operations, which looks a bit ugly. I want to learn smarter ideas.

Problem:

  Given a set of strings, return the smallest subset that contains the longest possible
  prefixes for every string.

  If the list is ['foo', 'foog', 'food', 'asdf'] return ['foo', 'asdf']

The return is foo since foo is prefix for foo (itself), prefix for foog and prefix for food (in other words, foo could "represent" longer string like foog and food). Output also contains asdf because it is not prefix for any other words in the input list, so output itself.

The empty set is not a correct answer because it does not contain the longest possible prefixes.

Source code:

from collections import defaultdict
class TrieTreeNode:
    def __init__(self):
        self.children = defaultdict(TrieTreeNode)
        self.isEnd = False
        self.count = 0
    def insert(self, word, result_set):
        node = self
        for i,w in enumerate(word):
            node = node.children[w]
            if node.isEnd:
                result_set[word[:i+1]].add(word)
        node.isEnd = True

if __name__ == "__main__":
    words = ['foo', 'foog', 'food', 'asdf']
    words_length = []
    sorted_words = []
    for w in words:
        words_length.append(len(w))
    for t in sorted(zip(words_length, words)):
        sorted_words.append(t[1])
    result_set = defaultdict(set)
    root = TrieTreeNode()
    for w in sorted_words:
        root.insert(w, result_set)
    match_set = set()
    for k,v in result_set.items():
        for i in v:
            match_set.add(i)
    unmatch_set = set(words) - match_set
    print unmatch_set | set(result_set.keys())

The best solution I have for this. If my understanding is good, this is a fairly simple to solve and you do not need anything else than a set.

There might be a better way than rebuilding entirely the set.

words = ['foo', 'foog', 'food', 'asdf']
prefixes = set()

for w in words:
    if any(p for p in prefixes if w.startswith(p)):
        # A prefix exists in the prefixes set.
        continue

    # Re-build the set with all elements not matching the set.
    prefixes = set(p for p in prefixes if not p.startswith(w))
    prefixes.add(w)

print(prefixes)


A simple little Python web crawler The crawler is in need of a mechanism that will dispatch threads based on network latency and system load. How does one keep track of network latency
in Python without using system tools like ping?

import sys
import re
import urllib2
import urlparse
import requests
import socket
import threading
import gevent
from gevent import monkey
import time

monkey.patch_all(
  socket=True,
  dns=True,
  time=True,
  select=True,
  thread=True,
  os=True,
  ssl=True,
  httplib=False,
  subprocess=False,
  sys=False,
  aggressive=True,
  Event=False)

# The  stack
tocrawl = set([sys.argv[1]])
crawled = set([])
linkregex = re.compile('<a\s*href=[\'|"](.*?)[\'"].*?>')

def Update(links):
  if links != None:
    for link in (links.pop(0) for _ in xrange(len(links))):
      link = ( "http://%s" %(urlparse.urlparse(link).netloc) )
      if link not in crawled:
        tocrawl.add(link)

def getLinks(crawling):
  crawled.add(crawling)
  try:
    Update(linkregex.findall(requests.get(crawling).content))
  except:
    return None

def crawl():
  try:
    print"%d Threads running" % (threading.activeCount())
    crawling = tocrawl.pop()
    print crawling
    print len(crawled)
    walk = gevent.spawn(getLinks,crawling)
    walk.run()
  except:quit()

def dispatcher():
  while True:
    T = threading.Thread(target=crawl)
    T.start()
    time.sleep(1)

dispatcher()

I see a flurry of downloading activity, but I don't see that you do anything with the pages that you download except parse some URLs for more downloading.  There's no rate limiting or any attempt to check robots.txt, making your web crawler a poor Internet citizen.

PEP 8 mandates four spaces per level of indentation.  Since whitespace is significant in Python, you should stick to the convention.  Furthermore, function names should be lower_case(), so Update() and getLinks() should be renamed.

Just a simple call to gevent.monkey.patch_all() will do.  There is no need to from gevent import monkey, nor is there any need to list all of the keyword parameters, since you're accepting all of the defaults.

Your linkregex fails if the <a> tag contains any intervening attributes before href.  For example, <a target="_blank" href="…"> will cause a link to be skipped.

I don't believe that your code is a well behaved multithreaded program.  For one thing, you indiscriminately spawn and start one thread per second.  If the average processing time per request exceeds one second, you'll end up with an uncontrolled proliferation of threads.

Another issue is that you add() and pop() tocrawl elements without any kind of locking.  Also, if one thread fails to pop() anything (probably when the tocrawl list becomes empty), you rudely call quit() without giving other threads a chance to finish what they are doing.

Finally, you process the URLs using a stack.  Web crawling is usually done using a queue, to avoid processing clusters of closely related URLs together and concentrating the load on one unfortunate webserver at a time.


Expanding url from shortened url obtained from tweet I have a twitter data set. I have extracted all the expanded urls from the json and now am trying to resolve the shortened ones. Also, I need to check which urls are still working and only keep those.

I am parsing over 5 million urls. The problem is that the code below is slow. Can anyone suggest how to make it faster? Is there a better way to do this?

import csv
import pandas as pd
from urllib2 import urlopen
import urllib2
import threading
import time

def urlResolution(url,tweetId,w):

    try:

        print "Entered Function"
        print "Original Url:",url

        hdr = {'User-Agent': 'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.11 (KHTML, like Gecko) Chrome/23.0.1271.64 Safari/537.11',
       'Accept': 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8',
       'Accept-Charset': 'ISO-8859-1,utf-8;q=0.7,*;q=0.3',
       'Accept-Encoding': 'none',
       'Accept-Language': 'en-US,en;q=0.8',
       'Connection': 'keep-alive'}

        #header has been added since some sites give an error otherwise
        req = urllib2.Request(url, headers=hdr)
        temp = urlopen(req)
        newUrl = temp.geturl()
        print "Resolved Url:",newUrl
        if newUrl!= 'None':
            print "in if condition"
            w.writerow([tweetId,newUrl])

    except Exception,e:
        print "Throwing exception"
        print str(e)
        return None

def urlResolver(urlFile):
    df=pd.read_csv(urlFile, delimiter="\t")

    df['Url']
    df2 = df[["Tweet ID","Url"]].copy()
    start = time.time()

    df3 = df2[df2.Url!="None"]

    list_url = []
    n=0
    w = csv.writer(open("OUTPUT_FILE.tsv", "w"), delimiter = '\t')
        w.writerow(["Tweet ID","Url"])

    maxC = 0
    while maxC < df3.shape[0]:
        #creates threads
        #only 40 threads are created at a time, since for large number of threads it gives <too many open files> error
        threads = [threading.Thread(target=urlResolution, args=(df3.iloc[n]['Url'],df3.iloc[n]['Tweet ID'],w)) for n in range(maxC,maxC+40)]

        for thread in threads:
                thread.start()
        for thread in threads:
                thread.join()
        if maxC+40 >= df3.shape[0]:
            threads = [threading.Thread(target=urlResolution, args=(df3.iloc[n]['Url'],df3.iloc[n]['Tweet ID'],w)) for n in range(maxC,df3.shape[0])]

                    print "threads complete"
                    for thread in threads:
                            thread.start()
                    for thread in threads:
                            thread.join()
            break
        maxC = maxC + 40
    print "Elapsed Time: %s" % (time.time() - start)

    w.close()

if __name__ == '__main__':
    df3 = urlResolver("INPUT_FILE.tsv")

Couple things I'd try:

switch to requests module reusing the requests.Session() to let it reuse the same TCP connection:

  ..if you're making several requests to the same host, the underlying TCP
  connection will be reused, which can result in a significant
  performance increase

use the "HEAD" HTTP method (in case of requests you may need the allow_redirects=True)
try out Scrapy web-scraping framework which is of an asynchronous nature and is based on the twisted network library. You would also move the CSV output part to an output pipeline.
another thing to try is use the grequests library (requests on gevent)

Some micro-optimization ideas:

move the hdr dictionary definition to the module level to avoid redefining it every time urlResolution() is called (and, since it is a constant use upper-case; and pick a more readable variable name - HEADERS?)


Algorithm maths quiz + write to a text file Are there any ways I can clean this code up to make it more user-friendly?

import random #Random Function
import os

print("What is your name?") #Asking User Name
name = input().title() #.title() capitilises the first letter in each word, so when the name is printed in later code the first letter will always be capitilised.
class_name = input("Which class do you wish to input results for? ")
print (name, ", Welcome to the OCR Controlled Assessment Maths Test")
score = 0  #Set Score to 0
question = 0 #Set Question Number to 0
while question < 10: #After 10 Questions, it will stop asking the user questions.
    operator = random.randint(1,3)#Addition,Multiplication,Subtraction
    num1 = random.randint(1,25)#Random Number between 1 and 10 to be used in the questions
    num2 = random.randint(1,25)#Random Number between 1 and 10 to be used in the questions
    if operator == 1: #If Random Operator = 1, it will ask addition questions
        print("What is", num1, "+", num2)
        ans = num1 + num2

    elif operator == 2:#If Random Operator = 2, it will ask multiplication questions
        print("What is", num1, "*", num2)
        ans = num1 * num2

    else: #If Random Operator = 3/Else, it will ask subtraction questions.
        print("What is", num1, "-", num2)
        ans = num1 - num2

    while True:
      try:
        user_ans = int(input()) #The Answer the user inputs to the question asked
      except ValueError: #When anything but a interger is inputted.
        print ("That is not a valid answer")
        continue #Dosen't print Invalid Code Error
      else:
        break

    if user_ans == ans: #If user answer matches that of the question
        print("Correct")
        score += 1

    else: #If user answer dosen't match that of a question.
        print("Incorrect")

    question += 1 #When a question is answered, it will +1 until it equals 10 at which point it will stop.

print("Well done", name, "you scored", score, "/10") #Print Final Score out of 10

class_name = class_name + ".txt"    #adds '.txt' to the end of the file so it can be used to create a file under the name a user specifies

file = open(class_name , 'a')   #opens the file in 'append' mode so you don't delete all the information
name = (name)
file.write(str(name + " : " )) #writes the information to the file
file.write(str(score))
file.write('\n')
file.close()    #safely closes the file to save the information

viewscore = input("Do you wish to view previous results for your class").lower()

if viewscore == "yes".lower():
    f = open(class_name, 'r')
    file_contents = f.read()
    print (file_contents)
    f.close()

elif viewscore != "yes".lower():
    print ("Press any key to exit")
    ex = input ("")
    os._exit(0)

A few advices :

Python has a code style called PEP8. It is a worth reading. Also, you'll find various tools to check that your code is PEP8-compliant like pep8. You'll also find various other tools to check the quality of your code : pyflakes, pychecker, pylint, etc.
avoid useless comments. It can be tempting to comment every line you write as you start programming. However, what you are doing adds more noise that it actually helps. As a rule of thumb, "Code Tells You How, Comments Tell You Why". Also, comments are easy to get out of sync with the code (you have places where your code says "25" and the comment says "10", how confusing).
you can add an argument to input so that you don't have to call print before.
your while question < 10 loop looks like it could/should be a for loop : for question in range(10): (and no need for an initialisation nor for a incrementation in the loop). Also, as a side-note, the convention in Python is to use _ for throw-away values, thus, the loop could be written : for _ in range(10):.
you should try to use the with keyword whenever you handle files. If you do so, you don't have to call close explicitly.
you should try to ask yourself if your variable describe how you use it. For instance, class_name is a proper name for what you do at the beginning. However, it doesn't sound like a proper filename. You could simple create a new variable : filename = class_name + ".txt"
avoid useless tests : in elif viewscore != "yes".lower(), the condition will always be true. You could simply write else.
calling lower() on "yes" doesn't seem really interesting does it ?

Now, for more "complicated" advices.

You can try to split the logic in small reusable units like functions (or classes). For instance, you could write :

def get_int_input():
    while True:
      try:
        return int(input())
      except ValueError:
        print ("That is not a valid answer")

Instead of having some code to handle the different possible operations, you could have a structure containing the relevant information : the symbol to be printed and the corresponding operation. You could use a list of tuple to store everything and then, it's easy to pick one at random and use it :

import operator

OPERATIONS = [
    (operator.add, "+"),
    (operator.mul, "*"),
    (operator.sub, "-")
    ]

for _ in range(10):
    num1 = random.randint(1,25)
    num2 = random.randint(1,25)
    op, symbol = random.choice(OPERATIONS)
    print("What is", num1, symbol, num2)
    if get_int_input() == op(num1, num2):
        print("Correct")
        score += 1
    else:
        print("Incorrect")

If you ever need to add one, it's quite easy.

It is also a good habit to use a if __name__ == '__main__': guard.

Similarly to what we've done to get an integer input, we can define a function to get a boolean input.

The final code looks like :

import random
import operator

OPERATIONS = [
    (operator.add, "+"),
    (operator.mul, "*"),
    (operator.sub, "-")
    ]

NB_QUESTIONS = 10

def get_int_input(prompt=''):
    while True:
      try:
        return int(input(prompt))
      except ValueError:
        print("Not a valid input (integer is expected)")

def get_bool_input(prompt=''):
    while True:
        val = input(prompt).lower()
        if val == 'yes':
            return True
        elif val == 'no':
            return False
        else:
            print("Not a valid input (yes/no is expected)")

if __name__ == '__main__':
    name = input("What is your name?").title()
    class_name = input("Which class do you wish to input results for? ")
    print(name, ", Welcome to the OCR Controlled Assessment Maths Test")

    score = 0
    for _ in range(NB_QUESTIONS):
        num1 = random.randint(1,25)
        num2 = random.randint(1,25)
        op, symbol = random.choice(OPERATIONS)
        print("What is", num1, symbol, num2)
        if get_int_input() == op(num1, num2):
            print("Correct")
            score += 1
        else:
            print("Incorrect")

    print("Well done", name, "you scored", score, "/", NB_QUESTIONS)

    filename = class_name + ".txt"

    with open(filename, 'a') as f:
        f.write(str(name) + " : " + str(score) + '\n')

    if get_bool_input("Do you wish to view previous results for your class"):
        with open(filename, 'r') as f:
            print(f.read())
    else:
        input ("Press any key to exit")


Racing promises and consuming them in the order of their resolution time The ideas are

When multiple async calls are made, to be able to start consuming from the first resolving one regardless in what order the promises are fired.
To construct a modern emitter of async values to be consumed by the for await loop.
One may achieve this functionality by making repeated (promise many times) Promise.race()s but i want to believe that no good programmer would feel comfortable with that.
Because both it's wasteful and  i never liked Promise.race(). It's a misnomer. I mean where in the world there is a race that finalizes once somebody wins? It's an insult. It's like calling the second one "Hey.. you are at the top of all losers". Let's change that.

One implementation of this functionality could be starting to render a multipart entity earlier than otherwise. There might be other use cases too, since this will not reject altogether even if a promise in the system gets somehow rejected. We can easily handle rejections in an isolated manner, separatelly, perhaps for retrying or whatnot. Also the promises can be identified as they resolve and we can tell where to use their resolution.
The code below is just a skeleton.

It just shows the mechanism with a simple exception handling.
It doesn't include any retry or promise identification abilities.
It doesn't remove the consumed (fulfilled) promise from the structure. This can be iplemented in the finally block. Once it's implemented, the count variable will be redundant and we can then use the dynamic length property instead.
Being able to use the dynamic length property would allow us to add, remove or replace promises at any point in time just by using standard array methods like .push() or .splice() etc.
It uses the new Private Class Fields which i like a lot.

We will start with a new data type called SortedPromisesArray which in fact is an extension to the Array type. We will empower it with generator and asyncIterator abilities.
export default class SortedPromisesArray extends Array {
  #resolve;
  #reject;
  #count;
  constructor(...args){
    super(...args.filter(p => Object(p).constructor === Promise)); // Make sure everybody is a Promise
    this.#count = this.length;
    this.forEach(p => p.then(v => this.#resolve(v), e => this.#reject(e)));
  };
  async *[Symbol.asyncIterator]() {
    while(this.#count--) {
      try {
        yield new Promise((...rs) => [this.#resolve,this.#reject] = rs);
      }
      catch(e){
        console.log(`Caught an exception ${e}`);
      }
      finally{
        // a handy stage to do useful things
      };
    };
  };
};

and we can consume it like;
import SPA from "../lib/promise-sort.js";

var promise  = (val,delay,isOK) => new Promise((v,x) => setTimeout(_ => isOK ? v(val) : x(val), delay)),
    promises = [ promise("Third", 3000, true)
               , promise("First", 1000, true)
               , Promise.resolve("this is solved first @ microtask queue")
               , promise("Second", 2000, false) // NOTE: this one rejects!
               ],
    sortedPS = new SPA(...promises);

async function sink() {
  for await (let value of sortedPS){
      console.log(`Got: ${value}`);
  };
};

sink();

Any thoughts and recommendations are most welcome.

Issues
Lost resolution
You are issuing promises without a guarantee of being able to handle all or some of the resolving promises.
This will happen if promises resolve before you start the for await loop.
The offending code is in the line this.forEach(p => p.then(v => this.#resolve(v), e => this.#reject(e)));
The calls to this.#resolve and this.#reject require that the asyncIterator is run so that this.#resolve and this.#reject are assigned functions. If not they are undefined when a promise resolves.
It is also possible for a promise to overwrite the previous promise's resolved value without the iterator getting to see the value.
For example:

if you delay the call to sink(); with setTimeout(sink, 4000) none of the promises are handled within the function sink. The results are lost and can not be retrieved

If the iterator awaits content that allows more than one of the iterateable promises to resolve between iterations.
Example the following function loses the second promise. (given your test data)

async function sink() {
    const p = new Promise(v=>setTimeout(v, 1200, "zz"))
    for await (const value of sortedPS){
       log(value);
       log(await p);
    };
};

Silencing errors!
Never silence errors that are not your responsibility.
You do this in the line:
super(...args.filter(p => Object(p).constructor === Promise)); // Make sure everybody is a Promise

Either throw an error or let the error happen naturally. If your code can not function correctly with the given arguments then throw
Example of vetting and throwing error:
constructor(...promises){
    super(...promises);
    if (promises.some(promise => !(promise instanceof Promise))) {
        throw new RangeError("Array can only contain Promises");
    }

Poor inheritance
You extend Array yet neglect to handle any of the inherited functions or properties.
For example one would expect that SortedPromisesArray.push would add a promise that will resolve. As it stands added promises are ignored.
Simple example of extending inherited function:
  push(...promises) {
      if (promises.some(promise => !(promise instanceof Promise))) {
          throw new RangeError("Array can only contain Promises");
      }
      this.#count += promises.length;
      super.push(...promises);
      promises.forEach(p => p.then(v => this.#resolve(v), e => this.#reject(e)));
  }

The same would apply to any of the inherited functions that modify the arrays content.
Arrays are independent of content
However this leaves the problem of behaviors that can not be extended without the use of a proxy. Eg sortedPS[sortedPS.length] = new Promise(...)
Without a proxy you are left with an object that has inconsistent behavior.
It is therefor not advisable to extend Arrays if behavior of the extended array is dependent on the content of the array.
Update
I have added an example RaceAll as a factory that creates an async iterate-able object that races an array of promises in order of resolution.
This is achieved by marking promises with ids and using the id of resolved promises to remove then from the race array.
It does not include any vetting, nor does it resolve rejected / failed promises (both can easily be implemented).
It does guarantee no promises are lost and does not require iteration before any promises are resolved.
function RaceAll(...promises) {
    var racing = promises.map((promise, id) => Object.assign(
            new Promise(r => promise.then(result => r({id, result}))), {id}
        ));
    return Object.freeze({
            async *[Symbol.asyncIterator]() {
                while (racing.length) {
                    const resolved = await Promise.race(racing);
                    yield resolved.result;
                    racing = racing.filter(r => r.id !== resolved.id);
                }
            }
        });
}


Indexing data with Lucene.net I'm using the following function to index ebook data with Lucene.  I'm looking to improve the structure and organization of this function.

public string BuildIndex(int pageNum, int pageSize)
{
    var watch = new System.Diagnostics.Stopwatch();
    watch.Start();
    var version = Lucene.Net.Util.Version.LUCENE_30;

    Analyzer analyzer = new StandardAnalyzer(version);
    DirectoryInfo path = new DirectoryInfo("C:\\LuceneIndex");
    Lucene.Net.Store.Directory directory = new MMapDirectory(path);
    using (IndexWriter iwriter = new IndexWriter(directory, analyzer, IndexWriter.MaxFieldLength.UNLIMITED))
    {
        //Initialize the StringBuilder with 4 MB to accommodate our data
        StringBuilder bodyBuilder = new StringBuilder(4*1024*1024);
        var files = ReaderDAL.FetchBooklistSearchData(pageNum, pageSize);
        foreach (var file in files) {
            bodyBuilder.Clear();
            DirectoryInfo filePath = new DirectoryInfo(file.IndexFilePath);

            if (filePath.Exists)
            {
                foreach (var pageFile in filePath.EnumerateFiles())
                {
                    using (StreamReader reader = pageFile.OpenText()) {
                        String line = reader.ReadToEnd();
                        bodyBuilder.AppendLine(line);
                    }
                }
            }
            //file.IndexFilePath
            Document doc = new Document();
            doc.Add(new Field("id", file.BookId.ToString(), Field.Store.YES, Field.Index.NO));
            doc.Add(new Field("author", file.Author, Field.Store.YES, Field.Index.ANALYZED));
            doc.Add(new Field("annotation", file.Annotation, Field.Store.YES, Field.Index.ANALYZED));
            doc.Add(new Field("publisher", file.Publisher, Field.Store.YES, Field.Index.ANALYZED));
            doc.Add(new Field("title", file.Title, Field.Store.YES, Field.Index.ANALYZED));
            doc.Add(new Field("body", bodyBuilder.ToString(), Field.Store.YES, Field.Index.ANALYZED));
            iwriter.AddDocument(doc);
        }

        iwriter.Optimize();
        iwriter.Flush(true, true, true);
    }

    watch.Stop();

    return watch.Elapsed.TotalSeconds.ToString();
}

The function is called BuildIndex, but it could be seen as doing the following:

Measuring its execution time.
Creating an Analyzer.
Creating a Lucene.Net.Store.Directory.
Using the analyzer and the directory to create an IndexWriter.
Iterating files
Loading document body from physical file if it exists at specified location.
Creating a new Document for each file, adding 6 Fields.
Returning execution time.

The first thing that strikes me is that there seems to have a performance concern that shadows the code's intent: is the goal of the method to build an index, or to calculate how long it takes to build an index? The first thing I'd do is return void and remove the first thing in that list - focused code does one thing, it has only a single responsibility in mind.

The next 3 points are closely related and could easily fit inside a method whose job is to return a new IndexWriter:

private IndexWriter CreateWriter(Analyzer analyzer, string path)
{
    var directory = new DirectoryInfo(path);
    var mMapDirectory = new MMapDirectory(directory);

    return new IndexWriter(mMapDirectory, analyzer, IndexWriter.MaxFieldLength.UNLIMITED)
}

The reason this method is taking an Analyzer parameter is because it leaves you the liberty of calling it and passing in a StandardAnalyzer today, and a CustomAnalyzer tomorrow, without changing anything in the method, only the way the analyzer gets instantiated.

In your original code, the version, analyzer path and directory locals all only exist to create the IndexWriter. Extracting the instantiation of this writer into its own method further reduces noise and increases signal. Consider this:

public void BuildIndex(Analyzer analyzer, int pageNum, int pageSize)
{
    using(var writer = CreateWriter(analyzer, @"C:\LuceneIndex"))
    {
        //...
    }
}

Now BuildIndex doesn't care anymore about what Lucene version the analyzer is configured with; not anymore does it care about the actual type of that analyzer either.

The body of the using block declares a bodyBuilder variable that I would have simply called builder. In fact, the StreamReader, is part of implementation details of another method, just like the creation of a new document:

using(var writer = CreateWriter(analyzer, @"C:\LuceneIndex"))
{
    var files = ReaderDAL.FetchBooklistSearchData(pageNum, pageSize);
    foreach (var file in files)
    {
        //Initialize the StringBuilder with 4 MB to accommodate our data:
        var builder = new StringBuilder(4*1024*1024);

        var directory = new DirectoryInfo(file.IndexFilePath);
        if (directory.Exists)
        {
            ReadAllPageFilesInFolder(directory, builder);
        }

        writer.AddDocument(CreateDocument(file, builder.ToString()));
    }

    writer.Optimize();
    writer.Flush(true, true, true);
}

The advantage of a StreamReader is that it allows you to load a large file's content in small chunks.. but using ReadToEnd() is essentially the same as calling File.ReadAllText(), so if the files are easily digestible it would be more easily readable to go that way:

private void ReadAllPageFilesInFolder(DirectoryInfo directory, StringBuilder builder)
{
    foreach (var file in directory.EnumerateFiles())
    {
        // is this file really a PageFile?

        builder.AppendLine(File.ReadAllText(file.FullName));
    }
}

Moving the document creation into its own method makes it easier to later change how a document gets created (new field(s), new settings, etc.):

private Document CreateDocument(WhateverThatTypeIs file, string body)
{
    var result = new Document();

    result.Add(new Field("id", file.BookId.ToString(), Field.Store.YES, Field.Index.NO));
    result.Add(new Field("author", file.Author, Field.Store.YES, Field.Index.ANALYZED));
    result.Add(new Field("annotation", file.Annotation, Field.Store.YES, Field.Index.ANALYZED));
    result.Add(new Field("publisher", file.Publisher, Field.Store.YES, Field.Index.ANALYZED));
    result.Add(new Field("title", file.Title, Field.Store.YES, Field.Index.ANALYZED));
    result.Add(new Field("body", body, Field.Store.YES, Field.Index.ANALYZED));

    return result;
}

That's all nice, but now you want BuildIndex to measure its execution time and return it as a string, right? Wrong.

Let's say the BuildIndex method is located inside an IndexBuilder class; I'd make the method virtual...

public class IndexBuilder
{
    public virtual void BuildIndex(Analyzer analyzer, int pageNum, int pageSize)
    {
        // the above code
    }
}

... and then I'd create a class whose role is to decorate the existing functionality with another functionality, that I can substitute for an IndexBuilder wherever I need one:

public IndexBuilderStopWatchDecorator : IndexBuilder
{
    private readonly IndexBuilder _builder;

    public IndexBuilderStopWatchDecorator(IndexBuilder builder)
    {
        _builder = builder;
    }

    public override void BuildIndex(Analyzer analyzer, int pageNum, int pageSize)
    {
        var watch = StopWatch.StartNew();
        _builder.BuildIndex(analyzer, pageNum, pageSize);
        watch.Stop();
        // log watch.ElapsedMilliseconds,
        // write watch.Elapsed.TotalSeconds to debug output,
        // and/or do whatever the original code is doing with that value.
    }
}


Delos MVC PHP Framework I've been working in PHP since almost three years ago. In this time I built a framework as base for my projects.

I want it to be improved, and which other best option there are that use StackExchange?, so I decided to post this here.

The framework is hosted in GitHub:
https://github.com/CPedrini/Delos

Now, the problem:
I think that there is a mess in the Input class, particularly in how the base's query string is parsed, I have problems to make a definitely decision of how manage URI's parameters.

I've created a .htaccess, which rewrites the URI like this:

From this: 127.0.0.1/home/initialize/param1:value1/param2:value2

To this: 127.0.0.1/index.php?p=home/initialize/param1:value1/param2:value2

Obviously, this is not needed, it's just a fancy stuff. If mod_rewrite is disabled then all the links on the web must be changed to the rewritten form.

So, in input class constructor the query string p is parsed.

I explode the $_GET['p'] var with separation slashes and delete empty values.

Then if first value doesn't contain a : char I store it in $_GET['section'] var, it will be the control to be instantiated.

Then, if section was there, in the second exploded value, if there isn't a : char then this is the method of the control to be called. I store it in $_GET['method'].

Finally, all the others stripped vars are parsed and stripped with the : char. In the previous example, to this:

$_GET['param1'] = value1

Do you guys think that it's uniform? What changes do you suggest?

EDIT: Added code in question.

$a = explode('/', $_GET['p']);

foreach ($a as $key => $value) {
    if($value == ''){
        unset($a[$key]);
    }
}

$b = 0;

if(isset($a[0]) && !strstr($a[0],':')){
    $_GET['section'] = $this->security->sanitize($a[0]);
    $b+=1;

    if(isset($a[1]) && !strstr($a[1],':')){
         $_GET['method'] = $this->security->sanitize($a[1]);
         $b+=1;
    }
}

if(isset($a[$b])){
    for($b = $b; $b < count($a); $b ++){
        if(!strstr($a[$b],'method') && !strstr($a[$b],'section')){

            $c = explode(':',$a[$b]);
            $_GET[$this->security->sanitize($c[0])] = isset($c[1]) ? $this->security->sanitize($c[1]) : '';
        }
    }
}
unset($_GET['p']);

Instead of this block:

foreach ($a as $key => $value) {
    if($value == ''){
        unset($a[$key]);
    }
}

You could simply use array_filter() without the second parameter:

$a = array_filter($a);

And also your method system/core/Security::sanitize
(I suppose that would be $this->security->sanitize in your code above):

strip_tags does simply nothing after htmlspecialchars, because all the tags like <b> got replaced with &lt;b&gt;.
If that should secure you from cross site scripting it's definitely applied at the wrong place. You should only escape the html tags before outputting them, not directly after retrieving them, because some methods (like HTML parsers) would then parse the string wrongly.


looking for OO input , is any of this code good OO or am i doing it all wrong? Some code im working with for a stock system , i have two ways to do this, looking for review on if any of them are correct and if so which one is better OO than the other

public interface Order {
    public void processed(AllocationResponse response);
}

the two solutions i have in my head are

public class Warehouse {

    public void allocate(Order order){
        AllocationResponse response = calculateAllocation(order.getQuantity());
        order.processed(response)
    }

    private AllocationResponse calculateAllocation(int quantity){
    // code
    }

}

public class AllocationResponse {

    int quantityAllocated = 0;
    int quantityBackOrdered = 0;
}

Order order = // get order
new Warehouse().allocate(order)

or

public class Warehouse {

    public AllocationResponse allocate(int quantity){
        return calculateAllocation(quantity);
    }

    private AllocationResponse calculateAllocation(int quantity){
        // code to generate response object
    }

}

Order order = // get order
AllocationResponse response = new Warehouse().allocate(order.getQuantity());
order.processed(response)

A partial answer to the partial question (with no partiality to C#, Java, or quantum mechanics):

The invariants:

Some unnamed caller needs to effectively call:

order.processed(warehouse.calculateAllocation(order.getQuantity()));

where calculateAllocation is private (so, presumably invisible to the caller).

[So, let's imagine the int getQuantity() method in the Order interface.]

The Warehouse implementation depends on AllocationResponse.
The unnamed caller must minimally depend on the existence of Order and Warehouse and needs some way to call calculateAllocation "for an order".

Comparison of the variants:

Option 1 pros:

The unnamed caller has no further dependencies beyond the above minimum.

Option 1 cons:

Warehouse now depends on Order, Order.getQuantity and Order.processed.

Option 2 pros:

Warehouse has no further dependencies.

Option 2 cons:

calculateAllocation is now completely exposed to abusive calling (via allocate) with not so much as an Order in sight. They might as well be one public function. Yet calculateAllocation was likely defined private for good reason -- otherwise:

order.processed(new Warehouse().calculateAllocation(order.getQuantity()));

would have been Option 0.
The unnamed caller now depends on Order.getQuantity, Order.processed, and AllocationResponse.

Note: The dependency on AllocationResponse could be reduced (at least in one sense) at some cost of readability (you judge?) and unique line numbers for breakpoints (typically just where and when you need one) with:

    order.processed(new Warehouse().allocate(order.getQuantity()))

Now, those are just the apparent trade-offs, but they may well be outweighed by how these design options fit in with other present and foreseeable code.

For example,

Option 1 would be more favorable in a wider system that might:

define Warehouse-based classes that specifically refine the behavior of allocate(order),
possibly with no change or a different change to calculateAllocation(int). That is, refining allocate(order) SOLELY indirectly by refining calculateAllocation(int) would be the same deal between Options 1 and 2.
have many different callers of the exact same idiom:

warehouse.allocate(order)

require other exposure to Order, Order.getQuantity and Order.processed in the Warehouse module (helping to justify/amortize the dependency cost).

Option 2 would be more favorable in a wider system that might:

require or desire other exposure to Order.getQuantity and Order.processed, and possibly AllocationResponse in the unnamed caller (to justify/amortize the dependency cost) other than just using the idiom:

order.processed(warehouse.allocate(order.getQuantity()))

Repeated use of this idiom would argue for giving it its own method -- under Option 2, a local method of the unnamed caller, so that would be pretty much the same deal between Options 1 and 2.

In some "binary" sense, the Order interface is already imported by the unnamed caller, so the exposure to the methods is implicitly already there "for free" or more like "for better or worse". Here "binary" has BOTH its meanings of "either on or off" and "down to the compiled bits". This "binary" distinction is important when designing interfaces for libraries intended for public consumption "to be supported forever".  But in a subtler practical sense, for an interface that's only used experimentally or in a small "in house" code base, isolating function calls to a small fraction of even the modules that import the interface can make it considerably less painful to "break the contract" later by evolving those functions. In this sense, calls to each distinct Order method from the unnamed caller is a "deeper" dependency than merely importing the Order interface.

require or desire other exposure to calculateAllocation/allocate(int) outside Warehouse. That seems unlikely.  Otherwise, calculateAllocation would not have been declared private, or it would have been changed to public as a knee-jerk Option 0 solution as soon as this design problem arose, with none the wiser -- now THAT's what I'd call "doing it all wrong".


Generic extension method for NullIf() supporting nullable and non-nullable value types I would like a simple, modestly efficient NullIf() generic extension for nullable and non-nullable value types including enums. The trickiness seems to be with equality testing in generics.

Any issues with this working implementation?

/// <summary>
/// Return null if the value is equal to the argument.  Applies to value types including enums.
/// </summary>
public static T? NullIf<T>(this T value, T equalsThis)
    where T : struct, IComparable // values types (including enum)
{
    return Comparer<T>.Default.Compare(value, equalsThis) == 0
        ? (T?)null
        : value;
}

/// <summary>
/// Return null if the value is null or the value is equal to the argument.
/// Applies to value types including enums.
/// </summary>
public static T? NullIf<T>(this T? value, T equalsThis)
    where T : struct, IComparable  // values types (including enum)
{
    return !value.HasValue
        ? (T?)null
        : value.Value.NullIf(equalsThis);
}

Test cases:

int i = 32;
int? i2 = i.NullIf(32); // null
int? i3 = i.NullIf(50); // 32

System.IO.FileAccess fa = System.IO.FileAccess.Read;
System.IO.FileAccess? fa2 = fa.NullIf(System.IO.FileAccess.Read);       // null
System.IO.FileAccess? fa3 = fa.NullIf(System.IO.FileAccess.ReadWrite);  // Read

References: Comparer<T>.Default

Just a couple of observations:

Neither Comparer<T> nor IComparer<T> pose any type restrictions on T. So I don't see any need for imposing an IComparable restriction in the implementation.
The other option would be to keep the IComparable restriction and make actually use of that interface (i.e. return value.CompareTo(equalsThis) == 0)

IComparable is the wrong abstraction to use. IComparable is meant for objects to provide a sort order as it has the concept of smaller, equal and greater. For your purposes you simply want to test for equality - so you should use IEquatable<T> or EqualityComparer<T> instead.

Update:
Turns out that enums generally don't implement IEquatable<T>. The best option in that case would probably be to allow the user to optionally pass in an IEqualityComparer<T> defaulting to EqualityComparer<T>.Default and again not impose any additional type restrictions.


Namespacing patterns After finishing this article : Lessons From A Review Of JavaScript Code

I was wondering if my namespace

/**
 * The primary namespace object
 * @type {Object}
 * @alias BDDS
 */
if(!window['BDDS']) {
    window['BDDS'] = {};
}

if attached to the window, is this snippet invalid,
according to the following?

  Problem 9

  Problem: The namespacing pattern used is technically invalid.

  Feedback: While namespacing is implemented correctly across the rest
  of the application, the initial check for namespace existence is
  invalid. Here’s what you currently have: 1    if ( !MyNamespace ) { 2
  MyNamespace = { }; 3  }

  The problem is that !MyNamespace will throw a ReferenceError, because
  the MyNamespace variable was never declared. A better pattern would
  take advantage of boolean conversion with an inner variable
  declaration, as follows: 01   if ( !MyNamespace ) { 02      var MyNamespace
  = { }; 03 } 04      05    //or 06 var myNamespace = myNamespace || {}; 07   08    // Although a more efficient way of doing this is: 09   //
  myNamespace || ( myNamespace = {} ); 10   // jsPerf test:
  http://jsperf.com/conditional-assignment 11     12    //or 13 if ( typeof
  MyNamespace == ’undefined’ ) { 14   var MyNamespace = { }; 15 }

  This could, of course, be done in numerous other ways. If you’re
  interested in reading about more namespacing patterns (as well as some
  ideas on namespace extension), I recently wrote “Essential JavaScript
  Namespacing Patterns.” Juriy Zaytsev also has a pretty comprehensive
  post on namespacing patterns.

No your code is correct.

!reference only throws a reference error if its not defined.

window is defined and window["not_defined"] just returns undefined for not defined variables.


Directory Stacks with Python I implemented basic pushd and popd functionality for a Python script. What's a more academic/formal way to implement a directory stack?

_directory_stack = {0:getcwd()}

def pushd(target: str):
    """Push a new directory to the top of the stack and changes to it.

    Arguments:
        target {str} -- Where you want to go
    """
    highest_seen = max(_directory_stack.keys())
    _directory_stack[highest_seen+1] = target
    chdir(target)

def popd():
    """Pops the most recent directory off the stop of the stack and returns you to the previous directory
    """
    current_top = max(_directory_stack.keys())
    _directory_stack.pop(current_top, None)
    chdir(_directory_stack[current_top-1])

Academic/formal is not what's needed here. Simple is what's needed.

A dict whose keys are all integers from 0 to n is suspicious. Why not use a list instead? You can add items to the end with .append and remove them with .pop. Then you don't have to bother with max(_directory_stack.keys()).

This is the usual way to implement a stack in Python.

Edit: oh, wait, here's an academic/formal issue: popd doesn't restore correctly with relative paths. For example:

pushd('foo')
pushd('bar')
popd()

Should the directory now be foo or foo/bar/foo?

There are two ways to fix this:

pushd can convert the path to an absolute path with os.path.abspath.
Instead of saving the directory it's changing to, pushd can save the current directory (getcwd), and popd can just chdir to whatever it popped.

Which one to do depends on where you want to restore to if someone changes the directory without telling pushd. Consider this sequence:

pushd('/foo')
chdir('bar')
pushd('/baz')
popd()

Should the directory now be /foo or /foo/bar?


Determining the next payment renewal date I am not very good a JavaScript, surely not date manipulation, but I hacked together the script below with the purpose to determine the next payment renewal date based on a start date and payment term. The script does what it is supposed to do. The script is part of a data transformation, which runs for every row in the data transformation (Pentaho). It has 1.1 million rows, so performance is very important. Who has ideas on increasing performance (currently only 18 rows / sec on my i5).

var renewaldate = new Date(START_DATE.getTime());
var renewalday = START_DATE.getDate();
var renewalmonth = START_DATE.getMonth();
var renewalyear = START_DATE.getYear();
var paymentterm= 12 / PAYMENTS_PER_YEAR;
var today = new Date();

while (renewaldate - today < 0) {
    renewalmonth = renewalmonth + paymentterm;
    if (renewalmonth > 12) {
        renewalmonth = renewalmonth - 12;
        renewalyear++;
    }
    renewaldate.setFullYear(renewalyear, renewalmonth, renewalday);
}

An interesting question..

I am assuming that the values of PAYMENTS_PER_YEAR can only be these:

var paymentFrequencies = [ 1 , 2 , 3 , 4 , 6 , 12 ];

If that is the case, then the renewalyear can only be todays year or tomorrows year.

I would create a cache that pre-calculates for each month and each payment frequency what the next payment month is, and whether a year will be skipped.

Then you can simply use a look up to determine when the next payment month is, there should be no faster way than that.

I am currently looking into building just that, it is definitely possible, but slightly harder than expected ;)

This is what I have, I tested it a bit, the principal should be clear:

function createCache( monthsOffset ){
  var cache = {},
      paymentFrequencies = [ 1 , 2 , 3 , 4 , 6 , 12 ],
      todaysMonth = (new Date ()).getMonth() + monthsOffset,
      renewalYear, renewalMonth, targetMonth, frequency, monthsShort, nextYear, term;
  for( var month = 0 ; month < 12 ; month++ ){
    cache[month] = {};
    for( var i = 0 ; i < paymentFrequencies.length ; i++ ){
      frequency = paymentFrequencies[i];
      term = 12 / frequency;
      targetMonth = todaysMonth < month ? todaysMonth + 12 : todaysMonth;
      monthsShort = ( targetMonth - month ) % term;
      cache[month][frequency] = todaysMonth + (monthsShort ? term - monthsShort : 0);

    }
  }
  return cache;
}

var stillThisMonth = createCache( 0 ),
    nextMonth = createCache(1);

function determineRenewalDate( startDate , paymentsPerYear  ){

    var today = new Date(),
        todaysYear =  today.getFullYear();
        startMonth =  startDate.getMonth();
  /*Compare the days, if we renew say monthly, and we start on the 15th,
    but today is the 20, then we need to renew for next month, if we are the
    the 8th then you can stick to this month */
  var todaysDayOfTheMonth = today.getDate(),
      startDayOfTheMonth = startDate.getDate();
  //One more cool thing, `setFullYear` and `new Date` will roll over the year if you set the month too high
  if( todaysDayOfTheMonth > startDayOfTheMonth  ){
     return new Date( todaysYear, nextMonth[startMonth][paymentsPerYear], startDayOfTheMonth );
  } else {
     return new Date( todaysYear, stillThisMonth[startMonth][paymentsPerYear], startDayOfTheMonth );
  }
}

document.write(  determineRenewalDate( new Date(2010 , 1 , 1 ) , 12 ) + '
' );
document.write(  determineRenewalDate( new Date(2010 , 1 , 2 ) , 2 ) + '
' );


Return from the parent process code or common code In this program, the parent process creates a child process, then the child executes ls -l. The parent process waits for the child process to complete and then returns using return 0;.

/*child's output redirected to file*/
/*sps, 6262015*/

/*Headers*/
#include<stdio.h>
#include<stdlib.h>
#include<unistd.h>
#include<sys/wait.h>
#include<sys/types.h>

/*Start*/
int main(void)
{
    //Create a child process
    int ret = fork();
    if(ret<0)
            exit(1);
    /*Child code*/
    else if(ret==0)
    {
            char *argv[] = {"ls", "-l", NULL};

            //Execute 'ls -l'
            execvp(argv[0], argv);
    }
    /*Parent code*/
    else if(ret>0)
    {
            int status;
            int w = waitpid(ret, &status, WUNTRACED | WCONTINUED);
    }
    /*Where to place this ???*/
    return 0;
}

Although above code works correctly, I am not sure where to place the return 0; statement. For instance, in the above code it is at the end so it is common to both the parent and child processes.

But in this program, the child process calls execvp, so it will never reach the last line of the code which has return 0;. So, taking this into account, can I place the return 0; inside the parent code as below?

 /*Parent code*/
 else if(ret>0)
 {
         int status;
         int w = waitpid(ret, &status, WUNTRACED | WCONTINUED);
         return 0;
 }

Is there any advantage or disadvantage to putting the return 0; statement inside the parent process code?

As for all these functions:  Take a look at the man page.  It states
that execvp (respectively execve) will return on error with return
value -1 and with errno set.

Since you are already handling such errors with fork you might as
well do it here and either exit(1), or fall through to the end.


Topologial Sort in Python I am trying to self study algorithms and this is my attempt at topological sort using Tarjan's version of DFS. It runs correctly for the graph I included. Can someone tell me if this is correct and if/what optimizations I can make?

graph = {
'A':['C'],
'B': ['C', 'D'],
'C': ['E'],
'D': ['F'],
'E': ['F'],
'F': ['G'],
'G': [],
}

def topological_sort(graph):
    visited, stack = set(), []

    for vertex in graph:
        handle_vertex(graph, vertex, visited, stack)

    print(stack)

def handle_vertex(graph, vertex, visited, stack):
    if vertex not in visited:
        visited.add(vertex)

    for neighbors in graph[vertex]:
        if neighbors not in visited:
            handle_vertex(graph, neighbors, visited, stack)

    if vertex not in stack:
       stack.insert(0, vertex)
    return stack

print(topological_sort(graph))

I don't get the point of returning the stack from the handle_vertex function. The return value is never used (and the stack which is passed as an argument is modified in place).
The topological_sort functions prints the stack, doesn't return anything and then it's returned value is printed: print(topological_sort(graph)). There're two reasonable options here:

not printing the returned value of the topological_sort and just calling it instead
Returning the stack from it instead of printing it inside the function.

The neighbors name here: for neighbors in graph[vertex] is misleading. This variable represent just one neighbor at a time. I don't see why would you make it plural.


Find missing number from list I was asked a question in an interview the other day:

  You have an array list of numbers from 1 to 100, the problem is that
  one number is missing. How would you find that number?

This is a mock of the question. The code seems to work.

Could you give me some feedback?

private int count;

public void find() {
    //prep for question
    List<Integer> ints = new ArrayList();
    for (int i = 0; i < 100; i++) {
        ints.add(i);
    }
    ints.remove(67);

    //find the missing number
    for (Integer i : ints) {
        if (i != count) {
            System.out.println(count);
            count++;
        }
        count++;
    }
}

Output = 67

This is a very common interview question. However, your algorithm won't suffice: the array may be unsorted.

The method is to find the sum of the numbers in the array and then subtract it from the sum of numbers from 1 through 100. What's left over is what is missing from a complete list 1..100.

Sum of natural numbers \$1..N\ = \dfrac{N(N+1)}{2}\$.


Rational class supporting precise numbers This is my JavaScript Rational class, similar to other languages' BigFraction class, but with one major difference:

This class doesn't support arbitrarily large rational numbers:

i.e. new Rational(Number.MAX_VALUE + 1) may fail.
However, it supports arbitrarily precise numbers
i.e. new Rational(1, 10).add(new Rational(2, 10)) will be exactly new Rational(3, 10) instead of .3000000000004.

window.Rational = (function() {
  var GCF, LCM;

  // Internal GCF and LCM

  GCF = function(a, b) {
    var temp;
    if (a < 0) a *= -1;
    if (b < 0) b *= -1;
    if (a < b) {
      temp = a;
      a = b;
      b = temp;
    }
    return a % b === 0 ? b : GCF(a % b, b);
  };

  LCM = function(a, b) {
    return Math.abs(a * b) / GCF(a, b);
  };

  function Rational(top, bottom) {
    this.top = top;
    // bottom defaults to 1
    this.bottom = bottom != null ? bottom : 1;
    this.simplify();
  }

  Rational.prototype.add = function(other) {
    var lcm = LCM(this.bottom, other.bottom);
    // If we multiply by lcm
    // it becomes an integer
    // but we round to prevent floating point innacuracies
    var top1 = Math.round(this * lcm);
    var top2 = Math.round(other * lcm);
    return new Rational(top1 + top2, lcm);
  };

  Rational.prototype.subtract = function(other) {
    return this.add(other.multiply(Rational.constants.NEGATIVE_ONE));
  };

  Rational.prototype.multiply = function(other) {
    return new Rational(this.top * other.top, this.bottom * other.bottom);
  };

  Rational.prototype.divide = function(other) {
    return this.multiply(other.reciprocal());
  };

  Rational.prototype.reciprocal = function() {
    return new Rational(this.bottom, this.top);
  };

  Rational.prototype.isLessThan = function(other) {
    return this < other;
  };

  Rational.prototype.isGreaterThan = function(other) {
    return this > other;
  };

  Rational.prototype.isEqualTo = function(other) {
    return this.top === other.top && this.bottom === other.bottom;
  };

  Rational.prototype.valueOf = function() {
    return this.top / this.bottom;
  };

  Rational.prototype.simplify = function() {
    var gcf;
    if (this.top === 0) {
      // No need to calculate GCF because GCF = 0
      this.bottom = 1;
      return;
    }
    if (this.bottom === 0) {
      throw new Error("Cannot have denominator 0");
    }
    gcf = GCF(this.top, this.bottom);
    if (this.top < 0 && this.bottom < 0) {
      // Since GCF doesn't handle negatives
      // if both numerator and denominators are negative
      // we can cancel the negative
      gcf *= -1;
    }
    // If the bottom is less than zero, then move the negative sign to the top
    if (this.bottom < 0 && this.top >= 0) {
      this.top *= -1;
      this.bottom *= -1;
    }
    this.top /= gcf;
    this.bottom /= gcf;
    return this;
  };

  Rational.constants = {
    ZERO: new Rational(0),
    ONE: new Rational(1),
    NEGATIVE_ONE: new Rational(-1)
  };
  return Rational;

})();

Questions:

I think that by doing this * 5 or other * 6, I implicitly call valueOf(). Is that okay? Should I make it explicit (to me, saying this * 5 is clear: multiply this by 5), but I'm not sure in general.
Same as the question above for this < other and this > other.
Can I refactor my GCF and LCM algorithms to take into account negative numbers so that I don't have to handle them as special cases in the simplify function? Right now, they simply make everything positive and return a positive number.

Not a complete answer, but I noticed some bugs:

LCM = function(a, b) {
  return Math.abs(a * b) / GCF(a, b);
};

The computation of a * b will easily overflow (thus losing precision) for large numbers a, b. You can improve matters by rewriting it as

function LCM(a, b) {
  var g = a / GCF(a,b);
  return Math.abs(g * b);
}

I also used function LCM as opposed to LCM = function just as a stylistic preference. See here.

Similarly:

Rational.prototype.add = function(other) {
  var lcm = LCM(this.bottom, other.bottom);
  // If we multiply by lcm
  // it becomes an integer
  // but we round to prevent floating point innacuracies
  var top1 = Math.round(this * lcm);
  var top2 = Math.round(other * lcm);
  return new Rational(top1 + top2, lcm);
};

If I understand correctly, in the expression this * lcm, lcm is a Number, so you're basically doing this.valueOf() * lcm, which again wipes out all your pretended precision. You should avoid big numbers (which become floating-point), and you should definitely avoid unnecessary floating-point divisions.

Rational.prototype.add = function(other) {
  var lcm = LCM(this.bottom, other.bottom);
  var top1 = this.top * (lcm / this.bottom);
  var top2 = other.top * (lcm / other.bottom);

  return new Rational(top1 + top2, lcm);
};


Chess programming algorithm minimax to Alpha Beta I have written some code for alpha beta for Chess programming and not sure completely if that is the correct way to do it. Can someone please tell me if I implemented the alpha_beta properly?

public class Minimax{
    private boolean side;
    public static int depth;

    public Minimax(boolean inputSide, int inputDepth) {
        side = inputSide;
        depth = inputDepth;
    }

    public int[] getMoves(Board board, boolean white, int inputDepth, boolean check) {
        ArrayList<int[]> values = new ArrayList<int[]>();
        int initScore = board.getScore();
        boolean multipleMoves = false;
        ArrayList<int[]> multiple = new ArrayList<int[]>();
        if (white) {
            values.add(new int[]{-2000, 0, 0, 0, 0});
        }
        else {
            values.add(new int[]{2000, 0, 0, 0, 0});
        }
        int position = 0;
        if (inputDepth == depth) {
            int[] ret = {board.getScore(), 0, 0, 0, 0};
            return ret;
        }
        for (Pieces[] subPiece : board.getArray()) {
            for (Pieces piece : subPiece) {
                if (piece != null && piece.white() == white) {
                    int[][] moves = piece.getMoves(board, false);
                    if (moves != null) {
                        for (int[] move : moves) {
                            Board newBoard = board.newBoard(move[0], move[1], move[2], move[3]);
                            if (newBoard != null) {
                                int score = getMoves(newBoard, !white, inputDepth + 1, newBoard.check(white))[0];
                                values.add(new int[]{score, move[0], move[1], move[2], move[3]});
                                if ((white && score > values.get(position)[0]) || (!white && score < values.get(position)[0])) {
                                    position = values.size() - 1;
                                    multipleMoves = false;
                                    multiple.clear();
                                }
                                else if (score == values.get(position)[0]) {
                                    multipleMoves = true;
                                    multiple.add(values.get(values.size() - 1));
                                    multiple.add(values.get(position));
                                }
                            }
                        }
                    }
                }
            }
        }
        if (values.size() == 1) {
            if (depth == 0) {
                return null;
            }
            else {
                if (white) {
                    return new int[]{-3000, 0, 0, 0, 0};
                }
                else {
                    return new int[]{3000, 0, 0, 0, 0};
                }
            }
        }
        if (multipleMoves && multiple != null) {
            position = (int)((multiple.size() - 1) * Math.random() + 1);
            return multiple.get(position);
        }

        return values.get(position);
    }

    public int getDepth() {
            System.out.println(depth);
            return depth;

    }
}

A couple of things on top of what @tim already covered.

Use Random.nextInt instead of Math.random

Instead of this:

position = (int)((multiple.size() - 1) * Math.random() + 1);

you can write much simpler like this:

position = 1 + random.nextInt(multiple.size() - 1);

where random is an instance of Random, created for example in the constructor. Simpler isn't it?

Use interface types instead of implementations

Instead of:

    ArrayList<int[]> values = new ArrayList<int[]>();

This would be better:

    List<int[]> values = new ArrayList<int[]>();

(but not quite good enough, see the next item)

Define Abstract Data Types for your problem domain

An Abstract Data Type (ADT) is essentially a collection of data and  operations on that data.
In object oriented languages an ADT is usually modeled with a class,
where data is represented by member fields and the operations are represented by methods.

When I look at that ArrayList<int[]> in the previous point,
I've no idea what it stands for,
what kind of logical object it represents,
and what the numbers in new int[]{-2000, 0, 0, 0, 0} and the other array initializations scattered in the code mean.

If you have defined an ADT for this,
the code would become all the more readable.
Instead of new ArrayList<int[]>,
there might be new MinimaxValueSet().
Instead of values.add(new int[]{-2000, 0, 0, 0, 0}),
there might be valueSet.addNewSetWithScore(-2000),
where this helper method would fill in the repetitive zeros.

Don't print to the console

I suppose the println here was added for debugging:

public int getDepth() {
        System.out.println(depth);
        return depth;

It doesn't belong there and should be removed.
(I suggest to remove debugging code before pasting for code review,
it's just noise for reviewers.)


Cosine similarity of one vector with many I'm keen to hear ideas for optimising R code to compute the cosine similarity of a vector x (with length l) with n other vectors (stored in any structure such as a matrix m with n rows and l columns).

Values for n will typically be much larger than values for l.

I'm currently using this custom Rcpp function to compute the similarity of a vector x to each row of a matrix m:

library(Rcpp)
cppFunction('NumericVector cosine_x_to_m(NumericVector x, NumericMatrix m) {
  int nrows = m.nrow();
  NumericVector out(nrows);
  for (int i = 0; i < nrows; i++) {
    NumericVector y = m(i, _);
    out[i] = sum(x * y) / sqrt(sum(pow(x, 2.0)) * sum(pow(y, 2.0)));
  }
  return out;
}')

Varying n and l, I'm getting the following sorts of timings:

Reproducible code below.

# Function to simulate data
sim_data <- function(l, n) {
  # Feature vector to be used for computing similarity
  x <- runif(l)

  # Matrix of feature vectors (1 per row) to compare against x
  m <- matrix(runif(n * l), nrow = n)

  list(x = x, m = m)
}

# Rcpp function to compute similarity of x to each row of m
library(Rcpp)
cppFunction('NumericVector cosine_x_to_m(NumericVector x, NumericMatrix m) {
  int nrows = m.nrow();
  NumericVector out(nrows);
  for (int i = 0; i < nrows; i++) {
    NumericVector y = m(i, _);
    out[i] = sum(x * y) / sqrt(sum(pow(x, 2.0)) * sum(pow(y, 2.0)));
  }
  return out;
}')

# Timer function
library(microbenchmark)
timer <- function(l, n) {
  dat <- sim_data(l, n)
  microbenchmark(cosine_x_to_m(dat$x, dat$m))
}

# Results for grid of l and n
library(tidyverse)
results <- cross_d(list(l = seq(200, 1000, by = 200), n = seq(500, 4000, by = 500))) %>%
  mutate(timings = map2(l, n, timer))

# Plot results
results_plot <- results %>%
  unnest(timings) %>%
  mutate(time = time / 1000000) %>%  # Convert time to seconds
  group_by(l, n) %>%
  summarise(mean = mean(time), ci = 1.96 * sd(time) / sqrt(n()))

pd <- position_dodge(width = 20)

results_plot %>%
  ggplot(aes(n, mean, group= l)) +
  geom_line(aes(color = factor(l)), position = pd, size = 2) +
  geom_errorbar(aes(ymin = mean - ci, ymax = mean + ci), position = pd, width = 100) +
  geom_point(position = pd, size = 2) +
  scale_color_brewer(palette = "Blues") +
  theme_minimal() +
  labs(x = "n", y = "Seconds", color = "l") +
  ggtitle("Algorithm Runtime",
          subtitle = "Error bars represent 95% confidence intervals")

I'm using Microsoft R (with Intel MKL) which makes matrix multiplications faster, but for fair comparison I set it to be single threaded.

setMKLthreads(1)

In my tests this pure R version cosine_x_to_m is twice faster than yours.

cosine_x_to_m2 = function(x,m){
  x = x / sqrt(crossprod(x));
  return(  as.vector((m %*% x) / sqrt(rowSums(m^2))) );
}

Rewriting rowSums(m^2) in C/C++ makes it even faster, about four times faster than the original.

library(ramwas)
cosine_x_to_m2 = function(x,m){
  x = x / sqrt(crossprod(x));
  return(  as.vector((m %*% x) / sqrt(rowSumsSq(m))) );
}

Initial performance:

Final version performance:


Balancing the server load I've created a program that will take, a bunch of numbers from a range of 1 - 255, and then put them into 3 different strings(servers) equally. It does this by having a count that starts at 1 and loops to 3, once the count has hit 3 it restarts until three servers have hit a max capacity of 100 numbers within it.

Once that max capacity has been hit, the numbers dump the remaining into a server called failsafe.

I'm looking for better ways to write this, that are cleaner, and will make this, if possible, more readable.

#!/local/usr/bin/ruby

users = (1..255) #<= Amount of users

calculon = " " #<= Random servers to be append into
darth = " "
invader_zim = " "
failsafe = " " #<= Failsafe

count = 1
users.each do |i|
  target_string = case count
   when 1 then calculon #<= Append into server
   when 2 then darth #<= Append into server
   when 3 then invader_zim #<= Append into server
  end

  target_string = failsafe if target_string.length == 100
  target_string << i #<= When amount is reached hits fail safe

  if count == 3
    count = 1
  else
    count += 1
  end
end

puts calculon.length == 1    ? 'No users on server' : calculon.length
puts darth.length == 1       ? 'No users on server' : darth.length
puts invader_zim.length == 1 ? 'No users on server' : invader_zim.length
puts failsafe.length == 1    ? 'No users on server' : failsafe.length

Example of usage:

86
86
86
No users on server

The main objective of this program is to have a script that will equally load three different servers, and if all servers have hit a max capacity, will load into a fail safe server.

This is perfect use case for group_by, which basically does all the work for you in a single line:

We also get the added benefit of having the user ids as arrays instead of strings.  This makes more sense, and if for some reason you do want to get the string representation back, you can simply call join(' ') on the array of user ids.

Finally, if you add more servers to your cluster, you just need to edit the single line defining the server names at the top of the file, and everything will still work.

users = (1..255) #<= Amount of users
servers = %w(calculon darth invader_zim)
server_capacity = 100

assigned_users = users.group_by.with_index do |u, i|
  all_servers_full = (i >= servers.size * server_capacity)
  all_servers_full ? 'failsafe' : servers[i % servers.size]
end

assigned_users.each do |server, users|
  puts "#{server} server has #{users.size} users"
end