Find the most frequent character in a string I'm trying to solve this problem https://community.topcoder.com/stat?c=problem_statement&pm=40

  Given a string made up of ONLY letters and
      digits, determine which character is repeated the most in the string ('A' is
      different than 'a'). If there is a tie, the character which appears first in
      the string (from left to right) should be returned.

Here is my approach is Scala. I feel it's not very functional, and slow(because of a new reversed String object).

def findMaxFunctional(word: String): Char = {
    var curMaxCount, maxCharId = 0
    word.reverse.foldLeft(new Array[Int](128))((acc, ch) => {
      acc(ch) = acc(ch) + 1
      if(acc(ch) >= curMaxCount) {
        curMaxCount = acc(ch)
        maxCharId = ch
      }
      acc
    })
    maxCharId.toChar
  }

Instead of reversing the input string you could foldRight over it, and instead of updating mutable variables let the fold accumulator carry the current data forward. As an Array is mutable by default there's no point in having the fold accumulator make copies in order to carry it forward.

def findMaxFunctional(word: String): Char = {
  val arr = new Array[Int](128)
  word.foldRight((' ', 0)){
    case (c, (mxChr, mxCnt)) =>
      arr(c) += 1
      if (arr(c) >= mxCnt) (c, arr(c))
      else (mxChr, mxCnt)
  }._1
}


Create "refresh token" action filter I am using idenity server 3 authentication.
I have to implement refresh token logic.
AccessTokenLifetime is 1 hour, and after that time I want to update access token with refresh token.

I want do this only once, by using filter in mvc. My idea is to check if token has expired, and if yes that get new token.

This is my code, I want to know if this is best practice or you maybe have better idea?

  public class RefreshTokenActionFilter : FilterAttribute, IActionFilter
        {
            void IActionFilter.OnActionExecuted(ActionExecutedContext filterContext)
            {
                var user = filterContext.HttpContext.User as ClaimsPrincipal;
                var request = filterContext.HttpContext.Request;
                RefreshToken(user, request);
            }

            void IActionFilter.OnActionExecuting(ActionExecutingContext filterContext)
            {
            }

            private  void RefreshToken(ClaimsPrincipal User, HttpRequestBase Request)
            {
                var claims = ClaimsPrincipal.Current.Claims;
                var refreshToken = claims.Where(c => c.Type == AuthenticationValues.RefreshTokenKey).Select(c => c.Value).SingleOrDefault();
                long epoch = Convert.ToInt64((claims.Where(c => c.Type == AuthenticationValues.ExpiresAtEpochKey).Select(c => c.Value).SingleOrDefault()));

                // 60 seconds is tolerance
                if (refreshToken != null && epoch - 60 < DateTime.UtcNow.ToEpochTime())
                {
                    var tokenClient = new TokenClient(ApplicationConfiguration.IdentityServerToken,
                                      AuthenticationController.ClientApplicationId,
                                      AuthenticationController.ClientApplicationSecret);

                    var response = (tokenClient.RequestRefreshTokenAsync(refreshToken)).Result;
                    var user = User as ClaimsPrincipal;
                    var identity = user.Identity as ClaimsIdentity;

                    //Remove old tokens:
                    var oldRefreshToken = user.Claims.Single(c => c.Type == AuthenticationValues.RefreshTokenKey);
                    var oldAccessToken = user.Claims.Single(c => c.Type == AuthenticationValues.AccessTokenKey);
                    var oldExpiresAt = user.Claims.Single(c => c.Type == AuthenticationValues.ExpiresAtKey);
                    var oldEpoch = user.Claims.Single(c => c.Type == AuthenticationValues.ExpiresAtEpochKey);
                    identity.RemoveClaim(oldRefreshToken);
                    identity.RemoveClaim(oldAccessToken);
                    identity.RemoveClaim(oldExpiresAt);
                    identity.RemoveClaim(oldEpoch);

                    //Add new tokens:
                    identity.AddClaim(new Claim(AuthenticationValues.AccessTokenKey, response.AccessToken));
                    identity.AddClaim(new Claim(AuthenticationValues.RefreshTokenKey, response.RefreshToken));
                    identity.AddClaim(new Claim(AuthenticationValues.ExpiresAtKey, (DateTime.UtcNow.ToEpochTime() + response.ExpiresIn).ToDateTimeFromEpoch().ToString()));
                    identity.AddClaim(new Claim(AuthenticationValues.ExpiresAtEpochKey, (DateTime.UtcNow.ToEpochTime() + response.ExpiresIn).ToString()));

                    var authenticationManager = Request.GetOwinContext().Authentication;

                    authenticationManager.AuthenticationResponseGrant = new AuthenticationResponseGrant(
                                                                                new ClaimsPrincipal(identity),
                                                                                new AuthenticationProperties { IsPersistent = true });
                }
            }
        }

I can see repetitive code which you can fix easily..

Before

//Remove old tokens:
var oldRefreshToken = user.Claims.Single(c => c.Type == AuthenticationValues.RefreshTokenKey);
var oldAccessToken = user.Claims.Single(c => c.Type == AuthenticationValues.AccessTokenKey);
var oldExpiresAt = user.Claims.Single(c => c.Type == AuthenticationValues.ExpiresAtKey);
var oldEpoch = user.Claims.Single(c => c.Type == AuthenticationValues.ExpiresAtEpochKey);
identity.RemoveClaim(oldRefreshToken);
identity.RemoveClaim(oldAccessToken);
identity.RemoveClaim(oldExpiresAt);
identity.RemoveClaim(oldEpoch);

After

var tokens = user.Claims
    .Where(c =>
        c.Type == AuthenticationValues.RefreshTokenKey ||
        c.Type == AuthenticationValues.AccessTokenKey ||
        c.Type == AuthenticationValues.ExpiresAtKey ||
        c.Type == AuthenticationValues.ExpiresAtEpochKey);

foreach (var token in tokens)
{
    identity.RemoveClaim(token);
}

You have filter claims by its type many times, which is similar code. You can get rid of duplicate code by introducing a method. I am not sure about the return or parameter type but it look somehow like below.

public static string GetClaimsType(string type)
{
    return ClaimsPrincipal.Current.Claims.Where(c => c.Type == type).Select(c => c.Value).SingleOrDefault();
}

Now your code will be much cleaner and shorter..

Before

var claims = ClaimsPrincipal.Current.Claims;
var refreshToken = claims.Where(c => c.Type == AuthenticationValues.RefreshTokenKey).Select(c => c.Value).SingleOrDefault();
long epoch = Convert.ToInt64((claims.Where(c => c.Type == AuthenticationValues.ExpiresAtEpochKey).Select(c => c.Value).SingleOrDefault()));

After

var refreshToken = GetClaimsType(AuthenticationValues.RefreshTokenKey);
long epoch = Convert.ToInt64(GetClaimsType(AuthenticationValues.ExpiresAtEpochKey));


Nested loop in lisp to process a 2d array - what's good style (with and/or without loop macro)? Code below is answer to an exercise in Paul Graham's ANSI Common Lisp (ex. 4.1). The challenge is to rotate an array by 90 degrees. I found an example solution which I've commented below.

It seems ok'ish and I'm enjoying learning how to use do, but in this particular case it seems actually more verbose than the familir c-style nested for loops to process a 2d array. I'm wondering if the below is good style or if it can be improved upon?

Is the answer going to be the loop macro?
I'm avoiding that at the moment while learning, waiting till I have better mastery of the basics. Yet would like to not pick up bad habits. Hence this question about style regarding the below.

(defun quarter-turn (arr)
  (let* ((dim (array-dimensions arr))  ; let* sets sequentially, not in parallel.
         (row (first dim))
         (col (second dim))
         (n row)  ; initialise n to row, just to organise
         new-arr) ; shorthand for (new-arr nil)
    (cond ((not (= row col))
           (format t "The arg is not a square array.~%"))
          (t (setf new-arr (make-array dim :initial-element nil))
             (do ((i 0 (+ i 1)))  ; Q. surprisingly, lisp's 'do' here looks more
                 ((= i n))        ; verbose than c-style 'for'. Is this good style?
                 (do ((j 0 (+ j 1)))
                     ((= j n))
                     (setf (aref new-arr j i) (aref arr (- n i 1) j))))
             new-arr))))

Source for the above code: http://www.cs.uml.edu/~lhao/ai/lisp/ansi-common-lisp/solution-ch04.htm

A few comments about your code:

