Patching Strings with Ed Scripts I'm using Ed scripts to track changes in a web app I'm developing. Ed scripts are probably the easiest kind of patch file for a computer to parse: detailed explanation of Ed scripts.

I quickly put together the Ruby code below with extremely little thought. I'd like to know what kind of high-level patterns I could use for a problem like this. I'm aware of state machines in an electrical engineering sense, but I'm not aware of the generally-accepted way to apply them in Ruby.

def self.apply_patch(old_text, patch)
  new_text_array = old_text.split("\n")
  patch_array = patch.split("\n")
  i=0
  current_line = 0
  while i < patch_array.length
    #grab the command
    patch_array[i].match(/(\d*),?(\d*)(\w)/)
    first_line = $1.to_i
    last_line = $2.to_i
    command = $3
    if first_line == 0
      first_line = current_line
    end
    if last_line == 0
      last_line = first_line
    end
    case command
    when "a"
      current_line = first_line
      i+=1
      while patch_array[i] != "."
        if patch_array[i] == ".."
          if patch_array[i+2] == "s/.//"
            new_text_array.insert(current_line, ".")
          else
            new_text_array.insert(current_line, "..")
          end
        else
          new_text_array.insert(current_line, patch_array[i])
        end
        current_line+=1
        i+=1
      end
    when "d"
      length = last_line - first_line
      for ii in 0..length
        new_text_array.delete_at(first_line-1)
      end
    when "c"
      length = last_line - first_line
      for ii in 0..length
        new_text_array.delete_at(first_line-1)
      end
      current_line = first_line-1
      i+=1
      while patch_array[i] != "."
        if patch_array[i] == ".."
          if patch_array[i+2] == "s/.//"
            new_text_array.insert(current_line, ".")
          else
            new_text_array.insert(current_line, "..")
          end
        else
          new_text_array.insert(current_line, patch_array[i])
        end
        current_line+=1
        i+=1
      end
    else
      if patch_array[i] == "s/.//"
      else
        raise "invalid ed command"
      end
    end
    i+=1
  end
  new_text = new_text_array.join("\n")
end

Strategy

The code would be more elegant if you made the apply_patch function more like a language interpreter.  Though it may be unavoidable that you have to treat the text as an array, you can treat the patch as a stream of instructions.  That would relieve you of the annoyance of having to increment i.

If your patch is already in string form, then you can turn it back into a stream using StringIO.  Chances are, you would be acquiring the patch from an input stream (such as STDIN), so you would use that stream directly instead.

To make that work, you would have to get rid of the if patch_array[i+2] == "s/.//" special case, which involves looking ahead by two lines.  In any case, you would be better off implementing support for s/.// as the string substitution command that it is meant to represent.

Command parsing

This feels like a lot of lines of code to parse the command line, especially to handle the fallback line numbers:

#grab the command
patch_array[i].match(/(\d*),?(\d*)(\w)/)
first_line = $1.to_i
last_line = $2.to_i
command = $3
if first_line == 0
  first_line = current_line
end
if last_line == 0
  last_line = first_line
end

You could simplify first_line and last_line by assigning the values correctly the first time.  To do that, you would need a regexp that produces a nil capture group when the number is missing.

For stricter validation, the regexp should be anchored by \A and \Z.  I suggest using a using the regexp to specify exactly what commands are supported, for validation.

If the command is unsupported, you should raise some kind of exception rather than raising a String object.  The exception message should include the bad command, to aid debugging.

Text manipulation

The name new_text_array is a bit cumbersome.  How about just text?  You don't need to assign the result to a new_text variable at the end of the function.

Instead of deleting a line at a time using delete_at, you can knock out an entire slice at once, using a Range for array indexing.

There is code duplication between the a command and the c command.  The key insight is that a c (change) is just a combination of d (deletion) and a (append).

Suggested solution

This is about half the original line count.

require 'stringio'

