Routing in MVC with PHP I am in the progress of creating a MVC structured website. I've opted to make my own instead of using pre-constructed MVC systems as a way to teach myself better coding and understanding how applications work. I've got everything laid out, but I'm not happy with my routing system. It is VERY crude and I'd like it to be more robust.

I'd like to get feedback to help improve my code.

Below is the full code for index.php, where my routing code is:

<?php

session_start();

// Estabilish Database Connections and system defaults
include('config.php');

function setReporting() {
    if (DEVELOPMENT_ENVIRONMENT == true) {
        error_reporting(E_ALL);
        ini_set('display_errors','On');
    } else {
        error_reporting(E_ALL);
        ini_set('display_errors','Off');
        ini_set('log_errors', 'On');
        ini_set('error_log', ROOT.DS.'tmp'.DS.'logs'.DS.'error.log');
    }
}
function db_connect() {
    $connection = mysql_connect(DB_HOST,DB_USERNAME,DB_PASSWORD);
    if (!$connection) {
        die("
Error Connecting to Database
");
    }
    if(!mysql_select_db(DATABASE, $connection)) {
        die("
Database Does Not Exist
");
    }
    return $connection;
}
function hook() {
    $params = parse_params();
    $url = $_SERVER['REQUEST_URI'];
    $url = str_replace('?'.$_SERVER['QUERY_STRING'], '', $url);

    $urlArray = array();
    $urlArray = explode("/",$url);
    var_dump($urlArray);
    if (isset($urlArray[2]) & !empty($urlArray[2])) {
        $route['controller'] = $urlArray[2];
    } else {
        $route['controller'] = 'front'; // Default Action
    }
    if (isset($urlArray[3]) & !empty($urlArray[3])) {
        $route['view'] = $urlArray[3];
    } else {
        $route['view'] = 'index'; // Default Action
    }
    include(CONTROLLER_PATH.$route['controller'].'.php');
    include(VIEW_PATH.$route['controller'].DS.$route['view'].'.php');
    var_dump($route['controller']);
    var_dump($route['view']);
    var_dump($urlArray);
    var_dump($params);

    // reseting messages
    $_SESSION['flash']['notice'] = '';
    $_SESSION['flash']['warning'] = '';
}

// Return form array
function parse_params() {
    $params = array();
    if(!empty($_POST)) {
        $params = array_merge($params, $_POST);
    }
    if(!empty($_GET)) {
        $params = array_merge($params, $_GET);
    }
    return $params;
}

// Prepare General Application Models
require($_SERVER['DOCUMENT_ROOT'].'/'.APP.'/'.'models/'.'general.php');

setReporting();
date_default_timezone_set(get_timezone());
$current_theme = current_theme();
hook();

if($_SESSION['flash']['notice']) {
    echo $_SESSION['flash']['notice'];
}

One big problem is that my hook call won't work properly when the urls are two levels deep. Say I wanted to visit mywebsite.com/admin, it would work, however mywebsite.com/admin/dashboard would not. The problem is in the arrays. How could I get the array to load content after the 2nd level along with the second level?

Would it be best to create an array like this?

Array
     - controller
     - view
          - dashboard

What would be the best way to set up "custom" URLs?

If I were to put in mywebsite.com/announcement, it would check to see if it has controllers.  Failing that, it would check to see if it has custom content (maybe a file of the same name in "customs" folder, and then if there's nothing, execute the 404 page not found stuff).  This isn't a priority question though, but loosely associated in how the code works so I thought it best to add.

First off, congrats on making your own, that's the best way to learn and usually you don't need all the added overhead that a prefab provides.

In the implementation of MVC I like to use, there is a router between the view and controller, so its more of a MVRC rather than MVC. Just another level of abstraction, but I like it. It seems like what you are trying to do here is similar. So maybe my experience will help. My routers are typically classes, but that's a preference. Here are some suggestions on your code, along with a possible solution to your problem.

Error Reporting

When you set error reporting, make sure you don't do the same thing twice, such as setting the same reporting level, it should always be the same, so just declare it before the if statement. This actually holds true for anything. This is called the DRY principle (Don't Repeat Yourself). Also, you might want to consider logging errors even while in the development environment. This ensures that you don't miss those "silent" errors and allows you to ensure that the logs are working correctly. So a possible rewrite of your setReporting() function.

function setReporting() {
    error_reporting(E_ALL);

    ini_set( 'display_errors', DEVELOPMENT_ENVIRONMENT );
    ini_set('log_errors', 'On');
    ini_set('error_log', ROOT.DS.'tmp'.DS.'logs'.DS.'error.log');
}

Would change DEVELOPMENT_ENVIRONMENT to just DEVELOPMENT or my preferred STAGING.

Edit Above I meant to say ENVIRONMENT instead of DEVELOPMENT. This is because you are no longer using this constant to check the development environment, you are using it to set an environment.

You might actually want to set up your error reporting and db connection using a config file of some sort and then just reference it in the router. Another level of abstraction, but a very common one.

Development Tools

Don't use die(), var_dump(), print_r(), etc... outside of you development environment. There are better, more elegant, ways of displaying data to the customer.

Unnecessary Work

Instead of using "REQUEST_URI" and removing "QUERY_STRING" from it, why not just use "SCRIPT_NAME"? It does the same thing.

$_SERVER[ 'SCRIPT_NAME' ];

You don't have to define $urlArray as an array if you are immediately going to assign an array to it. Essentially you are assigning it a value and then never using it, which is bad and confusing. This isn't Java or C, you don't have to type hint a variable before using it.

Always check to see if the language you are working in provides a way for you to do something easier than you could do it yourself.

Defining a Default Value

This is a stylistic choice, but instead of using else statements to define default values, just define the default values first. If those values need to change, then change them.

$route[ 'view' ] = 'index';
if (isset($urlArray[3]) & !empty($urlArray[3])) {
    $route['view'] = $urlArray[3];
}

Distinguishing Between isset() and empty()

isset() returns TRUE only if a variable is set, and its value is not NULL.

empty() returns FALSE only if a variable is not FALSE, NULL, an empty string, any form of zero, or an empty array.

Unless you explicitly need to distinguish between a variable not being set, or empty, then I would suggest only using  isset(), not both.

list()

A better way to assign array values to specific variables is to use a list(). You'll need to use array_pad() to ensure the array is the proper length first, but don't worry, it won't add anything if its already past that length. Also, I don't know why you are passing these variables to an array and then never using that array. It's unnecessary. However, I do show how to pass these variables back to an array in case you are planning on expanding upon that function.

$urlArray = array_pad( $urlArray, 4, NULL );
list( , , $controller, $view ) = $urlArray;
$route = compact( 'controller', 'view' );

Your Problem

Your array, at least in the examples you provided, is not four fields long, its three. You've skipped the first empty field, but then, instead of using "1" as your index, you use two. Arrays start indexing their fields at zero. So When you call for the indices at "2" and "3", you are actually calling for the third and fourth elements. This is why your example will work for one level, but not another. I'm surprised your examples aren't throwing out of bounds errors.

Anyways, a better way to write your hook.

function hook() {
    $params = parse_params();

    $url = $_SERVER[ 'SCRIPT_NAME' ];
    $url = trim( $url, '/' );//remove forward slash from beginning and end of $url

    $urlArray = explode( '/', $url );
    $urlArray = array_pad( $urlArray, 2, NULL );
    list( $controller, $view ) = $urlArray;

    if( ! $controller ) { $controller = 'front'; }
    if( ! $view ) { $view = 'index'; }

    $route = compact( 'controller', 'view' );
}

Now if you want to extend to another level you would just adjust the padding, add to the list, add a default, and then add it to the compact.

Custom URLs

These are done through the htaccess file with mod_rewrite. At least, I think that's right. I haven't tried figuring this out yet, so I can't give you details, but it is well documented on the internet, just google it.

UPDATE

Looks good. I put an edit in one of the above sections because of a typo. You followed most of my previous advice, so there's not much I can add. I did just notice a couple of things though.

It looks as if you are passing your post and get data directly to your PHP scripts. This is bad. You should always validate and sanitize them. If your PHP version is >= 5.2 you can use a helpful PHP function called filter_input_array(), or filter_input().

It is best to only use the concatenate . operator if you are adding variables to a string. Even though it doesn't look like much, it is still an operator and takes processing power. Admitedly not much, but you should still code so that you aren't blattantly ignoring such minor inefficiencies. Not to say that you need to go out of your way to make your script as quick as possible. Far from it. But if its a quick fix that doesn't hurt anything, why not. It may not present much improvement now, but say you find yourself working on a script with a massive array that you need to loop over. At such a point every little efficiency matters. If you are already taking care of some of the easier stuff automatically it won't be as difficult to refactor the rest.

require( $_SERVER[ 'DOCUMENT_ROOT' ] . '/' . APP . '/models/general.php' );

Last but not least, don't just copy-paste suggestions into your code. I notice my coding style made it into your code. Take time to understand suggestions and write them yourself. As you found out, my answer was not 100% operational and you had to adjust it. Even if all you are doing is manually typing out these suggestions, without any change, you will find you will remember it a little better for the next time you need it and something might even click so that you understand it better.

Hope this helps!


Managing users and profiles in Rails I am building a Rails marketplace application using TDD. I would like to get advice on the way in which I have built the User associated Profile associations and way in which these are then handled in the controllers.

User model

class User < ActiveRecord::Base
  has_one :profile, dependent: :destroy
  has_many :listings, dependent: :destroy
  has_many :watches, dependent: :destroy
  has_many :watched_listings,  -> { uniq }, :through => :watches, dependent: :destroy

  # Include default devise modules. Others available are:

  acts_as_messageable

  def mailboxer_email(object)
    email
  end

  def self.search(search)
    where("email ILIKE ?", "%#{search}%")
  end

  after_create :build_profile

end

Profile model

class Profile < ActiveRecord::Base
  belongs_to :user

  has_attached_file :avatar, :styles => { :medium => "300x300>", :thumb => "100x100>" }, :default_url => "/images/:style/missing.png"
  validates_attachment_content_type :avatar, :content_type => /\Aimage\/.*\Z/
end

Users controller

class UsersController < ApplicationController
  def index
    @users = User.all
    if params[:search]
      @users = User.search(params[:search]).order("created_at DESC")
    else
      @users = User.all.order('created_at DESC')
    end
  end

  def show
    @user = User.find(params[:id])
  end

  def watchlist
    @watched_listings = current_user.watched_listings.all
  end
end

Profiles controller

class ProfilesController < ApplicationController

  before_filter :authenticate_user!, :only [:edit, :update]
  before_filter :correct_user, :only [:edit, :update]

  def show
    @profile = Profile.find_by(user_id: params[:user_id])
  end

  def edit
    @profile = Profile.find_by user_id: current_user.id
  end

  def update
    @profile = Profile.find_by user_id: current_user.id
    if @profile.update(profile_params)
      flash[:notices] = ["Your profile was successfully updated"]
      render 'show'
    else
      flash[:notices] = ["Your profile could not be updated"]
      render 'edit'
    end
  end

  private

  def profile_params
    params.require(:profile).permit(:city, :country, :avatar)
  end

  def correct_user
    @profile = Profile.find_by(user_id: params[:user_id])
    redirect_to(root_path) unless current_user?(@profile)
  end
end

I have also defined:

Profiles helper

module ProfilesHelper
  def current_user?(user)
    user == current_user
  end
end

Within my Application Controller:

class ApplicationController < ActionController::Base
  protect_from_forgery with: :exception

  def after_sign_in_path_for(resource)
    edit_user_profile_path(resource)
  end

  def after_sign_up_path_for(resource)
    edit_user_profile_path(resource)
  end
end

I'm very keen to get any feedback on:

If the use of the after_create :build_profile is appropriate. Does this violate SRP principles by making the user model responsible for the creation of a profile?
The best way to validate the correct user in the profile controller - to enforce whether a user can edit a profile or not.
Mainly whether it is required that I be finding profiles by Profile.find_by(user_id: params[:user_id]) rather than by a Profile ID.
Any other improvements to the code!

I have RSpec and Capybara tests to back this here.

Here's my thoughts.
Profile Model
I'm not sure that the separate profile is pulling enough weight to justify its existence. It seems that a lot of this code could be avoided by collapsing it into the User model. I appreciate that the User and Profile can be two separate entities but it looks like it'll be much easier to keep them in one model until they grow too large to be viable, or there comes a time when a Profile has a separate lifecycle from a User.
Which then avoids your question around SRP altogether :)
But if we were to keep them separate, I don't think it would be a practical violation of SRP because your application likely depends on a profile always existing for a user. So having asserted that User should always have a Profile, you are then faced with the choice of leaving the creation in the hands of the controller, or the model. I favour leaving it in the hands of the model because the model is responsible for modelling the relationships between different business objects.
Finding the Profile and Validating Access
You could replace the find methods with a before_action that uses the user's relationship with the profile like so:
before_action :get_profile, only: [:edit, :update]

def get_profile
   @profile = current_user.profile
end

This will both reduce the amount of code required to select the profile, and ensure that the user can only access their profile to edit/update it.
However, at that point it doesn't make sense to have the profile edit and update actions available with a user ID. They're essentially now singular resources and should be handled accordingly in your routes.
You ask whether find by user_id or not is a good choice. It depends on whether the person accessing that route will be accessing it from the context of its User, or the Profile owner.
current_user?
I don't believe this code will work. You are passing it only Profile objects, and comparing it to the current_user method which returns a User object (or null if not logged in, if memory serves). This should always return false. You either need to call profile.user before passing it to the method, or have that method make the call.


Improve scala functional string transformation How could this scala code be made more concise and/or idiomatic? Do I need to define each functions as a val in order to pass them as values, or is there a shorthand for this? Also looking for feedback on the disenvowel function.

// See: http://nurkiewicz.blogspot.com/2012/04/secret-powers-of-foldleft-in-scala.html
def transformString(s: String, fns: List[Function1[String,String]]) :String =
  (s /: fns) { (s: String, f: Function1[String,String]) => f apply s }

val toUpper = (s: String) => s.toUpperCase
val reverse = (s: String) => s.reverse
val disenvowel = (s: String) => s filterNot { Set('a','e','i','o','u') contains _ }

// scala> transformString("blah",List(disenvowel))
// res175: String = blh

// scala> transformString("blah",List(disenvowel, toUpper))
// res176: String = BLH

// scala> transformString("blah",List(disenvowel, toUpper, reverse))
// res177: String = HLB

If I might chime in briefly, it all looks fine, any modifications here are mostly a matter of taste and personal preference; for example, I'd make it a curried function with varargs:

def transformString(s : String)(functions : String => String*) =
    functions.foldLeft(s) { (str, fun) => fun(str) }

And you don't need to define functions as literals, for example:

scala> def toUpper(s : String) = s.toUpperCase
toUpper: (s: String)String

scala> def reverse(s : String) = s.reverse
reverse: (s: String)String

And then, using the slightly modified version above:

scala> transformString("blah")(toUpper, reverse)
res3: String = HALB

Of course, you can always use the literal, too:

scala> val reverse2 = (s : String) => s.reverse
reverse2: String => String = <function1>

scala> transformString("blah")(reverse2)
res4: String = halb

Or even provide the lambda in-place:

scala> transformString("blah")(_ + "1", _.reverse)
res9: String = 1halb

IMO, varargs here work nicely, and you can transform any sequence/collection into varargs by :_*; there are a few differences, though:

def methods have to be partially applied in the list (e.g. : toUpper_ below)
Underscore shorthand syntax won't work

so:

scala> transformString("blah ")(List(reverse2, (s : String) => s + "12", toUpper_):_*)
res21: String = " HALB12"

[EDIT for the comments]

1 & 2. Oh, there is a difference. You can easily make a function partially applied if it's curried, not so much if it just takes in arguments. Also, you can make shorthand underscore syntax work after applying the function partially:

scala> def blahArg = transformString("blah")_
blahArg: Seq[String => String] => String

Note the trailing underscore and the return type: a function (with lower arity). Now you can pass that function somewhere else where the second argument can be supplied. You can't do that when taking in both arguments at once. That also means that your second parameter changed from varargs to Seq, which - in turn - means you explicitly have to pass a Seq, List or something similar:

scala> blahArg(List(_.reverse))
res10: String = halb

3.Removing vowels looks fine :) If that's contained inside an object, you can move the Set initialization to the object level, so it's only initialized once instead of on every function call, but that's nitpicking.


