function to get input group for user I got tasked with writing a page that will add as many profile entries as the user wants, and web programming is not at all my field. There are multiple input groups but I'm just using the "age" one as an example. Currently I'm using a function to create DOM elements, then I assemble them together and return the completed outer HTML code. Is this the correct way to do this? Here's my code:
function addInputGroup(userID, userAge) {
                let ageCol = document.createElement('div');
                ageCol.setAttribute('class', 'col-4');

                let ageLabel = document.createElement('label');
                ageLabel.setAttribute('class', 'sr-only');
                ageLabel.innerHTML = "Age";

                let ageInputGroup = document.createElement('div');
                ageInputGroup.setAttribute('class', 'input-group');

                let ageInputGroupPrepend = document.createElement('div');
                ageInputGroupPrepend.setAttribute('class', 'input-group-prepend');

                let ageInputGroupText = document.createElement('div');
                ageInputGroupText.setAttribute('class', 'input-group-text');
                ageInputGroupText.innerHTML = "Age";

                let ageInput = document.createElement('input');
                ageInput.setAttribute('id', 'fldAge'+userID);
                ageInput.setAttribute('type', 'text');
                ageInput.setAttribute('class', 'form-control');
                ageInput.setAttribute('placeholder', userAge);
                ageInput.setAttribute('value', userAge);

                //assemble age column
                ageInputGroupPrepend.appendChild(ageInputGroupText);
                ageInputGroup.appendChild(ageInputGroupPrepend);
                ageInputGroup.appendChild(ageInput);
                ageCol.appendChild(ageLabel);
                ageCol.appendChild(ageInputGroup);

                return(ageCol.outerHTML);
            }

Then I call document.write(addInputGroup("12345", "18")); when I need to create an "age" input group.
Does this look correct?

DOM
The outer HTML conversion is probably unecessary. You can manipulate DOM nodes directly. The appending could look something like this.
// note that the HTML representation is irrelevant; we go straight to the DOM
document.getElementById('input-groups').appendChild(ageCol)

JS
addInputGroup() seems to be a misleading name, as the function creates a new DOM element; it doesn't do anything with it. createInputGroup() would be more appropriate I think.
You can use const instead of let here. This provides some nice guarantees about consistency, and makes these guarantees immediately visible to the reader.
return(ageCol.outerHTML); is more commonly written as return ageCol.outerHTML;


Date and time of an article in a humanized and localized way The goal is displaying the date and time of an article in a humanized and localized way:

Yesterday
13:21

or if the Swedish language parameter is set it will display:

Igår
13:21

And if the date wasn't yesterday or today, it will print the date and 24-hour time. I think I succeeded with everything except handling the timezone:

def datetimeformat_jinja(value, format='%H:%M / %d-%m-%Y', locale='en'):
    now= datetime.now()
    info = None
    if datetime.date(value) == datetime.date(now):
      info= _('Today')
    elif (now - value).days < 2:
      info= _('Yesterday')
    else:
      month = value.month
      if month == 1:
        info = str(value.day)+' '+_('Jan')
      elif month == 2:
        info = str(value.day)+' '+_('Feb')
      elif month == 3:
        info = str(value.day)+' '+_('Mar')
      elif month == 4:
        info = str(value.day)+' '+_('April')
      elif month == 5:
        info = str(value.day)+' '+_('May')
      elif month == 6:
        info = str(value.day)+' '+_('June')
      elif month == 7:
        info = str(value.day)+' '+_('July')
      elif month == 8:
        info = str(value.day)+' '+_('Aug')
      elif month == 9:
        info = str(value.day)+' '+_('Sep')
      elif month == 10:
        info = str(value.day)+' '+_('Oct')
      elif month == 11:
        info = str(value.day)+' '+_('Nov')
      else:
        info = str(value.day)+' '+_('Dec')
    return info+'<br>'+format_time(value, 'H:mm', locale=locale)

The above code localizes and humanizes the output:

Today
3:29

I can also switch languages to, for example, Portuguese:

Hoje
3:29

Where "Hoje" means "today," so the filter appears to work.

Could you please review it or give me a hint on how I can admit my code to also allow time zones? I use babel for localization and Jinja2 for rendering.

Adding time zone support

Add a new argument to your function: now. This should be the localized datetime of the client. Compare now to the current server datetime and adjust the formatted time by the difference between them. Boom, timezone support added.

That said you cannot get the client timezone server-side. Look into client-side solutions with JavaScript (either full client-side date formatting, or phoning home). This should start you off on the right track.

On with the code review.

Tidying up the code

Variable and function naming

info and value are excessively vague.
datetimeformat_jinja() is a bit eccentric  as a function name, as its not in imperative form (eg format_datetime).

String formatting

Concatenating strings with addition is considered bad practice is Python. You should format individual elements of your message together, rather than starting with info='' and adding new parts as you go along. Use str.format() (or %s-formatting with Python 2.5 and below) to achieve this.

The If-elif-else chain

In lines 9 through 33 you copy-pasted the same logic twelve times over; that's a sure-fire sign you need to express things more generically.

Style

Take this suggestion for the petty nothing it is, but—please, please, please—unless you're following the style guidelines of your team or employer, read and obey PEP 8.

Result

MONTHS = ('Jan', 'Feb', 'Mar', 'April', 'May', 'June',
          'July', 'Aug', 'Sep', 'Oct', 'Nov', 'Dec')
FORMAT = '%H:%M / %d-%m-%Y'

def format_datetime(to_format, format=FORMAT, locale='en'):
    now = datetime.now()
    if datetime.date(to_format) == datetime.date(now):
        date_str = _('Today')
    elif (now - to_format).days < 2:
        date_str = _('Yesterday')
    else:
        month = MONTHS[to_format.month - 1]
        date_str = '{0} {1}'.format(to_format.day, _(month))
    time_str = format_time(to_format, 'H:mm', locale=locale)
    return "{0}<br>{1}".format(date_str, time_str)


Simple python script to shorten URLs in specified text files A recent question inspired me to write a script for this task:

Shorten URLs in specified text files using googleapis.com
Handle multiple URLs, whether on the same line or multiple lines

The original question tried to do this in Bash, but it gets quite tricky when it comes to handling multiple URLs, so I decided to rewrite in Python:

#!/usr/bin/env python

import sys
import os
import re
import urllib2
import json

re_url_pattern = re.compile(r'http[s]?://(?:[a-zA-Z]|[0-9]|[$-_@.&+]|[!*\(\),]|(?:%[0-9a-fA-F][0-9a-fA-F]))+')

def shorten_url(url):
    print("Shortening url {} ...".format(url))
    post_url = 'https://www.googleapis.com/urlshortener/v1/url?fields=id'
    postdata = {'longUrl': url}
    headers = {'Content-Type': 'application/json'}
    req = urllib2.Request(post_url, json.dumps(postdata), headers)
    return json.loads(urllib2.urlopen(req).read())['id']

def shorten_urls_in_text(text):
    found_urls = False
    for url in set(re_url_pattern.findall(text)):
        short_url = shorten_url(url)
        text = text.replace(url, short_url)
        found_urls = True

    return found_urls, text

def shorten_urls_in_file(path):
    text = read_file(path)
    found_urls, text = shorten_urls_in_text(text)

    if found_urls:
        write_file(path, text)

def shorten_urls_in_files(*paths):
    for path in paths:
        if os.path.isfile(path):
            print('Shortening urls in file {} ...'.format(path))
            shorten_urls_in_file(path)
        else:
            print('No such file: {}'.format(path))

def read_file(path):
    with open(path) as fh:
        return fh.read()

def write_file(path, text):
    with open(path, 'w') as fh:
        fh.write(text)

if __name__ == '__main__':
    shorten_urls_in_files(*sys.argv[1:])

And some unit tests:

#!/usr/bin/env python

import unittest
from test.test_support import run_unittest

from shorten_urls import shorten_url
from shorten_urls import shorten_urls_in_text

sample_url1 = 'http://news.ycombinator.com'
sample_url1_shortened = 'http://goo.gl/ovZB'

class UrlShortenerTestCase(unittest.TestCase):
    def test_shorten_url(self):
        self.assertEqual(sample_url1_shortened, shorten_url(sample_url1))

    def test_replace_nothing(self):
        orig = 'hello'
        found_urls, text = shorten_urls_in_text(orig)
        self.assertFalse(found_urls)
        self.assertEqual(orig, text)

    def assert_correct_replacement(self, fmt):
        orig = fmt.format(url=sample_url1)
        expected = fmt.format(url=sample_url1_shortened)
        found_urls, text = shorten_urls_in_text(orig)
        self.assertTrue(found_urls)
        self.assertEqual(expected, text)

    def test_replace_one(self):
        fmt = '''hello world {url}'''
        self.assert_correct_replacement(fmt)

    def test_replace_many(self):
        fmt = '''hello world {url}\nnewline {url} again\n{url} last'''
        self.assert_correct_replacement(fmt)

    def test_replace_many_on_same_line(self):
        fmt = '''hello world {url} same line {url} again'''
        self.assert_correct_replacement(fmt)

def test_main():
    run_unittest(UrlShortenerTestCase)

if __name__ == '__main__':
    test_main()

It's not great that it needs to read the entire file in memory, but for handling relatively small files I thought it should be good enough.

I would like a review in terms of Python coding practices, style, and possible over-engineering.

I'm also wondering if urllib2 is considered more widely available than requests. I would prefer requests as it has simpler syntax, but I used urllib2 to make it more widely usable (if that's even true).

Overall, LGTM.

Suggestions:

try ... except would look better than os.path.isfile(). Even if isfile returns True, you still may encounter an exception trying to open it (say, insufficient privileges), so you should be ready to handle exceptions anyway. Besides, using isfile may lead to a race (not likely in this code), if a file is removed right between testing and opening it.
The script spends most of the time in shorten_url, which in turn mostly waits for network IO. I'd recommend to give each invocation a thread.
Original files are destroyed. I'd prefer to keep originals intact, or at least provide an option for that. Also an ability to read urls from standard input is usually desirable.


Calculating the surrounding objects in a two-dimensional coordinate system leaving out the corners We have a dynamically created map with two dimensional coordinates. There's an object in this map on a random position and I need to validate the four adjacent fields. However, I need to leave out the corners since they are not relevant for the validation!

This is an illustration of what needs to be validated (the green fields saying "v"):

  Instance-Example: Object ("O") is on (2;2)
  Line above the object: Y-Coordinate is 1 (so, 2 - 1 )
  Line above the object: Needed x-coordinate is 2 (so, 2 + 0 )
  Line of the object: Y-Coordinate is 2 (so, 2 + 0 )
  Line of the object: Needed x-coordinate is 1 (so, 2 - 1 )
  Line of the object: Needed x-coordinate is 3 (so, 2 + 1 )
  Line beneath the object: Y-Coordinate is 3 (so, 2 + 1 )
  Line beneath the object: Needed x-coordinate is 2 (so, 2 + 0 )

Here is my solution.  The core magic lays within the inner for loop.

public bool validateEdibility ()
{

    for (int line = -1; line < 2; line ++)
    {

        for (int column = Math.Abs(line) - 1; column < 2; column += 2)
        {

                if (!(this.MyWorld.getObjectOnPosition(this.Y + line, this.X + column) is Wall))
                {

                    return true;

                }

        }

    }

    return false;

}

