Persistent Object using localStorage The goal is to

make an object that persists upon page reloads,
have an interface as close as possible to the Object class.

function PersistentObject(key,initial_value)
//use for(..in data.keys()) instead of for(..in data)
{
  var ret;
  if(localStorage[key]) ret=JSON.parse(localStorage[key]);
  if(!ret) ret = initial_value;
  if(!ret) ret = {};
  //JSON.stringify ignores put and keys functions
  ret.put=function() { localStorage[key]=JSON.stringify(this) };
  ret.keys=function()
  {
    var res = {};
    for(var prop in this) if(this[prop] !== this.put && this[prop] !== this.keys)
      res[prop]=true;
    return res;
  };
  return ret;
}

The code is used as follows:

var myPersistentObject = new PersistentObject('myPersistentObject');
var myPersistentArray  = new PersistentObject('myPersistentArray',[]);

Unfortunately there is AFAIK no way to get the name of the variable and therefore the name of the variable needs to be passed as first argument.

Also unfortunate is that the function keys needs to be called in order to iterate over the object. I'm aware of the defineProperty function that allow to add a propoerty that is not enumerable but AFAIK cross browser compability is an issue with defineProperty.

It is impossible to know the variable name without passing it separately because the name is not part of the object itself, but only one (of potentially many) signs pointing to that object. Expecting the object to know that variable name is like expecting a building to know which road you drove in on. Besides, specifying the localStorage key separately is a good thing - it allows you to refer to the same key using different names in different parts of the program.

You can get around having your methods show up in for-in loops by using getters instead of properties or prototypes. If you're concerned about compatibility, you can fallback to __defineGetter__.

Here's what I came up with.

function localObject(key) {
    var _this = JSON.parse(localStorage[key] || '{}');
    Object.defineProperty(_this, 'save', {
        get: function () {
            return function () {
                localStorage[key] = JSON.stringify(this);
            };
        }
    });
    return _this;
}

The new is optional - you can write both