How can I make my isSorted function faster? I made an isSorted function in Scala and Java as well and when I measured the time of functions' run I saw that the Scala version was very slow. It ran about 3.2 sec for 10000 Int but java version just ran about 10 ms!

How can I make faster my Scala version?

Scala:

def main(args: Array[String]) ={
    println(isSorted(getArray, (x:Int,y:Int) => x<y ))
}
def isSorted[A](items : Array[A], cond: (A,A) => Boolean) : Boolean = items match{
  case Array(_) => true
  case Array(x,y) =>cond(x,y)
  case Array(_*) => cond(items(0),items(1)) && isSorted(items tail,cond)
}

Java:

public static void main(String... args){
    Sorter sorter=new Sorter();
    System.out.println(sorter.isSorted(sorter.getList(),new Comparator<Integer>() {
        @Override
        public int compare(Integer o1, Integer o2) {
            return o2.compareTo(o1);
        }
    }));
}
public <A> boolean isSorted(List<A> items, Comparator<A> cond){
    for(int i=1;i<items.size();i++){
            if(cond.compare(items.get(i-1), items.get(i)) < 0){
                return false;
            }
        }
    return true;
}

Any suggestions?

I would like to use Scala, but this bad performance scared me!
Can I keep Scala's syntactic sugar?

The problem is simply that your two solutions are not equivalent. For example, you would probably not use Array, but barring that, a direct translation of the Java code to Scala would be:

def isSorted[A](items: Array[A], cond: (A, A) => Boolean): Boolean = {
    for (i <- 1 until items.length if !cond(items(i - 1), items(i))) {
      return false
    }
    return true
}

However, using an Array in Scala is a bit uncommon, and one would rather use Seq instead. Especially, a LinearSeq promises an efficient tail operation but slow indexing, this is reversed for an IndexedSeq.

@tailrec // remind the compiler that the recursion can be eliminated
def isSorted[A](items: LinearSeq[A], cond: (A, A) => Boolean): Boolean = items match {
  case Seq()     => true  // you forgot to handle empty input!
  case Seq(_)    => true
  case x :: tail => cond(x, tail.head) && isSorted(tail, cond)
}

But in this specific case, destructuring is actually silly: The isSorted test should male sense for any iterable (let's hope that it terminates, but that isn't our problem). It doesn't actually matter that much here that we can't do much destructuring for a general trait like Iterable. This here is a fairly elegant solution, but I wouldn't make any bets about performance.

def isSorted[A](items: Iterable[A], cond: (A, A) => Boolean): Boolean =
  items.isEmpty || (items zip items.tail forall (pair => cond(pair._1, pair._2)))

The zip method joins two lists pairwise: items = Seq(1, 2, 3, 4) would then produce ((1, 2), (2, 3), (3, 4)).

I am not sure whether any of these solutions would actually be faster – it's to be expected that Scala is a bit slower than Java, but the difference for a careful implementation should not be as large as in your question.

Style notes:

Always annotate tail recursive functions with @tailrec (from the scala.annotation package). This will either perform a tail call optimization or produce an error if the compiler is unable to do so.
When specifying the type of a variable, put a space after the :, but not before: name: Type. (source)
Invoke methods by using ., except when the method is an operator or takes a function as parameter – items.tail instead of items tail (source)
In general, the spacing around your operators is inconsistent. A comma should always be followed by a space, and binary operators like = or < should have a space on either side. (source)


Check string(line) in a file and send email I want to check some lines from a log file [check msyql replication slave status log] named "status.txt" (size is always about 3-4 KB):

  Slave_IO_Running: Yes
  Slave_SQL_Running: Yes

If any of these two line's value is 'No' then email is sent to alert@example.com.  These two lines are unique and both occur only once.

#!/usr/bin/python
import smtplib
import os
import email.Utils
from email.mime.text import MIMEText
import sys
i=0
for line in open("status.txt"):
    if "Slave_IO_Running: Yes" in line:
        print "match1",line
        i=i+1
    else:
        pass

for line2 in open("status.txt"):
    if "Slave_SQL_Running: Yes" in line2:
        i=i+1
    else:
        pass
print i

if i<2:
    msg=MIMEText("Mysql not running")
    fromaddr="root@DBausit.com"
    toaddr="alert@example.com"
    server = smtplib.SMTP('localhost')
    server.set_debuglevel(1)
    server.sendmail(fromaddr, [toaddr], msg.as_string())
    server.quit()
else:
    print "mysql running OK"

Please review my code.

Contents of "status.txt" file:

    Relay_Log_Pos: 195294327
        Relay_Master_Log_File: web01-bin.000001
             Slave_IO_Running: Yes
            Slave_SQL_Running: Yes
              Replicate_Do_DB:
          Replicate_Ignore_DB:
mysql,information_schema,cphulkd,eximstats,horde,leechprotect,logaholicDB_web01,performance_schema,roundcube,whmxfer
           Replicate_Do_Table:
       Replicate_Ignore_Table:

The else-pass construct is unnecessary. The else clause is not required. Leaving it of has the same result as what you have now.

You loop over the same file twice. Instead you can check the line for both messages in the same loop.

for line in open("status.txt"):
    if "Slave_IO_Running: Yes" in line or "Slave_SQL_Running: Yes" in line2:
        print "match",line
        i=i+1

