Webscraping application that collects metal prices and converts them from USD to MXN real-time using BeautifulSoup from bs4 import BeautifulSoup
import requests
import re
import datetime

metal_translation = {"Aluminium": "Aluminio", "Copper": "Cobre", "Zinc": "Zinc", "Nickel": "Níquel", "Lead": "Plomo", "Tin": "Estaño",
                     "Aluminium Alloy": "Aleación de Aluminio", "Cobalt": "Cobalto", "Gold*": "Oro*", "Silver*": "Plata*",
                     "Steel Scrap**": "Chatarra de Acero", "NASAAC": "NASAAC", "Steel Rebar**": "Varilla de Acero"}

def get_metal_values():
    names = []
    prices = []
    metals = requests.get('https://www.lme.com/').text
    soup = BeautifulSoup(metals, 'lxml')
    metal_table = soup.find("table", attrs={"class": "ring-times"})
    metal_table_names, metal_table_prices = metal_table.tbody.find_all("th"), metal_table.tbody.find_all("td")
    for name in metal_table_names:
        names.append(name.text.replace("LME ", ""))
    for price in metal_table_prices:
        prices.append(price.text.strip())
    return names, prices

def get_peso_conversion():
    peso = requests.get('https://themoneyconverter.com/USD/MXN').text
    soup1 = BeautifulSoup(peso, 'lxml')
    conversion = soup1.find("div", class_="cc-result").text
    rate = re.search("\d{2}\.\d{4}", conversion).group()
    return rate

def get_time():
    date = datetime.datetime.now()
    time = (f'{date.day}/{date.month}/{date.year} | {date.hour}:{date.minute}')
    return time

def convert_values():
    names, prices = get_metal_values()
    rate = get_peso_conversion()
    metal_data = dict(zip(names, prices))
    for k, v in metal_data.items():
        v = (float(v.replace(",", "")) * float(rate))
        v = ("%.2f" % v)
        k = metal_translation[k]
        print(f'{k}: {v} $')

def program_run():
    print("Metal Prices by ETHAN HETRICK")
    print("================================================")
    print(f'{get_time()} | 1 USD = {get_peso_conversion()} MXN\n')
    print("Precios de metales de London Metal Exchange: (Por tonelada métrica, *Por onza Troy)\n")
    convert_values()
    print("================================================")

program_run()
input("\nEscribe 'x' para terminar.\n")

EXAMPLE OUTPUT:

Metal Prices by ETHAN HETRICK
================================================
26/6/2020 | 18:28 | 1 USD = 23.0622 MXN

Precios de metales de London Metal Exchange: (Por tonelada métrica, *Por onza Troy)

Aluminio: 36484.40 $
Cobre: 138038.80 $
Zinc: 47438.95 $
Níquel: 293097.50 $
Plomo: 41004.59 $
Estaño: 391826.78 $
Aleación de Aluminio: 27997.51 $
NASAAC: 27444.02 $
Cobalto: 657272.70 $
Oro*: 40711.70 $
Plata*: 410.74 $
Chatarra de Acero: 6065.36 $
Varilla de Acero: 9720.72 $
================================================

Escribe 'x' para terminar.

My fiances father owns a metal recycling business in Mexico City, Mexico which requires him to do a lot of calculations when negotiating with clients. Since he was doing this by hand, I decided to automate this for him. I used London Metal Exchange, which is his preferred site to get the current prices, for the metal prices and also got the exchange rate for USD to MXN and applied that as well. I also needed to translate everything to spanish so I used a dictionary. This is my first time utilizing webscraping and the datetime module so any advice to make this program run more efficiently or make the code more concise is much appreciated!

Check for failure
metals = requests.get('https://www.lme.com/').text

should be
response = requests.get('https://www.lme.com/')
response.raise_for_status()
metals = response.text

String literals don't need parens
date = datetime.datetime.now()
time = (f'{date.day}/{date.month}/{date.year} | {date.hour}:{date.minute}')

would only need parens if it spans multiple lines.
Also, date shouldn't be called date for a couple of reasons:

it shadows datetime.date, and
it has a time, not just a date.

Finally: try using strftime rather than extracting the time's individual components to render a string.
No intermediate dict
metal_data = dict(zip(names, prices))
for k, v in metal_data.items():

The only reason you'd want to do this is if you're deeply concerned that there are duplicates, which this would remove. If there are no duplicates, then simply
for k, v in zip(names, prices):

Combine format strings
    v = ("%.2f" % v)
    k = metal_translation[k]
    print(f'{k}: {v} $')

can be
print(f'{metal_translation[k]}: {v:.2f}')


Want to check if I am using PDO correctly So today after few years of not programming in PHP I decided to go back, but to do so I needed to change all MySQL connections. So I want to check if I am using it good.

Earlier I used to write something like this:

$user = clean($_COOKIE["Logged"]);
$GameReg = mysql_fetch_array(mysql_query("SELECT * FROM DB.GameReg WHERE hash='$user'"));

But now it is like this:

$stmt = $dbh->prepare("SELECT * FROM DB.GameReg WHERE hash = :user");
$stmt->bindParam(':user', $user);
$user = $_COOKIE["Logged"];
$stmt->execute();
$GameReg = $stmt->fetch(PDO::FETCH_ASSOC);

So am I using it correctly? I used to do all kinds of filtering before inserting or updating SQL table, but I've read that prepared statements doesn't need that. Is this safe to use?

As I am knew to PDO I am trying to save up some code, so I kinda came up with this:

$stmt = $dbh->prepare("SELECT * FROM DB.GameStats WHERE hash = :user");
$stmt = $dbh->prepare("SELECT * FROM DB.GameReg WHERE hash = :user");

$stmt->bindParam(':user', $user);
$user = $_COOKIE["Logged"];

$stmt->execute();
$GameReg = $stmt->fetch(PDO::FETCH_ASSOC);

$stmt->execute();
$GameStats = $stmt->fetch(PDO::FETCH_ASSOC);

I checked this by trying to call some table members:

echo $GameReg["pass"];
echo "\n";
echo $GameStats["nick"];

It works, but I am not sure if this is the best way to do. Any comments?

Your code is fine. Really. However, it does show some lack of understanding the true benefits and power of prepared statements.
I'm also not convinced you fully understand how the PDOStatement::bindParam method works. Allow me to explain:

A prepared statement is sent to the DB server as a string, with the placeholders that will be replaced with the query values. This query string is already parsed, compiled and optimized at this point, in case it is possible, the result is already being stored in a temp-table, too.
At this point, you have a resource handle to this prepared statement, assigned to the variable $stmt. The really neat thing is that this statement can be used as much as you like, each times specifying new parameters, for example:

$stmt = $pdo->prepare('SELECT age, sex, nationality FROM people WHERE name = :name');
$names = array(
    'Frank',
    'Lisa',
    'John',
    'Jane'
);
$bind = array(
    ':name' => null
);
$personalia = array();
foreach ($names as $name)
{
    $bind[':name'] = $name;
    $stmt->execute($bind);
    $personalia[$name] = $stmt->fetch(PDO::FETCH_ASSOC);
}

As you can see, I only call prepare once, but execute 4 distinct queries. The advantage then, is simple: the query only has to be parsed, compiled and optimized once, and in some cases, the result set can be pre-fetched in such a way that you'd never need to search through the entire tbl.
Bottom line: prepared statements not only provide security against injection, it often outperforms regular queries, too.

You may have noticed how I used execute(array), and not bindParam, like your code does. Though sometimes I do use bindParam, I find it best to avoid this method when I can, in favour of execute(array), or if needs must PDOStatement::bindValue. As to why, we need to look at the method's signature:

public bool PDOStatement::bindParam ( mixed $parameter , mixed &$variable [, int $data_type = PDO::PARAM_STR [, int $length [, mixed $driver_options ]]] )

The $parameter argument is the Parameter identifier, or placeholder. In this case, the string :name.
The $variable argument is not the value that will be used in the query but if you look closely, the parameter is passed by reference: &$variable <-- that's what the & operator is for. Using it yourself is, arguably, bad practice, but no matter how you look at it, it does make your code harder to read and maintain. Imagine I did this, for example:

$stmt->bindParam(':name', $name);
while($name = array_pop($names))
{
    $stmt->execute();
}

This should work just fine, but what if $names changes, or someone re-assignes $name at some point as the while loop becomes more complex? You end up with quite error-prone code.

The advantage of this method, then, is the third parameter: $data_type. It allows you to specify the data-type you want to pass to the query. using the PDOStatement::execute(array) method, all parameters will follow the default behaviour, which means they'll be passed as strings. MySQL is capable to cast those string values to the appropriate types most of the time. As always, a cast implies some overhead, but it's highly unlikely that this will be a bottleneck and most of the time, this'll do just fine. It's an old quote, but keep in mind that premature optimization still is the root of all evil. Be that as it may, when dealing with values like NULL or booleans, it can sometimes come in handy to pass specific types.
Still, for those cases, PDOStatement::bindParam shouldn't be the default method to turn to. As you can see from the method's signature, it's powerful, but complex. Bugs and complexity go hand in hand, so let's look at another method, that does allow you to specify specific types: PDOStatement::bindValue. This method does the same thing as PDOStatement::bindParam, but does not use references (that's one less source of woes you don't have to think about), and doesn't take as many arguments:

public bool PDOStatement::bindValue ( mixed $parameter , mixed $value [, int $data_type = PDO::PARAM_STR ] )

If all you need is to specify a specific type, then this method is to be preferred: it allows you to specify all the types, but without the added risk of passing references around.

To recap: Unless you're looking to optimize your code to save every millisecond you can, or you need to pass some driver-specific options, I'd use bindValue rather than bindParam. Which, again, makes your code less error-prone and generally easier to maintain.

In short, then, and applied to your specific code, I'd recommend you write something like:

$stmt = $pdo->prepare(
    'SELECT * FROM DB.GameReg WHERE hash = :user'
);
$stmt->execute(
    array(
        ':user' => $_COOKIE['Logged']
    )
);
$gameReg = $stmt->fetch(PDO::FETCH_ASSOC);

Which gives you spacious, easy to read and maintain code.
If you want to be able to specify the type, and need nothing more, then use bindValue.