var scores = new localObject('highscores'); // and
var scores = localObject('highscores');`.

While the above solution works, it is somewhat inefficient and messy. This stems from the fact that you're trying to get javascript to work the way you want, when what you should be doing is trying to understand how javascript wants to work. Each language has a style, even a personality, and if you're flexible enough to go with it then your code will be shorter, faster, and cleaner. Here's how I would abstract away my JSONification.

function local(key, value) {
    if (value === undefined) {
        return key in localStorage ? JSON.parse(localStorage[key]) : undefined;
    } else {
        localStorage[key] = JSON.stringify(value);
        return value;
    }
}

Now we can write code like this:

var score = local('highscore');
// use score like a normal variable, because it is one. then, eventually:
local('highscore', score);


High execution time of LCS length program in Python2 I was trying to solve the Longest Common Subsequence problem on a HackerRank exercise.
It's quite simple and straightforward, just print out the length of the LCS. I submitted this code:

s1 = raw_input()
s2 = raw_input()

lcs = [[0 for j in xrange(len(s2)+1)] for i in xrange(len(s1)+1)]
for i, x in enumerate(s1):
    for j, y in enumerate(s2):
        if x == y:
            lcs[i+1][j+1] = lcs[i][j] + 1
        else:
            lcs[i+1][j+1] = max(lcs[i+1][j], lcs[i][j+1])

print lcs[-1][-1]

I think the logic is fine. The maximum string length is 5000. The problem is with the execution time. Some test cases take more than 10 seconds and the program terminates.

That looks like a classic LCS solution to me.

If all you want is to read the bottom-right element of the tableau, you don't actually have to store the entire matrix: all you need to keep is the most recent row.  That could save you a few megabytes of memory — I don't know how much processing time that would save, though.

To initialize the arrays, you can use the * operator instead of a list comprehension.  The indexing would be a bit less awkward, in my opinion, if you enumerated starting from 1.  I suggest c1 and c2 for characters out of s1 and s2, respectively, but that's a matter of personal taste.

def lcs_len(s1, s2):
    prev_lcs = [0] * (len(s2) + 1)
    curr_lcs = [0] * (len(s2) + 1)
    for c1 in s1:
        curr_lcs, prev_lcs = prev_lcs, curr_lcs
        for j, c2 in enumerate(s2, 1):
            curr_lcs[j] = (
                1 + prev_lcs[j - 1] if c1 == c2 else
                max(prev_lcs[j], curr_lcs[j - 1])
            )
    return curr_lcs[-1]


Excel VBE code find and replace I need to write a VBA code routine with an efficient way to replace blank cells with values from another cell. For example, my worksheet has some blank cells on random rows in column D. I want to replace these with the value on the same row in column A. In other words, go down all the rows and if column D is blank, replace it with column A.

I wrote some code that "walks" down every row of the worksheet looking for blanks in column D and copying the values from A into D, but it's ridiculously slow (some of these worksheets have more than half a million rows!)

 .Range("D1", Range("D1048576").End(xlUp)).Select
            Set A = XL.Selection
                For Each B In A.Rows
                        If Len(B.Value & vbNullString) = 0 Then
                            B.FormulaR1C1 = "=RC[-3]"
                        End If
                    DoEvents
                Next

Any suggestions?

try this code. It assumes that the worksheet with data is active.

Sub ReplaceCodeReview235023()
    With ActiveSheet.Columns("D")
       .SpecialCells(xlCellTypeBlanks).FormulaR1C1 = "=RC[-3]"
    End With
End Sub


Genotype as input, give the phenotype I want to write a Python script where the input is a genotype (e.g. "CCDd") based upon the following Punnett square. The output should be the phenotype (e.g. "seal").

My code works, but doesn't look that 'pythonic' at all.

def colour(input):

    if len(set(input)) == 2 and input[0] != 'C' and input[2] != 'D':
        return('lilac')
    elif len(set(input[2::])) == 1 and input[2] != 'D':
        return('blue')
    elif len(set(input[0:2])) == 1 and input[0] != 'C':
        return('chocolate')
    else:
        return('seal')

You correctly identified that you need to count the number of lowercase c and d. But using a set to do so is not the right tool. Instead, I suggest using collections.Counter:

from collections import Counter

def color(genotype):
    assert len(genotype) == 4

    count = Counter(genotype)
    blue = count['d'] == 2
    chocolate = count['c'] == 2

    if blue and chocolate:
        return 'lilac'
    if blue:
        return 'blue'
    if chocolate:
        return 'chocolate'
    return 'seal'

I also added a debug check to ensure that the length of the input sequence is always four.

Given the mini debate that occured in the comments, pertaining to variable names, you may consider this slight variation:

from collections import Counter

def color(genotype):
    assert len(genotype) == 4

    count = Counter(genotype)
    recessive_traits = [count[gene] == 2 for gene in 'cd']

    if all(recessive_traits):
        return 'lilac'
    if recessive_traits[0]:
        return 'blue'
    if recessive_traits[1]:
        return 'chocolate'
    return 'seal'

It may be more expressive as to what you're testing, and why.


Is returning promise form angular service bad? What is the problem when I return a promise from angular service instead of Observable?
If it's a matter of any manipulation or side-effect, that I can easily do inside angular service itself. Then why I shouldn't return a promise over observable?

Here's my code for service class

public async getCountriesAsync(filter: PaginateFilter): Promise<Paginate<IdNameTypeModel>> {
return await this.http.get<Paginate<IdNameTypeModel>>(`/api/{version}/Types/{lang}/Countries${filter.url()}`)
  .pipe(map(response => {
    return response == null ? new Paginate<IdNameTypeModel>() : response;
  })).toPromise();
}

Using inside component

async loadCountries() {
  const filter = new PaginateFilter(this.data.pageIndex, this.data.pageSize);
  this.countries = await this.dldTypesService.getCountriesAsync(filter);
}

Per se returning a promise from an Angular Service is not bad, however there are numerous things in Angular world that are heavily based on Observables:

usually you will likely change the change detection to ChangeDetectionStrategy.OnPush in your components and this will most likely be easier with Angular's async pipe not a problem, because async also supports Promises.
if you use your services for any kind of state and if your are using NgRx for effectively managing it, you'll be using @Effectsthat are basically a lot of RxJs magic and thus it might be easier to use Observables in the first place.
composing services for Guards is easier with Observables

Having that said, I agree with you, that typical REST services are fire and forget and thus IMO do not fit really into Observables well which are meant to be constant sources of events.

It is all about composition, Observables allow better composition for multiple async operations that need to be composed, whereas Promises can be nowadays used pretty easy with async/await for singular, isolated operations.

In the end, I'd advise to stick with the same convention throughout your whole app - either have all services return Promises and convert them to Observables when needed or always use Observables. I assume a lot of examples are using Observables because for non-trivial apps you end up needing better composition options than with pure Promises.


Refactoring complex phone validations I have a complex phone validation that does the following:

First, check if either a home phone, mobile phone or work phone is provided (there will be 3 text fields in form and at least one type of phone number is required).
Strip out any characters from the phone number that is not a digit except 'x' and '+', since those represent extensions and are valid.
Make sure that the phones that were provided actually pass the phone number regex test.

And this is how I implemented the 3 steps above:

  VALID_PHONE_FORMAT = /\A(?:(?:\+?1\s*(?:[.-]\s*)?)?(?:\(\s*([2-9]1[02-9]|[2-9][02-8]1|[2-9][02-8][02-9])\s*\)|([2-9]1[02-9]|[2-9][02-8]1|[2-9][02-8][02-9]))\s*(?:[.-]\s*)?)?([2-9]1[02-9]|[2-9][02-9]1|[2-9][02-9]{2})\s*(?:[.-]\s*)?([0-9]{4})(?:\s*(?:#|x\.?|ext\.?|extension)\s*(\d+))?\z/

  validate :phone_provided
  before_validation :clean_phone_numbers
  validates_format_of :phone_home, with: VALID_PHONE_FORMAT, if: :home_is_filled?
  validates_format_of :phone_mobile, with: VALID_PHONE_FORMAT, if: :mobile_is_filled?
  validates_format_of :phone_work, with: VALID_PHONE_FORMAT, if: :work_is_filled?
  validates_format_of :phone_fax, with: VALID_PHONE_FORMAT, if: :fax_is_filled?

 private

  def clean_phone_numbers
    if phone_home.present?
      self[:phone_home] = strip_bad_characters :phone_home
    end
    if phone_mobile.present?
      self[:phone_mobile] = strip_bad_characters :phone_mobile
    end
    if phone_work.present?
      self[:phone_work] = strip_bad_characters :phone_work
    end
    if phone_fax.present?
      self[:phone_fax] = strip_bad_characters :phone_fax
    end
  end

  def strip_bad_characters(attr)
    send("#{attr}_before_type_cast").gsub(/[^\d+!x]/,'')
  end

  def phone_provided
    if phone_home.blank? && phone_mobile.blank? && phone_work.blank?
      errors.add(:base, "Must provide a phone number")
    end
  end

  def home_is_filled?
    !phone_home.blank?
  end

  def mobile_is_filled?
    !phone_mobile.blank?
  end

  def work_is_filled?
    !phone_work.blank?
  end

  def fax_is_filled?
    !phone_fax.blank?
  end

I particularly don't like how I am calling 4 different methods in the if condition for validates_format_of. I would like to only use one method and be able to tell which attribute to call blank? on. How can I refactor this?

Let's summarize the business rules:

Characters other than digits, !, or x are stripped.
If any of the four types of phone numbers is provided, it must be valid.
At least one of { home, mobile, work } must be provided.

Requirement 3 is taken care of by validate :phone_provided, which is simple and fine.

The dissatisfaction comes from implementing Requirements 1 and 2.

The implementation of Requirement 1 can be simplified by making strip_bad_characters() tolerant of nil.  You can also call #read_attribute_before_type_cast() to avoid interpolation.

def strip_bad_characters(attr)
  input = read_attribute_before_type_cast(attr)
  input.gsub(/[^\d+!x]/, '') if input
end

Also, clean_phone_numbers() can use a loop:

def clean_phone_numbers
  [:phone_home, :phone_mobile, :phone_work, :phone_fax].each do |attr|
    self[attr] = strip_bad_characters(attr)
  end
end

The solution for Requirement 2 is simple: wrap the entire regular expression in \A( )?\Z to make it optional.  Then, you can get rid of all the …_is_filled? helpers.


Refactoring node.js database code I have a user model.

It basically forwards get/create/delete requests into my database client.

The problem I have is the error handling, it screams not DRY to me, but I can't find an elegant way to clean it up.

var UserModel = pd.make(Model, {
    get: function _get(id, cb) {
        // nano is a database client
        this.nano.get(id,
            // whitelist runs every error through the white list,
            // if it fails (false) throw it
            // if it succeeds (true) pass it to cb
            // if the return value is undefined it does nothing
            error.whitelist(function _errors(err) {
                // getaddrinfo is a random DNS error I have to swallow
                // no clue where the bug comes from, trying again is the easy fix
                if (err.syscall === 'getaddrinfo') {
                    UserModel.get(id, cb);
                } else if (err.error === "not_found") {
                    return true;
                } else {
                    return false;
                }
            }, cb)
        );
    },
    insert: function _create(json, cb) {
        this.nano.insert(json, json._id,
            error.whitelist(function _errors(err) {
                if (err.syscall === 'getaddrinfo') {
                    UserModel.insert(json, cb);
                } else if (err.error === "conflict") {
                    return true;
                } else {
                    return false;
                }
            }, cb)
        );
    },
    delete: function _delete(name, cb) {
        var that = this;
        this.get(name, function _getRev(err, body) {
            that.nano.destroy(name, body._rev,
                error.whitelist(function _errors(err) {
                    if (err.syscall === 'getaddrinfo') {
                        UserModel.delete(name, cb);
                    } else {
                        return false;
                    }
                }, cb)
            );
        });
    }
});

pd, error, nano

Based on feedback from JavaScript chat. Credit to @Zirak and @Chris

You want a hash of white listed error messages for each method

var whitelistMap = {
    "get": ["not_found"],
    "insert": ["conflict"],
    "delete": []
}

You want a whitelist error handler factory. This will include the "getaddrinfo" code for every method which is basically a try again short cutting.

It will check whether the error message is in the whitelistMap for that method and if so will let that error through.

otherwise it simply throws

function makeWhitelistCallback(method, thing, cb) {
    return error.whitelist(function _errors(err) {
        if (err.syscall === "getaddrinfo") {
            UserModel[method](thing, cb);
        } else if (whitelistMap[method].indexOf(err.error) !== -1) {
            return true;
        } else {
            return false;
        }
    }, cb);
}

And the rest of the code simply calls the whitelist callback factory

var UserModel = pd.make(Model,{
    get: function _get(id, cb) {
        this.nano.get(id,
            makeWhitelistCallback("get", id, cb)
        );
    },
    insert: function _create(json, cb) {
        this.nano.insert(json, json._id,
            makeWhitelistCallback("insert", json, cb)
        );
    },
    delete: function _delete(name, cb) {
        var that = this;
        this.get(name, function _getRev(err, body) {
            that.nano.destroy(name, body._rev,
                makeWhitelistCallback("delete", name, cb)
            );
        });
    }
});


Print all possible sequences that could have created this BST I was working through Cracking The Coding interview, and I couldn't wrap my brain around their solution to one of the problems, so I ended up coming up with my own that, IMHO, is a lot cleaner and easier to read. Would love your feedback. Here is the actual problem:

"A binary search tree was created by traversing through an array from left to right and inserting each element. Given a binary search tree with distinct elements, print all of the arrays that could have led to this tree."

In other words, what are all of the possible sequences of values that could have led to this tree.

For reference, here is a link to the 'official' solution.

And here is what I came up with

public class AllSequences {
public LinkedList<LinkedList<TreeNode>> allSeqs;

public AllSequences(TreeNode root) {
    allSeqs = new LinkedList<>();
    LinkedList<TreeNode> pre = new LinkedList<>();
    LinkedList<TreeNode> poss = new LinkedList<>();

    buildSeqs(pre, poss, root);
    for (LinkedList<TreeNode> ll : allSeqs) {
        for (TreeNode n : ll) {
            System.out.print(n.data + " ");
        }
        System.out.println();
    }
}

private void buildSeqs(LinkedList<TreeNode> pre, LinkedList<TreeNode> poss, TreeNode root) {
    pre.add(root);
    if (root.left != null) {
        poss.add(root.left);
    }
    if (root.right != null) {
        poss.add(root.right);
    }

    if (poss.isEmpty()) {
        allSeqs.add(pre);
    }

    for (TreeNode n: poss) {
        LinkedList<TreeNode> temp = (LinkedList<TreeNode>) pre.clone();
        LinkedList<TreeNode> ptemp = (LinkedList<TreeNode>) poss.clone();
        ptemp.remove(n);
        buildSeqs(temp, ptemp, n);
    }
}

public static void main(String[] args) {
    TreeNode root = new TreeNode(10);
    root.left = new TreeNode(5);
    root.right = new TreeNode(15);
    root.left.right = new TreeNode(7);
    root.left.left = new TreeNode(3);

    AllSequences all = new AllSequences(root);
}
}

Granted mine ends up with a set of LinkedLists of possible input sequences, not arrays, but it would be easy enough to convert that if the interviewer was a stickler about it. It seems to correctly print out for all of my hand-coded, relatively small test cases.

public class Solution {
    ArrayList<LinkedList<Long>> result;
    /*Return the children of a node */
    ArrayList<TreeNode> getChilden(TreeNode parent) {
        ArrayList<TreeNode> child = new ArrayList<TreeNode>();
        if(parent.left != null) child.add(parent.left);
        if(parent.right != null) child.add(parent.right);
        return child;
    }
    /*Gets all the possible Compinations*/
    void getPermutations(ArrayList<TreeNode> permutations, LinkedList<Long> current) {
        if(permutations.size() == 0) {
            result.add(current);
            return;
        }
        int length = permutations.size();
        for(int i = 0; i < length; i++) {
            TreeNode node = permutations.get(i);
            permutations.remove(i);
            ArrayList<TreeNode> newPossibilities = new ArrayList<TreeNode>();
            newPossibilities.addAll(permutations);
            newPossibilities.addAll(getChilden(node));
            LinkedList<Long> newCur = new LinkedList<Long>();
            newCur.addAll(current);
            newCur.add(node.val);
            getPermutations(newPossibilities, newCur);
            permutations.add(i,node);
        }
    }

    /*This method returns a array of arrays which will lead to a given BST*/
    ArrayList<LinkedList<Long>> inputSequencesForBst(TreeNode node) {
        result = new ArrayList<LinkedList<Long>>();
        if(node == null)
            return result;
        ArrayList<TreeNode> permutations = getChilden(node);
        LinkedList<Long> current = new LinkedList<Long>();
        current.add(node.val);
        getPermutations(permutations, current);
        return result;
    }
}

My solution. Works perfectly.


Front End Web exercise, no HTML I was given the following instructions

  Using the below HTML, using ONLY CSS, JavaScript or jQuery create the following (see attached gif animation):

This is a cube

This is a triangle

This is a big, green circle

  Rules:

  The HTML cannot be modified in any way
  For the animation portion, please use JavaScript;  however, no 3-rd party jQuery or JavaScript plugins allowed
  Compatibility: IE 9+, FF 4+, Chrome 10+, Safari 5.1+

I fudged the HTML some, especially when it came to making the Circle's text wrap to a second line and stay in the middle of the Circle.

$(document).ready(function() {
 downUp();
 upDown();
 spin();
});

function downUp() {
 $('.downUp').animate({'top': 500}, {
  duration: 1500,
  complete: function() {
   $('.downUp').animate({'top': 0}, {
    duration: 1500,
    complete: downUp
   });
  }
 });
}

function upDown() {
 $('.upDown').animate({'top': 0}, {
  duration: 1500,
  complete: function() {
   $('.upDown').animate({'top': 500}, {
    duration: 1500,
    complete: upDown
   });
  }
 });
}

function spin() {

 var degree = 0, timer;
 rotate();
 function rotate() {
 timer = setTimeout(function() {
  $('.spin').css({ WebkitTransform: 'rotate(' + degree + 'deg)'});
  $('.spin').css({ '-moz-transform': 'rotate(' + degree + 'deg)'});
  $('.spin').css({ '-ms-transform' : 'rotate(' + degree + 'deg)'});
  ++degree; rotate();
  },5);
 }
}
#square {
  position: absolute;
  height: 200px;
  width: 200px;
  background: black;
  /* For browsers that do not support gradients */
  background: -webkit-linear-gradient(black, gray, black);
  /* For Safari 5.1 to 6.0 */
  background: -moz-linear-gradient(black, gray, black);
  /* For Firefox 3.6 to 15 */
  background: linear-gradient(black, gray, black);
  text-align: center;
  line-height: 200px;
  color: white;
}
#triangle {
  position: absolute;
  left: 250px;
  top: 500px;
  width: 0px;
  height: 50px;
  border-style: solid;
  border-width: 0 100px 160px 100px;
  border-color: transparent transparent red transparent;
  white-space: nowrap;
  box-sizing: border-box;
  text-indent: -50px;
  line-height: 250px;
  color: white;
}
#circle {
  position: relative;
  top: 250px;
  left: 500px;
  height: 200px;
  width: 200px;
  background-color: green;
  border-radius: 100px;
  -moz-border-radius: 100px;
  -webkit-border-radius: 100px;
  text-align: center;
  color: white;
  display: table-cell;
  vertical-align: middle;
}

This is a square

This is a triangle

This is a big,

green circle

JSFiddle

I borrowed some code from a StackOverflow answer to make the circle spin, it can be found here

I am sure that the up down code can be found places on the internet as it is super simple.

I am sure that there is something that I could be doing better on the JavaScript side of things within the restrictions I have been given.

There isn't much (that I see) to say about this code; it is mostly straightforward and easy to understand. Therefore, my review will be fairly short.

Refactoring

Take a look at your upDown and downUp functions. These are very similar. The only differences are:

function downUp() {
    $('.downUp').animate({'top': 500}, {        <-- x2
        duration: 1500,
        complete: function() {
            $('.downUp').animate({'top': 0}, {  <--
                duration: 1500,
                complete: downUp                <--
            });
        }
    });
}

To simplify your code, you can combine these two functions and simply add an extra parameter: reversed.

function downUp(reversed) {
    var elements = $(reversed ? ".upDown" : ".downUp");
    var to = reversed ? 0 : 500; // the 'top' value to go *to*
    var back = reversed ? 500 : 0; // the 'top' value to go back to

    elements.animate({'top': to}, {
        duration: 1500,
        complete: function() {
            elements.animate({'top': back}, {
                duration: 1500,
                complete: function() { downUp(reversed) }
            });
        }
    });
}

A few other changes I made:
- Stored the $('...') elements in a variable (faster)
- Also store the 'top' values in variables (cleaner)

Now, your code is simpler since you don't have two functions that look almost the same, yet are doing opposite tasks. Now, in the $(document).ready(), you have:

downUp();
downUp(true);
spin();

Taking spin for a spin

Your spin function looks good for the most part. As I am not that familiar with jQuery, I don't know any other way to spin an element other than to use CSS animations (like you did).

I've made some minor changes to this function:

function spin() {
    var degree = 0, elements = $('.spin');
    rotate();
    function rotate() {
        setTimeout(function() {
            elements.css({ WebkitTransform: 'rotate(' + degree + 'deg)'});
            elements.css({ '-moz-transform': 'rotate(' + degree + 'deg)'});
            elements.css({ '-ms-transform' : 'rotate(' + degree + 'deg)'});
            elements.css({ 'transform'     : 'rotate(' + degree + 'deg)'});

            ++degree;
            rotate();
        },5);
    }
}

Here are some things that I changed:

Fixed indentation (cleaner).
Stored $('.spin') in a variable (faster). Note that DOM interaction functions like these where you are grabbing elements can be costly (especially the $() function because not only does it try to parse potentially complex parameters, it also attempts to wrap all the return elements).
Removed the timer variable since it is not needed (you don't use it at all).

Also, about the CSS animation properties that you are adding/changing, I believe that there are quite a few more than the ones you have listed. For one, I know that there is just a plain 'transform' property (which I added above).

I don't know how necessary these other transform types are (type as in webkit, moz, etc), but I think it would be best to find a list of them and to include them to make sure that your code is as compatible as possible with other browsers.


Insertion sort in Scheme Inspired by my merge sort in Scheme, I thought I'd try my hand at implementing other sorting algorithms, starting with simple ones (like insertion sort) and working my way up. So, here we go:

(define (insertion-sort lst (lt? <))
  (fold-right (lambda (item sorted)
                (let-values (((lhs rhs) (span! (cut lt? <> item) sorted)))
                  (append! lhs (cons item rhs))))
              '() lst))

Tested with Racket and Guile; requires SRFIs 1, 11, and 26. (If you want to use this for Guile, you will need to adjust the syntax used for optional arguments.)

What I'm looking for

I'd like to get critique on my code for better style (from the perspective of a seasoned Schemer) and performance, with the understanding that:

Insertion sort is O(n²) worst case, and there are obviously more scalable sorting algorithms around.
I'm trying to make this a pure-functional implementation, so that you can pass in a literal list datum without invoking undefined behaviour.

All the nodes in the sorted list are ultimately created using cons, so I can safely use span! and append! on sorted nodes without undefined behaviour.

Things I've considered

I originally implemented this using a left-fold instead of a right-fold. However, SRFI 1 explains that it's usually better to use right-folding when the left-folding version requires reversing afterwards due to better cache locality.

A left-folding implementation requires building the sorted list in reverse order in order to maintain the "fast if mostly sorted" property.

Update: coredump asked if the output list could share cons cells with the input list where possible. Here's one possible implementation; the code is somewhat more longwinded than the original implementation, so any suggestions for improvement are welcome here too:

(define (insertion-sort lst (lt? <))
  (pair-fold-right (lambda (pair sorted)
                     (define item (car pair))
                     (if (and (eq? (cdr pair) sorted)
                              (or (null? sorted)
                                  (not (lt? (car sorted) item))))
                         pair
                         (let-values (((lhs rhs) (span (cut lt? <> item) sorted)))
                           (append! lhs (cons item rhs)))))
                   '() lst))

I am not a seasoned Schemer and I think your code looks great as-is. I have only one remark.

You are sorting the list in a purely functional way in order to avoid mutating the list given in input. But I am not sure about what is your intent w.r.t. data-sharing, which is one of the strong point of purely functional data-structures (and maybe you don't want to share cons-cells, which I can understand too).

Currently, it appears to me that you can't have any sublist shared. You do    have an opportunity to share sublists between your intermediate lists, thanks to span! and append!. But if your input list is (6 5 7 8 9) and the sort <, the resulting list (5 6 7 8 9) has no way to share the same  sublist, namely (7 8 9), because you build the result from a fresh list () using cons (apologies for any misunderstanding).

A possible improvement for your implementation is to start finding the longest sublist being already sorted and build the resulting list on top of this sublist. I don't consider this to be in contradiction with the definition of insertion sort. The downside is that mutations are shared too, but this is a design decision (and this is where I failed to see if you are avoiding sharing data on purpose or not).


Email notifications system in Rails project I'm working on the Rails project where I have many-to-many :through relationship.  Each setting has many notifications through email_notification model where the notification can be primary or a secondary.

schema.rb

create_table "email_notifications", :force => true do |t|
  t.integer "setting_id"
  t.integer "notification_id"
  t.boolean "primary"
  t.boolean "secondary"

create_table "notifications", :force => true do |t|
  t.string   "name"

create_table "settings", :force => true do |t|
  t.string   "truck_identification"
  t.string   "email_periods"
  t.integer  "email_recepient_id"
  t.datetime "created_at",                             :null => false
  t.datetime "updated_at",                             :null => false
  t.integer  "truck_fleet_id"

In models

class EmailNotification < ActiveRecord::Base
  attr_accessible :notification_id, :setting_id, :primary, :secondary
  belongs_to :notification
  belongs_to :setting

class Notification < ActiveRecord::Base
  attr_accessible :name

  has_many :settings, :through => :email_notifications
  has_many :email_notifications
end

class Setting < ActiveRecord::Base
  # TODO: refactor this bullshit
  attr_accessible :email_periods, :email_recepient_id, :truck_identification,
                  :truck_fleet_id

  has_many :notifications, :through => :email_notifications
  has_many :email_notifications
  validates_uniqueness_of :truck_fleet_id
  # validates :email, :email_format => true
end

I used this many-to-many relationship because I need flexibility to add up new Notifications over the time, and keep ability to make notifications flexible as possible.  For example, I might add a column frequency, or interval etc.

Let me know if you can find a better solution.

# TODO: refactor this bullshit

While amusing to devs, swear words probably don't belong in production code.

In your copypasta of schema.rb, I do believe you forgot to paste the end keywords.

As a side note, you can also write null: false instead of :null => false; as far as I can tell, this is the accepted syntax. You can do the same thing with through: :email_notifications and anything of the form Symbol => BasicObject. This syntax was added after this question was posted, but if you're still interested in updating it, that's what I'd do.

Personally, I prefer to use Symbols instead of Strings as table names, though that's entirely personal preference.

In your models, you also forgot an end at the end of your first class.

Aside from that... Well, honestly, it looks good to me. Since I can't really tell what Setting and Notification are supposed to represent on a more concrete level than their names alone can give, I can't give good advice on how to link them together. However, I don't see why you need a many-to-many, if I'm understanding the purpose of Setting and Notification correctly. It seems like it could be a one-to-many relationship, though I'm not sure which side should be the one.


Using best practice with complex iteration I'm new to Rust so I'm still learning how to use best practice in complex examples.

Below is some code I've used in one of my programs, a simple implementation of a standard neural network.

Here is a the file I'm running: https://pastebin.com/p9ntgWPK

Here is the code snippet I'm particularly interested in (lines 60-72 in the pastebin):

    for x in 0..self.neuron_layers.len() - 1 {
        for y in 0..self.neuron_layers[x+1].len() {
            let mut sum:f64 = 0f64;
            for z in 0..self.neuron_layers[x].len() {
                sum += self.neuron_layers[x][z] * self.connection_layers[x][y][z];
            }
            self.neuron_layers[x+1][y] = sigmoid(sum);
        }
    }

To me this code stinks of inefficiency and bad practice, I just have a gut feeling.

So I wanted to ask 2 things.

How can I improve?
What elements within it are generally considered bad practice?

Honestly, that really doesn't look that bad to me.  It's probably what I would come out with after a first go - not that I'm some sort of Rust guru.  That being said, I'll discuss below a couple of things that you might like better.  It'd also be worth running Clippy over the program and having a look at what it says.  Not everyone likes everything Clippy says, but it can be quite informative.

As earlier commenters have said, it would be worth looking into using a linear algebra library, since this appears to be a matrix multiplication and that would probably end up being more efficient, and easier to use or manage.  All that being said, let's examine your program.

The first thing that jumps out at me is that you are doing a floating-point multiplication followed by an addition for every instance of the inner loop.  It so happens that Rust comes with something for that built-in these days.  You could update your inner-most loop to use this:

for x in 0..self.neuron_layers.len() - 1 {
        for y in 0..self.neuron_layers[x+1].len() {
            let mut sum:f64 = 0f64;
            for z in 0..self.neuron_layers[x].len() {
                sum = self.neuron_layers[x][z].mul_add(self.connection_layers[x][y][z], sum);
            }
            self.neuron_layers[x+1][y] = sigmoid(sum);
        }
    }

The two main advantages of this are that it should be less susceptible to floating-point imprecisions, meaning that you actually come out with a 'more accurate' result at the end, and that on certain hardware (actually most processors these days, I think) this should be a faster operation as it uses fused-multiply-add instructions built into said hardware to do both parts in one go.

Secondly, since you seem to be using the z index just to iterate over every entry in neuron_layers[x], you could actually swap over to using map, as per something like the below:

for x in 0..self.neuron_layers.len() - 1 {
    for y in 0..self.neuron_layers[x+1].len() {
        let sum = self.neuron_layers[x].iter().enumerate()
            .map(|(z, nl)| nl * self.connection_layers[x][y][z]).sum();
        self.neuron_layers[x+1][y] = sigmoid(sum);
    }
}

(I didn't have a chance to test this out, so it might need some tweaking to make the compiler totally happy).  In fact, I think you could expand this approach for every element in the first dimension of neuron_layers if you wanted to.  A potential benefit of this would be that Rayon should be quite good for targeting that level of the loop for parallelism.  Perhaps you could use this approach at the top level of the loop, and the mul_add approach in the inner loop.  You'd have to do benchmarks to figure out for sure what is fastest/most memory-efficient though.

Finally, having a quick look at your Pastebin link, I could see a couple of things worth mentioning.  Firstly, it looks like in at least one instance you create a Vec with length 1, but immediately push a known number of other elements onto it.  You should consider using Vec::with_capacity() to create this, as it should mean that you get the exact right amount of memory allocated, to begin with, which should be both faster and more efficient.

Secondly, I think with

for x in 0..input.len() {
            self.neuron_layers[0][x] = input[x];
        }

you could look at using extend_from_slice or copy_from_slice.  I must admit that I have never used either of those though, so I'm not 100% sure how to use them correctly with two Vecs.

I know this is nearly two months after you posted, but I hope this is helpful and/or informative nonetheless :)

EDIT: I realised that I had overlooked something an alternative to using map earlier, which I personally feel is a bit cleaner.  You could use fold instead, which should have the same net effect, but be (imho) a bit nicer to look at.

for x in 0..self.neuron_layers.len() - 1 {
    for y in 0..self.neuron_layers[x+1].len() {
        let sum = self.neuron_layers[x].iter().enumerate()
           .fold(0.0, |acc, (z, nl)| nl.mul_add(
           self.connection_layers[x][y][z], acc));
        self.neuron_layers[x+1][y] = sigmoid(sum);
    }
}

It might be the case that the compiler will complain about capturing things in closures, or something along those lines though, I must admit.  You could look at using zip first, e.g.

for x in 0..self.neuron_layers.len() - 1 {
    for y in 0..self.neuron_layers[x+1].len() {
        let sum = self.neuron_layers[x].iter().zip(self.connection_layers[x][y].iter())
           .fold(0.0, |acc, (nl, cl)| nl.mul_add(cl, acc));
        self.neuron_layers[x+1][y] = sigmoid(sum);
    }
}

but I'm not sure that would actually help.

The potential downside to this approach, however, is that you are effectively telling the compiler exactly how to do the summation, while with sum you leave that as an implementation detail of Vec - and there's a pretty good chance that Vec's approach will end up working as well as whatever we could write ourselves in 90%+ of cases.

As things currently stand, I think specifying it yourself is probably for the best, but if in the future someone works out a better way to sum over a Vec, that might end up being incorporated into the Vec struct.  If you are simply using sum, you get that 'for free' when you compile the program, whereas if you are using fold you continue using the exact same approach no matter what (which may or may not be what you want).  Again, you would probably have to test the different approaches out and decide which one works best for you.


Dictionary of key signatures for various major and minor scales I've been working on a simple program that matches a scale with its key signature. It's fairly simple, but I feel there is redundancy in the dict I created since I don't know how I could assign two keys for a value. Furthermore, I tried setting the dict in classes, but I didn't know how to format it correctly.

The strings are written in Spanish since this was a program requested by my friend.

major_and_sharp_notes = {
    'do mayor':'ninguna',
    'sol mayor':'fa#',
    're mayor':'fa#, do#',
    'la mayor':'fa#, do#, sol#',
    'mi mayor':'fa♯, do♯, sol♯, re♯',
    'si mayor':'fa♯, do♯, sol♯, re♯, la♯',
    'fa sostenido mayor':'fa♯, do♯, sol♯, re♯, la♯, mi♯',
    'do sostenido mayor':'fa♯, do♯, sol♯, re♯, la♯, mi♯, si♯',
}

minor_and_sharp_notes = {
    'la menor':'ninguna',
    'mi menor':'fa♯',
    'si menor':'fa♯, do♯',
    'fa sostenido menor':'fa♯, do♯, sol♯',
    'do sostenido menor':'fa♯, do♯, sol♯, re♯',
    'sol sostenido menor':'fa♯, do♯, sol♯, re♯, la♯',
    're sostenido menor':'fa♯, do♯, sol♯, re♯, la♯, mi♯',
    'la sostenido menor':'fa♯, do♯, sol♯, re♯, la♯, mi♯, si♯',
}

major_and_flat_notes = {
    'fa mayor':'si♭',
    'si bemol mayor':'si♭, mi♭',
    'mi bemol mayor':'si♭, mi♭, la♭',
    'la bemol mayor':'si♭, mi♭, la♭, re♭',
    're bemol mayor':'si♭, mi♭, la♭, re♭, sol♭',
    'sol bemol mayor':'si♭, mi♭, la♭, re♭, sol♭, do♭',
    'do bemol mayor':'si♭, mi♭, la♭, re♭, sol♭, do♭, fa♭',
}

minor_and_flat_notes = {
    're menor':'si♭',
    'sol menor':'si♭, mi♭',
    'do menor':'si♭, mi♭, la♭',
    'fa menor':'si♭, mi♭, la♭, re♭',
    'si bemol menor':'si♭, mi♭, la♭, re♭, sol♭',
    'mi bemol menor':'si♭, mi♭, la♭, re♭, sol♭, do♭',
    'la bemol menor':'si♭, mi♭, la♭, re♭, sol♭, do♭, fa♭',
}

def search():
    while True:
        user_search = input('Escriba la tonalidad para encontrar su armadura:').lower()
        response = "La armadura de %s es: " % user_search

        if user_search in major_and_sharp_notes:
            print(response + major_and_sharp_notes[user_search])

        elif user_search in minor_and_sharp_notes:
            print(response + minor_and_sharp_notes[user_search])

        elif user_search in major_and_flat_notes:
            print(response + major_and_flat_notes[user_search])

        elif user_search in minor_and_flat_notes:
            print(response + minor_and_flat_notes[user_search])

        elif user_search == 'quit':
            return False

        else:
            print("Estas seguro que esa es una tonalidad?")

def main():
    print ('Bienvenido al buscador de armaduras por Jfelix\n'
           'Escriba \'quit\' cuando termine de buscar.\n'
        )
    search()

if __name__ == '__main__':
    main()

As with many programming problems, this one can be solved with an extra layer of indirection.  As you know, the sharps and flats are added in a predictable order (the circle of fifths).  A highly compressed way to describe a key signature is to state how many sharps or flats there are.  Here, I'll choose to encode sharps as positive numbers, and flats as negative numbers.  I'll also write it using a two-column layout.

SCALES = {
    'do bemol mayor': -7,           'la bemol menor': -7,
    'sol bemol mayor': -6,          'mi bemol menor': -6,
    're bemol mayor': -5,           'si bemol menor': -5,
    'la bemol mayor': -4,           'fa menor': -4,
    'mi bemol mayor': -3,           'do menor': -3,
    'si bemol mayor': -2,           'so menor': -2,
    'fa mayor': -1,                 're menor': -1,
    'do mayor': 0,                  'la menor': 0,
    'sol mayor': +1,                'mi menor': +1,
    're mayor': +2,                 'si menor': +2,
    'la mayor': +3,                 'fa sostenido menor': +3,
    'mi mayor': +4,                 'do sostenido menor': +4,
    'si mayor': +5,                 'sol sostenido menor': +5,
    'fa sostenido mayor': +6,       're sostenido menor': +6,
    'do sostenido mayor': +7,       'la sostenido menor': +7,
}

The trick, then, is to write some code to perform a lookup using that data structure.  Here's how I would do it:

KEY_SIGNATURE_DECODERS = (
    lambda n: ['ninguna'],
    lambda n: ['fa♯', 'do♯', 'sol♯', 're♯', 'la♯', 'mi♯', 'si♯'][0:+n],
    lambda n: ['si♭', 'mi♭', 'la♭', 're♭', 'sol♭', 'do♭', 'fa♭'][0:-n],
)

def key_signature(scale):
    """
    Given the name of a major or minor scale in Spanish (e.g. "re mayor"),
    return a list of the sharps or flats in its key signature.
    For C Major and A Minor, return a one-element list containing
    the string "ninguna".
    """
    def signum(n): return -1 if n < 0 else +1 if n > 0 else 0
    count = SCALES[scale]
    return KEY_SIGNATURE_DECODERS[signum(count)](count)

Note that I would divide the work between the search() and main() functions differently, such that one function is purely responsible for the lookup, and the other serves as the user interface (prompting, formatting, and looping).

def main():
    print("Bienvenido al buscador de armaduras por Jfelix\n"
          "Escriba 'quit' cuando termine de buscar.")
    while True:
        scale = input('Escriba la tonalidad para encontrar su armadura: ')
        scale = scale.lower()
        if scale == 'quit':
            break
        try:
            print('La armadura de {0} es: {1}'.format(
                scale,
                ', '.join(key_signature(scale))
            ))
        except KeyError:    # Ha ha, KeyError -- get it?
            print('¿Estas seguro que esa es una tonalidad?')

if __name__ == '__main__':
    main()


SPOJ Adding Reversed Numbers Recently, I was taking up this challenge and choose my favorite language 'Ruby' to do it.
My following solution got accepted (Gist).

For each test case, I am to reverse two numbers, add them, and print the reverse of the sum.  Reversing means taking the base-10 digits of a number in reverse order, stripping any leading or trailing zeroes.

### Code
cases = gets.chomp.to_i

if cases.zero?
  puts "Enter number > 0"
  exit
end

def reverse_num(num)
  num.to_s.reverse.to_i
end

def rev_add(data)
  sum = 0
  data.each do |rec|
    sum += reverse_num(rec)
  end
  reverse_num(sum)
end

results = []

cases.times do |index|
  input_str = gets.chomp
  nums = input_str.split(' ').map(&:to_i)
  results[index] = rev_add(nums)
end

results.each { |e| puts e }

### End of Code

It showed that I have used 7.3MB memory which I don't have any idea.

I know this is not the optimized solution. I have used lots of looping which could be minizied.
Could you review it and help me to optimize it?

While your performance limits may be satisfied by producing your results as they are available, there are also other issues in your code which you should consider altering.

First up, 1-liner solutions are convenient for saving some typing, but often make compromises that affect other aspects of your code. In your case, the 1-liner for reversing the number:

  num.to_s.reverse.to_i

That line takes a number, converts it to a string, and then reverses the string, and then converts the string back to a number.

Note that in each conversion, you need to convert between base-2 and base-10 number systems, and that you need to allocate space for the String, etc. While the following takes more code, it actually does less work:

def reverse_num(num)
  rev = 0
  while num > 0 do
    rev *= 10
    rev += num % 10
    num /= 10
  end
  rev
end

There are no strings in the above, the loop iterates one time for each decimal digit, and well, just works.

A second issue I want to point out is with your line-by-line algorithm. You read an entire line, split on the spaces, and from the list of strings, you create a list of numbers with to_i. You then iterate this list, and sum the reverses.

A much neater solution is to do a map->reduce operation:

sum = input_str.split(' ').map{ |n| reverse_num(n.to_i) }.reduce(:+)

Then you can eliminate the entire rev_add method. The end result is:

cases = gets.chomp.to_i

def valid_numeric_input?(input)
  input.zero?
end

def reverse_num(num)
  rev = 0
  while num > 0 do
    rev *= 10
    rev += num % 10
    num /= 10
  end
  rev
end

valid_numeric_input?(cases)

cases.times do |index|
  input_str = gets.chomp
  sum = input_str.split(' ').map{ |n| reverse_num(n.to_i) }.reduce(:+)
  puts reverse_num(sum)
end


Thread-Safe Variadic Printing Function Printing to stdout is thread-safe in many systems when using printf or std::cout, but not in all systems (Windows!). So I decided to make my own thread-safe and type-safe printing function with some help of C++11 variadic templates functions.

#include <iostream>
#include <iomanip>
#include <mutex>

class Mutex:private std::mutex{
public:
    class Lock:private std::lock_guard<std::mutex>{
    public:
        Lock(Mutex& mutex):
        std::lock_guard<std::mutex>(mutex){}
    };
};

class{
private:
    Mutex m_mutex;

    void Print(){}

    template<typename T,typename... Ts>
    void Print(const T& t,const Ts&... ts){
        std::cout<<t<<std::flush;
        Print(ts...);
    }

public:
    template<typename T,typename... Ts>
    void operator()(const T& t,const Ts&... ts){
        Mutex::Lock lock(m_mutex);
        Print(t,ts...);
    }
}Print;

template<typename T>
void PrintLine(){
    Print('\n');
}

template<typename T,typename... Ts>
void PrintLine(const T& t,const Ts&... ts){
    Print(t,ts...,'\n');
}

int main(){
    Print(10," devided by ",3," equals ");
    PrintLine(std::fixed,std::setprecision(5),10.0/3);
    return 0;
}

Mutex

class Mutex:private std::mutex{
  public:

Don't see the need for your custom Mutex/Lock class. Just use the standard ones directly there is no need to add a layer of indirection that future developers need to go and check.

Excessive Flushing

Calling flush after each item:

std::cout<<t<<std::flush;

Is probably not a good idea. Let the user of your code decide when to flush he probably has more concept about what what is about to be printed and thus when flush needs to be called.

If you must do it then do it after all the items have been printed.

Template Recusrsion.

Rather than using a recursive call to print all the items:

Print(ts...);

Use a sub class and create a std::initializer_list

// std::cout<<t<<std::flush;
// Print(ts...);
// Create a list. This will act more like a loop than recursion.
// Items inside the {} are initialized left to right.
auto printer = { ItemPrinter(ts)... };

Then ItemPrinter just prints the item in the constructor.

template<typename T>
struct ItemPrinter { ItemPrinter(T const& t){std::cout << t;}};

As a nice benifit. You will not need the terminating case.

void Print(){}

Naming Convention.

Print

It is more standard to use an initial capitol letter to define a type. An initial lower case letter donates an object (which also encompasses functions).

Since you have a nameless type. With an object called Print. I would have called it streamPrinter. Then used your wrapper functions call that directly.

Declaration

The code works well as is for a single file program. But you have problems when using it from header files. Because the class is nameless you can not use it any declarations to mark the object external and thus you will be getting an instantiation of the object in every compilation unit. This will break your guarantee that it is thread safe as each has its own mutex.

I think your best bet is to put the Print object in its own file. Then expose all printing via wrapper functions which can be made external in the header file.

Expansion.

Currently your printer is only used for std::cout. Why not expand it so that it can be used for any stream.


Split string into "n" lengths with padding I quickly wrote a short program to split a string into \$n\$ lengths with variable-character padding:

#include <iostream>
#include <algorithm>

std::string getString();
int getBlockSize();
char getPaddingChar();
std::string addPadding(std::string str, int blockSize, char paddingChar);
std::string tokenize(std::string str, int blockSize);

int main(){
    std::string str = getString();
    int blockSize = getBlockSize();
    char paddingChar = getPaddingChar();

    str.erase(std::remove(str.begin(), str.end(), ' '), str.end());
    str = addPadding(str, blockSize, paddingChar);
    str = tokenize(str, blockSize);

    std::cout << str;

    std::cout << '\n';
    return 0;
}

std::string getString(){
    std::cout << "Input string: ";

    std::string str;
    std::getline(std::cin, str);

    return str;
}

int getBlockSize(){
    std::string inputStr;
    int blockSize;

    do{
        try{
            std::cout << "Block size: ";
            std::getline(std::cin, inputStr);
            blockSize = std::stoi(inputStr);
        } catch(...){
            blockSize = 0;
        }
    } while(blockSize < 1);

    return blockSize;
}

char getPaddingChar(){
    char paddingChar;

    do{
        std::cout << "Padding character: ";
        std::cin >> paddingChar;
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(),'\n');
    } while(std::cin.fail());

    return paddingChar;
}

std::string addPadding(std::string str, int blockSize, char paddingChar){
    while(str.length() % blockSize != 0){
        str += paddingChar;
    }
    return str;
}

std::string tokenize(std::string str, int blockSize){
    for(int i = blockSize; i < str.length(); i += blockSize+1){
        str.insert(i, 1, ' ');
    }
    return str;
}

This code firstly gets the input string (str), gets the size of the block (blockSize), and then gets the character that is to be used as padding (paddingChar).

It then removes all spaces from input string, adds padding, and then tokenizes the string by splitting it into blockSize pieces.

Example output:

Input string: Tis' but a scratch!
Block size: 11
Padding character: X
Tis'butascr atch!XXXXXX

The splitting of a stream of characters into blocks is common in cryptography. While there certainly are many, many Code Review questions on tokenization, I was unable to find one that was exactly like this and in C++, so I felt that this area should also be covered in that aspect.

I tried to use the helpful suggestions from my previous Code Review questions to improve the quality of my code as best I could. Despite that, there is always room to improve the way that I write code.

How can I improve this code?

My first impression is that we would probably wish to avoid copying and shuffle data as much as possible.

The code as presented does the following:

conditionally remove items (worst case time complexity occurs when every char is a space, in which case it's \$\mathcal{O}(\frac{n^2}{2})\$ (more or less))
append padding (worst case will involve a reallocation plus copy \$\mathcal{O}(n)\$)

I think we can do better, performing the entire operation in linear time and guaranteeing no memory reallocations. This involves iteratively conditionally copying the bytes. We can do this because std::string's iterators are random-access, so we can compute distances in constant time.

I have presented it here as a simple composition of methods in a lightweight engine object. Another way would be to create a state machine which could be chained to another encryption stage:

#include <iostream>
#include <algorithm>

struct squish_engine
{
    squish_engine(std::size_t blockSize, char paddingChar)
            : blockSize(blockSize)
            , paddingChar(paddingChar)
    {

    }

    // useful for initial allocation of destination string
    std::size_t estimate_size(std::size_t plaintext_length) const {
        return plaintext_length + blockSize;
    }

    // a predicate
    static bool matchesSpace(char c) {
        return c == ' ';
    }

    // operation in terms of iterators
    template<class FromIter, class ToIter>
    ToIter operator()(FromIter first, FromIter last, ToIter first2) const
    {
        // use stl algorithms where possible
        auto last2 = std::remove_copy_if(first, last, first2, matchesSpace);
        auto pads = std::distance(first2, last2) % blockSize;
        if (pads) {
            pads = blockSize - pads;
        }
        last2 += pads;
        return last2;
    };

    // this is the public interface
    template<class Iter>
    std::string operator()(Iter first, Iter last) const
    {
        // build an overly-large string to accept the result.
        // we may as well pad it now, as it's no extra cost compared
        // to zero-filling
        std::string result(estimate_size(std::distance(first, last)), paddingChar);
        // our internal operator will compute the end point of the
        // padded area for us
        auto last2 = (*this)(first, last, std::begin(result));

        // so we can simply truncate any padding chars we don't need
        result.erase(last2, result.end());

        // and return the cypher
        return result;
    }

    // convenience interface
    std::string operator()(const std::string& s)
    {
        return (*this)(std::begin(s), std::end(s));
    }

    std::size_t blockSize;
    char paddingChar;
};

// the concept of making a copy while filtering
template<class Filter>
std::string filter_copy(const std::string& source, Filter&& filter)
{
    return filter(source);
}

int main(){

    std::size_t blockSize = 11;
    char paddingChar = 'X';

    std::string str = "Tis' but a scratch!";

    // now we can conceivably choose the encryption engine at the
    // client call site
    auto compressed = filter_copy(str, squish_engine(blockSize, paddingChar));

    std::cout << compressed << '\n';
    return 0;
}


Checking if a remote server is online I have this vbs file to check if a remote server is accessible. Can I simplify or refactor to make it more robust/efficient?

const forReading = 1, forWriting = 2, forAppending = 8
public maxWaitTime
maxWaitTime = 60 '1800 '30mins
public connLogFileName
connLogFileName = "connectionLog.txt"
public pathToRemoteServer
pathToRemoteServer = "c:\"
public errMessage
public fs
set fs = createObject("scripting.fileSystemObject")

pathToRemoteServer = "c:\"

'::::::::::::::::::::::::::::::
call mainSub()

'::::::::::::::::::::::::::::::
sub mainSub()
  call createLogFile()

  errMessage = ""
  dim curDir
  curDir = fs.GetAbsolutePathName(".")
  dim logName
  logName = curDir & "\" & connLogFileName
  Dim aFile
  set aFile = fs.openTextFile(logName, forAppending, True)

  if (checkFolderPath(pathToRemoteServer)) then
    aFile.WriteLine lineToWrite & " ok"
  else
    aFile.WriteLine lineToWrite & " " & errMessage

    dim downTime
    downTime = 0
    do Until (checkFolderPath(pathToRemoteServer)) or downTime => maxWaitTime
      WScript.Sleep 500  'half second
      downTime = downTime + 0.5
    loop

    aFile.WriteLine lineToWrite & " back after seconds: " & downTime

  end if

  aFile.close
  if not aFile is nothing then set aFile = nothing
end sub

'::::::::::::::::::::::::::::::
sub createLogFile()
  dim logName
  logName = fs.GetAbsolutePathName(".") & "\" & connLogFileName
  If not fs.FileExists(logName) Then
    fs.createTextFile logName, false
  End If
end sub

':::::::::::::::::::::::::::::::::::::::::
function lineToWrite()
  dim curUser
  curUser = createObject("WScript.Network").userName
  lineToWrite = curUser & ": " & Now()     '& vbCrLf
end function

':::::::::::::::::::::::::::::::::::::::::
function checkFolderPath(folder_path)
  checkFolderPath = false
  On Error Resume Next

  dim fn
  fn = folder_path & "testfile." & myDateFormat(Now())

  dim aFile
  set aFile = fs.createTextFile(fn, True)
  if err.Number <> 0 Then
    errMessage = "Error on crt: " & err.description
    err.clear
    checkFolderPath = false
    exit function
  end if

  aFile.WriteLine ("This is a test file " & fn)
  if err.Number <> 0 Then
    errMessage = "Error on wrt: " & err.description
    err.clear
    checkFolderPath = false
    exit function
  end if

  aFile.Close
  if err.Number <> 0 Then
    errMessage = "Error on cls: " & err.description
    err.clear
    checkFolderPath = false
    exit function
  end if

  fs.DeleteFile fn
  if err.Number <> 0 Then
    errMessage = "Error on del: " & err.description
    err.clear
    checkFolderPath = false
    exit function
  end if

checkFolderPath = true
if not aFile is nothing then set aFile = nothing
end function

':::::::::::::::::::::::::::::::::::::::::
function myDateFormat(aDate)
  sc = second(aDate)
  mn = minute(aDate)
  hr = hour(aDate)
  dy = day(aDate)
  mt = month(aDate)
  yr = year(aDate)
  myDateFormat= yr & mt & dy & hr & mn & sc
end function

There's a lot to cover and talk about here. I'm going to take a line by line approach, so this review might seem nitpicky (and it probably is), but there are some seemingly innocuous things here that can cause bigger problems given a chance.

const forReading = 1, forWriting = 2, forAppending = 8

This is good. I like well named constants. These are implicitly Public though. That means they're available to all scripts. Which is also okay. I see no reason these can't be, but it's always better to be explicit and declare the scope.

Public Const forReading = 1, forWriting = 2, forAppending = 8

public maxWaitTime
maxWaitTime = 60 '1800 '30mins
public connLogFileName
connLogFileName = "connectionLog.txt"

Neither of these should be Public. Nor should they be variables. They're constants and should be declared as such. Let the compiler warn you if you ever try to assign a new value to these.

public pathToRemoteServer
pathToRemoteServer = "c:\"
public errMessage
public fs
set fs = createObject("scripting.fileSystemObject")

pathToRemoteServer = "c:\"

'::::::::::::::::::::::::::::::

Same advice here. These variables should be private to the script. Also, you're reassigning the same value to the same variable just a few lines apart?? Make it a constant and forget it.

I try not to pick on names, but fs would be better named fso. The latter seems to be an idiom in the community. Generally, I don't recommend overly abbreviated names like this, but idioms are ok. VBA/VBScript developers will recognize both of these as a FileSystemObject.

'::::::::::::::::::::::::::::::
call mainSub()

'::::::::::::::::::::::::::::::
sub mainSub()

Two things jump out at me here.

mainSub isn't a good name. It doesn't say anything at all about what the subroutine does.
It doesn't seem to make much sense to have a subroutine here at all. You could remove the Sub...End Sub statement and just execute it in your main script without changing the semantics or execution at all. (I don't recommend actually doing that, but I'm not quite there yet.)

sub mainSub()
  call createLogFile()

  errMessage = ""
  dim curDir
  curDir = fs.GetAbsolutePathName(".")
  dim logName
  logName = curDir & "\" & connLogFileName
' ....
End Sub

'::::::::::::::::::::::::::::::
sub createLogFile()
  dim logName
  logName = fs.GetAbsolutePathName(".") & "\" & connLogFileName
  If not fs.FileExists(logName) Then
    fs.createTextFile logName, false
  End If
end sub

So, you're going to your file system object to get the current directory twice in rapid succession and sharing a variable between these two methods. That's smelly to me. createLogFile should take in a full file path as an argument. Then, connLogFileName doesn't need to be scoped outside of mainSub. Actually, I don't see a need for that constant at all in this case. You can keep it if you want, but I've removed it below.

sub createLogFile(logName)
  If not fs.FileExists(logName) Then
    fs.createTextFile logName, false
  End If
end sub

sub mainSub()

  errMessage = ""

  dim logName
  logName = fs.GetAbsolutePathName(".") & "\" & "connectionLog.txt"

  call createLogFile(logName)

  Dim aFile
  set aFile = fs.openTextFile(logName, forAppending, True)

I also think it would be worthwhile to pass pathToRemoteServer into mainSub as an argument. It makes the sub more re-usable, but I would definitely think up a decent name for it.

I would rename checkFolderPath. Consider this snippet.

 if (checkFolderPath(pathToRemoteServer)) then

Wouldn't it read better as:

if (folderPathExists(pathToRemoteServer)) then

Speaking of method names, I really recommend PascalCased method names. It distinguishes them from camelCased variable names.

I'm surprised this compiles.

downTime => maxWaitTime

In VBA this results in a compiler error and it's expected to be downTime >= maxWaitTime. Perhaps VBScript is more forgiving, but it also looks like the lambda operator from another popular Microsoft language. So two things happen here. I can't paste this into the VBA Editor and run it and it could cause confusion for someone expecting this to be the lambda operator.

':::::::::::::::::::::::::::::::::::::::::
function lineToWrite()
  dim curUser
  curUser = createObject("WScript.Network").userName
  lineToWrite = curUser & ": " & Now()     '& vbCrLf
end function

I like this function and the abstraction it provides, but you are calling it over and over and over to get the same string. Declare a variable and call this function once. Also, as an option, It would be completely acceptable to remove the curUser variable and one line this function.

Function LineToWrite()

    LineToWrite = CreateObject("WScript.Network").userName & ": " & Now()

End Function

Also, you should remove dead, commented out code. It serves no purpose but to clutter things.

One last thing. Your error messages kind of suck. It's one thing to abbreviate variable names, but it's an entire other matter to abbreviate error/log messages.

errMessage = "Error on crt: " & err.description
errMessage = "Error on cls: " & err.description
errMessage = "Error on del: " & err.description

Don't do that. Spell it out for whoever has to read that log.

It's also noteworthy that there's a lot of copy paste in the CheckFolderPath function, as evidence by the snippet above. It's wholly worth while to extract that logic into a function that takes an Err object and a string in as arguments.


Game of life using NumPy I'm trying to improve my Game of life program for school in Python, to be able to run it quicker. So if you have some ideas, I will be glad to hear them.

My current code:

def neighbourhood(m, i, j):
    num = 0
    n_board = len(m)
    for k in range(i-1,i+2):
        for l in range(j-1,j+2):
            if k < 0 or l < 0 or k >= n_board or l >= n_board:
                num = num + 0
            else:
                num = num + m[k][l]
    return num

def update(m):
    s = np.copy(m)
    n_board = len(m)
    for i in range(0, n_board):
        for j in range(0, n_board):
            if s[i][j] == 1:
                num = neighbourhood(s, i, j) - 1
                if num <= 1 or num > 3:
                    m[i][j] = 0
                else:
                    m[i][j] = 1
            else:
                num = neighbourhood(s, i, j)
                if num == 3:
                    m[i][j] = 1
                else:
                    m[i][j] = 0

def square(x, y):
    turtle.penup()
    turtle.setposition(x,y)
    turtle.setheading(0)
    turtle.pendown()
    turtle.begin_fill()
    for i in range(4):
        turtle.forward(0.9)
        turtle.left(90)
    turtle.end_fill()

def draw(m):
    turtle.clear()
    n_board = len(m)
    for i in range(n_board):
        for j in range(n_board):
            if m[i][j] == 1:
                square(j, (n_board - i - 1))
    turtle.update()

def main(n_board):
    turtle.reset()
    turtle.setworldcoordinates(0, 0, n_board, n_board)
    turtle.hideturtle()
    turtle.speed('fastest')
    turtle.tracer(0, 0)
    turtle.color('black')
    turtle.bgcolor('white')
    board=np.zeros((n_board, n_board), int)
    for i in range(n_board):
        for j in range(n_board):
            if random() < 0.5:
                board[i][j] = 1
    draw(board)
    for k in range(100):
        update(board)
        draw(board)

So as you can see for the moment i'm only doing 100 repetition and the starting board is randomly created.

Since you aren't using any of the things that make numpy fast, this code would be about 2x faster if you just used python lists instead. Alternatively, if you vectorized the update function, you would probably be able to make it much faster. That said, this will not be easy. This question has a very good example of what the result would look like.


Grabbing the previous 4 quarters I'm curious if there is a way to slim down this code either with SQL or with C#.

-- VARS
declare @iagent_id int
declare @quarter int
declare @prev_quarter_1 int
declare @prev_quarter_2 int
declare @prev_quarter_3 int
declare @year int
declare @prev_quarter_year_1 int
declare @prev_quarter_year_2 int
declare @prev_quarter_year_3 int

set @quarter = datepart(QQ, getdate()) - 1
set @year = datepart(year, getdate())
set @prev_quarter_year_1 = @year
set @prev_quarter_year_2 = @year
set @prev_quarter_year_3 = @year

if @quarter = 0
    begin
        set @quarter = 4
        set @year = @year - 1
        set @prev_quarter_1 = 3
        set @prev_quarter_year_1 = @year
        set @prev_quarter_2 = 2
        set @prev_quarter_year_2 = @year
        set @prev_quarter_3 = 1
        set @prev_quarter_year_3 = @year
    end
else
    begin
        if @quarter = 3
            begin
                set @prev_quarter_1 = 2
                set @prev_quarter_2 = 1
                set @prev_quarter_3 = 4
                set @prev_quarter_year_3 = @year - 1
            end
        else
            begin
                if @quarter = 2
                    begin
                        set @prev_quarter_1 = 1
                        set @prev_quarter_2 = 4
                        set @prev_quarter_year_2 = @year - 1
                        set @prev_quarter_3 = 3
                        set @prev_quarter_year_3 = @year - 1
                    end
                else
                    begin
                        if @quarter = 1
                            begin
                                set @prev_quarter_1 = 4
                                set @prev_quarter_year_1 = @year - 1
                                set @prev_quarter_2 = 3
                                set @prev_quarter_year_2 = @year - 1
                                set @prev_quarter_3 = 2
                                set @prev_quarter_year_3 = @year - 1
                            end
                    end
            end
    end

select @quarter as 'Quarter', @year as 'Year', @prev_quarter_1 as 'pq1', @prev_quarter_year_1 as 'pqy1', @prev_quarter_2 as 'pq2', @prev_quarter_year_2 as 'pqy2', @prev_quarter_3 as 'pq3', @prev_quarter_year_3 as 'pqy3'

Your code is very procedural, with branching and variables and things we see in code... and bad queries. SQL likes sets/tables.

You're lucky your fiscal quarters line up with the "normal calendar" - every company I worked for had different periods for their fiscal calendar.

Since I worked with a data warehouse, one of the first things I do when I come on board in a company where I need to work with time data, is look at their databases to see if they have a table somewhere that contains their fiscal calendars. And if they don't have one, I simply create it:

create table dbo.FiscalCalendars (
     _Id int not null identity(1,1)
    ,_DateInserted datetime not null
    ,_DateUpdated datetime null
    ,CalendarDate date not null
    ,CalendarDayOfWeek int not null
    ,CalendarDayOfMonth int not null
    ,CalendarDayOfYear int not null
    ,CalendarWeekOfYear int not null
    ,CalendarMonthOfYear int not null
    ,CalendarYear int not null
    ,FiscalDayOfWeek int not null
    ,FiscalDayOfMonth int not null
    ,FiscalDayOfQuarter int not null
    ,FiscalDayOfYear int not null
    ,FiscalWeekOfMonth int not null
    ,FiscalWeekOfQuarter int not null
    ,FiscalWeekOfYear int not null
    ,FiscalMonthOfQuarter int not null
    ,FiscalMonthOfYear int not null
    ,FiscalQuarterOfYear int not null
    ,FiscalYear int not null
    ,constraint PK_FiscalCalendars primary key clustered (_Id asc)
    ,constraint NK_FiscalCalendars unique (CalendarDate)
);

With a table that stores everything you've always wanted to know about every date you ever need to know anything about, selecting the last 4 quarters becomes... simple:

declare @referenceDate as date;
set @referenceDate = getdate();

with quarters as (
    select
         t.FiscalYear
        ,t.FiscalQuarterOfYear
        ,min(t.CalendarDate) StartDate
        ,max(t.CalendarDate) EndDate
    from dbo.FiscalCalendars t
    group by
         t.FiscalYear
        ,t.FiscalQuarterOfYear
)
select top 4
     q.FiscalYear
    ,q.FiscalQuarterOfYear
    ,q.StartDate
    ,q.EndDate
from quarters q
where q.StartDate <= @referenceDate
order by
     q.EndDate desc

The hardest part is ...populating that time table, not querying it.

Now, to make the above return a column for each quarter, you'll need to pivot it. But if you're working (and thinking) in sets (LINQ works off IEnumerable<T> after all), you won't need to do this.

Speaking of LINQ... assuming you have the time table mapped to FiscalCalendar entities, you could query it like this:

var referenceDate = DateTime.Today;
var quarters = context.FiscalCalendars
                      .GroupBy(t => new { t.FiscalYear, t.FiscalQuarterOfYear })
                      .Select(g => new {
                                         FiscalYear = g.Key.FiscalYear,
                                         FiscalQuarterOfYear = g.Key.FiscalQuarterOfYear,
                                         StartDate = g.Min(q => q.CalendarDate),
                                         EndDate = g.Max(q => q.CalendarDate)
                                        })
                      .Where(q => q.StartDate <= referenceDate))
                      .OrderByDescending(q => q.EndDate)
                      //.Skip(1) // skip current quarter?
                      .Take(4);

That would give you an IQueryable<T> where T is an anonymous type with int FiscalYear, int FiscalQuarterOfYear, DateTime StartDate and DateTime EndDate properties.


Correct MVC in CodeIgniter In my CodeIgniter project I have a Login controller that load this view:

         Mis solicitudes

The controller: soporte, function: callSupport

public function callSupport($index = null, $order = null){
    $session_data = $this->session->userdata('session_user');
    $data['solicitud']  = $this->helpdesk_model->getRequest($session_data['usuario'], $index, $order);
    $data['categories'] = $this->helpdesk_model->getCategories();
    $this->load->view('modules/help_desk/supports', $data);
}

With jQuery I call the controller and render in a page content that is a
 element:

$(".sub-menu .menu-item, .module-item").click(function(event) {
    $(".page-content").load($(this).data('target'));
    $("title").text( $( this ).text() );
});

In the view 'modules/help_desk/supports':

    Estado
    Prioridad
    Responsable

    <?php foreach ($solicitud as $key => $value) { ?>

        <span class="label label-sm <?= $value['label']?> "><?= ucfirst($value['status']) ?>
        <span class="badge <?= $value['class']?> "><?= $value['priority'] ?>
        <?= $value['name'].' '.$value['last_name'] ?>

    <?php  } ?>

As you can see, in the first  a put an anchor tag calls another controller with jQuery similar than before. Please be critical, very severe with me.

General comments
I don't see much wrong with the code. The biggest concern I might have is that PHP code is mixed in with the HTML - an alternative to this would be using a template system. CodeIgniter has the template parser class which should allow you to do this.

Please be critical, very severe with me.

Okay - I am not typically this critical but you asked for it! The next section has a couple other review points
Minor nitpicks
In the PHP code, $data is not declared before values are assigned to specific keys:

$data['solicitud']  = $this->helpdesk_model->getRequest($session_data['usuario'], $index, $order);

I don't see a problem with this practice but the PHP documentation advises against it:

If $arr doesn't exist yet, it will be created, so this is also an alternative way to create an array. This practice is however discouraged because if $arr already contains some value (e.g. string from request variable) then this value will stay in the place and [] may actually stand for string access operator. It is always better to initialize a variable by a direct assignment.

I tried running the jQuery callback through the demo page on eslint.org. It showed this warning:

'event' is defined but never used.

For this line:

$(".sub-menu .menu-item, .module-item").click(function(event) {

While event could be used if necessary, it isn't needed.


Filtering from a file to show information I want to get my code cleanner and more efficient. This code gets variables form a PHP file and filters it to show the selected user name, all available usergroups on a list box and the groups he is currently into in another.
<script>

    function saveGroup(type,id){
      $('#useridgr').attr('placeholder', '').val('');
      $("#usernamegr").attr('placeholder', 'Nome').val('');
      $('#list_box').html('');
      $('#list_box_ini').html('');

      if (type == 1){

      } else {
        loadatagroup(id);
      }

    }
    var loadatagroup = (uid) => {

      $.ajax({
        type: 'GET',
        url: 'usergrupo/' + uid,
        data: {'_token': $("input[name='_token']").val()},
        success: function(data){

          console.log("success");

          var o = data[0];

          $('#useridgr').attr('placeholder',o.id).val(o.id);
          $("#usernamegr").attr('placeholder',o.name).val(o.name);

          console.log('areaa');

          var size = Object.size(data);

          console.log(size);

          for (var i=0; i<size; i++){
            var o = data[i];
            $("#list_box").append("<option value='"+o.idgrupo+"'>"+o.nome+"</option>");
          }

        }

      })

    }

    Object.size = function(obj) {
      var size = 0, key;
      for (key in obj) {
        if (obj.hasOwnProperty(key)) size++;
      }
      return size;
    };

    </script>

Since you are using jquery, you can use its built in each function to loop through the json results, which will eliminate the need for the size function.
i = index, v = values
$.each(data,function(i,v){
   $("#list_box").append("<option value='"+v.idgrupo+"'>"+v.nome+"</option>");
})


Lindenmayer System Python Fractral Tree I have created a fractal tree based on a Lindenmayer System in Python. If you could tell me about any algorithmic or efficiency-based problems in the code, I would be grateful.

import turtle
import time
t = turtle.Pen()

branch = []
code = "1[0]0" # Change this to modify the structure of the tree

t.left(90)
t.up()
t.backward(300)
t.down()
t.speed(10)

for c in code:
    if c == "1":
        t.pencolor("brown")
        t.forward(100) # Change this to modify the size of the tree

    elif c == "0":
        t.pencolor("green")
        t.forward(3) # Change this to modify the size of the leaves

    elif c == "[":
        t.left(45)
        branch.append((t.position(), t.heading()))

    elif c == "]":
        t.up()
        t.goto(branch[len(branch) -1][0], None)
        t.setheading(branch[len(branch) -1][1])
        t.right(90)
        t.down()
        del branch[len(branch) -1]

Personally I don't like multiples if statements inside a for, here is my suggestion:

def handler_option_c(t, b):
    t.pencolor("brown")
    t.forward(100) # Change this to modify the size of the tree

handlers = { "c" : handler_option_c, ....

And you do the same for the other options, and then on your main

for c in code:
    handlers[c](t, branch)

Or you can put that on a lambda also


firstDuplicate of array This question has been asked for other languages, but I haven't seen it for C++.  Most notably here and here.

  Given an array a that contains only numbers in the range from 1 to a.length, find the first duplicate number for which the second occurrence has the minimal index. In other words, if there are more than 1 duplicated numbers, return the number for which the second occurrence has a smaller index than the second occurrence of the other number does. If there are no such elements, return -1.

My tests are running correctly, but I'm getting a timeout from the website codesignalDotCom.  I'm wondering what I'm doing that could be taking so much time, and what could be done to fix it.

int firstDuplicate(std::vector<int> a) {
    std::vector<int> newVector;
    std::cout << a.size() << std::endl;
    for(int i = 0; i < a.size(); i++) {
        if(std::find(newVector.begin(), newVector.end(), a.at(i)) != newVector.end()){
            return a.at(i);
        }
        newVector.push_back(a.at(i));
    }
    return -1;
}

Find more appropriate data structures

  This question has been asked for other languages, but I haven't seen it for C++.

Getting timeout usually indicates an inefficient algorithm.
The language doesn't matter, because algorithms are language agnostic.
Some of the posts you linked mention the word "performance",
and they shed light to the problem,
with suggestions that could be applied here too,
even if the language is different.

The first thing to ask when a solution is too slow,
what's the time complexity of the algorithm?

   quadratic: \$O(n^2)\$

That's not so good. Looping over all elements is a linear operation (\$O(n)\$),
and checking if an element is in a vector is also a linear operation.

In other words, the weak spot is using a vector to store the elements already seen.
A vector doesn't provide a fast way to search for random elements.
Other data structures can provide faster than linear performance to lookup elements:
a tree set can do it in logarithmic time (\$O(\log n)\$, making the overall complexity of your algorithm log-linear \$O(n \log n)\$),
or a hash set can do it in constant time (\$O(1)\$, making the overall complexity of your algorithm linear \$O(n)\$).

Coding style

The name newVector is not great, for several reasons:

What's "new" about it?
Including the name of the data structure in the name of the variable usually doesn't give much new information to the reader, and therefore doesn't help understand its purpose.

This vector is used to store elements already seen. I would have called it seen.

The name a is even worse: it tells nothing to the reader about what it is.

The main loop goes over indexes of the input vector.
You could loop over the values of the input,
which would read more natural:

for (int value : a) {
    if (std::find(newVector.begin(), newVector.end(), value) != newVector.end()){
        return value;
    }
    newVector.push_back(value);
}


JavaScript - Does this bookmarklet need some optimization? I'm working on JavaScript bookmarklet that lets users send a text and link to a site.
This is the bookmarklet:
javascript:(function(){new_script=document.createElement('SCRIPT');new_script.src='http://bookmarklet.example.com/js/bookmarklet.js?v=1';document.getElementsByTagName('head')[0].appendChild(new_script);new_script.type='text/javascript';})();

This is the JavaScript for it:
if(frames.length>0){
      F=' (Frames not supported)'
  } else {
      F=''
  }
  Q=document.getSelection();
  if(!Q){
 void(Q=prompt('Enter a text...'+F+'',''))
  };
  if(Q){
      void (window.open('http://www.example.com/add?url='+encodeURIComponent(window.location)+'&text='+escape(Q)+'&title='+encodeURIComponent(document.title)))
  }

It's the first time I'm using JavaScript, so I would love to hear your feedback on the code. Is there anything that needs to be optimized?
Thanks.
Patrick

The loader could be simplified to the following:

javascript:(function () {document.getElementsByTagName('head')[0].appendChild(document.createElement('script')).src = 'http://bookmarklet.example.com/js/bookmarklet.js?v=1'}());

No more variables. By the way, you forgot to use var. It causes the variable to be declared as a global.
Browsers, by default, parse script tags as JavaScript, thus the type can be omitted.

And in your script body, as far as I can understand, it's a script that gets a selected text or, if none selected, lets the user manually input the text and sends it to a server. Thus we have:

//remember that you are loading this script into the global scope. Protect your
//variables by enclosing it in a closure
(function () {
      //a ternary if is a short 2-way evaluation (as far as reading is concerned)
  var F = (frames.length > 0) ? ' (Frames not supported)' : '',
      //the OR operator (`||`) could also be used to evaluate stuff
      //here, it returns either document.getSelection(),
      //or if non existent, a prompted value
      Q = document.getSelection() || prompt('Enter a text...' + F + '', '');

  //then we execute your command
  window.open('http://www.example.com/add?url=' + encodeURIComponent(window.location) + '&text=' + escape(Q) + '&title=' + encodeURIComponent(document.title))

}());

As an additional tip, I suggest you load the url to a hidden iframe for a cleaner interface. You could add listeners (there are hacks for cross domain iframe messaging), to detect a success and show a confirmation UI afterwards.


Finding the closest string match for each string in two dataframe columns I was wondering what's the fastest method to compare similarity between two lists of strings (e.g. two dataframes, documents etc.) using levenshtein distance or other procedures.

I am currently using:

def wuzzyfuzzy(df1, df2):
    myList = []
    total = len(df1)
    for idx1, df1_str in enumerate(df1.col1):
        myDict = {}
        my_str = ('Progress : ' + str(round((idx1/total)*100,3))+'%')
        sys.stdout.write('\r' + str(my_str))
        sys.stdout.flush()

        for idx2, df2_str in enumerate(df2.col1):
            s = SequenceMatcher(None, df1_str, df2_str)
            r = s.ratio()
            myDict.update({df2_str:r})
        best_match = max(myDict, key=myDict.get)
        myList.append([df1_str, best_match, myDict[best_match]])

    return myList

As the dataframes that are passed to the function have both > 30.000 values, it takes currently around 6 hours to compare each value in df1 with all other values in df2 to find the best match.

Of course I cleaned the strings as good as possible beforehand (all lowercase, get rid of punctuations etc.)

What's the most efficient way to perfom such task?

Comparing 30,000 strings against 30,000 other strings is 900 million comparisons.  It's going to take a while.

Run the Python profiler on a small data set to see where it is spending the most time.  So you can focus your efforts.

difflib

The documentation for SequenceMatcher says it caches information about the second sequence.  So that to compare one string against a bunch of other strings, use .set_seq2() to set the one string, and the use .set_seq() to check it against each of the other strings.

It also says that calculating ratio() is expensive to compute so you might want to use 'quick_ratio()orreal_quick_ratio()` first.