However, the above change points out a potential bug. We don't have any information about what the contents of status.txt are. If it is possible for either of those strings to appear more than once, the code as posted (and my above update) could find two matches without finding a result for IO and SQL.
1

sql_found = false
io_found = false
for line in open("status.txt"):
    if not io_found and "Slave_IO_Running: Yes" in line:
        print "match",line
        io_found = true
    if not sql_found and "Slave_SQL_Running: Yes" in line:
        print "match",line
        sql_found = true

if not sql_found or not io_found:
    # send email

If the status file is long, you can break out of the loop after both lines have been found. This would prevent unnecessary work when you already know the correct information.

i is not a good variable name. It does not convey any information about how it is being used. In addition, i is widely accepted as the variable name for indexing in a loop. In this code you are looping, but i is being used for something else. Changing the name will prevent any confusion.

You can use the with construct to ensure the file is closed when you are done with it.

with open(status) as f:
    for line in open("status.txt"):
        # operate on a line

This is a short running script, so it is not a huge deal, but it is a good practice to get into when you move to writing longer applications.
While this is a short running

You never use sys, os or email.Utils. These imports can be removed.

1: If we extrapolate on the idea that the line might show up more than once, that could also mean that the file could contain a Yes line followed by a No line. If this is the case, this simple loop logic would no longer be sufficient.


A Pythonic implementation of Conway's Game of Life I've just finished implementing Conway's Game of Life in python-3.6:

import curses
import numpy as np
import time

from collections import defaultdict
from enum import Enum
from typing import List

class State(Enum):
    ''' enum class to represent possible cell states '''
    dead = 0
    alive = 1

class Board:
    ''' Board class to represent the game board '''
    def __init__(self, m : int, n : int, init : List[List[int]]):
        self.m = m   # the number of rows
        self.n = n   # the number of columns
        self.board_ = [
            [State(init[i][j]) for j in range(self.n)] for i in range(self.m)
        ]

    def __str__(self) -> str:
        ''' return the __str__ representation of a Board object
            * represents a live cell, and a space represents a dead one
        '''
        return '\n'.join([
            ''.join(
                    ['*' if cell.value else ' ' for cell in row]
            ) for row in self.board_]
        )

    @property
    def population(self):
        ''' population — the number of live cells on the board '''
        return sum(cell.value for row in self.board_ for cell in row)

    def count_live_neighbours(self, x : int, y : int) -> int:
        ''' count the live neighbours of a cell '''
        count = 0
        for i in range(x - 1, x + 2):
            for j in range(y - 1, y + 2):
                if (i == x and j == y) or i < 0 or j < 0:
                    continue
                # handle IndexErrors raised during invalid indexing operations
                try:
                    count += self.board_[i][j].value
                except IndexError:
                    continue

        return count

    def next_cell_state(self, x : int, y : int) -> State:
        count = self.count_live_neighbours(x, y)
        cur_state = self.board_[x][y]
        # determine the next state based on the current state and
        # number of live neighbours
        if count in {2, 3} and cur_state == State.alive:
            return cur_state
        elif count == 3 and cur_state == State.dead:
            return State.alive

        return State.dead

    def next_board_state(self) -> List[List[State]]:
        ''' return board configuration for the next state '''
        return [
            [self.next_cell_state(i, j) for j in range(self.n)] for i in range(self.m)
        ]

    def advance_state(self):
        ''' update the board configuration with the config for the next state '''
        self.board_ = self.next_board_state()

    def has_live_cells(self) -> bool:
        ''' return whether there are any live cells or not '''
        return any(cell.value for row in self.board_ for cell in row)

if __name__ == '__main__':
    arr = np.random.choice([0, 1], (20, 100), p=[0.90, 0.1])
    board = Board(arr.shape[0], arr.shape[1], init=arr.tolist())

    step = 0
    while board.has_live_cells():
        step += 1
        print(board)
        print('f{step} {board.population}')
        board.advance_state()

        time.sleep(.1)

To run the code, just invoke python3.6 conway.py. The board shape is fixed; but the configuration is randomly generated with NumPy.

My aim with this project was:

Implement a concise and pythonic version of the algorithm
Get better with Python's OOP constructs
Kill time :-)

There are a few redundant functions and properties which felt would be useful at some point of time.

My next step with this program would likely be to host it on a server and let people run it on their browser with a Step and Run button. This might influence how I'd need to structure my code.

Your code is pretty good, and there's not much I'd change. However I would change it in the following ways:

It's standard to use UPPER_SNAKE_CASE for Enum values.
It's preferred to use typing.Sequence over typing.List to annotate arguments.
When you have to comment what a variable is, it indicates you've chosen a bad name. Instead use rows over m.
Either put your comprehensions on one line or split them out.

# Current way
[
    [State(init[i][j]) for j in range(self.n)] for i in range(self.m)
]

# One line
[[State(init[i][j]) for j in range(self.n)] for i in range(self.m)]

# Multiple lines
[
    [
      State(init[i][j])
      for j in range(self.n)
    ]
    for i in range(self.m)
]

It's easier to read a comprehension in a function if you keep the brackets together.

# Current
return '\n'.join([
    ''.join(
            ['*' if cell.value else ' ' for cell in row]
    ) for row in self.board_]
)

# Together
return '\n'.join([
    ''.join(['*' if cell.value else ' '
             for cell in row])
        for row in self.board_])

# Together and multiple lines
return '\n'.join([
    ''.join([
        '*' if cell.value else ' '
        for cell in row
    ])
    for row in self.board_
])

You don't need to use the surrounding brackets if your comprehension is the only argument to a function. However, this may be a couple of u seconds slower, which shouldn't matter to most people.

return '\n'.join(
    ''.join(
        '*' if cell.value else ' '
        for cell in row
    )
    for row in self.board_
)

I'd make another function neighbours. I find this nicer, as finding the neighbors can be a little annoying, and take away from the actual algorithm.

I'd also make the algorithm use a premade list, that filters out x==i already.

NEIGHBOURS = [(i, j) for i in range(-1, 2) for j in range(-1, 2) if i or j]

def neighbours(self, x: int, y: int) -> Iterator[Tuple[State, int, int]]:
    for i, j in ((x+i, y+j) for i, j in NEIGHBOURS):
        try:
            yield self.board_[i][j], i, j
        except IndexError:
            pass

def count_live_neighbours(self, x : int, y : int) -> int:
    ''' count the live neighbours of a cell '''
    return sum(cell.value for cell, _, _ in self.neighbours(x, y))

Your next_cell_state function can be simplified.

Your two ifs return State.ALIVE.
They can be merged together.
You can return early if count is three.
You can simplify the rest of the checks.

def next_cell_state(self, x : int, y : int) -> State:
    count = self.count_live_neighbours(x, y)
    if count == 3 or (count == 2 and self.board_[x][y] == State.ALIVE):
        return State.ALIVE
    return State.DEAD

import numpy as np
import time

from enum import Enum
from typing import List, Iterator, Sequence, Tuple

NEIGHBOURS = [(i, j) for i in range(-1, 2) for j in range(-1, 2) if i or j]

class State(Enum):
    ''' enum class to represent possible cell states '''
    DEAD = 0
    ALIVE = 1

class Board:
    ''' Board class to represent the game board '''
    def __init__(self, rows : int, columns : int, init : Sequence[Sequence[int]]):
        self.rows = rows   # the number of rows
        self.columns = columns   # the number of columns
        self.board_ = [
            [
                State(init[i][j])
                for j in range(self.columns)
            ]
            for i in range(self.rows)
        ]

    def __str__(self) -> str:
        ''' return the __str__ representation of a Board object
            * represents a live cell, and a space represents a dead one
        '''
        return '\n'.join([
            ''.join([
                '*' if cell.value else ' '
                for cell in row
            ])
            for row in self.board_
        ])

    @property
    def population(self) -> int:
        ''' population — the number of live cells on the board '''
        return sum(cell.value for row in self.board_ for cell in row)

    def has_live_cells(self) -> bool:
        ''' return whether there are any live cells or not '''
        return any(cell.value for row in self.board_ for cell in row)

    def neighbours(self, x: int, y: int) -> Iterator[Tuple[State, int, int]]:
        for i, j in ((x+i, y+j) for i, j in NEIGHBOURS):
            try:
                yield self.board_[i, j], i, j
            except IndexError:
                pass

    def count_live_neighbours(self, x : int, y : int) -> int:
        ''' count the live neighbours of a cell '''
        return sum(cell.value for cell, _, _ in self.neighbours(x, y))

    def next_cell_state(self, x : int, y : int) -> State:
        count = self.count_live_neighbours(x, y)
        if count == 3 or (count == 2 and self.board_[x][y] == State.ALIVE):
            return State.ALIVE
        return State.DEAD

    def next_board_state(self) -> List[List[State]]:
        ''' return board configuration for the next state '''
        return [
            [
                self.next_cell_state(i, j)
                for j in range(self.columns)
            ]
            for i in range(self.rows)
        ]

    def advance_state(self):
        ''' update the board configuration with the config for the next state '''
        self.board_ = self.next_board_state()

if __name__ == '__main__':
    arr = np.random.choice([0, 1], (20, 100), p=[0.90, 0.1])
    board = Board(arr.shape[0], arr.shape[1], init=arr.tolist())

    step = 0
    while board.has_live_cells():
        step += 1
        print(board)
        print('f{step} {board.population}')
        board.advance_state()

        time.sleep(.1)


Performant functional evaluation for enum value I have a Java enum which represents the side of an order (enum Side {ASK, BID}) and I'm constantly writing code with which I'd like to use the ternary operator, e.g.:

BigDecimal orderPrice = order.getSide() == Side.ASK ? calcAskPrice(order) : calcBidPrice(order)

The issue is that I want to future proof the code so that it fails if the provided value is neither ASK nor BID. The obvious solution seems unnecessarily verbose to me, because the pattern needs to be repeates tens (maybe even hundreds?) of times throughout the code. I'd rather not have this everywhere:

BigDecimal orderPrice;
if (order.getSide() == Side.ASK) {
   orderPrice = calcAskPrice(order);
} else if (side == Side.BID) {
   orderPrice = calcBidPrice(order);
} else {
   throw new IllegalArgumentException("The provided order must have a side that is either ASK or BID");
}

Instead I'd prefer a more functional approach, like the ternary operator.

A safer approach works, but is (i) also verbose and (ii) confusing:

BigDecimal orderPrice = order.getSide() == Side.ASK ? calcAskPrice(order) : order.getSide() == Side.BID ? calcBidPrice(order) : null;
assertNotNull(orderPrice);

Thinking about my desired syntax, I decided to add some methods to the enum such that I could write:

BigDecimal orderPrice = order.getSide().ifAsk(calcAskPrice(order)).ifBid(calcBidPrice(order)).elseFail();

This was implemented via an object that would store the value of the enum along with the two result values, and whose elseFail() method would return the appropriate result or fail. I was extremely satisfied with this, until (in pursuit of a separate issue) I noticed that my little evaluator object had a rather large footprint, but in terms of object creation & CPU time.