If you want to call a stored procedure, that uses an INOUT argument, then you have to resort to `PDOStatement::bindParam:

$stmt = $pdo->prepare(
    'CALL your_sp(:io)'
);
$was = $io = 'hello';
$stmt->bindParam(
    ':io',
    $io,
    PDO::PARAM_STR | PDO::PARAM_INPUT_OUTPUT,//string, inout argument
    6
);
//example:
$stmt->execute();
echo 'Argument was: ', $was, PHP_EOL, 'Now is: ', $io;

Could echo something like:

  Argument was: hello
  Now is: world


Accounting for floating point error when rounding I have to implement some business logic that will round a number (typically a double) upwards. Given that this number comes from a third-party and I don't know how they come to calculate it I'm anticipating errors in it. My proposed routine to mitigate rounding errors (like Math.ceil(1.000000000000002) = 2) is as follows:
export const roundupDiscount = (discount: number) => {
    // We have to give advantage to the customer for the discount value when rounding,
    // that is to say, we will sacrifice the penny when we can't be exact. This means
    // we have to always round the discount upwards (so we can't Math.round because
    // half the time we'll round in the wrong direction.)
    // To account for numbers like, say, 1.000000000000002 (i.e. (1 / 5 / 7) * 5 * 7))
    // then we subtract off a "small" number (scaled with the magnitude of discount)
    // which will nudge below the number we will round up to

    if (!discount) return 0 // discount shouldn't be negative

    const magnitude = Math.floor(Math.log(discount) / Math.log(10))
    const epsilon = Math.pow(10, magnitude - 14)
    return Math.ceil(discount - epsilon)
}

Given that a double has between 15-17 significant digits in it, I'm choosing epsilon, arbitrarily, such that I have a couple of trailing digits to play with.
Can anybody spot any cases where this is going to get things wrong? simplify this thing, or, given this is node.js point me towards an npm module that does this kind of thing for me?
Note: I have to get this past code-review at work so I'd much prefer if there was a good library out there that I can use instead and not even bother submitting this!

Discovered this needs rework
Testing this with small numbers shows that for all practical purposes, this function is broken. I'd like roundupDiscount(Number.EPSILON) to be 0 (which could come about from subtracting two numbers meant to be equal, but differing by an error (similar to the classic (0.1 + 0.2) - 0.3)).
However for a number like Number.EPSILON its value, including significant digits, is 0.000000000000000222044604925031, and so the computed epsilon here being 10e-28 is way too small to have any effect. This function only really works when the integer part > 0
It can be corrected in this respect by clamping the magnitude at 0 but I think I'll have a battle to convince anyone this should go into our codebase ...
Updated implementation:
export const roundupDiscount = (discount: number) => {
    // Given that discount is "sensible", if it is equal to some integer x plus a
    // small positive error component, then it must return x. Otherwise it must
    // return Math.ceil(discount)

    if (discount < 0 || discount > Number.MAX_SAFE_INTEGER) return discount
    if (discount < Number.EPSILON) return 0

    const magnitude = Math.floor(Math.max(0, Math.log(discount) / Math.log(10)))
    const epsilon = Math.pow(10, magnitude - 12) // treat last 3-4 significant digits as error
    return Math.abs(Math.ceil(discount - epsilon)) // Math.abs to prevent -0
}

Test cases:
describe('roundupDiscount', () => {
    it('rounds the item discount correctly for numbers of differing magnitude', () => {
        let epsilon = Number.EPSILON; // machine epsilon - smallest positive number which, added to 1, will produce a value different from 1

        expect(roundupDiscount(1 + epsilon)).toBe(1)
        expect(roundupDiscount(1 + (10 * epsilon))).toBe(1)
        expect(roundupDiscount(1 + (100 * epsilon))).toBe(1)
        expect(roundupDiscount(1 + (1000 * epsilon))).toBe(1)
        expect(roundupDiscount(1 + (10000 * epsilon))).toBe(2) // "error" no longer small

        // test it handles large numbers ...
        const largeNum = 1000000000;
        const largeEpsilon = largeNum * epsilon;
        expect(roundupDiscount(largeNum + largeEpsilon)).toBe(largeNum)
        expect(roundupDiscount(largeNum + (10 * largeEpsilon))).toBe(largeNum)
        expect(roundupDiscount(largeNum + (100 * largeEpsilon))).toBe(largeNum)
        expect(roundupDiscount(largeNum + (1000 * largeEpsilon))).toBe(largeNum)
        expect(roundupDiscount(largeNum + (10000 * largeEpsilon))).toBe(largeNum + 1)

        // test it handles small numbers ...
        expect(roundupDiscount(epsilon)).toBe(0)
        expect(roundupDiscount(10 * epsilon)).toBe(0)
        expect(roundupDiscount(100 * epsilon)).toBe(0)
        expect(roundupDiscount(1000 * epsilon)).toBe(0)
        expect(roundupDiscount(10000 * epsilon)).toBe(1)
    })
})


Average of positive and negative part of numpy matrix I’ve got a matrix of data and I want to take the average of the positive and negative parts of the values.
Is there a more Pythonic way of doing it?

from numpy import *

…

exposures = matrix([0]*numPaths*numExposures)
exposures.shape = numPaths, numExposures

… # fill matrix

@vectorize
def plusv(x): return max(x,0)

@vectorize
def minv(x): return min(x,0)

ee  = transpose(exposures.mean(axis=0))
epe = transpose(plusv(exposures).mean(axis=0))
ene = transpose(minv(exposures).mean(axis=0))

There are a few things which we could improve:

When importing a module, you would not usually import all symbols from that module. You can however rename the module to make it more convenient, e.g.:

import numpy as np
# now we can do np.matrix(…), np.transpose(…), etc.

Instead of initializing a matrix and then reshaping it, create it directly in the shape you need:

exposures = np.zeros((numPaths, numExposures))

When defining a function put the function body on a new line, no matter how short it might be:

def plusv(x):
    return max(x, 0)

Instead of stuffing this normalization into a custom function, you could just use the builtin clip. It takes two arguments: a minimum and maximum bound, both inclusive. So plusv(a) would be equivalent to a.clip(0, np.inf) for floats and a.clip(0, np.iinfo(np.int).max) for integers.

The numpy functions can either be called as functions or as methods. Using them as methods is usually easier to read for people used to reading left-to-right.

ee  = exposures.mean(axis=0).transpose()
epe = exposures.clip(0, np.inf).mean(axis=0).transpose()
ene = exposures.clip(-np.inf, 0).mean(axis=0).transpose()

Oh dear, we are repeating code! Well, let's just use a list comprehension/generator expression instead:

positive_exposures = exposures.clip(0, np.inf)
negative_exposures = exposures.clip(-np.inf, 0)
ee, epe, ene = (xs.mean(axis=0).transpose() for xs in
    (exposures, positive_exposures, negative_exposures))

In the above snippet, the temporary variables positive_exposure and negative_exposure are entirely optional, but they make the code more self-documenting.

Note that I use snake_case for my variables. This is the convention in Python for normal variables or method names. The lowerCamelCase scheme is not generally used. While I recommend you follow the general Python convention of words separated by underscores, the only important thing is to be consistent. Whatever your naming scheme, names like ee and epe are not acceptable. Prefer full words over abbreviations, unless the abbreviations are common enough (i for an index is ok, and everyone understands str means “string“).


Creating a symbol toolbar with Angular I have been given a requirement to create a symbol toolbar, a toolbar where each button represents a unique symbol.

This is how the toolbar would work:

A user clicks inside a text field and enters data.
The user would then click a symbol on the toolbar and it will be appended to that specific text field.
The browser will put focus back on that text field.

I have created this toolbar, but I really dislike my implementation because it feels really hacky. In my implementation, I created two functions, addSymbol and setLastFocused in my controller:

$scope.addSymbol = function (field)
{
    if (!$scope.product[$scope.lastFocusedModelKey])
    {
        $scope.product[$scope.lastFocusedModelKey] = "";
    }

    $scope.product[$scope.lastFocusedModelKey] = $scope.product[$scope.lastFocusedModelKey] + field.symbol.SymbolCharacter;

    $('#' + $scope.lastFocusedModelKey).focus();
}

$scope.setLastFocused = function (lastFocusedModelKey)
{
    $scope.lastFocusedModelKey = lastFocusedModelKey;
}

SetLastFocused is called every time a user puts focus on a text field. I pass in a string of the field name like this:

<input type="text" id="Foo" name="FooName" ng-model="product.Foo" class="form-control input-xs" ng-focus="setLastFocused('Foo')">

AddSymbol accepts the button field as a parameter, and appends the symbol to the model that is bound to the text field, and then re-focuses on the text field.

I feel like this is hacky because:

I have to use jQuery to re-focus on the text field.
I'm passing in a hardcoded string representing the field name (meaning if I change the name of the field and forget to change the hardcoded string, then this symbol feature will break for that specific text field).

This works fine now with a few text fields in which the users may want to append symbols, but now I'm learning I need to add the functionality to append symbols to text fields inside a whole Angular Ui-Grid (which exists inside a separate controller). So I am realizing I need to refactor this if I want this to work with a Ui-Grid as well. I know I need to move this code to a separate service, but I would also like to make this cleaner. I'm just not really sure how.

How can I make this code follow best Angular practices?

With custom directives you could save the focus in a $scope.focusTarget variable and the resetting of the focus can be done with a focus broadcast that have the instanceId (=focusTarget e.g. 1, 2, ...) and the new icon class so each directive called iconInput will receive the broadcast and check if the passed ID is matching. If it matches it applies the focus and sets the new icon class.

The ID is generated with a counter that is incremented at every controller run of the iconInput directive.

Please have a look at the code below (then it's more clear how it works, it's a bit difficult to describe) or in this jsfiddle.

The first focus is only working in jsfiddle if you open the fiddle in full screen mode. Not sure what's wrong.

angular.module('demoApp', [])
    .directive('iconbar', IconbarDirective)
    .directive('iconInput', IconInputDirective)
 .factory('focus', FocusFactory)
    .controller('mainController', MainController);

function IconbarDirective(focus) {
    return {
        restrict: 'EA',
        transclude: true,
        scope: {
            focusTarget: '@'
        },
        template: '

',
        link: function (scope, element) {
            var icons = element.find('i');
            console.log(icons);
            icons.addClass('iconbar-icon');
            element.find('i').bind('click', function (evt) {
                console.log('clicked', evt.target, this);
                var icon = angular.element(this),
                    iconClasses = icon[0].className.split('ng-scope'), // third is ng-scope
                 target = scope.focusTarget;

                //console.log(iconClasses);
                //console.log(iconClasses[0]);
                focus('focus' + target, iconClasses[0]);
                scope.$apply();
            });
        }
    }
}

function IconInputDirective() {
    var instanceCounter = 0;
    return {
        restrict: 'EA',
        scope: {
            target: '='
        },
        template: '
' +
            '<input type="text" placeholder="enter text..."/>' +
            '
',
        controller: function ($scope) {
            instanceCounter++;
            this.instanceId = instanceCounter;
            console.log('instance no.', instanceCounter);
            $scope.placeholder = function() {
             return $scope.icon || 'fa fa-bars icon-invisible';
            };
        },
        link: function (scope, element, attr, ctrl) {
            var inputBox = element.find('input');
            inputBox.bind('focus', function () {
                // clear other acitve classes
                element.parent().find('input').removeClass('active');
                inputBox.addClass('active');
                scope.target = ctrl.instanceId;
                scope.$apply();
            })
            /*   .bind('blur', function () {
                inputBox.removeClass('active');
            });*/

            // code for focusOn from here http://stackoverflow.com/questions/14833326/how-to-set-focus-on-input-field
            scope.$on('focusOn', function (e, name, iconClass) {
                console.log('focuson triggered', name, element[0]);
                if (name === 'focus' + ctrl.instanceId) {
                    console.log('focus now!!', element.find('input')[0]);
                    element.find('input')[0].focus();
                    inputBox.addClass('active');
                    scope.icon = iconClass;
                }
            });
        }
    }
}

function MainController($scope, focus) {
    $scope.focusTarget = 1; // last acitve input box --> sets first focus
    focus('focus' + $scope.focusTarget); // activate focus with-out class
}

function FocusFactory($rootScope, $timeout) {
    return function (name, iconClass) {
        $timeout(function () {
            $rootScope.$broadcast('focusOn', name, iconClass);
        });
    }
}
.iconbar-icon {
  cursor: pointer;
}
.icon-placeholder {
  width: 16px;
  padding: 5px;
}
[iconbar] {
  border: 1px solid;
  padding: 5px;
  margin-bottom: 5px;
}
.active {
  border: 2px solid blue;
}
i {
  padding-right: 5px;
}
.icon-invisible {
  visibility: hidden;
}
/* spacer not working */

.icon-spacer::before,
.icon-blank::before {
  width: 1em;
  content: ' ';
}
<link href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/4.4.0/css/font-awesome.css" rel="stylesheet" />

  <icon-input target="focusTarget"></icon-input>
  <icon-input target="focusTarget"></icon-input>


Defines and prints shortest word in set, sorts the rest by length, and gives average word length I need to clean up and simplify this code (I am a beginner in Python):

print ("Type in four inputs.")
word_first = raw_input("What is your first input? ")
word_second = raw_input("What is your second input? ")
word_third = raw_input("What is your third input? ")
word_fourth = raw_input("What is your fourth input? ")

list_of_words = [word_first, word_second, word_third, word_fourth]
print
print sorted(list_of_words, key=len)

def average(numbers):
    return sum(numbers)/len(numbers)

all_words = word_first, word_second, word_third, word_fourth
words_total = word_first, word_second, word_third, word_fourth.split()
length_words = [len(word) for word in words_total]

word_first_length = len(word_first)
word_second_length = len(word_second)
word_third_length = len(word_third)
word_fourth_length = len(word_fourth)
total_letters = word_first_length + word_second_length + word_third_length +
word_fourth_length
average_length = total_letters / 4
print
print ("Shortest input is: ") + (min((word for word in list_of_words if word), key=len))
print ("Average length is %.1f.") %(average_length)

Up to, and including the line list_of_words, your code looks good. But then it gets confusing.

To summarize what your program is supposed to do:

4 times: read a word from the user
put all these words in a list
sort this list by the word length
print the first element of the list (which is the shortest)
take a break (in other words, no need to sort the rest of the list, since it is already sorted)
ask your teacher about the unclear specification:

the average word length of what exactly: the whole list, or the rest?
why should the rest of the words be sorted when there is no requirement to print them afterwards?

Your definition of the average function was already good, you just didn't use it.

In Python, the above instructions could look like:

words = [first_word, second_word, third_word, fourth_word]
sorted_words = sorted(words, key=len)
sorted_lengths = [len(word) for word in sorted_words]

print('Shortest word: {}'.format(sorted_words[0]))
print('Remaining words: {}'.format(sorted_words[1:]))
print('Longest word: {}'.format(sorted_words[-1]))
print('Average word length: {}'.format(average(sorted_lengths)))
print('Average remaining word length: {}'.format(average(sorted_lengths[1:])))

Basically, you already had all the ingredients for solving the problem, you just mixed them up a bit.


Refactor to provide for multiple instances on a single page I have a script that separates the date from a datepicker into day, month, and year. It works fine when there is one form on the page but not when there is a second one (presumably due to the id being the same). How can I add a value to my id and name so that they are unique values if the script runs twice on the same page?

var now = new Date();
var date = jQuery('.form-date');
var day = ("0" + now.getDate()).slice(-2);
var month = ("0" + (now.getMonth() + 1)).slice(-2);
var today = now.getFullYear()+"-"+(month)+"-"+(day) ;

date.val(today);

jQuery('form').on('submit', function() {

    var newVal = date.val().split('-'),
        dateParts = {
            year: parseInt(newVal[0], 10),
            month: parseInt(newVal[1], 10),
            day: parseInt(newVal[2], 10)
        };

    jQuery('.anchor').append(jQuery.map(dateParts, function (index, key) {
        var name = 'date_' + key;

        return jQuery('<input>', {
            type: 'hidden',
            name: name,
            id: name,
            value: dateParts[key]
        });
    }));

});

Here is the form that isn't posting the correct date:

<form id="modal_res" action="http://www.site.com/search.php" method="get" name="disponibilita" target="_blank" class="custom">

                <?php _e('Date:', 'hostel_mama' )?><input type="date" name="date" id="modal_date" class="form-date datepicker">

                <?php _e('Nights:', 'hostel_mama' )?><input type="number" name="nights" id="modal_nights" value="2">

<?php _e('Guests:', 'hostel_mama' )?><input type="number" name="people" id="modal_people" value="1">

                <input name="ref" type="hidden" id="modal_ref" value="558" />
                <input name="langClient" type="hidden" id="modal_langClient" value="eng" />

                <?php _e('Currency:', 'hostel_mama' )?>
                    <select name="expr" id="modal_expr">
        <option value="EUR" selected="selected"><?php _e('EURO', 'hostel_mama' )?></option>
        <option value="USD"><?php _e('US Dollar', 'hostel_mama' )?></option>
        <option value="GBP"><?php _e('British Pound', 'hostel_mama' )?></option>
        </select>

                <input type="submit" name="submit" class="button radius text-center" value="Click to Book">

            </form>

Use a closure, so everything will be only inside it's scope. Additionally you can use $ now!

(function ($) {
    var now = new Date();
    var date = $('.form-date');
    var day = ("0" + now.getDate()).slice(-2);
    var month = ("0" + (now.getMonth() + 1)).slice(-2);
    var today = now.getFullYear() + "-" + (month) + "-" + (day);

    date.val(today);

    var form = $('form');

    form.on('submit', function (e) {
        var currentForm = $(this);
        var newVal = currentForm.find('.form-date').val().split('-'),

            dateParts = {
                year: parseInt(newVal[0], 10),
                month: parseInt(newVal[1], 10),
                day: parseInt(newVal[2], 10)
            };

        currentForm.find('.anchor').append($.map(dateParts, function (index, key) {
            console.log(['date', form.index(currentForm), key].join('_'));
            return $('<input>', {
                type: 'hidden',
                name: ['date', key].join('_'),
                id: ['date', form.index(currentForm), key].join('_'),
                value: dateParts[key]
            });
        }));

    });
})(jQuery);

Also change class="anchor1" to class="anchor"

Edit: Whoops, looks like I'm wrong. You have to scope your selectors as $('.anchor').append will work on all .anchor elements. I'm assuming that it's the child of form now.


Function to take a positive integer and return the next bigger number formed by the same digits I've written a solution to a Codewars challenge:

  You have to create a function that takes a positive integer number and returns the next bigger number formed by the same digits:

  12 ==> 21; 513 ==> 531; 2017 ==> 2071

I am getting the following error on Codewars: "Execution Timed Out (12000 ms)". I am a rookie, and would appreciate if anyone could give me some suggestions on optimizing my code, as this is something I know very little about.

I've made some minor tweaks but nothing that really reconstructs the solution from the ground up.

//takes a num and finds the next biggest num composed of the same digits.

function nextBigger(num) {
  let newNum = 0;
  let otherNum = 0;
  let indicator = 0;
  while (num > newNum) {
    if (indicator === 0) {
      otherNum = num;
      indicator++;
      continue;
    }

    if (
      String(num)
        .split("")
        .sort()
        .join("") ===
      String(otherNum)
        .split("")
        .sort()
        .join("")
    ) {
      if (otherNum > num) {
        newNum = otherNum;
      }
    }

    otherNum++;
  }

  return newNum;
}

I know whatever program codewars uses does not care about readability (most computer programs don't), but in the real world readability is important.

The first step to refactoring should be creating tests. This way you can easily figure out if you've broken something:

let test1 = getNextNumberOfGreaterSize(12);
let test2 = getNextNumberOfGreaterSize(513);
let test3 = getNextNumberOfGreaterSize(2017);

assertEquals(21, test1);
assertEquals(531, test2);
assertEquals(2071, test3);

function assertEquals(expected, actual) {
  if (expected !== actual) {
    throw new Error("expected: " + expected + " but was: " + actual);
  }
}

While designing tests, you may discover other edge cases such as:

What if the number given is already the highest?
What if the number doesn't have any other combinations (such as 11)?
What if a single digit number is given (1)?

You didn't provide a function name but assuming it wasn't descriptive I suggest you change it. It's tricky coming up with a name for such a function (Functions like these probably wouldn't exist in the 'real world'), but something like getNextNumberOfGreaterSize(originalNumber) with a comment describing what it does.

Set otherNumber (Note I changed the name to be more readable. Cutting off 3 letters isn't saving you, or the interpreter any time) outside of the while loop and get rid of your if statement:

let otherNum = originalNumber;
while (originalNumber > newNum) {
if (....

Whatever you are doing inside the if statement deserves a comment. Better yet, you should also put it inside a function (especially when you know you'll refactor it later!):

function numbersContainSameDigits(number1, number2) {
  return String(number1)
        .split("")
        .sort()
        .join("") ===
      String(number2)
        .split("")
        .sort()
        .join("");
}

Now that we got the important stuff out of the way, let's move onto what you're actually asking for. How can we shave a few milliseconds off each run / Why is coderank telling us the methods too slow?

Computers suck at computing Strings. However they rock at computing numbers. Let's see if we can cut down on the amount of String computing.

The way we check if the two numbers contain the same letters can be changed. First we get an array containing the characters, then compare the two arrays after sorting:

function numbersContainSameDigits(number1, number2) {
  return checkIfTwoArraysAreEqual(getArrayContainingDigits(number1), getArrayContainingDigits(number2));
}

function checkIfTwoArraysAreEqual(array1, array2) {
  return JSON.stringify(array1.sort()) === JSON.stringify(array2.sort());
}

function getArrayContainingDigits(number) {
    let arrayOfDigits = [];
    while (number >= 1) {
        arrayOfDigits.push(Math.floor(number % 10));
      number /= 10;
    }

    return arrayOfDigits;
}

Edit:

You are going through a much wider range of numbers than needed. You could instead get all available combinations of a number, and select the permutation 1 index greater than the input.


JavaScript object to JSON Since I didn't find a way to properly transform JavaScript object to JSON with PHP, I wrote a snippet.

Lets assume the following excerpt from Google Fonts JavaScript API:

WebFontConfig = {
    google: { families: [ 'Source+Sans+Pro:300,400,600,700,300italic,400italic,600italic,700italic:latin,latin-ext' ] }
};

This, given straight to PHP's json_decode will null out and JSONLint will say it's invalid.

Since I need this data in JSON after the template has been parsed (working on an exporter), I wrote this snippet (which double-quotes the hell out of this one):

// gather WebFontConfig arrays
$webfonts = preg_match_all('/(WebFontConfig\s*?=\s*?)\{(.+?)(\};)/s', $scriptData, $fontdata, PREG_SET_ORDER);

// kids, dont do this at home
foreach ($fontdata as $founddata)
{
    $original = $founddata[0];

    $WFCVariable = $founddata[1];

    // leave outer braces only
    $usable = str_replace($WFCVariable, '', $original);
    $usable = trim($usable, ';');

    // transform keys into double-quoted keys
    $usable = preg_replace('/(\w+)(:\s*?)(\{|\[)/im', '"$1"$2$3', $usable);

    // prepare to transform array wrapping single quotes to double quotes
    $lookups = array(
        '/(\[)(\s*?)(\')/',
        '/(\')(\s*?)(\])/'
    );

    $replace = array(
        '$1$2"',
        '"$2$3'
    );

    $usable = preg_replace($lookups, $replace, $usable);

    // decode
    $jsoned = json_decode($usable);

    // and check
    var_dump($jsoned);
}

$scriptData essentially is the above JavaScript object trim-extracted out of my template through DOMDocument .. DOMNode->nodeValue.

There is no problem with the snippet, it converts my data into PHP array without problems.

My concern, though, is, whether this is the most optimal way of doing it?

As this works for you and as there is no javascript interpreter in PHP unless you install it (there is a V8 PHP extension IIRC), I would say you did this right so far.

Which else criteria could there be than your own ones? You should probably write unit and acceptance tests for it. And remove the var_dump debug cruft out of it. But by the way you do this on it's own, it does not look too far off.


Basic JavaScript OOP model object I'm relatively new to JavaScript OOP and would like to know the best practice when doing a simple object such as a Model.

Here's a piece of code representing class named Child:

What I'm asking for is:

The usage of this (and the weak reference I'm doing which I name self)
The global shape of the code (methods in prototype etc)
Declaring a class as a function, or with JSON semantics

var Child = function( json ) {
    var self = this;
    // Properties
    this.id = 0;
    this.firstname = "";
    this.moneyCurrent = 0;
    this.moneyDue = 0;
    this.missionsPlay = 0;
    this.missionsWait = 0;
    this.missionsStop = 0;

    this.missions = [];

    this.initialize( json );
}

Child.prototype = {
    initialize: function( json ) {
        var self = this;
        self.id = json.id_child;
        self.firstname = json.firstname;
        self.moneyCurrent = parseFloat(json.already_paid) + parseFloat(json.to_pay) - parseFloat(json.spent_money);
        self.moneyDue = parseFloat(json.to_pay);
        self.missionsPlay = json.nb_missions.in_progress;
        self.missionsWait = json.nb_missions.waiting;
        self.missionsStop = json.nb_missions.completed;
    },

    getMissions: function( type, callback ) {
        var self = this;
        session.requestServer( "parent_getChildMissionList", {
            sid: session.sid,
            id_child: self.id,
            status: type ? type : "",
            strict_status: false,
            page: 0
        }, function( data ){
            var missions = [];
            $(data.missionList).each(function(){
                self.missions.push(new Mission(this));
                missions.push(self.missions[self.missions.length-1]);
            });
            callback( missions );
        });
    },
};

Classic way to declare a class:

var obj = function() {
    var attribut = "foo";
    this.metho = function(parameter1, parametre2) {
        alert("parameters: " + parameter1 + ", " + parameter2);
    }
}

JSON way to declare a class:

var obj = {
    attribut: "foo",
    method: function(parameter1, parameter2) {
        alert("parameters: " + parameter1 + ", " + parameter2);
    }
}

For your questions:

The usage of this : In my mind, unless you are using closures, do not use self = this, so only keep it in getMissions
Functions in prototype <- Good stuff
Function <> JSON, is an odd question. You want to use constructors, and those are functions.

Other than that,

You are using lowerCamelCase, that is good. Though firstname -> firstName
initialize: function( json ) does not really get a JSON string, instead it gets an object, so I would not name that parameter json
The function to deal with returned missions seems a bit clumsy, I would either assign each new mission to a variable and push it to both arrays:

function( data ){
    var missions = [], mission;
    $(data.missionList).each(function(){
        mission = new Mission(this)
        self.missions.push( mission );
        missions.push( mission );
    });
    callback( missions );
}

or, I would only push to missions and then concatenate missions into self.missions

function( data ){
    var missions = [];
    $(data.missionList).each(function(){
        missions.push(new Mission(this));
    });
    self.missions = self.missions.concat( missions );
    callback( missions );
}

All in all, nice code, very maintainable and self explanatory.


"Dungeon Game" solution The demons had captured the princess (P) and imprisoned her in the
  bottom-right corner of a dungeon. The dungeon consists of M x N rooms
  laid out in a 2D grid. Our valiant knight (K) was initially positioned
  in the top-left room and must fight his way through the dungeon to
  rescue the princess.

  The knight has an initial health point represented by a positive
  integer. If at any point his health point drops to 0 or below, he dies
  immediately.

  Some of the rooms are guarded by demons, so the knight loses health
  (negative integers) upon entering these rooms; other rooms are either
  empty (0's) or contain magic orbs that increase the knight's health
  (positive integers).

  In order to reach the princess as quickly as possible, the knight
  decides to move only rightward or downward in each step.

  Write a function to determine the knight's minimum initial health so
  that he is able to rescue the princess.

  For example, given the dungeon below, the initial health of the knight
  must be at least 7 if he follows the optimal path RIGHT-> RIGHT ->
  DOWN -> DOWN.

  Notes:

  The knight's health has no upper bound.
  Any room can contain threats
  or power-ups, even the first room the knight enters and the
  bottom-right room where the princess is imprisoned.

I was tackling this problem and I ended up with code that is functional, but times out with really large input (meaning it's poorly optimized). I'm a beginner in programming and working out a functional algorithm was extremely hard for me. Now that I have to optimize it, I don't even know how to start.

Is there a way to optimize this code that doesn't change the fundamental underlying logic or is the approach I took doomed to be inefficient? If there is a way to optimize this, could you please provide some examples?

class Solution {
public:
int calculateMinimumHP(vector< vector<int> > &dungeon) {
    return 1 - actualFunction(dungeon, 0, 0, 0);
}
int actualFunction(vector< vector<int> > &dungeon, int roomCoordX, int roomCoordY, int shield){
    if(dungeon.size() == roomCoordX + 1 && dungeon[0].size() == roomCoordY + 1){
        if(shield + dungeon[roomCoordX][roomCoordY] < 0)
            return (dungeon[roomCoordX][roomCoordY] + shield);
        else
            return (0);
    }
    if(dungeon.size() == roomCoordX + 1){
         if(shield + dungeon[roomCoordX][roomCoordY] < 0)
            return (dungeon[roomCoordX][roomCoordY] + shield) + actualFunction(dungeon, roomCoordX, roomCoordY + 1, 0);
        else
            return 0 +  actualFunction(dungeon, roomCoordX, roomCoordY + 1, dungeon[roomCoordX][roomCoordY] + shield);
    }
    if(dungeon[0].size() == roomCoordY + 1){
         if(shield + dungeon[roomCoordX][roomCoordY] < 0)
            return (dungeon[roomCoordX][roomCoordY] + shield) + actualFunction(dungeon, roomCoordX + 1, roomCoordY, 0);
        else
            return 0 +  actualFunction(dungeon, roomCoordX + 1, roomCoordY, dungeon[roomCoordX][roomCoordY] + shield);
    }
        int temp1;
        int temp2;
    if(shield + dungeon[roomCoordX][roomCoordY] < 0){
        temp1 = actualFunction(dungeon, roomCoordX + 1, roomCoordY, 0);
        temp2 = actualFunction(dungeon, roomCoordX, roomCoordY + 1, 0);
        return(shield + dungeon[roomCoordX][roomCoordY]) + ((temp1 > temp2) ? temp1 : temp2);
    }
    else{
        temp1 = actualFunction(dungeon, roomCoordX + 1, roomCoordY, dungeon[roomCoordX][roomCoordY] + shield);
        temp2 = actualFunction(dungeon, roomCoordX, roomCoordY + 1, dungeon[roomCoordX][roomCoordY] + shield);
        return 0 + ((temp1 > temp2) ? temp1 : temp2);
    }
}
};

Dynamic programming solution

This problem can be solved with a dynamic programming solution by starting at the end cell and working upwards and leftwards to the starting cell.  The basic premise is:

You need to construct a new matrix of values which holds the minimum health required to have before reaching a square.  For example, if m[5][3] is 10, it means that you need 10 health to reach the finish, if you were at (4,3) or (5,2) and were about to move to (5,3).
Start with the end cell, and set its value to max(1, 1 - dungeon[lastcell]).
Then working your way upwards and leftwards, you can set each cell like this:

downHealth  = max(1, m[downNeighbor ] - d[thiscell]);
rightHealth = max(1, m[rightNeighbor] - d[thiscell]);
m[thiscell] = min(downHealth, rightHealth);

Note that some cells do not have both neighbors, so you just set the value to the health computed for the neighbor that exists.


Detecting headers status takes too long I have a table in my database with 35000 unique URLs. I use them to create my XML sitemaps for the site. I have setup crontab jobs to create automatically new URLs, delete non existing URLs and determine if a URL is valid.

For the last part I use the following function:

function get_url_status($url)
{
    $httpCode = 0;
    $headers = get_headers($url);
    $http_header_info = $headers[0];
    $httpCode = substr($http_header_info, 9, 3);

    if ($httpCode=='200')
    {
        // check in page for noindex and dissable that url
        $metas = get_meta_tags($url);
        if ( isset($metas['robots']) && strpos( strtolower($metas['robots']),  'noindex')!==false  )
        {
            $httpCode = 410;
        }
    }

return $httpCode;
}

This will return to me the HTTP status code - 200 for legal URLs that will be included in sitemap, 400 for not found, 410 for URLs with noindex in them, 301 for redirections, etc.

In my XML sitemaps I include only URLs with status code 200.

My problem is that this function takes around 40 minutes to check the URLs in database. Is there a way to speed things up?

So you're fetching 70k pages.  You could half that if you only fetch the page once and then do what get_headers and get_meta_tags do manually.  That will presumably take now 20 minutes.

Then you can try doing this in parallel, so run two instances of the script with one half of the URLs each.


Extracting RAR files in a directory I just started programming, and something is telling me that this working function looks to big for what it does. Can you give a better version of the code so I can understand how to code faster, shorter code ?

import os, rarfile

xpath= "/root/tree/def/"

dpath="/root/tree/down"

def unrar():
        for rf in os.listdir(dpath):
            f = os.path.join(dpath,rf)
            o = rarfile.RarFile(f)
            for u in o.infolist():
             print (u.filename, u.file_size)
             o.extractall(xpath)

You should use more descriptive names than f, o, u and rf. It makes your code harder to read, even if it's all quite small and contained.

def unrar():
    for rar in os.listdir(dpath):
        filepath = os.path.join(dpath, rar)
        opened_rar = rarfile.RarFile(filepath)
        for f in opened_rar.infolist():
            print (f.filename, f.file_size)
        opened_rar.extractall(xpath)

Note that I still use f for the files you iterate over in the opened_rar, because single letter variables are occasionally ok. Especially in limited contexts, where they're commonly used (f will often refer to a file when iterating). But using multiple ones together can easily be confusing and less readable.

Your function should also take xpath and dpath as parameters, rather than using constants. This makes it easier to reuse the function and have it run on different files or in different places, rather than having to manually change the constant values. It's also easy to change:

def unrar(dpath, xpath):

And then just call it with:

unrar(dpath, xpath)

Also Python has a great tool called context managers. These will basically ensure that your file is always closed no matter what happens. Every time you open a file with Python it needs to be closed again. And you actually never closed yours, so it's better practice to use a context manager, like this:

def unrar(dpath, xpath):
    for rar in os.listdir(dpath):
        filepath = os.path.join(dpath, rar)
        with rarfile.RarFile(filepath) as opened_rar:
            for f in opened_rar.infolist():
                print (f.filename, f.file_size)
            opened_rar.extractall(xpath)

Everything inside the with block is now what happens with the opened_rar file. Once you exit that block, the file is always closed. Crucially, this holds true even if an error causes your script to end prematurely. I haven't worked with rar files, but I can tell you that when using zip files you can lose your data by having errors interrupt your script. A context manager prevents this data loss, making it safer.


Generate all possible numbers which can be representated as the sum of 2 cubes below N I have written a program that generates all possible numbers which can be represented by adding 2 cubes which have value less than N

For example: when \$N = 10, 1^3 + 2^3 = 9\$

import java.util.ArrayList;
import java.util.List;
import java.util.Scanner;

public class AllNumberLessThanNasCubes {

    public static void main(String[] args) {
        // TODO Auto-generated method stub
        System.out.println("enter an integer n");
        int n = new Scanner(System.in).nextInt();

        System.out.println("all possible numbers are as follows ");
        generateAllPossibleNumbers(n);

    }

    /**
     * This function will generate all possible numbers less the {@link n} which can
     * be represented as the sum of 2 cubes
     * For eg.when N=10 ,  1^3 + 2^3 = 9
     * @param n
     */
    public static  void generateAllPossibleNumbers(int n) {
            // TODO Auto-generated method stub

    List<Long> cubes = new ArrayList<>((int) Math.cbrt(n));
    List<Long> allSum = new ArrayList<>();
    for (int i=1; Math.pow(i, 3)< n ;++i){
        cubes.add((long) Math.pow(i, 3));
    }
    long sum=0 , tempSum=0;
    int cubesSize= cubes.size();
    System.out.println(cubes);
    for (int i=0; i < cubesSize ;++i){
        sum = cubes.get(i);

        for (int j=i+1; j< cubesSize ; ++j){
            tempSum = sum + cubes.get(j) ;
            //System.out.println(tempSum);
            if (tempSum >= n){
                break;
            }else{
                allSum.add(tempSum);
            }
        }
    }
    System.out.println(allSum);
}
}

Is this the correct way to solve the problem? Is there an efficient way to solve it?

Except for error checking while taking input, what else can be improved?

Except for error checking

Error checking would be nice, and can easily be done, so why not add it to the top of your method:

    if (n < 0) {
        throw new IllegalArgumentException("number must not be negative");
    }

  what else can be improved?

your formatting, especially your spacing, which seems very random, and thus makes the code hard to read. Just use an IDE to fix this for you.
define variables in as small a scope as possible. For example, sum can be declared inside the loop as long sum = cubes.get(i);.
don't print in your method, but return the values instead. This makes the next point a lot easier, and makes your method reusable in general.
write unit tests to verify the functionality of your code, and to make it a lot easier to make changes without being afraid of breaking the code.


Conway's Game of Life F# I am a C# developer learning F#. Would appreciate any suggestions on how this code can be improved. Main points I am interested in:

Functional style of the code, absense of imperative style(C#) artifacts
F# collection data types and functions usage
General code(not algorithm) efficiency
Code style, naming etc.

let life board =
    let flatten = Seq.cast<int> >> Seq.toArray

    let neighboursCount (board: int[,]) x y =
        board.[y..y+2, x..x+2]
        |> flatten
        |> Array.mapi (fun i el -> (i <> 4, el))
        |> Array.filter fst |> Array.map snd
        |> Array.filter ((=) 1) |> Array.length

    let expandBoard board =
        let N = Array2D.length1 board
        array2D [|
            yield                        [| yield board.[N - 1, N - 1]; yield! board.[N - 1, *]; yield board.[N - 1, 0] |]
            for i in [0..N - 1] do yield [| yield board.[i,     N - 1]; yield! board.[i,     *]; yield board.[i,     0] |]
            yield                        [| yield board.[0,     N - 1]; yield! board.[0,     *]; yield board.[N - 1, 0] |]
        |]

    let getNextBoard board =
        let eBoard = expandBoard board
        board |> Array2D.mapi (fun i j elem ->
            match neighboursCount eBoard j i with
            | 2 -> elem
            | 3 -> 1
            | _ -> 0)

    let isGameOver board prevBoards =
        board |> flatten |> Array.forall ((=) 0) || List.contains board prevBoards

    let rec lifeRec board prevBoards =
        printfn "%A \n" board
        if isGameOver board prevBoards
        then ()
        else getNextBoard board |> lifeRec <| board::prevBoards

    lifeRec board []

let N = 5
let rnd = System.Random()
Array2D.init N N (fun _ _ -> rnd.Next 2) |> life

This is a most elegant solution. I have two small improvements:

let neighboursCount (board: int[,]) x y =
    board.[y..y+2, x..x+2]
    |> flatten
    |> Array.indexed
    |> Array.sumBy (fun (i, n) -> if i <> 4 then n else 0)

let expandBoard board =
    let N = (Array2D.length1 board) - 1
    array2D [|
        yield                    [| yield board.[N, N]; yield! board.[N, *]; yield board.[N, 0] |]
        for i in [0..N] do yield [| yield board.[i, N]; yield! board.[i, *]; yield board.[i, 0] |]
        yield                    [| yield board.[0, N]; yield! board.[0, *]; yield board.[N, 0] |]
    |]

Here is N set to (Array2D.length1 board) - 1 because that's the value you're using through out the function.


Improving some plugin user-input code I made this nice bit of code that I can insert into any of my projects requiring user input. It seems quite long for what it does and I would love some insight as to how I can do this more efficiently i.e. fewer lines of code.

I love to make my code elegant, but I don't see how to do this any better.

String x = (JOptionPane.showInputDialog(". Each parameter should be separated with a ',' and without spaces. (eg. 1,2,3)") + ",");
int w = 0, a = 0, y = -1;
for(int i = 0; i < x.length(); i++)
{
    if(x.charAt(i) == ',')
    {
        w++;
    }
}
Double[] z = new Double[w];
for(int i = 0; i < x.length(); i++)
{
    if(x.charAt(i) == ',')
    {
        z[a] = Double.parseDouble(x.substring((y + 1), i));
        y = i;
        a++;
    }
}

There is absolutely no need to put parentheses around the first statement (small improvement, but it improves readability). "parameter" is also misspelled (improving readability but in a different way).

String x = JOptionPane.showInputDialog(". Each parameter should be separated with a ',' and without spaces. (eg. 1,2,3)") + ",";

There is no need to use Double[] when you can use double[]. The difference is, simply put, that a Double can be null while a double can not.
In the rest of your code it seems like you are reading all the doubles from the String (separated by commas), which can be done much easier like this:

String[] splitted = x.split(",");
double[] result = new double[splitted.length];
for (int i = 0; i < splitted.length; i++) {
    result[i] = Double.parseDouble(splitted[i]);
}

The key here is to use the split-method on the String, which does most of the work for you. For example, it splits the string "0.37,42,21.4" into a string array with the three elements "0.37", "42" and "21.4". The rest of the code loops through that array and converts from String to double.


Reverse Polish Notation Calculator Working through Learn You a Haskell, I made a Reverse Polish Notation calculator.

Please critique it.

-- LYAH uses `(Num a) => String -> a` as the signature
solveRPN :: String -> Double
solveRPN xs = head $ foldl (\acc x -> foldingFunction acc x) [] $ words xs

foldingFunction :: [Double] -> String -> [Double]
foldingFunction acc elem
 | isOp elem  = calculate (take 2 acc) elem : (drop 2 acc)
 | otherwise  = (read elem :: Double) : acc

calculate :: [Double] -> String -> Double
calculate (y:x:_) op
 | op == "+" =  x + y
 | op == "-" =  x - y
 | op == "*" =  x * y
 | op == "/" =  x / y

isOp :: String -> Bool
isOp x = x `elem` ["+", "-", "*", "/"]

Using foldl is the right idea, I think.  (\acc x -> foldingFunction acc x) is a useless lambda, which could just be written as foldingFunction.  The fact that it's a folding function is evident from the fact that you passed it to foldl; you could name it something more useful, such as manipulateStack.

Consider breaking up solveRPN.  For example, it might be useful to inspect the end state of the whole stack rather than just taking the top element.  Also, it's possible that the input might already be split into words (from the command line via getArgs, for example).

The beauty and simplicity of RPN comes from the fact that operators can manipulate the stack directly.  Instead, you've implemented a calculate function that performs binary operations only.  That results in two problems:

In the case of RPN stack underflow, you'll get a "Non-exhaustive patterns in function calculate" error.
You can't support unary operators (such as "sqrt"), nor can you support nullary operators (such as a "pi" operator that pushes 3.141592653589793 onto the stack).

import System.Environment

manipulateStack :: [Double] -> String -> [Double]
manipulateStack stack s
  | s == "+"     = (next + top) : stack''
  | s == "-"     = (next - top) : stack''
  | s == "*"     = (next * top) : stack''
  | s == "/"     = (next / top) : stack''
  | s == "^"     = (next ** top) : stack''
  | s == "sqrt"  = (sqrt top) : stack'
  | s == "sin"   = (sin top) : stack'
  | s == "cos"   = (cos top) : stack'
  | s == "tan"   = (tan top) : stack'
  | s == "pi"    = pi : stack
  | s == "e"     = (exp 1) : stack
  | otherwise    = (read s::Double):stack
  where top = head stack
        stack' = tail stack
        next = head stack'
        stack'' = tail stack'

rpn :: ([Double] -> [String] -> [Double])
rpn = foldl manipulateStack

solveRPN :: String -> Double
solveRPN s = head $ rpn [] $ words s

main = do
  args <- getArgs
  putStrLn $ show $ head $ rpn [] args


Building shape based on user input The purpose of the below code is to build a shape based on the user input, specifically:

Prompt the user to enter a shape - either a triangle or a square
Determine which "texture" to use for the shape - currently set at #'s for squares, *'s for triangles
Build the shape based on the selected texture

Sample output for "Square":

#####
#####
#####

Sample output for "Triangle":

*
**
***
****
*****

How can I completely redo the structure and flow of the program to better follow convention, especially in regards to the way the classes are structured? I still want use multiple classes however, as the main thing i'm trying to do here is work out how to use classes properly. Any other general refactoring welcome too.

Also sorry for the tab spacing. It's set to 2, but is a lot more when I paste it here.

# Shape Builder v0.1

class BuildShape
  def buildShapeCheck
        if @text == "square"
            self.buildSquare
        else
            self.buildTriangle
        end
    end

    def buildSquare
        (1..3).each do
            puts @texture*5
        end
    end

    def buildTriangle
        j = 1
        (1..5).each do
            puts @texture*j
            j += 1
        end
    end
end

class Textures < BuildShape
    def shapeCheck
        @texture = ""
        if @text == "square"
            @texture = "#"
            buildShapeCheck
        else
            @texture = "*"
            buildShapeCheck
        end
    end
end

class UserInput < Textures
    def initialize(text)
        @text = text
    end

    def printInput
        print "you entered: "
        puts @text
        self.testIfValid
    end

    def testIfValid
        if @text == "square" || @text == "triangle"
            puts "#{@text} is a valid shape."
            shapeCheck
        else
            puts "#{@text} is not a valid shape, try again"
            newUserInput = UserInput.new(gets.chomp.downcase)
            newUserInput.testIfValid
        end
    end
end

puts "Enter either \"triangle\" or \"square\""
user_input = gets.chomp
newUserInput = UserInput.new(user_input.downcase)

newUserInput.printInput

Ok, so... there's a lot to talk about here. I've simply gone through it line by line, and added comments. So prepare for a pretty long review.

As for the indentation, the reason it's different when you paste it here is probably that you're still using tabs. It should be soft tabs, i.e. just 2 space characters. Not 1 tab character set to be 2 characters wide, but actual spaces.

Anyway, long review incoming.

# BuildShape is a poor name for a class. Classes should generally be nouns,
# but "Build shape" is an imperative. It's true that the class does build a
# shape, but the class in itself is not "the act of building".
class BuildShape

  # This class has no "initialize" method, but because its parent
  # class (which is Object, when nothing else is specified) has one,
  # I can still call "BuildShape.new.buildSquare" (for instance). But
  # if I do, things will be weird, because @text and @texture haven't
  # been defined. And without an initialize method (or attribute
  # setters), there's no way to define them.

  # Don't duplicate the name of the class in the names of its methods.
  # There's simply no need to do that; you don't need to name a file
  # on your computer after the folder it's in.
  # Also, Ruby uses underscored method names, so if anything, it should
  # be called "build_shape_check"
  def buildShapeCheck
    if @text == "square"
      self.buildSquare   # no need for "self." here
    else
      self.buildTriangle # or here
    end
  end

  # This produces a 5x3 rectangle - not a square!
  # It looks like a square because of the font you use to
  # show it, but that's coincidental and may not hold true
  # in all situations. So either make the method produce a
  # real NxN square, or make it very clear in the comments
  # that "by square, I do not actually mean a square" (i.e.
  # admit the deceit and its reasons)
  def buildSquare
    (1..3).each do # you could also just use "3.times do"
      puts @texture*5
    end
  end

  # again: Underscore naming style
  def buildTriangle
    j = 1  # This is completely unnecessary...
    (1..5).each do # ...if you just add |j| as a block argument
      puts @texture*j
      j += 1 # And then this can go too
    end
  end
end

# When I said "classes should be nouns" I meant *singular* nouns.
# The String class is not called "Strings" for instance.
# If anything, this should be called "Texture"
# But more importantly, this class doesn't make sense. You use
# class inheritance because you have a generic class and want to
# make a specialization of that class. E.g. you have a class called
# "Vehicle", and you make a class called "Car" or "Boat". Those are
# specializations of a common thing: a vehicle.
# But here, you seem to be making a new class for a completely
# different reason - a reason I can't quite figure out.
class Textures < BuildShape

  # Again, no initialize method...

  # Again you're kinda-sorta repeating the name of the class
  # in the method name, because - due to inheritance - this class
  # is a BuildShape. It'd be enough to call the method "check".
  # ... buuut, this method isn't checking anything, so it'd
  # still be a terrible name.
  def shapeCheck
    # This is unnecessary; @texture *will* be set to either "#"
    # or to "*" - there's no reason to set it to anything else
    # beforehand.
    @texture = ""

    # Another thing is that this method is pointless in many ways.
    # You can't set the @texture variable anywhere else, which means
    # @texture is 100% dependent on what @text is. Which, in turn,
    # means you can just figure out the right texture when you're
    # drawing the shape. You don't even need the @texture variable;
    # the methods that print the shape are already different depending
    # on whether you want a triangle or a square; they can just print
    # the correct character themselves.
    if @text == "square"
      @texture = "#"

      # Don't call this here, *and* in the else-block: Call it *after*
      # the if-else instead, since you want to call independent of
      # what the @text is
      buildShapeCheck
    else
      @texture = "*"
      buildShapeCheck # Again: Delete this
    end
  end
end

# Ok, this is a better name of a class. UserInput sounds like a
# class name. But of course, your inheritance chain is saying that
# "UserInput is a kind of Textures, which is a kind of BuildShape"
# Do you see how that doesn't make sense?
# It also highlights why the other classes are problematic: You
# can't use them on their own. Only after 2 levels of inheritance
# do you get the class that actually solves the task. The preceding
# classes don't solve anything by themselves, you're simply
# treating them as stepping stones to the class you actually want.
# They're dependent on you extending them, which, to reuse the
# analogy from earlier, is like saying that vehicles don't work
# until someone builds a boat; it's backwards.
class UserInput < Textures

  # Yay! An initialize method!
  def initialize(text)
    @text = text
  end

  # Ok, it prints the input - but it also checks the input
  # and that's not at all obvious. Your methods should only
  # do what it says on the tin.
  def printInput
    print "you entered: "
    puts @text
    self.testIfValid
  end

  # Uh, no. The name of this method is again pretty wrong.
  # Does it mean "it only does its test, if things are valid"?
  # The Ruby-like name for this thing would be "valid?"
  # And again, the method isn't just testing; it's also
  # the method that actually produces the output! AND it
  # completely duplicating code from *outside* the class.
  # What's worse, it creates a instance of itself, within
  # itself, and... honestly, I don't know how to best explain
  # how weird this is. Sorry, but that the case.
  def testIfValid
    if @text == "square" || @text == "triangle"
      puts "#{@text} is a valid shape."
      shapeCheck
    else
      puts "#{@text} is not a valid shape, try again"
      newUserInput = UserInput.new(gets.chomp.downcase)
      newUserInput.testIfValid
    end
  end
end

# If you just used single quotes, you wouldn't have to escape
# the inner double quotes...
puts "Enter either \"triangle\" or \"square\""

# Terrible naming: "user_input" is *not* a UserInput object;
# that's "newUserInput"! Which, again, shouldn't be CamelCased
# and just plain shouldn't be called that.
user_input = gets.chomp
newUserInput = UserInput.new(user_input.downcase)

# So... when you say printInput, you actually mean
# "do everything and print the *output*"?
newUserInput.printInput

Here's a different way of doing things:

def print_triangle(size = 5)
  size.times { |count| puts "*" * count }
end

def print_square(size = 5)
  size.times { puts "#" * size }
end

while true # loop until further notice
  puts 'Please type either "triangle" or "square"'
  type = gets.chomp.downcase
  if %w(triangle square).include?(type) # did the user enter something valid?
    send("print_#{type}")               # call the proper method
    break                               # stop looping
  else
    puts "Sorry, that didn't make sense. Try again."
  end
end

And done. I know this doesn't use any class hierarchy or anything like that, but that's also to illustrate that it isn't always necessary. Just because you can doesn't mean you should.


n number of x on the y Everyone knows "99 bottles of beer on the wall".

Mat's Mug made a comment about mugs on the wall in The 2nd Monitor and I realized I never wrote a "beer on the wall" program. But that seemed way too easy, so I made it a "n number of x on the y" instead. It also seemed like a good excuse to use templates in Python, which I hadn't done before.

import sys

n = int(sys.argv[1])
x = sys.argv[2]
y = sys.argv[3]

template = '''\
%i %s on the %s
%i %s
Take one down, pass it around
%i %s on the %s
'''

for n in range(n, 0, -1):
    print template % (n, x, y, n, x, n-1, x, y)

Usage:

python script.py 99 "mugs of beer" wall

For obvious reasons there's no way (that I know of) to remove multiples of x if n = 1. Sure, it's not hard to turn mugs of beer into mug of beer, but it's generic. One can input anything and I can't catch all methods of pluralization.

I prefer readability over 'clever code' and my naming is probably not too good.

Your code is simple and just works. For that reason, you can be happy and there is not much comment to give. However, being very picky just for the sake of learning, one could say various things.

print statements

You are using print without parenthesis. This is the one and only reason why your code wouldn't work on Python 3. Just to be future (well, actually Python 3 is not the future, it is the present) proof, you might as well add the 2 parenthesis.

String formatting

Instead of using the % format, you could use the format method. You'll find more information online. Also using named place holder makes things easier to read and avoid duplicated values.

You'd get something like :

template = '''\
{idx} {x} on the {y}
{idx} {x}
Take one down, pass it around
{prev} {x} on the {y}
'''

for n in range(n, 0, -1):
    print(template.format(idx=n, prev=n-1, x=x, y=y))

Error handling

You should probably check the number of arguments provided and tell the user if anything is wrong.

Variable names

Even though n is acceptable, x and y do not convey much information, a better name can probably be found.


Reusable Unit Of Work Interface / Factory Given my IUnitOfWork interface

using System;

public interface IUnitOfWork : IDisposable
{
    void Commit();
}

I then create an abstract factory interface called IUnitOfWorkFactory

using System.Transactions;

public interface IUnitOfWorkFactory
{
    IUnitOfWork GetUnitOfWork(IsolationLevel isolationLevel);
}

I then create a default implementation of my IUnitOfWork called TransactionScopeUnitOfWork

using System;
using System.Transactions;

public class TransactionScopeUnitOfWork : IUnitOfWork
{
    private bool disposed = false;

    private readonly TransactionScope transactionScope;

    public TransactionScopeUnitOfWork(IsolationLevel isolationLevel)
    {
        this.transactionScope = new TransactionScope(
                TransactionScopeOption.Required,
                new TransactionOptions
                {
                    IsolationLevel = isolationLevel,
                    Timeout = TransactionManager.MaximumTimeout
                });
    }

    public void Dispose()
    {
        this.Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (!disposed)
        {
            if (disposing)
            {
                this.transactionScope.Dispose();
            }

            disposed = true;
        }
    }

    public void Commit()
    {
        this.transactionScope.Complete();
    }
}

I then create the factory to return that implementation called TransactionScopeUnitOfWorkFactory

using System.Transactions;

public class TransactionScopeUnitOfWorkFactory : IUnitOfWorkFactory
{
    public IUnitOfWork GetUnitOfWork(IsolationLevel isolationLevel)
    {
        return new TransactionScopeUnitOfWork(isolationLevel);
    }
}

The reason for creating the factory is to allow DI (Dependency Injection) frameworks to use different unit of work implementations depending on configuration.

If TransactionScopeUnitOfWorkFactory was mapped to IUnitOfWorkFactory in a DI container, some sample code for using it in an application could be:

public class Test
{
    private readonly IUnitOfWorkFactory unitOfWorkFactory;

    private readonly IRepository testRepository;

    public Test(
        IRepository testRepository,
        IUnitOfWorkFactory unitOfWorkFactory)
    {
         this.testRepository = testRepository;
         this.unitOfWorkFactory = unitOfWorkFactory;

         using (IUnitOfWork unitOfWork = this.unitOfWorkFactory.GetUnitOfWork(IsolationLevel.Serializable))
        {
            this.testRepository.Delete(1); // Some valid CRUD
            unitOfWork.Commit();
        }
    }

I am asking if this seems like a good implementation.  Am I missing anything?
I want an IUnitOfWork interface that I can use across applications and not worry about maintaining later on.  Any opinions?

Leaky Abstractions

public interface IUnitOfWork : IDisposable
{
    void Commit();
}

I like that there's only a Commit method here - it makes the IUnitOfWork interface very well segregated/focused. However the interface specifying that all implementations must also implement IDisposable, is a leaky abstraction - you have a specific implementation in mind, and that implementation is leaking into the abstraction: what if I wanted a mock-up implementation that doesn't need to be disposed?

public interface IUnitOfWorkFactory
{
    IUnitOfWork GetUnitOfWork(IsolationLevel isolationLevel);
}

GetUnitOfWork makes it sound like the "factory" is hiding a Singleton or something (the name is reminiscent of "GetInstance") - I like my abstract factories when they expose a single parameterless Create method, and intake their dependencies from the constructor; IsolationLevel is also an implementation detail that's leaking into an abstraction - one specific implementation requires an "isolation level", and that implementation detail is leaked into the abstraction.
I would move : IDisposable to TransactionScopeUnitOfWork, because that specific implementation must implement IDisposable.
Then I'd change the abstract factory interface to look something like this:
public interface IUnitOfWorkFactory
{
    IUnitOfWork Create();
}

And the specific implementation that creates a TransactionScopeUnitOfWork would be responsible for specifying an IsolationLevel:
public class TransactionScopeUnitOfWorkFactory : IUnitOfWorkFactory
{
    private readonly IsolationLevel _isolationLevel;

    public TransactionScopeUnitOfWorkFactory(IsolationLevel isolationLevel)
    {
        _isolationLevel = isolationLevel;
    }

    public IUnitOfWork Create()
    {
        return new TransactionScopeUnitOfWork(_isolationLevel);
    }
}

With IoC Containers such as Ninject, you can easily specify IsolationLevel.Serializable or IsolationLevel.ReadCommitted as needed, depending on whether the factory dependency is being injected into a Smurf or into a FooBar's constructor.
That said I've noticed I seldom need to actually implement an abstract factory with Ninject: you can easily configure DI to inject the result of an anonymous method whenever a class has an IUnitOfWork dependency to be constructor-injected: you just do the binding with .ToFactory and let Ninject do the hard work.
The client code shouldn't be bothered with configuring a transaction isolation level if all it needs to care for is that it's depending on an IUnitOfWork that provides a Commit method; if the client code needs to know about an isolation level, then it knows more than it should about the implementation behind the interface it's presented.


Basic Java library to hold books For my education, I have written this basic library in Java. Maybe someone can see something that I can do better, as I'm just beginning with Java and I'm hungry to learn more.

public class Library {
    int Capacity = 10;
    int Volume = 0;
    Book[] storage = new Book[10];

    public Library() {
        System.out.println("Hello, I am a library, which can store up to 10 books!");
        this.storage = new Book[10];
    }

    public void add(Book book) {
        if (this.Volume < this.Capacity) {
            this.storage[this.Volume] = book;
            System.out.println("I added the book " + book + "!");
            this.Volume++;
        } else if (this.Volume >= this.Capacity) System.out.println("The library is full!");

    }

    public Book search(String title) {
        for (int i = 0; i < this.Volume; i++) {
            if (title.equals(this.storage[i].toString())) {
                System.out.println("The book with the title " + title + " exists in the library!");
                return this.storage[i];
            }
        }
        System.out.println("The book with the title " + title + " does not exist in the library!");
        return null;
    }

}

public class Book {
    String title;

    public Book(String title){
        this.title = title;
        System.out.println("Book " + title + " created.");}

    public String toString(){
        return this.title;
    };
}

Naming

Java has a Naming Conventions that you currently don't follow.

int Capacity = 10;
int Volume = 0;

You should have : Capacity -> capacity and Volume -> volume. I really suggest you to read the convention when you learn a language. It helps readers to quickly scan your code.

Visibility

Currently, your fields in your class are package private, which mean that every class in your package can access your fields. I recommend to use private for fields in your classes, until you determine that you need something else.

You really want to have the minimum scope for you variable.

Initialization

You're doing your initalization two times. First when you declare Book[] storage = new Book[10]; you create an array of 10 books with it. Next in your constructor, you do: this.storage = new Book[10];. This does the same thing as previously so you should delete one of them.

Magic values

Magic values are constant that are not named but used directly in the code. In your case you have a default size for your library, but it's hard-coded. You could have a : private static final int DEFAULT_SIZE = 10. That would make it make clear what it used for.

Brackets and code block

I prefer to always use bracket in code block, I find it easier to read, less prone to bugs when you change add lines of code and there is less changes when you actually change it (think for source control like Git).

} else if (this.Volume >= this.Capacity) System.out.println("The library is full!");

Should be

} else if (this.Volume >= this.Capacity) {
    System.out.println("The library is full!");
}

For each loops

This is something you really want to use a lot. They exist in a lot of language and will make it easier to read when you loop over a collection and want to access each object. When you don't need to know where in the collection your are (the index), prefer using for-each.

So instead of :

for (int i = 0; i < this.Volume; i++) {
            if (title.equals(this.storage[i].toString())) {
                System.out.println("The book with the title " + title + " exists in the library!");
                return this.storage[i];
            }
        }

You would have :

for (Book book : this.storage) {
   if( book == null) {
        System.out.println("The book with the title " + title + " does not exist in the library!");
        return null;
   }
   if (title.equals(book.toString())) {
      System.out.println("The book with the title " + title + " exists in the library!");
      return book;
   }
}

Someone point me out in comments that there was a bug with my original for-each loop. Now that I fixed it, it is really less elegant. There will be duplication and a new corner case added to the method. This stuff make the method more complicated than not. This is partly because you have an array[] instead of something like and ArrayList (always use List the interface if you use ArrayList). This has already been covered in other answers, so if you change to a more dynamic structure use for-each, if not then continue using your for loop but keep in mind that for-each exist.


Performance: getting first value from comma delimited string I've got a string that has values that are delimited by comma's, like so:

$var = '1,23,45,123,145,200';

I'd like to get just the first value, so what I do is create an array from it and get the first element:

$first = current(explode(',', $var));

Fine enough. But this string can sometimes contain perhaps hundreds of values. Exploding it into an array and only using the first one seems kind of a waste. Is there a smarter alternative which is also more performant/less wasteful? I'm thinking some sort of regex or trimming, but I'm guessing that could be actually slower...

UPDATE:
A more complete benchmark script:

$start = $first = $str = null;//create vars, don't benchmark this
//time preg_match
$start = microtime(true);
$first = $str = implode(',', range(213,9999));
if (preg_match('/^[^,]+/', $str, $match))
{
    $first = $match[0];
}
echo $first, PHP_EOL, microtime(true) - $start, ' time taken
', PHP_EOL;
//time str* functions
$start = microtime(true);
$first = $str = implode(',', range(213,9999));
$first = substr($str, 0, strpos($str, ','));
echo $first, PHP_EOL, microtime(true) - $start, ' time taken
', PHP_EOL;
//now explode + current
$first = null;
$start = microtime(true);
$str = implode(',', range(213, 9999));
$first = current(explode(',', $str));
echo $first, PHP_EOL, microtime(true) - $start, ' time taken';

The result varried a little, but after 100 runs, the averages amounted to:

#1 substr+strpos: ~.0022ms as 1//base for speed
#2 preg_match: ~.0041 as ~2//about twice as slow as #1
#3 explode: ~.00789 as ~4//about 4 times <=> #1, twice as slow <=> regex

You're absolutely right, exploding a string, constructing an array to get just the first value is a waste of resources, and it is not the fastest way to get what you want.
Some might run to regex for help, and chances are that, in your case that will be faster. But nothing I can think of will beat the speed of PHP's string functions (which are very close to the C string functions). I'd do this:

$first = substr($var, 0, strpos($var, ','));

If the comma isn't present (say $var = '123'), then your current approach will assign 123 to $first. To preserve this behaviour, I'd go for:

$first = strpos($var, ',') === false ? $var : substr($var, 0, strpos($var, ','));

This is to say: if strpos returns false, then there is no comma at all, so assign the entire string to $first, else get everything in front of the first comma.

For completeness sake (and after some initial bench-marking), using preg_match did indeed prove to be faster than using explode with large strings ($var = implode(',', range(1, 9999));), when using this code:

$first = $var = implode(',', range(1,9999));
if (preg_match('/^[^,]*/',$var, $match))
{
    $first = $match[0];
}

But honestly, I wouldn't use regex in this case.

In the interest of fairness, and to to clarify how I found the regex to be faster:

$start = microtime(true);
$first = $str = implode(',', range(213,9999));
if (preg_match('/^[^,]+/', $str, $match))
{
    $first = $match[0];
}
echo $first, PHP_EOL, $str, PHP_EOL, microtime(true) - $start, ' time taken';
$first = null;
$start = microtime(true);
$str = implode(',', range(213, 9999));
$first = current(explode(',', $str));
echo $first, PHP_EOL, microtime(true) - $start, ' time taken';


Tax calculator using functional programming class Program
{
    static void Main(string[] args)
    {
        var priceList = new List<int>() { 10, 20, 30, 40 };
        var totalAmount = InvoiceCalculator.CalculateTotalPriceWithTax(priceList, 5);
        Console.WriteLine(totalAmount);
    }
}

static class InvoiceCalculator
{
    private static int CalculatePrice(List<int> priceList)
    {
        return priceList.Sum();
    }
    private static int CalculateTax(int price, int taxRate)
    {
        return (price * taxRate / 100); // Ignore int datatype for simplicity. input values are immutable
    }
    public static int CalculateTotalPriceWithTax(List<int> priceList, int taxRate)
    {
        int totalPrice = CalculatePrice(priceList); // input values are immutable
        int tax = CalculateTax(totalPrice, taxRate); // input values are immutable
        return totalPrice + tax;
    }
}

Features of the program are

Inputs are immutable.
Functions are not altering the state
Use of pure functions
CalculateTotalPriceWithTax is calling CalculatePrice and CalculateTax but not altering the state

•   For simplicity I have used int datatype instead float, double for tax calculation.

Is it violating functional programming principle because CalculateTotalPriceWithTax() function is calling CalculateTax and CalculatePrice function?

Note: None of these calls are altering the state.
For simplicity exception handling and aspects are ignored. Just interested in knowing the functional aspects of the program

Hi and welcome to CodeReview.

You are right, your code does not modify state (has not side effects) and the output of the methods depends only from the input... Therefore I would guess that the functions are "pure functions".

However, I would prefere an IEnumerable of int instead of List of int as parameter type because with an IEnumerable the API shows that the method does not change the passed list.

If you value immutability, there are also some immutable collection in .net (e.g.: The ImmutableList).

If you like functional programming, F# is really worth a look! FSharpForFunAndProfit is a good point to start :D


Euler #3 largest prime factor Problem Statement

This problem is a programming version of Problem 3 from projecteuler.net

  The prime factors of 13195 are 5, 7, 13 and 29.

  What is the largest prime factor of a given number N?

  Input Format First line contains T, the number of test cases. This is followed by T lines each containing an integer N.

  Output Format

  For each test case, display the largest prime factor of N.

  Constraints

  \$1 \le T \le 10\$

  \$10 \le N \le 10^{12}\$

#include<stdio.h>  //C language
int main()
{
long long  number; //Declaring Variables
int cases;
scanf("%d",&cases);
while(cases--)
{
    scanf("%lld",&number);
    long long div=2, ans = 0, maxFact;
    while(number!=0)
        {
            if(number % div !=0)
                div = div + 1;
            else
            {
                maxFact  = number;
                number = number / div;

                if(number == 1)
                    {
                        printf("%lld\n",maxFact);
                        ans = 1;
                        break;
                    }
            }
        }
 }
 return 0;
}

How can the speed be increased? This code gives me timeout error in the last test case of HackerRank.

Use properties of prime numbers

The only prime number that is even is 2, for the simple reason that if the number is even, it is evenly divisible by 2 and is thus not a prime.

So after checking if 2 is a factor and removing it, simply start with div = 3  and in the update do div = div + 2. And you're twice as fast!

But there is more, consider \$x=6k+l\$ where \$l\in\left[0,5\right]\in \mathcal{Z}\$ and \$k\ge 0\$. It's obvious \$x\$ can represent any positive integer.

Assume for a second that \$k>0\$ and lets look a bit closer:

\$x=6k+0\$  is always be divisible by 2 and 3.
\$x=6k+1\$ This might be a prime (7, 13 for example are primes).
\$x=6k+2=2\cdot (3k+1)\$ is always divisible by 2.
\$x=6k+3=3\cdot (2k+1)\$ is always divisible by 3.
\$x=6k+4=2\cdot(3k+2)\$  is always divisible by 2.
\$x=6k+5\$ This might be a prime (11, 19 for example are primes).

Now note that \$6k+5 = 6\left(k+1\right)-1\$ so after checking the divisors we would miss at the start (2 and 3) set div=6 and try division with div-1 and div+1 then increase div by six and repeat.

This means that you're skipping 2/3rds of all divisors, i.e. ~300% faster.

Your exit condition for the loop is useless

As the input is guaranteed larger than zero, number!=0 will always be true in your loop condition. In fact you can reduce branching even further like this:

while(number!=1){
    while(number % div == 0){
        number = number / div;
        maxFact = div;
    }
    div = div + 1;
}
printf("%lld\n",maxFact);
ans = 1;

that's one branch less per iteration.


Get list and default value of columns of table with Eloquent I use the below code to get a list of columns in a table, and assign the default value to the protected var $attributes in a Laravel Model.

In this code, I use the DB function because I don't know how to achieve the same result with Eloquent.

Model.php

protected $attributes = [];

public function __construct(array $attributes = array())
{
    parent::__construct($attributes);

    $columns = DB::connection()->getDoctrineSchemaManager()->listTableColumns('mailboxes');

    foreach ($columns as $column) {
        $name = $column->getName();
        $value = $column->getDefault();
        if (!is_null($value)) {
            $this->attributes[$name] = $value;
        }
    }
}
```