def wuzzyfuzzy(df1, df2):
    myList = []
    total = len(df1)

    s = SequenceMatcher(isjunk=None, autojunk=False)

    for idx1, df1_str in enumerate(df1.col1):
        s.set_seq2(df1_str)

        my_str = ('Progress : ' + str(round((idx1/total)*100,3))+'%')
        sys.stdout.write('\r' + str(my_str))
        sys.stdout.flush()

        best_str2 = ''
        best_ratio = 0

        for idx2, df2_str in enumerate(df2.col1):
            s.set_seq2(df2_str)

            if s.real_quick_ratio() > best_ratio and s.quick_ratio() > best_ratio:
                r = s.ratio()

                if r > best_ratio:
                    best_match = df2_str
                    best_ratio = r

        myList.append([df1_str, best_match, best_ratio])

    return myList

You could also consider difflib.get_close_matches(string, possibilities, n, cutoff).  It compares string against a list possibilities and returns a list of upto n that match better than cutoff.

def wuzzyfuzzy(df1, df2):
    myList = []

    possibilities = list(df2.col1)

    s = SequenceMatcher(isjunk=None, autojunk=False)

    for idx1, df1_str in enumerate(df1.col1):
        my_str = ('Progress : ' + str(round((idx1/total)*100,3))+'%')
        sys.stdout.write('\r' + str(my_str))
        sys.stdout.flush()

        # get 1 best match that has a ratio of at least 0.7
        best_match = get_close_matches(df1_str1, possibilities, 1, 0.7)

        s.set_seq(df1_str, best_match)
        myList.append([df1_str, best_match, s.ratio()])

        return myList