In retrospect this makes sense, because the code processes large numbers of updates, and is scattered with checks to the order side.

I'm now left with trying to think up a performant/low overhead way to provide similar syntax. The easiest solution that I can think of is to have a method on the enum:

public <T> T ifAskBid(T askResult, T bidResult) {
   // . . .
}

but I'm curious if anybody can think of a nicer solution.

BigDecimal orderPrice = order.getSide().ifAsk(calcAskPrice(order)).ifBid(calcBidPrice(order)).elseFail();

This seems rather long and complicated.  Consider

BigDecimal orderPrice = order.getSide().calculatePrice(order);

Or you might hide it as

BigDecimal orderPrice = order.calculatePrice();

With

BigDecimal calculatePrice() {
    return getSide().calculatePrice(this);
}

As you can read on Stack Overflow, you can then define different implementations of the method for each value of the enum.

public enum Side {

    ASK {
        @Override
        public BigDecimal calculatePrice(Order order) {
            return Something.calcAskPrice(order);
        }
    },
    BID {
        @Override
        public BigDecimal calculatePrice(Order order) {
            return Something.calcBidPrice(order);
        }
    };

    public BigDecimal calculatePrice(Order order) {
        throw new IllegalArgumentException("The provided order must have a side that is either ASK or BID");
    }

}

I wrote Something, as you don't provide enough context to write whatever it actually is.  It's possible that the calcAskPrice and calcBidPrice methods should have their contents pulled into the override methods.  Or that you need to pass some kind of additional object to the calculatePrice methods.

There's an argument that overriding a method like this is bad form.  I'll leave it up to you if that matters.  I did it here to duplicate your original functionality.  But maybe you don't need the original functionality.  Again, without more context, it's difficult to tell.

Perhaps it should go without saying, but I didn't test this, as there isn't enough context.  I did not go the extra mile and verify syntactical correctness by dummying up a driver for this code.


Sum the largest five numbers I'm doing a code-challenge problem:

Input
The first line consists of an integer T (T ≤ 100) which represents the number of test cases. Each test case consists of an integer N (5 ≤ N ≤ 100.000) which represents the number of integers, followed by a line that consists of N integers Ai (0 ≤ Ai ≤ 1000) separated by a space.
Output
For each test case, print in a line an integer that represents the sum of 5 largest numbers.

My code (below) works for small tests, but on the online judge, it resulted in Time Limit Exceeded. Some approaches I've tried that I think speed up the program (need confirmation):

Changing language from C++ to C (does it give significant boost?)
When calculating the sum, I use while instead of for loop (which one is more effective?)

#include <stdio.h>

int main () {
    int testCases;
    scanf("%d", &testCases);

    for (int i = 1; i <= testCases; i++) {
        int size, sum, temp, k, limit;
        sum = 0;
        temp = 0;
        scanf("%d", &size);

        int array[size];

        for (int i = 0; i < size; i++) {
            scanf("%d", &array[i]);
        }

        for (int i = 0; i < size; i++) {
            for (int j = i+1; j < size; j++) {
                if (array[j] < array[i]) {
                    temp = array[i];
                    array[i] = array[j];
                    array[j] = temp;
                } else {
                    continue;
                }
            }
        }

        k = size - 1;
        limit = size - 6;

        while (k > limit) {
            sum += array[k];
            k--;
        }

        printf("%d\n", sum);
    }
}

We need only 5 maximum elements, for that we don't need to sort whole array. We can use alternative approach

Approach 1: Use 5 local variables and keep track of minimum element in it.

Approach 2: Use Min-heap and insert first 5 elements of array in min-heap, then iterate on array from index = 5, if heap-root is smaller than number in array then extract that number from min-heap, and insert new.


Solving this Python implementation for Stock Gains I wanted to ask for a code review for the following interview question:

  Problem: Max Gain

  Given an list of integers, write a method - max_gain - that returns the maximum gain. Maximum Gain is defined as the maximum difference between 2 elements in a list such that the larger element appears after the smaller element. If no gain is possible, return 0.

Example:

max_gain([100,40,20,10]) ==> 0
max_gain([0,50,10,100,30]) ==> 100

Solution

def max_gain(input_list):
    if len(input_list) < 2:
        return 0
    a = input_list[0]
    max_val = 0
    for i in input_list[1:]:
        max_val = max(max_val, i-a)
        a = min(a, i)
    return max_val

As a variable name, i has a connotation of being an index, rather than a value, of a list.  I would expect it to be used in contexts more like for i, value in enumerate(values).

Making the slice input_list[1:] would involve duplicating most of the list. I would use an iterator to avoid doing that, and also to simplify the handling of the special case.

With a few improvements in naming…

def max_gain(input_list):
    prices = iter(input_list)
    best_buy = next(prices, 0)
    profit = 0
    for price in prices:
        profit = max(profit, price - best_buy)
        best_buy = min(best_buy, price)
    return profit


Simplifying the code with same list comprehensions I have such method where:

primary_images is dict,
additional is dict,
get_uri_text is some decorating method,
marker is telling how to parse two dicts.

How can I simplify this method?

    @staticmethod
    def decorate_sort_images(primary_images, additional_images, get_uri_text, marker=False):
        srt_keys = sorted(primary_images.keys())
        new_result = OrderedDict()
        if not additional_images:
            #only primary
            for _key in srt_keys:
                new_result[_key] = [get_uri_text(iter_el) for iter_el in primary_images[_key]]
            return new_result
        for _key in srt_keys:
            new_result[_key] = [get_uri_text(iter_el) for iter_el in primary_images[_key]]
            if marker:
                if _key in additional_images:
                    new_result[_key] += [get_uri_text(iter_el) for iter_el in additional_images[_key]]
            else:
                new_result[_key] += [get_uri_text(iter_el) for iter_el in additional_images[_key]]
        return new_result

As far as I can tell, the difference between the conditional branches is only whether or not to avoid KeyError occurring at additional_images[_key] when key does not exist. This code should have the same behavior:

@staticmethod
def decorate_sort_images(primary_images, additional_images, get_uri_text, marker=False):
    srt_keys = sorted(primary_images)
    new_result = OrderedDict()
    allow_key_error = additional_images and not marker
    for _key in srt_keys:
        new_result[_key] = [get_uri_text(iter_el) for iter_el in primary_images[_key]]
        if allow_key_error or _key in additional_images:
            new_result[_key] += [get_uri_text(iter_el) for iter_el in additional_images[_key]]
    return new_result

When additional_images is empty, _key in additional_images is always False, so we don't need the first if statement any more.

Note that I also removed a redundant .keys() from sorted(primary_images).


Program to solve variations of the seven thieves & diamonds riddle I answered a question yesterday on SO based on this riddle, but the code was so hard to read/follow to actually understand what was going on it was easier to write my own code to test the scenario and debug the program.

I wrote it pretty fast, but I want to make sure the program I wrote is easy to follow and adheres to basic C practices to avoid someone else have the same problem reading and following my code as I did in the original question. The program itself was a variation of the following riddle:

  There are seven thieves who steal diamonds from a diamond merchant and run away in the jungle. While running, night sets in and they decide to rest in the jungle. When everybody is sleeping, two of the best friends get up and decide to distribute the diamonds among themselves and run away. So they start distributing but find that one diamond was extra. So they decide to wake up 3rd one and divide the diamonds again...only to their surprise they still find one diamond extra. So they decide to wake up the fourth one. Again one diamond is spare. 5th woken up...still one extra. 6th still one extra. Now they wake up 7th and diamonds are distributed equally.

The solution is as follows:

  Lets say there are x diamonds, now these diamonds are exactly
  divisible by 7 and:

  x = 1 + N1*2;

  x = 1 + N2*3;

  x = 1 + N3*4;

  x = 1 + N4*5;

  x = 1 + N5*6;
  x = N6*7;

  Where N1, N2, N3, N4 and N5 are
  integers. From above we can also say:
  N1*2 = N2*3 = N3*4= N4*5 = N5*6 =y

  Now y should be divisible by 2, 3, 4, 5 and 6, its nothing but
  common multiple of all.

  LCM of 2, 3, 2*2, 5, 2*3 => 2*3*2*5 => 60

  At the same time common multiple + 1 should be divisible by 7 as well.

  60 + 1 is not divisible by 7 120(60*2) + 1 is not divisible by 7
  180(60*3) + 1 is not divisible by 7 240(60*4) + 1 is not divisible by
  7 300(60*5) + 1 is divisible by 7

  Thus they must have stolen minimum 301 diamonds.

The generalization is the program takes any number of thieves and returns a number of diamonds the thieves stole if it is a valid input (prime number).

#include<stdio.h>

int isPrime(int tmp)
{
    int i;
    for(i = 2; i <= tmp/2; i++)
    {
        if(tmp % i == 0)
            return 0;
    }
    return 1;
}

long gcd(long a, long b) {
  if (b == 0) {
    return a;
  }
  else {
    return gcd(b, a % b);
  }
}

int main()
{
    int thieves, i;
    long diamonds, lcm = 1;
    printf("Enter the number of thieves that is prime and greater than 1:\n");
    scanf("%d",&thieves);

    if(isPrime(thieves) && thieves > 1)
    {
        for(i = 2;i < thieves;i++)
        {
                lcm = (lcm*i)/gcd(i,lcm);
        }

        i = 1;
        diamonds = lcm*i + 1;
        while(diamonds % thieves != 0)
        {
            diamonds = lcm*++i + 1;
        }
        printf("The minimum number of diamonds is: %d\n",diamonds);
    }
    else
    {
        printf("Input invalid.\n");

    }
}

Given that this was quick and dirty program to solve a simple problem, it's understandable that some things aren't as "polished" as could be.  Here are some things I spotted:
gcd()
I'm guessing you copy and pasted this function because the style of the braces, the indentation, and the spacing doesn't match the rest of your code.
Overflow
When I ran your program (on a 32-bit system) I couldn't get an answer for 23 thieves because it overflowed.  Here where you compute the lcm:

       lcm = (lcm*i)/gcd(i,lcm);

you can do slightly better like this:
         lcm *= i/gcd(i,lcm);