If I’m fully understanding your need, the best approach would be to set the defaults at the database level. If a record is saved with a null value for that attribute, it will automatically get the default value set at the database level.  The end result is much cleaner code and no extra processing needs during data load as the default is there at rest.

In the migration, for example, $table->string(‘favorite_color’)->default(‘Blue’);  , as described at https://laravel.com/docs/master/migrations#column-modifiers

Would this solve your need? If not, it probably means your need is contextual, and that this default doesn’t always need to apply. Think critically about that context and what it represents. It might indicate a difference in data which might cause further problems down the road.

I hope this helps!


Drawing a bufferedimage into another For what I'm doing, I need to be able to draw a buffered image directly into a larger buffered image. I've searched around a little and still have failed to find a better way, but my current way is extremely inefficient and I would like a better way if it's possible. Any improvements onto this design would also be great, even if it's not a totally separate way.

public static final void drawOntoBI(final BufferedImage on, final BufferedImage draw, final int x, final int y)
{
    // Calls method instead of needing to call them every loop
    final int width = draw.getWidth(), height = draw.getHeight(), onwidth = on.getWidth(), onheight = on.getHeight();
    // Stores temporary variables so they wont be recalculated
    int x3, y3;
    // Goes through all of the pixels in the drawn image to copy them
    for(int x2 = 0; x2 < width; x2++)
    {
        x3 = x + x2; // Prevents having to do the same math 3 times
        for(int y2 = 0; y2 < height; y2++)
        {
            y3 = y + y2;
            // Makes sure the image to be drawn is not out of bounds, if it is, go to the next location
            if(x3 > onwidth - 1 || x3 < 1 || y3 > onheight - 1 || y3 < 1) break;
            // Draws the pixel on the image where it needs to be from the x and y looped in the image
            on.setRGB(x3, y3, draw.getRGB(x2, y2));
        }
    }
}