Bubble sort with counting swaps and comparsions I have bubble sort code below, which has to count swaps and comparisons of elements. How can it be made more elegant and clear, if it is possible? I think using lots of tuples is not the best way to do this.

bsort' x = bsort (x,0,0)

bsort :: (Ord a) => ([a], Int,Int) -> ([a], Int, Int) -- (table, nswaps, ncompares)
bsort ([],i,j) = ([], i, j)
bsort (lst, inswp, incmp)
    | hasNoSwaps = (lst, swaps, cmps)
    | otherwise = ((next ++ [last bubbled]), subswaps, subcounts)
    where
        (next, subswaps, subcounts) = bsort ((init bubbled), swaps, cmps)
        (bubbled, hasNoSwaps, swaps, cmps) = bubble (lst, inswp ,incmp)

        bubble :: (Ord a) => ([a], Int, Int) -> ([a], Bool, Int, Int)
        bubble ([x], i, j) = ([x], True, i, j)
        bubble ((x:y:xs), swaps, cmps)
            | x > y = (y : bubbled_x, False, newswaps_x+1, newcmps_x+1)
            | otherwise = (x : bubbled_y, swaps_y, newswaps, newcmps+1)
            where
                (bubbled_y, swaps_y, newswaps, newcmps) = bubble ((y:xs), swaps, cmps)
                (bubbled_x, swaps_x, newswaps_x, newcmps_x) = bubble ((x:xs), swaps, cmps)