Why introduce a new variable n copy of another variable? If you want to be more concise you could use n and m insted of row and col (that I don't like because they are singular names, while they represent the number of rows and of columns).
Why define new-arr before the test and then assign it after the test of squareness? Don't by shy of use let as many times as you need, to introduce variables when they are really necessary.
Use assert instead of cond to check for correct parameters: in this way you give the user the opportunity of correct them, and produce a shorter and more readable code.
Why initialize the new array to a value nil which is immediately overwritten?
The fist do could return the result, so to avoid the last line of the function.
Use the primitive function 1+ instead of summing by 1.
Document the function with a comment about its scope as first form of the body of the function.

So, after this first set of comments, the function could be rewritten as:

(defun quarter-turn (arr)
  "rotate a square matrix 90° clockwise"
  (let* ((dim (array-dimensions arr))
         (n (first dim))
         (m (second dim)))
    (assert (= n m) (arr) "The argument is not a square array.")
    (let ((new-arr (make-array dim)))
      (do ((i 0 (1+ i)))
          ((= i n) new-arr)
        (do ((j 0 (1+ j)))
            ((= j n))
          (setf (aref new-arr j i) (aref arr (- n i 1) j)))))))

DO versus LOOP

As you have already noted, in this case do is more verbose than loop. I think that in a case like this they are more or less equivalent (just a personal opinion). I would prefer the loop form not only because it seems to me slightly more concise, but also because it is more “natural” for me to think of a classical nested loop for a bidimensional array in terms two nested loop forms:

(defun quarter-turn (arr)
  "rotate a square matrix 90° clockwise"
  (let* ((dim (array-dimensions arr))
         (n (first dim))
         (m (second dim)))
    (assert (= n m) (arr) "The argument is not a square array.")
    (let ((new-arr (make-array dim)))
      (loop for i below n
            do (loop for j below n
                     do (setf (aref new-arr j i) (aref arr (- n i 1) j))))
      new-arr)))


Convolution function I implemented the following function using the matrix object from breeze library. Basically it is just a glorified while loop.
The convolution there is extremely slow so I rolled out my own, optimized on a kernel which is just a vector. This is already much faster but I figured there must be something else I can to to make this go faster.
The most expensive operation according to the profiler is the instantiation of the ArrayDeque  (pointed by the arrow) which I don’t really need because all I wanted was a circular buffer, but could not find much in the library.
The second thing is calling until on the Int. I don't think that can be avoided.
Boxing the doubles is also taking quite some time, maybe there is a way to specialize?
Finally the majority of the time is taken by the function call itself (circled in red) which I don’t know what it means. Any insight?
def conv(m: DenseMatrix[Int], k: DenseVector[Double]): DenseMatrix[Double] = {
    val kData = k.data
    val dataIter = m.data.iterator
    val height = m.rows
    val convoluted = Array.newBuilder[Double]
    val prev = mutable.ArrayDeque.empty[Double]
    for (_ <- 1 until k.length) {
      prev.addOne(dataIter.next())
    }
    var count = k.length - 1
    while (dataIter.hasNext) {
      val cur = dataIter.next()
      val slice = prev.append(cur)
      if (count % height >= k.length - 1) {
        var r = 0D
        for (i <- 0 until k.length) {
          r += kData(i) * slice(i)
        }
        convoluted.addOne(r)
      }
      prev.removeHead()
      count += 1
    }
    DenseMatrix.create(m.rows - (k.length - 1), m.cols, convoluted.result())
  }

Following is the annotated flamechart, please note that fut is the above function conv. Everything else is unchanged.

all I wanted was a circular buffer

Consider using an array accessed with mod.
That is, assign size s = k.length - 1,
allocate a "previous" array p,
and then access p[i % s].
If you're lucky, the compiler may notice that
a portion of the code is just copying,
and it will issue memcpy instructions.


Adding a custom field to Routes - clever solution or terrible hack? I need to make sure the correct menu item is highlighted according to which URL is currently being viewed.

Now the problem with our site is that each menu item can represent several pages. So the Client menu will show the client list - but should still be highlighted when you go to the edit client page. So I can't parse the current URL and match it against the menu item to determine if it should be highlighted.

Instead I have decided to add a custom field - menu - to the $routeProvider setup. So when setting up the routes :

$routeProvider
            .when(
                '/SecurityGroup/Index',
                {
                    templateUrl: '/SecurityGroup/IndexPartial',
                    controller: 'GroupListController',
                    menu: 'group'
                }
            )
            .when(
                '/SecurityGroup/MemberClients/:groupId&:groupName',
                {
                    templateUrl: '/SecurityGroup/MemberClientsPartial',
                    controller: 'MemberClientsController',
                    menu: 'group'
                }
            )

Then in the HTML, the menus are specified as (the tr-menu attribute matches against the name field in the route) :

Groups

Clients

Then I have a directive that manages the menu highlighting - essentially adding and removing the Active class if the current route contains the correct name :

(function() {
    'use strict';
    angular.module('Administration')
        .directive('menuitem', ['$location', function($location) {
            return {
                restrict: 'A',
                scope: false,
                transclude: true,
                replace: false,
                template: '',
                link: function (scope, element, attrs) {
                    var href = attrs.href;
                    var name = attrs.trMenu;
                    var link = element.find('a');
                    link.attr('href', href);

                    scope.$on('$routeChangeSuccess', function (event, currentRoute, previousRoute) {
                        var menu = currentRoute.$$route.menu;
                        if (menu === name)
                            element.addClass('active');
                        else
                            element.removeClass('active');
                    });

                }
            };

        }]);
}());

Is this a clever solution, or a terrible abuse of javascripts dynamic abilities? Is there a more Angular way of doing things rather than relying on adding my own fields to the existing Angular data structures?

I'm not sure if my solution is relevant to your problem, but I had a similar problem with keeping the correct page selected on page refresh as refresh was resetting defaults and home would be selected on what ever page you refreshed from.

This is how I solved it:

page controllers

app.controller('aboutCtrl', ['$rootScope', function($rootScope){
    $rootScope.$broadcast('pageSelected', "about");
}]);

app.controller('stuffCtrl', ['$rootScope', function($rootScope){
    $rootScope.$broadcast('pageSelected', "stuff");
}]);

app.controller('homeCtrl', ['$rootScope', function($rootScope){
    $rootScope.$broadcast('pageSelected', "home");
}]);

When the page controller is initiated, it broadcasts pageSelected with the name of the page as the value (important to use $rootScope to avoid parent/child cases).

navigation menu controller

app.controller('headerCtrl', ['$scope', '$rootScope', function($scope, $rootScope){
    $scope.$on('pageSelected', function(e, val){
        $scope.selected = val;
    });
}]);

The menu controller is listening for the pageSelected broadcast and will set $scope.selected to the value received.

navigation menu html

Home

To Do

Stuff

About

Finally, the menu will update which item has the selected class dynamically based on $scope.selected.

In your case, you could maybe have the page controllers that need the same menu item selected to broadcast the same value?

Although not necessary, I also recommend using ui-router.  It is state-based instead of URL-based; that's why I have ui-sref="home" ect.


Executing remote command using expect from C++ I wrote some code to execute remote commands over ssh, using expect, from c++. The code works but is a bit of a mess of c / c++ since there doesn't seem to be an idiomatic way to achieve much of what I wanted in pure c++. I appreciate that there are nominally some security concerns with the way I'm using passwords, but these aren't a concern for my use cases.

remote_commmand.h

#pragma once

#include <stdexcept>

struct RemoteException : public std::runtime_error {
    using std::runtime_error::runtime_error;
};

void remote_command(const std::string &host, const std::string &user, const std::string &password, const std::string &cmd);

remote_command.cpp

#include "remote_command.h"
#include <cstdio>
#include <cstring>
#include <unistd.h>
#include <fstream>
#include <vector>

using std::string;

namespace {

const char* expect_template =
"set timeout 60\n"
"spawn ssh %s@%s %s\n"
"expect {\n"
"   \"*yes/no*\"\n"
"   {\n"
"       send \"yes\\r\"\n"
"       exp_continue\n"
"   }\n"
"   \"*?assword:*\"\n"
"   {\n"
"       send -- \"%s\\r\"\n"
"       send -- \"\\r\"\n"
"       expect eof\n"
"   }\n"
"}\n";

class TempFile {

public:
    explicit TempFile(const char* contents){
        file_name = std::tmpnam(nullptr);
        std::ofstream f(file_name.c_str(), std::ios::out);
        if(!f.write(contents, std::strlen(contents))){
            throw std::runtime_error("ofstream");
        }
    }

    ~TempFile(){
        std::remove(file_name.c_str());
    }

    TempFile(TempFile &other) = delete;
    TempFile& operator=(TempFile &other) = delete;

    std::string file_name;
};

}

void remote_command(const string &host, const string &user, const string &password, const string &cmd){
    std::vector<char> formatted_cmd;
    const std::size_t formatted_size = std::strlen(expect_template) + host.size() + user.size() + password.size() + cmd.size();
    formatted_cmd.resize(formatted_size);

    const std::size_t res = std::snprintf(&formatted_cmd[0], formatted_size, expect_template, user.c_str(), host.c_str(), cmd.c_str(), password.c_str());

    if (res <= 0 || res >= formatted_size ){
        throw RemoteException("infsufficient size for snprintf");
    }

    TempFile tempfile(&formatted_cmd[0]);

    int r = execl("/usr/bin/expect", "-f", tempfile.file_name.c_str(), (char *)0);
    if (r == -1){
        throw RemoteException(string("execl") + std::strerror(errno) );
    }
}

example usage

remote_command("my-host", "user", "pw", "/path/to/my/script.sh");

Consider using raw literals

Your expect_template is a prime candidate to be written as a raw string literal:

const char* expect_template =
R"(set timeout 60
spawn ssh %s@%s %s
expect {
   "*yes/no*"
   {
       send "yes\r"
       exp_continue
   }
   "*?assword:*"
   {
       send -- "%s\r"
       send -- "\r"
       expect eof
   }
}
)";

I may have missed something in the editing, but the basic idea is pretty simple: you don't use escapes at all, just insert exactly what you want the string to contain (including new-lines).

Given an R immediately before the quote, the compiler treats the content as a raw literal. The opening delimiter is the quote, other optional "stuff", and an open paren. The closing delimiter is a close paren, the same optional stuff, and the close quote. In your case, the optional "stuff" isn't needed--you'd use it if you might need to include )" as part of your string. In that case, you might have something like R"<^>( at the beginning so the end of the string would only be signaled by )<^>" (the exact content doesn't matter a whole lot--you just have to be sure it's something that can't occur inside the string).

Use of .c_str()

C++98/03 required that you use s.c_str() when passing s to a stream's constructor or open member function. C++11 eliminated that requirement, so you can just pass the string directly.

Passing strings as parameters

Absent a reason to do otherwise, I'd at least consider passing std::string const & as the parameter to (for one example) TempFile::TempFile. This can simplify some of the content a little, and still lets you pass a C-style string when/if you want:

explicit TempFile(std::string const &contents) {
    file_name = std::tmpnam(nullptr);
    std::ofstream f(file_name);
    if(!(f << contents)) {
        throw std::runtime_error("writing ofstream");
    }
}

Use of snprintf

You might want to consider using a stringstream instead of snprintf. This does ease at least a few parts of the job, although it's not exactly perfect either.

std::ostringstream s;

s <<
R("set timeout 60
spawn ssh )" << user << "@" host << " " command << R"(
expect {
   "*yes/no*"
   {
       send "yes\r"
       exp_continue
   }
   "*?assword:*"
   {
       send -- ")"
<< password << R"(\r"
        send -- "\r"
        expect eof
    }
}
)";

TempFile f(s.str());


Finding the tree height in Python I'm trying to get an efficient algorithm to calculate the height of a tree in Python for large datasets. The code I have works for small datasets, but takes a long time for really large ones (100,000 items) so I'm trying to figure out ways to optimize it but am getting stuck. Sorry if it seems like a really newbie question, I'm pretty new to Python.

The input is a list length and a list of values, with each list item pointing to its parent, with list item -1 indicating the root of the tree. So with an input of:

5
4 -1 4 1 1