With that change, I was able to get the answer for 23 (but not for 29).  You might want to use uint64_t instead of long to get the largest range possible (I was able to get the answer for 43 thieves using uint64_t).  Btw, that line was also indented too far for some reason.
Spacing
I personally like more spaces between things such as if(, but it's subjective.  One place that was really hard for me to read was this:

       diamonds = lcm*++i + 1;

Minor ordering change
For this line:

if(isPrime(thieves) && thieves > 1)

I would rather it be like this:
 if(thieves > 1 && isPrime(thieves))

That way you don't end up calling isPrime with some strange negative input that can have unintended consequences.


Exists Method Implementation for Multidimensional Array in C# To determine whether the specified array contains specific element or not, Array.Exists can be used if the given array is one dimensional. I am attempting to implement Exists method for multidimensional array cases.
The experimental implementation
static class MDArrayHelpers
{
    public static bool Exists<T>(Array array, Predicate<T> match)
        where T : unmanaged
    {
        if (ReferenceEquals(array, null))
        {
            throw new ArgumentNullException(nameof(array));
        }

        Type elementType = array.GetType().GetElementType();
        if (!elementType.Equals(typeof(T)))
        {
            throw new System.InvalidOperationException();
        }

        foreach (var element in array)
        {
            if (match((T)element))
            {
                return true;
            }
        }
        return false;
    }
}

Test cases
Predicate<int> isOne = delegate (int number) { return number == 1; };
Predicate<int> isFour = delegate (int number) { return number == 4; };

Console.WriteLine("One dimensional case");
int[] array1 = new int[] { 1, 2, 3 };
Console.WriteLine($"Is one existed in {nameof(array1)}: {Array.Exists(array1, isOne)}");
Console.WriteLine($"Is four existed in {nameof(array1)}: {Array.Exists(array1, isFour)}");
Console.WriteLine("");

Console.WriteLine("Two dimensional case");
int[,] array2 = { { 0, 1 }, { 2, 3 } };
Console.WriteLine($"Is one existed in {nameof(array2)}: {MDArrayHelpers.Exists(array2, isOne)}");
Console.WriteLine($"Is four existed in {nameof(array2)}: {MDArrayHelpers.Exists(array2, isFour)}");
Console.WriteLine("");

Console.WriteLine("Three dimensional case");
int[,,] array3 = { { { 0, 1 }, { 2, 3 } }, { { 0, 1 }, { 2, 3 } } };
Console.WriteLine($"Is one existed in {nameof(array3)}: {MDArrayHelpers.Exists(array3, isOne)}");
Console.WriteLine($"Is four existed in {nameof(array3)}: {MDArrayHelpers.Exists(array3, isFour)}");
Console.WriteLine("");

Console.WriteLine("Four dimensional case");
int[,,,] array4 = { { { { 0, 1 }, { 2, 3 } }, { { 0, 1 }, { 2, 3 } } }, { { { 0, 1 }, { 2, 3 } }, { { 0, 1 }, { 2, 3 } } } };
Console.WriteLine($"Is one existed in {nameof(array4)}: {MDArrayHelpers.Exists(array4, isOne)}");
Console.WriteLine($"Is four existed in {nameof(array4)}: {MDArrayHelpers.Exists(array4, isFour)}");

The output of the test code above:
One dimensional case
Is one existed in array1: True
Is four existed in array1: False

Two dimensional case
Is one existed in array2: True
Is four existed in array2: False

Three dimensional case
Is one existed in array3: True
Is four existed in array3: False

Four dimensional case
Is one existed in array4: True
Is four existed in array4: False

If there is any possible improvement, please let me know.

You can simply use the LINQ Enumerable.Any extension Method.
bool result = array4.Cast<int>().Any(x => x == 1);

This works for any collection implementing IEnumerable<T> and also for enumerations created algorithmically by C# Iterators.
According the the C# Programming Guide (Arrays):

Array types are reference types derived from the abstract base type Array. Since this type implements IEnumerable and IEnumerable<T>, you can use foreach iteration on all arrays in C#.

This statement is, however, misleading, since we need to cast the array here. Obviously, we only have the access to the non-generic interface IEnumerable.
LINQ Extension methods found in the Enumerable Class apply to IEnumerable<T> or IEnumerable.


Find largest substring in alphabetical order in a given string #include<iostream>
#include<string>

using namespace std;

int main()
{
    string s, sub;
    cin >> s;
    int check = 1, count = 0, pos = 0; // keeping count of number of alphabetical ordered substring characters
    int i = 1; // loop variable
    int prev = 0; //to hold previous value of loop variable

    while (i< s.size())
    {
        check = 1;
        while ((int)s[i] >= (int)s[i - 1])
        {
            ++check;
            ++i;
        }
        if (check > count)
        {
            count = check;
            pos = i - check;
        }

        if (i == prev)
            ++i;
        prev = i; //if the inner while loop isn't executed, then i will be incremented here
    }
    cout << "Longest substring in alphabetical order is :" << s.substr(pos, count) << endl;
}

This was a part of my assignment. I could solve this in 10 minutes but I am not sure if this is a very efficient code. Though a (if any) more efficient algorithm exists for this, it may not make much of a different but I am keen on making all my programs as efficient as possible right from beginning.

Here are a few details :

Variable declaration

It is usually a good idea to move your variable declaration in the smallest possible scope as close as possible to where they are getting used.

This for instance shows that sub is never used.

Also, you can take this chance to change your while loop into a for loop and declare i in it.

Warnings

It is a good thing to activate all warnings. For instance, I get :

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

Simplifying things

Instead of messing with prev, you can detect if i has been increased already by considering the value of check.

Cast

I do not think these casts are useful.

Length

It could be a good idea to store the length of the string in a variable not to retrieve it at every iteration.

Using namespace

using namespace is sometimes frowned upon.

At this stage the code looks like :

#include <iostream>
#include <string>

int main()
{
    std::string s = "vjvdfkjckvjkjrdsfjsdkjfliureyqnbcxvfjhdeiutogfvjdsvkdfcdcoivjriofdjsrs";
//    std::cin >> s;
    int count = 0, pos = 0; // keeping count of number of alphabetical ordered substring characters

    for (size_t i = 1, len = s.size() ; i < len ; )
    {
        int check = 1;
        while (s[i] >= s[i - 1])
        {
            ++check;
            ++i;
        }
        if (check > count)
        {
            count = check;
            pos = i - check;
        }

        if (check == 1)
            ++i;
    }
    std::cout << "Longest substring in alphabetical order is :" << s.substr(pos, count) << std::endl;
}

Algorithmic trick

A trick could be to say that if you have a current result of length n, you don't care about any substring of length m < n. Thus, you can check backward from position i + n : as long as it is in descending order, you continue, otherwise, it gives you a starting position to start again.

I am not sure if this is really clear. I'll tr to improve asap.


Best practice for cleaning Pandas dataframe columns I am cleaning str columns in a Pandas dataframe (see below for an example), and I have been wondering if there are more concise ways or additional inplace methods to do so. What are the general best practices for cleaning columns in Pandas?
import pandas as pd
df = pd.DataFrame.from_dict({"col1": [0, 1, 2, 3], "col2": ["abcd efg", ".%ues", "t12 ^&3", "yupe"]})

df["col2"] = df["col2"].str.lower()
df["col2"] = df["col2"].str.strip()
df["col2"].replace(to_replace="[^a-zA-Z ]", value="", regex=True, inplace=True)

This is not too bad. It's a good thing you use keyword arguments for the replace method
I always try to keep my original data in its original state, and continue with the cleaned dataframe.
fluent style
This lends itself very well to a kind of fluent style as in this example. I use it too, and use a lot of df.assign, df.pipe, df.query...
In this example I would do something like
df_cleaned = df.assign(
    col2=(
        df["col2"]
        .str.lower()
        .str.strip()
        .replace(to_replace="[^a-zA-Z ]", value="", regex=True)
    )
)

So definately no inplace replacements


Python program to find the subarray with maximum sum An assignment at school required me to write a program for this task using Kadane's algorithm (explained here):

  Define the function find_max_subarray that takes a list as an argument and two indices, start and end. It finds the maximum subarray in the
  range [start, end – 1].
  The function find_max_subarray returns the tuple (l, r, m) where l and r are the left and right indices of the maximum subarray and m is its
  sum.
  The function uses a loop to keep track of the maximum subarray ending at index i. That is, the maximum subarray that has the element
  at index i as its last element.
  Then the maximum subarray will simply be the maximum of all these subarrays.
  The maximum subarray ending at index i + 1 is given by max(list[i] + maximum sum of subarray ending at i, list[i]).
  The function keeps track of the maximum sum of the subarray seen so far and its left and right indices as the loop iterates.

Here is my solution to this task (using Python):

def find_max_subarray(alist, start, end):

    max_ending_at_i = max_seen_so_far = alist[start]
    max_left_at_i = max_left_so_far = start
    max_right_so_far = start + 1
    for i in range(start + 1, end):
        if max_ending_at_i > 0:
            max_ending_at_i += alist[i]
        else:
            max_ending_at_i = alist[i]
            max_left_at_i = i
        if max_ending_at_i > max_seen_so_far:
            max_seen_so_far = max_ending_at_i
            max_left_so_far = max_left_at_i
            max_right_so_far = i + 1
    return max_left_so_far, max_right_so_far, max_seen_so_far

alist = input('Enter the list of numbers: ')
alist = alist.split()
alist = [int(x) for x in alist]
start, end, maximum = find_max_subarray(alist, 0, len(alist))
print('The maximum subarray starts at index {}, ends at index {} and has sum {}.'.format(start, end - 1, maximum))

Here are some example outputs:

Enter the list of numbers: 3 -2 4 -6 7 8 -10 4
The maximum subarray starts at index 4, ends at index 5 and has sum 15.

Enter the list of numbers: 4 5 -2 0 -5 15
The maximum subarray starts at index 0, ends at index 5 and has sum 17.

Enter the list of numbers: 3
The maximum subarray starts at index 0, ends at index 0 and has sum 3.

So I would like to know whether I could make this program shorter and more efficient.

Any help would be highly appreciated.

You should always handle edge cases. For instance, if one were to provide an empty list (or None) to your find_max_subarray() function, alist[start] would throw an IndexError.


Integer to roman number converter This is (yet another) integer to Roman numeral converter.

To give it a slightly interesting twist, I've implemented this as a num_put facet of a C++ locale, so by creating the right locale, and imbuing a stream to use that locale, all the numbers written to that stream will be written out in Roman numerals (yeah, I know that's probably rarely useful or desirable, but it seemed like an interesting thing to do at the time).

Here's the header:

#ifndef ROMAN_H_INC_
#define ROMAN_H_INC_

#include <string>
#include <locale>
#include <ios>
#include <iostream>

template <class charT, class OutputIterator = std::ostreambuf_iterator<charT> >
class roman : public std::num_put < char > {
public:
    typedef charT char_type;
    typedef OutputIterator iter_type;

protected:

    virtual iter_type do_put(iter_type out, std::ios_base &, char_type, long v) const {
        return fmt(v, out);
    }

    virtual iter_type do_put(iter_type out, std::ios_base &, char_type, long long v) const {
        return fmt(v, out);
    }

    virtual iter_type do_put(iter_type out, std::ios_base &, char_type, unsigned long v) const {
        return fmt(v, out);
    }

    virtual iter_type do_put(iter_type out, std::ios_base &, char_type, unsigned long long v) const {
        return fmt(v, out);
    }