The first idea behind this code is to get an integer which represents the subtrahend for the specific coordinate of the object. For example we need to have "-1" for the y-coordinate if we want to iterate through the first line (the line above the object).

With this idea we need to get a sequence like this:

Line: -1

Column: 0

Line: 0

Column: -1
Column: 1

Line: 1

Column: 0

The second idea is the horizontal for loop (the inner loop). We take the absolute of the line and subtract it with 1. This provides us the right first and last sequence since we've terminated the only difference between them: the negative sign.

Than we add +2 to the index for each run since the only line surviving this is the one in the middle ("0"). Remember: we subtract 1, so the 0-line starts with "-1".

I think this is quite a brainfuck and that's why I wonder if there's a more elegant way to solve this. Is the code evolving from these ideas somehow improvable?

This code is extremely hard to read and much longer than it needs to be.
Use documented naming conventions for C#.
Make small methods that do one thing well.

public bool IsWall(int x, int y)
{
  return this.MyWorld.GetObjectOnPosition(x, y) is Wall;
}
public bool IsValidForEditing()
{
  int x = this.X;
  int y = this.Y;
  return
    !IsWall(x-1, y) || !IsWall(x, y-1) ||
    !IsWall(x+1, y) || !IsWall(x, y+1);
}

Anyone who knows even basic C# -- or any C-like language -- can read this code and immediately understand what it means. Your original code, not so much.

Any time you write a loop, ask yourself "would this be more clear without the loop?"  Odds are good, yes. In this case, definitely.


Using AutoMapper to map to and from Sometimes, I need to map from a Domain entity to a ViewModel - to display information.

Other times, I need to map from a ViewModel to a Domain entity - for persistance of data.

Is this kosher or does this code smell?

protected void Application_Start()
{
    AreaRegistration.RegisterAllAreas();
    MapObjects();
    RegisterGlobalFilters(GlobalFilters.Filters);
    RegisterRoutes(RouteTable.Routes);
}

private void MapObjects()
{
    Mapper.CreateMap<UserModel, User>();

    Mapper.CreateMap<ProductBrandModel, ProductBrand>();
    Mapper.CreateMap<ProductBrand, ProductBrandModel>();

    Mapper.CreateMap<ProductCategoryModel, ProductCategory>();
    Mapper.CreateMap<ProductCategory, ProductCategoryModel>();
}

Mapping to and from domain models and view models is exactly what automapper is designed for.


Split a list into two parts with a given length I'm learning Common Lisp as a hobby and am trying tasks from L-99: Ninety-Nine Lisp Problems, but I'm not sure how good my code is. Here is one of the tasks which I solved:

GitHub solution