The answer would be 3 — the tree is: ({key:1, children: [{key: 3}, {key:4, children:[{key:0, {key:2}]}] }

Here is the code that I have so far:

import sys, threading
sys.setrecursionlimit(10**7) # max depth of recursion
threading.stack_size(2**25)  # new thread will get stack of such size

class TreeHeight:
    def read(self):
        self.n = int(sys.stdin.readline())
        self.parent = list(map(int, sys.stdin.readline().split()))

    def getChildren(self, node, nodes):
        parent = {'key': node, 'children': []}
        children = [i for i, x in enumerate(nodes) if x == parent['key']]
        for child in children:
            parent['children'].append(self.getChildren(child, nodes))
        return parent

    def compute_height(self, tree):
        if len(tree['children']) == 0:
            return 0
        else:
            max_values = []
            for child in tree['children']:
                max_values.append(self.compute_height(child))
            return 1 + max(max_values)

def main():
  tree = TreeHeight()
  tree.read()
  treeChild = tree.getChildren(-1, tree.parent)
  print(tree.compute_height(treeChild))

threading.Thread(target=main).start()

You don't need to explicitly build the tree to compute its depth.

class TreeDepth(object):
    def __init__(self, parents):
        self._parents = parents
        self._n = len(parents)
        self._max_depth = None
        self._depths = [None] * self._n

    def max_depth(self):
        if self._max_depth is None:
            for idx, parent in enumerate(self._parents):
                depth = self.get_depth(idx)
                if self._max_depth < depth:
                    self._max_depth = depth
        return self._max_depth

    def get_depth(self, idx):
        depth = self._depths[idx]
        if depth is not None:
            return depth
        parent = self._parents[idx]
        if parent == -1:
            depth = 1
        else:
            depth = self.get_depth(parent) + 1
        self._depths[idx] = depth
        return depth

>>> TreeDepth([4, -1, 4, 1, 1]).max_depth()
3

If my math is good, this goes over each item in self._parents at most twice, so it will have \$O(n)\$ performance. Also, I have used a recursive approach to compute the depth, but if speed is your main goal, you will probably want to turn that into an iterative approach:

def max_depth2(self):
    if self._max_depth is not None:
        return self._max_depth
    for idx, parent in enumerate(self._parents):
        parent_stack = []
        while parent != -1 and self._depths[idx] is None:
            parent_stack.append(idx)
            idx, parent = parent, self._parents[parent]
        if parent == -1:
            depth = 1
        else:
            depth = self._depths[idx]
        while parent_stack:
          self._depths[parent_stack.pop()] = depth
          depth += 1
        if self._max_depth < depth:
            self._max_depth = depth
    return self._max_depth


Compute bounding rectangle given 2 points quickly and efficiently Is there a faster way to compute the bounding area between 2 points?

// Minified Version
public static Rectangle computeBounds(Point2D.Double p1, Point2D.Double p2) {
    double dx = p2.x - p1.x, dy = p2.y - p1.y;
    return new Rectangle((int) (dx < 0 ? p2.x : p1.x),
        (int) (dy < 0 ? p2.y : p1.y), (int) Math.abs(dx), (int) Math.abs(dy));
}

// Readable Version
public static Rectangle computeBounds(Point2D.Double p1, Point2D.Double p2) {
    double dx = p2.x - p1.x;
    double dy = p2.y - p1.y;

    int x = (int) (dx < 0 ? p2.x : p1.x);
    int y = (int) (dy < 0 ? p2.y : p1.y);
    int w = (int) Math.abs(dx);
    int h = (int) Math.abs(dy);

    return new Rectangle(x, y, w, h);
}

Using a completely different tack... the simplest way to do this would be to use the native mechanisms available in the AWT toolkit....

For a regular Rectangle

public static Rectangle computeBounds(Point2D.Double p1, Point2D.Double p2) {
    return new (Line2D.Double(p1, p2)).getBounds();
}

For a Rectangle2D

public static Rectangle2D computeBounds(Point2D.Double p1, Point2D.Double p2) {
    return new (Line2D.Double(p1, p2)).getBounds2D();
}

Results

I have put this all together in a test harness, to measure the performance, and included two additional tests for you to consider:

import java.awt.geom.Line2D;
import java.awt.geom.Point2D;
import java.awt.geom.Rectangle2D;
import java.awt.geom.Point2D.Double;
import java.awt.Rectangle;
import java.util.Arrays;
import java.util.Random;

@SuppressWarnings("javadoc")
public class Bounds {

    private static abstract class BoundIt {
        private final String name;
        public BoundIt(String name) {
            this.name = name;
        }

        public String getName() {
            return name;
        }

        abstract Object computeBounds(Point2D.Double p1, Point2D.Double p2);
    }

    private static final BoundIt[] solvers = {
        new BoundIt("OPMinified") {

            @Override
            // Minified Version
            public Rectangle computeBounds(Point2D.Double p1, Point2D.Double p2) {
                double dx = p2.x - p1.x, dy = p2.y - p1.y;
                return new Rectangle((int) (dx < 0 ? p2.x : p1.x),
                    (int) (dy < 0 ? p2.y : p1.y), (int) Math.abs(dx), (int) Math.abs(dy));
            }

        },

        new BoundIt("OPReadable") {

            @Override
            // Readable Version
            public Rectangle computeBounds(Point2D.Double p1, Point2D.Double p2) {
                double dx = p2.x - p1.x;
                double dy = p2.y - p1.y;

                int x = (int) (dx < 0 ? p2.x : p1.x);
                int y = (int) (dy < 0 ? p2.y : p1.y);
                int w = (int) Math.abs(dx);
                int h = (int) Math.abs(dy);

                return new Rectangle(x, y, w, h);
            }

        },

        new BoundIt("KeepDouble") {

            @Override
            // Readable Version
            public Rectangle2D computeBounds(Point2D.Double p1, Point2D.Double p2) {
                double dx = p2.x - p1.x;
                double dy = p2.y - p1.y;

                double x = dx < 0 ? p2.x : p1.x;
                double y = dy < 0 ? p2.y : p1.y;
                double w = Math.abs(dx);
                double h = Math.abs(dy);

                return new Rectangle2D.Double(x, y, w, h);
            }

        },

        new BoundIt("Native") {

            @Override
            // Readable Version
            public Rectangle computeBounds(Point2D.Double p1, Point2D.Double p2) {
                return new Line2D.Double(p1, p2).getBounds();
            }

        },

        new BoundIt("Native2D") {

            @Override
            // Readable Version
            public Rectangle2D computeBounds(Point2D.Double p1, Point2D.Double p2) {
                return new Line2D.Double(p1, p2).getBounds2D();
            }

        },

    };

    public static final void testRound(int id, boolean print, Point2D.Double[] as, Point2D.Double[] bs) {

        Object[] results = new Object[as.length];

        for (BoundIt solver : solvers) {
            long time = System.nanoTime();
            for (int i = 0; i < as.length; i++) {
                results[i] = solver.computeBounds(as[i], bs[i]);
            }
            time = System.nanoTime() - time;

            int hc = 0;
            for (Object o : results) {
                hc ^= o.hashCode();
            }
            if (print) {
                System.out.printf("Solved Rep %d Solver %10s in %.3fms (hash %d)\n", id, solver.getName(), time / 1000000.0, hc);
            }
        }
    }

    public static void main(String[] args) {
        int datacnt = 409600;
        double span = 100;
        double offset = - span / 2;
        Random rand = new Random(0);

        Point2D.Double[] data = new Point2D.Double[datacnt << 1];

        for (int i = data.length - 1; i >= 0; i--) {
            data[i] = new Point2D.Double(offset + span * rand.nextDouble(), offset + span * rand.nextDouble());
        }

        Point2D.Double[] as = Arrays.copyOfRange(data, 0, datacnt);
        Point2D.Double[] bs = Arrays.copyOfRange(data, datacnt, data.length);
        data = null;

        System.gc();

        for (int i = 0; i < 100; i++) {
            System.out.println("\n\nProcessing round " + i);
            testRound(i, i >= 90, as, bs);
        }
    }

}

The reults from that test look like:

Processing round 97
Solved Rep 97 Solver OPMinified in 16.059ms (hash 1545060352)
Solved Rep 97 Solver OPReadable in 15.169ms (hash 1545060352)
Solved Rep 97 Solver KeepDouble in 15.726ms (hash -2058404443)
Solved Rep 97 Solver     Native in 26.782ms (hash 1558544384)
Solved Rep 97 Solver   Native2D in 12.478ms (hash -2058404443)

Processing round 98
Solved Rep 98 Solver OPMinified in 13.386ms (hash 1545060352)
Solved Rep 98 Solver OPReadable in 16.300ms (hash 1545060352)
Solved Rep 98 Solver KeepDouble in 13.484ms (hash -2058404443)
Solved Rep 98 Solver     Native in 59.174ms (hash 1558544384)
Solved Rep 98 Solver   Native2D in 14.190ms (hash -2058404443)

Processing round 99
Solved Rep 99 Solver OPMinified in 14.764ms (hash 1545060352)
Solved Rep 99 Solver OPReadable in 14.199ms (hash 1545060352)
Solved Rep 99 Solver KeepDouble in 13.572ms (hash -2058404443)
Solved Rep 99 Solver     Native in 30.009ms (hash 1558544384)
Solved Rep 99 Solver   Native2D in 13.721ms (hash -2058404443)


Service Layer Design I have written the following service class for my application and at the moment it only contains a single method which is responsible for carrying out the necessary actions to add a new team to the database. More methods will be added later but I want to make sure that I'm on the right track with regards to OOP and it's various principles.

The current addTeam() method essentially accepts a number of parameters, validates them to ensure that they are in the correct format and then determines if they violate any business rules i.e. team name must be unique and the league & country id's must already exist in the database.

I've only recently started to write in the OOP style, so it would be great to get some feedback on whether this is the right approach to take. The class seems to have a lot of dependencies which must be injected through the constructor method so I'm sure there is a better way to do it. In addition, the code within the addTeam() method still looks very procedural like and probably violates the single responsibility principle too.

class TeamService implements TeamServiceInterface {

     private $validator;
     private $teamMapper;
     private $leagueMapper;
     private $countryMapper;

     function __construct(ValidatorInterface $validator, TeamMapperInterface $teamMapper, LeagueMapperInterface $leagueMapper, CountryMapperInterface $countryMapper)
     {
         // Attempt to populate the class's variables with the information that has been passed to the object
         $this->validator = $validator;
         $this->teamMapper = $teamMapper;
         $this->leagueMapper = $leagueMapper;
         $this->countryMapper = $countryMapper;
     }

     public function addTeam($name, $country, $league, $active)
     {
         // Set the rules for the data that we want to pass to our team object
         $validator->setRule($name, 'name', 'required|alpha_numeric_ampersand_space|min_length[3]|max_length[30]');
         $validator->setRule($country, 'country', 'required|is_natural_no_zero');
         $validator->setRule($league, 'league', 'required|is_natural');
         $validator->setRule($active, 'active', 'required|boolean');

         // Execute the validation rules
         if($validator->run() === false)
         {
             throw new Exception('The information provided doesn\'t meet the required validation rules.');
         }

         // Check to see if the team name already exists in the database
         if($this->teamMapper->isTeamByName($name) === true)
         {
             throw new Exception('The name specified for the team already exists in the system.');
         }

         // Check to see if the country id specified exists in the database
         if($this->countryMapper->isCountryById($country))
         {
             throw new Exception('The country id specified could not be found in the system.');
         }

         // Check to see if the league id specified exists in the database
         if($this->leagueMapper->isLeagueById($league) === null)
         {
             throw new Exception('The league id specified could not be found in the system.');
         }

         // Create an instance of the team class
         $team = new Team($name, $country, $league, $active);

         // Attempt to persist the new team to the database
         if($this->teamMapper->insert($team) === false)
         {
             // Throw an exception because a certain event happened
             throw new Exception('The were was a problem adding the team to the system.');
         }

         return true;
     }
}

Maybe it's a matter of preference, but IMO Exceptions shouldn't be used for validation errors (see:Don't Use Exceptions For Flow Control). First, it's not that exceptional and second, you should still handle user friendly response: re-display form with error messages (or error list only in case of ajax request). Insetad exception one-liner you should build (View)Model that gets messages from validator and (invalid) $_POST data.

Other Exceptions (league/country/insert) are ok. They're thrown when user have done something illegal (bypassed GUI). On the other hand it doesn't deserve any specified message then (serve debugging purpose only).

The thing that looks smelly is collaborating with league and country mappers. I wouldn't use them at all in this context. I would either handle it with db constraints (foreign keys) that would throw me db exception* or (if I wanted to decide details later) handle it within $this->teamMapper->insert() method.
If you intend to use ORM there are propably some sophisticated methods for such constraints too, but can't tell for sure - got allergy to ORM.

*) The downside of using key constraints only is relying on something that is hidden outside the sourcecode. Db schema might be messed up (during migration for example) and vulnerability might pass unnoticed. Unless you have full controll over your code it's better to make checks anyway (preferably using transactions).


PHP ideal way to check function returns true Which of the following way of checking whether a function returns TRUE or FALSE is best, in terms of efficiency and code readability?

I was told by a friend that Method B is a good practice, but I believe Method A is better since it checks whether function returns TRUE then assigns.

Method A: (Checks whether function is TRUE and then assigns.)

if ($result = $db->getResults($id)) {
    echo $result;
} else {
    echo 'fail';
}

Method B: (First assigns the value, then checks—two operations.)

$result = $db->getResults(90);
if ($result) {
    echo $result;
} else {
    echo 'fail';
}

public function getResults($no) {
    if ($no > 85) {
        $result = 'pass';
        return $result;
    } else {
       return FALSE;
    }
}

Actually, both methods assign the value first before comparing its value. Therefore, in terms of efficiency, there's no difference.

For code readability, you could write extra parentheses around the assignment if you choose method A. Method B has a slight advantage when you have to perform debugging before the branch, i.e.:

$res = $db->getResult(95);
var_dump($res); // added for debugging without having to move the code out of the condition
echo $res ? $res : 'fail';

The ternary operator I've used above is yet another way to do your branching; its efficiency is in the same ballpark though. Since the variable itself is used solely in the truthy branch you can also use the shortened ternary operator (>= 5.3) like so:

echo $res ?: 'fail';

Although ternary operators are a powerful concept, they're easy to abuse as well and can cause either bugs or a WTF during code review. My personal guideline is to start favouring an if/else construct when either the condition has multiple sub-expressions or when the manual is opened under operator precedence.


Time calculator for a given speed and file size I wrote this little code in c++. It calculates the needed time for a given speed of a medium (for example the speed is 1024 B/s, the file size is 1MB, it'll take 17 minutes and 4 seconds to finish). The code works, but I'm not sure if it's proper.

Can you tell me if it's okay or not?

#include <iostream>

int main()
{
    int transmission_speed; //speed of the transmission in bytes per seconds
    int file_size_mb; //file's size in MBs

    std::cout << "Enter the speed of the transmission in bytes per seconds: ";
    std::cin >> transmission_speed;
    std::cout << "Enter the file's size in megabytes: ";
    std::cin >> file_size_mb;

    int file_size_b = file_size_mb *1024*1024;

    int seconds_needed = file_size_b / transmission_speed;

    int days_needed = (seconds_needed / 3600) / 24;
    seconds_needed -= days_needed*86400;

    int hours_needed = seconds_needed / 3600;
    seconds_needed -= hours_needed*3600;

    std::cout << "Days needed: " << days_needed << std::endl;
    std::cout << "Hours needed: " << hours_needed << std::endl;
    std::cout << "Seconds needed: " << seconds_needed << std::endl;
    return(0);
}

It's a very simple little program, however, there's a few comments to make:

All of the code is in main. Of course, for a program of this size, that doesn't really matter too much, however, you should prefer to break things up into self-contained functions where possible. For example, there should be a separate function here to actually do the calculations.
Use the correct data type for what you need. Can time ever be negative here? The answer is no, so I'd prefer to use unsigned over int.
You should be somewhat careful about overflow. Any filesize over 2048MB (2GB) will overflow the size of an int (assuming a 4 byte int - in actuality, an int is only technically required to be at least 2 bytes). Using an unsigned will change this to 4GB, however, if you expect any filesizes larger than that, you should look at another datatype (std::uint64_t perhaps).
Magic numbers. It's not so bad when dealing with time, because it's generally fairly obvious here, but numbers like 86400 shouldn't be shown as-is. They should be a named constant, such as const unsigned seconds_in_day = 86400.

I'd suggest breaking this up into a main function and a required_time(unsigned transmission_rate, unsigned file_size) function.


Pythonic way to add each previous element in list It's a very simple question, but I wonder if there is a more pythonic way to add each previous elements in a list like this (for example with a list comprehension maybe) :

input : L = [4, 2, 1, 3]
output : new_L = [4, 6, 7, 10]

Which I've done this way :

def add_one_by_one(L):
    new_L = []
    for elt in L:
        if len(new_L)>0:
            new_L.append(new_L[-1]+elt)
        else:
            new_L.append(elt)
    return new_L

new_L = add_one_by_one(L)

In Python 3 itertools gained a new function accumulate that does what you want.
I'd recommend that you instead used this function. Or if you can't if you're in Python 2 to upgrade.
This would single handedly change your code to:

from itertools import accumulate

new_l = accumulate(l)

If you however done this as a learning exercise, then I'd instead use iterators.
I'd first change l to an iterator, via iter.
Which would allow you to use next to remove the default value.
After this I would then loop through the iterator and yield rather than new_list.append the new values.
This can allow you to get something like:

def accumulate_sum(l):
    l = iter(l)
    try:
        total = next(l)
    except StopIteration:
        return
    yield total
    for item in l:
        total += item
        yield total

Which funnily enough is almost exactly the same to how it's done in itertools.accumulate.
If you wanted to at a later date use a different function rather than addition, then you could pass that as a function, to call on each iteration.


Tag wikis under a certain length I wrote a SEDE query to list all tag wikis with a body or an excerpt under a given number of characters. I've used this to find empty wikis or wikis with very little information. The tags can be sorted by post count to make choosing which to improve easier.

This is one of the first times I've written anything in SQL, I took ideas from other queries I found and threw them together. The conditional WHERE clause is my biggest concern. It looks really messy, but I'm not sure what the best way to achieve the same effect would be.

I'm aware that it doesn't actually get wikis with a length under the given number, but under or on. I wanted 0 to mean an empty wiki as that feel more intuitive than 1. I'm not sure about this design decision, what do you think?

-- MaxBodyLength: Max body length "Set to -1 to disable"
DECLARE @max_body_length INT = ##MaxBodyLength:INT?100##;
-- MaxExcerptLength: Max excerpt length "Set to -1 to disable"
DECLARE @max_excerpt_length INT = ##MaxExcerptLength:INT?-1##;

-- Max SE post length (see http://meta.stackexchange.com/a/176447/299387)
DECLARE @LEN_MAX INT = 30000;

BEGIN
  SELECT
    t.TagName,
    t.Count AS [Post count],
    LEN(pExcerpt.body) AS [Excerpt length],
    LEN(pWiki.Body) AS [Wiki length]
  FROM Tags t
    LEFT JOIN Posts pExcerpt ON t.ExcerptPostId = pExcerpt.Id
    LEFT JOIN Posts pWiki ON t.WikiPostId = pWiki.Id
  WHERE
    LEN(pWiki.body) <= CASE
      WHEN @max_body_length >= 0 THEN
        @max_body_length
      ELSE
        @LEN_MAX
      END
    AND LEN(pExcerpt.body) <= CASE
      WHEN @max_excerpt_length >= 0 THEN
        @max_excerpt_length
      ELSE
        @LEN_MAX
      END
END

Overall, even for a non-first-timer with SQL, this is a good query. The consistency and styling makes it very clear you're not a programming newbie.

To answer your concern about "under" versus "under or on", and 0 meaning an empty wiki - I totally agree. For me, 0 meaning empty is the only logical interpretation - empty strings have length 0 in every language I've come across. The way you've named your parameters (MaxSomething) implies clearly that the user is inputting the highest number that will be included in the results - and that's what they get. This is all perfectly clear and sensible to me, no need to worry.

Minor niggles

You don't need the BEGIN and END wrappers here. They don't hurt, but they're only required for control-of-flow blocks (usually, IF and WHILE constructs).

You don't need @LEN_MAX in the current version of the query.

For a "report" type query like this, I'd always prefer to have default ordering set by the query (an ORDER BY clause) so the results are guaranteed consistent.

The WHERE clause

Looking at this half a year on from your original post, I can see you've refined the WHERE clause yourself. At the time of my writing the WHERE clause on the linked SEDE query is:

WHERE
(@max_body_length >= 0 AND LEN(pWiki.body) <= @max_body_length)
OR (@max_excerpt_length >= 0 AND LEN(pExcerpt.body) <= @max_excerpt_length)

This is definitely a much clearer way to express your desired logic than the CASE statement in the original post.  For me, the reason it's clearer is that it maps quite easily to reading in plain English:

  Show me rows where the max body length is 0 or more and the length of the Wiki Body is less than or equal to that, or where...

I reckon we could go further though :)

Different operators can improve readability

This is a matter of opinion only, but if your aim is to make query logic easily readable in plain English, sometimes inverting your operators can help that.

@max_body_length >= 0

  "max body length is greater than or equal to 0, I mean 0 or more"

This is what I think when I read this code; I re-word it post-hoc to "0 or more" after understanding the meaning.

@max_body_length !< 0

  "max_body_length is not less than 0"

For me this reads more clearly first-pass, and doesn't need a post-hoc re-scan. It'll depend a lot on the reader and how familiar they are with ! as a negation operator (some people prefer <>).

An even shorter alternative

Shorter isn't always better: for complex logic, spelling it out step by step (more verbosely) as you have can be much clearer, because of the "mapping to plain English" property.

However, we could go shorter here using nullif() to equate your -1 default values with SQL NULL i.e. "unknown"/"not applicable":

WHERE
LEN(pWiki.body) <= nullif(@max_body_length,-1)
or LEN(pExcerpt.body) <= nullif(@max_excerpt_length,-1)

This takes advantage of the all-consuming nature of NULL: if I ask "is the length of this (known) thing less than an unknown number", the only possible answer can be "I don't know" - another NULL.

Then, because SQL NULL doesn't evaluate to true, the overall WHERE clause as a whole works out just as you intend.

However, the catch here (you probably spotted it) is that this only works for -1 as a default, not any negative input. Since negative inputs apart from -1 are meaningless in this query, you could validate this at the start of the query:

IF (@max_body_length<-1) or (@max_excerpt_length<-1)
    RAISERROR('Input parameters should be -1, 0 or positive integers',16,1)

Frankly this is probably overkill for a SEDE query but demonstrates the right approach for more generic application.

Ideally, one would actually make the default input for each parameter be NULL (rather than -1) and only permit 0 or positive input values: then you wouldn't need the nullif functions at all. However I'm not aware of a way to do that in SEDE.


The number of connected component Compute the number of connected component in a matrix, saying M. Given
  two items with coordinates [x1, y1] and [x2, y2] in M, if

  M(x1, y1) == -1 and
  M(x2, y2) == -1 and
  |x1-x2| + |y1-y2| == 1

  then they are connected.

  Example:

-1  0 -1  0  0
-1 -1  0 -1 -1
 0  0  0  0 -1
 0  0  0 -1 -1
 0  0  0 -1  0
 0 -1  0  0  0
 0 -1  0  0  0

  Output: 4. And they are:

-1
-1 -1

-1

-1 -1
   -1
-1 -1
-1

-1
-1

My idea is to scan the matrix.

Initialization:
count = 0.

For every item in the matrix, do the following three tests.

  If it's 0, skip
  If it's -1, check its four neighbors. If there is a neighbor whose
  value is not 0 and -1, assign the value of this neighbor to the current item. Otherwise, count++, ant then assign count to the current item.
  If it's not 0 and -1, assign the value of current item to its four
  neighbors whose value is -1.

The following is my code:

int num_cc(int m[][COLS])
{
    int count = 0;
    int r;
    int c;

    for(r = 0; r < ROWS; ++r)
    {
        for(c = 0; c < COLS; ++c)
        {
            if(m[r][c] == 0)
                continue;

            if(m[r][c] == -1)
            {
                if(r-1>=0 && m[r-1][c] > 0)
                    m[r][c] = m[r-1][c];
                else if(r+1<ROWS && m[r+1][c] > 0)
                    m[r][c] = m[r+1][c];
                else if(c-1>=0 && m[r][c-1] > 0)
                    m[r][c] = m[r][c-1];
                else if(c+1<COLS && m[r][c+1] > 0)
                    m[r][c] = m[r][c+1];
                else
                {
                    count++;
                    m[r][c] = count;
                }
            }

            if(m[r][c] > 0)
            {
                if(r-1>=0 && m[r-1][c] == -1)
                    m[r-1][c] = m[r][c];
                if(r+1<ROWS && m[r+1][c] == -1)
                    m[r+1][c] = m[r][c];
                if(c-1>=0 && m[r][c-1] == -1)
                    m[r][c-1] = m[r][c];
                if(c+1<COLS && m[r][c+1] == -1)
                    m[r][c+1] = m[r][c];
            }

        }
    }

    return count;
}

Can anyone help me verify it? Is it correct? Or is there any other solutions?

Style: Depending on coding guidelines and preferences you may want to consider moving the declaration of r and c into the for-loops (for (int r = 0...), as these variables are not supposed to be used outside of the loop, and hence, an outside declaration kind of contradicts this contract.

Correctness: I'm afraid your greedy approach does not work out well - in other words: it gives wrong results. Consider the following matrix (or if you loop the other way round its symmetric variant) :

 0  0  0  0 -1
-1 -1 -1 -1 -1

By going through the first row first, you would create your first connected group with this intermediate result, before the row-loop switches to the second row:

 0  0  0  0  1
-1 -1 -1 -1  1

Next, the inner loop will find another -1 cell with no higher-numbered neighbors and create a second connected component, although there really is only one.

Succinctness: If the code was correct, you might as well eliminate all the if statements in the loop except for the one comparing against -1. The others are redundant in their behavior.


Matrix to Tree Algorithm The code takes a matrix and turns it into a tree of all the possible combinations.  It then "maps" the tree by setting the value of the ending nodes to the total distance of the nodes from beginning node to ending node.

It seems to work fairly well but I've got a couple questions:

Is a Python dict the best way to represent a tree?
Any ways to simplify, speed up, or otherwise make it more clean and legible?

Keep in mind I'll need to sort the set so I can display the most attractive routes.

You can find the code on my GitHub page (starts at line 61).

def matrix_to_tree(nodes):
  """
  Creates a tree of all possible combinations of
  provided nodes in dict format
  """
  tree = {}
  for node in nodes:
    children = nodes[:]
    children.remove(node)
    tree[node] = matrix_to_tree(children)
  return tree

def set_start(tree, start):
  """
  Removes extraneous starting nodes if only one
  starting location is desired.
  """
  tree = tree[start]
  return tree

def set_end(tree, end):
  """
  Removes ending nodes when they are not the
  last node of the branch.  Used when one
  ending location is desired.
  """
  if tree[end]:
    del tree[end]
  nodes = tree.keys()
  if len(nodes) > 1:
    for node in nodes:
      set_end(tree[node], end)
  return tree

def map_tree(tree, matrix, start, distance=0):
  """
  Maps the distance from the root to each
  ending node.
  """
  for node in tree:
    new_distance = distance + node_distance(matrix, start, node)
    if tree[node]:
      map_tree(tree[node], matrix, node, new_distance)
    else:
      tree[node] = new_distance
  return tree

def node_distance(matrix, start, end):
  """
  Searches a matrix for the value of two
  points.
  """
  return matrix[start][end]

nodes = [key for key in x.keys()]
a = matrix_to_tree(nodes)
b = set_start(a, 'A')
c = set_end(b, 'G')
d = map_tree(c, x, 'A')
print(d)

Input example:

  {
         A: {B:1, C:2},
         B: {A:1, C:3},
         C: {A:2, B:3},
     }

Output example:

# 'A' being the root node with no ending node specified
{
    A: {B:{C:4}, C:{B:5}},
}

The code is clean, modular and readable. You could try to work with generators
to make the calculations lazy.

I think the namedtuple are a better choice than your dict approach, something like:

Node = collections.namedtuple('Node', ['name', 'distance', 'children'])

You can look into the way a functional language like Hskell or Clojure represents trees. It is weird that you mark the leaves of your tree by empty dicts and replace them with the calculated distance.

PEP8:

Use 4 spaces per indentation level.
Separate top-level function and class definitions with two blank lines.

set_start(): Why not just return tree[start]?
set_end():

tree[end] can result in a KeyError
Why the if len(nodes) > 1?

if end in tree:
    del tree[end]
for child in tree.values():
    set_end(child, end)
return tree

node_distance(): This is an internal function and can be inlined in map_tree().
Please do not place code on module level, but wrap it in a main() or test() function.

nodes = [key for key in x.keys()] can be simplified to nodes = list(x.keys()).


Knapsack greedy algorithm in Python I implemented the well-known knapsack problem and now I would like to improve it using list comprehension or lambda. I don't want to use NumPy. Could you help me?

def get_optimal_value(capacity, weights, values):
    value = 0.
    numItems = len(values)
    valuePerWeight = sorted([[values[i] / weights[i], weights[i]] for i in range(numItems)], reverse=True)
    while capacity > 0 and numItems > 0:
        maxi = 0
        idx = None
        for i in range(numItems):
            if valuePerWeight[i][1] > 0 and maxi < valuePerWeight[i][0]:
                maxi = valuePerWeight[i][0]
                idx = i

        if idx is None:
            return 0.
        if valuePerWeight[idx][1] <= capacity:
            value += valuePerWeight[idx][0]*valuePerWeight[idx][1]
            capacity -= valuePerWeight[idx][1]
        else:
            if valuePerWeight[idx][1] > 0:
                value += (capacity / valuePerWeight[idx][1]) * valuePerWeight[idx][1] * valuePerWeight[idx][0]
                return value
        valuePerWeight.pop(idx)
        numItems -= 1
    return value

For completeness here is the client code to test the implementation with a simple example:

if __name__ == "__main__":
    n = 3
    capacity = 50
    values = [60, 100, 120]
    weights = [20, 50, 30]
    opt_value = get_optimal_value(capacity, weights, values)
    print("{:.10f}".format(opt_value)) # print 180.0000000000

list comprehensions aren't too useful in that code. Well, I think I can help improve your code anyway:

valuePerWeight = sorted([[values[i] / weights[i], weights[i]] for i in range(numItems)], reverse=True)

is overcomplicated instead of the index and range. Use zip instead, which is faster and cleaner:

valuePerWeight = sorted([[v / w, w] for v,w in zip(values,weights)], reverse=True)

Same there: not very pythonic to work with indexes when you can use enumerate:

for i in range(numItems):
    if valuePerWeight[i][1] > 0 and maxi < valuePerWeight[i][0]:
        maxi = valuePerWeight[i][0]
        idx = i

could be:

for i,item in enumerate(valuePerWeight):
    if item [1] > 0 and maxi < item [0]:
        maxi = item [0]
        idx = i

clearer and faster right? and after having found idx you could save a lot of access-by-index time by creating:

v = valuePerWeight[idx][0]
w = valuePerWeight[idx][1]

and refer to v and w in the rest of the code: clearer and faster (double access by index is costly cpu-wise)

Last item: you're dividing and multiplying by the same value. Since they're float operations, you could simplfy:

if valuePerWeight[idx][1] > 0:
    value += (capacity / valuePerWeight[idx][1]) * valuePerWeight[idx][1] * valuePerWeight[idx][0]
    return value

by (using v and w as instructed above)

if w > 0:
    value += capacity * v
    return value

Also, you don't need numItems at all now. Just turn the while loop as:

while capacity > 0 and valuePerWeight:

(when valuePerWeight is empty, the loop ends)

So to sum it all up, here's my proposal for an improved code of yours:

def get_optimal_value(capacity, weights, values):
    value = 0.

    valuePerWeight = valuePerWeight = sorted([[v / w, w] for v,w in zip(values,weights)], reverse=True)
    while capacity > 0 and valuePerWeight:
        maxi = 0
        idx = None
        for i,item in enumerate(valuePerWeight):
            if item [1] > 0 and maxi < item [0]:
                maxi = item [0]
                idx = i

        if idx is None:
            return 0.

        v = valuePerWeight[idx][0]
        w = valuePerWeight[idx][1]

        if w <= capacity:
            value += v*w
            capacity -= w
        else:
            if w > 0:
                value += capacity * v
                return value
        valuePerWeight.pop(idx)

    return value

if __name__ == "__main__":
    n = 3
    capacity = 50
    values = [60, 100, 120]
    weights = [20, 50, 30]
    opt_value = get_optimal_value(capacity, weights, values)
    print("{:.10f}".format(opt_value)) # print 180.0000000000

tested and stil returns 180.0, fortunately.


Haversine formula in SQL This is an implementation of the Haversine formula in Microsoft Transact SQL.

How can I simplify the function?

CREATE FUNCTION dbo.Haversine(@point_a geography, @point_b geography)
RETURNS FLOAT
AS BEGIN
    DECLARE @result FLOAT
    DECLARE @lat1 FLOAT = @point_a.Lat
    DECLARE @lon1 FLOAT = @point_a.Long
    DECLARE @lat2 FLOAT = @point_b.Lat
    DECLARE @lon2 FLOAT = @POINT_b.Long

    DECLARE @earth_radius FLOAT =  6371
    DECLARE @dLat FLOAT = RADIANS(@lat2 - @lat1)
    DECLARE @dLon FLOAT = RADIANS(@lon2 - @lon1)
    SET @lat1 = RADIANS(@lat1)
    SET @lat2 = RADIANS(@lat2)

    DECLARE @a FLOAT
    SET @a = POWER(SIN(@dLat/2),2) + COS(@lat1)*COS(@lat2)*POWER(SIN(@dLon/2),2)
    DECLARE @c FLOAT = 2*ASIN(SQRT(@a))
    SET @result = @earth_radius * @c;
    RETURN @result
END

Here is a test of the function:

DECLARE @target_point geography = (SELECT GeoLocation FROM WA_Features WHERE FEATURE_NAME = 'Seattle' AND FEATURE_CLASS='Populated Place');

SELECT FEATURE_NAME, dbo.Haversine(@target_point, GeoLocation)*0.62137 FROM WA_Features WHERE FEATURE_CLASS='Lake' ORDER BY dbo.Haversine(@target_point, GeoLocation);

Am I calling the function properly? And is there a way to call it only once per row?

Why do you need the function? To measure the distance (in miles) between two points? Then you want to select all points of a certain type and order them by distance from a given origin.

With a CTE (common table expression)  you can simplify things a lot too.

But, the real issue, is that you should use the functions already available in the geography datatype: STDistance. This STDistance function returns the 'LineString' distance, which is 0.25% accurate to the geodesic distance. Additionally, this is better than the 0.5% accuracy of the Haversine distance.

So, by the sounds of it, you have horribly overcomplicated this process. Consider this (SQLFiddle example here too):

Select s.FEATURE_NAME as SName,
       s.GeoLocation.Lat as SLat,
       s.GeoLocation.Long as SLong,
       t.FEATURE_NAME as TName,
       t.GeoLocation.Lat as TLat,
       t.GeoLocation.Long as TLong,
       s.GeoLocation.STDistance(t.GeoLocation) * 0.00062137 as Distance
from Features s,
     Features t
where s.FEATURE_NAME = 'Seattle'
  and s.FEATURE_CLASS = 'Populated Place'
  and t.FEATURE_CLASS = 'Lake'
order by Distance desc


Changing a user status flag, with validation I am writing a function which on update of any attribute of model sets the status of variable is_kyc_verified to false.

Here is the code of the User model and the method which changes the status:

class User < ActiveRecord::Base
  # Include default devise modules. Others available are:
  # :confirmable, :lockable, :timeoutable and :omniauthable
  devise :database_authenticatable, :registerable,
         :recoverable, :rememberable, :trackable, :validatable
  before_update :change_kyc_status, unless: :is_kyc_verified_changed?

  #Associations
  has_one  :address, dependent: :destroy
  has_one  :kyc, dependent: :destroy
  has_one  :pan_detail, dependent: :destroy
  has_one  :document, dependent: :destroy
  has_many :nominee_details, dependent: :destroy
  has_many :bank_details, dependent: :destroy

  #Accept Attributes for associated model

  accepts_nested_attributes_for :address, :kyc, :pan_detail,
                                :document, :nominee_details, :bank_details,
                                allow_destroy: true, reject_if: :all_blank

  #validates
  validates :name, :mobile_no, :gender, :dob,  presence: true
  validates :mobile_no, numericality: true, length: { is: 10 }

  private
  ##
  # Check if is_kyc_verified is set to true
  # if 'yes' then alert user and set is_kyc_verified to false
  def change_kyc_status
    self.is_kyc_verified = false if self.valid? and self.is_kyc_verified.present?
    true
  end

end

As you can see the method initially used to return the self.is_kyc_verified which was false, which in turn resulted in "rollback of transaction" so I explicitly added a true at the end so it won't "rollback the transaction"

However I feel, that this is not the right way to implement this function.
Can you please review my code and suggest me the right way to do so?

Maybe just change the before_update to execute when #changed? is true?

http://api.rubyonrails.org/classes/ActiveModel/Dirty.html#method-i-changed-3F

EDIT: After looking at the question again, I'd change my answer. I think it's reasonable to return true. There's not really anyway around it, considering the way callbacks handle the return value. I suppose you could invert the values of the is_kyc_verified to be something like is_kyc_unverified?. But, that already seems confusing.

I do however think you should move the unless: :is_kyc_verified_changed? into the change_kyc_status method. I find the current flow a little hard to follow, as you must check in two places if is_kyc_verified will be set.

I might change it to something like:

before_update :confirm_is_kyc_verified
...
def confirm_is_kyc_verified
  if !is_kyc_verified.changed? && is_kyc_verified && valid?
    is_kyc_verified = false
  end

  true
end


Print concentric rectangular patterns I am solving interview questions from here.

  Problem : Print concentric rectangular pattern in a 2d matrix. The outermost rectangle is formed by A, then the next outermost is formed by A-1 and so on. You will be given number as an argument to the function you need to implement, and you need to return a 2D array.

  Example 1: Input: A = 4.

        Output:

        4 4 4 4 4 4 4
        4 3 3 3 3 3 4
        4 3 2 2 2 3 4
        4 3 2 1 2 3 4
        4 3 2 2 2 3 4
        4 3 3 3 3 3 4
        4 4 4 4 4 4 4

How can I make this code better?

def pretty_print(num):

    m = n = 2*num -1 ##    m x n matrix , length of each row and column
    k = 0 # row start counter
    l = 0 # column start counter
    i = 0 # iterator

    matrix = [[0 for _ in range(n)] for _ in range(m)]

    while k < m and l < n :
        #insert the first row
        for i in range(l, n) :
            if matrix[k][i] == 0:
                matrix[k][i] = num   # row index constt, change values in columns

        k += 1   # first row printed, so increment row start index

        #insert the last column
        for i in range(k, m) :
            if matrix[i][n-1]==0:
                matrix[i][n-1] = num   # column index constt, change values in rows
        n -= 1   # last column printed, so decrement num of columns

        #insert the last row
        if (k<m):   #  if row index less than number of rows remaining
            for i in range(n-1, l-1, -1):
                if matrix[m-1][i] == 0:
                    matrix[m-1][i] = num   # row index constt, insert in columns
        m -= 1   # last row printed, so decrement num of rows

        #insert the first column
        if (l<n):    #  if column index less than number of columns remaining
            for i in range(m-1, k-1, -1):
                if matrix[i][l] == 0:
                    matrix[i][l] = num # column index constt, insert in rows
        l += 1      # first column printed, so increment column start index

        num -= 1    # all elements of value A inserted , so decrement

    return matrix

print pretty_print(6)

Simplify

I find the logic a bit complicated. It could be simpler:

Loop from -A to +A, let's call the loop variable n
Take the absolute value of n
Generate the values in the row:

Loop from -A to +A, let's call the loop variable m
Use the maximum of abs(n) + 1 and abs(m) + 1

Like this:

def pretty_print(num):
    def print_line(n):
        low = abs(n)
        print(' '.join(str(max(low + 1, abs(i) + 1)) for i in range(-num + 1, num)))

    for i in range(-num + 1, num):
        print_line(i)

Testing

Doctests are awesome, I recommend to use them.
Here's the complete solution with doctests,
you can run this with python -mdoctest pretty_print.py:

#!/usr/bin/env python

def pretty_print(num):
    """
    >>> pretty_print(1)
    1

    >>> pretty_print(2)
    2 2 2
    2 1 2
    2 2 2

    >>> pretty_print(6)
    6 6 6 6 6 6 6 6 6 6 6
    6 5 5 5 5 5 5 5 5 5 6
    6 5 4 4 4 4 4 4 4 5 6
    6 5 4 3 3 3 3 3 4 5 6
    6 5 4 3 2 2 2 3 4 5 6
    6 5 4 3 2 1 2 3 4 5 6
    6 5 4 3 2 2 2 3 4 5 6
    6 5 4 3 3 3 3 3 4 5 6
    6 5 4 4 4 4 4 4 4 5 6
    6 5 5 5 5 5 5 5 5 5 6
    6 6 6 6 6 6 6 6 6 6 6
    """

    def print_line(n):
        low = abs(n)
        print(' '.join(str(max(low + 1, abs(i) + 1)) for i in range(-num + 1, num)))

    for i in range(-num + 1, num):
        print_line(i)

pretty_print(6)

Style

There are some minor style issues with the posted code.
I suggest to follow the PEP8 guidelines.


Better way to check if Javascript Date object exists When a button is pressed, an animation loop is triggered.  At the start of the loop, I create a new time object.

I also have a pause button.  A problem was created when paused and then started as the Date object would be recreated each time.

The variable is global and not defined to be anything.  I could set it to 0 and run the conditional test on that instead, but either way there has to be a cleaner way of doing this.  Apologies, I'm quite new to JS.

The code works, but it's ugly (in my opinion):

var t;//not defined

//for starting the animation
startSim.onclick = function(){

    if(t == undefined){
        //get time object
        t = new Date();
    }

    //other code...

}

EDIT:  In case the question is unclear - I'm checking to see if the Date object has been created by finding out if the variable is undefined or not.  What I'm asking is simply Is there a way to find out if the Date object already exists that doesn't rely on checking if the var is undefined.
There is little code as the above is the only relevant part, I can post the rest but it's pointless as not required.  (I've added a little more to show structure but I don't think it's required to understand the question)

I can think of the following improvements:

Hide t. If you're running other JS modules, you can't really be sure that no-one else writes to the same variable.
If you want to be sure that your code doesn't mix up an uninitialized timer with a "cleared" timer, you might want to pre-initialize the variable to null instead.
Use === when comparing null, undefined, etc. In JavaScript, null == undefined, but only null === null.

You might want to do something like this:

var timer = (function () {
    var t = null,

    start = function () {
        if (t === null) {
            t = new Date();
        }
    },

    stop = function () {
        var time = new Date() - t;
        t = null;
        return time;
    };

    return {
        start: start,
        stop: stop
    };
})();

timer.start();

var timePassed = timer.stop();


Optimisation of palindrome My program takes as first input the number of test cases, n. For the following n inputs, I output the smallest positive integer that has to be added to the input to make the input a numeric palindrome. If the input is already a palindrome the output shall be zero.

I have a problem with the performance, the algorithm is correct but I get get time limit exceeded. The input number is in the range \$[1, 1000000]\$.

#include <cstdio>
#include <cstdlib>
#include <string>
using namespace std;

extern bool isPal(const int &number) __attribute__((fastcall));

bool isPal(const int &number)
{
    char buffer[6];
    sprintf(buffer,"%i",number);
    const string x = buffer;
    for(int i=0, end = x.size()-1;i<end;i++,end--)
        if(x[i]!=x[end])
        return false;
    return true;
}

int main()
{
    int n, number, counter, t;
    scanf("%i",&n);
    while(n--)
    {
        counter = 0;
        scanf("%i",&number);
        t = number+counter;
        while(!isPal(t))
        {
            ++counter;
            t = number+counter;
        }
        printf("%i\n",counter);
    }
    return 0;
}

Please help me optimise the above code.

Comments on the code as it is

This:

  __attribute__((fastcall))

will not improve your performance, as the culprit is the algorithm not the compiler. If you really want to tell the compiler to optimize your code harder, you can try inline instead.

This forward declaration:

  extern bool isPal(const int &number) __attribute__((fastcall));

is unnecessary as the function body directly follows the forward declaration.

Looking at the isPal function the name could have been better. I would suggest isPalindrome.

bool isPal(const int &number)
{
    char buffer[6];
    sprintf(buffer,"%i",number);
    const string x = buffer;
    for(int i=0, end = x.size()-1;i<end;i++,end--)
        if(x[i]!=x[end])
        return false;
    return true;
}

Unless you need to modify the argument, always take simple types by value; so const int& number should be int number.
The input range is \$[0,1000000]\$ so your buffer is too small it needs to be at least 7 + 1 (for the null terminator).
The use of sprintf is discouraged, please prefer snprintf instead. You might want to consider stringstream for type conversion but I'll leave that up to you.
The variable const string x is unnecessary, simply use strlen from <cstring> instead.
You really should put some white spaces in there as the code is hard to read as it is.
Always prefer to use curly braces ({}) on control statements. Your missing indentation on the return false statement is confusing.
Also you can do without the end variable, but that's up to taste. But I will leave it there.

Fixing the above, your function should look like:

bool isPalindrome(int number)
{
    char buffer[8];
    snprintf(buffer, 8, "%i", number);
    int len = strlen(buffer);
    for(int i = 0, end = len - 1; i < end; i++, end--){
        if(buffer[i] != buffer[end]){
            return false;
        }
    }
    return true;
}

Now looking at your main() function.

Always prefer to declare your variables in the smallest possible scope. This makes the code easier to read. This means that counter, number and t should be declared inside of the loop body.
You're using C-style IO in a C++ program, this is not recommended. Instead you should use C++ streams cin and cout.
Again you could use some more white spaces.

The fixed code:

int main()
{
    int n;
    cin >> n;;
    while(n--)
    {
        int counter = 0;
        int number;
        cin >> number;
        int t = number + counter;
        while(!isPal(t))
        {
            ++counter;
            t = number + counter;
        }
        cout << counter << endl;
    }
    return 0;
}

Data handling improvements

You are formatting the input to a string to check if it is a palindrome. This is quite  unnecessary. To test if an integer is a numeric palindrome, you need to count the number of digits in the number, then extract and flip the digits in the top half and compare them to the bottom half. Like this:

int64_t ipow(int64_t base, int exp, int64_t result = 1) {
  return exp < 1 ? result : ipow(base*base, exp/2, (exp % 2) ? result*base : result);
}

int countDigits(int value){
  int digits = 0;
  while(value != 0){
    value /= 10;
    digits++;
  }
  return digits;
}

int flipDigits(int value){
  int ans = 0;
  while(value){
    ans *= 10;
    ans += value % 10;
    value /= 10;
  }
  return ans;
}

int isPalindrome(int value){
  int digits = countDigits(value);
  int ten_pow_digits_2 = ipow(10, digits/2);
  int high_digits = value / ten_pow_digits_2;
  if(digits%2){
    high_digits /= 10;
  }
  int low_digits = value % ten_pow_digits_2;

  return flipDigits(high_digits) == low_digits;
}

This will check if the number is a palindrome without formatting to a string and will be faster. However we can do better by improving the algorithm.

Algorithmic improvements

I'm not going to describe the correct algorithm here as @outoftime has already done so in his answer. Instead I'm going to provide a different implementation of the same algorithm with less code that doesn't abuse inheritance of the standard containers. You can find such improvements as predefined values and so on.

#include <iostream>

constexpr int64_t ipow(int64_t base, int exp, int64_t result = 1) {
  return exp < 1 ? result : ipow(base*base, exp/2, (exp % 2) ? result*base : result);
}

// This covers the range of int.
const int pow10[]{ipow(10,0), ipow(10,1), ipow(10,2), ipow(10,3),
    ipow(10,4), ipow(10,5), ipow(10,6), ipow(10,7), ipow(10,8), ipow(10,9) };

int countDigits(int value){
  int digits = 0;
  while(value != 0){
    value /= 10;
    digits++;
  }
  return digits;
}

int getDigit(int value, int digit){
  int place = pow10[digit];
  return (value/place) % 10;
}

int calcPalindromeDeficiency(int value){
  const int digits = countDigits(value);
  int digit = 0;
  int ans = 0;
  while(digit != (digits+1)/2){
    int top_digit = getDigit(value, digits - digit - 1);
    int bottom_digit = getDigit(value, digit);
    int to_add = ((10 + top_digit - bottom_digit)%10)*pow10[digit];
    ans += to_add;
    value += to_add;
    if(getDigit(value, digits - digit - 1) == getDigit(value, digit)){
      digit++;
    }
  }
  return ans;
}

int main(){
  int n;
  std::cin >> n;

  while(n--){
    int input;
    std::cin >> input;
    std::cout << calcPalindromeDeficiency(input) << std::endl;
  }
  return 0;
}

I find it more elegant to not convert to string when handling numbers.


Finding the order of sorting of an array I am finding the order of sorting of an array. The code works but can it be made better especially the return values of the function findSortOrder.

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

// Returns 0 for unsorted, 1 for sorted in increasing order, 2 for sorted in decreasing order
int findSortOrder(int array[], int len)
{
    // holds the sorting order of the subarray array[0...i]
    // 0 represents no information about sorting order
    // 1 represents sorting in increasing order
    // 2 represents sorting in decreasing order
    int order = 0;
    int i;
    for (i = 1; i < len; i++)
    {
        if (order == 0)
        {
            if (array[i] > array[i - 1])
            {
                order = 1;
            }
            else if (array[i] < array[i - 1])
            {
                order = 2;
            }
        }
        else if (order == 1)
        {
            if (array[i] < array[i - 1])
            {
                return 0;
            }
        }
        else
        {
            if (array[i] > array[i - 1])
            {
                return 0;
            }
        }
    }
    if (order == 0 || order == 1)
    {
        return 1;
    }
    else
    {
        return 2;
    }
}

int main()
{
    printf("Enter length of the array: ");
    int len;
    scanf("%d", &len);
    int* input = malloc(len * sizeof(*input));
    int i;
    for (i = 0; i < len; i++)
    {
        scanf("%d", &input[i]);
    }
    int order = findSortOrder(input, len);
    switch (order)
    {
        case 0:
            printf("Unsorted\n");
            break;
        case 1:
            printf("Sorted in increasing order\n");
            break;
        case 2:
            printf("Sorted in decreasing order\n");
            break;
    }
    free(input);
    return 0;
}

Edit:
I will be using this function to merge two sorted arrays in their sorting order. So I think if no. of elements is 1 or all are equal then sort order could be returned as increasing.

The cleanest would be an enum and a switch (current) with three cases:

In each case you can then check if you stay in the current_order or if you
switch to another or if you can return 0/1/-1.

Simplified:

current=NONE;
for()
    a=... b= ..
    switch(current)
        case NONE: if (a>b) current=DEC; else if (a<b) current=INC; break;
        case INC:  if (a>b) return NONE; break;
        case DEC:  if (a<b) return NONE; break;
return current;

I don't know if it's the right term, but I think that is a finite state machine. You could read about those to get more information; it's always good to use for parsing stuff.

From the comments:

  What should be returned if the array is one element long?

Good point. I would introduce another return value MIXED:

current=NONE;
for()
    a=... b= ..
    switch(current)
        case NONE: if (a>b) current=DEC; else if (a<b) current=INC; break;
        case INC:  if (a>b) return MIXED; break;
        case DEC:  if (a<b) return MIXED; break;
return current;


Django REST Framework - PDF Generator I created a new endpoint for an API (Django REST Framework) that generates a PDF from an HTML template. I follow the example from the plugin django-easy-pdf.

It's working, but I would like to improve and to know what could be done in a better way.

Asset is an object from the database with information. The idea is to print some rows on an HTML template to then convert to PDF. The HTML is fine, but I'm just not sure if I could improve on this view.

URLs:

 url(r'^report/asset/(?P<pk>[0-9]+)$', api.DemoPDFView.as_view()),

views.py

from easy_pdf.views import PDFTemplateResponseMixin, PDFTemplateView

class DemoPDFView(PDFTemplateView):
    template_name = 'reports/asset.html'
    pdf_filename = 'asset.pdf'

    def get_context_data(self, **kwargs):
        pk = self.kwargs.get('pk', None)

        asset = Asset.objects.get(id=pk)
        project = asset.project.name
        id = asset.id
        name = asset.title

        return super(DemoPDFView, self).get_context_data(
            pagesize='A4',
            title='Asset',
            project=project,
            name=name,
            id=id,
            **kwargs
        )

The plugin is available:

http://django-easy-pdf.readthedocs.io/en/v0.2.0-dev1/

You have a URL endpoint with a numeric parameter to retrieve an asset and generate a PDF report. But the parameter is not validated. Django will ensure that the value is numeric, thanks to the way you defined the endpoint, but the asset might not exist.
When a nonexistent asset is requested, the program will crash.

Since kwargs is a parameter of get_context_data, it would be better to access it directly, rather than through self.kwargs.

Since Django ensures that the pk parameter exists, you can write simpler like this:

pk = kwargs['pk']


CSV file download in Spring I recently worked on a module, in which I implemented the functionality of downloading the CSV files.

@RequestMapping(value = "/export_data")
public void downloadDataInCsv(
        @RequestParam("type") String type,
        @RequestParam("tID") String tableId,
        HttpServletRequest request,
        HttpServletResponse response) throws IOException {
    if (type.equals("csv")) {
        List<UserInfo> list = userInfoDao.findById(tableId);
        ExportCsvUtil.downloadCsv(request, response, list);
    }
}
private void downloadCsv(HttpServletRequest request, HttpServletResponse response, List<UserInfo> list) throws IOException {
    String headerKey = "Content-Disposition";
    String headerValue = String.format("attachment; filename=Table_Data.csv");
    response.setContentType("text/csv");
    response.setHeader(headerKey, headerValue);

    try (final CSVWriter writer = new CSVWriter(response.getWriter(), ",")) {

        writer.writeNext(new String[]{"User Id", "First Name", "Last Name", "Roll No", "Email ID", "Gender"});

        for (UserInfo entry: list) {
            // cast/convert to String where needed
            writer.writeNext(new String[]{entry.getUserId()+"", entry.getFirstName(), entry.getLastName(),entry.getRollNo(),entry.getEmail(),entry.getGender()});
        }
        writer.close();
    }
}

I am using OpenCSV library for reading and writing the CSV files, in addition, one thing should be note down here is, the function writeNext() accepts String[] .

I initialized the String[] inside writeNext(). I want to know the better way to do the same, so the readability enhanced.

Please review my code and suggest me the best practice.

First of all, please don't take what I am writting personnaly. Also, I am sure there would be something to argue with my proposals so feel free to tell me!

Let's begin with:

@RequestMapping(value = "/export_data")
public void downloadDataInCsv(
        @RequestParam("type") String type,
        @RequestParam("tID") String tableId,
        HttpServletRequest request,
        HttpServletResponse response) throws IOException {
    if (type.equals("csv")) {
        List<UserInfo> list = userInfoDao.findById(tableId);
        ExportCsvUtil.downloadCsv(request, response, list);
    }
}

I hate using plain strings when doing some comparison especially when it looks like a real constant like csv

In order to remove the plain String, I usually use enum to do that.

Why not replace type.equals("csv") by something like type.equals(FileType.CSV.name())

Next, if we are looking here at this chunk:

List<UserInfo> list = userInfoDao.findById(tableId);
ExportCsvUtil.downloadCsv(request, response, list);

we see that your are proceding with ExportCsvUtil.downloadCsv() even if the list is empty. Should this be a normal behaviour of your application ? Maybe you should send a message telling the user that they were no data to retrieve ?

Last thing for downloadDataInCsv() method is that if your type isn't a csv file, it will return a HTTP code 200 with nothing. I think the client would be kind of confused with that. Maybe try to send a different message like I said before. (e.g: only CSV export is available at the moment)

Next chunk! :)

private void downloadCsv(HttpServletRequest request, HttpServletResponse response, List<UserInfo> list) throws IOException {
    String headerKey = "Content-Disposition";
    String headerValue = String.format("attachment; filename=Table_Data.csv");
    response.setContentType("text/csv");
    response.setHeader(headerKey, headerValue);

    try (final CSVWriter writer = new CSVWriter(response.getWriter(), ",")) {

        writer.writeNext(new String[]{"User Id", "First Name", "Last Name", "Roll No", "Email ID", "Gender"});

        for (UserInfo entry: list) {
            // cast/convert to String where needed
            writer.writeNext(new String[]{entry.getUserId()+"", entry.getFirstName(), entry.getLastName(),entry.getRollNo(),entry.getEmail(),entry.getGender()});
        }
        writer.close();
    }
}

If you look at String headerValue = String.format("attachment; filename=Table_Data.csv");, you don't use String.format() capabilities at all! (if I am not mistaken)

String.format() allows you to do things like String.format("Hello %s", name)

You can replace this with an old plain String then.

response.setContentType("text/csv");, if you are using the enum like I said before, you could add a property contentType that allows you to do FileType.CSV.getContentType() in order to remove the hardcoded string.

writer.writeNext(new String[]{"User Id", "First Name", "Last Name", "Roll No", "Email ID", "Gender"});, since you know the header of the file, why not store is as a constant so it is not recreated every time you call the endpoint ?

Like so:

private static final String[] CSV_HEADER = new String[]{"User Id", "First Name", "Last Name", "Roll No", "Email ID", "Gender"});

I think you could do something about the for loop with some of Java 8 fanciness but I am not confident enough to write this without tools for testing.

Lastly, you don't have to do writer.close() since you are opening your file with a try with resource

Basically, CSVWriter class should implement AutoCloseable wich call close() method for you at the end of the try statement so you do not have to worry about this.

Documentation about this is here

Hope my answer helps !


Copying and pasting one inputs value into other input with the same name I am trying to write something that will copy the current <input>s value and enter it into any <input> that start with the same name.

<input> names will follow this pattern: price-0, price-1, price-2, upc-0, upc-1, upc-2.

So if a user enters a value in <input name="price-0"> and hits copy the value should be transferred over to all input whos name start with price

This is the code I've written:

$(document).on('click', '.--copy', function () {
    var input_name = $(this).closest('div').find('input').attr('name').split('-')[0];
    $('input[name^=' + input_name + ']').val($(this).closest('div').find('input').val());
});

A fiddle to make everyones life easier: http://jsfiddle.net/6jGLD/

I feel like there are too many selectors being called upon and the code is somewhat difficult to read.

Why not at least save the intermediate to avoid rerunning that:

$(document).on('click', '.--copy', function () {
    var obj = $(this).closest('div').find('input');
    var input_name = obj.attr('name').split('-')[0];
    $('input[name^=' + input_name + ']').val(obj.val());
});


Model classes for an expense tracking app in Django I want to build a simple expense tracking app in Django. Each user can create categories, and each category can have multiple expenses:

Additionally, I wanted to have a value field that shows the total value of the category per month. For this I am using the def save() function in the Expenses class and a def calc_category_value() function called from the signals. I was wondering if this approach is correct:

def calc_category_value(instance):
    today = date.today()
    days = calendar.monthrange(today.year,today.month)[1]
    range_start = "%s-%s-%s" % (today.year,today.month,1)
    range_end = "%s-%s-%s" % (today.year,today.month,days)
    category = Category.objects.get(id=instance.category.id)
    expenses = Expense.objects.filter(date__range=[range_start,range_end],category=category)
    total = 0
    for e in expenses:
        total += e.amount
    category.value = total
    category.save()

class Profile(models.Model):
    id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False)
    user = models.OneToOneField(User, on_delete=models.CASCADE)
    create_date = models.DateTimeField('date added', auto_now_add=True)
    modify_date = models.DateTimeField('date modified', default=timezone.now)