    std::string fmt_internal(unsigned long long value) const {

        struct conv {
            int val;
            std::string rep;
        };

        static const conv table[] = {
            { 1000, "M" },
            { 900, "CM" },
            { 500, "D" },
            { 400, "CD" },
            { 100, "C" },
            { 90, "XC" },
            { 50, "L" },
            { 40, "XL" },
            { 10, "X" },
            { 9, "IX" },
            { 5, "V" },
            { 4, "IV" },
            { 1, "I" }
        };

        std::string roman;

        for (auto const &c : table) {
            while (value >= c.val) {
                roman += c.rep;
                value -= c.val;
            }
        }
        return roman;
    }

    template <class T, class Iter>
    Iter fmt(T t, Iter i) const {
        std::string s = fmt_internal(t);
        std::copy(s.begin(), s.end(), i);
        return i;
    }
};

#endif

And here's a short demo program:

#include <iostream>
#include "roman.h"

int main() {
    auto loc = std::locale(std::locale(), new roman<char>);
    std::cout.imbue(loc);

    for (int i = 1; i <= 100; i++) {
        std::cout << i << "\t";
        if (i % 10 == 0)
            std::cout << "\n";
    }
}

Using Iterator

Rather than building and passing a string around.

std::string fmt_internal(unsigned long long value) const;

Why not use the output iterator directly?

You build a string letter by letter in the order you would output it, then you move that back to the calling function, then you copy the string to a stream (via an iterator). You can bypass the move and the copy by just writing the letters to the iterator.

Output Iterator not passed through:

template <class charT, class OutputIterator = std::ostreambuf_iterator<charT> >
class roman : public std::num_put < char > {
         //        Expecting =>    ^^^^^^  <CharT, OutputIterator>

OR not making it a template class.

class roman : public std::num_put<char> {

Member Types

Your base type defines some member types.

char_type   CharT
iter_type   OutputIt

This is exactly like your class. Usually I would expect you to pull these types from the parent rather than defining them independently ( unless there is a good reason).

So I would have expected:

typedef std::num_put<CharT, OutputIterator>   parent;
typedef parent::char_type  char_type;
typedef parent::iter_type  iter_type;

Return value

std::copy(s.begin(), s.end(), i);
return i;

Should this not be:

i = std::copy(s.begin(), s.end(), i);
return i;

It still seems to work (even when the stream is std::stringstream), but I am a tiny bit surprised. You are supposed to return the value of the iterator after putting stuff on the stream (not the original value). It seems to work anyway because the iterator changes the state of the object that it is putting stuff into.


Mersenne Twister random-number generator function Have a look at this Mersenne Twister random-number generator function that I've written based on what I found on SO.

As I am a beginner and don't understand half of the stuff below, I would very much appreciate if you guided me a little on my journey to improving this random number generator.

I don't need it to be fast, I just want it to be secure.
I want it to be more or less the most secure way of generating numbers.
All that, because I plan on generating random passwords with it later on.

#include <iostream>
#include <random>

unsigned long long int random_integer(unsigned long long int rand_min, unsigned long long int rand_max)
{

    // initialize = seed the random device
    std::random_device random_device;

    // use Mersenne Twister as random-number generator engine
    std::mt19937 random_number_generator(random_device());

    // number distribution, guaranteed unbiased
    std::uniform_int_distribution<int> number_distribution(rand_min, rand_max);

    return number_distribution(random_number_generator);

}

int main()
{

    for (int i = 1; i <= 10; i++)
    {
        std::cout << random_integer(1, 1000) << std::endl;
    }

    return 0;

}

Notes:

I'm on Linux.
Compiler is g++5.
Standard being C++17.

Math

Well, I believe usually you could avoid the math, but this is a special case. The main things to cover is random variable, random functions and distributions. It is gonna help a lot. Do note that int distribution is discrete, whereas real distribution is continuous.

Code Review

Incorrect usage of twister

First of all, std::mt19937 generates only 32 bit random numbers. This means that by truncating it to int (not always, depends on the platform), the code will discard higher (or lower) 16 bits of the number.

The twister has state, which means that when function generates one random number, it will discard the state and start from scratch when called again. Although std::random_device might normalize the behavior, the output will not be as intended.
Incorrect usage of distribution

std::uniform_distribution has a constructor taking T min, T max, which means that unsigned long long will overflow the arguments, which I believe is undefined behavior.

Return type is unsigned long long, but generates values are of type int, which means that higher (or lower) bits will be zero or garbage. Both of the cases are wrong.

Different solution

If you want a generator without all of the hoops to jump through, just slam everything into a class. Here is the rough sketch (might not compile):

#include <random>

class uniform_int_generator
{
    std::mt19937_64 twister;
public:
    uniform_int_generator():
                  twister(std::random_device{}())
    {}

    unsigned long long operator()(unsigned long long max_val, unsigned long long min_val)
    {
        std::uniform_int_distribution<unsigned long long> distr(max_val, min_val);
        return distr(twister);
    }
};

You might want to add some customization points using templates. Anyway, if you need more, make this one compile and post question with the updated code.


Questions regarding the use of Requests Futures for accessing REST URLs This is a much simplified version of the real code focusing just on the handling of Futures from Requests Futures.

I have a few questions:

I had to implement my own version of as_completed because the data handlers may add more Futures to _pending. Is this a decent way to handle the problem, or is there another approach?
Is stop sufficient to handle KeyboardInterrupt in all cases? It has worked well in my limited testing. I found it hard to find a solution via Google.
Is my rate limiting solution OK or is there a better approach? It is not about the number of concurrent connections but about the number of connections per second.

import argparse
from concurrent.futures import ThreadPoolExecutor
import requests
from requests_futures.sessions import FuturesSession
import time

def background_callback(sess, resp):
    # parse the json storing the result on the response object
    if resp.status_code == requests.codes.ok:
        resp.data = resp.json()
    else:
        resp.data = None

class JSONRetriever(object):
    def __init__(self):
        self._executor = ThreadPoolExecutor(max_workers=10)
        self._session = FuturesSession(executor=self._executor)
        self._pending = {}

    def fetch(self, url):
        future = self._session.get(url,
                                   background_callback=background_callback)
        self._pending[future] = url

    def drain(self):
        # Look for completed requests by hand because in the real code
        # the responses my trigger further URLs to be retrieved so
        # self._pending is modified. New requests being added really
        # confused as_completed().
        for future in [f for f in self._pending if f.done()]:
            url = self._pending[future]
            del self._pending[future]

            response = future.result()
            response.raise_for_status()
            if response.status_code == requests.codes.ok:
                print response.data
                # real code would handle data possibly adding more requests
            else:
                # the real code is smarter, this is just for CR
                raise Exception("FIXME: unhandle response")

    def finish(self):
        while self._pending:
            self.drain()
            if self._pending:
                time.sleep(1)

    def stop(self):
        for i in self._pending:
            try:
                i.cancel()
            except Exception as e:
                sys.stderr.write("Caught: " + str(e) + "\n")

        self._executor.shutdown()

if __name__ == "__main__":
    parser = argparse.ArgumentParser(description="Perform all REST calls")
    parser.add_argument("--delay", type=int, default=0)
    parser.add_argument("urls", nargs="+")
    args = parser.parse_args()

    retriever = JSONRetriever()

    try:
        for url in args.urls:
            retriever.fetch(url)
            if args.delay > 0:  # may need a delay to rate limit requests
                time.sleep(args.delay)
                retriever.drain()  # clear any requests that completed while asleep