This is the second biggest bottleneck on my program, so it really is important to be improved. It's called all the time, and is rather slow. Memory isn't an issue, so memory-using methods can be considered.

Java Native Solution

The solution Java offers for adding two BufferedImages should be faster than anything that you will program yourself:

 /**
  * prints the contents of buff2 on buff1 with the given opaque value.
  */
 private void addImage(BufferedImage buff1, BufferedImage buff2,
         float opaque, int x, int y) {
     Graphics2D g2d = buff1.createGraphics();
     g2d.setComposite(
             AlphaComposite.getInstance(AlphaComposite.SRC_OVER, opaque));
     g2d.drawImage(buff2, x, y, null);
     g2d.dispose();
 }

Your Code

As I said, if you remove the unnecessary x3 checks, you would at least save two comparisons for each loop of y2 (and those can really add up).

You can also move the onheight - 1 and onwidth - 1 computations outside the loop.

If most of the time the smaller image does fit into the larger image, you can also remove the bounds check completely. Create two new methods. One for when the image fits - no bound checks necessary - and one for when it does not fit (here you obviously still need the check). Then in your main method, you check once if the image would fit and use the quicker method without bounds check. If it does not fit, you use the method with bounds check.

If the smaller image does not fit most of the time, you could also first crop the smaller image (if it does not fit), so that it does always fit. I doubt that it will be faster, but it is worth a try.