Here's a refactored version using a State monad to handle the threading of the counters.

We start with the type of counters, the notion of being Instrumented (i.e. a computation with counters) and some basic combinators increasing the counters.

import Control.Monad.State

data Counters = Counters
  { swaps       :: !Int
  , comparisons :: !Int
  }

type Instrumented = State Counters

incrSwaps :: Instrumented ()
incrSwaps = do
  cnts <- get
  put $ cnts { swaps = swaps cnts + 1 }

incrComparisons :: Instrumented ()
incrComparisons = do
  cnts <- get
  put $ cnts { comparisons = comparisons cnts + 1 }

We can then write down a comparison operator which records the fact it was called by increasing the appropriate counter:

cmpGT :: Ord a => a -> a -> Instrumented Bool
cmpGT x y = do
  incrComparisons
  return $ x > y

Using the same idea, we can port your definition of bubble. Two things to be careful about: you need to use do blocks now and you shouldn't forget to call incrSwaps in the branch where you performed a swap:

bubble :: Ord a => [a] -> Instrumented (Bool, [a])
bubble []  = return (True, [])
bubble [x] = return (True, [x])
bubble (x : y : zs) = do
  xgty <- cmpGT x y
  if xgty
  then do
    incrSwaps
    rec <- snd <$> bubble (x : zs)
    return $ (False, y : rec)
  else do
    fmap (x :) <$> bubble (y : zs)