class Category(models.Model):
    id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False)
    user = models.ForeignKey(Profile, on_delete=models.CASCADE)
    name = models.CharField(max_length=200, unique=True)
    value = models.DecimalField(max_digits=11,decimal_places=2, default=0)
    create_date = models.DateTimeField('date added', auto_now_add=True)
    modify_date = models.DateTimeField('date modified', default=timezone.now)

    class Meta:
        verbose_name_plural = "Categories"

    def __unicode__(self):
        return self.name +' (' + str(self.value) + ')'

class Expense(models.Model):
    id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False)
    title = models.CharField(max_length=200, blank=True)
    amount = models.DecimalField(max_digits=11, decimal_places=2,default=0)
    category = models.ForeignKey(Category, on_delete=models.CASCADE)
    date = models.DateField(default=timezone.now)
    create_date = models.DateTimeField('date added', auto_now_add=True)
    modify_date = models.DateTimeField('date modified', default=timezone.now)

    def save(self, *args, **kwargs):
        super(Expense, self).save(*args, **kwargs)
        today = date.today()
        days = calendar.monthrange(today.year,today.month)[1]
        range_start = "%s-%s-%s" % (today.year,today.month,1)
        range_end = "%s-%s-%s" % (today.year,today.month,days)
        expenses = Expense.objects.filter(date__range=[range_start,range_end],category=self.category)
        total = 0
        for e in expenses:
            total += e.amount
        self.category.value = total
        self.category.save()

    class Meta:
        verbose_name_plural = "Expenses"
        order_with_respect_to = 'category'

    def __unicode__(self):
        return self.title +', ' + str(self.amount) +' (' + self.category.name + ')'