Practice with dictionaries Post a cursory read of this I implemented a simple dictionary and an interface to  assign, replace, look up, and redefine its terms to apply the concepts.

It's simple, but I'd still like to know any way I could make this cleaner and more pythonic.

def print_dict(dictionary):
  for key in dictionary:
    print("{} : {}".format(key, dictionary[key]))

def display_menu():
  print("\n0 = Quit"
    + "\n1 = Look up a term"
    + "\n2 = Add a term"
    + "\n3 = Redefine a term"
    + "\n4 = Delete a term"
    + "\n5 = Display Dictionary"
  )

def is_integer(value):
  try:
   temp = int(value)
   return True
  except ValueError:
   return False

def validate(choice):
  if is_integer(choice) and 0 <= int(choice) <= 5:
    return int(choice)
  else:
    print("Input must be an integer between 0 and 5, inclusive")
    return validate(input("\nEnter Selection: "))

def lookup_term(dictionary):
  term = input("which term would you like to look up? ")
  if term in dictionary:
    print("{} : {}".format(term, dictionary.get(term)))
  else:
    print("Term does not exist, input 2 to add new term")

def redefine_term(dictionary):
  term = input("which term would you like to redefine? ")
  if term in dictionary:
    dictionary[term] = input("and its definition? ")
  else:
    print("Term does not exist, input 2 to add new term")