;;; (*) Split a list into two parts; the length of the first part is given.
;;; Do not use any predefined predicates.
;;;
;;; Example:
;;; * (split '(a b c d e f g h i k) 3)
;;; ((A B C) (D E F G H I K))

(defun split (given-list cnt &optional (result-left nil))
  (if (= cnt 0)
    (if (null result-left)
      given-list
      (cons result-left (list given-list)))
    (if (null given-list)
      result-left
      (split (cdr given-list) (- cnt 1) (append result-left (list (car given-list)))))))

Let’s use a completely different approach: first try to solve the problem using predefined (complex) functions, then implement those functions with the primitive ones (recursion, car, cdr, etc.)

So, how could be solved your problem in Common Lisp with predefined functions? I would suggest this simple solution (note the use of values instead of list to return two different values);

CL-USER> (defun split (list count)
           (values (subseq list 0 count) (nthcdr count list)))
SPLIT
CL-USER> (split '(A B C D E F G) 3)
(A B C)
(D E F G)

In other words, copy the first count values from the list to a new list with subseq (reference), then simply return the elements of the list starting from count with nthcdr (reference).

Or, if you prefer to use a single predefined function, with the following definition we make also a copy of the second part of the list (which is not strictly necessary):

(defun split (list count)
  (values (subseq list 0 count) (subseq list count)))

Now let's assume subseq it is not predefined and try to implement my-subseq with just recursion and primitive functions:

(defun copy-to-end (l end)
  "copy the list l from beginning to the element at position end - 1"
  (if (zerop end)
      nil
      (cons (car l) (copy-to-end (cdr l) (1- end)))))

(defun my-subseq (l start &optional (end (length l)))
  "copy the list l from the element at position start to the element at position end - 1"
  (if (zerop start)
      (copy-to-end l end)
      (my-subseq (cdr l) (1- start) (1- end))))

So the final function will be:

(defun split (list count)
  (values (my-subseq list 0 count) (my-subseq list count)))

It is left as an exercise to define my-nthcdr with recursion and primitive functions to implement my first version of the function split.


A less verbose const_cast Motivation

const_cast is a beast that I seldomly encounter. But when I do so it is mostly a bad experience.

Nobody knows what most of the C++ programmers do but so far I only ever had one use for const_cast: to cast away a const from a type. For this task const_cast is too verbose:

const std::vector<int> constVectorOfInts = {/*...*/};
// never do this in real code!
auto &mutableVectorOfInts = const_cast<std::vector<int>>(constVectorOfInts);

IMHO, there is no gain in readability when repeating the type.

Enter my proposal:

unconst_cast

We can write a template function unconst_cast that solves only the problem of removing const.

/**
 * @brief Returns a mutable reference to the same object
 */
template <class T> T &unconst_cast(const T &v) { return const_cast<T &>(v); }

/**
 * @brief Returns a mutable pointer to the same object
 */
template <class T> T *unconst_cast(const T *v) { return const_cast<T *>(v); }

int main() {
  int i;
  const int *constPointerToI = &i;
  int *unconstPointerToI = unconst_cast(constPointerToI);
  unconstPointerToI = unconst_cast(&i);
  int &unconstReferenceToI = unconst_cast(*constPointerToI);
  int &unconstReferenceToI2 = unconst_cast(i);
}

Our code from above would become:

auto &mutableVectorOfInts = unconst_cast(constVectorOfInts);

Other pros:

can overload for other types (in contrast to const_cast), e.g. for own iterators (mutableIterator = unconst_cast(constIterator))
we find usages when string searching for "const_cast"

Review Goals

Is this a bad idea?
Is my implementation correct?
Is the naming alright/understandable?

•Is this a bad idea?

It is unnecessary.

  Nobody knows what most of the C++ programmers do but so far I only ever had one use for const_cast: to cast away a const from a type.

You can also use it to partially take away constness:

int do_things(const char* p);

const char* const xyz = "abc";
do_things( const_cast<const char*>(xyz) );

The gain in readability comes from the result type being explicitly specified.

If you have APIs that only work for non-const pointers when they should work for const (this is the most legitimate use for const_cast) you should wrap them and centralize/localize/hide the call to const_cast.

If you find yourself writing const_cast calls again and again, the problem is not poor readability of the code, but bad API design.


Validation code tips if (isset($_POST['submit'])) {
    if (isset($_SESSION['token']) && $_POST['token'] == $_SESSION['token']) {
        $errors = array();
        $error = 0;

        if (!preg_match('/^[A-Za-z](?=[A-Za-z0-9_.]{3,20}$)[a-zA-Z0-9_]*\.?[a-zA-Z0-9_]*$/i', $_POST['username'])) {
            $errors[] = 'Your username must start with a letter (A-Z) and be between 3-20 characters and may only contain alphanumeric characters (A-Z, 0-9 _ ) or a period (1) (".")';
            $error = 1;
        }

        $STH = $DBH->prepare("SELECT username FROM users WHERE username = ?");
        $STH->execute(array($_POST['username']));

        if ($STH->rowCount() > 0) {
            $errors[] = 'The username is already taken!';
            $error = 1;
        }

        if (strlen($_POST['password']) < 3) {
            $errors[] = 'Your password must be longer than 3 characters!';
            $error = 1;
        } else if ($_POST['password'] != $_POST['passconf']) {
            $errors[] = 'You didn\'t verify your password correctly!';
            $error = 1;
        }

        if (!filter_var($_POST['email'], FILTER_VALIDATE_EMAIL)) {
           $errors[] = 'The e-mail is not valid!';
           $error = 1;
        } else {
            $STH = $DBH->prepare("SELECT email FROM users WHERE email = ?");
            $STH->execute(array($_POST['email']));

            if ($STH->rowCount() > 0) {
                $errors[] = 'The email is already taken!';
                $error = 1;
            }
        }
    }
}

This is my current validation for a registration page it works, but I'd like to improve it, give me some tips

You have two long if blocks:

if (isset($_POST['submit'])) {
    if (isset($_SESSION['token']) && $_POST['token'] == $_SESSION['token']) {
    ...
    }
}

Why not combine them?

if (isset($_POST['submit']) && (isset($_SESSION['token']) && $_POST['token'] == $_SESSION['token'])) {
    ...
}

also the way you've structured this, you don't need $error, to see if there are errors you can simply check empty($errors) (true => ok, false => not ok).

Other than that, your code looks fine.


Javascript pager logic I implemented a function for pager link creation, it looks like this:

function createPagerObject(pageUri, page, totPages) {

    const MAX_PAGES_IN_PAGER = 6;
    // for the first three pages the starting would be 1
    let pageNumber = Math.sign(page - 3) > 0 ? page - 3 : 1;

    let pager = [];
    for(let i = 0; i < MAX_PAGES_IN_PAGER; i++){
        pager.push({
            "page": pageNumber,
            "href": pageUri.replace("list-" + page, "list-" + pageNumber),
            "type": pageNumber == page ? "current" : pageNumber > page ? "next" : "prev"
        });
        pageNumber++;
        if(pageNumber > totPages) {
            break;
        }
    }

    return pager;
}

the code is self explanatory, but the idea is to create an Array of objects with the structure:

  page → the page number
  href → the link to the destination page
  type → "current", "next" or "prev" depending on the current page

having as input the current page URI, the current page number and the number of pages.

The question is "it could be improved", because it seems not so clean to me.

Assuming ES6 is available (you tagged ES6, you have let and const, etc.), this can be simplified.

First, you can compute ahead the number of pages you want to include. You don't need to loop through to check. In this case, your loop depends on who is lesser between MAX_PAGES_IN_PAGER and totPages. You can use Math.min.

const limit = Math.min(totPages, MAX_PAGES_IN_PAGER);

Next, instead of loops and push, you can use array methods. Let's start off by taking out the loop. To loop n times with arrays, you can create a filled array with n dummy items. This is possible with Array(n).fill(0). Then, instead of push, we use map. We turn the n dummy values into values we need.

return Array(limit).fill(0).map(_ => ({
  page: ...,
  href: ...,
  type: ...
}));

In other news, use template strings to construct strings instead of concatenating. Note that template strings use backticks instead of quotes.

pageUri.replace(`list-${page}`, `list-${pageNumber}`)

Ternaries are hard to read. However, there's a way to keep them readable. What you are doing is right, putting further conditions at the false end of the ternary. They can be written in a cascading manner for readability.

"type": pageNumber == page ? "current"
       : pageNumber > page ? "next"
                           : "prev"

Lastly, name your variables properly. totPages is confusing because "tot" doesn't tell me what it is. pager is confusing because it sounds like an object with methods, when it's just an array of objects representing pages.

Also, quotes on object keys are optional unless you are using key names with invalid characters.

In the end:

function createPageList(pageUri, page, totalPages) {

  const limit = Math.min(totalPages, 6);

  return Array(limit).fill(0).map((fill, index) => {

    const pageNumber = (page < 3 ? 1 : page - 3) + index ;

    return {
      page: pageNumber,
      href: pageUri.replace(`list-${page}`, `list-${pageNumber}`)
      type: pageNumber == page ? "current"
           : pageNumber > page ? "next"
                               : "prev"
    };
  });
}


Context menu with click handlers I'm trying to learn how to structure my jQuery code better. I'm only about a week into jQuery so if something looks way off please tell me. I'm attempting to use object literals and it appears to work ok, but I am wondering if there is possibly a better way to structure my code.

var application = {
    wrapper: $('#wrapper'),
    addButton: $('#add'),
    init: function() {
        this.addButton.click(this.buttonHandlers.clickHandler);
        this.wrapper.on("contextmenu", "#wrapper div", this.buttonHandlers.contextMenuHandler);
    },
    buttonHandlers: {
        clickHandler: function(e) {
            console.log("clickyHandler");
        },
        contextMenuHandler: function(e) {
            console.log("right click");
            return false;
        }
    }
};

application.init();

This won't scale if you entire application is one large object.

If you seperated it into multiple objects

var application = []

application.push({
  wrapper: $("#wrapper"),
  init: function () {
    this.wrapper.on(bla, this.foo)
  },
  foo: function () {

  }
})

...

applications.forEach(function (app) { app.init() })

it will becomes significantly more maintainable.

You may also want to look into techniques for spreading application infrastructure across multiple files. For example ncore uses browserify for it's module system, and uses dependency injection and an expected object format to stitch multiple "modules" together.


Simple IPSec/tunnel setup script I've developed a script to set up an encrypted tunnel between two Linux hosts, using iproute2, ssh and setkey.
The goal is to allow setting up ad-hoc secure tunnels with minimum of setup and dependencies.

Mirrored here:

#!/bin/bash

# Setup encrypted IPv4 tunnel over IPv4 or IPv6 on two Linux nodes using SSH for tunnel setup.
# Requires only ipsec-tools, iproute2, ssh and necessry kernel modules locally and remotely.

# Warning: it flushes IPsec settings both locally and remotely.
# Don't use with other IPsec tunnnels.

# Sample usage:
# simplevpn -6 fc::1 fc::2    ssh -T root@fc::2
#    fc::1 is your IPv6 address
#    fc::2 is other peer's IPv6 address
# after successful run it should create tunnel named "simplevpn" locally and remotely
# and set up addresses 192.168.77.1 and 192.168.77.2 locally and remotely respectively

# Note: tested only once in my configuration. This is not a serious production-ready VPN solution.

# Implemented by Vitaly "_Vi" Shukela in 2013, License=MIT

MODE="ipip"
PROT="-4"

if [ "$1" == "-6" ]; then
    shift;
    MODE=ipip6
    PROT="-6"
fi

SRC="$1"; shift
DST="$1"; shift

if [ -z "$1" ]; then
    echo Usage: simplevpn [-6] source_ip destination_ip ssh_command_line
    exit 1;
fi
set -e

KEY1=0x`dd if=/dev/urandom count=32 bs=1 2> /dev/null| xxd -p -c 64`
KEY2=0x`dd if=/dev/urandom count=32 bs=1 2> /dev/null| xxd -p -c 64`

true ${LOCALIP:="192.168.77.1"}
true ${REMOTEIP:="192.168.77.2"}
true ${DEVNAME:="simplevpn"}

# 4 is encapsulated IPv4 both in IPv4 an IPv6
setkey -c << EOF
    flush;
    spdflush;
    spdadd $SRC $DST   4 -P out ipsec esp/transport//require  ah/transport//require;
    spdadd $DST $SRC   4 -P in ipsec esp/transport//require  ah/transport//require;
    add $SRC $DST esp 0x4444 -E rijndael-cbc $KEY1 ;
    add $DST $SRC esp 0x4444 -E rijndael-cbc $KEY1 ;
    add $SRC $DST ah 0x4445 -A hmac-sha256 $KEY2 ;
    add $DST $SRC ah 0x4445 -A  hmac-sha256 $KEY2 ;
EOF

modprobe ip6_tunnel
ip $PROT tunnel del $DEVNAME || true

ip $PROT tunnel add  $DEVNAME mode $MODE local $SRC remote $DST
ip link set $DEVNAME up
ip -4 addr add $LOCALIP/32 dev $DEVNAME
ip -4 route add $REMOTEIP/32 dev $DEVNAME

"$@" << EOF
    set -e
    # the same as above, but "in" and "out" swapped
    setkey -c << EOF2
        flush;
        spdflush;
        spdadd $SRC $DST   4 -P in ipsec esp/transport//require  ah/transport//require;
        spdadd $DST $SRC   4 -P out ipsec esp/transport//require  ah/transport//require;
        add $SRC $DST esp 0x4444 -E rijndael-cbc $KEY1 ;
        add $DST $SRC esp 0x4444 -E rijndael-cbc $KEY1 ;
        add $SRC $DST ah 0x4445 -A hmac-sha256 $KEY2 ;
        add $DST $SRC ah 0x4445 -A  hmac-sha256 $KEY2 ;
EOF2

    modprobe ip6_tunnel
    ip $PROT tunnel del $DEVNAME || true

    ip $PROT tunnel add  $DEVNAME mode $MODE remote $SRC local $DST
    ip link set $DEVNAME up
    ip -4 addr add $REMOTEIP/32 dev $DEVNAME
    ip -4 route add $LOCALIP/32 dev $DEVNAME
EOF

Are there any glaring errors? Do you have any suggestions (apart from not flushing entire security tables)? I'm primarily interested in security issues.

As far as I can tell, it checks if you provide too many options, but it doesn't check if you provide too few. This should be done before trying to do anything with SRC and DST. set -o errexit -o nounset as the first line would be prudent.

set stuff is not inherited by subshells. So to make a failure in the dd command propagate you should start with set -o errexit -o pipefail. A simpler way to achieve the same effect would be to add -o pipefail to set, create a function with the dd | xxd commands, and use this directly instead of assigning to variables.

What happens if dd fails, for whatever reason? For example, dd() { echo foo; } will override the command. Use command dd [sic] if you want to make sure it doesn't use shell builtins, aliases or functions.
There's no == operator in POSIX [ - AFAIK this only works by accident on some systems. You can either use Bash' [[ or a single =.
|| true can hide issues with the preceding command. Either check first if the command should be run before trying (and not ignoring the error), or run the command and check if the exit code matches the one you expect.
Use More Quotes™

I can't comment on the safety of the KEYn generation or setkey use.


Tic Tac Toe Game Python I've just started a self-learning "course" on Python practical programming for beginners. I found this book online, and decided to go through the chapters.

The book had a half-complete implementation of a Tic-Tac-Toe game, and I was supposed to finish it as an exercise. This is my code so far:

theBoard = {'top-L': ' ', 'top-M': ' ', 'top-R': ' ',
        'mid-L': ' ', 'mid-M': ' ', 'mid-R': ' ',
        'low-L': ' ', 'low-M': ' ', 'low-R': ' '}

def printBoard(board):
    """ This function prints the board after every move """

    print(board['top-L'] + '|' + board['top-M'] + '|' + board['top-R'])
    print('-+-+-')
    print(board['mid-L'] + '|' + board['mid-M'] + '|' + board['mid-R'])
    print('-+-+-')
    print(board['low-L'] + '|' + board['low-M'] + '|' + board['low-R'])

def checkWin(board):
    """
    This functions checks if the win condition has been
    reached by a player
    """
    flag = False
    possibleWins = [['top-L', 'top-M', 'top-R'],
                    ['mid-L', 'mid-M', 'mid-R'],
                    ['low-L', 'low-M', 'low-R'],
                    ['top-L', 'mid-L', 'low-L'],
                    ['top-M', 'mid-M', 'low-M'],
                    ['top-R', 'mid-R', 'low-R'],
                    ['top-L', 'mid-M', 'low-R'],
                    ['top-R', 'mid-M', 'low-L']]

    for row in range(len(possibleWins)):
        temp = board[possibleWins[row][0]]
        if temp != ' ':
            for position in possibleWins[row]:
                if board[position] != temp:
                    flag = False
                    break
                else:
                    flag = True
            if flag:
                return True

    return False

turn = 'X'
for i in range(9):
    printBoard(theBoard)
    print('Turn for ' + turn + '. Move on which space?')
    while True:
        move = input()
        if move in theBoard:
            if theBoard[move] != ' ':
                print('Invalid move. Try again.')
            else:
                break
        else:
            print('Invalid move. Try again.')
    theBoard[move] = turn
    if checkWin(theBoard):
        printBoard(theBoard)
        print('Player ' + turn + ' wins!')
        break
    if turn == 'X':
        turn = 'O'
    else:
        turn = 'X'

My checkWin function is very "stupid", it detects a win based on predetermined scenarios, and may not be very efficient in that regard as well. What if the board was of an arbitrary size nxn? Is there an algorithm to determine the victory condition without having to rewrite the entire game?

For a 3x3 tic-tac-toe board it hardly provides any real advantage, but if you where to play on an nxn board, with n consecutive same player marks determining a winner, you could do the following...

class TicTacToe(object):
    def __init__(self, n=3):
        self.n = n
        self.board = [[0 for j in range(n)] for k in range(n)]
        self.row_sum = [0 for j in range(n)]
        self.col_sum = [0 for j in range(n)]
        self.diag_sum = 0
        self.diag2_sum = 0

    def add_move(self, player, row, col):
        assert player in (0, 1)
        assert 0 <= row < self.n and 0 <= col < self.n
        delta = [-1, 1][player]
        winner = None
        self.board[row][col] = delta
        self.row_sum[row] += delta:
        if self.row_sum[row] = delta * self.n:
            winner = player
        self.col_sum[col] += player:
        if self.col_sum[col] = player * self.n:
            winner = player
        if col == row:
            self.diag += delta
            if self.diag == delta * self.n:
                winner = player
        if row == self.n - row - 1:
             self.diag2 += delta
             if self.diag2 == delta* self.n:
                 winner = player
        return winner

You basically initialize the board to all zeros, use -1 for one player and 1 for the other, and keep track of the sum of values in each row, column and diagonal. Whenever you add a new move, you can update all your data in constant time, and check in constant time if the player has won.


Adding two binary numbers I want to improve this code with better logic and using STL.

#include <iostream>
#include <vector>

void addition(const std::vector<int>& A,
              const std::vector<int>& B,
                    std::vector<int>& C)
{
   int size_A = A.size();
   int size_B = B.size();
   int max_size;
   if (size_A >= size_B)
   {
      max_size = size_A;
   }
   else
   {
      max_size = size_B;
   }

   int carry = 0, sum = 0;

   int i = size_A - 1, j = size_B - 1, k = max_size;
   C.resize(k + 1);

   while ( i >= 0 && j >= 0 && k >= 0)
   {
      sum = A[i] + B[j] + carry;

      if (sum == 0)
      {
         carry = 0;
         C[k] = 0;
      }
      else if (sum == 1)
      {
         carry = 0;
         C[k] = 1;
      }
      else if (sum == 2)
      {
         carry = 1;
         C[k] = 0;
      }
      else if (sum == 3)
      {
         carry = 1;
         C[k] = 1;
      }
      i--;
      j--;
      k--;
   }
  //if size of first binary number is greter than first
  if (max_size == size_A)
  {
     while (i >= 0 && k >= 0)
     {
        sum = A[i] + carry;

        if (sum == 0)
        {
           carry = 0;
           C[k] = 0;
        }
        else if (sum == 1)
        {
           carry = 0;
           C[k] = 1;
        }
        else if (sum == 2)
        {
           carry = 1;
           C[k] = 0;
        }
        else if (sum == 3)
        {
           carry = 1;
           C[k] = 1;
        }
        i--;
        k--;
     }
  }
  //If size of second binary number is greater than first
  else if (max_size == size_B)
  {
     while (j >= 0 && k >= 0)
     {
       sum = B[j] + carry;

       if (sum == 0)
       {
          carry = 0;
          C[k] = 0;
       }
       else if (sum == 1)
       {
          carry = 0;
          C[k] = 1;
       }
       else if (sum == 2)
       {
          carry = 1;
          C[k] = 0;
       }
       else if (sum == 3)
       {
          carry = 1;
          C[k] = 1;
       }
       j--;
       k--;
     }
  }
  C[0] = carry;
}

int main()
{
   int n1, n2, val;
   std::cout << "Enter the  number of bits for first binary numbers \n";
   std::cin >> n1;
   std::cout << "Enter the  number of bits for second binary numbers \n";
   std::cin >> n2;

   std::vector<int> A;
   std::vector<int> B;
   std::vector<int> C;

   std::cout << "Enter bits for first binary number \n";
   for (int i = 0; i < n1; i++)
   {
      std::cin >> val;
      A.push_back(val);
   }

   std::cout << "Enter bits for second binary number \n";
   for (int i = 0; i < n2; i++)
   {
     std::cin >> val;
     B.push_back(val);
   }

   std::cout << "First number    : ";
   for (int i = 0; i < A.size(); i++)
   {
      std::cout << A[i];
   }

   std::cout << "\nSecond number   : ";
   for (int i = 0; i < B.size(); i++)
   {
      std::cout << B[i];
   }

   addition(A, B, C);

   std::cout << "\nResult          : ";
   for (int i = 0; i < C.size(); i++)
   {
     std::cout << C[i];
   }
   std::cout << "\n";
}

Write DRY code:

DRY - Don't repeat yourself.

std::cout << "Enter bits for first binary number \n";
for (int i = 0; i < n1; i++)
{
   std::cin >> val;
   A.push_back(val);
}

std::cout << "Enter bits for second binary number \n";
for (int i = 0; i < n2; i++)
{
  std::cin >> val;
  B.push_back(val);
}

These two bits of code are way to similar. Make a function that does this work and pass as parameters any differences.

Highest order bit is where?

  std::cin >> val;
  B.push_back(val);

With this code your most significant bit is at location 0. This gives you no room to expand the number. Put the least significant bit at 0 and the more significant bits at higher locations. This will make it easier to do the standard maths operations in the long run.

This looks like a switch:

  if (sum == 0)
  {
     carry = 0;
     C[k] = 0;
  }
  else if (sum == 1)
  {
     carry = 0;
     C[k] = 1;
  }
  else if (sum == 2)
  {
     carry = 1;
     C[k] = 0;
  }
  else if (sum == 3)
  {
     carry = 1;
     C[k] = 1;
  }

I would write as:

  switch (sum) {
      case 0: carry = 0;C[k] = 0;break;
      case 1: carry = 0;C[k] = 1;break;
      case 2: carry = 1;C[k] = 0;break;
      case 3: carry = 1;C[k] = 1;break;
      default: throw std::exception("Something very wrong happened");
  }

Another case of DRY:

  while ( i >= 0 && j >= 0 && k >= 0)
  {
      sum = A[i] + B[j] + carry;
      // STUFF
      k--;i--;j--;
  }
  //if size of first binary number is greter than first
  if (max_size == size_A)
  {
     while (i >= 0 && k >= 0)
     {
        sum = A[i] + carry;
        // STUFF
        k--;i--;
     }
  }
  //If size of second binary number is greater than first
  else if (max_size == size_B)
  {
     while (j >= 0 && k >= 0)
     {
       sum = B[j] + carry;
       // STUFF
      k--;j--;
     }
  }

Not only can all the "stuff" by factored out into its own function. But those three loops can be combined into a single loop.

  for(;k >= 0;--k)
  {
      int a = (i > 0) ? A[i--] : 0
      int b = (j > 0) ? B[j--] : 0;
      sum = a + b + carry;
      // STUFF
  }


Folder watching in Go To try and track down a troublesome AS400 bug, my company wants me to keep track of the order in which files are put into a folder. I spent an hour whipping something up in Go, but I'm still pretty new to Go so I'd like an opinion on a couple points:

Idiomatic Go: how do I make it more "Go"-like?
My method of freezing os.FileInfo objects

package main

import (
    "flag"
    "fmt"
    "io/ioutil"
    "log"
    "os"
    "time"
)

var POLLTIME = 10 * time.Second

type watchinfo struct {
    head     string      // path/to/the/
    fileinfo os.FileInfo // file
    seen     time.Time
}

// freeze the os.FileInfo to be stored in a map[miniFileInfo]bool
type miniFileInfo struct {
    name string
    size int64
}

func watchFolder(dirname string, ch chan<- watchinfo) {
    set := make(map[miniFileInfo]bool)
    for {
        fis, err := ioutil.ReadDir(dirname)
        if err != nil {
            panic("I failed to read " + dirname)
        }
        for _, fi := range fis {
            minifi := miniFileInfo{name: fi.Name(), size: fi.Size()}
            if _, ok := set[minifi]; !ok {
                // fi doesn't exist in the set, add it
                set[minifi] = true
                now := time.Now()
                // and send its watchinfo to the channel
                ch <- watchinfo{head: dirname, fileinfo: fi, seen: now}
            }
        }
        time.Sleep(POLLTIME) // let's not spam the server
    }
}

// WatchFolders creates a goroutine for each folder to watch
func WatchFolders(dirnames []string, outch chan<- string) {
    inch := make(chan watchinfo)
    for _, dir := range dirnames {
        go watchFolder(dir, inch)
    }
    for watch := range inch {
        outch <- fmt.Sprintf("%s\\%s with modified time %s", watch.head, watch.fileinfo.Name(), watch.fileinfo.ModTime().Format("Jan 2 15:04:05"))
    }
}

func main() {
    flag.Parse()
    ch := make(chan string)
    go WatchFolders(flag.Args(), ch)
    for seenat := range ch {
        log.Println(seenat)
    }
}

My original problem seemed to stem from using os.FileInfo objects directly. Every polling period would show all the files as "new." I'm not sure how Go handles identity in maps (in Python I could implement __hash__ to tell it how to behave) but as long as the file has the same name and the same size in bytes, I don't mind assuming it's the same file.

I overengineered a bit to allow for future use, allowing for multiple directories (I only need one) and keeping the first seen time in the struct (I can just use the log time for now).

Your code looks ok to me, but here are a couple of points to improve:

You used the flag package to obtain the arguments to your program. The original intention of the flag package is to process optional, named arguments to your program and store their parsed values into variables (designated by pointers). Using it just to obtain a list of arguments is somewhat "overkill", better would be just to use a simple variable in the os package: os.Args. os.Args holds the command-line arguments, starting with the program name. Since you don't need the program name, skip that by re-slicing this slice. Your new main() function:

func main() {
    ch := make(chan string)
    go WatchFolders(os.Args[1:], ch)
    for seenat := range ch {
        log.Println(seenat)
    }
}

By doing this we also got rid of the flag package dependency (so it won't have to be added to your compiled executable; and os was already used, so that's not a plus).

Next on to your map utilized as a Set. This is how you check if something is in your map:

if _, ok := set[minifi]; !ok {
    // fi doesn't exist in the set, add it
}

This works, but can be written simpler. Note that if a key is not in the map, indexing a map yields the zero value of the value type of the map (Spec: Index expressions). Zero value for the bool type is false. So the expression set[minifi] will be true if minifi is in the map (because when you put it into the map, you stored true as the value for this key), and it will be false if minifi is not in the map (the zero value of bool). So this can be rewritten as follows:

if !set[minifi] {
    // fi doesn't exist in the set, add it
}

Now an explanation why your original os.FileInfo didn't work as the key. When you want to find an element in a map by key, Go uses the == comparison operator to test for equality of keys (hash code is generated and used under the hood to speed up the lookups, but that is irrelevant now).

This is how the spec defines the terms and the result of the comparision. Quoting the relevant part from Spec: Comparision operators:

  Struct values are comparable if all their fields are comparable. Two struct values are equal if their corresponding non-blank fields are equal.
  Interface values are comparable. Two interface values are equal if they have identical dynamic types and equal dynamic values or if both have value nil.
  Pointer values are comparable. Two pointer values are equal if they point to the same variable or if both have value nil. Pointers to distinct zero-size variables may or may not be equal.

Your miniFileInfo type is a struct, and 2 values of the same struct type are equal if all their fields are comparable and are equal. So you will find a value in the map if they match by name and size.

os.FileInfo is an interface, so a different comparison rule applies here. At runtime interface values can hold values of different dynamic type. Both the dynamic type and the value must match in order to claim 2 interface values equal. (You can read more about the representation of interface values in blog post The Laws of Reflection: The representation of an interface.)

The dynamic type of the values stored os.FileInfo interface is a pointer, a pointer to the unexported type os.fileStat. So now we need to apply the comparison rule for Pointers. 2 pointers are equal if they point to the same variable.

So 2 values of os.FileInfo will match only if the pointers stored in them match. You used ioutil.ReadDir() to obtain the slice of FileInfos, and whenever you call it, it creates new os.fileStat pointers and hence they will never be equal, meaning the wrapper os.FileInfo interface values will never be equal, meaning indexing the map with these FileInfo values will never yield any results.


Managing user defined and default values for instance attributes The following works as expected but I wonder if there is a more idiomatic way to check the input kwargs against the user-required (non-defaulted) arguments.

It is written in this way: so that as I continue to develop and modify my script and add attributes, I simply need to add them to the class variable defaults = {'A':None, 'B':0, 'C':0} and set it to None if the user is required to specify it. I also like that managing this as a class variable is visible at this time.

I've included a modified adaptation from item #6 in this excellent answer that makes sure all of the arguments end up as either floats or np.arrays with all the same length.

If they are: the attributes are set to default or user values and .ok set to True
If not: attributes are not set to defaults or user values and .ok remains False

In this example, a value for A is required of the user. They may enter values for B and C but if not those are initialized to 0.0. Any extra arguments such as D=42 will just be ignored.

import numpy as np

class O(object):
    defaults = {'A':None, 'B':0, 'C':0}
    required = [key for (key, value) in defaults.items() if value == None]
    ok       = False

    def __init__(self, **kwargs):
        if not all([key in kwargs for key in self.required]):
            print('problem, something required is missing')
        setup = self.defaults.copy()
        for (key, value) in kwargs.items():
            if key in setup:
                setup[key] = kwargs[key]  # user specified overrides default
        setup = self.fixem(setup)
        if setup:
            for (key, value) in setup.items():
                setattr(self, key, value)
            self.ok = True
        else:
            print('something did not work')

    def fixem(self, setup):
        # adapted from https://codereview.stackexchange.com/a/233169/145009
        results        = None
        keys, values   = zip(*setup.items())
        arrays         = list(map(np.atleast_1d, values))
        sizes_ok       = len(set(map(np.size, arrays)).difference(set((1,)))) <= 1
        all_1d         = set(map(np.ndim, arrays)) == set((1,))
        all_good_types = all(array.dtype in (np.int64, np.float64) for array in arrays)
        if all([sizes_ok, all_1d, all_good_types]):
            arrays = [array.astype(float) for array in arrays]  # make all arrays np.float64
            values = list(map(lambda x: float(x) if len(x) == 1 else x, arrays)) # downcast length=1 arrays to float
            results = dict(zip(keys, values))
        return results

# TESTING:

attrs = ('A', 'B', 'C')

print('\nBEGIN good seup testing: ')

o = O(A=42)
print("\nEXPECT:[('A', 42.0), ('B', 0.0), ('C', 0.0)]")
print('GOT:  ', [(attr, getattr(o, attr)) for attr in attrs if hasattr(o, attr)])

o = O(A=[1, 2, 3], B=np.exp(1), C=np.array([2, 3, 4]))
print("\nEXPECT:[('A'. array([1., 2., 3.])), ('B', 2.718281828459045), ('C', array([2., 3., 4.]))]")
print('GOT:  ', [(attr, getattr(o, attr)) for attr in attrs if hasattr(o, attr)])

print('\nBEGIN bad seup testing: \n')

o = O(B=42)
print('\nEXPECT:[] (i.e. nothing!)')
print('GOT:  ', [(attr, getattr(o, attr)) for attr in attrs if hasattr(o, attr)])

o = O(A=[1, 2, 3], B=[1, 2, 3, 4])
print('\nEXPECT:[] (i.e. nothing!)')
print('GOT:  ', [(attr, getattr(o, attr)) for attr in attrs if hasattr(o, attr)])

OUTPUT:

BEGIN good seup testing:

EXPECT:[('A', 42.0), ('B', 0.0), ('C', 0.0)]
GOT:   [('A', 42.0), ('B', 0.0), ('C', 0.0)]

EXPECT:[('A'. array([1., 2., 3.])), ('B', 2.718281828459045), ('C', array([2., 3., 4.]))]
GOT:   [('A', array([1., 2., 3.])), ('B', 2.718281828459045), ('C', array([2., 3., 4.]))]

BEGIN bad seup testing:

problem, something required is missing
something did not work

EXPECT:[] (i.e. nothing!)
GOT:   []
something did not work

EXPECT:[] (i.e. nothing!)
GOT:   []

Some minor comments on the code:

Class definition should be separated from the line with import by two spaces.
My personal preference is to have key-value pairs in dictionaries separated with a space after colon as shown in PEP 8:

defaults = {'A': None, 'B': 0, 'C': 0}

Comparing to None should be done by is instead of ==:

required = [key for key, value in defaults.items() if value is None]

Note that I also removed redundant brackets around key, value.  There are several other lines where brackets are not needed around them.
PEP 8 also discourages aligning several lines with assignments by =, so instead of, for example:

results        = None
keys, values   = zip(*setup.items())

it should be

results = None
keys, values = zip(*setup.items())

There is no need to specify object in class O(object), class O will work fine.
Here:

for key, value in kwargs.items():
    if key in setup:
        setup[key] = kwargs[key]  # user specified overrides default

you don't use value, but you could:

for key, value in kwargs.items():
    if key in setup:
        setup[key] = value

Here:

  keys, values   = zip(*setup.items())

you don't need values as you overwrite them later. So, I'd just remove this line altogether.
set((1,)) can be replaced with {1}, and set.difference can be replaced with just -. BTW, I like how you combined two conditions from my previous review in one!
Don't forget to use np.can_cast instead of checking the dtypes against np.int64. The current version failed for me until I changed it.
[array.astype(float) for array in arrays] can be written as list(map(np.float64, arrays)) but both versions are fine.
The overall design looks quite unusual to me. If it would be me, I'd separate validating data from the container that will keep it. In other words, I'd not keep it in one class. BTW, if a class has just two methods and one of them is __init__ then it shouldn't be a class. Another thing you could try is pydantic library. Never had a chance to try it myself though, but with this problem of data validation I'd give it a shot.


Pagination implementation for a blog system Last night I was working on a project that I've embarked upon as a learning exercise. I decided to add a pagination feature to my blog system. I made an attempt at it and got close... but I decided to undo my changes and I searched online for other 'better' solutions. I found this example which uses PagedList.Mvc NuGet and this example from asp.net which uses the same package.

Long story short, I decided that before I install another package I would make one more attempt at my own implementation and I was successful... so, on to my question.

Have I missed something important in my implementation that should make me want to reconsider my approach?

Here's my relevant Action in my HomeController.cs:

public ActionResult Index(int num = 0)
{
    var postsPerPage = 3;
    ViewBag.startNum = num;

    using (UnitOfWork uwork = new UnitOfWork())
    {
        IEnumerable<Post> posts = uwork.PostRepository.GetAll().ToList();

        int totalPosts = posts.Count();
        ViewBag.pages = totalPosts / postsPerPage;
        int skip = num * postsPerPage;

        return View(posts.Skip(skip).Take(postsPerPage));
    }
}

and this is the relevant code from my Index.cshtml:

@Html.ActionLink("Home", "Index", "Home", new { num = 0 }, null)

    @for (var i = 1; i < ViewBag.pages + 1; i++)
    {
        var cur = i.ToString();
        var name = i + 1;

@Html.ActionLink(name.ToString(), "Index", "Home", new { num = cur }, null)

        if (i >= 10) { break; }

    }

Note: I know there's going to be a bit more logic required in order to account for the remainder of pages after they are divided. But that's not my biggest concern as of yet.

int num

What does num represent exactly? Number of pages? Number of posts per page? Current page number? Properly naming the variable would help a lot in clarity here.

var postsPerPage = 3;

Turn it into a parameter with default value 3. That way you can always decide later that the user can change this without much trouble.

Instead of using 0-based pages and then adding 1 to the presentation, I would consider using the reverse: 1-based pages and subtracting 1 when determining the pages to display. In essence this would turn your loop from i = 2, name becomes unnecessary and in your controller you use int skip = (num - 1) * postsPerPage instead.

using (UnitOfWork uwork = new UnitOfWork())
{
    IEnumerable<Post> posts = uwork.PostRepository.GetAll().ToList();

    int totalPosts = posts.Count();
    ViewBag.pages = totalPosts / postsPerPage;
    int skip = num * postsPerPage;

    return View(posts.Skip(skip).Take(postsPerPage));
}

ViewBag.pages (note the incorrect naming convention) indicates to me a collection of pages. In your case however, a more appropriate name would be AmountOfPages.

I feel like there's room for an optimization here: what if you have 100.000 pages? Or even just 100? You will load all your items each time someone visits the frontpage and you're only doing this to show the user how many pages of items there are. In fact you're not even doing that: you won't show them past 10 pages anyway!

I would argue that maybe you should just use two queries:

var totalPosts = uwork.PostRepository.GetAll().Count()

which will just perform a select count(*) from posts at the database side and another query that gives you your pages:

var pages = uwork.PostRepository.GetAll().Skip(skip).Take(postsPerPage)

Another way to keep performance in mind is to use

uwork.PostRepository.GetAll().Take(postsPerPage * 10).ToArray();

This will you give

The amount of posts (the exact amount being pages.Length)
The concrete posts (being pages.Skip(skip).Take(postsPerPage))

The benefit of this is that it will only call the database once (network latency is one of your demons) with the downside being that it retrieves more items than you're interested in.

Instead of looping to ViewBag.pages + 1 and possibly breaking out at i >= 10, I would just loop until Math.Min(ViewBag.Pages + 1, 10).


Displaying a conflict marker I'm displaying a conflict marker for each actor for each day, if the current day is in their conflicts:

  <template v-for="actor in cast">
    <template v-for="conflict in actor.conflicts">

    </template>
  </template>

This works, but it just feels like a lot of code (despite it already being a dozen or so lines shorter than how I was doing it before using Vue JS).

Here's my Vue.js:

new Vue({
  el: '#schedule-builder',

  data: {

    cast: [
      {{ cast_list }}
          {
            actor: {
              id: "{{ actor_id }}",
              name: "{{ actor_name or 'TBD' }}",
            },
            conflicts: [
              {{ conflict_calendar }}
                { datestamp: "{{ value }}" },
              {{ /conflict_calendar }}
            ],
          },
      {{ /cast_list }}
    ],

    next_three_months: [
      {
        datestamp: '{{ now | format:Y-m-d }}',
      },
      {{ loop times="90" }}
        {
          datestamp: '{{ now | modify_date:+1 day | format:Y-m-d }}',
        },
      {{ /loop }}
    ],

This post has been around for too long and I don't want it to be a zombie any longer... You haven't replied to our questions in comments but we see you have been on SE sites lately so I am going to go ahead with the code as is.

As was pointed out in comments there appears to be some type of rendering (perhaps server-side) that is mixed in with the JavaScript code, which is not declared, so I wouldn't really be able to try running the code even if I wanted to :/ I could try to dig through your StackOverflow posts to find relevant code but I don't have time for that.

For the code you posted, it doesn't really seem like "a lot" but that depends on perspective... if you really wanted, you could abstract the code in the v-if (i.e. v-if="date.datestamp==conflict.datestamp") to a method and call it instead of having the logic in the markup.

I question whether <template> is the best tag to use in the for loops...While there are no restrictions on permitted content within a <template> tag, it seems odd to have one nested inside the other. Would a simple <div> suffice for one or both?


Stack which finds the minimum element Write a program to implement stack which implements min a method
  which gives the minimum element below the top element in the stack.

const items = Symbol('Stack items');

class Stack {
  constructor() {
    this[items] = [];
  }

  pop() {
    return this[items].pop();
  }

  push(data) {
    this[items].push(data);
  }

  size() {
    return this[items].length;
  }

  isEmpty() {
    return this.size() === 0;
  }

  peek() {
    return this[items][this.size() - 1];
  }
}

class MinStack {
  constructor(iterable) {
    this.stack = new Stack();
    this.minStack = new Stack();
    if (iterable) {
      for (let val of iterable) {
        this.push(val);
      }
    }
  }

  min() {
    if (this.stack.isEmpty()) {
      throw new Error('Min of empty stack');
    }
    return this.minStack.peek();
  }

  pop() {
    if (this.stack.isEmpty()) {
      throw new Error('Popping from empty stack');
    }
    let val = this.stack.pop();
    if (val === this.minStack.peek()) {
      this.minStack.pop();
    }
    return val;
  }

  push(data) {
    if (this.minStack.isEmpty() && this.stack.isEmpty()) {
      this.minStack.push(data);
    } else if (data <= this.minStack.peek()) {
      this.minStack.push(data);
    }
    this.stack.push(data);
  }

  size() {
    return this.stack.size();
  }

  isEmpty() {
    return this.stack.isEmpty();
  }

  peek() {
    return this.stack.peek();
  }
}

const s = new MinStack();

console.log('//  Empty Stack  //');
try {
  console.log(s.min()); // Err
} catch (err) {
  console.log('Err: ' + err.message);
}

console.log('// Non-Empty Stack  //');
s.push(1);
s.push(2);
s.push(3);

console.log(s.pop() === 3);
console.log(s.min() === 1);
console.log(s.pop() === 2);
console.log(s.min() === 1);
console.log(s.pop() === 1);
try {
  console.log(s.min()); // Err
} catch (err) {
  console.log('Err: ' + err.message); // Err
}

console.log('// Using iterator  //');

const s2 = new MinStack([3, 2, 5, 1]);
console.log(s2.min() === 1);
console.log(s2.pop() === 1);
console.log(s2.min() === 2);
console.log(s2.pop() === 5);
console.log(s2.min() === 2);
console.log(s2.pop() === 2);
console.log(s2.min() === 3);
console.log(s2.pop() === 3);

/*
Space complexity: O(2n) ~ O(n)

Time complexities:
1. Construction
   Iterator: O(n) else O(1)
2. Push
   O(1) for both stacks
3. Pop
   O(1) for both stacks
4. Min
   O(1)
5. Size
   O(1)
6. isEmpty
   O(1)
*/

Nicely written. The space saving trick you did with min stack is especially nice.

When you push, the conditions can be a bit simpler:

push(data) {
  if (this.minStack.isEmpty() && this.stack.isEmpty()) {
    this.minStack.push(data);
  } else if (data <= this.minStack.peek()) {
    this.minStack.push(data);
  }
  this.stack.push(data);
}

The min stack will only be empty when the stack is empty, so you can drop the second condition in the first if statement. And since the else if branch does the same thing as the if branch, you can combine them:

push(data) {
  if (this.minStack.isEmpty() || data <= this.minStack.peek()) {
    this.minStack.push(data);
  }
  this.stack.push(data);
}


Adding arbitrarily long integers C++ (string represented) I'm working through some Project Euler problems using C++ and some of the numbers in question are starting to get quite large (21000 for Problem 16) and I need to add some of these large numbers together, so I quickly threw together a string represented (positive) integer adder:

#include <vector>
#include <string>

std::string AddStringNumbers(std::string n1, std::string n2) {
    while (n1.length() < n2.length()) {
        n1 = "0" + n1;
    }
    while (n2.length() < n1.length()) {
        n2 = "0" + n2;
    }

    std::vector<int> sums = std::vector<int>(n1.length(), 0);
    for (int i = 0; i < n1.length(); i++) {
        sums[i] = (n1[i] - '0') + (n2[i] - '0');
    }

    for (int i = sums.size() - 1; i > 0; i--) {
        while (sums[i] > 9) {
            sums[i] -= 10;
            sums[i - 1] += 1;
        }
    }
    if (sums[0] > 9) {
        sums.insert(sums.begin(), 0);
        while (sums[1] > 9) {
            sums[1] -= 10;
            sums[0] += 1;
        }
    }

    std::string o = "";
    for (int i = 0; i < sums.size(); i++) {
        o += (char)('0' + sums[i]);
    }
    return o;
}

Once this was implemented, the solution was simple:

int Problem016(int n) {
    std::string num = "1";
    for (int64_t i = 0; i < n; i++) {
        num = AddStringNumbers(num, num);
    }

    int sum = 0;
    for (int i = 0; i < num.length(); i++) {
        sum += num[i] - '0';
    }
    return sum;
}

Called with:

Problem016(1000);

Is there anything that I can do to improve my AddStringNumbers function? I'm especially interested in the use of storage types other than std::string and what the advantages of doing so would be. It should also be noted that I do have an intention of doing something like this in a class however, as this was quickly thrown together for the solution that's parked for the time being.

I'm less interested in my method of solving Problem 16, however any comments on that would also be appreciated.

Prepending zeros one at a time is quite inefficient.  It could be improved by making a string of the appropriate length in one go:

{
    auto l1 = n1.length();
    auto l2 = n2.length();
    if (l1 < l2)
        n1 = std::string{"0", l2-l1} + n1;
    else if (l1 > l2)
        n2 = std::string{"0",  l1-l2} + n2;
}

Reassembling the string afterwards character by character is also inefficient.  This can be improved by telling the string how long it will need to be:

std::string o = "";
o.reserve(sums.size());

The index variables i should be an unsigned type to compare against string length or vector size - I recommend using the same type as those methods return.

Greater gains can be made by keeping the long integer as a chain of digits without adding '0' to make them printable characters at each stage; it also helps to store them in little-endian order (so that any carry gets appended rather than prepended).  See Digit-by-digit addition using vectors for a start.  You could reduce storage requirements by using base-100 instead of base-10 (char is guaranteed to hold ranging at least from 0 to 127).


Implementing a shared array cache without locking I am trying to implement a shared cache for arrays.  It must support two operations: set(owner, idx, value) and fetch(owner, idx) where idx is the index into the array and owner is an opaque handle to an owning object -- fetch(owner_1, idx) should return the value stored by set(owner_1, idx) only if the owner argument matches.  The cache must be thread - safe but I do not want to rely on locking, i.e. mutexes.  Failure in looking up cached values is fine - it is OK and expected that other threads will overwrite existing values in the shared cache, in which case fetch should just fail.

So the fetch operation has to read the cache slot's owner field to check against its argument, and if it matches return the cache slot's value field.  The problem is, without locks, another thread could overwrite one of these fields during that operation.  This approach tries to get around that by assigning a version field to each cache slot.  It only increases.  The fetch operation reads the version (atomically) at the start of the operation and at the end; if these are not the same, something changed during the read and the result is invalid, even if the owner field apparently matched.

The code below ensures that version is always incremented before value is updated, thus preventing fetch from returning a value from a different owner.  (The functions g_atomic_... are provided by glib.) It "seems to work" - but can it be proven correct or incorrect?

struct _cache_slot
{
  void*        owner;
  gint         version;
  gdouble      value;
};

struct _cache_slot cache[SIZE];

int
point_cache_fetch(void *owner, gdouble* ret, gsize idx)
{
   struct _cache_slot *slot = &cache[idx];

   gint version_start  = g_atomic_int_get(&(slot->version));
   void* slot_owner    = g_atomic_pointer_get(&(slot->owner));
   gdouble value       = slot->value;
   gint version_finish = g_atomic_int_get(&(slot->version));

   if ((version_start == version_finish) && (slot_owner == owner))
   {
     *ret = value;
     return 1;
   }
   else
   {
     return 0;
   }
}

void
point_cache_store(void *owner, gsize idx, gdouble value)
{
   struct _cache_slot *slot = &cache[idx];

   g_atomic_pointer_set(&(slot->owner), NULL);
   slot->version++;
   g_atomic_pointer_set(&(slot->owner), owner);
   slot->value = value;
}

It's not clear from you're post whether owner is unique for each thread or if two threads can call the functions with the same value of owner. If two threads can call the functions with the same value for owner, your code is incorrect.

If one thread is executing point_catch_store() and another thread calls point_catch_fetch() while the first thread is between these two lines, you'll get the old value (from the previous owner of the entry), not the new one:

g_atomic_pointer_set(&(slot->owner), owner);
slot->value = value;

As far as I can tell, swapping those two lines would solve that problem.


Submit form data to server side I have this method here called SubmitProc which submits form data that the user filled out to the server side and does a check if the response to the server goes through. If it's fine, great. If not, return the status error from the server. The data actually gets sent to SharePoint (not sure if it's politically correct to say it goes to server side if it goes to SP) but that's the idea.

If an exceptions results, then check if the files upload match the current procedure file count. If yes, do nothing, otherwise there is an error.

public async Task<string> SubmitProc(PFormData formData)
{
    Proj.Repositories.Models.External.Proc proc = formData.ProcForm;
    try
    {
        string url = Config.BaseAPIUrl;
        url = url + "AddPProc";

        string authToken = await Authentication.GetAuthToken();

        _client.DefaultRequestHeaders.Accept.Add (new System.Net.Http.Headers.MediaTypeWithQualityHeaderValue ("application/json"));
        _clientHandler.CookieContainer.Add (new Uri (url), new System.Net.Cookie ("FedAuth", authToken));

        Proj.Repositories.Models.External.ProcResponse serviceResponse = await submitForm (url, proc).ConfigureAwait (false);

        if (serviceResponse.StatusCode == 200)
            return null;
        else
            return serviceResponse.StatusDescription;
    }
    catch (WebException ex)
    {

    }
    catch (Exception ex)
    {
    }

    try
    {
        if (proc.File != null)
        {
            int filesCount = await getUploadFilesCount ("Foo", proc.UniqueId);
            if (proc.File.Count == filesCount)
                return null;
            else
                return "Encountered error uploading attachments.";
        }
    }
    catch (Exception ex)
    {

    }

    return "Encountered unknown error.";
}

Where getUploadFilesCount is:

private async Task<int> getUploadFilesCount(string type, string guidString)
{
    try
    {
        string url = Config.BaseAPIUrl;
        url = url + "/VerifyUpload/" + type + "/" + guidString;// + "/" + {*procDate};

        string authToken = await Authentication.GetAuthToken ();

        _client.DefaultRequestHeaders.Accept.Add (new System.Net.Http.Headers.MediaTypeWithQualityHeaderValue ("application/json"));

        _clientHandler.CookieContainer.Add (new Uri (url), new System.Net.Cookie ("FedAuth", authToken));

        HttpRequestMessage request = new HttpRequestMessage (HttpMethod.Get, url);
        HttpResponseMessage response = await _client.SendAsync (request);

        Proj.Repositories.Models.External.ProcResponse procResponse = JsonConvert.DeserializeObject<Proj.Repositories.Models.External.ProcResponse> (await response.Content.ReadAsStringAsync ());

        if(procResponse != null)
            return procResponse.FilesCount;

        return 0;
    }
    catch(Exception)
    {

    }

    return 0;
}

It's been working so far but the code above looks terrible. I'm maintaining this project and I just looked at this code and it definitely looks like it needs to be refactored but I'm afraid of breaking it. Any recommendations or advice on condensing this code or refactor it?

This code is currently inside a Xamarin.iOS App.

I don't see any rough mistake with your code, but I would refactor some parts of it.

There are a lot of magic string constants like "AddPProc", "VerifyUpload", "FedAuth". They should be defined as named constants:

private const string AddPProc = "AddPProc";

This code

string url = Config.BaseAPIUrl;
url = url + "AddPProc";

can be replaced with

var url = Config.BaseAPIUrl + AddPProc;

The same thing with the second method.

In both methods there are fully qualified types. It is hard to read this code. For example, instead of System.Net.Http.Headers.MediaTypeWithQualityHeaderValue place using System.Net.Http.Headers; at the start of the file and use MediaTypeWithQualityHeaderValue without qualification. Especially it is recommended for this line:

Proj.Repositories.Models.External.ProcResponse procResponse = JsonConvert.DeserializeObject<Proj.Repositories.Models.External.ProcResponse> (await response.Content.ReadAsStringAsync ());

In this code

if (serviceResponse.StatusCode == 200)
    return null;
else
    return serviceResponse.StatusDescription;

you don't need else keyword. You can even replace this if-else with ternary operator and single return:

return serviceResponse.StatusCode == 200
    ? null
    : serviceResponse.StatusDescription;

getUploadFilesCount is bad name according to C# naming guidelines. Method's name should start with uppercase letter.

This code

if (procResponse != null)
    return procResponse.FilesCount;

return 0;

can be replaced with

return procResponse?.FilesCount ?? 0;

Also it is not good to return string to show that something went wrong. Let exceptions be thrown, don't swallow them. Code that calls these methods should take care about exceptions.

And I don't understand why a space is placed before each ( in methods calls.


Looping through file finding sequences For an assignment, I had to loop through a text file, and report any positions that a user entered sequence was found. Is there any way I can make this code more efficient, because it seems like I'm writing a lot of nested loops, which I've learned can be dangerous/complicated. Any and all help is appreciated!

import os #OperatingSystem
import sys #System
import re #Regex (Regular Expressions)

def checkSequenceCharacters(seq):
    for c in seq:
        if c != 'A' and c != 'G' and c != 'T' and c != 'C':
            print("[!] ERROR: Not a valid sequence (A/G/T/C ONLY)")
            sys.exit(0)

def checkSequenceLength(seq):
    if len(seq) != 5:
        print("[!] ERROR: Has to be exactally 5 letters long!")
        sys.exit(0)

seq = raw_input("Enter a 5 basepair sequence using only A,G,T,C: \n")

checkSequenceCharacters(seq)
checkSequenceLength(seq)

input_file_path = os.getenv("HOME") + "/Desktop/sequencer/inputfile.txt"
output_file_path = os.getenv("HOME") + "/Desktop/sequencer/output/" + seq + ".txt"

try:
    input_file = open(input_file_path)
    output_file = open(output_file_path, 'w')
    count = 0
    line_location = 0
    lines = []
    for line in input_file:
        line_location = line_location + 1
        arr = re.findall(seq,line)
        if arr:
            x = arr[0]
            if x == seq:
                count = count + 1
                lines.append(line)
                print("[+] Sequence found at line " + str(line_location))
                output_file.write("Line " + str(line_location) + ": " + line + "\n")
    if count != 0:
        print("[*] Matches: " + str(count))
        print("[*] FILE CREATED AT: " + output_file_path)
    else:
        print("[!] ERROR: No matches found!")
finally:
    input_file.close()
    output_file.close()

TL;DR: See end of answer for updated code with suggested improvements

While I don't have many suggestions regarding nested for-loops, as you only seem to have two of them, I do have a few other suggestions:

Suggestions

1. Use with for file opening

You use try/finally to open/close your files, and while you're absolutely right to close your files, Python's with statement takes care of this for you. Another lesser-known characteristic of the with statement is that you can "nest" them in a single line, which is useful in your case.

So, I would recommend updating

try:
    input_file = open(input_file_path)
    output_file = open(output_file_path, 'w')
    ...  # Do stuff
finally:
    input_file.close()
    output_file.close()

to

with open(input_file_path) as input_file, open(output_file_path, "w") as output_file:
    ...  # Do stuff

When the context of the with statement is exited, your opened files will automatically be closed.

2. Clean up variable naming

Python convention is to use lower snake-case for normal variable names and function names - Camel-case (upper) is usually only used for class definitions. Therefore, I would recommend renaming your checkSequenceCharacters and checkSequenceLength functions to check_sequence_characters and check_sequence_length, respectively. This might be considered nitpicking, but maintaining consistent naming conventions always makes things easier for me.

3. Use str in str, instead of checking individual equality

In check_sequence_characters, you can more easily ensure that each character in seq is in an allowed set of characters using the following:

if any(c not in "AGTC" for c in seq):
    raise ValueError("Not a valid sequence (A/G/T/C ONLY)")

4. Raise Exceptions, instead of printing "ERROR"

There are a few places in your example where you print error messages. That's fine if you really don't want to raise actual exceptions, but that does seem to be the behavior your code is dancing around. I would recommend reading Python's documentation on raise if you're unfamiliar with it.

5. Use if __name__ == "__main__" when running module as source

If you don't know what if __name__ == "__main__" means, check out this explanation. Basically, my recommendation is to start using this pattern even if it may not be strictly necessary in your example (although it does protect you from the lurking seq name-shadowing bug). This will facilitate maintenance of your code should you need to import your example code as a module in the future, for example.

6. Use enumerate to loop through input_file with line index

Instead of using the line_location variable, you can remove the line_location = 0 line, and change

for line in input_file:
    line_location = line_location + 1

to

for line_location, line in enumerate(input_file):

If you still want line_location to be 1-indexed, just add 1 when you print/write it below.

Other Notes

For more recent versions of Python, you'll need to use input instead of raw_input
If there is any chance your output file has not already been created by you (which seems likely), you should open it using mode="x", which will create and open the file
If any of the subdirectories in the output file path have not already been created, you'll need to use something like os.makedirs to handle directory creation
The line print("[*] FILE CREATED AT: " + output_file_path) near the bottom may be misleading, as the output file is created when you call open, not when you call write. This means that even if count == 0, an output file was created (albeit a blank one)
In the provided suggested code, I removed the lines variable, since it isn't used; however, if you actually do need it for something not shown in your example, it can be safely re-added, along with the lines.append(line) line
I would recommend embracing Python's string format method, instead of using + to construct strings

TL;DR

If you decide to follow the above suggestions, your code will probably end up looking something like this:

import os
import re

def check_sequence_characters(seq):
    if any(c not in "AGTC" for c in seq):
        raise ValueError("Not a valid sequence (A/G/T/C ONLY)")

def check_sequence_length(seq):
    if len(seq) != 5:
        raise ValueError("Has to be exactly 5 letters long!")

def _execute():
    seq = raw_input("Enter a 5 basepair sequence using only A,G,T,C: \n")
    count = 0
    check_sequence_characters(seq)
    check_sequence_length(seq)
    input_file_path = os.getenv("HOME") + "/Desktop/sequencer/inputfile.txt"
    output_file_path = os.getenv("HOME") + "/Desktop/sequencer/output/" + seq + ".txt"

    with open(input_file_path) as input_file, open(output_file_path, 'x+') as output_file:
        for line_location, line in enumerate(input_file):
            arr = re.findall(seq, line)

            if arr and arr[0] == seq:
                count += 1
                print("[+] Sequence found at line " + str(line_location + 1))
                output_file.write("Line " + str(line_location + 1) + ": " + line + "\n")

        if count != 0:
            print("[*] Matches: " + str(count))
            print("[*] FILE CREATED AT: " + output_file_path)
        else:
            raise RuntimeError("No matches found!")

if __name__ == "__main__":
    _execute()


Going back with a Vengeance So I received a comment on my previous question that suggested creating a Dictionary of my menus to store the data in.  So that got me thinking, all I am doing is going back to the first instance of MenuItem in the ItemList that has a Menu of Back[0].Menu.  So why not just iterate through there and open the menu when I find it?

So, here is my final code:

public void GoBack()
{
    NavButtonUsed = true;

    Forward.Insert(0, Back[0]);
    Back.RemoveAt(0);

    if (Back[0].Title.StartsWith(" ") && CurrentItem.Menu != Back[0].Menu)
    {
        foreach(MenuItem mi in ItemList)
        {
            if (mi.Menu == Back[0].Menu)
            {
                CurrentItem = ItemList[ItemList.IndexOf(mi)];
                break;
            }
        }
    }

    CurrentItem = ItemList[ItemList.IndexOf(Back[0])];
    NavButtonUsed = false;
}

Everything else stayed the same as in my previous question, please check that for details.

CurrentItem = ItemList[ItemList.IndexOf(mi)];

can just be

CurrentItem = mi;

You could also use FirstOrDefault:

var item = ItemList.FirstOrDefault(i => i.Menu == Back[0].Menu);
if (item != null)
{
    CurrentItem = item;
}


RecyclerView with different ViewHolder using generic I have different ViewHolder for different Feed implementations. But I can't use the best practice of having each ViewHolder corresponds to different layout id because i am reusing layout for 2 different ViewHolder (aka. FeedWithImageHolder and FeedWithoutImageHolder)

class FeedAdapter extends RecyclerView.Adapter<FeedAdapter.BaseFeedHolder>{

    @NonNull
    @Override
    public BaseFeedHolder onCreateViewHolder(@NonNull ViewGroup parent, int viewType) {
        View v = null;
        switch (viewType){
            case FeedType.FEED_WITH_IMAGE :
                v = LayoutInflater.from(context).inflate(R.layout.feed_status,parent,false);
                //notice that i use the same R.layout.feed_status for two different ViewHolder

                return new FeedWithImageHolder(v);
            case FeedType.FEED_WITHOUT_IMAGE:
                v = LayoutInflater.from(context).inflate(R.layout.feed_status,parent,false);
                return new FeedWithoutImageHolder(v);
            case FeedType.ADS:
                v = LayoutInflater.from(context).inflate(R.layout.ads,parent,false);
                return new AdsHolder(v);
        }
        return null;
    }

    @Override
    public void onBindViewHolder(@NonNull BaseFeedHolder holder, int position) {
        //noinspection unchecked
        holder.bindFeed(feeds.get(position));
    }

    abstract class BaseFeedHolder<T extends Feed> extends RecyclerView.ViewHolder {
        BaseFeedHolder(@NonNull View itemView) {
            super(itemView);
        }

        abstract void bindFeed(T feed);
    }

    class FeedWithImageHolder extends BaseFeedHolder<FeedStatus>{

        FeedWithImageHolder(@NonNull View itemView) {
            super(itemView);
        }

        @Override
        void bindFeed(FeedStatus feed) {
            doSomethingWithFeed(feed);
            bindImage(feed.getPostImage());
        }

        protected void bindImage(String image){
            clipImageToImageView(image);
        }
    }

    class FeedWithoutImageHolder extends FeedWithImageHolder{

        FeedWithoutImageHolder(@NonNull View itemView) {
            super(itemView);
            removeImageViewFromCoinstraintLayout(itemView);
        }

        @Override
        protected void bindImage(String image) {
            //void
        }
    }

    class AdsHolder extends BaseFeedHolder<Ads>{
        AdsHolder(@NonNull View itemView) {
            super(itemView);
        }

        @Override
        void bindFeed(Ads ads) {
        }
    }

}

Instead what I do is making Feed have a method .getType() so every implementation can have different type corresponding to it.

For example:

class FeedStatus implements Feed {

    private String image;

    @Override
    public int getType() {
        return (this.image == null) ? FeedType.FEED_WITHOUT_IMAGE : FeedType.FEED_WITH_IMAGE;
    }

}

There is tingling worry here because I think there is code smell in there but I am not sure which one and I hope you can help me with it. Thanks

From a quick glance, I share your concerns.

One 'smell' is that a sub-class (FeedWithoutImageHolder) provides less functionality than the parent class, i.e. it does not actually "extend" the class.

I suggest trying to align layout structure and functionality:

Create two different layouts: The first one without an image, and a second one which includes the first and extends it by adding an image. Depending on your desired layout design, you may instead want to declare only one layout which includes some empty placeholder element of the desired image display size which either stays empty or receives an image, so that all visual elements align.

I recommend against having a Feed handle the type of layout at all. Keep data objects and layout concerns separate, that's what the Adapter is for. Have the adapter check if an image needs to be displayed for an item and determine the appropriate view type in Adapter.getItemViewType().


Initialize various commands for the SqlDataAdapter This is my code for my SqlDataAdapter which is similar to the code example given in the docs:

public static int CommitBirdData(DataSet pDataSet)
{
    int rowsAffected = 0;

    using (SqlConnection conn = new SqlConnection(connString))
    using (SqlDataAdapter birdDataAdapter = new SqlDataAdapter())
    {
        SqlCommand cmd = new SqlCommand(
            "INSERT INTO [dbo].[Bird] ([BirdID], [Name], [Description]) " +
            "VALUES (@BirdID, @Name, @Description)"
            , conn);

        cmd.Parameters.Add("@BirdID", SqlDbType.NVarChar, 10, "BirdID");
        cmd.Parameters.Add("@Name", SqlDbType.NVarChar, 50, "Name");
        cmd.Parameters.Add("@Description", SqlDbType.NVarChar, 200, "Description");

        birdDataAdapter.InsertCommand = cmd;

        cmd = new SqlCommand(
            "UPDATE [dbo].[Bird] " +
            "SET [BirdID] = @BirdID, [Name] = @Name, [Description] = @Description " +
            "WHERE [BirdID] = @BirdID"
            , conn);

        cmd.Parameters.Add("@BirdID", SqlDbType.NVarChar, 10, "BirdID");
        cmd.Parameters.Add("@Name", SqlDbType.NVarChar, 50, "Name");
        cmd.Parameters.Add("@Description", SqlDbType.NVarChar, 200, "Description");

        birdDataAdapter.UpdateCommand = cmd;

        cmd = new SqlCommand(
            "DELETE FROM [dbo].[BirdCount] WHERE [BirdID] = @BirdID " +
            "DELETE FROM[dbo].[Bird] WHERE[BirdID] = @BirdID"
            , conn);

        cmd.Parameters.Add("@BirdID", SqlDbType.NVarChar, 10, "BirdID");

        birdDataAdapter.DeleteCommand = cmd;

        rowsAffected = birdDataAdapter.Update(pDataSet, "Bird");

        return rowsAffected;

    }

I noticed that this code is repetitive and I was just wondering whether there is a more efficient way to initialize the commands of the SqlDataAdapter with or without the using statements.

With the following, you'll only add each parameter a single time and just change the command text before assigning it to the adapter command, allowing you to reuse the existing cmd object.

using (SqlDataAdapter birdDataAdapter = new SqlDataAdapter())
{
    SqlCommand cmd = new SqlCommand("DELETE FROM [dbo].[BirdCount] WHERE [BirdID] = @BirdID; DELETE FROM[dbo].[Bird] WHERE[BirdID] = @BirdID",conn);
    cmd.Parameters.Add("@BirdID", SqlDbType.NVarChar, 10, "BirdID");

    birdDataAdapter.DeleteCommand = cmd;

    cmd.Parameters.Add("@Name", SqlDbType.NVarChar, 50, "Name");
    cmd.Parameters.Add("@Description", SqlDbType.NVarChar, 200, "Description");

    cmd.CommandText="INSERT INTO [dbo].[Bird] ([BirdID], [Name], [Description]) VALUES (@BirdID, @Name, @Description)";
    birdDataAdapter.InsertCommand = cmd;

    cmd.CommandText= "UPDATE [dbo].[Bird] SET [BirdID] = @BirdID, [Name] = @Name, [Description] = @Description WHERE [BirdID] = @BirdID";
    birdDataAdapter.UpdateCommand = cmd;

    rowsAffected = birdDataAdapter.Update(pDataSet, "Bird");

    return rowsAffected;
}


Improve efficiency for finding word frequency in text Premise: Write a program that counts the frequencies of each word in a text, and
output each word with its count and line numbers where it appears. We
define a word as a contiguous sequence of non-white-space characters.
Different capitalizations of the same character sequence should be
considered same word (e.g. Python and python). The output is formatted as
follows: each line begins with a number indicating the frequency of the word,
a white space, then the word itself, and a list of line numbers containing this
word. You should output from the most frequent word to the least frequent.
In case two words have the same frequency, the lexicographically smaller
one comes first. All words are in lower case in the output.

Solution:

# Amazing Python libraries that prevent re-invention of wheels.
import re
import collections
import string
from operator import itemgetter

# Simulate work for terminal
print 'Scanning input for word frequency...'

# Text files
input = open('input.txt', 'r')
output = open('output.txt', 'w')

# Reads every word in input as lowercase while ignoring punctuation and
# returns a list containing 2-tuples of (word, frequency).
list = collections.Counter(input.read().lower()
       .translate(None, string.punctuation).split()).most_common()
# Sorts the list by frequency in descending order.
list = sorted(list, key=itemgetter(0))
# Sorts the list by lexicogrpahical order in descending order while maintaining
# previous sorting.
list = sorted(list, key=itemgetter(1), reverse=True)

# Gets the lines where each word in list appears in the input.
for word in list:
  lineList = []
  # Reads the input line by line. Stores each text line in 'line' and keeps a
  # counter in 'lineNum' that starts at 1.
  for lineNum, line in enumerate(open('input.txt'),1):
    # If the word is in the current line then add it to the list of lines
    # in which the current word appears in. The word frequency list is not
    # mutated. This is stored in a separate list.
    if re.search(r'(^|\s)'+word[0]+'\s',
                 line.lower().translate(None, string.punctuation),
                 flags=re.IGNORECASE):
      lineList.append(lineNum)
  # Write output with proper format.
  output.write(str(word[1]) + ' ' + word[0] + ' ' + str(lineList)+'\n')

# End work simulation for terminal
print 'Done! Output in: output.txt'

How can I get the line numbers without re-reading the text? I want to achieve sub-O(n^2) time complexity. I am new to Python so feedback on other aspects is greatly appreciated as well.

You're right, the basic point to make this program faster is to store the line numbers in the first pass.

The for word in list loop will run many times. The count depends on how many words you have. Every time it runs, it reopens and rereads the whole input file. This is awfully slow.

If you start using a more generic data structure than your Counter(), then you'll be able to store the line numbers. For example you could use a dictionary. The key of the dict is the word, the value is a list of line numbers. The hit count is stored indirectly as the length of the list of line numbers.

In one pass over the input you can populate this data structure. In a second step, you can properly sort this data structure and iterate over the elements.

An example implementation:

from collections import defaultdict
import sys

if __name__ == "__main__":
    hit = defaultdict(list)
    for lineno, line in enumerate(sys.stdin, start=1):
        for word in line.split():
            hit[word.lower()].append(lineno)
    for word, places in sorted(hit.iteritems(), key=lambda (w, p): (-len(p), w)):
        print len(places), word, ",".join(map(str, sorted(set(places))))

A few more notes:

If you open() a file, close() it too. As soon as you're done with using it. (Or use the with statement to ensure that it gets closed.)
Don't hardcode input/output file names. Read from standard input (sys.stdin), write to standard output (sys.stdout or simply print), and let the user use his shell's redirections: program <input.txt >output.txt
Don't litter standard output with progress messages. If you absolutely need them use sys.stderr.write or get acquainted with the logging module.
Don't use regular expressions, unless it's necessary. Prefer string methods for their simplicity and speed.
Name your variables in lower_case_with_underscores as PEP8 recommends.


Interface for unit of work pattern and repository pattern I'm trying to design a well defined yet simple interface for the unit of work and repository patterns. My UoW's are exposed to services and services then "get repositories" that it needs to query. I know returning IQueryable<T> for repositories is a religious war. Because repositories are only exposed to the service, all queries are performed inside the service and therefore I can test the queries. Is there anything I should change for these interfaces? All criticisms are greatly appreciated!

public interface IUnitOfWork : IDisposable
{
    bool IsActive { get; }

    bool WasCommitted { get; }

    /// <summary>
    /// Commits all changes made on the unit of work.
    /// </summary>
    void Commit();

    bool WasRolledBack { get; }

    /// <summary>
    /// Rolls back all changes made on the unit of work.
    /// </summary>
    void Rollback();

    /// <summary>
    /// Returns an instance of an entity with the specified key that is attached to the unit of work without
    /// loading the entity from a repository.
    /// </summary>
    /// <param name="id"></param>
    /// <returns></returns>
    T Load<T>(int id)
        where T : class;

    void Attach<T>(T entity)
        where T : class, IIdentifiable;

    void Detach<T>(T entity)
        where T : class;

    IRepository<T> GetRepository<T>()
        where T : class;
}

public interface IRepository<T>
    where T : class
{

    IUnitOfWork UnitOfWork { get; }

    void Add(T entity);

    void Remove(T entity);

    /// <summary>
    /// Returns an instance of an entity with the specified key that is attached to the unit of work by loading
    /// the entity from the repository.
    /// </summary>
    /// <param name="id"></param>
    /// <returns></returns>
    T Get(int id);

    IQueryable<T> All();
}

I think your interfaces are mostly well-defined, with a couple exceptions:

I don't think Load, Attach and Detach should be members of IUnitOfWork, but rather IRepository.  It seems that the repository that manages objects of type T would be the best place to place methods that acts on that type.

The UnitOfWork property on IRepository does not belong there. Things that modify one repository shouldn't be allowed to go and get other repositories to modify on their own.  If they need them, pass them in explicitly.  Otherwise, you're hiding from potential callers the fact that your implementation actually depends on more than just the one repository, and hiding your dependencies like you're ashamed of them is one of my least favorite code smells.


Counting primes in a range whose digits are all prime Codewars Kata: Given integers a and b, count all primes p where a ≤ p < b and the digits of p are all prime. b may be as large as 107.

To start with, I hardcoded all valid 1903 primes in \$[1, 10^7]\$ that can be formed from digits \$\{2, 3, 5, 7\}\$ and stored it in a list. Then I use binary search to locate the indexes corresponding to the given bounds, then return the difference of these indexes + 1, and my code times out. That is frankly baffling to me. Like I can't even coherently theorise why the program would be slow. All the heavy duty computation is handled by the binary search which is logarithmic time, and is called only a few times and on a 1903 element list (Also, the binary search function completes 1000 searches of 200,000 element lists in 228.67s in another kata, so I don't think I implemented it poorly).

My Code

def xbinsearch(pred, lst, *extras, type = "min", default = None):
    low, hi, best = 0, len(lst)-1, default
    while low <= hi:
        mid = (low+hi)//2
        p = pred(mid, lst, *extras)
        if p[0]:    #The current element satisfies the given predicate.
            if type == "min":
                if best == default or lst[mid] < lst[best]: best = mid
                hi = mid-1
            elif type == "max":
                if best == default or lst[mid] > lst[best]: best = mid
                low = mid+1
        elif p[1] == 1:     #For all `x` > `lst[mid]` not `P(x)`.
            hi = mid - 1
        elif p[1] == -1:    #For all `x` < `lst[mid]` not `P(x)`.
            low = mid + 1
    return best

def pred_a(idx, lst, *extras):
    tpl, val = [None, None], lst[idx],
    if extras:  a, b = extras[0], extras[1]
    tpl[0] = a <= val < b
    if val > b: tpl[1] = 1
    elif val < a:   tpl[1] = -1
    return tuple(tpl)

def get_total_primes(a, b):
    low, hi = xbinsearch(pred_a, primes, a, b), xbinsearch(pred_a, primes, a, b, type="max")
    return hi + 1 - low

Please read PEP8 and apply its advice; this will make your code look like Python code to others. Mainly, avoid code blocks on the same line as their conditions, and avoid cramming too many assignments on the same line.

You can also make your predicate function return a single value, as the first element of the returned tuple can be computed from the second (mainly p[0] is p[1] is None). You could also use the more common values -1, 0 and 1 and add an else clause in your xbinsearch loop to raise an exception. This would have caught the case where val == b in your usage.

Lastly, bisect should be the module to reach for when dealing with binary search in Python. In fact, having your list of primes ready, the code should be:

def get_total_primes(a, b):
    low = bisect.bisect_left(primes, a)
    high = bisect.bisect_left(primes, b)
    return high - low

And if you ever want to include the upper bound, you can:

def get_total_primes_inclusive(a, b):
    low = bisect.bisect_left(primes, a)
    high = bisect.bisect_right(primes, b)
    return high - low