Signals.py:

@receiver(post_delete, sender=Expense)
def post_delete(sender, instance, **kwargs):
    calc_category_value(instance)

You would probably want to show total expenses in the category for each month, not just the last. So Category doesn't have a value. It has a value (which is a bad naming by the way) for each month. This can be modelled simply as a query like (pseudo sql) select sum(amount) from expenses where group = 'some group' group by date.month

And by the way you can use this declarative style here.

total = 0
for e in expenses:
    total += e.amount
category.value = total

category.value = sum(e.amount for e in expenses)


Grokking the basics of BlockChain with Python As I was streaming I had a brilliant visitor suggest we write a blockchain in Python.

So we did.

Note that there is no validation or voting simulated here.

Here's the results, in package/blockchain.py:

from __future__ import annotations
# https://mypy.readthedocs.io/en/latest/running_mypy.html#missing-imports
from oscrypto.symmetric import aes_cbc_pkcs7_encrypt as encrypt # type: ignore

class Node:
    __slots__ = 'prior_node', 'data', 'hash'
    prior_node: Node
    data: bytes
    hash: bytes

    def __init__(self, prior_node, key, data):
        self.prior_node = prior_node
        self.data = data
        if prior_node is None:
            init_vector = bytes(16)
        else:
            init_vector = _ensure_byte_length(prior_node.hash, 16)
        key = _ensure_byte_length(key, 32)
        self.hash = encrypt(key, data, init_vector)[1]

    def __repr__(self):
        return f'Node<{self.data}\n{self.hash}\n{self.prior_node}>'