def add_term(dictionary):
  term = input("What term would you like to add? ")
  if term in dictionary:
    print("Already exists. To redfine input 3")
  else:
    dictionary[term] = input("and its definition? ")

def delete_term(dictionary):
  del dictionary[input('Which term would you like to delete? ')]

def process_request(choice, dictionary):
  if choice == 0:
    print("Thank you for using Stack Exchange Site Abbreviation!")
    quit()
  elif choice == 1:
    lookup_term(dictionary)
  elif choice == 2:
    add_term(dictionary)
  elif choice == 3:
    redefine_term(dictionary)
  elif choice == 4:
    delete_term(dictionary)
  else:
    print_dict(dictionary)

def main():
  site_dictionary = {
    'SO'  : 'Stack Overflow',
    'CR'  : 'Code Review',
    'LH'  : 'Lifehacks',
    '??'  : 'Puzzling',
    'SR'  : 'Software Recommendations',
    'SU'  : 'Super User',
    'M'   : 'Music: Practice & Theory',
    'RE'  : 'Reverse Engineering',
    'RPi' : 'Raspberry Pi',
    'Ro'  : 'Robotics'
  }

  print_dict(site_dictionary)
  print("\nWelcome to Stack Exchange Site Abbreviation Translator!")
  display_menu()
  while(True):
    process_request(validate((input("\nEnter Selection: "))), site_dictionary)

