Boolean String Normalizer How can the normalize() method below be improved? I've extracted the functionality from a project I'm working on into a simple class for the purposes of this question.

The method takes a string input and compares it to a set of 'truthy' and 'falsey' strings, returning true or false respectively if a case insensitive match is found. The truthy and falsey strings are mutable so cannot be hard coded into the method.

If the $nullable property is set to true and no match is found, null is returned. Otherwise false is returned.

I feel as though I may be missing a trick by looping through the array and calling strcasecmp twice for each iteration.

<?php

class BooleanNormalizer
{
    /** @var array */
    public $binaries = [
        'yes'     => 'no',
        '1'       => '0',
        'on'      => 'off',
        'enabled' => 'disabled',
    ];

    /** @var bool */
    public $nullable = false;

    /**
     * @param string $value
     *
     * @return bool|null
     */
    public function normalize($value)
    {
        foreach ($this->binaries as $truthy => $falsey) {
            if (strcasecmp($value, $truthy) === 0) {
                return true;
            }

            if (strcasecmp($value, $falsey) === 0) {
                return false;
            }
        }

        return ($this->nullable) ? null : false;
    }
}

Usage:

$normalizer = new BooleanNormalizer();

$normalizer->nullable = true;
$normalizer->binaries = ['enabled' => 'disabled'];

$normalizer->normalize('enabled'); // returns true
$normalizer->normalize('disabled'); // returns false
$normalizer->normalize('blah'); // returns null

I guess foremost, I question the mutability of this class. Should one really be able to change the value of $binaries?

I very much question the optional "nullable" thing here.  First of all, name-wise this doesn't seem to make sense as typically when you think of something as "nullable" it means it is a variable, property, field, etc. that is allowed to be set to null.  You are not setting anything here, so the terminology seems weird.  Perhaps it should be called $returnFalseOnMissingValue or something that more directly ties it to the return behavior of the normalize() method.

I would however get rid of it, as it will become a source of fragility to calling code, as in some cases (including default case) your calling code would not be able to distinguish a false "hit" result from a false "miss" result.  Let the calling code deal with converting null to false if it needs to send false up the call stack.  Don't obfuscate the result right here in this class which is designed to own this determination.

Why use strcasecmp()?  This seems an odd choice vs. just casting $value to lowercase for direct comparison.
 i.e.

$value = strtolower($value);
if ($value === $truthy) return true;
if ($value === $falsey) return false;
return null;

I also don't understand the lookup mechanism and comparison mechanisms here.  It seems obscure upon first read to have key/value positioning determine how the string is mapped to true/false. I think I would be more explicit like:

protected $mappings = [
    'yes' => true,
    'no' => false,
    '1' => true,
    '0' => false,
    'on' => true,
    'off' => false;
    'enabled' => true,
    'disabled' => false
];

And have your comparison method simply do something like:

public function normalize($value) {
    $value = strtolower($value);
    if(!array_key_exists($value, $this->mappings) return null;
    return $this->mappings[$value];
}

If you truly need to keep the concept of true/value pairs for some reason not shown in this code, then perhaps a structure like:

protected $mappings = [
    'yes' => ['boolean' => true, 'inverse' => 'false'],
    'no' => ['boolean' => false, 'inverse' => 'true'],
    ...
];

This prevents you from having to iterate through the $binaries values every time you want to do a lookup like you are currently doing.  The name $binaries also seems odd here as that typically has a much different meaning when talking about writing code.

I would suggest that 'true' and 'false' should be strings in your default mapping of boolean values.

Does this class really need a concrete instantiation?

Consider adding some parameter validation anytime you have public methods.  Ideally this could be a combination of type hinting and/or variable inspection to make sure you are being passed a valid parameter.  What happens now you if your method is passed an integer, array, object, empty string, etc.?


PHP Response Wrapper I'm writing a routing system that may or may not be part of a public API later for a personal project. A main part of the routing system is a response object for the user to send headers, status code, and the body of the response that is sent from the server.

I've created a response class that I think covers everything the user would want to do with the response (it could use a few more "convenience" functions however). I wanted to post it here to make sure my code is clean, and that I didn't miss anything that the end user might want to do.

An instance of the request class is provided to the user when a request is made to a specified URL (see my last code review)

I present my response class:

    class Response {

    private $headers = array();
    private $code = 200;
    private $body = '';
    private $sent = false;

    private $log;

    public function __construct() {
        $this->log = Logger::getLogger(get_class($this));
    }

    public function headers ($headers) { // Add some headers to the headers array
        $this->headers = array_merge($this->headers, $headers);
        return $this;
    }

    public function header ($key, $value) { // set a single header in the headers array
        $this->headers[$key] = $value;
        return $this;
    }

    public function code ($code) { // Set the status code
        $this->code = $code;
        return $this;
    }

    public function status ($code) { // Alternate method for setting the status code
        return $this->code($code);;
    }

    public function json($str) { // respond with json, set the body text and set the content-type header
        $this->header('Content-Type', 'application/json');
        if(is_array($str)) { // handle either raw JSON text, or php arrays
            $this->body = json_encode($str);
        } else {
            $this->body = $str;
        }
        return $this;
    }

    public function html ($str){ // respond with HTML
        $this->header('Content-Type', 'text/html');
        $this->body = $str;
        return $this;
    }

    public function form ($str) { // Respond with form data
        $this->header('Content-Type', 'application/x-www-form-urlencoded');
        // TODO: Allow the user to user an array
        $this->body = $str;
        return $this;
    }

    public function render ($file){ // Render an HTML file from the templates folder
        //TODO: Restrict to templates folder open
        //TODO: Add server-side rendering code
        $this->body = file_get_contents($file);
        return $this;
    }

    public function sent() { // Check if the request has been sent
        return $this->sent;
    }

    public function send () { // send the request
        if($this->sent()){
            $log->error('Attempted to call send on a sent response!');
            exit('Attempted to call send on a sent response!');
        }
        // Log the request for debugging
        $this->log->info($this->headers);
        $this->log->info('HTTP Responce Code: ' . $this->code);
        $this->log->info($this->body);
        // Set the headers
        foreach($this->headers as $key => $value) {
            header($key .': ' . $value);
        }
        // Set the status code
        http_response_code($this->code);

        // Send out the body
        echo $this->body;

        // Set the sent variable
        $this->sent = true;
    }
}

EDIT:
The idea with the short and clear function names that all return $this is to be able to have clear and simple chainable responses. (e.g $res->code(200)->json(... Json...);)

I this this class is not a Response. But a bundle of things.

class Response
{

Name its clear. But if it is clear, I suggest you to make this class final.

    private $headers = array();
    private $code = 200;
    private $body = '';
    private $sent = false;

By default a non sent response is 200 and without headers?

    private $log;

Here my first big confusion. Why a response should log something? A Response is just a response.

    public function __construct() {
        $this->log = Logger::getLogger(get_class($this));
    }

Alt ^_^. I really suggest you to inject a logger. If you like standards, I suggest to inject Psr\Log\LoggerInterface

    public function headers ($headers) { // Add some headers to the headers array
        $this->headers = array_merge($this->headers, $headers);
        return $this;
    }

I suggest 'set' prefix. From the api I do not understand if the method is a setter or a getter.

    public function header ($key, $value) { // set a single header in the headers array
        $this->headers[$key] = $value;
        return $this;
    }

As above: if this is a setter, ... I suggest 'set' prefix.

    public function code ($code) { // Set the status code
        $this->code = $code;
        return $this;
    }

Instead of comment "set the status code" change the method in `public function setStatusCode()".

    public function status ($code) { // Alternate method for setting the status code
        return $this->code($code);;
    }

Why?

    public function json($str) { // respond with json, set the body text and set the content-type header
        $this->header('Content-Type', 'application/json');
        if(is_array($str)) { // handle either raw JSON text, or php arrays

Why dont you use type hinting and create a method that accept array and one that accept text?

            $this->body = json_encode($str);
        } else {
            $this->body = $str;
        }
        return $this;
    }

    public function html ($str){ // respond with HTML

A method called toHtml is more appropriate here.

        $this->header('Content-Type', 'text/html');
        $this->body = $str;
        return $this;

Fluent interface is ok just for builders. This class should be rename in ResponseBuilder.

    }

    public function form ($str) { // Respond with form data

Why you comment your code instead of make it more readable?

        $this->header('Content-Type', 'application/x-www-form-urlencoded');
        // TODO: Allow the user to user an array
        $this->body = $str;
        return $this;
    }

    public function render ($file){ // Render an HTML file from the templates folder
        //TODO: Restrict to templates folder open
        //TODO: Add server-side rendering code
        $this->body = file_get_contents($file);
        return $this;
    }

    public function sent() { // Check if the request has been sent
        return $this->sent;
    }

This method return a boolan. Should be named to isSent. And also should have a return type hint like public function isSent() : bool.

    public function send () { // send the request

Comment is useless. $request->send() speaks very well.

        if($this->sent()){
            $log->error('Attempted to call send on a sent response!');
            exit('Attempted to call send on a sent response!');

I prefer Exceptions every time something unexpected happens.

        }

        // Log the request for debugging
        $this->log->info($this->headers);
        $this->log->info('HTTP Responce Code: ' . $this->code);
        $this->log->info($this->body);

Anytime you use a comment to group some lines of code, you should remove the comment and make a private method like $this->logRequestForDebugging().

        // Set the headers
        foreach($this->headers as $key => $value) {
            header($key .': ' . $value);
        }

$this->setHeaders()

        // Set the status code
        http_response_code($this->code);

$this->setResponseStatusCode()

        // Send out the body
        echo $this->body;

...

        // Set the sent variable
        $this->sent = true;
    }
}


Adding an item to a hierarchical data structure What I need to do is create a new object, modify it using a recursive function, then merge it with another object, and do this all in such a way that the values of the new object take precedence over the values in the object I'm merging it with.

Currently, my code works, but I'd like to have more readable code, particularly this statement for the add operation:

@set.merge!(
    update(@set, insert(parent, id), @set[parent][1] + 1)
)

I've already refactored quite a bit, but I feel like there's a functional way, or at least more readable way, to write what I current have. I want it to be something like: new object → update it → merge it.

Here is the whole class with tests:

# Nested set for comments, for making it easy to load all comments in a list, sorted according to their nesting,
# and for nesting to be indicated.
class Database
    def initialize
        @set = {}
        @largest = -1
    end

    def fetch
        #Need Better syntax for this.
        @set.to_a.sort { |el1, el2| el1[1][0] <=> el2[1][0] }
    end

    def add(parent: nil, id: -1)
        if parent

            @set.merge!(
                update(@set, insert(parent, id), @set[parent][1] + 1)
            )
        else
            @set[id] = [@largest+1, @largest+2]
        end
        @largest += 2
        @set
    end

    private

    def insert(parent_id, id)
        parent_range = @set[parent_id]
        {
            parent_id => [parent_range[0], parent_range[1] + 2],
            id => [parent_range[1], parent_range[1] + 1]
        }
    end

    # O(n) * 2^n
    def update(old_state, state, target)
        # optimization for this:
        id,range = old_state.to_a.select{ |entry| entry[1][0] == target || entry[1][1] == target }[0]
        # better readability for this:
        if range && !state[id]
            state[id] =
                range.index(target) == 0 ? range.map{|n| n + 2} : [range[0], range[1] + 2]
            2.times do |i|
                update(old_state, state, range[i] + 1) if range[i] != state[id][i]
            end
        end
        state
    end
end

d = Database.new
d.add({id: 1, parent: nil })
d.add({id: 2, parent: nil })
d.add({id: 3, parent: nil })

p d.fetch == [
            [1, [0, 1]],
            [2, [2,3]],
            [3, [4,5]],
            ]

d.add({id: 4, parent: 1 })
d.add({id: 5, parent: 2 })
d.add({id: 6, parent: 3 })

p d.fetch == [
            [1, [0, 3]],
            [4, [1, 2]],
            [2, [4,7]],
            [5, [5,6]],
            [3, [8,11]],
            [6, [9,10]],
            ]

d.add({id: 7, parent: 1 })
d.add({id: 8, parent: 7 })

# Final output.
# 1
#     4
#     7
#         8
# 2
#     5
# 3
#     6

p d.fetch == [
            [1, [0, 7]],
            [4, [1, 2]],
            [7, [3, 6]],
            [8, [4, 5]],
            [2, [8,11]],
            [5, [9,10]],
            [3, [12,15]],
            [6, [13,14]],
            ]

UPDATE: Will probably just do this:

            insert(parent, id)
                .yield_self { |obj| update(@set, obj, @set[parent][1] + 1) }
                .yield_self { |updated_set| @set.merge!(updated_set) }

Rubocop Report

Rubocop was able to correct a lot of style and layout offenses. 1 file inspected, 59 offenses detected, 53 offenses corrected.

The following offenses remain:

Metrics/LineLength: Line is too long. [112/80]
Metrics/AbcSize: Assignment Branch Condition size for update is too high. [24.1/15]
Style/MultipleComparison: Avoid comparing a variable with multiple items in a conditional, use Array#include? instead.
Metrics/LineLength: Line is too long. [99/80]
Style/NumericPredicate: Use range.index(target).zero? instead of range.index(target) == 0.
Metrics/LineLength: Line is too long. [85/80]

Fixing Offenses

LineLength and MultipleComparison offense:

# optimization for this:
id,range = old_state.to_a.select{ |entry| entry[1][0] == target || entry[1][1] == target }[0]

id, range = old_state.to_a.select do |entry|
  [entry[1][0], entry[1][1]].include?(target)[0]
end

LineLength + NumericPredicate offense:

state[id] =
   range.index(target) == 0 ? range.map{|n| n + 2} : [range[0], range[1] + 2]

  state[id] =
    if range.index(target).zero?
      range.map { |n| n + 2 }
    else
      [range[0], range[1] + 2]

After refactoring, we get a new issue that our method got too big. The complexity is still too high.

Metrics/AbcSize: Assignment Branch Condition size for update is too high. [25.06/15]
Metrics/MethodLength: Method has too many lines. [15/10]

We are adviced to break up this method. It's doing a bit too much. So we'll put the next part in another method.

   state[id] =
      if range.index(target).zero?
        range.map { |n| n + 2 }
      else
        [range[0], range[1] + 2]
      end
    2.times do |i|
      update(old_state, state, range[i] + 1) if range[i] != state[id][i]
    end

  def update_range(old_state, state, target, id, range)
    state[id] =
      if range.index(target).zero?
        range.map { |n| n + 2 }
      else
        [range[0], range[1] + 2]
      end
    2.times do |i|
      update(old_state, state, range[i] + 1) if range[i] != state[id][i]
    end
  end

There are no offenses remaining and complexity is within bounds.

Refactored Code

# frozen_string_literal: true

# Nested set for comments, for making it easy to load all comments in a
# list, sorted according to their nesting, and for nesting to be indicated.
class Database
  def initialize
    @set = {}
    @largest = -1
  end

  def fetch
    @set.to_a.sort { |el1, el2| el1[1][0] <=> el2[1][0] }
  end

  def add(parent: nil, id: -1)
    if parent

      @set.merge!(
        update(@set, insert(parent, id), @set[parent][1] + 1)
      )
    else
      @set[id] = [@largest + 1, @largest + 2]
    end
    @largest += 2
    @set
  end

   private

  def insert(parent_id, id)
    parent_range = @set[parent_id]
    {
      parent_id => [parent_range[0], parent_range[1] + 2],
      id => [parent_range[1], parent_range[1] + 1]
    }
  end

  # O(n) * 2^n
  def update(old_state, state, target)
    id, range = old_state.to_a.select do |entry|
      [entry[1][0], entry[1][1]].include?(target)[0]
    end
    update_range(old_state, state, target, id, range) if range && !state[id]
    state
  end

  def update_range(old_state, state, target, id, range)
    state[id] =
      if range.index(target).zero?
        range.map { |n| n + 2 }
      else
        [range[0], range[1] + 2]
      end
    2.times do |i|
      update(old_state, state, range[i] + 1) if range[i] != state[id][i]
    end
  end
 end


Bubble Sort and Selection Sort (C) I used the "long" form of all math statements, avoided the use of pointers, and used only while loops for aesthetic purposes (simply put, I think the code is prettier when it is written like that.) I am mainly concerned about the CPU efficiency of the code.

Bubble Sort - The quintessential example of an "inefficient" sorting algorithm. Often the first sorting algorithm introduced to college students.

void BubbleSort(int Array[], int Length)
{
    int CurrentIndex[2];
    int TemporaryValue;
    int WasThereASwap;

    WasThereASwap = 1;
    while (WasThereASwap == 1)
    {
        WasThereASwap = 0;
        CurrentIndex[0] = 0;
        CurrentIndex[1] = 1;
        while (CurrentIndex[1] != Length)
        {
            if (Array[CurrentIndex[0]] > Array[CurrentIndex[1]])
            {
                WasThereASwap = 1;
                TemporaryValue = Array[CurrentIndex[0]];
                Array[CurrentIndex[0]] = Array[CurrentIndex[1]];
                Array[CurrentIndex[1]] = TemporaryValue;
            }

            CurrentIndex[0] = CurrentIndex[0] + 1;
            CurrentIndex[1] = CurrentIndex[1] + 1;
        }
    }
}

Selection Sort - Usually outperforms bubble sort, but it is still not a very good sorting algorithm for large arrays.

void SelectionSort(int Array[], int Length)
{
    int BeginComparisonsIndex;
    int CurrentComparisonIndex;
    int CurrentLowestIndex;
    int TemporaryValue;

    BeginComparisonsIndex = 0;
    while(BeginComparisonsIndex != Length)
    {
        CurrentComparisonIndex = BeginComparisonsIndex;
        CurrentLowestIndex = BeginComparisonsIndex;
        while (CurrentComparisonIndex != Length)
        {
            if (Array[CurrentComparisonIndex] < Array[CurrentLowestIndex])
            {
                CurrentLowestIndex = CurrentComparisonIndex;
            }

            CurrentComparisonIndex = CurrentComparisonIndex + 1;
        }

        TemporaryValue = Array[BeginComparisonsIndex];
        Array[BeginComparisonsIndex] = Array[CurrentLowestIndex];
        Array[CurrentLowestIndex] = TemporaryValue;
        BeginComparisonsIndex = BeginComparisonsIndex + 1;
    }
}

About aesthetics

  I used the "long" form of all math statements, avoided the use of pointers, and used only while loops for aesthetic purposes (simply put, I think the code is prettier when it is written like that.)

While making the code look aesthetically pleasing can be beneficial (especially if it makes it more readable), you should not give that a higher priority than writing maintainable code. The "short" form of modifying variables has the advantage that you don't have to repeat the variable name. This might make the code easier to read, but the big advantage is that it reduces potential errors: for example, it's impossible to accidentily put the wrong variable name on the right hand side if there is none to begin with. So if you'd write:

CurrentIndex[0]++;

You can't accidentily misspell the variable name, you can't accidentily use [1] on the right hand side, and you can't accidentily write + 2 instead of + 1.

Another benefit is that incrementing a counter using ++ is more ideomatic, which means that there is a big chance that other programmers that are used to C will find the short form more natural.

Toby Speight already mentioned why for-loops can improve clarity over while-loops with separate initialization and incrementing statements.

The use of arrays instead of pointers is perfectly fine here, since you are indeed manipulating arrays.

Variable names

Descriptive variable names help understand code, but having too long variable names can start to hinder readability. Also, some one-letter variable names are justified, for example it is idiomatic to use i and j as loop counters.

Also, when you write CurrentIndex in BubbleSort(), it makes me wonder: is there also PreviousIndex or a NextIndex? If not, just Index would be better (or even just i). In SelectionSort() it makes more sense.

CPU efficiency

Your bubblesort implementation might have the advantage of quiting early if the input array was already sorted, but if it was not, then it takes twice as long as necessary. Instead of comparing two consecutive elements at a time, sort by first comparing the first element to all other elements, and swapping when necessary. After that first pass, you know that the first element is the lowest, so you don't have to consider it in the next pass anymore, and so on.

If you want to sort arrays of integers in the most efficient way, you probably want to implement radix sort.

Make sure you handle corner cases

In BubbleSort(), if the length of the input array is zero, then your code will go into an infinite loop and read out of bounds. Always check that you handle corner cases like arrays of length 0 and 1.

Another issue is that since Length is an int, someone could pass a negative value. To avoid that, use size_t Length instead (and change the type of the indices to match), or at least add in a check that Length is not negative. It still make sense to allow a length of 0 or 1.


Removing a header and footer from text output with PDFminer I use PDFminer to extract text from a PDF, then I reopen the output file to remove an 8 line header and 8 line footer. Is there a more efficient way to remove the header/footer, either in place or without re-opening/closing the file? Please mention general best practices I did not follow.

# convert PDF to text
def write_txt(pdf):
    outfile = pdf.split("_")[4] + "_" + pdf.split("_")[1] + "_" + \
        pdf.split("_")[0] + ".txt"
    pagenos = set([2,3])

    codec = 'utf-8'
    caching = True
    laparams = LAParams()

    rsrcmgr = PDFResourceManager(caching=caching)
    outfp = file(outfile, 'w+')
    device = TextConverter(rsrcmgr, outfp, codec=codec, laparams=laparams)
    fp = file(pdf, 'rb')
    interpreter = PDFPageInterpreter(rsrcmgr, device)
    for page in PDFPage.get_pages(fp, pagenos):
        interpreter.process_page(page)
    fp.close()
    device.close()
    outfp.close()

    # remove header/footer
    with open(outfile, 'r') as txtin:
        lines = txtin.read().splitlines(True)
    with open(outfile, 'w') as txtout:
        txtout.writelines(lines[8:-8])

    return outfile

This line performs the same string splitting 3 times:

outfile = pdf.split("_")[4] + "_" + pdf.split("_")[1] + "_" + \
    pdf.split("_")[0] + ".txt"

It would be better to split only once, and also to replace the messy string concatenation:

parts = pdf.split("_")
outfile = '{}_{}_{}.txt'.format(parts[4], parts[1], parts[0])

The variables could have better names.
When I see "pdf",
it makes me think of a PDF object.
But in this code it's just the filename string.
So I'd call it pdf_filename.

I don't have this Python library you're using,
and I'm just wondering if you could rewrite all those resources that you close manually (fp, device, outfp) to use the with syntax instead like you used for txtin and txtout.
If that's not possible with this library, then nevermind.


Changing sales stage I think this code is not optimized:

function changeSalesStage(value){

    var $form = $( getProductsForm() );

    if( typeof(value) == 'undefined' ) value = $("#sales_stage", $form).val();

    // Failure
    var cause = $("#causes_failure", $form);
    var notes = $("#failure_notes", $form);
    var seconder = $("#seconder", $form);

    if( cause.length == 0 || seconder.length == 0 ) return;

    var el1 = cause.parentsUntil('tr');
    var el2 = notes.parentsUntil('tr');
    var el3 = seconder.parentsUntil('tr');
    var display = 'none';
    if(value == 'Closed Lost'){
        el1.css('display', '');
        el2.css('display', '');
        el1.prev().css('display', '');
        el2.prev().css('display', '');
        el3.css('display', 'none');
        el3.prev().css('display', 'none');
        seconder.val('');
    }
    else if(value == 'IdentifyAnalysis') {
        el3.css('display', '');
        el3.prev().css('display', '');
        cause.val('');
        notes.val('');
        el1.css('display', 'none');
        el2.css('display', 'none');
        el1.prev().css('display', 'none');
        el2.prev().css('display', 'none');
    }
    else {
        display = 'none';
        el3.css('display', display);
        el3.prev().css('display', display);
        el1.css('display', display);
        el2.css('display', display);
        el1.prev().css('display', display);
        el2.prev().css('display', display);
    }
    // validate
    var formname = $form.attr('name');
    var formname1 = $form.attr('name1');
    if( typeof(validate[formname1]) == 'undefined' ) {return;}
    else if( typeof(validate[formname]) == 'undefined' ) return;

    var index = -1, label, name = "causes_failure", name1 = "seconder";
    for( i=0; i < validate[formname].length ||  i < validate[formname1].length; i++ ) {
        if( validate[formname][i][nameIndex] == name || validate[formname][i][nameIndex] == name1){
            index = i;
            label = validate[formname][i][msgIndex];
            label = validate[formname1][i][msgIndex];
            break;
        }
    }
    if( index == -1 ) return;

    if( display == 'none' ) {
        cause.val('');
        notes.val('');
        seconder.val('');
        validate[formname][index][requiredIndex] = false;
        validate[formname1][index][requiredIndex] = false;
    }
    else {
        validate[formname][index][requiredIndex] = true;
        validate[formname1][index][requiredIndex] = true;
    }
}

It's not "unoptimized". it's just bad.

your JS code shouldn't contain CSS manipulation (unless you are actually making complex animations)
even when your if and for statements have a single line following it, that code should be wrapped in { .. }
use === when comparing values, unless you explicitly require behaviour, that is provided by == loose comparison
you should use meaningful variable names, instead of el1, el2, etc.
none of this required jQuery
don't define variables in the middle of a function


Convert carry-add loop to a map or reduce to one-liner I want to perform standard additive carry on a vector. The base is a power of 2, so we can swap modulus for bitwise AND.

def carry(z,direction='left',word_size=8):
        v = z[:]
        mod = (1<<word_size) - 1
        if direction == 'left': v = v[::-1]
        accum = 0
        for i in xrange(len(v)):
                v[i] += accum
                accum = v[i] >> word_size
                v[i] = v[i] & mod
                print accum,v
        if direction == 'left': v = v[::-1]
        return accum,v

Is there any way to make this function even tinier?

Your code has a lot of copying in it. In the default case (where direction is left) you copy the array three times. This seems like a lot. There are various minor improvements you can make, for example instead of

v = v[::-1]

you can write

v.reverse()

which at least re-uses the space in v.

But I think that it would be much better to reorganize the whole program so that you store your bignums the other way round (with the least significant word at the start of the list), so that you can always process them in the convenient direction.
The parameters direction and word_size are part of the description of the data in z. So it would make sense to implement this code as a class to keep these values together. And then you could ensure that the digits always go in the convenient direction for your canonicalization algorithm.

class Bignum(object):
    def __init__(self, word_size):
        self._word_size = word_size
        self._mod = 2 ** word_size
        self._digits = []

    def canonicalize(self, carry = 0):
        """
        Add `carry` and canonicalize the array of digits so that each
        is less than the modulus.
        """
        assert(carry >= 0)
        for i, d in enumerate(self._digits):
            carry, self._digits[i] = divmod(d + carry, self._mod)
        while carry:
            carry, d = divmod(carry, self._mod)
            self._digits.append(d)

Python already has built-in support for bignums, anyway, so what exactly are you trying to do here that can't be done in vanilla Python?

Edited to add: I see from your comment that I misunderstood the context in which this function would be used (so always give us the context!). It's still the case that you could implement what you want using Python's built-in bignums, for example if you represented your key as an integer then you could write something like:

def fold(key, k, word_size):
    mod = 2 ** (k * word_size)
    accum = 0
    while key:
        key, rem = divmod(key, mod)
        accum += rem
    return accum % mod

but if you prefer to represent the key as a list of words, then you could still implement the key-folding operation directly:

from itertools import izip_longest

class WordArray(object):
    def __init__(self, data = [], word_size = 8):
        self._word_size = word_size
        self._mod = 1 << word_size
        self._data = data

    def fold(self, k):
        """
        Fold array into parts of length k, add them with carry, and return a
        new WordArray. Discard the final carry, if any.
        """
        def folded():
            parts = [xrange(i * k, min((i + 1) * k, len(self._data)))
                     for i in xrange((len(self._data) + 1) // k)]
            carry = 0
            for ii in izip_longest(*parts, fillvalue = 0):
                carry, z = divmod(sum(self._data[i] for i in ii) + carry, self._mod)
                yield z
        return WordArray(list(folded()), self._word_size)


Calculation of Energy Spectrum using C++ My purpose is to calculate the energy spectrum of three-dimensional turbulent flow from three velocity components. The calculation is too expensive. Therefore, I hope the calculation time can be reduced. The above code is a sample of calculating the energy spectrum. I expect to receive some suggestions on parallel computing using OpenMP for the energy E[k]. The size of i and j loops are 1000 000 while that of k is 100. Some parameters, x, y, z are velocity components and d1 is velocity magnitude, are created for code structure. I desire that the calculation is designed well on a pc with 40 cores.
This code is compiled as g++ test.cpp -o run -fopenmp
How can i parallelize this following code using openmp:
#include <iostream>
#include <math.h>
#include <vector>
#include <math.h>
#include <cmath>
#include <omp.h>

using namespace std;

int main()
{

    int i,j,k;
    int np = 1000000;  // number of fluid particle
    int kmax = 100;   // wavenumber
    double E[kmax+1] = {0.0};  //energy

    // need to be parallelized
    for (i = 0; i < np-1; i++){
        for (j = i+1; j < np; j++){

            double x = sin(double(2*i+j)/(3.0*np));//x,y,z,d1 for test.
            double y = sin(double(i+j)/(2.0*np));//x,y,z are velocity
            double z = sin(double(i*j)/np/np); //d1 velocity magnitude
            double d1 = sqrt(x*x + y*y + z*z);

            for (k = 1; k <= kmax; k++){

                double d2 = k * d1;
                E[k] += x + y + sin(d2) / d2;
            }

        }
    }

    return 0;
}

Avoid using namespace std;
If you are coding professionally you probably should get out of the habit of using the using namespace std; statement. The code will more clearly define where cout and other identifiers are coming from (std::cin, std::cout). As you start using namespaces in your code it is better to identify where each function comes from because there may be function name collisions from different namespaces. The identifiercout you may override within your own classes, and you may override the operator << in your own classes as well. This stack overflow question discusses this in more detail.
Include Files
You are currently including math.h 3 times, it is only necessary to do this once. Since you are using C++, stick with #include <cmath>.
Declare the Variables as Needed
The code contains this line:
    int i,j,k;

These are the variables you are using as loop control variables, they can therefore be declared in the loops themselves:
    for (int i = 0; i < np-1; i++){
        for (int j = i+1; j < np; j++){
        ...
        }
    }

Break the Code Into Functions to Make it Easier to Implement and Debug
Currently the code is all in main(), as programs grow and features get added the code would be to complex to easily update if it is all in main(). Also to achieve the ultimate goal of multi-threading for this program you will need to break the code up into multiple functions that can run on a separate core.
void FUNCTION_NAME(int i, int j, int np, double E[])
{
    double x = sin(double(2*i+j)/(3.0*np));//x,y,z,d1 for test.
    double y = sin(double(i+j)/(2.0*np));//x,y,z are velocity
    double z = sin(double(i*j)/np/np); //d1 velocity magnitude
    double d1 = sqrt(x*x + y*y + z*z);

    for (k = 1; k <= kmax; k++){
        double d2 = k * d1;
        E[k] += x + y + sin(d2) / d2;
    }
}

int main()
{
    int np = 1000000;  // number of fluid particle
    int kmax = 100;   // wavenumber
    double E[kmax+1] = {0.0};  //energy

    // need to be parallelized
    for (i = 0; i < np-1; i++){
        for (j = i+1; j < np; j++){
            FUNCTION_NAME(i, j, np, E[]);
        }
    }
}


Regex, match the most informative pattern I have a function that is designed to parse an utterance (or typed in string) and identify the intent as a yes or no answer.

There are many ways of saying "yes" to something, and similarly for "no" or "unsure"; however, some of these phrases may include sub-phrases that have alternative meanings. For example: certainly vs. certainly not.

I can't guarantee that not will always refer to a negative statement either, for example sure vs not sure; I do not know; or even why not! (the latter could be taken as an affirmative.)

Since I am interested only in phrases and sub-phrases (not words within words), I realised that the way to identify the most informative phrase is to attempt to match them in order of size. Also, if the larger phrase matches, I also don't want to match the sub-phrases as separate. Fortunately, there is a feature of regular expressions that can do this quite easily.

/\b(a b c|a b|a c|b c|a|b|c)\b/ where 'a', 'b', and 'c' are words, will match the first phrase it can find over the others. Therefore, /\b(a b c|a b|a c|b c|a|b|c)\b/.matches("a b c a") == ["a b c", "a"] as opposed to ["a", "b c", "a"] or another combination.
Example here

(Note that for this to work, the order of the 'or'ed phases need to be in descending order of length. Regex captures lazy, and will succeed on the first 'or'-block in the capture group.)

So here's a test function of the idea in C#:

public enum Affirmation { No=0, Yes, Unsure, NoAnswer }
public Affirmation getAffirmation(string utterance)
{
    string[] yesses = new string[] { "yes", "yep", "y", "yeah", "certainly", "sure", "why not", "agree", "affirmative" };
    string[] nos = new string[] { "no", "nope", "nah", "nup", "negative", "n", "certainly not", "yeah nah" };
    string[] unsures = new string[] { "unsure", "not sure", "i don't know", "maybe", "come again", "i do not know" };
    string[] responses = yesses.Concat(nos).Concat(unsures).OrderByDescending(w => w.Length).ToArray();
    Regex response = new Regex($"\\b({string.Join("|", responses)})\\b", RegexOptions.IgnoreCase);
    MatchCollection matches = response.Matches(utterance);
    if (matches.Count == 0)
    {
        return Affirmation.NoAnswer;
    }

    IEnumerable<Affirmation> affirmations = matches.Cast<Match>().Select(m => {
        if (yesses.Contains(m.Value.ToLower())) { return Affirmation.Yes; }
        else if (nos.Contains(m.Value.ToLower())) { return Affirmation.No; }
        else { return Affirmation.Unsure; }
    });
    return affirmations.Aggregate((agg, aff) =>
    {
        if (agg == Affirmation.Unsure || agg == aff) { return agg; }
        else { return Affirmation.Unsure; }
    });
}

Obviously the three string arrays aren't particularly long in this example; however there's no reason I couldn't try and enumerate all sorts of different phrases and categorise them by intent.

My question is with that Regex builder:

Regex response = new Regex($"\\b({string.Join("|", responses)})\\b", RegexOptions.IgnoreCase);

This pattern could end up being very large.

Is there a limit to the number of characters for a Regex string?
Is there a performance issue to worry about as a capture block
becomes an extremely large 'Or' block?
Is there a better way to do this in general?

I cannot say much about the limits of the pattern, the only one that I encountered so far was the length of the group name.

IEnumerable<Affirmation> affirmations = matches.Cast<Match>().Select(m => {
    if (yesses.Contains(m.Value.ToLower())) { return Affirmation.Yes; }
    else if (nos.Contains(m.Value.ToLower())) { return Affirmation.No; }
    else { return Affirmation.Unsure; }
});

You can however make the lookup faster by using a HashSet<T>. Its Contains method is an O(1) operation unlike the array's O(n).

Additionaly you should instantiate it with StringComparer.OrdinalIgnoreCase so that you don't have to use ToLower/ToUpper.

Another possibility would be to use named groups and catch the expressions there so that you can just use them without looking them up in the collections.


Adding order actions like "Print" or "RePrint" The eventual desire to implement code that's clean and extendable. I always fear that I'm overusing If's and or not seeing the appropriate pattern to reduce conditionals.

Now I love architecture, micro service's etc I even enjoy IoC, ISP(SOLID) But when it comes down to small encapsulated methods, the nuances of reducing If's , I struggle to grasp.

I know I've instantiated concrete types and coupled the responsibility(May have just answered my own question). The reality is, I want to minimize the use of If's.

Could I kindly request a Code Review on the below snippet?

public async Task AddOrderActionAsync(Identity identity,
    string value,
    string order_number)
{
    if (!string.IsNullOrWhiteSpace(value))
    {
        var client = await _context.tbl_Client.FirstOrDefaultAsync(c =>
                            c.ClientKey == identity.ClientKey);
        try
        {
            // See if order action exists, if it does it could contain multiple
            // so delete all and re-add one
            var orderAction = _context.tbl_OrderActions.Where(o =>
            o.tbl_Client.ClientKey == identity.ClientKey &&
            o.OrderNumber == order_number).ToList();

            if (orderAction != null &&
                orderAction.Count() > 0)
            {
                _context.tbl_OrderActions.RemoveRange(orderAction);
               await AddOrderActionAsync(_context, identity,
                                       value, order_number);
            }
            else
            {
                await AddOrderActionAsync(_context, identity, value, order_number);
            }

            await _context.SaveChangesAsync();
        }
        catch (AddOrderActionException ex)
        {
            new Email().SendEmail(identity.ClientName,
                    "Add Order Action Error - Data API",
                    "Add Order Action Error(" + value + " - " + order_number + "): " + ex.Message,
                    "error@retain.me");

            //System needs to run regardless if this fails
            return;
        }
    }
}

The above code is adding an "OrderAction" which could be "Print" or "RePrint", there is a possibility that multiple could be added else where in the pipeline or another app. The ideas is to remove the multiple and re-add one an/or just add.

There aren't too many ifs in this code. However, what you can reduce is indentation and it will help readability.

This code:

public async Task AddOrderActionAsync(...)
{
    if (!string.IsNullOrWhiteSpace(value))
    {
        // Everything here
    }
}

Can be rewritten as:

public async Task AddOrderActionAsync(...)
{
    if (string.IsNullOrWhiteSpace(value))
        return;

    // Everything here
}

With the nice side-effect to also remove negation.

Looking into the code you check orderAction != null, it seems you try to be defensive here but it's misleading because it can't be null. Ever. Also orderAction.Count() > 0 is equivalent to orderCount.Any() which is shorter, faster and communicates better the intent. Content of the else clause is repeated then one of them should be dropped:

if (orderAction.Any())
    _context.tbl_OrderActions.RemoveRange(orderAction);

await AddOrderActionAsync(_context, identity, value, order_number);

In your query you're using .ToList() but it's unnecessary and without it EF (or ORM you're using) might be able to optimize database calls (if not then you at least gain in clarity).

It's not a rule but an accepted convention in C# to do not use underscore to separate words, a variable named order_number should be orderNumber.

You're catching a specific exception, nice! However I think it can be thrown only by AddOrderActionAsync() then it should be the only part inside try/catch:

try
{
    await AddOrderActionAsync(_context, identity, value, order_number);
}
catch (AddOrderActionException ex)
{
    // ...
}

Really don't ignore t3chb0t's suggestion about dependency injection, sending e-mails like that it's definitely not something you want to happen inside your unit testing...


Capitalised initials of username Given a list of words, this program outputs the starting letters of the words capitalized.

I was reviewing this question that solves the same task, but over time so many changes accumulated, that I think my programme bears little resemblance to the original one, so instead of posting it as an answer, I ask you for further improvement.

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

const int NAME_LEN = 160 + 1;

bool is_letter(char ch)
{
    return ch >= 'a' && ch <= 'z';
}

bool is_terminating_char(char ch)
{
    return ch == '\n' || ch == '\0';
}

const char * first_letters_only_capitalized(char words[NAME_LEN]) {

    char *capitals = malloc(sizeof(char) * NAME_LEN);
    int pos = -1;

    for(int i = 0; ! is_terminating_char(words[i]); i++)
    {
        capitals[++pos] = is_letter(words[i]) ? toupper(words[i]) : words[i];

        while (is_letter(words[i]))
        {
            i++;
        }
    }

    capitals[++pos] = '\0';

    return capitals;
}

int main()
{
    char user_name[NAME_LEN], user_initials[NAME_LEN];

    puts("Enter your name, I will output the initials capitalized: ");
    if ( ! fgets(user_name, NAME_LEN, stdin))
    {
        puts("Error reading input.");
        return 1;
    }

    strcpy(user_initials, first_letters_only_capitalized(user_name));
    printf("%s\n", user_initials);
}

Example session:

Enter your name, I will output the initials capitalized:
foo bar buz
FBB

Here are some things that may help you improve your code:

Avoid memory leaks

The first_letters_only_capitalized function allocates memory (or tries to, see next suggestion) and returns a pointer to that memory, but it is never freed by the calling function.  Instead of using strcpy into a fixed size string, you could instead use the return value directly:

char *user_initials = first_letters_only_capitalized(user_name);
if (user_initials != NULL) {
    puts(user_initials);
}
free(user_initials);

Check for failed memory allocation

Whenever a program allocates memory explicitly, as with using malloc or calloc, it should check to make sure that the allocation succeeded before using the resulting pointer.  When malloc fails, it returns NULL, so you could modify your program like this:

char *capitals = malloc(sizeof(char) * NAME_LEN);
if (capitals == NULL) {
    return capitals;
}

Use const appropriately

One of the functions in your program is declared like this:

const char * first_letters_only_capitalized(char words[NAME_LEN])

But I think it should be this instead:

char * first_letters_only_capitalized(const char words[NAME_LEN])

The reason is that the first signature says that that words might be altered but that the returned pointer may not be.  That seems backwards from the real situation which is that words is not altered by the program (and therefore should be declared const) but that the return value may be used by the caller in any way, including by using free.

Use consistent formatting

Using consistent formatting helps readers of your code understand it without distraction.  This code is mostly well formatted, but the opening brace of the first_letters_only_capitalized is on the same line, which is unlike all the other functions in the program.

Reconsider the sense of boolean functions

The only time is_termininating_char is used, its return value is negated.  That suggests the function sense should be inverted:

bool is_not_terminating_char(char ch)
{
    return ch != '\n' && ch != '\0';
}

Allow mixed case input

As the program stands, if the user enters "John Smith" the program returns "JOSM" which is rather odd.  Instead, alter the program to allow for mixed case input.  It may also be interesting to consider other cases such as hyphenated last names and last names such as "von Trapp" in which part of the name is never capitalized.

Trim leading space

The program currently does not trim non-alphabetic leading characters including spaces, so an input of "  #!%!* Al Alpha" yields an output of "  #!%!* ALAL".

Use standard functions where appropriate

Instead of writing your own, it would make more sense to me to use standard function isalpha rather than writing your own is_letter function.

Consider using pointers

Using pointers in your first_letters_only_capitalized function would make it more idiomatic C in my opinion.  Your first_letters_only_capitalized function might  look like this.

char * first_letters_only_capitalized(const char words[NAME_LEN])
{
    char *capitals = malloc(sizeof(char) * NAME_LEN);
    if (capitals == NULL) {
        return capitals;
    }
    char *init = capitals;

    for(const char *ch = words; is_not_terminating_char(*ch); ++ch)
    {
        if (isalpha(*ch)) {
            *init++ = toupper(*ch);
        }
        while (isalpha(*ch)) {
            ++ch;
        }
    }
    *init++ = '\0';

    return capitals;
}

Avoid overly long names

In writing this review, I typed first_letters_only_capitalized nine times.  It's exhausting!  Show some mercy to other programmers and use shorter names, although using a very long name for a function whose purpose is to shorten names might be amusing irony to some.


Reorder the letters of one string to match the order of letters in a second string I have a solution to the following problem that I'd like to know if there's a way I could decrease runtime complexity for.

The directions are: reorder the letters of the first string to match the order of the letters in the second string. If a letter in the first string is not present in the second string, add it to the end of the reordering.

Example input: 'shouts', 'soup'
Example output: 'ssouht'

My code is as follows:

const reOrderLetters = (string1,string2) => {
  let tailString = '';
  let newString = '';
  const mapped = {};

  for (let i=0;i<string1.length;i++) {
    let char = string1[i];
    if (string2.indexOf(char) !== -1) {
      if (!mapped[char]) {
        mapped[char] = char;
      } else {
        mapped[char] = mapped[char] += char;
      }
    } else {
      tailString += char;
    }
  }
  for (let i=0;i<string2.length;i++) {
    let char = string2[i];
    if (mapped[char]) {
      if (mapped[char].length > 1 && string2.slice(i+1).indexOf(char) !== -1) {
        newString += mapped[char].slice(mapped[char].length-1);
        mapped[char] = mapped[char].slice(0,mapped[char].length-1);
      } else {
        newString += mapped[char];
      }
    }
  }
  return newString += tailString;

}

Your code is pretty good, except for using indexOf(char), as that can be quite expensive: O(string2.length). Here is how I would do it to avoid using that. Assuming hashmap access is O(1) and n being the max length between both strings, my implementation runs in O(n)

function characterCount (str) {
  var chars = {};

  for (var i = 0; i < str.length; i ++) {
    chars[str[i]] = (chars[str[i]] || 0) + 1;
  }

  return chars;
}

function reOrderLetters (str1, str2) {
  var count1 = characterCount(str1); // O(n)
  var count2 = characterCount(str2); // O(n)
  var result = [];
  var seenCount = {}

  for (var i = 0; i < str2.length; i++) {
    var char = str2[i];
    seenCount[char] = (seenCount[char] || 0) + 1;

    if (count1.hasOwnProperty(char)) {
      var remaining = count1[char] % count2[char];
      var toRepeat = Math.floor(count1[char] / count2[char]);

      if (seenCount[char] <= remaining) {
        toRepeat = Math.ceil(count1[char] / count2[char]);
      }

      result.push(char.repeat(toRepeat));
    }
  }

  for (var i = 0; i < str1.length; i++) {
    var char = str1[i];

    if (count2.hasOwnProperty(char) == false) {
      result.push(char.repeat(count1[char]));
      count2[char] = true;
    }
  }

  return result.join("");
}

console.log(reOrderLetters("shouts", "soups") == "sousht")
console.log(reOrderLetters("aa", "aaaa") == "aa")
console.log(reOrderLetters("aabbbba", "abab") == "aabbabb")


jQuery Chat UI Widget I'm creating a chat UI with jQueryUI:

$.widget("ui.chatwindow", {
    options: {
        nickname: "obama";
    },
    setNickname: function(nick){
        var self = this;
        var id_pre = 'wchat_' + self.options.nickname;
        $('#' + id_pre + '\\:name').text(self.options.nickname);
    },
    setStatus: function(status){
            var self = this;
            var id_pre = 'wchat_' + self.options.nickname;
            switch(status){
                case 1:
                    $('#' + id_pre + '\\:status').removeAttr('class');
                    $('#' + id_pre + '\\:status').addClass('chat-icon-online');
                    $('#' + id_pre + '\\:status').attr('title','Online');
                    break;
                    ...
                default:
                    break;
            }
            ...
        },
    ...
}

I always write this in every method to change element class or text content:

var self = this;
var id_pre = 'wchat_' + self.options.nickname;

Is this a good or efficient way to code?

I can't say there's anything technically wrong with doing it that way, and maybe some people who are only familiar with "self" in certain languages will get the point, but I think "this" is just as readable, and it saves you some code (along with some of us wondering where in the world "self" was defined if we're looking at a lot of code where the declaration isn't obvious).

So short of adding some extra work for yourself (and raising a couple of eyebrows), I don't see a big problem with it. Do you need it? No. Will it hurt? Not likely.


Alternative to starting enum values with a number I have an enum like the following:

enum MeasurementBandwidth
{
    Hz1 = 1,
    Hz3 = 3,
    Hz10 = 10,
    ...
}

But I do not like the Hz1, and 1Hz is not valid as it starts with a number.

Does anyone have an idea how to solve this "more elegant"?

Why I am doing this:

I have an API, which offers a method like the following:

public double MeasureInput(double frequency, MeasurementBandwidth bandwidth) { /*...*/ }

And the bandwidth only has a few possible values which can be used and I want to make it as easy as possible for the "user" to choose a valid one. Using an enum was the best solution I came up with, as it is directly visible from the parameter type, what values are valid.

public enum Hertz{
    none = 0,
    Hz= 1,
    KHz = 1000,
    MHz = 1000000,
    GHz = 1000000000
}

public static class Frequency{
    public static int Hz(this int frequency){
        return frequency;
    }

    public static int KHz(this int frequency) {
        return  frequency * (int)Hertz.KHz;
    }

}

client code:

int freq = 10.Hz();
int faster = 10.KHz();
int thousandsOfGigahertz = 234.KHz().GHz();


Inefficient Python script eating up memory (I believe) I created a simple Python script to log track listens in iTunes and it seems that it's pretty inefficient. I'm primarily a front-end developer, so this is clearly not my area of expertise. I know loops can cause problems in any language, so I assume that's the issue here.

If I run this script for a while (say, a couple of hours or more) the fan on my computer starts working overtime and won't stop until I kill the script.

Any idea what might be making this happen? Suggestions on how to optimize? Appreciate any help I can get.

#!/usr/bin/python
# -*- coding: utf-8 -*-

import wx, sqlite3, datetime, threading
from appscript import *
it = app('iTunes')
from AppKit import *

# Create the DB
Tapedeck = sqlite3.connect('Tapedeck.db', check_same_thread = False)
TapedeckQuery = Tapedeck.cursor()
Tapedeck.execute('''CREATE TABLE IF NOT EXISTS plays (id INTEGER PRIMARY KEY, track_id TEXT, track TEXT, artist TEXT, album TEXT, genres TEXT, play_count INTEGER, rating INTEGER, skip_count INTEGER, year_released INTEGER, date_played DATE)''')
Tapedeck.commit()

def getSec(s):
    l = s.split(':')
    return int(l[0]) * 60 + int(l[1])

def addTrack(track_id, track, artist, album, genres, play_count, skip_count, year_released, date_played, rating):
    #print '        --\n        Position: {}\n      Length: {}'.format(it.player_position(),getSec(it.current_track.time()))
    TapedeckQuery.execute('''INSERT INTO plays (track_id, track, artist, album, genres, play_count, skip_count, year_released, date_played, rating) VALUES (?,?,?,?,?,?,?,?,?,?)''',
    (track_id, track, artist, album, genres, play_count, skip_count, year_released, date_played, rating))
    Tapedeck.commit()

loop_int = 1.0
last_track = '0'

def listen():

    global loop_int, last_track
    threading.Timer(loop_int, listen).start()
    if it.player_state() == k.playing:
        # check to see if track was restarted
        if it.player_position() < getSec(it.current_track.time())/2 and last_track == it.current_track.persistent_ID():
            self.last_track = '0'
        # has the track played beyond the halfway mark?
        if it.player_position() >= getSec(it.current_track.time())/2 and last_track != it.current_track.persistent_ID():
            today = datetime.datetime.today()
            now = '{}-{}-{} {}:{}:{}'.format(today.year, '%02d' % today.month, '%02d' % today.day, '%02d' % today.hour, '%02d' % today.minute, '%02d' % today.second)
            print '\nArtist:        {}\nTrack:      {}\nAlbum:      {}\nGenres:     {}\nDatetime:   {}'.format(it.current_track.artist().encode('ascii','ignore'), it.current_track.name().encode('ascii','ignore'), it.current_track.album().encode('ascii','ignore'), it.current_track.genre().encode('ascii','ignore'), now)
            last_track = it.current_track.persistent_ID()
            addTrack(it.current_track.persistent_ID(), it.current_track.name(), it.current_track.artist(), it.current_track.album(), it.current_track.genre(), it.current_track.played_count(), it.current_track.skipped_count(), it.current_track.year(), now, it.current_track.rating())

listen()

#!/usr/bin/python
# -*- coding: utf-8 -*-

import wx, sqlite3, datetime, threading
from appscript import *

import * is frowned upon as it makes it difficult to trace where names came from.

it = app('iTunes')

I recommend avoiding abbreviated names like it its hard to figure out what it means later.

from AppKit import *

# Create the DB
Tapedeck = sqlite3.connect('Tapedeck.db', check_same_thread = False)
TapedeckQuery = Tapedeck.cursor()
Tapedeck.execute('''CREATE TABLE IF NOT EXISTS plays (id INTEGER PRIMARY KEY, track_id TEXT, track TEXT, artist TEXT, album TEXT, genres TEXT, play_count INTEGER, rating INTEGER, skip_count INTEGER, year_released INTEGER, date_played DATE)''')
Tapedeck.commit()

Generally, performing actions at the module level is frowned upon. You should do it in a main function.

def getSec(s):

Python style guide recommends lowercase_with_underscores for function names. Also, don't needlessly abbreviate.

    l = s.split(':')
    return int(l[0]) * 60 + int(l[1])

I'd have done:

minutes, seconds = s.split(':')
return int(minutes)*60 + int(seconds)

I think its easier to see what's going on

def addTrack(track_id, track, artist, album, genres, play_count, skip_count, year_released, date_played, rating):
    #print '        --\n        Position: {}\n      Length: {}'.format(it.player_position(),getSec(it.current_track.time()))
    TapedeckQuery.execute('''INSERT INTO plays (track_id, track, artist, album, genres, play_count, skip_count, year_released, date_played, rating) VALUES (?,?,?,?,?,?,?,?,?,?)''',
    (track_id, track, artist, album, genres, play_count, skip_count, year_released, date_played, rating))
    Tapedeck.commit()

loop_int = 1.0
last_track = '0'

def listen():

    global loop_int, last_track

Avoid global variables. You should generally only be using globals as constants.

    threading.Timer(loop_int, listen).start()

Python isn't Javascript. Instead of playing with threads, use time.Sleep(1) to sleep for a second between each attempt. Put everything in a loop. It'll be easier to follow.

    if it.player_state() == k.playing:
        # check to see if track was restarted
        if it.player_position() < getSec(it.current_track.time())/2 and last_track == it.current_track.persistent_ID():
            self.last_track = '0'

self?

        # has the track played beyond the halfway mark?
        if it.player_position() >= getSec(it.current_track.time())/2 and last_track != it.current_track.persistent_ID():
            today = datetime.datetime.today()
            now = '{}-{}-{} {}:{}:{}'.format(today.year, '%02d' % today.month, '%02d' % today.day, '%02d' % today.hour, '%02d' % today.minute, '%02d' % today.second)

The datetime has a formatting function which may be better suited to that

            print '\nArtist:        {}\nTrack:      {}\nAlbum:      {}\nGenres:     {}\nDatetime:   {}'.format(it.current_track.artist().encode('ascii','ignore'), it.current_track.name().encode('ascii','ignore'), it.current_track.album().encode('ascii','ignore'), it.current_track.genre().encode('ascii','ignore'), now)
            last_track = it.current_track.persistent_ID()
            addTrack(it.current_track.persistent_ID(), it.current_track.name(), it.current_track.artist(), it.current_track.album(), it.current_track.genre(), it.current_track.played_count(), it.current_track.skipped_count(), it.current_track.year(), now, it.current_track.rating())

listen()

As for your actual problem. I don't know this appscript/AppKit you are using. But I'll speculate that under some circumstances iTunes stops responding. Maybe a dialog gets popped or something. As a result, each thread gets stuck waiting for iTunes. But because you set the timer as your first action, a new thread keeps getting started which also gets stuck. Eventually, your poor scripts piles many many threads and that explains the hang.

If you use the sleep method I suggested above, that should prevent the script from running many threads.


Making a function which can be called with an object literal I am working on a code snippet to make a function which can be called with an object literal.

Problem area: If I do not need to track the product value, then a check has been placed for undefined. But do I really need to make check for each and every value if it's not there in the object literal?

trackProduct = function (args) {

      if(args.label == undefined) {
        value.push([args.category, args.action, args.value]);
      } else if(args.value == undefined){
        value.push([args.category, args.action, args.label]);
      } else {
        value.push([args.category, args.action, args.label, args.value]);
      }
};

I'm calling this function via below object literal. I have not passed the value parameter.

trackProduct({
    category: elemcategory,
    action: elemaction,
    label: elemlabel
});

Do we need to check for undefined for all three parameters? Any suggestions on how to improve this code?

First, just a note: You check against undefined, but undefined is unfortunately not a keyword, meaning it's mutable. Better to use typeof something === 'undefined'

If I understand you right, you want to be able to "leave out" anyone of the object values. In that case, here's my take on the code (I'm returning an array, just for clarity)

function trackProduct(obj) {
  var keys = ['category', 'action', 'label', 'value'],  // properties to look for
      values = [],
      i, l, value;

  for(i = 0, l = keys.length ; i < l ; i++) {
    value = obj[keys[i]];
    if(typeof value !== 'undefined') {
      values.push(value); // if the value's there, put it in the array
    }
  }

  return values;
}

This would work like so:

trackProduct({
  category: "foo",
  label:    "bar",
  value:    "23",
  ignored:  "something"
}); // => ["foo", "bar", "23"]


Sum of all arguments ever passed to accumulator Q2. Define a function make_accumulator that returns an accumulator
  function, which takes one numerical argument and returns the sum of
  all arguments ever passed to accumulator. Use a list and not a
  nonlocal statement.

Below is the solution:

def make_accumulator():
    """Return an accumulator function that takes a single numeric argument and
    accumulates that argument into total, then returns total.

    >>> acc = make_accumulator()
    >>> acc(15)
    15
    >>> acc(10)
    25
    >>> acc2 = make_accumulator()
    >>> acc2(7)
    7
    >>> acc3 = acc2
    >>> acc3(6)
    13
    >>> acc2(5)
    18
    >>> acc(4)
    29
    """
    lst = []
    def accumulate(n):
        lst.append(n)
        length = len(lst)
        total = 0
        for index in range(length):
            total = total + lst[index]
        return total
    return accumulate

Can we improve this solution?

An alternative to the closure is to add an attribute to the function, so rather than:

lst = []
def accumulate(n):
    lst.append(n)
    ...

you have:

def accumulate(n):
    accumulate.lst.append(n)
    ...
accumulate.lst = []

It's a little more typing, but one advantage is that it allows easier access to the list so far from outside the function:

>>> accum = make_accumulator()
>>> accum(7)
7
>>> accum(8)
15
>>> accum.lst
[7, 8]

With the closure, this would be accum.func_closure[0].cell_contents.

Using indices is not a very Pythonic way to handle iterating over a list, and x = x + y can be simplified to x += y; compare:

...
length = len(lst)
for index in range(length):
    total = total + lst[index]
...

with:

...
for item in lst:
    total += item
...

There is also sum(lst), which the specification doesn't forbid.

  How can I think of writing pure functional code for above program
  (i.e., without any state change)?

I don't think that this can be achieved in a purely functional way as specified - you are explicitly told to use a list to store the values passed, so there will always be state, and the function you create will give different outputs for the same inputs depending on the previous inputs (no referential transparency).


Generating a topographic string I am working my way through some exercises, using Java, to learn about programming. I have just finished the following Code Golf challenge:

  Here is some example input, so I can explain what the problem is:

  ((1 2)(3 (4 5) moo)) (i (lik(cherries)e (woohoo)))

  Think of this line of text as a topographic map of some mountains. Each set of parentheses illustrates one unit of altitude.

  If we "view" this from the side, so that we see the mountains vertically, we will see:

           4 5                cherries    woohoo
  1 2  3       moo       lik          e
                      i

  Given one of these topographic maps, output the map, but on a vertical scale, like the output above. Separate the different items in the map with the number of characters to the next item. For example, there are 4 spaces in the output between moo and i. Likewise, there are 4 characters in the input between moo and i.

As a beginner, I am certainly not aiming to fulfill the challenge of the fewest characters. I'm merely trying to learn. I haven't looked at any other answers yet in case it puts me off posting this for feedback. So, here is my code:

public static String getLevelContent(String str, int count){

    int levelToPrint = 0;
    int spaceCount = -1;

    StringBuilder s = new StringBuilder();

    for (int i = 0; i < str.length(); i++) {

        spaceCount++;

        if (str.charAt(i) == '(') {
            levelToPrint++;
        }

        if (str.charAt(i) == ')') {
            levelToPrint--;
        }

        if (levelToPrint == count && str.charAt(i) != '(' && str.charAt(i) != ')') {

            for (int j = 0; j < spaceCount; j++) {
                s.append(" ");
            }

            spaceCount = -1;
            s.append(str.charAt(i));

        }
    }
    return s.toString();
}

public static void printTopographic(String str){

    for (int i = 3; i >= 1; i--) {
        System.out.println(getLevelContent(str, i));
    }
}

public static void main(String[] args) {

    String str = "((1 2)(3 (4 5) moo)) (i (lik(cherries)e (woohoo)))";
    printTopographic(str);
}

Output:

           4 5                cherries    woohoo
  1 2  3       moo       lik          e
                      i

I am largely trying to better my efforts of structuring code. My questions are:

The reason I made getLevelContent a separate method is to avoid multiple for loops for each level of the output (3, 2, and 1). Is separating into methods, as I have done here, a good way to solve the problem?
In the printTopographic method, is the for loop more necessary, or more readable, than three separate lines, each calling separately on its own? (I suppose with this loop in place it would be more useful for dozens of lines as opposed to just three.)
Are there parts of my code which are just plain inefficient and should/would generally be written in a different way?
In projects like this, where I am simply testing what happens and how, I have completely gotten used to labeling static methods, pretty much out of laziness than anything else (for when calling from other methods). Is this a bad habit?

The reason I made getLevelContent a separate method is to avoid multiple for loops for each level of the output (3, 2, and 1). Is separating into methods, as I have done here, a good way to solve the problem?

If it works, it works. You said this is codegolf, so in the end you'll need to determine if it's worth it character-wise to use "extra" methods. Take a look at my response to point 2 and 3 though.

Apart form code golf though, I think "extra" methods help readability. By "extra" methods I mean private methods that are only used once. A better name may be superfluous methods. Say you have some long code that does something easy to understand but you only use it once in one method so it doesn't immediately make sense to extract it to its own method, if it makes the code more readable then I'd suggest extracting it.

  In the printTopographic method, is the for loop more necessary, or more readable, than three separate lines, each calling separately on its own? (I suppose with this loop in place it would be more useful for dozens of lines as opposed to just three.)

A for-loop is better than a separate method call for each line, in fact I'd say a separate call for each line is wrong, see the next one though.

  Are there parts of my code which are just plain inefficient and should/would genereally be written in a different way?

What happens if the input is (1(2(3(4))))? I don't think it's good to hard code the number of levels. Just because the example has three levels doesn't mean every input will have 3 levels. So if you want to do this right I would do something like putting the '(' on a stack to tell what level you are on, the highest number of '(' on the stack is the total number of levels, plus one if 1(2) is allowed. Pop '(' when ')' is met. Look up stuff on converting infix notation (3 * (1 + 2))) to postfix (1 2 + 3 *) for hints on how to do this.

  In projects like this, where I am simply testing what happens and how, I have completely gotten used to labelling static methods, pretty much out of laziness than anything else (for when calling from other methods). Is this a bad habit?

It's more important to understand what the difference between a static method and non-static method is than worrying about always using static in small projects like this.

A method being static means a few things, first is

static methods are on the class itself, not an instantiated object

and because of that

static methods are the same across all instances of the object

which may or may not matter depending on the context. (If you have what should be a "utility class" then it shouldn't be instantiated, see this where I explain what a utility class is.)

For small projects like this that are, for lack of a better term, more like making a script than a large program, most of the methods will end up being able to be static. So if you are making them static because they can be static then you are doing fine. In my opinion if a method can be static it should be static. If you are making them static because you have an obsession with static methods then you're probably missing something. static is not preferable to non-static if you need to do something related to the instance of the object. Perfect example, the idea of getters and setters...

class NumberHolder {
    static int num;
    public static int getNum() {
        return num;
    }
    public static void setNum(int num) {
        this.num = num;
    }
}

// ...

// numberHolders is a List<NumberHolder> with size 10
for (int i = 0; i < numberHolders.size(); i++) {
    numberHolders.get(i).setNum(i);
}
for (int i = 0; i < numberHolders.size(); i++) {
    System.out.println()numberHolders.get(i).getNum(i));
}

will output

9
9
9
9
9
9
9
9
9
9

Which, depending on the context and the problem might be what you want to happen, but personally seeing something like that I would expect

0
1
2
3
4
5
6
7
8
9


Spoj-Prime Generator Question-

Generate all prime numbers between two given numbers.

https://www.spoj.com/problems/PRIME1/

My attempt-

I used segmented sieve method.

t=int(input())
import math
def segsieve(x):
    if x==1:
        return False
    for i in range(2,int(math.sqrt(x))+1):
        if x%i==0:
            return False
    return True

while t>0:
    f,l=map(int,input().split())
    for j in range(f,l):
        a=segsieve(j)
        if a==True:
            print(j)
    t-=1

Issue-

I am getting time limit exceeded. How can I make it faster?

Algorithm

Hint

You don't need to check all integers between 2 and int(math.sqrt(x))+1. You only need to check primes between 2 and int(math.sqrt(x))+1.

Stylistic

__main__

I would add a "main function" to your program. This is typically done in Python by:

def main():
    ...

if __name__ == "__main__":
    main()

See this answer for more reasons why this paradigm is followed.

sqrt

I technically have not measured the performance of sqrt, but I have more commonly seen your for loop written as:

while i * i < x:
    ...

unlike the current:

for i in range(2,int(math.sqrt(x))+1):

No need to import math then.

PEP 8

According to the PEP 8 style guide, you should add a space between the mathematical operations. So for instance, instead of:

if x%i==0:

it should be:

if x % i == 0:


Find smallest index that is identical to the value in an array The task is taken from LeetCode

  Given an array A of distinct integers sorted in ascending order,
  return the smallest index i that satisfies A[i] == i.  Return -1 if no
  such i exists.

  Example 1:

Input: [-10,-5,0,3,7]
Output: 3
Explanation:
// For the given array, A[0] = -10, A[1] = -5, A[2] = 0, A[3] = 3, thus the output is 3.

  Example 2:

Input: [0,2,5,8,17]
Output: 0
Explanation:
// A[0] = 0, thus the output is 0.

  Example 3:

Input: [-10,-5,3,4,7,9]
Output: -1
Explanation:
// There is no such i that A[i] = i, thus the output is -1.

  Note:

  1 <= A.length < 10^4

  -10^9 <= A[i] <= 10^9

My solution

has time complexity of \$O(n)\$ and space complexity of \$O(1)\$. I start to look from the start to the last element. If I find a value that is greater than i, then I can exit early (because there won't be an element that is equal to i anymore). If I find A[i] === i, then I have a result.

Is there a faster solution than the one provided?

/**
 * @param {number[]} A
 * @return {number}
 */
var fixedPoint = function(A) {
    for (let i = 0; i < A.length; i++) {
        if (A[i] > i) { return -1; }
        if (A[i] === i) { return i; }
    }
    return -1;
};

Review

Your solution naively walks the array of ascending integers from starting position s = 0. In some situations, this means you are walking tons of negative numbers, knowing they can never match an array index, which is always nonnegative.

Optimization

You could optimize s before walking the array. Since array indices are nonnegative integers, you should skip walking the array where the values are strict negative.

As en example, if input = [-10000, -9999, ..., 0, 1] you just want to check 0 and 1.

The way I would optimize the algorithm:

determine starting point s

if first item is positive: s = 0
if last item is strict negative: return -1
perform binary search to find s (you want s to hold the first positive integer in the array)

walk i as from s to end of array

on match: return match
on array[i] > i: return -1
on end reached without match: return -1

Optimized Time Complexity

~\$0(\lg m)\$ with m <= n and m being the number of positive integers in the array


String similarity algorithm (c++) Parameters:

first - the first string.
second - the second string.
similarCharacters - outputs the number of similar characters.
tolerance - the number of mistakes allowed to consider the strings "similar."

Function:

bool Str_Similar(std::string first, std::string second, unsigned int* similarCharacters = nullptr, int tolerance = INT_MIN)
{
    // Don't even check if either strings are empty.
    if(first.empty() || second.empty()) return false;

    // Determine if the first is greater than or equal to the second.
    const bool firstGreaterOrEqualToSecond = first.length() >= second.length();

    // By default, set the tolerance to half the length of the smaller string.
    if(tolerance == INT_MIN) tolerance = (firstGreaterOrEqualToSecond ? (second.length() / 2) : (first.length() / 2));
    if(tolerance < 0) tolerance = 0;

    // Start off with any length difference, which are considered mistakes.
    unsigned int mistakes = (unsigned int)abs(first.length() - second.length());
    // Search only the length of the smaller string.
    const size_t searchLength = (firstGreaterOrEqualToSecond ? second.length() : first.length());

    // Do the search.
    for(size_t i = 0, max = searchLength; i < max; i++)
    {
        if(first.at(i) != second.at(i)) mistakes++;
    }

    // Output the similar characters.
    if(similarCharacters != nullptr) *similarCharacters = (unsigned int)abs(searchLength - mistakes);

    // Compare the mistakes to the tolerance.
    return (mistakes <= tolerance);
}

1. Prefer passing parameters by const reference

The std::string parameters should be passed by const reference rather than by value.

Even if passing by value would work properly, it makes the function signature clearer for the caller semantically and may be more efficient.

2. Fix all warnings

The line

return (mistakes <= tolerance);

results in a compiler warning:

warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

3. Prefer to use numeric_limits over the C-style INT_MIN

For C++ code you should prefer to use std::numeric_limits<int>::min() instead of the INT_MIN macro (I couldn't even get that to compile, though stdint.h was included).

4. Always use {} braces for conditional code sections

You always should use braces to enclose conditional code sections

if(similarCharacters != nullptr) {
     *similarCharacters = (unsigned int)abs(searchLength - mistakes);
}

Not only it improves the readability of the code, omitting the braces may make the code error prone for changes.

My compiling version can be found here.


Feeding stateful and stateful-effectful computations into each other I am writing code that runs a simulation in an interactive way. The building blocks are:
runStep :: I -> State S O
runWorld :: World m -> O -> StateT W m I

Note that the world has some state W but is also allowed to have effects of type m. My question is about the function that feeds runStep and runWorld into each other. My current implementation is quite heavy on the Control.Lens noise:
sim :: (Monad m) => World m -> StateT (I, S, O) m Result
sim w = do
  inp <- use _1
  out <- zoom _2 $ state . runState $ runStep inp
  inp' <- zoom _3 $ runWorld w out
  _1 .= inp'
  return $ resultComputedPurelyFrom out

What I'd like to improve on this code is two things:

Reduce the lens piping. I could, of course, get rid of _1, _2 and _3 by using a bespoke record type, but then that introduces its own noise -- sim is the only function that uses this particular combination of I, S and O.

The write-back to inp' could easily be accidentally omitted, resulting in not-obviously-wrong-looking, well-typed code that wouldn't work correctly.

ETA: An alternative formulation is to forego the lens stuff completely,  at the cost of making even the updating of the S component something that can be accidentally omitted:
sim :: (Monad m) => World m -> StateT (I, S) (StateT O m) Result
sim w = do
  (inp, s) <- get
  let (out, s') = runState (runStep inp) s
  inp' <- lift $ runWorld w out
  put (inp', s')
  return $ resultComputedPurelyFrom out

What I'd like to improve on this code is two things:

Reduce the lens piping. I could, of course, get rid of _1, _2 and _3 by using a bespoke record type, but then that introduces its own noise -- sim is the only function that uses this particular combination of I, S and O.

Is this really a problem? I don't understand what could be improved about that part.

The write-back to inp' could easily be accidentally omitted, resulting in not-obviously-wrong-looking, well-typed code that
wouldn't work correctly.

You can move the state-passing of I into a separate function for which that's the only job:
withInput :: ALens' s i -> (i -> StateT s i) -> StateT s ()
withInput l go = do
  i <- use (cloneLens l)
  i' <- go i
  cloneLens l := i'

...
sim = do
  withInput _1 $ \inp ->
    inp' <- ...
    pure inp'
  ...

The result type () of withInput makes it difficult to forget to update the state: the function would fail to typecheck if you forget the last clause.


Monkeypatching builtin open and File Mock-Up for unit testing For a specific test-scenario I wanted:

avoid accessing the file system through pythons builtin open-function
don't want to use 3rd party libraries like Michael Foord's Mock library

I would like to hear your opinion, if this seems like a valuable solution, or if there are

similar solutions with less lines of code
grave errors or caveats in the provided solution

from io import StringIO

class MockFile(StringIO):
    """Wraps StringIO, because of python 2.6: clumsy name-property reset"""

    name = None #overwrite TextIOWrapper-property - part 1/2
    _vfs = {} #virtual File-System enables to "re-read" already written MockFiles

    def __init__(self, name, mode = 'r', buffer_ = None):
        self.name = name #overwrite TextIOWrapper-property - part 2/2
        if self._vfs.has_key(name):
            buffer_ = self._vfs[name]
        super(MockFile, self).__init__(unicode(buffer_))
        __enter__ = lambda self: self
        __exit__ = lambda self, exc_type, exc_value, traceback: None
        read = lambda self, size = None: super(Mockfile, self).read(size)

    def write(self, data):
        super(MockFile, self).write(data)
        self._vfs[self.name] = self.getvalue()

def replace_builtin(builtinname, replacementobj):
    import __builtin__
    builtin_func = getattr(__builtin__, builtinname)
    setattr(__builtin__, builtinname, replacementobj)

    def decorator_factory(func):
        def decorator(*args, **kwarg):
            return func(*args, **kwarg)

        #attach the original name and function to the decorator:
        decorator.replaced_builtin = (builtinname, builtin_func)
        return decorator
    return decorator_factory

from io import StringIO

class MockFile(StringIO):
    """Wraps StringIO, because of python 2.6: clumsy name-property reset"""

    name = None #overwrite TextIOWrapper-property - part 1/2
    _vfs = {} #virtual File-System enables to "re-read" already written MockFiles

Storing files here causes them to be persistent between unit tests.

    def __init__(self, name, mode = 'r', buffer_ = None):
        self.name = name #overwrite TextIOWrapper-property - part 2/2
        if self._vfs.has_key(name):
            buffer_ = self._vfs[name]

I'd use buffer_ = self._vfs.get(name, buffer_)

        super(MockFile, self).__init__(unicode(buffer_))

What _buffer is None at this point? Won't that have strange effects?

        __enter__ = lambda self: self
        __exit__ = lambda self, exc_type, exc_value, traceback: None
        read = lambda self, size = None: super(Mockfile, self).read(size)

None of these lines of code do anything. They assign to local variables which are thrown away at the end of the function.

    def write(self, data):
        super(MockFile, self).write(data)
        self._vfs[self.name] = self.getvalue()

I wouldn't update self._vfs for every write. I'd put that in a close function. That way your tests fail if you forget to invoke close.

def replace_builtin(builtinname, replacementobj):
    import __builtin__
    builtin_func = getattr(__builtin__, builtinname)
    setattr(__builtin__, builtinname, replacementobj)

    def decorator_factory(func):
        def decorator(*args, **kwarg):
            return func(*args, **kwarg)

        #attach the original name and function to the decorator:
        decorator.replaced_builtin = (builtinname, builtin_func)
        return decorator
    return decorator_factory

I'm having real trouble figuring out how you would use this function.

UPDATE:

from io import StringIO

class MockFile(StringIO):
    """Wraps StringIO"""
    _vfs = {} #virtual File-System
    name = None #overwrite TextIOWrapper-property - part 1/2
    def __init__(self, name, mode = 'r', buffer_ = ''):
        self.name = name #overwrite TextIOWrapper-property - part 2/2
        if self._vfs.has_key(name):
            buffer_ = self._vfs.get(name, buffer_)

No need for the if. The second parameter to get is a default. Using get replaces checking for the key.

        super(MockFile, self).__init__(unicode(buffer_))

    def read(self, size = None):
        return super(MockFile, self).read(size)

    def write(self, data):
        super(MockFile, self).write(data)

These functions just call the superclass which happens anyways. So why include them?

    def close(self):
        self._vfs[self.name] = self.getvalue()
        super(MockFile, self).close()

def replace_builtin(builtinname, replacementobj):
    """makes it possible to mock the built-in functions like open.
    >>> @replace_builtin('open', MockFile)
    >>> def test_whatever(self, *args, **kwargs)
            with open(filename, 'r') as file:
                self.content = file_.read()
    """
    import __builtin__
    builtin_func = getattr(__builtin__, builtinname)

    def decorator_factory(func):
        def decorator(*args, **kwarg):
            setattr(__builtin__, builtinname, replacementobj)
            result = func(*args, **kwarg)

Bad things will happen if an exception is thrown by the function. I'd use a try/finally block here.

            setattr(__builtin__, builtinname, builtin_func)
            return result
        #attach the original name and function to the decorator:
        decorator.replaced_builtin = (builtinname, builtin_func)
        return decorator
    return decorator_factory


Code duplication where impliment extension method for IEnumerable and IQueryable on entity set Since Entity Set does not support asqueryable, I have to do this.  How can I reduce the amount of redundant code between the two methods?

public static IQueryable<T> WhereCreateWithin<T>(this IQueryable<T> query, DateManager.TimeRange timeRange, DateTime? beginDayParam = null) where T : IHasCreateDate
{
    if (beginDayParam == null)
    {
        beginDayParam = DateTime.Today;
    }

    DateTime beginDay = beginDayParam.Value.Date;
    DateTime startRange;
    DateTime endRange;

    switch (timeRange)
    {
        case DateManager.TimeRange.All:

            return query;

        case DateManager.TimeRange.ThisYear:

            startRange = new DateTime(beginDay.Year, 1, 1); // January 1, this year
            endRange = startRange.AddYears(1);

            return query.Where(p => p.create_date.Date >= startRange && p.create_date < endRange);

        case DateManager.TimeRange.ThisMonth:

            startRange = new DateTime(beginDay.Year, beginDay.Month, 1);
            endRange = startRange.AddMonths(1).Date;

            return query.Where(p => p.create_date.Date >= startRange && p.create_date < endRange);

        case DateManager.TimeRange.ThisWeek:

            startRange = BuilkHelper.DateHelper.StartOfWeek(beginDay, DayOfWeek.Monday);
            endRange = startRange.AddDays(6);

            return query.Where(p => p.create_date.Date >= startRange && p.create_date < endRange);

        case DateManager.TimeRange.ToDay:

            return query.Where(p => p.create_date.Date == beginDay);

        case DateManager.TimeRange.Last30Day:
            startRange = beginDay.AddDays(-29);
            endRange = beginDay;

            return query.Where(p => p.create_date.Date >= startRange && p.create_date <= endRange);
    }
    return query;
}

public static IEnumerable<T> WhereCreateWithin<T>(this IEnumerable<T> query, DateManager.TimeRange timeRange, DateTime? beginDayParam = null) where T : IHasCreateDate
{
    if (beginDayParam == null)
    {
        beginDayParam = DateTime.Today;
    }

    DateTime beginDay = beginDayParam.Value.Date;
    DateTime startRange;
    DateTime endRange;

    switch (timeRange)
    {
        case DateManager.TimeRange.All:

            return query;

        case DateManager.TimeRange.ThisYear:

            startRange = new DateTime(beginDay.Year, 1, 1); // January 1, this year
            endRange = startRange.AddYears(1);

            return query.Where(p => p.create_date.Date >= startRange && p.create_date < endRange);

        case DateManager.TimeRange.ThisMonth:

            startRange = new DateTime(beginDay.Year, beginDay.Month, 1);
            endRange = startRange.AddMonths(1).Date;

            return query.Where(p => p.create_date.Date >= startRange && p.create_date < endRange);

        case DateManager.TimeRange.ThisWeek:

            startRange = BuilkHelper.DateHelper.StartOfWeek(beginDay, DayOfWeek.Monday);
            endRange = startRange.AddDays(6);

            return query.Where(p => p.create_date.Date >= startRange && p.create_date < endRange);

        case DateManager.TimeRange.ToDay:

            return query.Where(p => p.create_date.Date == beginDay);

        case DateManager.TimeRange.Last30Day:
            startRange = beginDay.AddDays(-29);
            endRange = beginDay;

            return query.Where(p => p.create_date.Date >= startRange && p.create_date <= endRange);
    }
    return query;
}

Given that the logic is exactly the same (i did not do a line by line comparision), one of the possible solutions whould be:

private static TCollection WhereCreateWithin<T, TCollection>(TCollection query,
             DateManager.TimeRange timeRange,
             DateTime? beginDayParam,
             Func<TCollection, Func<T, bool>, TCollection> selector)
    where T : IHasCreateDate
    where TCollection : IEnumerable<T>
{
    //insert your logic here
    //replace query.Where(x => ... ) call with selector(query, x => ... ) call
}

public static public static IQueryable<T> WhereCreateWithin<T>(this IQueryable<T> query, DateManager.TimeRange timeRange, DateTime? beginDayParam = null) where T : IHasCreateDate
{
    return WhereCreateWithin<T, IQueryable<T>>(query, timeRange, beginDayParam, (q, f) => q.Where(x => f(x)))
}

public static public static IEnumerable<T> WhereCreateWithin<T>(this IEnumerable<T> query, DateManager.TimeRange timeRange, DateTime? beginDayParam = null) where T : IHasCreateDate
{
    return WhereCreateWithin<T, IEnumerable<T>>(query, timeRange, beginDayParam, (q, f) => q.Where(f))
}


Smartly joining possibly empty strings I want to join two fragments into one string with a separator, but when one of the fragments is empty I only want one of them.

I have two implementations. The first is long and slightly repetitive, but I believe it is easier to understand, since it is a more step-by-step description of the process. The second is way shorter, but less self-explanatory. It uses the Where method, and it may not be clear to everybody what the semantics of $_ are. Maybe both approaches can even be merged into an even better implementation.

Which would you recommend?

Version 1:

if ($Fragment1.Length -ne 0 -and $Fragment2.Length -ne 0) {
    $String = $Fragment2 + $Seperator + $Fragment2
} elseif ($Fragment1.Length -ne 0) {
    $String = $Fragment1
} elseif ($Fragment2.Length -ne 0) {
    $String = $Fragment2
} else {
    $String = $null
}

Version 2:

$String =
    @($Fragment2, $Fragment1).Where({$_.Length -ne 0}) -join $Seperator

On PowerShell v6+, I would do the following:
$String = $Fragment1, $Fragment2 |
    Where-Object { -not [System.String]::IsNullOrWhiteSpace($_) } |
    Join-String -Separator $Separator

You could use [System.String]::IsNullOrEmpty() instead of [System.String]::IsNullOrWhiteSpace(), but I find [System.String]::IsNullOrWhiteSpace() to be the desired functionality in almost all cases.
On Windows PowerShell (versions prior to v6), the Join-String command doesn't exist. There I would use:
$String = ($Fragment1, $Fragment2 |
    Where-Object { -not [System.String]::IsNullOrWhiteSpace($_) }) -join $Separator

The @().Where({ ... }) method is fairly obscure, so I tend to avoid it.

it may not be clear to everybody what the semantics of $_ are.

I'm not sure it's valuable to consider this possibility. Simply put, if your reader doesn't understand the semantics of $_, then they cannot read PowerShell. It's a ubiquitous and pervasive variable both in practice and in design.


Initialize a gitlab project both locally and remotely with a single command This shell script, with only a few configurations, has the goal of initializing a project on Gitlab.com without having to go through the go through the website's ui.
This means: Let's say you just a project with a few source files, but you haven't done anything with git yet. The goal of this script is to allow you to write something like git-init-remote <my_repository_name> on the command line and have both the project on your personal gitlab.com and the local git repository.
#!/bin/sh

repo=$1
token=<YOUR-PRIVATE-TOKEN> # You get this from https://gitlab.com/-/profile/personal_access_tokens
username=<YOUR-USERNAME>

test -z $repo && echo "Repo name required." 1>&2 && exit 1

curl --request POST --header "Private-Token: $token" --header "Content-Type: application/json" --data "{\"name\":\"$repo\"}" "https://gitlab.com/api/v4/projects"

echo "\nDone initializing on remote."
echo "\nCreating local git repository..."

git init
git remote add origin https://gitlab.com/$username/$repo.git
git add .
git commit -m "Initial commit"
git push -u origin master

echo "\nDone."

I would like to have any of your thoughts on how this can break and how scalable this can go. I had trouble finding similar scripts on the web so I felt like a had to write here.

Welcome to CodeReview.SE and thanks for submitting your script for a code review!
Good points

Good output conveying how progress is being made.
Clear variable names

Suggestions

Are there limitations for what characters can be in a repo name?  If so, you might want to check that before sending it to the API.  It is good that you're checking to make sure they passed something in.
Aside from making sure $1 is set, there is no error checking.  If the curl fails do you want to continue with the git commands?
It isn't clear if you want this to run in bash or POSIX sh.  The question is tagged for bash, but your sh-bang line points at POSIX shell.  But yay for including the sh-bang line.
One shell best practice is putting your variable substitutions in double quotes.  git remote add origin "https://gitlab.com/$username/$repo.git" shows how to change one of your lines of code.  You want the variables inside of double quotes so that spaces in a variable name don't cause the shell to split what you think is one argument into multiple arguments to the command.

Ideas

Do you really want the token stored in the source code?  Can you pull this from a secrets vault or store it in a separate file?  If you want to commit your source code to a code repository (like git) it would be much better if it didn't include secret tokens.
I'd try seeing if I could get this to work as a git subcommand.  Here is a medium article on writing your own git subcommands.
Use shellcheck to get automated best practice suggestions for your shell code.  It can also be run locally on the command line.


Given a string, return n distinct characters with their respective counts, as well as the remainder of the string Going through some old code, I found this function.

Given a string carrier and integer nsyms, it consumes nsyms distinct characters from the carrier string and returns a dictionary containing consumed characters and their respective counts, and the remainder of the carrier string.

def enc_consume(carrier, nsyms):
  chrs = defaultdict(int)

  i = 0  # see this?

  for i,c in enumerate(carrier):
    # `i` is not used in the loop
    chrs[c] += 1
    if len(chrs) == nsyms:
      break

  return chrs, carrier[i+1:]  # but it's used here

How would you rewrite this? I found the assignment i = 0 confusing, since it was followed by a for i..., which will of course do the assignment as well.

EDIT: The point of the explicit assignment i = 0 is that if carrier is empty, the loop will not execute. If it weren't for the assignment, i would be undefined by the time it is used in return.

I would avoid using indexes altogether and rely on iter to advance character by character and remember what's left in the string:

def enc_consume(carrier, nsyms):
    characters = defaultdict(int)
    stream = iter(carrier)

    for char in stream:
        characters[char] += 1
        if len(characters) >= nsyms:
             break

    return characters, ''.join(stream)