def _ensure_byte_length(bytes_, length):
    return bytes(bytes_.ljust(length, b'\x00')[:length])

class Chain:
    __slots__ = 'nodes'
    def __init__(self, key: bytes, data: bytes):
        self.nodes = Node(None, key, data)

    def __repr__(self):
        return f'Chain:\n{self.nodes}'

    def new_node(self, key, data):
        self.nodes = node = Node(self.nodes, key, data)
        return node

    def __len__(self):
        length = 0
        nodes = self.nodes
        while nodes:
            length += 1
            nodes = nodes.prior_node
        return length

def main():
    chain = Chain(b'the key', b'here is a bit of data')
    chain.new_node(b'P@$$w0rd', b'and here is a bit more data')
    chain.new_node(b'hunter2', b'and finally here is some more')
    print(chain)

if __name__ == '__main__':
    main()

And here's a tiny test, tests/test_blockchain.py:

from package.blockchain import Chain

def test_blockchain():
    chain = Chain(b'the key', b'here is a bit of data')
    chain.new_node(b'P@$$w0rd', b'and here is a bit more data')
    chain.new_node(b'hunter2', b'and finally here is some more')
    assert len(chain) == 3

Note that we did require oscrypto and openssl to run this.

Ran coverage with pytest under Python 3.7. Ran with black and mypy as well.