if __name__ == "__main__":
  main()

Some small improvements:

print_dict method: use iteritems()

for key, value in dictionary.iteritems():
print("{} : {}".format(key, value))

You can merge validate and is_integer. There are no benefits here gained by separating them in two different functions.
For lookup_term, use either:

value = dictionary.get(term)
if value:
    print("{} : {}".format(term, value))
else:
    print("Term does not exist, input 2 to add new term")

Or you could use:

if term in dictionary:
    print("{} : {}".format(term, dictionary[term]))
else:
    print("Term does not exist, input 2 to add new term")

The point I wanted to illustrate was use of dict.get()
In delete_term, you are not validating for input when key is not in dictionary.

Some other things:

Since all functions work with same dictionary, they could have been grouped together under same class with dictionary being shared data member.

Also, your dictionary is case sensitive. Looking at your use-case, this is not something you want.

If at anytime you need to change menu, you would need to change validate function. Maybe there should be easier way to do this? (Instead of else denoting 5 in menu, else could work for something else?)


Is there any faster way instead of using preg_match in the following code? this code finds if there is the string 2004 in <date_iso></date_iso> and if it is so, I echo some data from that specific element that the search string was found.

I was wondering if this is the best/fastest approach because my main concern is speed and the XML file is huge. Thank you for your ideas.