def self.apply_patch(old_text, patch)
  text = old_text.split("\n")
  patch = StringIO.new(patch)
  current_line = 1

  while patch_line = patch.gets
    # Grab the command
    m = %r{\A(?:(\d+))?(?:,(\d+))?([acd]|s/\.//)\Z}.match(patch_line)
    raise ArgumentError.new("Invalid ed command: #{patch_line.chomp}") if m.nil?
    first_line = (m[1] || current_line).to_i
    last_line = (m[2] || first_line).to_i
    command = m[3]

    case command
    when "s/.//"
      (first_line..last_line).each { |i| text[i - 1].sub!(/./, '') }
    else
      if ['d', 'c'].include?(command)
        text[first_line - 1 .. last_line - 1] = []
      end
      if ['a', 'c'].include?(command)
        current_line = first_line - 1
        while (patch_line = patch.gets) && patch_line.chomp! != '.'
          text.insert(current_line, patch_line)
          current_line += 1
        end
      end
    end
  end
  text.join("\n")
end


Insert node in sorted doubly linked list Description:

  Given a reference to the head of a doubly-linked list and an integer,
  create a new Node object having data value  and insert it into a
  sorted linked list.

Code:

Node SortedInsert(Node head, int data) {

    Node current  = head;
    Node previous = null;
    while (current != null && current.data < data) {
        previous = current;
        current  = current.next;
    }

    Node node = new Node();
    node.data = data;
    node.next = current;
    node.prev = previous;

    if (previous == null) {
        head = node;
    } else {
        previous.next = node;
    }

    if (current != null) {
        current.prev = node;
    }

    return head;
}

This solution required me to do a lot of head scratching which I fear would be difficult to do in front of an interviewer. Any other approach with fewer chances of mistakes?

Well, the problem statement is broken. Maybe this is a trick question? In the situation that the value to be inserted is at the beginning of the list (perhaps the list is empty, or the value is small), then you can't successfully control the changes needed to modify the head reference for everyone who has it.

The code you have implies that the possibly new head is always returned after each call, but you run a risk of creating multi-headed lists with multiple heads, if the code outside your function does not do the correct assignments with the head.

That sort of risk is not acceptable in a Java library/function, and thus your encapsulation is wrong, etc.

In front of an interviewer, I would recommend that you say something like: "Well, this function would not be useful in a Java context, the data should be encapsulated and the head node should not be exposed to the user. The function should have no return value, and should not take in the head node as a parameter, but instead the function should be a method on a class that encapsulates a sorted list"

Having said that, though, and looking at your code, your code does as good a job as can be done in the circumstances, and it has good style, naming, and formatting.

If I was really picky, I would suggest that you reverse the if/else condition:

if (previous == null) {
    head = node;
} else {
    previous.next = node;
}

to be:

if (previous != null) {
    previous.next = node;
} else {
    head = node;
}

because that makes it match the logic of the next condition:

if (current != null) {
    current.prev = node;
}

Otherwise, it's all good.


Structuring a reusable angular component with multiple communicating directives I have a nested JSON of multiple levels. The last level has the "series data" that is to be added to the highcharts options to render the chart data of interest.

I am sniffing the JSON and populating the options in each level which is set as a watched model in the controller and modifying the chart data, which is watched inside of the directive and chart updated on the change of these models.

I am looking to structure this as a reusable component and wondering on the best practice to do so. Seems to me, the "JSON selection" part and the "chart rendering" part can be best as their own directives that communicate amongst each other.

Do you concur or suggest an alternative approach?

Controller:

app.controller('MyCtrl',function($scope,Data,ForwardChart){
    http.get('scripts/lib/bds2.json').success(function(api_data){
        $scope.r_api_data = api_data.res;

        var available_options = [];
        var chosen_options= [];
        var current_json = $scope.r_api_data;
        for (var i=0; i<2; i++){
            var json_options = _.keys(current_json);
            available_options.push(json_options);
            chosen_options.push(json_options[0]);

            current_json = current_json[json_options[0]];
        }

        $scope.plot_data = function(){
            var bdfs_chart = {
                series : Data.get_serieses($scope.r_api_data,$scope.chosen_options)
            };
            $scope.bdfs_chart = $.extend(true,angular.copy(ForwardChart),bdfs_chart);
        };

        $scope.chosen_options = [];
        $scope.available_options = available_options;

        $scope.$watch('chosen_options', function(){
            $scope.plot_data();
        },true);

        $scope.chosen_options = chosen_options;
    })
});

Directive:

app.directive('intchart', function(){
    return {
        restrict: 'E',
        template: '

',
        scope: {
            chartData: "=chartId"
        },

        link: function(scope, element, attrs){
            var chartsDefaults = {
                chart: {
                    renderTo: element[0],
                    type: attrs.type || null,
                    height: attrs.height,
                    width: attrs.width
                },
                colors: attrs.color.split(",")
            };
            var chart = {};
            scope.$watch(function() { return scope.chartData; }, function(value){
                var chart_json = {};
                if (chart.hasOwnProperty('series') && chart.series.length>0){
                    for (var i = 0; i < value.series.length; i++) {
                        chart.series[i].setData(value.series[i].data);
                    }
                } else{
                    $.extend(true, chart_json, chartsDefaults, value);
                    chart = new Highcharts.Chart(chart_json);
                }
            }, true);
        }
    }
})

It's difficult to tell from these snippets exactly what you're trying to achieve, but I think the questions you have to ask are; how do you anticipate reusing this code? If reusability is the feature, what are the use cases?

If you anticipate needing multiple distinct controllers with a significant and coherent subset of the logic in the get request in the controller, the consider extracting it into a service, to save logic duplication.

Alternatively, if the contents of the controller are only needed for the directive, consider making the controller part of the directive, rather than requiring them to be correctly used in conjunction.

The directive looks good from what I can tell.


Custom matrix object in C++ I have been getting back into C++ today, after a few years of Python and R. I am completely rusty, but decided to create a matrix object to re-familiarise. In reality I wouldn't use this class, since I know Boost has matrix objects, but it's good practice!

It can be compiled with g++ -std=c++11 matrix.cpp -o m.

Any thoughts or comments are most welcome.

#include <iostream>
#include <vector>

class matrix {
    // Class: matrix
    //
    // Definition:
    // creates a 2d - matrix object as a vector of vectors and
    // populates with (int) zeros.
    //
    // Methods: get_value, assign_value, get_row.

    // create vector of vectors
    std::vector<std::vector<int> > m;
    public:
        // constructor for class, matrix dimension (rows=X, cols=Y).
        matrix( int X, int Y) {
        m.resize(X, std::vector<int>(Y, 0));
    }

    class row {
        //class for matrix row object. Pass in a
        // vector and overload `[]`.

        std::vector<int> _row;
        public:
            // constructor
            row(std::vector<int> r) : _row(r) {
            }

            // overload `[]` to return y element.
            // note `.at()` does a range check and will throw an error
            // if out of range
            int operator[]( int y) {
                return _row.at(y);
            }
    };

    // overload [] to return x element
    row operator[]( int x) {
        return row(m.at(x));
    }

    int get_value ( int x, int y ) {
        //  Function: get_value
        //  Definition: returns value `v` of element
        //  `xy` in matrix `M`.
        return m[x][y];
    }

    void assign_value ( int x, int y, int v ) {
        //  Function: assign_value
        //  Definition: Assigns value `v` to element
        //  `xy` in matrix.
        m[x][y] = v;
    }

    std::vector<int> get_row(int y, int X){
        // Function get_row
        // Definition: returns a vector object with row
        // of x-values of length X at y.
        std::vector<int> ROW;
        for ( int i=y; i<=y;i++){
            for (int j=0; j<=X-1;j++){
                ROW.push_back(m[i][j]);
            }
        }
        return ROW;
    }

};

int main(){

    // specify matrix dimensions
    int N = 10; // number of rows
    int M = 10; // number of cols

    // create a matrix object
    matrix mm(N,M);

    // print elements
    int i, j;
    for (i=0; i<=N-1;i++){
        for (j=0;j<=M-1; j++){
            std::cout << mm[i][j];
        }
        std::cout << std::endl;
    }

    // grab a value and print it to console
    std::cout << mm.get_value(0,0) << std::endl;

    // assign a new value (v = 1) to element (0,0)
    mm.assign_value(0,0,1);

    // re-print the updated matrix
    for (i=0; i<=N-1;i++){
        for (j=0;j<=M-1; j++){
            std::cout << mm[i][j];
        }
        std::cout << std::endl;
    }

    // `get_row()` test
    std::vector<int> R = mm.get_row(0, M);
    for( int i: R){
        std::cout << i << ' ';
    }
}

That's some nicely presented code.  I found it very easy to read and understand.
A vector of rows isn't the best structure for a matrix.  The reason is that each vector has its storage elsewhere, so you lose locality of access.  A better structure is a flat array (or vector) of elements, and a knowledge of the stride from one row to the next.  (we can make the stride be the same as the row length, for simplicity; separate members for width and stride can be useful in more advanced scenarios).
    std::vector<int> m;
    std::size_t width;

public:
    # Constraint: x * y must not overflow size_t
    matrix(std::size_t x, std::size_t y)
        : m(x*y, 0),
          width{x}
    {
    }

I've made the dimensions be size_t, as that's the natural type for a size or count in C++.
Now, when we need to index into the array, we need to multiply the y value by width and add x:
int get_value(std::size_t x, std::size_t y)
{
    return m[x + y*width];
}

We can improve on this, by returning a reference to the value.  Instead of having a "get" and "set" method, we have a single method (for now), and we can give it a more convenient name:
int& operator()(std::size_t x, std::size_t y)
{
    return m[x + y*width];
}

This means that instead of having to write
mm.assign_value(0,0,1);

we can instead use the more intuitive
mm(0,0) = 1;

Now it's time admit to a slight lie above.  We actually need two methods, because if we have a const matrix, we should be allowed to read, but not write, its elements.  So we also need:
const int& operator()(std::size_t x, std::size_t y) const
{
    return m[x + y*width];
}

For printing, it's helpful to provide an operator<<().  Mine would look like this:
friend auto& operator<<(std::ostream& os, const matrix& m)
{
    for (std::size_t row = 0;  row < m.height;  ++row) {
        for (std::size_t col = 0;  col < m.width;  ++col) {
            os << m.m[col + row*m.width] << ' ';
        }
        os << '\n';
    }
    return os;
}

I added a height member to make this easier.

With these changes, see how much easier it is to use:
#include <iostream>

int main()
{
    // create a matrix object
    matrix mm(4,6);

    // print elements
    std::cout << mm;

    // grab a value and print it to console
    std::cout << mm(0,0) << std::endl;

    // assign a new value (v = 1) to element (0,0)
    mm(0,0) = 1;

    // re-print the updated matrix
    std::cout << mm;
}

Here's the full version of matrix after my edits:
#include <ostream>
#include <vector>

class matrix {
    std::vector<int> m;
    std::size_t width;
    std::size_t height;

public:
    matrix(std::size_t x, std::size_t y)
        : m(x*y, 0),
          width{x},
          height{y}
    {
    }

    int& operator()(std::size_t x, std::size_t y)
    {
        return m[x + y*width];
    }

    const int& operator()(std::size_t x, std::size_t y) const
    {
        return m[x + y*width];
    }

    friend auto& operator<<(std::ostream& os, const matrix& m)
    {
        for (std::size_t row = 0;  row < m.height;  ++row) {
            for (std::size_t col = 0;  col < m.width;  ++col) {
                os << m.m[col + row*m.width] << ' ';
            }
            os << '\n';
        }
        return os;
    }
};

Further exercises

If you actually want a public get_row() (and get_column()), these will need new implementations, perhaps copying values.
Think about providing a get_subarray(x, y, width, height) to give a view of part of the matrix - you'll need new members for offset and stride.  See how we can now more easily implement get_row() and get_column() using this new method.
Make the matrix a template, so we can have elements of whatever type we choose, rather than only int.


add_const or remove_const based on another type Here is a type trait which - I feel - could fit in std next to std::add_const and std::remove_const:

#include <type_traits>

template <typename T>
struct set_constness_of {
    template <typename U>
    struct by {
        using type = typename std::conditional<
            std::is_const<U>::value,
            typename std::add_const<T>::type,
            typename std::remove_const<T>::type
            >::type;
    };
};

Note this is intentionally C++11 to minimize requirements for using it. One can obviously define set_constness_of_t in which by is a type rather than a struct with a type.

I've also written a tiny sample program (coliru.com) to ensure it runs, at least for some cases.

/* the above definition of set_constness_of goes here (paste it) */
#inlcude <iostream>

struct foo {
    void m() { std::cout << "Non-const\n"; }
    void m() const { std::cout << "Const\n"; }
};

template <class T> void call_m() { T().m(); }

int main() {
    call_m<foo>();
    using bar = const int;
    call_m< set_constness_of<foo>::by<bar>::type >();
}

Questions:

Is this correctly defined for all cases?
What do you think about placing the second template parameter as an inner struct's template param, rather than having two template parameters on the outer struct?
What do you think about the choice of naming?

Other comments are welcome.

I don't see any obvious omissions or bugs.

The naming and inner struct are neatly expressive (and remind me of some testing frameworks' precondition/assertion chaining).  It's not something that's done by the standard library, but I don't think it should be hugely controversial.

I might go with a naming like copy_const<foo>::from<bar> - or perhaps even the other way around: copy_const<bar>::to<foo>.  Or even both!  (Actually, I now wish std::is_assignable worked like that - it would be easier to remember which parameter is which!)

None of the above is a concrete suggestion - intended merely as food for thought!


Expand form at runtime using a template Purpose:

In an HTML page, a user can filter results, by setting conditions.

At the beginning, there is no condition displayed, but a button to add one.

The code for each condition is always the same, and is know during design.

I am trying to insert the template in the page in the most efficient and standard way.

Code:

I have added the template of the condition, with display:none at the bottom of the page, and when a new condition is requested, the template is cloned and added to the DOM

$("#addConditionButton").click(function() {
  $("#template")
 .clone()
 .attr('id','condition'+$(".condition").length)
 .show()
 .appendTo("#conditions")
})

      Filter conditions

    <button id="addConditionButton">
      Add Condition
    </button>

  <select>
    <option>&lt;</option>
    <option>&gt;</option>
    <option>=</option>
  </select>
  <input type='text'>

My concerns:

I am not sure that the method I used, hidden template then cloning is the best way to do this, in terms of performance, but mostly in terms of best practice, since I'm learning about this.

Thanks,

Your code looks good, and very clean. Best practices are usually subjective; there's no absolute best way to clone and insert an element. That being said, I would make two suggestions:
1. Be consistent with your HTML
Your conditions div doesn't have quotes around its ID, whereas your template div does. I'd recommend always surrounding IDs in quotes as a standard practice.
2. Swap the .show() and .appendTo() order
This suggestion is purely subjective, but semantically, I think it makes more sense to add the cloned element to the DOM first, then change its display property, rather than change its display property first (which won't have any affect yet anyway) and then add it to the DOM. Again, that's just my opinion.
Other notes
If you're interested in adding autofocus to your input elements (so that as a new element is added, it automatically gets focus) you could do that fairly easily by inserting this line just before you close your function:
[0].childNodes[3].focus();

Also, if you ever modify your code to where you need to access the cloned elements by ID, keep in mind that your cloned elements will start with condition2.  You're using the length to generate the number, so when you add the first element the length will be 2.


Checking if parentheses are balanced This script checks if parentheses are balanced. I wonder if there is something that can be improved here including ES6 features.

function isBalanced(str) {
    var stack = [];
    const allowedSymbols = '()[]<>';

    for(let i = 0, l = str.length; i < l; i++) {
        var c = str[i];
        var isOpen;
        var symbolPosition = allowedSymbols.indexOf(c);

        if(!~symbolPosition) {
            continue;
        }

        isOpen = symbolPosition % 2 ? false : true;

        if(isOpen) {
            stack.push(c);
        } else {
            if(stack.length < 1 || allowedSymbols.indexOf(stack.pop()) !== symbolPosition - 1) {
                return false;
            }
        }
    }

    return stack.length < 1;
}

console.log('()', isBalanced('()')); //true
console.log(')(', isBalanced(')(')); //false

I linked to a very related question in a comment, and this will repeat some points from my answer to that other question. Still, there are some other things here.

Don't do stack.push(c). Do stack.push(symbolPosition + 1) instead. I.e. push the expected position, not the current character. When you just push the character, you have to call indexOf again later to get that character's symbol position when you pop it off of the stack. By just pushing the position to the stack to begin with, you avoid that. And by adding 1 when pushing it, you get a cleaner conditional, since you can just do stack.pop() !== symbolPosition
If you do the above, you don't need stack.length < 1 before the stack.pop() call either. Nothing will break if the stack's empty; pop() will just return undefined which won't match any symbol position, and thus trigger the return false.
I'd skip the isOpen variable. Sure, it might help explain what that modulo operation means, but you only need the result once. I'd rather put the explanation in a comment, and skip the single-use variable.
Similarly, I'd skip the c variable, which - if you follow the first point above - is also only used once (and whose single-letter name doesn't help explain much), and just do indexOf(str[i]).
I'd change the last line to return stack.length === 0 just to be explicit. Yes the result is the same, but the intent is clearer in my opinion. "Is the stack empty?" versus "Is the stack's length less than 1?". You could also do return !stack.length but that's unnecessarily cryptic.

In the end you get something like:

function isBalanced(str) {
    var stack = [];
    const allowedSymbols = '()[]<>';

    for(let i = 0, l = str.length; i < l; i++) {
        var symbolPosition = allowedSymbols.indexOf(str[i]);

        if(!~symbolPosition) {
            continue;
        }

        if(symbolPosition % 2 === 0) {
            stack.push(symbolPosition + 1);
        } else {
            if(stack.pop() !== symbolPosition) {
                return false;
            }
        }
    }

    return stack.length === 0;
}

As others have pointed out, you can do some tweaking with regard to const vs let vs var, but I'll leave that to you.


Caching data on disk in iOS I've got an app that's got an activity/timeline like view. Since I don't want to retrieve the entire timeline every time, I'm caching all the events.

Currently, this is how I go about it:

- (void)saveEventArray:(NSMutableArray *)eventArray {
    // save hier in bg
    dispatch_async(kAsyncQueue, ^{
        __block int loopCount = 0;
        NSArray *copyEventArray = [eventArray copy];
        NSMutableArray *archiveArray = [NSMutableArray arrayWithCapacity:copyEventArray.count];

        if([copyEventArray count] == 0){ // If it's an empty array to save, it won't loop
            NSUserDefaults *userData = [NSUserDefaults standardUserDefaults];
            [userData setObject:[[NSMutableArray alloc]init] forKey:@"events"];
            NSLog(@"save is done, no events left in eventarray");
        }

        for (NSMutableDictionary *event in copyEventArray) {
            // PFFile isn't easy to encode, but UIImage is, so whenever we encounter a PFFile, we convert it to UIImage
            id imageFile = [event objectForKey:@"img"];
            if([imageFile isKindOfClass:[PFFile class]]){
                [imageFile getDataInBackgroundWithBlock:^(NSData *imageData, NSError *error) {
                    if (!error) {
                        UIImage *image = [UIImage imageWithData:imageData];
                        [event setObject:image forKey:@"img"]; // the PFFile is now replaced for an UIImage
                        NSData *eventEncodedObject = [NSKeyedArchiver archivedDataWithRootObject:event];
                        [archiveArray addObject:eventEncodedObject];
                        loopCount++;

                        if(loopCount == [copyEventArray count]){ // when done looping, save it all
                            NSUserDefaults *userData = [NSUserDefaults standardUserDefaults];
                            [userData setObject:archiveArray forKey:@"events"];
                            NSLog(@"save is done");

                        }
                    }
                }];
            } else {
                loopCount++;
                NSData *eventEncodedObject = [NSKeyedArchiver archivedDataWithRootObject:event];
                [archiveArray addObject:eventEncodedObject];

                if(loopCount == [copyEventArray count]){ // when done looping
                    NSUserDefaults *userData = [NSUserDefaults standardUserDefaults];
                    [userData setObject:archiveArray forKey:@"events"];
                    NSLog(@"save is done");
                }
            }

        }
    });

}

However, I noticed that this takes insane amounts of storage and the app grows very quickly in size when doing this. Is there a better/more efficient way for this?

This doesn't help with your primary concern of disk space, but you've got a high amount of unnecessary duplication in your code, as well as what appears to be some unnecessary checking and duplication.

We effectively see this block of code three times:

if(loopCount == [copyEventArray count]){ // when done looping, save it all
    NSUserDefaults *userData = [NSUserDefaults standardUserDefaults];
    [userData setObject:archiveArray forKey:@"events"];
    NSLog(@"save is done");
}

Moreover, as a result of your approach, there's actually a bug that could cause the entire method to be a wasted effort.  If the last object in copyEventArray is a PFFile object, but it has an error being fetched, your array won't save to user defaults at all.

And ultimately, this is code is probably a little bit hard to follow (even for you perhaps?)  In a background thread, we're iterating through an array and dispatching other background threads out (presuming getDataInBackgroundWithBlock actually does what it suggests it does).

The problem is, we're trying to asynchronously load a ton of images into the same array but then wait until we're completely done loading them all into the same array before we save that single array into NSUserDefaults.

So, with all of that said, I've got a few recommendations.

Have you tried storing your images purely as NSData objects?  Have you compared the size of NSData versus UIImage?
Consider as an option compressing your downloaded images.  Perhaps, realistically, it might be better if the version stored on the server is more compressed, but certainly a mobile device could compress the image.  Once you have a UIImage object, you can get an NSData object for the .png representation of that image, or you could get an NSData object for the .jpg representation of that image.  A .png will compress the image as much as possible without any loss.  A .jpg, you can specify the compression rate.
I highly recommend looking into Core Data.  This would greatly simplify the downloading and archiving process.  You could greatly improve the rate at which you pull the images back off the disk to display.  And it's probably more efficient than using NSUserDefaults.


Dynamic dropdown update for site reuse This is based (somewhat loosely) on the code written in this railscast. I'm planning to re-use it in different places throughout the site, so I made it a bit more generic rather than based on IDs. However, I'm certain there's probably room for improvement.

jQuery ->
  $('.org_type_choice').each ->
    cat_obj = $(this).find('.org_category')
    type_obj = $(this).find('.org_type')
    categories = cat_obj.html()

    org_type = type_obj.find(':selected').text()
    opts = $(categories).filter("optgroup[label='#{org_type}']").html()
    cat_obj.html(opts)

    type_obj.change ->
      org_type = $(this).find(':selected').text()
      opts = $(categories).filter("optgroup[label='#{org_type}']").html()
      cat_obj.html(opts)

I see a couple of small ways to improve this.

DRY (Don't Repeat Yourself) the code. The code that runs on load is identical to the change event handler. So make it a function you can call from both places.
Decouple it completely from the markup by dropping the "org-*" classes. It may not be "orgs" with "categories" and "types" that you're trying to select, but, say, "cars" with "makes" and "models". The basic idea is simply that there's a primary select element, and it scopes (filters) a secondary select element.

For the second point, I'd use a custom data-* attribute on the primary select. You might call it data-secondary, and let its value be the ID of the secondary select element. E.g.

<select data-secondary="categories">...</select>
<select id="categories">...</select>

Now you can find all select elements that have an associated secondary select element, by saying $("select[data-secondary]"). From there, you can find that secondary, and use the current logic. The markup declares the relationship and behavior, while the code can stay completely generic.

I end up with code like this:

jQuery ->
  $("select[data-secondary]").each ->
    primary   = $ this
    secondary = $ "##{primary.data 'secondary'}" # find the secondary
    items     = secondary.clone()                # clone the secondary into memory

    updateSecondarySelect = ->
      scope = primary.find(":selected").text()
      secondary.html items.find("optgroup[label='#{scope}']").html()

    primary.on "change", updateSecondarySelect # set up the event listener
    updateSecondarySelect()                    # do the initial filtering

(I've changed to CamelCase, but that's not important; use whatever style you prefer)

Some extra error checking would be good (e.g. what happens if the secondary isn't found? If there's no optgroup matching the selected option in the primary, should the secondary maybe just be hidden? etc).

Here's a demo


Re-implementing an Array Class in Python I gave an attempt at reinventing the wheel and recreate list methods using my own Array Class to broaden my understanding about lists. I would like advice regarding efficiency, and in implementing negative indexes, since my code only works for positive ones.

class Array:

    def __init__(self, List=[]):
        self.array = List

    def display(self):
        print(self.array)

    def len(self):
        array = self.array
        count = 0
        for _ in array:
            count += 1
        return count

    def append(self, value):
        array = self.array
        length = self.len()
        results = [None] * (length+1)
        for i in range(length):
            results[i] = array[i]
        results[length] = value
        self.array = results

    def search(self, value):
        array = self.array
        pos = -1
        for index in range(self.len()):
            if array[index] == value:
                pos = index
        return pos

    def insert(self, index, value):
        array = self.array
        length = self.len()
        results = [None] * (length+1)
        if index > length:
            raise IndexError
        elif index == length:
            self.append(value)
            results = self.array
        else:
            for i in range(length):
                if i == index:
                    for j in range(index + 1, length + 1):
                        results[j] = array[j-1]
                    results[index] = value
                    break
                else:
                    results[i] = array[i]
        self.array = results

    def delete(self, value):
        array = self.array
        length = self.len()
        results = [None] * (length-1)
        pos = self.search(value)
        if pos == -1:
            raise "Index Error: Element Not Found"
        else:
            for i in range(length):
                if i != pos:
                    results[i] = array[i]
                else:
                    for j in range(pos+1, length):
                        results[j-1] = array[j]
                    break
        self.array = results

    def pop(self):
        array = self.array
        length = self.len()
        results = [None] * (length-1)
        if length == 0:
            raise IndexError

        value = array[-1]
        for i in range(length-1):
            results[i] = array[i]
        self.array = results
        return value

Seems like cheating to re-implement a list using a list... I feel like the real challenge would be doing so without a list of any sort, say creating a linked list or a tree or something like that. That might be pedantic, but it'd clarify what your limitation are and thus what efficient solutions are available versus what you're making needlessly hard for yourself because it's fun.

Most of your functions are going to be very expensive because you're constantly copying memory around. A more common approach is to over-allocate space and then keep two values, "length" and "_allocated". The first is how many valid elements your array contains; the second is how much space you've reserved to store values. When you want to append X to your array, you can just assign X to self.array[self.length] and then increment self.length. If that would take you beyond what you've allocated, only then you do the expensive act of allocating a new chunk of memory (in your case, the [None] * (length + 1) line) and copying the data. To minimize the number of copies, it's common practice to double the length of the array each time you resize it (maybe up to some maximum, at which point you add on new 4K element blocks or whatever rather than multiplying, to prevent running out of memory prematurely). When inserting, then, you'd just need to shift the later values rather than copy everything; similarly, when popping values off, just decrement self.length to mark that last element as unimportant and available to be overwritten.

To complement that approach, if you want to optimize for mid-array insertions and deletions, you can maintain a parallel bit-array of which values are valid and which aren't. Iterating through the array becomes trickier (you need to zip the values with the validity flags and only return the valid ones), but deleting an item becomes cheaper since you only need to find the element and mark it as deleted (no shifting required), and inserting an item only requires shifting things until you find an unused element that can be overwritten instead of shifted.

Caching the length of the array is a good idea in general (though not necessary if you don't over-allocate). It requires extra code and care to keep synchronized with the rest of the array, but checking a list's length is a very common thing for users to want to do, and the O(N) computation each time can get painful.

As a general Python'ism, I'd say use def __len__ instead of def len and implement __get_item__, as well as any other magic methods that generally correspond to an array. For reference see this documentation page, and consider that most people think of an array as some combination of a "Collection" and "[Mutable]Sequence"


Python Automate the Boring Stuff Collatz exercise I'm looking for some feedback on my code for the collatz function exercise in the Python book Automate the Boring Stuff. It works, but doesn't feel very elegant to me. Is there a better way to approach it? I'm trying to avoid just looking at someone else's solution so general advice would be preferred.

def collatz(number):
 if (number %2 ==0 ) :#even
     newNumber=number//2
     print(newNumber);
     if newNumber!=1:
         collatz(newNumber);
     else:
         return (newNumber)
 elif (number % 2 == 1) : #odd
     newNumber=3*number+1
     print (newNumber)
     collatz(newNumber)
     return newNumber;

#Input validation loop
while True:
    try:
        number = int(input("Enter number:"))
    except ValueError:
        print("Enter a valid integer")
        continue
    else:
        if number < 1:
            print("enter a valid positive integer")
            continue
        break

collatz(number)

Some quick thoughts:

Linting the code will make it more easily readable to Python developers (including yourself when you get used to it).
elif (number % 2 == 1) : could just be else: because a number which is not even must be odd.
return (newNumber) should be just return newNumber.
The code below #Input validation loop should be in a main function, called if __name__ == '__main__'. That way it's possible to import your function for reuse in other code.
Rather than returning just the next number in the sequence you probably want to return an infinite iterator so that the caller can get more numbers trivially:

class Collatz():
    def __init__(self, start):
        self.number = start

    def __iter__(self):
        return self

    def __next__(self):
        if self.number in [0, 1]:
            raise StopIteration()
        if self.number % 2 == 0:
            self.number = int(self.number / 2)
        else:
            self.number *= 3
            self.number += 1
        return self.number

Use:

>>> print(list(Collatz(15)))
[46, 23, 70, 35, 106, 53, 160, 80, 40, 20, 10, 5, 16, 8, 4, 2, 1]


Calculate amount of mugs StackExchange should give away so I'll receive one Description

Calculate how many mugs StackExchange should give away(for graduation) to top reputed users on codereview.stackexchange.com so I'll also receive one. (output is divisible by 50).

Code

DECLARE @mugs INT,@rounded_mugs INT,@inc INT,@remainder INT;
SELECT  @mugs = COUNT(1) FROM Users WHERE Reputation >=
  (SELECT Reputation FROM Users WHERE DisplayName='JaDogg');
SET  @remainder = @mugs%50;
IF (@remainder = 0) SET @inc = 0 ELSE SET @inc=50-@remainder;
SELECT  @mugs+@inc AS 'AMOUNT OF MUGS TO GIVE AWAY';

Link

Calculate amount of mugs to give away so I'll receive one

Documentation

I think that what the query is doing is looking at your reputation rank, then rounding up to the nearest 50.  It would be nice to hit "edit description" and document that for the benefit of anyone else using Stack Exchange Data Explorer.

Generality

Data Explorer allows parameterization.  Use it.

Style

You shouldn't have to declare variables for such a simple query.  In SQL, try to say what you want.  Don't state the steps for calculating it.

A good way to avoid defining variables is to use Common Table Expressions (the WITH clause) instead.

Brevity

It would help to use the relevant functions RANK() and CEILING().

WITH UserRanks AS (
    SELECT DisplayName
         , RANK() OVER (ORDER BY Reputation DESC) AS RepRank
        FROM Users
)
SELECT *, 50 * CEILING((RepRank + 0.0) / 50) AS BatchedRepRank
    FROM UserRanks
    WHERE DisplayName = ##Username:string##;


Request for Comments: Singleton pattern implemented in Java I have several utility classes who should only ever have one instance in memory, such as LogHelper, CacheHelper, etc. One instance to rule them all.

I've never implemented Singleton in Java before, so I wanted to post my first pass and see if anybody had major issues with this, or just helpful critiques.  I want to make sure that my design will have the intended affect when I go to use these singleton classes throughout my codebase.

Singleton:

public class Singleton
{
    private static Singleton instance;

    protected Singleton()
    {
        super();
    }

    public static synchronized Singleton getInstance()
    {
        if(instance == null)
            instance = instantiate();

        return instance;
    }

    // Wanted to make this abstract and let subclasses @Override it,
    // but combining 'abstract' and 'static' is illegal; makes sense!
    protected static Singleton instantiate()
    {
        return new Singleton();
    }
}

LogHelper (a singleton since all my classes will use it to write messages to the same destinations):

public class LogHelper extends Singleton
{
    protected static LogHelper instantiate()
    {
        return new LogHelper(...blah...);
    }

    public LogHelper(...blah...)
    {
        // Initialize the singleton LogHelper
    }

    // More methods for logging, etc.
}

Here's how I'd like to use it throughout my codebase:

LogHelper logHelper = LogHelper.getInstance();
logHelper.logInfo("...");

What I'm worried about is that I may have inadvertently designed this wrong and heading down the wrong avenue. Can anybody weigh-in on my design?

Thanks!

The best pattern for singleton that is guaranteed to be thread safe and minimizes lock overhead (because it has none) is this:

public class Singleton {
    private Singleton() {}

    private static class InstanceHolder {
        static Singleton INSTANCE = new Singleton();
    }

    public static Singleton getInstance() {
        return InstanceHolder.INSTANCE;
    }
}

This is lazy initialized but carries no locking overhead. It is guaranteed safe by the Java Memory Model: The inner static class is not loaded until the first call to getIinstance() and all static initializtion is guaranteed to be completed before the class is used, therefore INSTANCE is fully constructed when first accessed.

Note: The double checked locking pattern is BROKEN - see this article that explains exactly why.


Count the frequency of items in an array The question is:

  Write a program that prompts the user to input ten values between 80 and 85 and stores them in an array. Your program must be able to count the frequency of each value appears in the array.

Is there any more efficient ways to present the count for frequency number between 80 to 85? Below is the sample code which had been done in the simplest ways to count the frequency for each number.

The part which needs more efficient ways to present:

 for(int i = 0; i < inputSize; i++) {
        if(inputValue[i] == 80) {
            tempCount80++;
        } else if(inputValue[i] == 81) {
            tempCount81++;
        } else if(inputValue[i] == 82) {
            tempCount82++;
        } else if(inputValue[i] == 83) {
            tempCount83++;
        } else if(inputValue[i] == 84) {
            tempCount84++;
        } else if(inputValue[i] == 85) {
            tempCount85++;
        } else {
            cout << "Error Accurs." << endl;
        }
    }

    cout << 80 << "      " << tempCount80 << endl;
    cout << 81 << "      " << tempCount81 << endl;
    cout << 82 << "      " << tempCount82 << endl;
    cout << 83 << "      " << tempCount83 << endl;
    cout << 84 << "      " << tempCount84 << endl;
    cout << 85 << "      " << tempCount85 << endl;

The original code is: -

#include <iostream>
using namespace std;

int main() {
    const int inputSize = 10;
    int inputValue[inputSize];
    int tempCount80 = 0;
    int tempCount81 = 0;
    int tempCount82 = 0;
    int tempCount83 = 0;
    int tempCount84 = 0;
    int tempCount85 = 0;

    for(int i = 0; i < inputSize; i++) {
        int tempValue = 0;
        cout << "Please enter a number between 80 and 85: ";
        cin >> tempValue;

        if(tempValue > 79 && tempValue < 86) {
            inputValue[i] = tempValue;
        } else {
            cout << "The number must be between 80 and 85" << endl;
            do {
                i--;
                break;
            } while(i > 0);
        }
    }

    for(int i = 0; i < inputSize; i++) {
        if(inputValue[i] == 80) {
            tempCount80++;
        } else if(inputValue[i] == 81) {
            tempCount81++;
        } else if(inputValue[i] == 82) {
            tempCount82++;
        } else if(inputValue[i] == 83) {
            tempCount83++;
        } else if(inputValue[i] == 84) {
            tempCount84++;
        } else if(inputValue[i] == 85) {
            tempCount85++;
        } else {
            cout << "Error Accurs." << endl;
        }
    }

    cout << 80 << "      " << tempCount80 << endl;
    cout << 81 << "      " << tempCount81 << endl;
    cout << 82 << "      " << tempCount82 << endl;
    cout << 83 << "      " << tempCount83 << endl;
    cout << 84 << "      " << tempCount84 << endl;
    cout << 85 << "      " << tempCount85 << endl;

    return 0;
}

The output is as expected, but need to find more efficient ways to solve the question.

Answer to the Performance Issue
Anytime there are a list of variables named NAMEi where i is an integer, there is a strong chance that a container such as std::array or std::vector should be used. Sometimes using a table rather than multiple if statements can improve performance.

Indexing into an array will prevent the repetitive code that is currently in the solution:

#include <array>

int main()
{
const int inputSize = 10;
const int frequencyCount = 6;
std::array<int, inputSize> inputValues;
std::array<int, frequencyCount> freqs;

    ...
}

The array freqs will contain the frequency of the occurrence, there are a couple of ways to index into the array freqs. One would be to subtract 80 from the input value.

This will reduce the multiple if statements into a simple increment of an item in an array. It will also reduce the amount of code necessary to print the frequencies.

When performance is an issue, prefer "\n" over std::endl. The use of std::endl flushes the output which may mean there is a system call for each use. A system call can add a great deal of time.

Remove the do/while loop in the error checking.

Use the Container Classes Provided by C++
The code currently appears to be C rather than C++. It is using the old style C arrays, C++ supplies an array container class as part of the standard library. Using the array container class would allow you to use iterators instead of indexes, at least for printing. Here is a second reference.

Avoid Using Namespace STD
If you are coding professionally you probably should get out of the habit of using the "using namespace std;" statement. The code will more clearly define where cout and other functions are coming from (std::cin, std::cout). As you start using namespaces in your code it is better to identify where each function comes from because there may be function name collisions from different namespaces. The function cout you may override within your own classes. This stack overflow question discusses this in more detail.

DRY Code
There is a programming principle called the Don't Repeat Yourself Principle sometimes referred to as DRY code. If you find yourself repeating the same code mutiple times it is better to encapsulate it in a function. If it is possible to loop through the code that can reduce repetition as well.

Complexity
The function main() is too complex (does too much). As programs grow in size the use of main() should be limited to calling functions that parse the command line, calling functions that set up for processing, calling functions that execute the desired function of the program, and calling functions to clean up after the main portion of the program.

There is also a programming principle called the Single Responsibility Principe that applies here. The Single Responsibility Principle states:

  that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

There are at least 3 possible functions in main().
  - Get the user input
  - Get process the frequencies
  - Print the frequencies

Magic Numbers
There are Magic Numbers in the main() function (79 and 86), it might be better to create symbolic constants for them to make the code more readble and easier to maintain. These numbers may be used in many places and being able to change them by editing only one line makes maintainence easier.

Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them. There is a discussion of this on stackoverflow.


A python default dictionary which seamlessly saves to disk I sometimes do experiments at work and separate the computation and the analysis so I can do the computation on a cluster and the analysis locally and sometimes in a Jupyter notebook. I wrote a class which allows me to save results to a hidden file as if it was a dictionary. The idea is to create an object specifying the name of the experiment and from there you can use it as a dictionary, and it is saved to disk so you can access it from other python files. I'd appreciate any thoughts since IO isn't my forte. I used python 2.7 but I think it should work for python 3.0

import os
import cPickle as pickle

class FileDict():

    def __init__(self, name, default = None):
        self.fpath = '.{}.fd'.format(name)
        self.default = default

    def __getitem__(self, key):
        if os.path.isfile(self.fpath):
            d = pickle.load(open(self.fpath))
            if key in d:
                return d[key]
        else:
            return self.default

    def __setitem__(self, key, value):
        if os.path.isfile(self.fpath):
            d = pickle.load(open(self.fpath))
            d[key] = value
        else:
            d = {key : value}
        pickle.dump(d, open(self.fpath, 'w'))

if __name__ == '__main__':
    test = FileDict('test', 0)
    print(test[1])
    test[1] = 'thing'
    print(test[1])
    print(test[2])

I think you should rather returns a default value if file exists but key is not found in it.
Opening file every time you want to get or set a value is expensive. Consider reading it in __init__ method, saving the data in handler registered using atexit module and adding a flush() method if for some reason you'd like to dump data right now. You could also add some no_cache option in __init__ to force saving thins right away?
pickle module is potentially insecure. Consider famous example import pickle; pickle.loads("cantigravity\n") If you open a maliciously prepared file with your class a weird things can happen. It should be used for internal used only, not for a general purpose class that can read any input.
What if multiple instances of FileDict will use the same file as source? Consider using tempfile module or generating unique names with uuid module, and then allowing returning generated the name with some method
Following collections.defaultdict example you might want to use a callable to generate a default value. You can always pass lambda: 0 if you want to have only one value returned


Raspberry Pi LED for new emails I'm learning how to use Python with the Raspberry Pi. I successfully followed the tutorial for how to have a Python script run on the Pi to check for new email and turn on an LED if any new messages are in an inbox. I wanted to give myself a challenge by adding a feature to blink a secondary LED to indicate the number of waiting messages. I played around for a while and got a script working, but would like to know if I implemented this correctly.

Am I using threading correctly? Is there an easier way to check a feed every n seconds, and if new messages are present, then continually run blink() until the next feed check?

#!/user/bin/env python

from threading import Thread, Event
from time import sleep
import feedparser
import RPi.GPIO as GPIO

GPIO.setmode(GPIO.BCM)
BLUE_LED = 17
BIG_LED = 23
GPIO.setup(BLUE_LED, GPIO.OUT)
GPIO.setup(BIG_LED, GPIO.OUT)

USERNAME = "user@gmail.com"
PASSWORD = "gzvioCNiAvFvKoqY"

class Repeat(Thread):
    def __init__(self,delay,function,*args,**kwargs):
        Thread.__init__(self)
        self.abort = Event()
        self.delay = delay
        self.args = args
        self.kwargs = kwargs
        self.function = function
    def stop(self):
        self.abort.set()
    def run(self):
        while not self.abort.isSet():
            self.function(*self.args,**self.kwargs)
            self.abort.wait(self.delay)

def blink(count):
    for x in range(0, count):
        print "blink", x + 1
        GPIO.output(BLUE_LED, True)
        sleep(.25)
        GPIO.output(BLUE_LED, False)
        sleep(.25)
    sleep(1) #pause between blinks

try:
    while True:

        print "Starting b"
        b = Repeat(1,blink,0)
        b.start()
        big_led = 0

        print "---> ---> Checking feed"
        messages = int(feedparser.parse("https://" + USERNAME + ":" + PASSWORD + "@mail.google.com/gmail/feed/atom")["feed"]["fullcount"])

        if messages >= 1:
            # turn on big LED
            print "Turning on Big LED"
            GPIO.output(BIG_LED, True)
            big_led = 1
            print "There are", messages, "unread email messages"
            b = Repeat(1,blink,messages)
            b.start()
        else:
            print "There are no new messages"
            if big_led == 1:
                print "Turning off Big LED"
                GPIO.output(BIG_LED, False)

        sleep(60) # check the feed every minute for new mail
        print "Stopping b"
        b.stop()
        b.join()
except (KeyboardInterrupt, SystemExit):
    b.stop()
    b.join()
    GPIO.cleanup()
    print 'Program Stopped Manually!'
    raise

That looks about right. Your thread doesn't do very much, though, so you might want to consider something like so (untested):

def blink(abort, delay, count):
    while not abort.isSet():
        for x in range(0, count):
            print "blink", x + 1
            GPIO.output(BLUE_LED, True)
            sleep(.25)
            GPIO.output(BLUE_LED, False)
            sleep(.25)
        sleep(1) #pause between blinks
        abort.wait(delay)

Then later

thread_stop = Event()
b = threading.Thread.run(target=blink, args=(thread_stop, 1, 0))

which prevents need for the whole class. It's not a suggestion as much as a note that you did more than you needed to.

Note that your blink should really be using abort.wait and should cooperate with the other abort.wait to have sensible timings.


Avoiding Boundaries `if` Cases When Filtering Image Matrix I have the following MATLAB Code which I want to optimize.
It's related to an image matrix (2D) which I want to filter.
Though it is MATLAB code, the same problem would arise in C code.

The problem is that I'm trying to access neighbors of the pixel which might cause problems at the boundaries of the image matrix.

vWeightsCoeff = zeros((2 * numRows * numCols), 1);

weightsIdx = -3;

% Filteration
for iColIdx = 1:numCols
    for iRowIdx = 1:numRows
        if(mod((iRowIdx + iColIdx), 2) == 0)
            continue;
        end

        weightsIdx = weightsIdx + 4;

        refPixel = mOutputImage(iRowIdx, iColIdx);

        if(iRowIdx < numRows)
            relPixel1 = mOutputImage((iRowIdx + 1), iColIdx);
            relWeight1 = vRangeTable(round((refPixel - relPixel1) * RANGE_RESOLUTION) + RANGE_CENTER_IDX);
        else
            relPixel1 = 0;
            relWeight1 = 0;
        end

        if(iRowIdx > 1)
            relPixel2 = mOutputImage((iRowIdx - 1), iColIdx);
            relWeight2 = vRangeTable(round((refPixel - relPixel2) * RANGE_RESOLUTION) + RANGE_CENTER_IDX);
        else
            relPixel2 = 0;
            relWeight2 = 0;
        end

        if(iColIdx < numCols)
            relPixel3 = mOutputImage(iRowIdx, (iColIdx + 1));
            relWeight3 = vRangeTable(round((refPixel - relPixel3) * RANGE_RESOLUTION) + RANGE_CENTER_IDX);
        else
            relPixel3 = 0;
            relWeight3 = 0;
        end

        if(iColIdx > 1)
            relPixel4 = mOutputImage(iRowIdx, (iColIdx - 1));
            relWeight4 = vRangeTable(round((refPixel - relPixel4) * RANGE_RESOLUTION) + RANGE_CENTER_IDX);
        else
            relPixel4 = 0;
            relWeight4 = 0;
        end

        weightsNormalizationFactor = PREDICT_NORMALIZATION_FACTOR / (relWeight1 + relWeight2 + relWeight3 + relWeight4);

        relWeight1 = relWeight1 * weightsNormalizationFactor;
        relWeight2 = relWeight2 * weightsNormalizationFactor;
        relWeight3 = relWeight3 * weightsNormalizationFactor;
        relWeight4 = relWeight4 * weightsNormalizationFactor;

        mOutputImage(iRowIdx, iColIdx) = mOutputImage(iRowIdx, iColIdx) - ((relWeight1 * relPixel1) + (relWeight2 * relPixel2) + (relWeight3 * relPixel3)+ (relWeight4 * relPixel4));

        vWeightsCoeff(weightsIdx:(weightsIdx + 3)) = [relWeight1; relWeight2; relWeight3; relWeight4];

    end
end

Just to clarify the variables:

mOutputImage is the image matrix.
numCols and numRows are the matrix dimensions.
vWeightsCoeff is the vector of the weights.

Since each pixel is related to 4 others yet working only on half the pixels its size is 2 times the number of pixels.

Now, the questions are:

Can I do something smart to go over the same pixels yet skip the first if which works like dividing the pixels as in chess board (White Black Board)?
Can I do something to avoid the if which secures I'm not stepping out of boundaries and yet go through the same pixels?

If someone comes which a completely different arrangement I will be open for that as well.

visiting black chess board squares

You might do a mod2 pre-calculation of mod(iRowIdx + iColIdx, 2) by creating a chess board boolean matrix before the loop, using matrix operations.

But much better would be to step iColIdx by 2. And have iRowIdx start at 1 or 2 as appropriate, and then step it by 2.

boundary checking

Your matrix now is C x R. Simply allocate a (C+2) x (R+2) matrix, so you have zeros running along all four sides. Then iterate over just the middle, rather than over the whole matrix as you are now. So iColIdx for example would start at 2 and stop one short of the end.

Doing this will delete a great many ifs, to give you Version2 of the code. You might even find that puts you in a position to produce Version3, which takes advantage of matrix operations instead of scalar operations, pushing much of the looping work down into matlab primitives, which would be faster.

style

It would have been helpful to see the method signature, but your English description that mOutputImage is the only return value worked well enough.

Typo: % Filteration

It would be natural to express the normalization e.g. like this:

    relWeight4 *= weightsNormalizationFactor;

Leave a blank after if, please.

The extra parentheses in mod and places like this:

        relPixel1 = mOutputImage((iRowIdx + 1), iColIdx);

are a little odd, you could safely elide them.


Handling HTTP requests and saving user to database I am using Play Framework and Slick. In userController.scala I am handling HTTP post requests and sending to personRepository.scala. Is this controller and method written in the correct way?

userController.scala

  def register = Action.async { request =>
    var user = new UserInsert("", "", "", "", "", "")
    request.body match {
      case AnyContentAsText(string) => user = userModel(Json.parse(string))
      case AnyContentAsJson(json) => user = userModel(json)
      case _ => BadRequest {
        "Bad Request: " + play.api.http.Status.BAD_REQUEST
      }
    }
    personRepository.create(user.email, user.provider, user.uid, user.facebook_token, user.display_name, user.photo_url).map(i => {
      Ok(Json.toJson(i))
    })
  }

  def userModel(json: JsValue) = {
    new UserInsert((json \ "email").as[String], (json \ "provider").as[String], (json \ "uid").as[String], (json \ "facebook_token").as[String], (json \ "display_name").as[String], (json \ "photo_url").as[String])
  }

personRepository.scala

  def create(
              email: String,
              provider: String,
              uid: String,
              facebook_token: String,
              display_name: String,
              photo_url: String) = db.run {
    val salt = current.configuration.getString("salt")
    val token = BCrypt.hashpw(email, salt.get)
    (user.map(p => (p.token, p.email, p.provider, p.uid, p.facebook_token, p.display_name, p.photo_url))
      returning user.map(p => p)
      ) +=(token, email, provider, uid, facebook_token, display_name, photo_url)
  }

I know that PersonRepository#create returns a Future[A] as that is the only signature that can make the map call at the end of register actually compile. I'll assume it returns a Future[UserInser], and therefore has a signature equivalent to :

class PersonRepository {
  def create(email: String, provider: String, uid: String, facebook_token: String, display_name: String, photo_url: String):Future[UserInsert] = ???
}

That I have to make this assumption at all shows a first issue in your code : the name i used in

personRepository.create(user.email, user.provider, user.uid, user.facebook_token, user.display_name, user.photo_url).map(i => {
      Ok(Json.toJson(i))
    })

expresses no intent and forces the reviewer to go and check the signature of create to know what's going on.

Assuming I am right that it returns the inserted user, it would be nice to improve the naming as such:

personRepository.create(user.email, user.provider, user.uid, user.facebook_token, user.display_name, user.photo_url).map(savedUser => {
      Ok(Json.toJson(savedUser))
    })

Apart from that, your code definitely doesn't do what you want as it will try to insert an empty user in the database if the request is incorrect.

This could be avoided along with the whole pattern matching by delegating to play using a form such as :

val userRegisterForm = Form(
      mapping(
        "email"->text,
        "provider"->text,
        "uid"->text,
        "facebook_token"->text,
        "display_name"->text,
        "photo_url"->text
      )(UserInsert.apply)(UserInsert.unapply)
    )

Your controller then becomes  :

  /**
    * Login/register action.
    */
  def register = Action.async { implicit request =>
      val maybeUser: Form[UserInsert] = userRegisterForm.bindFromRequest()
    maybeUser.fold(
      errors => Future.successful(BadRequest("Bad Request: " + play.api.http.Status.BAD_REQUEST)),
      user => personRepository.create(user.email, user.provider, user.uid, user.facebook_token, user.display_name, user.photo_url).map(savedUser => {
        Ok(Json.toJson(savedUser))
      })
    )
  }

I would also argue that

errors => Future.successful(BadRequest("Bad Request: " + play.api.http.Status.BAD_REQUEST))

doesn't make sense. It is just repeating bad request all over the place without providing any useful information. Just replace it with

errors => Future.successful(BadRequest)

or provide a meaningful error message

errors => Future.successful(BadRequest("Unable to parse request" + Json.prettyPrint(errors.errorsAsJson)))

which gives you :

  /**
    * Login/register action.
    */
  def register = Action.async { implicit request =>
      val maybeUser: Form[UserInsert] = userRegisterForm.bindFromRequest()
    maybeUser.fold(
      errors => Future.successful(BadRequest),
      user => personRepository.create(user.email, user.provider, user.uid, user.facebook_token, user.display_name, user.photo_url).map(savedUser => {
        Ok(Json.toJson(savedUser))
      })
    )
  }

Regarding the remainder of your code :

def userModel(json: JsValue) = {
    new UserInsert((json \ "email").as[String], (json \ "provider").as[String], (json \ "uid").as[String], (json \ "facebook_token").as[String], (json \ "display_name").as[String], (json \ "photo_url").as[String])
  }

should probably be made private.

Last but not least, the scala style guide says All public methods should have explicit type annotations. This should definitely be applied to PersonRepository#create

It can probably be improved further but that's a start.


__repr__ for a binary tree whose constructor has default arguments I recently have been working on a BinaryTree class, and looked into creating a proper __repr__ which when eval'ed would recreate the exact binary tree as the original.

The following code for __repr__ works as expected, but I'm wondering if there is a better way to handle the removal of values which already has a default parameter in the __init__ method of the class. That is, avoiding to use replace() to remove the None cases at the end of __repr__.

class BinaryTree(object):
    """Structure to hold a binary tree."""

    def __init__(self, value, left = None, right = None):
        self.value = value
        self.left = left
        self.right = right

    def __repr__(self):
       """Return a string which when eval'ed will rebuild tree"""

       return '{}({}, {}, {})'.format(
                 self.__class__.__name__,
                 repr(self.value),
                 repr(self.left) if self.left else None,
                 repr(self.right) if self.right else None) \
                      .replace(', None, None)', ')') \
                      .replace(', None)', ')')

    def __eq__(self, other):
        """Check if both trees exists, and have same value and subtrees."""

        # Return early if only one of the tree exists
        #   or if only one of them has a right tree
        #   or if only one of them has a left tree
        if (bool(self) ^ bool(other)
            or (bool(self.right) ^ bool(other.right))
            or (bool(self.left) ^ bool(other.left))):

            return False

        # Otherwise compare the values, and that sub trees are equal
        return (self.value == other.value
                and self.right == other.right
                and self.left == other.left)

    def __ne__(self, other):
        """Negated version of BinaryTree.__eq__"""
        return not self.__eq__(other)

Some test code verifying that it actually works:

trees = []
trees.append(('balanced manual', BinaryTree('D', BinaryTree('B', BinaryTree('A'), BinaryTree('C')),
                            BinaryTree('F', BinaryTree('E')))))

trees.append(('alphalevel manual', BinaryTree('A', BinaryTree('B', BinaryTree('D'), BinaryTree('E')), BinaryTree('C', BinaryTree('F')))))
trees.append(('integer_tree manual', BinaryTree(4, BinaryTree(2, BinaryTree(1), BinaryTree(3)), BinaryTree(6, BinaryTree(5)))))
trees.append(('strange_tree manual',
               BinaryTree('G',
                 BinaryTree('C',
                   BinaryTree('B',
                     BinaryTree('A')),
                   BinaryTree('F',
                     BinaryTree('D',
                       None,
                       BinaryTree('E')))),
                 BinaryTree('H',
                   None,
                   BinaryTree('I')))))

for tree_name, tree in trees:
    print('Treename: {}, equal to __repr__ version: {}'.format(tree_name,
                tree == eval(repr(tree))))

I do know that it doesn't hurt to explicitly specify the default parameters when eval'ing the repr version, but I'm contemplating on using some the repr-versions in tests and so on, and the version without None looks a little nicer than the one with:

BinaryTree('D', BinaryTree('B', BinaryTree('A'), BinaryTree('C')), BinaryTree('F', BinaryTree('E')))
BinaryTree('D', BinaryTree('B', BinaryTree('A', None, None), BinaryTree('C', None, None)), BinaryTree('F', BinaryTree('E', None, None), None))

One thing you can simplify is this subexpression:

repr(self.left) if self.left else None

to just:

repr(self.left)

since repr(None) == str(None) == 'None'. Just saves some typing.

Furthermore, when you are doing:

'({}, {}, {})'.format(repr(a), repr(b), repr(c))

that is equivalent to just repring the tuple:

repr((a, b, c))

Other than that, is it worth the added complexity of the replaces? It's not the end of the world that:

repr(BinaryTree(2)) == 'BinaryTree(2, None, None)'

I'd say just keep it simple:

return '{}{}'.format(
          self.__class__.__name__,
          repr((self.value, self.left, self.right)))


Expanding a map into an infinite sequence I have a map that I want to 'expand' into an infinite sequence in the following manner:

{0 'zero 3 'three 10 'ten}
=>
('zero 'zero 'zero 'three 'three 'three 'three 'three 'three 'three 'ten 'ten 'ten ...)

The indexes of the map indicating the index of the sequence where the value should change.

The following code works, but it does not please me.  Can this be written in a smarter way?

(defn expand-map
  [m]
  (letfn [(em
           [last-value indexes]
           (let [value (m (first indexes))]
                (if value
                  (lazy-seq (cons value (em value (rest indexes))))
                  (lazy-seq (cons last-value (em last-value (rest indexes)))))))]
    (em 'zero (iterate inc 0))))

(take 20 (expand-map {0 'zero 3 'three 10 'ten}))

It really depends on your specific goals.  The code as it is is very
lazy, but also recomputes quite a lot by accessing the map on every
single step.

(iterate inc 0) can be more easily written as (range) as noted again
below and the definition of expand-map is partially repetitive
((lazy-seq (cons ...)) comes up twice, as does (rest indexes) -
generally it's better to keep the amount of duplicate code to a minimum.

The 'zero initial value is hardcoded, which is not that nice for a
general solution, but apart from that it looks okay.

I have two other rewritten options below with different degrees of
laziness and verboseness.

For both options I suggest a different way of iterating over the keys,
that is, getting and sorting the keys of the input map once, then
reusing it instead of looking up the current index that often.

Thus, option one, lazier then the other one, using repeat to create
the sub-list with the indicated value, then concatenating it with the
remainder.

Also, since it's not indicated what should happen on an empty map
(except the presented code defaulting to 'zero); that could be
indicated a bit nicer as well, by throwing a custom error perhaps.

(defn expand-map-2 [m]
  (letfn [(aux [indexes]
            (let [index (first indexes)
                  rest (next indexes)
                  value (m index)]
              (if rest
                (lazy-cat (repeat (- (first rest) index) value) (aux rest))
                (repeat value))))]
    (aux (sort (or (keys m) (throw (Exception. "No specification supplied.")))))))

The other way, less lazy, but IMO a bit nicer assuming your input map
isn't huge, is to precompute all lazy lists and concatenate them.  Also
uses the mapcat over two shifted maps.  The
(concat (rest keys) [nil]) seems necessary since mapcat terminates
early instead of till all the collections are exhausted.

(defn expand-map-3 [m]
  (let [keys (sort (or (keys m) (throw (Exception. "No specification supplied."))))]
    (mapcat (fn [first second]
              (let [value (m first)]
                (if second (repeat (- second first) value) (repeat value))))
            keys (concat (rest keys) [nil]))))

Since this post was migrated, there are actually a number of existing
solutions here.  I've already asked for one of them and got the go-ahead
to incorporate it with an explanation of the solution.

Courtesy of @amalloy:

(defn expand-map-4 [m]
  (rest (reductions (fn [prev idx]
                      (get m idx prev))
                    'zero
                    (range))))

Benchmarked this solution seems very similar to the above ones (and the
initial code), but is much more concise.  range will generate the the
infinite list instead of (iterate inc 0), so that is already nice.
reductions will return all intermediate results will reducing with the
provided function; since we start from 'zero here, this works
similarly to the code in the question (I'd rather replace this with
similar behaviour to what I posted above, since it doesn't rely on the
hard-coded 'zero though).  get is used instead of using the map
directly, but that's not actually necessary.  The first rest is used
to discard the spurious initial value (the supplied 'zero).

With those remarks in mind, I'd suggest this amended version:

(defn expand-map-5 [m]
  (let [keys (or (keys m)
                 (throw (Exception. "No specification supplied.")))]
    (rest (reductions (fn [prev idx]
                        (m idx prev))
                      (m (first (sort keys)))
                      (range)))))


Query string substitutions for links on a page If the current page URL has an argument 'myid1' or 'myid2' in the querystring, for each link in my webpage with class 'rewrite', I want the link href's querystring to be replaced by the current page URL's querystring. I'm using the code given below. Since I'm new to javascript, I'm not sure if its optimized. I want it to execute as fast as possible.

Hyperlink

The code you provide is inefficient in two ways:

Unnecessary loops. It loops through a.rewrite anchor each time for one querystring match. It could be optimized into one loop;
Repeated calculation on regular expression matches. regexS is executed for each gup function and it could be reduced into one calculation.

The solutions are:

Fetch the window.location.href data into one variable which could be referred to later;
Integrate the two (or more) loops into one and finish all the replacement in one loop.

Here the optimized code goes:

//First you fetch the query string as key-value pairs in the window.location.href, this equals your gup function.

//This code, fetch the ?key1=value1&key2=value2 pair into an javaScript Object {'key1': 'value1', 'key2':'value2'}
var queryString = {};
var queryStringPattern = new RegExp("([^?=&]+)(=([^&]*))?", "g");
window.location.href.replace(
    queryStringPattern,
    function($0, $1, $2, $3) { queryString[$1] = $3; }
);

//Second you collect all the anchor with class rewrite and execute the replacement.

$("a.rewrite").each(function () {
  this.href.replace(
    queryStringPattern,
    function ($0, $1, $2, $3) {
      return queryString[$1] ? $1 +  "=" + queryString[$1] : $1 + '=' + $3;
    }
  )
});


Minimal substring with all characters contained in string I was given following task in the interview:

  Given a string, find shortest substring in it that contains all of the different characters contained in the original string.

Here is my solution in emacs lisp:

(defun add-to-hash (c ht)
  "Add an instance of character C to hash table HT"
  (puthash c (1+ (gethash c ht 0)) ht))

(defun remove-from-hash (c ht &optional try)
  "Remove an instance of character C from hash table HT
Return nil if no instances of C remain in HT"
  (let ((old (gethash c ht 0)))
    (if (and try (< 2 old))
        nil
      (if (> 1 (puthash c (1- (gethash c ht 0)) ht))
          (remhash c ht)
        t))))

(defun find-min-substring (sstr)
  "Find minimal substring that contains all the characters in a given string"
  ;; get all characters
  (let* ((all-chars (make-hash-table))
         (slen (length sstr))
         (fcnt (progn
                 (mapc (lambda (c) (add-to-hash c all-chars)) sstr)
                 (hash-table-count all-chars)))
         (beg 0) (end fcnt)
         (res sstr))
    (if (= end slen)
        res
      (let* ((cand (substring sstr beg end))
             (cand-chars (make-hash-table))
             (ccnt (progn
                     (mapc (lambda (c) (add-to-hash c cand-chars)) cand)
                     (hash-table-count cand-chars))))
        ;; find first candidate, that is a substring with all the characters
        (while (< ccnt fcnt)
          (add-to-hash (aref sstr end) cand-chars)
          (setq end (1+ end))
          (setq ccnt (hash-table-count cand-chars)))
        (setq cand (substring sstr beg end))
        ;; shorten it as much as possible
        (while (remove-from-hash (aref sstr beg) cand-chars t)
          (setq beg (1+ beg)))
        (setq cand (substring sstr beg end))
        (setq res cand)
        ;; check other variants
        (while (< end slen)
          ;; advance both ends
          (add-to-hash (aref sstr end) cand-chars)
          (setq end (1+ end))
          (remove-from-hash (aref sstr beg) cand-chars)
          (setq beg (1+ beg))
          (setq ccnt (hash-table-count cand-chars))
          ;; another candidate is found
          (when (= ccnt fcnt)
            ;; only change candidate if it was shortened
            (while (remove-from-hash (aref sstr beg) cand-chars t)
              (setq beg (1+ beg))
              (setq cand (substring sstr beg end)))
            (setq res cand)))
        res))))

I know this is old, but I will write some remarks anyways, hope you will find it useful.

This code lacks knowledge of usual language shortcuts, eg. (if x nil y) is equivalent to (unless x y). Similarly, (setq x (1+ x)) is the same as (incf x), (setq x y) (setq z q) is the same as (setq x y z q) inside (implicit) progn. (puthash c (1- (gethash c ht 0)) ht) is the same as (decf (gethash c ht 0)).
Language offers specialized data-structure for working with characters--char-table.
It is better to express algorithmic code in terms it is easy for humans to interpret (i.e. leave the low-level details of implementation to helper functions, while giving helpers meaningful names. For example add-to-hash isn't particularly better than puthash. Perhaps better partitioning of your code would involve functions like unique-characters-of, generate-candidate etc.

Finally, and most importantly, your code doesn't really solve the problem, try this for instance:

(find-min-substring "aaaaabacbaaaaac") ; => "acbaaaa"

Below is an alternative solution.  It is very straight-forward (probably inefficient), but (hopefully) correct:

(defun string-cost (s)
  (cl-loop
   with mask = (make-vector 128 0)
   for c across s do
   (setf (aref mask c) 1)
   finally (cl-return (cl-reduce '+ mask))))

(defun generate-candidates (s)
  (list (substring s 0 (1- (length s)))
        (substring s 1)))

(defun choose-candidate (a b min-cost)
  (let ((a-candidate (min-common-substring a min-cost))
        (b-candidate (min-common-substring b min-cost)))
    (if (< (length a-candidate) (length b-candidate))
        a-candidate
      b-candidate)))

(defun min-common-substring (s &optional min-cost)
  (unless min-cost (setf min-cost (string-cost s)))
  (cl-destructuring-bind (a b)
      (generate-candidates s)
    (let ((a-cost (string-cost a))
          (b-cost (string-cost b)))
      (cond
       ((and (>= a-cost min-cost)
             (>= b-cost min-cost))
        (choose-candidate a b min-cost))
       ((>= a-cost min-cost)
        (min-common-substring a min-cost))
       ((>= b-cost min-cost)
        (min-common-substring b min-cost))
       (t s)))))

Where more involved, but slightly more efficient version of string-cost could look like this:

(defun string-cost (s)
  (cl-loop
   with mask = (make-char-table t 0)
   for c across s do
   (set-char-table-range mask c 1)
   finally
   (cl-return
    (let ((cost 0))
      (map-char-table
       #'(lambda (k v)
           (when (and (/= v 0) (consp k))
             (setf v (- (cdr k) (1- (car k)))))
           (incf cost v))
       mask)
      cost))))


velocity profile of laminar flow in a pipe for a university project I want to compute the laminar flow velocity at 7 points in a cross-section of a pipe. Later we want to use this data for a flow animation.

Inputs of my function are the diameter and length of the pipe, the viscosity of the fluid, the pressure difference and the average velocity.

For the calculation I only need the pressure difference or the average velocity. So I have to set either the pressure difference or the average velocity to False.

Do you have any idea how I can improve the code?

import numpy as np

def flow(diameter, viscosity, len_pipe, dp, v_average):
    radius = diameter / 2.
    x = np.linspace(-radius, radius, 7)

    if not v_average and dp:
        dpdx = dp / float(len_pipe)
        v = radius**2 / (4*viscosity) * (-dpdx) * (1-(x/radius)**2)
        vmax = max(v)
        v_average = vmax / 2
    elif not dp and v_average:
        vmax = 2 * v_average
        dpdx = -4 * vmax * viscosity / radius**2
        dp = dpdx * len_pipe
        v = radius**2 / (4*viscosity) * (-dpdx) * (1-(x/radius)**2)

    return dp, v_average, x, v, vmax

I think this is a good starting point, but some better code organization will help you in the long run

I would first summarize a few main problems I see:

Unclear input parameters
Ambiguous path execution
The most important part is hard-coded
Too many output parameters
Repetition of code (i.e. violation of DRY)

All of this basically boils down to code organization.  Rather than just showing you a different solution, I think its best to explain the issues:

The usage of db and v_average is such that it isn't immediately clear how to use your method.  It's best if a user can look at your method and see exactly how it is supposed to be called.  Some docstrings would help.  Also, since these parameters are optional, they should be named parameters, not positional ones: def flow(diameter, viscosity, len_pipe, dp=False, v_average=False).  Otherwise they have to pass in False to dp in order to use v_average, and that is less than ideal.
Your code will break if someone passes False in both for dp and v_average.  You want a nice guard up top: if not dp and not v_average: raise ValueError('You must provide either dp or v_average)  This makes your code self-documenting, and avoids bugs.  Also, you should have a guard to return an error if both have a value.  That is just as much a user mistake, and will have an ambiguous return value if the two passed in values don't agree.
The post important thing in your code is the locations where you want the flow velocity.  This is the entire point of your function: to calculate flow velocity at some points.  However, you have the points hard-coded.  Sure, you want it at 7 points right now, but what if your needs change?  You have to re-write your function.  That's no good.  Just let x be an input parameter, and let the user provide it.
You are returning too many things as output parameters.  This is basically a violation of separation of concerns.  All you should really be returning is v, which is easier if the user passes in x.  You don't have to return dp or v_average.  Effectively, your function is doing 5 things: calculating some x values of interest, calculating dp if v_average is provided, calculating v_average if dp is provided, calculating v, and calculating vmax.  You need a function that does less.

Also, while I don't know much about laminar flows, the statistician/physicist in me says that you are using a strange definition of average velocity: v_average = vmax / 2.  I'm going to assume for now that v_average doesn't refer to an actual statistical average, but that it has a different definition in your particular use-case.

Small Changes

So what do you do about this?  Personally, I think you should just separate this into two separate functions: flow_from_dp and flow_from_v_average.  Pass in x, lose the extraneous input parameter, and return only v.  If the user wants to figure out vmax, then make a separate function: calculate_vmax.  Similarly, I would make helper functions that can calculate dp from v_average and vice-versa.  Having more functions that do less is actually a good thing in this case.  Each function will have a clear calling sequence and return only one thing.  That is exactly what you want.  I'll leave actually making that change as an exercise for the reader :)

Big Changes

Another option would be to make this code OOP.  I would envision a pipe class that has a diameter and length.  You could also have a fluid class that has a viscosity and dp or v_average.  It would then automatically calculate the one given the other.  Your flow method would then become bound to one of the classes, or it could be a method that simply accepts a pipe and a fluid and calculates the velocity at a given set of points.  I'm not going into a crazy amount of detail here simply because I don't know whether or not that might be a useful suggestion for you, as that largely depends on your level of familiarity with OOP.  If you are interested, I would be happy to give an example of what an OOP solution would look like.


Generic paging algorithm I have made a simple pagination algorithm. It shows "next" and "previous" buttons when appropriate.

It also limits the amount of buttons if the page amount exceeds a certain amount, e.g. 1 2 ... 15 16 17 18 19 ... 99 100.

It works fine, but I feel it can be simplified.

function appendTruncateButtons(buttons, pageCount) {
  buttons.push("...");
  buttons.push(pageCount - 1);
  buttons.push(pageCount);
}

function prependTruncateButtons(buttons) {
  buttons.push(1);
  buttons.push(2);
  buttons.push("...");
}

function generateButtons(currentPage, pageCount, surroundingPageLinkSize) {
  var buttons = [];

  if (currentPage > 1) {
    buttons.push("<");
  }

  var truncate = pageCount > (7 + (surroundingPageLinkSize * 2));
  if (!truncate) {
    for (var i = 1; i <= pageCount; i++) {
      buttons.push(i);
    }
  }
  else {
    var truncateEnd = currentPage < (1 + (surroundingPageLinkSize * 2)),
        truncateBoth = (pageCount - (surroundingPageLinkSize * 2)) > currentPage && currentPage > (surroundingPageLinkSize * 2);

    if (truncateEnd) {
      for (var j = 1; j < (4 + (surroundingPageLinkSize * 2)); j++) {
        buttons.push(j);
      }
      appendTruncateButtons(buttons, pageCount);
    }
    else if (truncateBoth) {
      prependTruncateButtons(buttons);
      for (var k = (currentPage - surroundingPageLinkSize); k <= (currentPage + surroundingPageLinkSize); k++) {
        buttons.push(k);
      }
      appendTruncateButtons(buttons, pageCount);
    }
    else { // Truncate start
      prependTruncateButtons(buttons);
      for (var l = pageCount - (2 + (surroundingPageLinkSize * 2)); l <= pageCount; l++) {
        buttons.push(l);
      }
    }
  }

  if (currentPage < pageCount) {
    buttons.push(">");
  }

  return buttons;
}
// generateButtons(currentPage, pageCount, surroundingPageLinkSize)
console.log(generateButtons(1, 100, 3));
console.log(generateButtons(50, 100, 3));
console.log(generateButtons(99, 100, 3));

var truncate = pageCount > (7 + (surroundingPageLinkSize * 2));
  if (!truncate) {
    for (var i = 1; i <= pageCount; i++) {
      buttons.push(i);
    }
  }
  else {
    var truncateEnd = currentPage < (1 + (surroundingPageLinkSize * 2)),
        truncateBoth = (pageCount - (surroundingPageLinkSize * 2)) > currentPage && currentPage > (surroundingPageLinkSize * 2);

    if (truncateEnd) {
      for (var j = 1; j < (4 + (surroundingPageLinkSize * 2)); j++) {
        buttons.push(j);
      }
      appendTruncateButtons(buttons, pageCount);
    }
    else if (truncateBoth) {
      prependTruncateButtons(buttons);
      for (var k = (currentPage - surroundingPageLinkSize); k <= (currentPage + surroundingPageLinkSize); k++) {
        buttons.push(k);
      }
      appendTruncateButtons(buttons, pageCount);
    }
    else { // Truncate start
      prependTruncateButtons(buttons);
      for (var l = pageCount - (2 + (surroundingPageLinkSize * 2)); l <= pageCount; l++) {
        buttons.push(l);
      }
    }
  }

Note that all of these cases have essentially the same for loop.

  var start = 1,
      end = pageCount;

  if (currentPage > surroundingPageLinkSize * 2) {
      start = currentPage - surroundingPageLinkSize;
      prependTruncateButtons(buttons);
  }

  if (currentPage < pageCount - surroundingPageLinkSize * 2 - 2) {
      end = currentPage + surroundingPageLinkSize;
  }

  for (var i = start; i <= end; i++) {
      buttons.push(i);
  }

  if (end < pageCount) {
      appendTruncateButtons(buttons);
  }

Now we call it just once, only changing the parameters.

We also only call the prepend and append methods once each.  You could even pull those into this method if you wanted, as they are no longer repeated code.

I also like that this gets rid of the three single use Boolean variables.  Since they weren't reused, they don't seem necessary.

I'm not sure if the output is an exact match for what you had originally, but it should give the same kind of display.

I didn't change it here, but consider changing buttons to pages or even pageNumbers.  These are the pages that you want to make links or buttons that go there.  But they aren't buttons yet.


Bank account model implementation in Django There is a draft of my models.py. What can I do for code quality and readability increase?

from datetime import timedelta

from django.contrib.auth.models import User
from django.db import models
from django.utils import timezone

class Rule(models.Model):
    start_date = models.DateField()
    end_date = models.DateField()

    deposit_percent = models.FloatField()
    credit_percent = models.FloatField()

    @staticmethod
    def on_date(_date):
        return Rule.objects.get(start_date__lte=_date, end_date__gte=_date)

class BankAccount(models.Model):

    LEGAL = 1
    NATURAL = 2

    DEBET = 1
    CREDIT = 2

    owner_type = models.PositiveSmallIntegerField(choices=(
        (LEGAL, 'Физическое лицо'),
        (NATURAL, 'Юридическое лицо'),
    ))

    method = models.PositiveSmallIntegerField(choices=(
        (DEBET, 'Дебет'),
        (CREDIT, 'Кредит'),
    ))

    name = models.CharField(max_length=254)
    owner = models.ForeignKey(User, related_name='w_accounts')
    spectators = models.ManyToManyField(User, related_name='r_accounts')
    when_opened = models.DateField()

    @property
    def balance(self):
        assert self.method == self.DEBET
        today = timezone.now()
        rule = Rule.on_date(self.when_opened)
        result = 0
        current_date = self.when_opened
        while current_date <= today:
            result -= sum(x.total for x in self.cashing_set.on_date(current_date))
            result -= sum(x.total for x in self.outcome_transfers.on_date(current_date))
            result += sum(x.total for x in self.deposit_set.on_date(current_date))
            result += sum(x.total for x in self.income_transfers.on_date(current_date))
            result *= 1 + rule.deposit_percent * 0.01
            current_date += timedelta(days=1)
        return result

    @property
    def debt(self):
        assert self.method == self.CREDIT
        today = timezone.now()
        rule = Rule.on_date(self.when_opened)
        result = 0
        current_date = self.when_opened
        while current_date <= today:
            result += sum(x.total for x in self.cashing_set.on_date(current_date))
            result += sum(x.total for x in self.outcome_transfers.on_date(current_date))
            result -= sum(x.total for x in self.deposit_set.on_date(current_date))
            result -= sum(x.total for x in self.income_transfers.on_date(current_date))
            result *= 1 + rule.credit_percent
            current_date += timedelta(days=1)
        return result

class OperationManager(models.Manager):

    def on_date(self, date):
        return (super(OperationManager, self).get_queryset()).filter(when__gte=date, when__lte=date)

class MoneyTransfer(models.Model):
    sender = models.ForeignKey(BankAccount, related_name='outcome_transfers')
    receiver = models.ForeignKey(BankAccount, related_name='income_transfers')
    when = models.DateTimeField()
    total = models.FloatField()
    comment = models.CharField(max_length=255)

    objects = OperationManager()

class Deposit(models.Model):
    account = models.ForeignKey(BankAccount)
    total = models.FloatField()
    banker = models.ForeignKey(User)
    when = models.DateTimeField()

    objects = OperationManager()

class Cashing(models.Model):
    account = models.ForeignKey(BankAccount)
    total = models.FloatField()
    banker = models.ForeignKey(User)
    when = models.DateTimeField()

    objects = OperationManager()

In general this seems fine, easy to read, you seem to be following PEP8, that's excellent.

At first look, this repeated logic in balance and debt jumps in the eye:

@property
def balance(self):
    # ...
    while current_date <= today:
        result -= sum(x.total for x in self.cashing_set.on_date(current_date))
        result -= sum(x.total for x in self.outcome_transfers.on_date(current_date))
        result += sum(x.total for x in self.deposit_set.on_date(current_date))
        result += sum(x.total for x in self.income_transfers.on_date(current_date))
        # ...

@property
def debt(self):
    # ...
    while current_date <= today:
        result += sum(x.total for x in self.cashing_set.on_date(current_date))
        result += sum(x.total for x in self.outcome_transfers.on_date(current_date))
        result -= sum(x.total for x in self.deposit_set.on_date(current_date))
        result -= sum(x.total for x in self.income_transfers.on_date(current_date))
        # ...

My immediate reaction is to extract those summations to a helper method, for example:

def sum_total_transfers(self):
    result = 0
    result -= sum(x.total for x in self.cashing_set.on_date(current_date))
    result -= sum(x.total for x in self.outcome_transfers.on_date(current_date))
    result += sum(x.total for x in self.deposit_set.on_date(current_date))
    result += sum(x.total for x in self.income_transfers.on_date(current_date))
    return result

So that the above methods can reuse that with less duplication:

@property
def balance(self):
    # ...
    while current_date <= today:
        result += self.sum_total_transfers()
        # ...

@property
def debt(self):
    # ...
    while current_date <= today:
        result -= self.sum_total_transfers()
        # ...

On closer look, these assert lines don't look good:

@property
def balance(self):
    assert self.method == self.DEBET
    # ...

@property
def debt(self):
    assert self.method == self.CREDIT
    # ...

These assertions force the callers to check the value of self.method before using these properties,
or to organize the code in a way that ensures that .debt will only be called when self.method == self.CREDIT, otherwise the program would crash.
It would be better to avoid such unwritten rules.

For example, it might be better to have one public property that applies the correct calculation based on the method:

@property
def balance(self):
    if self.method == self.DEBET:
        return self._credit_balance
    return self._debt_balance

@property
def _credit_balance(self):
    assert self.method == self.DEBET
    # ...

@property
def _debt_balance(self):
    assert self.method == self.CREDIT
    # ...

Lastly, "debt" is misspelled in self.DEBET.