By default mypy doesn't test much. This is because a large part of its philosophy is to allow dynamic and statically typed code at the same time. This is so migrating to mypy is easier and less daunting. Having thousands of errors when you start to port your legacy app is likely to scare a fair few mortals away.

Please use the --strict flag to have typed Python, rather than hybrid Python.

When you use the flag and you type all the functions and methods, you'll notice that there's an issue with Node.prior_node. Currently it's assigned the type Node, but we know that's a lie because we have if prior_node is None.
I personally use --ignore-missing-imports rather than ignoring each import, as they quickly build up over time.
Your __repr__ are non standard.

  Called by the repr() built-in function to compute the “official” string representation of an object. If at all possible, this should look like a valid Python expression that could be used to recreate an object with the same value (given an appropriate environment). If this is not possible, a string of the form <...some useful description...> should be returned.

You probably want to be using __str__.
I find it a little confusing that init_vector is being assigned two different things. It would make more sense if you pass in an empty bytes to _ensure_byte_length.

prior_hash = b'' if prior_node is None else prior_node.hash
init_vector = _ensure_byte_length(prior_hash, 16)

You could change the ternary into a getattr.

prior_hash = getattr(prior_hash, 'hash', b'')

I would change Node to a dataclass. Why write code when you can just not?

This would require moving the hash generation into a class method.
I would define an __iter__ method on the node so that we can easily traverse the chain from any node.