this is a sample of the XML

<entry ID="4406">
    <id>4406</id>
    <title>Book Look Back at 2002</title>
    <link>http://www.sebastian-bergmann.de/blog/archives/33_Book_Look_Back_at_2002.html</link>
    <description></description>
    <content_encoded></content_encoded>
    <dc_date>20.1.2003, 07:11</dc_date>
    <date_iso>2003-01-20T07:11</date_iso>
    <blog_link/>
    <blog_title/>
</entry>

this is the code

<?php
$books  = simplexml_load_file('planet.xml');
$search = '2004';
foreach ($books->entry as $entry) {
    if (preg_match('/' . preg_quote($search) . '/i', $entry->date_iso)) {
        echo $entry->dc_date;
    }
}
?>

First of all, put up a timer so you know if things get better.

You're repeating '/' . preg_quote($search) . '/i'for each book. You should create the search string only once or else you are wasting time:

<?php
$books  = simplexml_load_file('planet.xml');
$search = '2004';
$regex = '/' . preg_quote($search) . '/i';
foreach ($books->entry as $entry) {
    if (preg_match($regex, $entry->date_iso)) {
        echo $entry->dc_date;
    }
}
?>

If you are only looking for 2004 or similar you might analyze if simpler functions would be faster e.g. strpos.

Also the /i modifier might be unnecessary.