        retriever.finish()
    except KeyboardInterrupt:
        retriever.stop()

Nice code, clearly written.

I understand the rate limiting requirement.
Having the drain() call within the loop doesn't seem like the caller's responsibility, better to let the BG callback handle it, or defer until finish() as written, which does make sense. Each url fetch could take more or less than the delay time. So this seems to be a bug / wart still lurking within the code.


Login field validator I have this basic JavaScript that, using a constructor, validates the form where the login fields can't be empty. All this WITHOUT using any JS libraries.

function Validator(txtbox) {
    this.txtbox = txtbox;
}

var validator = new Validator([ "username", "password"]);

Validator.prototype.validate = function(form) {
    for(var i = 0, l=this.txtbox.length; i < l; i++) {
        var status = document.getElementById("status-msg");
        if (form[this.txtbox[i]].value == 0) {
            status.innerHTML="The " + form[this.txtbox[i]].name + " is empty";
            status.style.display = "inline-block"
            status.className = "error";
            return false;
        }
        else {
             status.innerHTML="Login successful";
            status.style.display = "inline-block"
             status.className = "success";
        }
    }
}

function runValidate(form) {
    validator.validate(form);
}

I'm wondering how I can make it more efficient or better.

Why not add some JSDoc to your code:

/**
*@constructor
*@param {Array} txtbox - An array of whatever
*@returns {Validator}
*/
function Validator(txtbox) {
    /**
    *@private
    */
    this.txtbox = txtbox;
}

var validator = new Validator([ "username", "password"]);

/**
*@public
*@memberOf Validator.prototype
*@param {Form} form - The form holding the login information
*/
Validator.prototype.validate = function(form) {
    for(var i = 0, l=this.txtbox.length; i < l; i++) {
        var status = document.getElementById("status-msg");
        if (form[this.txtbox[i]].value == 0) {
            status.innerHTML="The " + form[this.txtbox[i]].name + " is empty";
            status.style.display = "inline-block"
            status.className = "error";
            return false;
        }
        else {
             status.innerHTML="Login successful";
            status.style.display = "inline-block"
             status.className = "success";
        }
    }
}

function runValidate(form) {
    validator.validate(form);
}


Is this an elegant/accurate simulation of the Monty Hall problem? I'm trying to simulate the Monty Hall problem, to statistically determine if there is any benefit to changing my choice after a door containing a goat is open (testing this guy's Monty Hall theory on YouTube).

So, does this manage to simulate it properly? (Just for the curious: Changing doors doesn't seem to have any benefit, but I want to make sure my simulator isn't flawed).  I also want style help, if possible!

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

#define DOOR_COUNT 3
#define TEST_COUNT 1000000

void InitRandom();
int RunTest(char change);
void RandomizeDoors();
int GetRandom(int floor, int roof);
void OpenGoatDoor (int choice);
int OtherDoor (int choice);
int ChoiceWasCar(int choice);

char doors[DOOR_COUNT+1];

int main (const char argc, const char **argv)
{
    int run[2], yes[2];
    InitRandom();

    run[0] = 0;
    run[1] = 0;
    yes[0] = 0;
    yes[1] = 0;
    // Control Group
    for (int i = 0; i < TEST_COUNT; i++ ) {
        run[0]++;
        yes[0] += RunTest(0);
    }
    for (int i = 0; i < TEST_COUNT; i++ ) {
        run[1]++;
        yes[1] += RunTest(1);
    }
    printf("Control Group:\n\t%d tests run\n\t%d yes.\n\n", run[0], yes[0]);
    printf("Other Group:\n\t%d tests run\n\t%d yes.\n\n", run[1], yes[1]);
}

void InitRandom()
{
    unsigned int iseed = (unsigned int)time(NULL);
    srand (iseed);
}

int RunTest(char change)
{
    RandomizeDoors();
    int choice = GetRandom(1, DOOR_COUNT);
    //printf("Choice is: %d\n", choice);
    OpenGoatDoor(choice);
    if (change) {
        choice = OtherDoor(choice);
    }
    return ChoiceWasCar(choice);
}

void RandomizeDoors()
{
    char n = GetRandom(1,DOOR_COUNT);
    for (int i = 0; i <= DOOR_COUNT; i++) {
        doors[i] = n == i ? 1 : 0;
        //printf("Door %d is %d\n", i, doors[i]);
    }
//  printf("\n");
}

int GetRandom(int floor, int roof)
{
    int n;
    n = (rand()%(roof-floor));
    n += floor;
    return n;
}

void OpenGoatDoor (int choice)
{
    int doorCur = GetRandom(1, DOOR_COUNT);
    while ( doors[doorCur] == 1 || doorCur == choice ) {
        if (doorCur > DOOR_COUNT) {
            doorCur = GetRandom(1, DOOR_COUNT);
        } else {
            doorCur++;
        }
    }
    //printf("Opened Door is %d\n", doorCur);
    doors[doorCur] = -1;
}

int OtherDoor (int choice)
{
    int doorCur = 1;
    while ( doorCur == choice || doors[doorCur] == -1 ) {
        doorCur++;
    }
    return doorCur;
}

int ChoiceWasCar(int choice)
{
    return doors[choice] == 1;
}

ok, so i ended up changing more than was strictly necessary, sorry.  as you probably know, it does pay to change doors, so your code did have a bug.  i didn't try to trace it down exactly, but i assume it was related to you having 4 doors instead of 3, which i guess was because you had

for (int i = 0; i <= DOOR_COUNT; i++) {

instead of

for (int i = 0; i < DOOR_COUNT; i++) {

in C you always start from 0 and use < in loops.  it's the law.

[in general your code is super neat and pretty good.  you had a muddle with array indexing, which is very common, and didn't make use of enum, but apart from that, it was nice code.  edit: also, your general approach was fine.  this is a good way to test the problem.]

some other issues were:

the random number generation was strangely general, yet always picked a door.  it also started from 1, which i guess is related to the confusion over door array indexing.
it wasn't clear to me what the doors array contained at first.  when i understood i clarified it with an enum.
it wasn't clear to me what the two groups were ("control" didn't explain things).  when i understood i clarified things with another enum (note the GROUP_COUNT trick which gives the number of entries in the enum, excluding that).
given the group enum, there's no need to have two print statements - we can use an array of group names.  and there's no need for arrays of results, since we have everything in a loop now.
i clarified the logic in OpenGoatDoor - the code initialises the door to choice, which we know is "bad" so that the while is triggered and so we only need a single call to RandomDoor.
i tried to clarify the logic in OtherDoor but i am not sure i helped.
i don't think ChoiceWasCar is necessary - with the enum it's pretty clear what return doors[choice] == car means.

here's the modified code.  thanks for posting - i was bored and this was interesting to look at.

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

#undef DEBUG
#define DOOR_COUNT 3
#define TEST_COUNT 1000000

typedef enum {goat, car, opened} door;
door doors[DOOR_COUNT];

typedef enum {nochange, change, GROUP_COUNT} group;
char *groupName[GROUP_COUNT] = {"No Change", "Change"};

void InitRandom();
int RunTest(group g);
void RandomizeDoors();
int RandomDoor();
void OpenGoatDoor(int choice);
int OtherDoor(int choice);

int main (void)
{
  InitRandom();

  for (group g = 0; g < GROUP_COUNT; ++g) {
    int wins = 0;
    for (int i = 0; i < TEST_COUNT; ++i) {
      wins += RunTest(g);
    }
    printf("%s:\n\t%d tests run\n\t%d win car.\n\n",
           groupName[g], TEST_COUNT, wins);
  }
  return 0;
}

void InitRandom()
{
  unsigned int iseed = (unsigned int)time(NULL);
  srand (iseed);
}

int RunTest(group g)
{
  RandomizeDoors();
  int choice = RandomDoor();
  #ifdef DEBUG
  printf("Choice is: %d\n", choice);
  #endif
  OpenGoatDoor(choice);
  if (g == change) {
    choice = OtherDoor(choice);
  }
  return doors[choice] == car;
}

void RandomizeDoors()
{
  char carDoor = RandomDoor();
  for (int i = 0; i < DOOR_COUNT; i++) {
    doors[i] = carDoor == i ? car : goat;
    #ifdef DEBUG
    printf("Door %d is %d\n", i, doors[i]);
    #endif
  }
  #ifdef DEBUG
  printf("\n");
  #endif
}

int RandomDoor()
{
  // this is very slightly biased, but we don't care.
  return rand() % DOOR_COUNT;
}

void OpenGoatDoor(int choice)
{
  int randomDoor = choice;
  while ( randomDoor == choice || doors[randomDoor] != goat) {
    randomDoor = RandomDoor();
  }
  #ifdef DEBUG
  printf("Opened Door is %d\n", randomDoor);
  #endif
  doors[randomDoor] = opened;
}

int OtherDoor(int choice)
{
  int otherDoor = choice;
  while ( otherDoor == choice || doors[otherDoor] == opened ) {
    otherDoor = (otherDoor + 1) % DOOR_COUNT;
  }
  return otherDoor;
}

for anyone following along at home, gcc needs -std=c99.

output:

: gcc -std=c99 mhall.c; ./a.out
No Change:
        1000000 tests run
        333875 win car.

Change:
        1000000 tests run
        667098 win car.


Looping over a map requiring casting and instance checks The following method works. I am looking for advice to make it more readable.
To see if I could avoid any unneccesary checks / casting and the instance of check.
Also not a fan of having to recreate the initial String with every loop but not sure if there is a way around it.

  Things to note.
  I am using Java 8 and Spring boot.
  I can't change the productMap. It will remain as a Map < String, Object> .
  Can use external library Apache commons but not Guava.
  It is possible for initialText and productMap to be null / empty.
  In the map, I am only looking for values which are of type String and value contains only numbers.
  If it exists, I want to use it and replace initialText

Example scenario with following data in the parameters being passed in:

String initialText = "This is a sample text with PLACEHOLDER and yet another PLACEHOLDER and more message."
productMap = {
    "Some key" : new SomeObject(), // I won't use this cos value is not String.
    "PLACEHOLDER" : "26000", // I will use this to replace cos the key PLACEHOLDER exists in above text and it contains only numbers.
    "SOME_OTHER" : "SOME_TEXT", // it is a String value but the key doesn't exist in the initial text thus no difference.
}

After executing the following code, I expect the method to return the updated String as follows (notice the comma due to String format):

"This is a sample text with 26,000 and yet another 26,000 and more message."

Method as follows. Appreciate the advices. Thanks.

public String update(String initialText, Map<String, Object> productMap){
    if(StringUtils.isBlank(initialText) || MapUtils.isEmpty(productMap)){
        return initialText;
    }
    for (Map.Entry<String, Object> productData : productMap.entrySet()) {
        String key = productData.getKey();
        Object value = productData.getValue();
        if(value instanceof String && ((String) value).matches("[0-9]+")){
            String offerValue = (String)value;
            initialText = initialText.replaceAll(key, String.format("%,d", Integer.parseInt(offerValue)));
        }
    }
    return initialText;
}

public String update(String initialText, Map<String, Object> productMap){         //  1
    if(StringUtils.isBlank(initialText) || MapUtils.isEmpty(productMap)){         //  2
        return initialText;                                                       //  3
    }                                                                             //  4
    for (Map.Entry<String, Object> productData : productMap.entrySet()) {         //  5
        String key = productData.getKey();                                        //  6
        Object value = productData.getValue();                                    //  7
        if(value instanceof String && ((String) value).matches("[0-9]+")){        //  8
            String offerValue = (String)value;                                    //  9
            initialText = initialText.replaceAll(key, String.format("%,d", Integer.parseInt(offerValue)));   //10
        }                                                                         // 11
    }                                                                             // 12
    return initialText;                                                           // 13
}                                                                                // 14

Line 1

The method and parameter names could be clearer. A method named update could do many things. How about something akin to replacePlaceholders. Analogously, initialText would become template. Also, the Hungarian notation for the map parameter isn't needed (How should I name a java.util.Map?). Maybe substitutionByPlaceholder or something similar. I feel like there could be a better name, but I lack the domain knowledge.

Also, I always make parameters, variables and such final. I swear by it, but maybe your style guide disallows it.

Making the parameters final also prevents you from reassigning them, which is usually a good thing, because it prevents silly mistakes like

public void setFoo(int foo) {
    foo = foo;
}

It looks obvious now, but in a more complex method, it can cause a bug real' quick.

Lines 5, 6, 7

Again, final variables are recommended.

Lines 6, 7

key and value aren't great names. How about placeholder and replacement?

Line 8

Your method of testing if the String is an int is flawed. How about negative Integers, or something like 100000000000000000000000000. That sure isn't an Integer. Be sure to think about corner cases like this!

If it's almost always an Integer or performance doesn't matter to you, you can just use Integer.parseInt wrapped in a try catch block. Deliberately catching Exceptions for control flow is usually not recommended though. For alternatives, see What's the best way to check if a String represents an integer in Java?

The rest looks okay, but it's a big method. Not huge by any means, but it still does too much. To quote Robert C. Martin in Clean Code:

  Functions should do one thing. They should do it well. They should do it only.

If you look at your method, it does multiple things. It guards against bad input, it loops over the map, it checks if entries are valid, and it does the replacement.

If we split your method into four method, they all become just a few lines each, each method doing exactly one thing (Barring the last one, which does two things, checking if the String is an integer and replacing, which is a symptom of us using try-catch for control flow) that is very easy to understand.

Also all methods may be static since they don't use any instance members.

public static String replacePlaceholders(final String template, final Map<String, Object> productMap) {
    if (StringUtils.isBlank(template) || MapUtils.isEmpty(productMap)) {
        return template;
    }

    return replaceAllPlaceholders(template, productMap);
}

private static String replaceAllPlaceholders(final String template, final Map<String, Object> productMap) {
    String result = template;

    for (final Map.Entry<String, Object> productData : productMap.entrySet()) {
        result = replacePlaceholderIfValueIsIntegerString(result, productData);
    }

    return result;
}

private static String replacePlaceholderIfValueIsIntegerString(final String template, final Map.Entry<String, Object> entry) {
    if (entry.getValue() instanceof String) {
        return replacePlaceholderIfReplacementIsInteger(entry.getKey(), (String) entry.getValue(), template);
    }
    return template;
}

private static String replacePlaceholderIfReplacementIsInteger(final String placeholder, final String replacement, final String template) {
    try {
        return template.replaceAll(placeholder, String.format("%,d", Integer.parseInt(replacement)));
    } catch (final NumberFormatException e) {
        return template;
    }
}

Recommended reading:

Robert C. Martin; Clean Code
Joshua Block; Effective Java, 3rd Edition


Notification script | from RSS to Email | Bash This is a script that must send an email at each new article published on a specific website. Any suggestions or improvements to do?

SENDER_EMAIL="sender@example.com"
TO_EMAIL="myemail@example.com"
RSS_SITE="example.com/feed.xml"
CHECK_INTERVAL=10

while [ 1 ]; do

    LINK_ARTICLE=$(rsstail -i 1 -u $RSS_SITE -l -n 0 -1 | grep -oP "Link:+ \K.*")
    TITLE_ARTICLE=$(rsstail -i 1 -u $RSS_SITE -n 0 -1 | grep -oP "Title:+ \K.*")

    if [ "$LINK_ARTICLE" != "" ] && [ "$TITLE_ARTICLE" != "" ]; then
        echo "New article published on the site. TITLE: $TITLE_ARTICLE - LINK: $LINK_ARTICLE" | EMAIL="$SENDER_EMAIL"  mutt -s "Nuovo Articolo BDO" "$TO_EMAIL"
        echo "New article published on the site. TITLE: $TITLE_ARTICLE - LINK: $LINK_ARTICLE"
    fi

    sleep $CHECK_INTERVAL
done

I see a number of things that may help you improve your code.

Use "shebang" line

The shebang is the line at the beginning of a shell script that tells which program to use.  In this case, you probably want this:

#! /usr/bin/env bash

See this question for details.

Consider using cron instead of sleep

If this is something you want to run automatically, consider running it as a cron tab instead of using sleep within the script.

Include some comments

The program requires rsstail, mutt and sleep which is a requirement that should be documented in a comment.

Be cautious about handing variables to programs

The mutt program, like many Linux programs, has a -- option which specifies that no further options are on the command line.  This prevents the contents of $TO_EMAIL in a line like the following from being misinterpreted as a command line option.

mutt -s $TITLE -- $TO_EMAIL < $BODYTEXT

Combine strings

The echo is used twice with an identical string.  An alternative approach is

TITLE="Nuovo Articolo BDO"
BODYTEXT="New article published on the site. TITLE: $TITLE_ARTICLE - LINK: $LINK_ARTICLE"
mutt -s $TITLE -- $TO_EMAIL < $BODYTEXT
echo $BODYTEXT

Avoid creating extraneous variables

Instead of creating SENDER_EMAIL, you could just specify EMAIL and then the reassignment of the latter variable before mutt is called would not be necessary.

Consider writing a portable script

By sticking closely with Posix and avoiding bashisms your code could run on many different kinds of systems, including recent versions of Ubuntu which don't use bash.


Keyboard music maker I enjoy making silly programs every once in a while. This is one of them. It makes nice sounding notes that vary slightly depending on the key being entered into a TextArea. When you press enter, it plays back all the notes you have entered. That's pretty much all it does. I'm looking for constructive criticism regarding the coding style, and perhaps usability.

package boop;

import java.awt.TextArea;
import java.awt.event.KeyEvent;
import java.awt.event.KeyListener;
import java.util.HashMap;
import javax.sound.sampled.AudioFormat;
import javax.sound.sampled.AudioSystem;
import javax.sound.sampled.LineUnavailableException;
import javax.sound.sampled.SourceDataLine;
import javax.swing.JFrame;

public class Boop extends JFrame implements KeyListener {
    private final TextArea textArea;

    private class Noise implements Runnable {

        private int noise[];
        public boolean sound = true;
        private final int freq;
        private int ms = 100;
        private int samples = (int) ((ms * SAMPLE_RATE) / 1000);
        private byte[] output = new byte[samples];
        private int i = 0;
        private double period;

        public Noise(int freq) {
            this.freq = freq;
            period = (double) SAMPLE_RATE / freq;
            new Thread(this).start();
        }

        @Override
        public void run() {
            SourceDataLine line;
            try {
                line = AudioSystem.getSourceDataLine(af);
                line.open(af, SAMPLE_RATE);
            } catch (LineUnavailableException ex) {
                ex.printStackTrace();
                return;
            }
            line.start();
            while (i < output.length) {
                double angle = 2.0 * Math.PI * i / period * 1.0 + 0.08;
                output[i++] += (byte) (Math.sin(angle) * 127f);
            }
            while (sound) {
                line.write(output, 0, output.length);
            }
            line.close();
        }
    }

    private class Reader implements Runnable {
        private final String sz;
        Reader(String s) {
            sz = s;
            new Thread(this).start();
        }
        public void run() {
            Noise n;
            for (char c: sz.toCharArray()) {
                n = new Noise(c % 20 * 110);
                try {
                    Thread.sleep(100);
                } catch (InterruptedException ex) {
                    continue;
                }
                n.sound = false;
            }
        }
    }

    private static final int SAMPLE_RATE = 16 * 1024;
    private final AudioFormat af =
            new AudioFormat(SAMPLE_RATE, 8, 1, true, true);
    private HashMap<Character, Noise> noises;

    public static void main(String[] args) {
        new Boop();
    }

    public Boop()  {
        noises = new HashMap<>();
        textArea = new TextArea(20, 80);
        textArea.addKeyListener(this);
        add(textArea);
        pack();
        setVisible(true);
        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    }

    @Override
    public void keyTyped(KeyEvent ke) {
        try {
            keyReleased(ke);
        } finally {
            char ch = ke.getKeyChar();
            Noise t = new Noise(ch % 20 * 110);
            noises.put(ch, t);
        }
    }

    @Override
    public void keyPressed(KeyEvent ke) {
        if (ke.getKeyCode() == KeyEvent.VK_ENTER)
            new Reader(textArea.getText().toString());
    }

    @Override
    public void keyReleased(KeyEvent ke) {
        try {
            noises.get(ke.getKeyChar()).sound = false;
        } finally {
            return;
        }
    }
}

Key-to-frequency mapping

Characters with an ASCII code that is a multiple of 20 (such as d) are mapped to 0 Hz, resulting in silence.  You wrote new Noise(c % 20 * 110) in two places, so fixing this problem will require changing both call sites.  To avoid such duplication, it would be better to have a Noise(char c) constructor that chains to the Noise(int freq) constructor.

Since the frequencies are spaced at uniform intervals of 110 Hz, the higher frequencies will sound similar in pitch.  From e to f is one octave; going from v to w is a half step.  That's fine, if that's the effect you want.

Your sampling frequency, 16 * 1024 Hz, is sufficiently high to provide good sound quality for the tones you want to generate.  However, it's still a magic number pulled from nowhere.  I suggest that you stick to conventional values, such as 44100 Hz (CD audio quality) or 48000 Hz (DVD audio quality).

Continuity

When you hold down a key, you'll hear something that sounds like machine gun fire, at 10 rounds per second.  It appears that your intention is to play the tone continuously (possibly simultaneously with other tones) until the key is released, in which case a smooth sinusoid would be preferable.

The main cause for the machine-gun effect is that you are writing sounds to the audio device without regard to what is already playing.  A secondary problem is that you write 214 samples, which is in general not an integral number of wave periods.

The way to play a waveform repeatedly is not to have your own thread pump the audio data to the output, but to use Clip.loop(Clip.LOOP_CONTINUOUSLY).

Your KeyListener handlers need adjusting as well.  I would use just keyPressed() and keyReleased(), not keyTyped().  In keyReleased(), the finally is superfluous.

Constructors

Constructors should just construct objects.  I don't expect that instantiating a Noise also starts playing it, since new Thread() doesn't imply starting the thread.  Similarly, instantiating a Boop shouldn't also cause the frame to be shown, since that's not how JFrames behave.  Automatically calling .show() in the constructor violates the substitutability principle for subclasses.

As per the documentation, Swing code should run on the Event Dispatch, not the main thread.

Noise should be a static inner class.  af should be a static field of Boop.

Suggested solution

import java.awt.TextArea;
import java.awt.event.KeyEvent;
import java.awt.event.KeyListener;
import java.util.HashMap;
import javax.sound.sampled.AudioFormat;
import javax.sound.sampled.AudioSystem;
import javax.sound.sampled.Clip;
import javax.sound.sampled.LineUnavailableException;
import javax.sound.sampled.SourceDataLine;
import javax.swing.JFrame;
import javax.swing.SwingUtilities;

public class Boop extends JFrame implements KeyListener {
    private static class Noise {
        private int freq;
        private Clip clip;
        private static Clip currentClip;

        public Noise(char c) {
            this((c % 20 + 1) * 110);
        }

        public Noise(int freq) {
            this.freq = freq;
            double slice = 2 * Math.PI * freq / SAMPLE_RATE;
            byte[] waveform = new byte[(int)(SAMPLE_RATE / freq + 1)];
            for (int i = 0; i < waveform.length; i++) {
                waveform[i] = (byte)(127 * Math.sin(i * slice));
            }

            try {
                this.clip = AudioSystem.getClip();
                this.clip.open(af, waveform, 0, waveform.length);
            } catch (LineUnavailableException ex) {
                ex.printStackTrace();
            }
        }

        public void play(int msec) {
            if (this.clip != null) {
                if (Noise.currentClip != null) {
                    Noise.currentClip.drain();
                }
                Noise.currentClip = this.clip;
                this.clip.loop(msec * this.freq / 1000);
            }
        }

        public void start() {
            if (this.clip != null) {
                this.clip.loop(Clip.LOOP_CONTINUOUSLY);
            }
        }

        public void stop() {
            if (this.clip != null) {
                this.clip.stop();
            }
        }
    }

    private static final int SAMPLE_RATE = 44100;
    private static final AudioFormat af =
            new AudioFormat(SAMPLE_RATE, 8, 1, true, true);
    private HashMap<Character, Noise> noises = new HashMap<>();

    private final TextArea textArea;

    public Boop() {
        textArea = new TextArea(20, 80);
        textArea.addKeyListener(this);
        add(textArea);
        pack();
        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    }

    public void playText(String sz) {
        for (char c: sz.toCharArray()) {
            (new Noise(c)).play(100);
        }
    }

    @Override
    public void keyTyped(KeyEvent ke) {
    }

    @Override
    public void keyPressed(KeyEvent ke) {
        if (ke.getKeyCode() == KeyEvent.VK_ENTER) {
            playText(textArea.getText().toString());
        } else {
            char ch = ke.getKeyChar();
            if (!noises.containsKey(ch)) {
                Noise t = new Noise(ch);
                noises.put(ch, t);
                t.start();
            }
        }
    }

    @Override
    public void keyReleased(KeyEvent ke) {
        Noise n = noises.get(ke.getKeyChar());
        if (n != null) {
            n.stop();
            noises.remove(ke.getKeyChar());
        }
    }

    public static void main(String[] args) {
        SwingUtilities.invokeLater(new Runnable() {
            public void run() {
                (new Boop()).setVisible(true);
            }
        });
    }
}