bsort is not that different (I don't post lastAndInit :: [a] -> (a, [a]) here as it's not important, it's in the gist)

bsort :: Ord a => [a] -> Instrumented [a]
bsort []  = return []
bsort xxs = do
  (hasNoSwaps, bubbled) <- bubble xxs
  if hasNoSwaps
  then return xxs
  else do
    let (y, ys) = lastAndInit bubbled
    next <- bsort ys
    return $ next ++ [y]

Finally, bsort' corresponds to the combination of an initial state and runState:

initialCounters :: Counters
initialCounters = Counters 0 0

bsort' :: Ord a => [a] -> ([a], Counters)
bsort' xs = bsort xs `runState` initialCounters


Improve my if-statement to prevent double checks I write the following version of an if-statement very often. And every time, I hate the way that I make it, but I don't know how to improve it.

For example, I have two ArrayList's. If one of them is not empty, then I convert it to a StringBuilder and send it per mail. The problem is, first I check if one of them is not empty to know if I should send a mail, and then I check it again to know which of them is not empty. Example code:

if(!array1.isEmpty() || !array2.isEmpty()){
   // I need to distinguish the StringBuilder
   StringBuilder array1string = new StringBuilder();
   StringBuilder array2string = new StringBuilder();
   // so one of them is not empty. but which? now I must double check it
   if(!array1.isEmpty()){
      for(String string : array1){
         array1string.append(string);
      }
   }
   if(!array2.isEmpty()){
      for(String string : array2){
         array2string.append(string);
      }
   }
   mail.sendMail("Array1: " + array1string == null?"-":array1string.toString()
      + ", Array2: " + array2string == null?"-":array2string.toString());
}

So is there a way to prevent this double check?

Your code can be shortened to:

if(!array1.isEmpty() || !array2.isEmpty()) {
    mail.sendMail("Array1: " + getContent(array1) + ", Array2: " + getContent(array2));
}

When you have:

String getContent(ArrayList<String> list) {
    if(!list.isEmpty()) {
        StringBuilder sb = new StringBuilder();
        for(String string : list) {
            sb.append(string);
        }
        return sb.toString();
    }
    return "-";
}