This makes the __len__ method of Chain really simple and clean.
I'd rename _ensure_byte_length to _pad. The function has two jobs, pad is well known and allows us to have a much shorter function name.
_ensure_byte_length doesn't need the extra bytes call.
The method Chain.add_node is un-Pythonic. In Python it's standard to return nothing from a function with mutations.

from __future__ import annotations

import dataclasses
from typing import Optional, Iterator

from oscrypto.symmetric import aes_cbc_pkcs7_encrypt as encrypt

@dataclasses.dataclass
class Node:
    prev_node: Optional[Node]
    data: bytes
    hash: bytes

    @classmethod
    def build(cls, key: bytes, data: bytes, prev_node: Optional[Node] = None) -> Node:
        prev_hash = b"" if prev_node is None else prev_node.hash
        hash = encrypt(_pad(key, 32), data, _pad(prev_hash, 16))[1]
        return cls(prev_node, data, hash)

    def __iter__(self) -> Iterator[Node]:
        node: Optional[Node] = self
        while node is not None:
            yield node
            node = node.prev_node

def _pad(bytes_: bytes, length: int) -> bytes:
    return bytes_.ljust(length, b"\x00")[:length]

@dataclasses.dataclass
class Chain:
    node: Node

    def add_node(self, key: bytes, data: bytes) -> None:
        self.node = Node.build(key, data, self.node)

    def __len__(self) -> int:
        return sum(1 for _ in self.node)

def main() -> None:
    chain = Chain(Node.build(b"the key", b"here is a bit of data"))
    chain.add_node(b"P@$$w0rd", b"and here is a bit more data")
    chain.add_node(b"hunter2", b"and finally here is some more")
    print(chain)

if __name__ == "__main__":
    main()