Coding standard and Readability: Verbose/Redundant statements vs Short/Compressed I have the following 3 different versions of code.
Version 1 is overly verbose and contains redundancy but on the other hand seems easier to read. Version 2 is somewhere in the middle, and Version 3 is shortened and straight to the point.

Their all equivalent in terms of functionality. I am not sure about readability.
According to best practices which is the best? Please add actual pros-cons based on theory.
Is there a chance situations like these are also determined by programming style and hence are opinion based as opposed to having black/white clear guidelines?

Version 1:

for (int i = 0; i < resultsSize; i++)
            {
                bundle = ja.getJSONObject(i).getString(SOLR_BUNDLEID_FIELD);
                app = DataStore.applicationsCache.get(bundle);
                if (app!=null)
                {

                    if(catalogRequest.filter.query_pub==null)
                    {
                        addApp(DataStore.applicationsCache.get(bundle),preparedApps);
                        continue;
                    }

                    if(app.vendor.equalsIgnoreCase(catalogRequest.filter.query_pub))
                        addApp(DataStore.applicationsCache.get(bundle),preparedApps);
                }

            }

Version 2:

for (int i = 0; i < resultsSize; i++)
            {
                bundle = ja.getJSONObject(i).getString(SOLR_BUNDLEID_FIELD);
                app = DataStore.applicationsCache.get(bundle);
                if (app!=null)
                {
                    if(catalogRequest.filter.query_pub==null || app.vendor.equalsIgnoreCase(catalogRequest.filter.query_pub))
                    addApp(DataStore.applicationsCache.get(bundle),preparedApps);
                }

            }

Version 3:

for (int i = 0; i < resultsSize; i++) {
                if ((app = DataStore.applicationsCache.get(ja.getJSONObject(i)
                        .getString(SOLR_BUNDLEID_FIELD))) != null
                        && (catalogRequest.filter.query_pub == null || app.vendor
                                .equalsIgnoreCase(catalogRequest.filter.query_pub)))
                    addApp(app, preparedApps);
            }

Version 1 has code duplication for one seemingly very important line:

addApp(DataStore.applicationsCache.get(bundle), preparedApps);

Duplicating this line is not a good idea if it can be easily avoided.

Version 3 is using && and || mixed in the same condition which makes it a lot harder to understand. Try to avoid mixing those two together, instead extract conditions into variables, as palacsint has done with boolean emptyQueryPub.

Version 2 is the one I like the best, but with some modifications.

Java Coding Conventions about braces is that they should be on the same line as the condition/loop and not on it's own line.

Adding a few spaces makes the code more readable.

Most IDEs has an Auto-format key-shortcut. Ctrl + Shift + F in Eclipse and Alt + Shift + F in NetBeans. I recommend using it.

The results of the above mentioned fixes:

for (int i = 0; i < resultsSize; i++) {
    bundle = ja.getJSONObject(i).getString(SOLR_BUNDLEID_FIELD);
    app = DataStore.applicationsCache.get(bundle);
    if (app != null) {
        if (catalogRequest.filter.query_pub == null ||
                app.vendor.equalsIgnoreCase(catalogRequest.filter.query_pub)) {
            addApp(DataStore.applicationsCache.get(bundle), preparedApps);
        }
    }
}


group list of dictionary I have a list of dictionaries and I want to group them by a specific value
let's say this is the list:
lst = [{'name':'john','course':'geography','grade':'80'},
       {'name':'Mary','course':'geography','grade':'70'},
       {'name':'John','course':'calculus','grade':'70'},
       {'name':'Mary','course':'calculus','grade':'70'}]

I will want all the dictionaries with the same value of "name" to be in a sublist, like this :
lst_new = [[{'name':'John','course':'geography','grade':'80'},
            {'name':'Mary','course':'geography','grade':'70'}],
           {'name':'John','course':'calculus','grade':'70'},
           {'name':'Mary','course':'calculus','grade':'70'}]

so I am doing it this way :
lst_new = [[elt for elt in lst if elt['name']==n] for n in np.unique(np.array([elt['name'] for elt in lst]))]

but I am feeling there are too much iterations, is there a better way to do it?

Use collections.defaultdict to "bucketize" the items in your list. Then collect the buckets to get the desired result.
from collections import defaultdict

bucket = defaultdict(list)

for record in lst:
    bucket[record['name']].append(record)

lst_new = list(bucket.values())


Ternary gate handling - more succinct way then a bunch of if statements I have classes representing gates (OR, XOR, AND etc) with the following method that is called whenever a property changes.

Here is the XORGate code:

public void computeOutput() {
        if (this.input1 != null && this.input0 != null) {
            if (this.input0.getValue() || this.input1.getValue()) {
                if (this.input0.getValue() && this.input1.getValue()) {
                    this.output.setValue(false);
                }
                else {
                    this.output.setValue(true);
                }

            } else {
                this.output.setValue(false);
            }
        }
        else {
            this.output.Value = false;
        }

    }

It's horrible, but I can't think of a nicer way. It works as follows:

If any of the inputs are null, treat the output as false. If any of the inputs are true BUT not all of the outputs are true (i.e just one is true), set the output to true. If BOTH input terminals are true, output is false. And finally, if none of the input is true, output is false.

Very basic stuff.

public void computeOutput() {
    output.setValue(
            (input1 == null || input0 == null)
            ? false
            : (input0.getValue() ^ input1.getValue()));
}


Script to toggle touchpad This sequence properly shows whether the TouchpadOff option for synclient is on or not (outputs either a 0 or a 1):

synclient | grep 'TouchpadOff' | tail -n 1 | awk '{print substr($3, 1)}'

Trying to make a keyboard shortcut to toggle the touchpad with a basic script based on that line, I wrote this:

#!/bin/bash

PADOFF=$(synclient | grep 'TouchpadOff' | tail -n 1 | awk '{print substr($3, 1)}')
if [ "$PADOFF" == "0" ]
then
    synclient TouchpadOff=1
else
    synclient TouchpadOff=0
fi.

I don't understand the purpose of substr($3, 1) — isn't that the same as $3?

AWK alone can do the work of grep, tail, and negation.  I would write it this way:

synclient TouchpadOff=$(synclient | awk '/TouchpadOff/ { off=int($3) }
                                         END           { print !off }')


Formatting fractions found in a given string to html fraction I'm working on a site running woocommerce and wp where the user will add/has added a dimension attribute.

The dimension string will take the following format originally:

Ht: 30 1⁄2" W: 22 1⁄2" D: 18"

and should return the string with html to format the fraction e.g:

Ht: 30 1&frasl;2" W: 22 1&frasl;2" D: 18"

will the following to consider:

The amount of fractions in the string will be unknown (0 - X)
The string can be any valid fraction
The user may/may not add a space in between the fraction and the quotation e.g Ht: 30 1/2 " W...

Here's my solution:

add_filter( 'woocommerce_attribute', __NAMESPACE__ . '\\format_dimension_attr', 10, 2 );

/**
 * Format fractions in the Dimension attr string to include proper html
 *
 * e.g The 1/2 in 'Height: 5 1/2"' should be
 *
 * 1&frasl;2
 */
function format_dimension_attr( $value, $att ) {

    if ( 'Dimensions' === $att['name'] ) {
        // Use the raw value instead of $value (which includes html)
        $raw = $att->get_options()[0];

        if ( strpos( $raw, '/' ) !== false ) {

            $raw = explode( ' ', $raw );

            // Check for accidental space before quotation and remove it
            // e.g 'height: 5 1/2 "' will produce [5, 1/2, "]
            $raw = array_filter( $raw, function( $val ) {
                return '"' !== $val;
            } );

            // Run through the array
            $raw = array_map( function( $val ) {

                // Format the current item if it contains a slash
                if ( strpos( $val, '/' ) !== false ) {
                    // Remove any " before splitting
                    $val = explode( '/', trim( $val, '"' ) );
                    $val = '' . $val[0] . '&frasl;' . $val[1] . '"';
                }

                return $val;

            }, $raw );

            // Replace the p tag we removed by using the raw value
            $value = wpautop( join( ' ', $raw ) );
        }
    }

    return $value;
}

Well, regex is often a bad idea, but it's one of the case I find it way better than the alternative :

$raw='30 1/2 et 22 3 / 4';
echo preg_replace('/([0-9]+)\s*\/\s*([0-9]+)/', '$1&frasl;$2',$raw);

Just test it and see for yourself.


Determine if .deb package is installed using Bash I was able to use dpkg-query to determine if a package is installed on Ubuntu, but it's a bit of a mess because the name used to install the package doesn't always align with what it's installed as. For instance, gnutls-dev is installed under libgnutls-dev...

Any suggestions on how I could clean this up a bit?

function isInstalled() {
    if dpkg-query -W -f'${Status}' "$1" 2>/dev/null | grep -q "ok installed" || dpkg-query -W -f'${Status}' "lib$1" 2>/dev/null | grep -q "ok installed"; then
        echo 1;
    fi

    echo 0
}

It seems odd that the function prints 0 in all cases, including when 1 has been printed.

For a utility like this, it's probably better to communicate via the exit status:

isInstalled() {
    !   dpkg-query -W -f'${Status}' "$1" 2>/dev/null | grep -q "ok installed" \
     || dpkg-query -W -f'${Status}' "lib$1" 2>/dev/null | grep -q "ok installed"
}

I'd be inclined to remove the !, returning zero (success) if the package is found.

If you have aptitude available, it's possible to ask it to search for any installed package of the given name, or providing the name:

aptitude -q2 search "~i~P?exact-name($1)|~i?exact-name($1)" >/dev/null

~i: installed package
~P: provides name
|: alternation
^$1\$
?exact-name: exactly what it says

This is less brittle, as it doesn't rely on the providing package being formed by prepending lib to the virtual package name.  For example, with www-browser, it finds all installed web browsers.  Also, it avoids matching superstrings of the supplied name.

If you know that $1 will always be the name of a virtual package, you can of course use just the ~P side of the alternation.


Simple CSS minifier for twig, with basic caching On part of a project, I have some CSS being read using twig's source() function.
But sending a huge chunk of CSS is not optimal.
To reduce the size of the HTML, I've written this twig filter.

However, I've noticed that some big files would take a bit more to be minimified.
To reduce the time to re-minify the CSS, I've added a crude caching system.

Assuming that you have the following:

$twig = new Twig_Environment(new Twig_Loader_Filesystem( ... ), array( ... ));

You have the following code:

$twig->addFilter(new Twig_SimpleFilter('minify_css', function($css, $cache = true){
        $path = $_SERVER['DOCUMENT_ROOT'] . '/cache/minify_css/';
        $min_length = 1024;

        if(!is_dir($path))
        {
            mkdir($path, 0755, true);
        }
        $length = strlen($css);
        $result = '';

        if($cache && $length > $min_length && @is_file($file = $path . ($hash = md5($css)) . '.css' ))
        {
            $result = file_get_contents($file);
        }
        else if($length >= 9)
        {
            $result = preg_replace(
                array(
                    '@/\*[^*]*\*+([^/][^*]*\*+)*\/@', // /*abc*/ ->
                    '@([\-;\{]\w*)\s*:\s*@', //a : b -> a:b
                    '@\s*;\s*\}\s*@', //a:b ; } -> a:b}
                    '@\s*\{\s*@', // a { b:c} -> a{b:c}
                    '@\s*!important\s*@i', // a:b !important } -> a:b!important}
                    '@([,;}!:]|^)\s*0+\s*px@i', // a: 0px -> a:0
                    '@([,;:!.>~\+=\[\{\}\(]|^)[\s\r\n]+@', // a, b -> a,b
                    '@[\s\r\n]+([,;!>~\+=\]\{\}\)]|$)@', // a ,b -> a,b
                    '@([,:\( ])\s*#([\da-f])\2([\da-f])\3([\da-f])\4@i' // a: #FF00CC -> a:#F0C
                ),
                array(
                    '',
                    '$1:',
                    '}',
                    '{',
                    '!important',
                    '${1}0',
                    '$1',
                    '$1',
                    '$1#$2$3$4'
                ),
                str_replace(array("\r\n", "\r", "\n", "\t", '  ', '    ', '    '), '', $css)
            );

            if($cache && $length > $min_length)
            {
                file_put_contents($file, $result);
            }
        }
        return $result;
    }), array('is_safe' => array('html', 'css', 'js'))
);

This filter takes 1 parameter, to indicate if there's or not caching, and returns the minified CSS when applicable.

An example of it's usage:

{% for file in css_files %}
    <style>{{ source('assets/css/' ~ file ~ '.css') | minify_css }}</style>
{% endfor %}

This assumes that the template has access to the variable css_files, containing the name of those files.

What else can I improve here?
Any speed improvements or readability improvements?

The very obvious thing is that str_replace.

Here, there are 2 things to change:

Completely remove this line.
All that replace can be made into the following regular expression: @(?:\s{2,}|[\r\n\t])+@
This should be the very first regular expression.
Since all the other rules handle whitespace, one only needs to normalize it. The rest of the replacements will dictate if it needs to be replaced or not.
If the whitespace is simply removed, it will lead to removing needed whitespace.
Here's an example CSS:

#services  img{
    max-width: 100%;
}

#slide .carousel-caption {
display: none
}

With the replacement as an empty string, you would remove the double-space in #services  img. Normalizing it to a single space will return #services img instead of minifying into #servicesimg.

With this said, and a bug fixed, the code will look like the following:

    $twig->addFilter(new Twig_SimpleFilter('minify_css', function($css, $cache = true){
        $path = $_SERVER['DOCUMENT_ROOT'] . '/cache/minify_css/';
        $min_length = 1024;

        if(!is_dir($path))
        {
            mkdir($path, 0755, true);
        }
        $length = strlen($css);
        $result = '';

        if($cache && $length > $min_length && @is_file($file = $path . ($hash = md5($css)) . '.css' ))
        {
            $result = file_get_contents($file);
        }
        else if($length >= 9)
        {
            $result = preg_replace(
                array(
                    '@(?:\s{2,}|[\r\n\t])+@', // normalize whitespace
                    '@/\*[^*]*\*+([^/][^*]*\*+)*\/@', // /*abc*/ ->
                    '@([\-;\{]\w*)\s*:\s*@', //a : b -> a:b
                    '@\s*;\s*\}\s*@', //a:b ; } -> a:b}
                    '@\s*\{\s*@', // a { b:c} -> a{b:c}
                    '@\s*!important\s*@i', // a:b !important } -> a:b!important}
                    '@([,;}!:]|^)\s*0+\s*px@i', // a: 0px -> a:0
                    '@([,;:!.>~\+=\[\{\}\(]|^)[\s\r\n]+@', // a, b -> a,b
                    '@[\s\r\n]+([,;!>~\+=\]\{\}\)]|$)@', // a ,b -> a,b
                    '@([,:\( ])\s*#([\da-f])\2([\da-f])\3([\da-f])\4@i' // a: #FF00CC -> a:#F0C
                ),
                array(
                    ' ',
                    '',
                    '$1:',
                    '}',
                    '{',
                    '!important',
                    '${1}0',
                    '$1',
                    '$1',
                    '$1#$2$3$4'
                ),
                $css
            );

            if($cache && $length > $min_length)
            {
                file_put_contents($file, $result);
            }
        }
        return $result;
    }), array('is_safe' => array('html', 'css', 'js'))
);


General Retry Strategy #3 with TryResult I wanted to use the Try helper from @DmitryNogin's General Retry Strategy #2 but each attempt to implement it revealed another thing that is missing.

In my review I mentioned that it would be useful to be able to log the exceptions but later I came to the conclusion that perhaps it would be better to implement something similar to the ParallelLoopResult - this is how the TryResult was formed.

public struct TryResult
{
    public TryResult(bool isCompleted, IEnumerable<Exception> exceptions) : this()
    {
        IsCompleted = isCompleted;
        Exceptions = exceptions;
    }

    public bool IsCompleted { get; private set; }

    public IEnumerable<Exception> Exceptions { get; private set; }
}

Then I added it to the base class:

public abstract class Try
{
    public static readonly Try Never = new Never();
    public static readonly Try Once = Retry(delay: 0, times: 0, ratio: 0);

    public static Try Retry(int delay, int times, double ratio) =>
        RetryAfter(from i in Enumerable.Range(0, times)
                   select delay * Math.Pow(ratio, i) into d
                   select (int)d);

    public static Try RetryAfter(params int[] delays) => RetryAfter(delays.AsEnumerable());
    public static Try RetryAfter(IEnumerable<int> delays) => new Retry(delays);

    public void Execute(Action action) => Execute(action, CancellationToken.None);
    public abstract TryResult Execute(Action action, CancellationToken cancellationToken);

    public Task ExecuteAsync(Action action) => ExecuteAsync(action, CancellationToken.None);
    public Task ExecuteAsync(Action action, CancellationToken cancellationToken) =>
        ExecuteAsync(() => { action(); return Task.CompletedTask; }, cancellationToken);

    public Task ExecuteAsync(Func<Task> action) => ExecuteAsync(action, CancellationToken.None);
    public abstract Task<TryResult> ExecuteAsync(Func<Task> action, CancellationToken cancellationToken);

}

and adjusted the implementations:

class Never : Try
{
    public override TryResult Execute(Action action, CancellationToken cancellationToken)
    {
        return new TryResult(true, Enumerable.Empty<Exception>());
    }
    public override Task<TryResult> ExecuteAsync(Func<Task> action, CancellationToken cancellationToken)
        => Task.FromResult<TryResult>(new TryResult(true, Enumerable.Empty<Exception>()));
}

the same for Retry:

class Retry : Try
{
    IEnumerable<int> Delays { get; }

    public Retry(IEnumerable<int> delays)
    {
        Delays = delays;
    }

    public override TryResult Execute(Action action, CancellationToken cancellationToken)
    {
        var exceptions = new List<Exception>();

        foreach (var delay in Delays)
        {
            try
            {
                action();
                return new TryResult(true, exceptions);
            }
            catch
            {
                cancellationToken.WaitHandle.WaitOne(delay);
                cancellationToken.ThrowIfCancellationRequested();
            }
        }
        return new TryResult(false, exceptions);
    }

    public override async Task<TryResult> ExecuteAsync(Func<Task> action, CancellationToken cancellationToken)
    {
        var exceptions = new List<Exception>();

        foreach (var delay in Delays)
        {
            try
            {
                await action();
                return new TryResult(true, exceptions);
            }
            catch
            {
                await Task.Delay(delay, cancellationToken);
            }
        }

        return new TryResult(false, exceptions);
    }
}

Let’s put the cart before the horse – I mean Catch before the Try :)

class Program
{
    static void Main(string[] args)
    {
        using (new Catch<InvalidOperationException>())
        using (new Catch<FormatException>(ex => Console.WriteLine("Oops!")))
            Try.Retry(100, 3, 3).Execute(() =>
            {
                Console.WriteLine("Trying!");
                throw new FormatException();
            });
    }
}

These using Catch statements define exceptions we can tolerate and continue trying. All other exceptions will be interpreted as critical. We can also rethrow or log in the optional handler.

Library classes:

public class Catch<TException> : Catch
    where TException : Exception
{
    public Catch()
        : this(ex => { })
    {
    }

    public Catch(Action<TException> handler)
    {
        Handler = handler;
    }

    protected internal override bool HandleCore(Exception ex)
    {
        if (ex is TException)
        {
            Handler(ex as TException);
            return true;
        }

        if (Previous == null)
            throw ex;
        else
            return Previous.HandleCore(ex);
    }

    Action<TException> Handler { get; }
}

And:

public abstract class Catch : Ambient<Catch>
{
    public static void Handle(Exception ex) =>
        Current?.HandleCore(ex);

    protected internal abstract bool HandleCore(Exception ex);
}

And:

public abstract class Ambient<T> : IDisposable where T : Ambient<T>
{
    static readonly string Id = typeof(T).FullName;
    protected static T Current
    {
        get { return (T)CallContext.LogicalGetData(Id); }
        set { CallContext.LogicalSetData(Id, value); }
    }

    protected Ambient()
    {
        Previous = Current;
        Current = (T)this;
    }

    public void Dispose() => Current = Previous;
    protected T Previous { get; }
}

And we also need to update Retry strategy:

    public override void Execute(Action action, CancellationToken cancellationToken)
    {
        foreach (var delay in Delays)
            try
            {
                action();
                return;
            }
            catch(Exception ex)
            {
                Catch.Handle(ex); // <- NEW LINE
                cancellationToken.WaitHandle.WaitOne(delay);
                cancellationToken.ThrowIfCancellationRequested();
            }

        action();
    }

UPDATE

Let's have this for the Log class - note Log.IfFail():

class Log : Ambient<Log>
{
    public static Exception IfFail(Action action)
    {
        try
        {
            action();
            return null;
        }
        catch(Exception ex)
        {
            Write(ex.Message);
            return ex;
        }
    }

    public static void Write(string line) =>
        Current?.WriteCore(line);

    public Log(string fileName)
    {
        FileName = fileName;
    }

    void WriteCore(string line) => File.AppendAllText(FileName, line);
    string FileName { get; }
}

And:

public static class Error
{
    public static void Rethrow(this Exception ex)
    {
        if (ex != null)
            throw ex;
    }
}

So we can have TryExecute:

    public override bool TryExecute(Action action, CancellationToken cancellationToken)
    {
        foreach (var delay in Delays)
            try
            {
                Log.IfFail(action).Rethrow();
                return true;
            }
            catch(Exception ex)
            {
                Catch.Handle(ex);
                cancellationToken.WaitHandle.WaitOne(delay);
                cancellationToken.ThrowIfCancellationRequested();
            }

        return Log.IfFail(action) == null; // yes, ugly :) let's invent something better
    }

We can pass more context for logging at the moment of ambient log construction.

UPDATE #2 :)

public static class Error
{
    public static void Rethrow(this Exception ex)
    {
        if (ex != null)
            throw ex;
    }

    public static bool ToTryResult(this Exception ex)
    {
        return ex == null;
    }
}

And:

    public override bool TryExecute(Action action, CancellationToken cancellationToken)
    {
        foreach (var delay in Delays)
            try
            {
                Log.IfFail(action).Rethrow();
                return true;
            }
            catch(Exception ex)
            {
                Catch.Handle(ex);
                cancellationToken.WaitHandle.WaitOne(delay);
                cancellationToken.ThrowIfCancellationRequested();
            }

        return Log.IfFail(action).ToTryResult();
    }


Removing jQuery event handler from single element bound by delegate I'm binding a handler to the keyup event of all input and textareas in a document:

$(document).on('keyup','input,textarea',$.debounce(600, editor.handleGlobalChange));

I don't want the handler to fire on a specific input field. So I want to unbind the keyup event:

$('#afield').off('keyup');

However this does not unbind the keyup event, the handler is still called. I assume this is because there is no keyup event bound to the $('#afield') element, the event is just bubbling up to the $(document) where the actual handler is bound.

So to stop it triggering I can do the following:

$('#afield').on('keyup',function(e) {
    e.stopImmediatePropagation();
});

This works, but it feels grim, setting a handler to stop another.

Is this acceptable or should I just no bind the event in the first place?

I could ignore certain classes or a custom data-nokeyup attribute:

$(document).on('keyup','input,textarea,not([data-nokeyup]',$.debounce(600, editor.handleGlobalChange));

I have to use a delegated event handler because my inputs and textareas are created after the initialisation code is run. $('input,textarea').keyup(...) would return no elements. $(document).on('keyup','input,textarea',...) catches all keyup events and inspects the event's target element.

Well, as far as I can tell, there are 3 ways to deal with this; you've mentioned 2 of them already:

Pre-empt the event for a specific element by adding another handler,
Scope the event handler differently to exclude a specific element, or
Do the filtering in the common handler

(Edit: All of this is assuming you really want to keep the document-level, event bubble handler, rather than go with mseancole's straightforward solution)

The 3rd option would look something like:

$(document).on('keyup','input,textarea', function (event) {
  if( event.type === 'sometype' && this === someElement ) {
    return;
  }
  $.debounce(600, editor.handleGlobalChange).call(this, event);
});

... which isn't pretty either. Of course you could make it fancier by maintaining a list of event type/element combos to be ignored, rather than hardcoding it, but... eh, seems like a lot of work.

I'd say go with option 1, but perhaps wrap that bit of logic in a function or plugin. I.e. something that would let you say:

$(document).on('keyup','input,textarea',$.debounce(600, editor.handleGlobalChange));
$('#afield').ignore('keyup');

.ignore() would then set the event handler that kills the event in its tracks. In your case, you can use stopPropagation instead of stopImmediatePropagation. That way, you can still attach other handlers to that specific element, without ignore eating the event.

Edit: Now that I think about it, ignore is a bad name. Too strong a word. dontBubble is accurate, but I'm a stickler for using proper apostrophes, and doNotBubble sounds too aggressive. Maybe popBubble? Too cutesy? Maybe just something like halt? debubble? Yessss, I'm thinking way too much about this :)

Here's a demo. It's in CoffeeScript, because... well, because I like CoffeeScript. The general idea should be clear enough though. Here's the code in case jsfiddle implodes

makeNoise = ->
  $('pre').append "You typed!<br>"

# 4-line jQuery plugin
$.fn.ignore = (eventType) ->
  $(this).each ->
    $(this).on eventType, (event) ->
      event.stopPropagation();

$ ->
  # General event handler for all inputs
  $(document).on 'keyup', 'input', makeNoise

  # Exempt a specific element
  $('#quiet').ignore 'keyup'

  # Handlers added *directly* to the #quiet element
  # are still called, only the bubbling is silenced.
  $('#quiet').on 'keyup', ->
    alert "Shhhh!"

None of this is any different from what you're already doing. It's just wrapped up for convenience.


Finding inorder successor I want you to pick my code apart and give me some feedback on how I could make it better or more simple. This code finds inorder successor in a binary tree.

public class Successor {

    private TreeNode root;

    private class TreeNode {
        TreeNode left;
        TreeNode right;
        TreeNode parent;
        int item;

        TreeNode(TreeNode left, TreeNode right, TreeNode parent, int item) {
            this.left = left;
            this.right = right;
            this.parent = parent;
            this.item = item;
        }
    }

    public void makeBinarySearchTree(int[] a) {
        for (int i : a) {
            addElement(i);
        }
    }

    public void addElement(int element) {
        if (root == null) {
            root = new TreeNode(null, null, null, element);
        } else {
            TreeNode prevNode = null;
            TreeNode node = root;

            while (node != null) {
                prevNode = node;
                // no definition exists, and for convenience we will stick to this one.
                if (element < node.item) {
                    node = node.left;
                } else {
                    node = node.right;
                }
            }

            if (element < prevNode.item) {
                prevNode.left = new TreeNode(null, null, prevNode, element);
            } else {
                prevNode.right = new TreeNode(null, null, prevNode, element);
            }
        }
    }

    private TreeNode search (int x) {
        TreeNode node = root;

        while (node != null) {
            if (x < node.item) {
                node = node.left;
            } else if (x  > node.item) {
                node = node.right;
            } else {
               return node;
            }
        }

        return null;
    }

    public Integer getSucessor(int val) {
        TreeNode node = getInorderSuccessor(search(val));
        return node == null ? null : node.item;
    }

    private TreeNode getInorderSuccessor(TreeNode node) {
        if (node == null) return null;

        if (node.right != null) {
            node = node.right;
            while (node.left != null) {
                node = node.left;
            }
            return node;
        } else {
            while (node.parent != null && node == node.parent.right) {
                node = node.parent;
            }
            return node.parent;
        }
    }
  }

Please give clear meaningful name. Your class name is Successor... now who's successor? yours? king Luthar's? who's? See the concern. I think BinarySearchTree is the apt. name for the class.
In makeBinarySearchTree you are creating a Binary Tree, so someone seeing your code will think it as a factory method, so better make it that way.
You can make your class generic but that's not a big deal for now but in future keep this in head and try to find always general solution for any given problem.
Someone using your getSucessor method(should be getSuccessor) will always get the inorder-successor. Now what if someday someone want to find preorder-successor or preorder-successor, what will you do then?

Otherwise your code is good and self-documenting.


Parsing nested JSON with Haskell and Aeson I'm trying to reduce nested JSON data using Haskell and Aeson without creating data/record types.
The following code works, but seems ugly and unreadable.
Any advice on cleaning this up would be great.

Thank you

import GHC.Exts
import Data.Maybe (fromJust)
import Data.Text (Text)
import Data.Aeson
import qualified Data.HashMap.Strict as M

testVal :: Value
testVal = Object $ fromList [
  ("items", Array $ fromList [
    (Object $ fromList [
      ("entity", (Object $ fromList [
        ("uuid", String "needed-value1")]))]),
    (Object $ fromList [
      ("entity", (Object $ fromList [
        ("uuid", String "needed-value2")]))])
    ])]

getItems :: Value -> Value
getItems (Object o) = fromJust $ M.lookup "items" o

getEntities :: Value -> Value
getEntities (Array a) = Array $ fmap (\(Object o) -> fromJust $ M.lookup "entity" o) a

getUuids :: Value -> Value
getUuids (Array a) = Array $ fmap (\(Object o) -> fromJust $ M.lookup "uuid" o) a

getTexts :: Value -> [Text]
getTexts (Array a) = toList $ fmap (\(String s) -> s) a

someFunc :: IO ()
someFunc = do
  let items = getItems testVal
      entities = getEntities items
      uuids = getUuids entities
      texts = getTexts uuids
  print texts

output:

["needed-value1","needed-value2"]

You should really use an algebraic data type to implement your JSON interface. You can even automatically derive FromJSON instances with DeriveGeneric GHC extension, e.g.

{-# LANGUAGE DeriveGeneric #-}
{-# LANGUAGE OverloadedStrings #-}

import GHC.Generics
import Data.Aeson
import Data.ByteString.Lazy (ByteString)

data Query = Query { items :: [Entity] }
             deriving (Show, Generic)

instance FromJSON Query -- automatically derived instance by DeriveGeneric

data Entity = Entity { uuid :: String }
              deriving (Show)

instance FromJSON Entity where
    parseJSON = withObject "Entity" $ \v -> do
        -- as your json data is additionally nested with an entity object
        -- extract the entity object first
        obj <- v .: "entity"
        -- then extract the uuid field from the entity object
        uid <- obj .: "uuid"
        return $ Entity uid

testVal :: ByteString
testVal = "{\"items\": [{\"entity\": {\"uuid\": \"needed-value1\"}}, {\"entity\": {\"uuid\": \"needed-value2\"}}]}"

main :: IO ()
main = do
    let mayQuery = decode testVal
    case mayQuery of
        Just query -> print $ map uuid $ items query
        Nothing -> putStrLn "JSON parsing error"

I transformed your sample Value into a JSON string, to make the parsing clearer.

The FromJSON instance for Query is automatically derived, if you want to write it by hand, you can do this in analogy to the FromJSON instance of Entity.

This way of parsing your JSON data is very scalable, as you can easily add new fields to your data types, without complicating your code.


Reducing code with two identical objects I have two identical objects on the page, so I get more of the same lines of code.  Can anything be made simpler, perhaps with merging?

for(var i=0;i<ticks.length,i<ticks2.length;i++) {

  var tick = ticks[i];
  var tick2 = ticks2[i];
  var tickCenter = tick.offsetWidth / 2
  var offset = (i - currentTick) * pixelsPerTick;
  var percent = 1-(Math.abs(i-currentTick)/10);

  tick2.style.top = ((offset + center - tickCenter)|0) + "px";
  tick2.style.opacity = percent;

  tick.style.top = ((offset + center - tickCenter)|0) + "px";
  tick.style.opacity = percent;
}

You can combine the two identical assignments at the end into the same line rather than recalculating the same value again:

for (var i = 0; i < ticks.length && i < ticks2.length; i++) {

    var tick = ticks[i];
    var tick2 = ticks2[i];
    var tickCenter = tick.offsetWidth / 2
    var offset = (i - currentTick) * pixelsPerTick;
    var percent = 1 - (Math.abs(i - currentTick) / 10);

    tick.style.top = tick2.style.top = ((offset + center - tickCenter) | 0) + "px";
    tick.style.opacity = tick2.style.opacity = percent;
}

I also fixed your condition in the for loop so that it safely checks both .length values.

You could also avoid some of the intermediate values, but I personally would preserve them the way you have them because of the readability/understandability advantage of having it this way vs. putting them all in one giant formula.


Giveth me thy easier user input in C++ - follow up I've decided to take some of the many suggestions for improvement on my previous question, Easier user input in C++, and actually get it to work as expected. This time around, a few things are different, namely:

The function will now accept string input with spaces, like "Ethan Bierlein", and not just spit back the first "word" in the input.
The function now also accepts a different input stream rather than just std::cin, if, for some reason you do something like that.
The function now allows for a "no-prompt" option, as well, which just means that the argument prompt has a default value of "".
There is an optional way to specify the type of the prompt as well now. It's default type is std::string.
The function, now named get_input, has it's own namespace, easy_input, rather than being patched into std.

I do have a few concerns about my code though, and I'd like to hear your opinions on them:

Is it a good idea to declare a static variable, just so I can have a default value for prompt? Is it a good idea to have a default value for prompt?
How necessary is it to provide an option to get input from a different input stream?
Am I writing good C++, or am I doing certain things horribly wrong?
Is there anything else that stands out for improvment?

easy_input.h

#if HAVE_PRAGMA_ONCE
#pragma once
#endif

#ifndef EASY_INPUT_H_
#define EASY_INPUT_H_

#include <iostream>
#include <string>
#include <boost/lexical_cast.hpp>

namespace easy_input
{
    const std::string input_error_message = "an I/O error was encountered.";
    static std::string default_prompt = "";

    template <typename TInput, typename TPrompt = std::string>
    TInput get_input(
        const TPrompt& prompt = default_prompt,
        std::istream& input_stream = std::cin
    );
}

/**
 * This function serves as a useful wrapper for getting user input.
 * Rather than forcing the user to type out multiple lines every
 * time they want to get input, they only have to type one line.
 * @tparam          TInput  - The type of the input to obtain.
 * @tparam          TPrompt - The type of the prompt to be used. The default type is std::string.
 * @param {TPrompt} prompt  - The prompt to be used.
 */
template <typename TInput, typename TPrompt = std::string>
TInput easy_input::get_input(
    const TPrompt& prompt = easy_input::default_prompt,
    std::istream& input_stream = std::cin)
{
    std::cout << prompt;
    std::string user_input_value { };

    if(!std::getline(input_stream, user_input_value)) {
        throw std::istream::failure { easy_input::input_error_message };
    }
    return boost::lexical_cast<TInput>(user_input_value);
}

#endif

main.cpp (tests)

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

int main()
{
    std::string a = easy_input::get_input<std::string>("Enter your name please! ");
    std::cout << a << "\n";

    int b = easy_input::get_input<int>("Enter an integer please! ");
    int c = easy_input::get_input<int>("Enter an integer please! ");
    std::cout << b + c << "\n";

    std::string d = easy_input::get_input<std::string>();
    std::cout << d << "\n";

    int e = easy_input::get_input<int>();
    int f = easy_input::get_input<int>();
    std::cout << e + f << "\n";
}

Redefinition of default arguments

First of all, you cannot redefine default arguments. From [dcl.fct.default], slightly abridging the example:

  A default argument shall not be redefined by a later declaration (not even to the same value). [ Example:

void m() {
    void f(int, int);     // has no defaults
    f(4);                 // error: wrong number of arguments
    void f(int, int = 5); // OK
    f(4);                 // OK, calls f(4, 5);
    void f(int, int = 5); // error: cannot redefine, even to
                          // same value
}

  —end example ]

So the definition of get_input should just be:

template <typename TInput, typename TPrompt>
TInput easy_input::get_input(
    const TPrompt& prompt,
    std::istream& input_stream)
{ ... }

Otherwise, the code is ill-formed and should not compile (though apparently earlier versions of gcc happily allow this).

Include guards AND pragma

Given that you have include guards, it's unnecessary to additionally have the conditional #pragma once. If you don't want to unconditionally use the pragma, just stick with the include guards.

Default Prompt and Error

You use the error message in a single place - it'd be better to just write out the string literal there. It'll be easier to find when grepping through code, and there's no advantage I see in having the global constant.

With the default prompt, rather than all the additional complexity of default arguments, I think it'd be simpler to just add an overload:

template <typename TInput>
TInput get_input(std::istream& input_stream = std::cin);

template <typename TInput, typename TPrompt>
TInput get_input(const TPrompt& prompt,
    std::istream& input_stream = std::cin)
{
    std::cout << prompt;
    return get_input<TInput>(input_stream);
}

Ultimately though...

My biggest takeaway from 5gon12eder's excellent answer to your original question was that there really isn't anything easy about this input, and it may just be easier to avoid it and stick to being explicit with the streams.

Here's something this solution can't handle. Let's say I want to input two ints. I could write:

int a, b;
std::cout << "$";
std::cin >> a >> b;

And end up doing something like:

$42 15

And that works. But if I tried to do:

int a = get_input<int>("$");
int b = get_input<int>();

and tried to input both on the same line... I'd get a lexical error since "42 15" isn't an int. Maybe you're OK with that? But it's limiting and sadly not easy. Then again, nothing IO related in C++ is really easy anyway.


Calculating the most profit from an array of stock prices I'm a newbie learning how to code, and I am trying to find the most efficient way to calculate the most profit from an array of stock prices. For a given array,I have a list of stock prices over n days. So for {1, 4, 5, 100}, my maxProfit would be 99 (100-1). The rule for maxProfit would be the greatest difference between two numbers in the Array where you cannot count backwards in the array.

I'm able to get the right solution, but it's not the most efficient. I'm striving for my solution to be \$O(n)\$. At the moment given that single loop and then the Max function, my solution is n^2 because there are two loops. Is there a way I can break it down to just one?

var stockPrices = new List<int>() { 30, 50, 3, 2, 1, 1, 10, 25, 2, 10 };

var maxProfit = 0;
for(int x = 0; x < stockPrices.ToArray().Length; x++)
{
    int maxValue = stockPrices.Max();
    int maxIndex = stockPrices.IndexOf(maxValue);

    var allValuesToTheLeftOftheMax = stockPrices.GetRange(0, maxIndex);

    //Find the min value
    if (maxIndex == 0)
    {
        stockPrices.Remove(maxValue);
    }
    else
    {
        int minValue = allValuesToTheLeftOftheMax.Min();

        //Get the max profit
        if (maxProfit < maxValue - minValue)
        {
            maxProfit = maxValue - minValue;
        }

        stockPrices.Remove(maxValue);
    }
    Console.WriteLine("Max profit currently is {0}", maxProfit);
}

Console.WriteLine("Final max profit is {0}", maxProfit);
Console.ReadLine();

The following implementation will be \$O(n)\$.

public static int getMaxProfit(int[] stockPricesYesterday) {
    // make sure we have at least 2 prices
    if (stockPricesYesterday.length < 2) {
        throw new IllegalArgumentException("Getting a profit requires at least 2 prices");
    }

    // we'll greedily update minPrice and maxProfit, so we initialize
    // them to the first price and the first possible profit
    int minPrice = stockPricesYesterday[0];
    int maxProfit = stockPricesYesterday[1] - stockPricesYesterday[0];

    // start at the second (index 1) time
    // we can't sell at the first time, since we must buy first,
    // and we can't buy and sell at the same time!
    // if we started at index 0, we'd try to buy *and* sell at time 0.
    // this would give a profit of 0, which is a problem if our
    // maxProfit is supposed to be *negative*--we'd return 0.
    for (int i = 1; i < stockPricesYesterday.length; i++) {
        int currentPrice = stockPricesYesterday[i];

        // see what our profit would be if we bought at the
        // min price and sold at the current price
        int potentialProfit = currentPrice - minPrice;

        // update maxProfit if we can do better
        maxProfit = Math.max(maxProfit, potentialProfit);

        // update minPrice so it's always
        // the lowest price we've seen so far
        minPrice = Math.min(minPrice, currentPrice);
    }

    return maxProfit;
}


Properly create and customise a web app using visual studio 2015 C# MVC template I'm new to C# web, I want to know how to properly create a web app using
visual studio 2015 MVC templates, and being able to customise some of its code. I want to both practice C# web and clean code...
So I have 2 projects, one for Domain and another which is the Web App.

For example: In my Domain I have a class named User, with the following code (It is aimed at users who speak Spanish so error messages are in Spanish):

using System.ComponentModel;
using System.ComponentModel.DataAnnotations;
using System.ComponentModel.DataAnnotations.Schema;

namespace Domain
{
    public class User
    {
        public int Id { get; set; }

        [DisplayName("Nombre de Usuario")]
        [Required(ErrorMessage = "El Nombre de Usuario es obligatorio.")]
        [StringLength(30)]
        [Index(IsUnique = true)]
        public string UserName { get; set; }

        [DisplayName("Correo Electrónico")]
        [Required(ErrorMessage = "El Correo Electrónico es obligatorio.")]
        [StringLength(45)]
        [EmailAddress]
        [Index(IsUnique = true)]
        public string Email { get; set; }

        [DisplayName("Contraseña")]
        [Required(ErrorMessage = "La Contraseña es obligatoria.")]
        [StringLength(100, ErrorMessage = "El número de caracteres de {0} debe ser al menos {2}.", MinimumLength = 6)]
        [DataType(DataType.Password)]
        public string Password { get; set; }

        [Display(Name = "Confirmar Contraseña")]
        [DataType(DataType.Password)]
        [NotMapped]
        [Compare("Password", ErrorMessage = "La contraseña y la contraseña de confirmación no coinciden.")]
        public string ConfirmPassword { get; set; }

        public bool Locked { get; set; } = false;

        public bool ConfirmedAccount { get; set; } = false;

        [DisplayName("Perfil")]
        [Required(ErrorMessage = "El Perfil es obligatorio.")]
        public Profile Profile { get; set; }

        private User() { }

        public User(string userName, string email, string password, string confirmPassword, Profile profile)
        {
            UserName = userName;
            Email = email;
            Password = password;
            ConfirmPassword = confirmPassword;
            Profile = profile;
        }
    }
}

Before continuing, any suggestions about this code will be very welcome!

I want to use ResetPasswordViewModel, ForgotPasswordViewModel, ExternalLoginConfirmationViewModel, and many other functionalities that come with MVC templates (you can see them inside AccountViewModels.cs) without programming them myself (that is the idea of using a template isn't it?), but I also want to have my own User class...

Reading the template code, I've found that ApplicationUser class inside IdentityModels.cs would have the logic for User the template applies, It also has a commentary telling me that if I want any other User properties added, they should be added here. But I don't think it would be a good idea to have Business bound code inside the web app, what if (hypothetically speaking) tomorrow I want to make the same application on another platform? (and by the way, why does the template encourage to have the business code inside the web app in the first place??)

I hope you get the idea by now, how to use my own User code to work properly with the MVC template? For example to work along with the class AccountViewModels.cs.

I know those are a lots of questions (I've got plenty more)... As I said, I'm new to this and want to get the hand of it, any suggestions on how to improve my code is welcome.

Regards

The short answer to what you are trying to do is; let the Identity classes handle Authentication and User management. You can put your User into a Model class of it's own. Then reference that from the ApplicationUser class that comes with MVC.

Another approach would be to extend the ApplicationUser class itself by adding the additional properties for your profile.

As for Business Logic and N-Tier architecture; put all that aside for the time being. The MVC architecture Semantic approach and Entity Framework kind of help to handle that stuff organically.

I would suggest going through the free training at the Microsoft Video Academy website. That should clear things up for you.
There's also a class specifically for Entity Framework.

As for the class you submitted. It looks fine. You'll probably want to revisit all the annotations learning more about Entity Framework, depending on your use case..

MVC

Identity Resources


Load JSON from server using jQuery and display it in html5 I am just starting with JavaScript and jQuery, so I appreciate your help!

I want to read items from a JSON file like this:

{
  "items": [
    "this",
    "that"
  ]
}

My code (inspired by a number of stackoverflow and jQuery reference example code) looks like this:

    <!DOCTYPE html>

<meta charset="utf-8">
<title></title>

And it works. Yay.

My questions:

Is that more or less ok what I nitted? What can be enhanced?
Why does the commented-out section not work? I took this from https://api.jquery.com/jQuery.getJSON/

Some ideas:

#1- I suggest using .map().join() or even better .reduce() which is more semantically correct for what you want to do: reduce a array to an html string.

#2 - You should also use var jsonResult = {items: []}; otherwise if the catch gets called you will have a error down the line because you expect  jsonResult to be an Object.

#3 - Another thing: I suppose your example is simplified so much it became incorrect, since the items array has strings, and not objects with key and val...

{
  "items": [
    "this",
    "that"
  ]
}

#4- You did have an error in your code, you were using function( key, val ) {, but that is not the jQuery API. The callback gets the iterated item as this and in the second argument. The first argument is the index...

You could use a callback with destructuring like this:

 var items = []
 $.each(jsonResult['items'], function(i, {key, val}){
    items.push( "
" + val + "
" );
 });

Improved code suggestion:

$.get('list.json', {}, function(data, response) {
  var jsonResult = {items: []};
  try {
    jsonResult = JSON.parse(data);
  } catch (e) {
    $('div#result').html('cannot load data because: "' + e + '"');
  };

  var html = jsonResult['items'].reduce(function(string, item) {
    return string + "
" + item.val + "
"
  }, '');

  $('ul#items').html(html);

});

#5 - About your commented code question:

You are trying to use .join() on objects. It does work if you fix what I suggested above(jsFiddle).


Generating a maximal geometric progression of integers I'm working on a program that generates a maximal geometric progression of at least 3 integers when given a number that the greatest value shouldn't exceed.

My current code works, and it's quick(ish) with small numbers, but anything past 100 takes far longer than I'd like, as I need to calculate numbers in ranges from \$10^7\$ to \$10^{20}\$.

If possible, I'd like to eliminate using Threads, as Python's are known for their lackluster performance.

from itertools import izip,islice,tee,takewhile
from threading import Thread
from Queue import Queue

#initialize the queue
g=Queue()

#checks that all values end with `.0`
def int_check(l):
    for i in l:
        if i-int(i)!=0: return False
    return True

#checks if a generated list fits the criteria
def check(obj):
    cpy0,cpy1=tee(obj)
    iter0,iter1=tee(float(x)/y for x,y in izip(cpy0,islice(cpy1,1,None)))
    return all(x==y for x,y in izip(iter0,islice(iter1,1,None))) and len(obj)>2 and int_check(obj)

#thread target
def calc(s,n,q):
    for r in xrange(125,n*100+1,5):
        t=[s]
        while t[-1]<=n:
            t.extend([t[-1]*((0.0+r)/100)])

        if check(t):
            q.put(takewhile(lambda i: i<=n,t))

#starts threads, calculates final value, and manages queue
def geo_prog(n):
    #Queue -> list
    def dump_queue(q):
        q.put('EOQ')
        res=[]
        return (i for i in iter(q.get,'EOQ'))

    for s in xrange(int(n/4),int(n*0.8)+1):
        t=Thread(target=calc,args=(s,n,g))
        t.start()
        t.join()

    return max(map(sum,dump_queue(g)))

#ex usage
print geo_prog(100)

Examples

geo_prog(4) -> 7.0
geo_prog(10) -> 15.0
geo_prog(12) -> 21.0
geo_prog(100) -> 240.0

No threading, please!!

First of all, get rid of the threads all-together. That should speed things up: Python threads are (sort-of) fine when things are blocking on IO, but in this case everything is CPU-based, and that means the GIL (global interpreter lock) makes sure that only one thread is running.

Also, the following code is a bit curious:

for s in xrange(int(n/4),int(n*0.8)+1):
    t=Thread(target=calc,args=(s,n,g))
    t.start()
    t.join()

I know that threading is a bit of a problem anyway here, but I would just like to comment on this specifically: by writing t.join() you are waiting on the thread to finish before starting the next iteration. This gets read of the threading all together, as there will only be two threads alive at a given time: 1 worker thread, and the main thread waiting on the worker to die.

I'd suggest writing it as:

threads = []
for s in iterable:
    t = Thread(...)
    threads.append(t)
    t.start()

for t in threads:
    t.join()

Naming and describing

Ok, with threading out of the way, I'm having a difficult time finding what the application is supposed to do.

It does something with an n, and each worker with an s and an n. What are those? The code could be a lot clearer in this regard.

Anyway, let us clean things up a bit first, and maybe I can find a pattern to help you improve the algorithm.

Performance

Checking for integer-valued lists.

Let's start looking at int_check.

def int_check(l):
    for i in l:
        if i-int(i)!=0: return False
    return True

You're checking if a number is integral by doing a lengthy calculation. Why not just

if i.is_integer(): return False

? Available since Python 2.6. https://docs.python.org/2/library/stdtypes.html#float.is_integer

Also, looping is expensive! Suggestion, use a comprehension together with the all.

def int_check(l):
    return all(i.is_integer() for i in l)

You should also name it better:

def is_integer_list(l):
    return all(i.is_integer() for i in l)

Useless duplication of lists

Let's look at the following code:

def check(obj):
    cpy0,cpy1=tee(obj)
    iter0,iter1=tee(float(x)/y for x,y in izip(cpy0,islice(cpy1,1,None)))
    return all(x==y for x,y in izip(iter0,islice(iter1,1,None))) and len(obj)>2 and int_check(obj)

First of all, let us do some short-circuiting. You're doing a lot of logic to see if it is a nice geometric progression, and after all the hard work, you check some very easy conditions. Turn that around!

def check(obj):
    if len(obj) <= 2 or not int_check(obj):
        return False
    cpy0,cpy1=tee(obj)
    iter0,iter1=tee(float(x)/y for x,y in izip(cpy0,islice(cpy1,1,None)))
    return all(x==y for x,y in izip(iter0,islice(iter1,1,None)))

I'm doubtful of you using tee for this. But I don't see a better builtin for it either. But I see a lot of logic here which I'd like to separate a bit more:

def steps(iterable):  # Needs better name, please help me!
    iterator = iter(iterable)  # Cast iterable to iterator.
    cur = next(iterator)
    for val in iterator:
        yield cur, val
        cur = val

A quick test to see that it does what we want.

>>> list(steps(range(10)))
[(0, 1), (1, 2), (2, 3), (3, 4), (4, 5), (5, 6), (6, 7), (7, 8), (8, 9)]

Using that, we get

def check(obj):
    if len(obj) <= 2 or not int_check(obj):
        return False
    ratios = (float(x)/y for x, y in steps(obj))
    return all(x == y for x, y in steps(ratios))

Don't know if it's faster, but at least it is more readable! And I expect faster. But the main gain might be in the next section

The main loop

Your code was written as

def calc(s,n,q):
    for r in xrange(125,n*100+1,5):
        t=[s]
        while t[-1]<=n:
            t.extend([t[-1]*((0.0+r)/100)])

        if check(t):
            q.put(takewhile(lambda i: i<=n,t))

Since I want to get rid of the threading, I'll assume q is a list instead of a queue.

def calc(s,n,q):
    for r in xrange(125,n*100+1,5):
        t=[s]
        while t[-1]<=n:
            t.extend([t[-1]*((0.0+r)/100)])

        if check(t):
            q.append(takewhile(lambda i: i<=n,t))

(In reality I'd rather yield the results instead of adding to a queue. Anyhow, I'm shelving that for now)

takewhile vs slicing.

In the q.put you write takewhile. By the while loop above, you already know that will be all but the last value.

So, write

q.append(t[:-1])

Or, even

t.pop()
q.append(t)

That should be faster, because now the lambda i: i <= n doesn't have to be executed anymore.

Useless recalculation

Having written the above loop now as

def calc(s,n,q):
    for r in xrange(125,n*100+1,5):
        t=[s]
        while t[-1]<=n:
            t.extend([t[-1]*((0.0+r)/100)])

        if check(t):
            t.pop()
            q.append(t)

We can turn our focus to the expression t.extend(...) above:

    for r in xrange(125,n*100+1,5):
        t=[s]
        while t[-1]<=n:
            t.extend([t[-1]*((0.0+r)/100)])

Now in each iteration of the while loop, (0.0+r)/100 gets recalculated. Why not

    for r in xrange(125, n*100+1,5):
        r_ = float(r) / 100
        t=[s]
        while t[-1]<=n:
            t.extend([t[-1]*r_)

Incorrect algorithm

The worst part, I think, is that your code works by trying different ratios, and will miss some geometric sequences.

For instance, the sequence

81, 90, 100  # r == 100.0 / 90.0 == 90.0 / 81.0 (approx 1.1111111)

While your code will only try 1.25, 1.30, 1.35, ... .

Now, maybe there is some high-level mathematical proof that the solution will always involve a ratio which can be expressed as (125 + 5*k)/100, but I don't think so.

I would suggest turning the problem up-side-down. The following is an O(n^2 * log(n)) suggestion (estimated, but I think correct. Maybe even O(n^2)).

for x1 in xrange(1, n):
    for x2 in xrange(x1, n):
        # Using that
        #     x1 == a
        #     x2 == a * r
        # we get that
        #     r == x2 / x1
        ratio = float(x2) / x1
        # Continue as before, starting from t = [x1, x2]

Another point of incorrectness

In check, you check that the length is > 2, but then you filter on <=n. This should be the other way around.

A tiny suggestion.

Floating point arithmetic is challenging for problems like these. Maybe look into the fractions module?

from fractions import Fraction

...
ratio = Fraction(x2, x1)
...

Now, Fraction does not have an is_integer, but you can do the following:

>>> Fraction(5, 1).denominator == 1
True

Hope that all helps.

Corner cases

As an addendum, here are some cases your code flagged incorrectly:

>>> geom_prog(16)
28.0  # 4 + 8 + 16

but it should be at least 37: 9 + 12 + 16.

>>> geom_prog(100)
244.0  # 64 + 80 + 100

but it should be at least 271: 81 + 90 + 100 (maybe even more, but doubt it).


Check the security of my image upload script I am writing a basic image upload script in PHP, and am looking for critiques. The script below is the result of various suggestions I have found online... for now I am running it locally and it works fine, but I would love for someone more experienced to look over it.

Is this secure? Poorly implemented? Too redundant? Not redundant enough? What am I missing and/or doing incorrectly? Any suggestions/help/advice is GREATLY appreciated. Thank you.

PHP:

if (isset($_FILES['image']))
{
    $files_array = $_FILES['image'];
    $num_of_image_uploads = count($files_array['name']);

    // Loop through array of file uploads
    for ($i = 0; $i < $num_of_image_uploads; $i++)
    {
        // Check if there is a file to upload
        // If so -> define, validate, and upload
        if ($files_array['tmp_name'][$i] != "")
        {
            $errors = false;    // Flag for errors
            $allowed_ext = array('jpg', 'jpeg', 'png', 'gif');

            $file_name = $files_array['name'][$i];
            $file_ext = strtolower(end(explode('.', $file_name)));
            $file_size = $files_array['size'][$i];
            $file_tmp = $files_array['tmp_name'][$i];

            // Extension check 1
            if (in_array($file_ext, $allowed_ext) === false)
            {
                echo "Extension not allowed.
";
                $errors = true;
            }

            // Extension check 2 - if file is not empty
            if ($file_size == 0)
            {
                echo "File is empty.
";
                $errors = true;
            }
            else if (!getimagesize($file_tmp))
            {
                echo "Not an image.
";
                $errors = true;
            }

            // Extension check 3
            $blacklist = array(".php", ".phtml", ".php3", ".php4");
            foreach ($blacklist as $item)
            {
                if(preg_match("/$item\$/i", $file_name))
                {
                    echo "File not allowed.
";
                    $errors = true;
                }
            }

            // File size check
            if ($file_size > 4718592)
            {
                echo "File size must be under 4.5MB.
";
                $errors = true;
            }

            // Rename file
            $file_name = preg_replace("/[^A-Z0-9_]/i", "_", $file_name);
            $file_name = md5($i.$file_name.time());

            // Insert if no errors
            if ($errors == false)
            {
                if (move_uploaded_file($file_tmp, 'uploads/'.$file_name.'.'.$file_ext))
                {
                    echo "Success! File uploaded.
";
                }
            }
        }
    }
}

HTML:

  <form enctype="multipart/form-data" action="" method="post">

    <input name="image[]" type="file" />
    <input name="image[]" type="file" />
    <input name="image[]" type="file" />
    <input name="image[]" type="file" />

    <input type="submit" value="Upload" />

  </form>

Your script has one underlying assumption that invalidates the whole set of security checks: That the value of $files_array['name'] is safe - it is not. This value is user supplied, and can very easily be spoofed to pass malicious data into your server that would get through all your security checks.

Neither $_FILES['control']['name'] nor $_FILES['control']['type'] are safe and you should not use either value for anything, ever.

In order to make your image uploads as safe as possible you must do all of the following:

Verify that the data you are working with is an uploaded file (move_uploaded_file() / is_uploaded_file() will take care of this for you)
Create the file name used to store the file on the file system entirely dynamically. You cannot use user input in any way to generate this. If your application requires that you store the original file name, you should keep this association in an external data store such as a database.
Verify that the image really is an image. The GD extension can verify the headers for you (use the getimagesize() function).
Copy the pixel data out of the image into a new one. Again, GD can do this for you (imagecopyresampled()). It is important to do this - it is possible to hide malicious code inside something with headers that make it appear to be a valid image. Resampling the image will destroy such malicious code.
I recommend that if you store the image on the file system, store it outside the document root and proxy the retrieval process with a script, so it cannot be directly accessed via the web server. The above measures should mitigate the implied security risk here, but better safe than sorry ;-)
Never include/require a file uploaded by a user. If you want to provide a file download, always use filesystem functions. readfile() is often the most appropriate choice.

Unfortunately, in order to comply with all these requirements you are basically going to need to throw your code out and start again.

If you want something for reference, this Gist contains something that's not a million miles away from the code I use in my real-life applications, and contains a basic usage example.


Active record pattern for a subscription service website I just jumped to a new project, which uses the active record pattern. It's a subscription service website.

I have the following User object that extends a framework specific ORM object:

class User extends ORM {

    public function isSubscriber()
    {

        $activeSubscription = $this->subscriptions->where('active','=',1)->find_all();
        return $activeSubscription->count();

    }

}

In client code, I have the following code:

class Test_Controller extends Controller {

    public function test()
    {

        // Framework speific way to get a current user
        // returns User orm
        $user = Auth::instance()->get_user();

        var_dump($user->isSubscriber());

    }

}

As you see, it's very straightforward and easy to understand. What's bothering me is that there's no way to test this simple User object unless I hack it.

The problem is that inside ORM objects, it uses a singleton pattern (something like Database::getInstance()) to get a database object and uses it to query to database, making it impossible to properly mock it.

I extracted isSubscriber() from User to a service like this:

class SubscriberVerificationService {

    public function __construct(SubscriptionRepositoryInterface $subscriptionRepository)
    {

        $this->subscriptionRepository = $subscriptionRepository;

    }

    public function isSubscriber(User $user)
    {

        $subscriptions = $this->subscriptionRepository->findActiveSubscriptionsByUserId($user->id);
        return count($subscriptions);

    }

}

Client code

class Test_Controller {

    public function __construct(SubscriberVerificationService $subscriberVerificationService)
    {
        $this->subscriberVerificationService = $subscriberVerificationService;
    }

    public function test()
    {

        $user = Auth::instance()->get_user();
        var_dump($this->subscriberVerificationService->isSubscriber($user));

    }

}

Now I can properly mock SubscriptionRepositoryInterface and and User objects in my test.

I feel like this is the way to go, but I just want to hear advice from people who use the active record pattern day-to-day.

It doesn't have to be a repository (SubscriptionRepositoryInterface) in the service. It can be a Subscription ORM as long as I can mock it.

Avoid hacking the code, if possible. Instead, you should have a test environment with a testing database. Database::getInstance() should use a configuration file containing the database connection parameters. It should configure itself to use the testing database when running unit tests.


Tuning a query for performance I am running this query and it takes several minutes to complete.  How can I tune it (unfortunately I don't have the privileges to run an SQL tuning advisor)?

select 'Master' || d.ntt_id || '_' || d.deal_nummas, p.prof_currency,
sum(p.prof_ts1), sum(p.prof_ts2), sum(p.prof_ts3), sum(p.prof_ts4), sum(p.prof_ts5),
sum(p.prof_ts6), sum(p.prof_ts7), sum(p.prof_ts8), sum(p.prof_ts9), sum(p.prof_ts10),
sum(p.prof_ts11), sum(p.prof_ts12), sum(p.prof_ts13), sum(p.prof_ts14), sum(p.prof_ts15),
sum(p.prof_ts16), sum(p.prof_ts17), sum(p.prof_ts18), sum(p.prof_ts19), sum(p.prof_ts20),
sum(p.prof_ts21), sum(p.prof_ts22), sum(p.prof_ts23), sum(p.prof_ts24), sum(p.prof_ts25),
sum(p.prof_ts26), sum(p.prof_ts27), sum(p.prof_ts28), sum(p.prof_ts29), sum(p.prof_ts30),
sum(p.prof_ts31), sum(p.prof_ts32), sum(p.prof_ts33), sum(p.prof_ts34), sum(p.prof_ts35),
sum(p.prof_ts36), sum(p.prof_ts37), sum(p.prof_ts38), sum(p.prof_ts39), sum(p.prof_ts40),
sum(p.prof_ts41), sum(p.prof_ts42), sum(p.prof_ts43), sum(p.prof_ts44), sum(p.prof_ts45),
sum(p.prof_ts46), sum(p.prof_ts47), sum(p.prof_ts48), sum(p.prof_ts49), sum(p.prof_ts50),
sum(p.prof_ts51), sum(p.prof_ts52), sum(p.prof_ts53), sum(p.prof_ts54), sum(p.prof_ts55),
sum(p.prof_ts56), sum(p.prof_ts57) from profiles p
join deals d on d.deal_id = p.deal_id and d.deal_scope='Y'
join runs r on r.deal_cnt_id = d.deal_cnt_id
where r.run_id=7 and d.deal_nummas!=0 and d.deal_nummas is not null
group by d.ntt_id, d.deal_nummas, p.prof_currency;

Some explanation on what I am doing:

I want to compute the sum on each timestep of a deal's profile, with deals being grouped by the node they belong to (identified by their deal_nummas). I only take into account deals which are in scope, and in the specified run.

The execution plan:

SELECT STATEMENT                                                                                3540470
    |_ HASH                                                                 GROUP BY            3540470
        |_ HASH JOIN                                                                            2957025
        |   |_ Access Predicates
        |   |   |_ D.DEAL_ID = P.DEAL_ID
        |   |_ HASH JOIN                                                                        624679
        |       |_ Access Predicates
        |       |   |_ R.DEAL_CNT_ID = D.DEAL_CNT_ID
        |       |_ TABLE ACCESS                                 RUNS        BY INDEX ROWID      1
        |       |   |_ INDEX                                    PK_RUN_ID   UNIQUE SCAN         0
        |       |       |_ Access Predicates
        |       |           |_ R.RUN_ID = 7
        |       |_ TABLE ACCESS                                 DEALS       FULL                624091
        |           |_ Filter Predicates
        |               |_ AND
        |                   |_ D.DEAL_SCOPE = 'Y'
        |                   |_ TO_NUMBER(D.DEAL_NUMMAS) <> 0
        |                   |_ D.DEAL_NUMMAS IS NOT NULL
        |_ TABLE ACCESS                                         PROFILES    FULL                922142

The table profiles stores what we could call curves, each with an identifier and 57 points (y-axis values), the columns being the x-axis values.

I'll echo what 200_success said about the schema. But working with what you have, I would think breaking it down into two steps (via temp table if this is a one-time aggregation, or via a view if repeat query) may improve performance. Then just perform your SUM() operations on the temp table/view. Preferably though, I would think a better organized schema would be more useful, but this may not be an option in your situation. I would also use a more explicit JOIN to reduce the amount of guessing the SQL engine has to do. I'm less familiar with what seems to be Oracle syntax but here is my take on it based on SQL Server syntax. Anyone else feel free to correct any Oracle-related syntax errors I make.

-- Create temp table, leaving out group by clause
select || d.ntt_id || '_' || d.deal_nummas, p.*
INTO #tmp_nummas
from profiles p
inner join deals d on d.deal_id = p.deal_id and d.deal_scope='Y'
inner join runs r on r.deal_cnt_id = d.deal_cnt_id
where r.run_id=7 and d.deal_nummas!=0 and d.deal_nummas is not null;

-- Perform operation
select 'Master' ntt_id || '_' || deal_nummas, prof_currency
sum(prof_ts1), sum(prof_ts2), sum(prof_ts3), sum(prof_ts4), sum(prof_ts5),
sum(prof_ts6), sum(prof_ts7), sum(prof_ts8), sum(prof_ts9), sum(prof_ts10),
sum(prof_ts11), sum(prof_ts12), sum(prof_ts13), sum(prof_ts14), sum(prof_ts15),
sum(prof_ts16), sum(prof_ts17), sum(prof_ts18), sum(prof_ts19), sum(prof_ts20),
sum(prof_ts21), sum(prof_ts22), sum(prof_ts23), sum(prof_ts24), sum(prof_ts25),
sum(prof_ts26), sum(prof_ts27), sum(prof_ts28), sum(prof_ts29), sum(prof_ts30),
sum(prof_ts31), sum(prof_ts32), sum(prof_ts33), sum(prof_ts34), sum(prof_ts35),
sum(prof_ts36), sum(prof_ts37), sum(prof_ts38), sum(prof_ts39), sum(prof_ts40),
sum(prof_ts41), sum(prof_ts42), sum(prof_ts43), sum(prof_ts44), sum(prof_ts45),
sum(prof_ts46), sum(prof_ts47), sum(prof_ts48), sum(prof_ts49), sum(prof_ts50),
sum(prof_ts51), sum(prof_ts52), sum(prof_ts53), sum(prof_ts54), sum(prof_ts55),
sum(prof_ts56), sum(prof_ts57)
from #tmp_nummas
group by ntt_id, deal_nummas, prof_currency;


Convert string to mixed case This method converts a string to mixed case, id est mixedCase("hello, world!") -> "HeLlO, wOrLd!".

// Any input string is lower case already
private static String mixedCase(String input) {

    // getAsCharacterList() simply turns a string into a List of Characters.
    List<Character> charactersAsList = getAsCharacterList(input);
    StringBuilder result = new StringBuilder();
    int index = 0;

    for (Character character : charactersAsList) {
        if (Character.isLetter(character)) {
            if (index % 2 == 0) {
                result.append(Character.toUpperCase(character));
            } else {
                result.append(character);
            }
            index++;
        } else {
            result.append(character);
        }
    }

    return result.toString();
}

It works, but admittedly it looks terrible and I have difficulties making it more concise. I would have preferred to use streams if I didn't had to reference indices...

Is there a more concise way to write this Java string converter?

Sure. No need for the intermediary charactersAsList variable, nor the getAsCharacterList method.

Since you know in advance the final length of the StringBuilder,
it's good to pass that to the constructor when initializing.

// input is already lowercase
private static String mixedCase(String input) {
    StringBuilder result = new StringBuilder(input.length());
    int index = 0;

    for (char character : input.toCharArray()) {
        if (Character.isLetter(character)) {
            if (index % 2 == 0) {
                result.append(Character.toUpperCase(character));
            } else {
                result.append(character);
            }
            index++;
        } else {
            result.append(character);
        }
    }

    return result.toString();
}

Borrowing from the idea of @BrainFRZ to set the already lowercased input in StringBuilder, and from @ferada's comment to use a boolean to decide to uppercase or not, here's another variation that has some advantages:

private static String mixedCase(String input) {
    StringBuilder result = new StringBuilder(input);
    int index = 0;
    boolean toUpper = true;

    for (char character : input.toCharArray()) {
        if (Character.isLetter(character)) {
            if (toUpper) {
                character = Character.toUpperCase(character);
            }
            toUpper = !toUpper;
        }
        result.setCharAt(index, character);
        index++;
    }

    return result.toString();
}

This takes advantage of the input being already lowercased,
and naturally letting StringBuilder to use that.
With some of the conditions eliminated, this version is slightly more compact.


Counting consecutive selections of a given number when sampling integers in a range This script counts consecutive number strikes of a number that you determine in a number-sequence that you also determine.

I would be very pleased if this script can be reviewed in terms of compactness, readability and quality. If this is perfect, please also let me know. But I am shure it will not be.

The script can be tested here

import random

desired_number = int(input("Desired number: "))
lower_range = int(input("Lower range: "))
upper_range = int(input("Upper range: "))
iterations = int(input("Iterations: "))

consecutive_strikes = 0
biggest_strike = 0

for i in range(iterations):
  actual_number = random.randint(lower_range, upper_range)
  print("Actual number: ", actual_number)

  if actual_number == desired_number:
    consecutive_strikes += 1
  else:
    if (consecutive_strikes > biggest_strike):
      biggest_strike = consecutive_strikes
    consecutive_strikes = 0

# if all numbers are the desired number, the second if-statement in the for-loop
# would never be executed
if (consecutive_strikes > biggest_strike):
  biggest_strike = consecutive_strikes

print("Biggest consecutive strike: ", biggest_strike)

You can use the itertools for this.

Just groupby the value to get groups of equal values, filter for the desired value, then get the length of each streak and finally take the max of that length:

from itertools import groupby

def longest_streak(values, desired_value):
    return max(len(list(group))
               for value, group in groupby(values)
               if value == desired_value)

(I think it should be streak, 4. b: "an uninterrupted series" and not strike.)

Then your main can code become this:

from random import randint

if __name__ == "__main__":
    desired_number = int(input("Desired number: "))
    lower_range = int(input("Lower range: "))
    upper_range = int(input("Upper range: "))
    iterations = int(input("Iterations: "))

    numbers = (randint(lower_range, upper_range) for _ in range(iterations))
    print("Biggest consecutive strike: ", longest_streak(numbers, desired_number))

Here numbers is a generator, so the function can just consume the numbers as they are generated (meaning that this will occupy at most len(group) space in memory).

if __name__ == "__main__": is a guard so that the code under it is only executed when directly executing this script, but not when importing from it.


Plotting the Koch Snowflake The Koch snowflake is a well known fractal. Beginning with an equilateral triangle, a smaller equilateral triangle is placed halfway along each edge of the shape. This process then repeats on each edge of the new shape. Here is my attempt to plot this in MATLAB.

clc, clearvars, close all;

length = 1; % Original side length of triangle
theta = 60*pi/180; % Angle of the equilateral triangle in radians

% Points of the original equilateral triangle
p1 = [0,0];
p2 = [1/2*length,length*sin(theta)];
p3 = [length,0];

flake_points = [p1; p2; p3]; % Vertices of the snowflake

for iteration = 1:7
    n_points = size(flake_points,1); % No. points currently in data
    new_triangle_points = nan(n_points*3,2); % New points to be added
    for i = 1:n_points
        p = flake_points(i,:);
        if i == n_points
            vector = flake_points(1,:)-flake_points(i,:);
        else
            % Vector between next point and current point
            vector = flake_points(i+1,:)-flake_points(i,:);
        end
        b1 = p + 1/3*vector; % Base point 1 of the new triangle
        b2 = p + 2/3*vector; % Base point 2 of the new triangle
        top = p + 1/2*vector + perp(vector)*length/3*sin(theta);
        new_triangle_points(3*i-2:3*i,:) = [b1; top; b2];
    end
    flake_points(end+1:end+9,:) = nan(9,2); % Add rows to data
    new_flake_points = nan(n_points*3+n_points,2);
    n = 1;
    for i = 1:n_points
        new_flake_points(n,:) = flake_points(i,:);
        index = 3*i-2;
        new_flake_points(n+1:n+3,:) = new_triangle_points(index:index+2,:);
        n = n + 4; % Three new points have been inserted,
                    % so we now skip to the next empty spaces
    end
    flake_points = new_flake_points;
    length = 1/3*length; % Revise length
end

plot(flake_points(:,1),flake_points(:,2),'w')
hold on; % Plot last data point with 1st (ie, close the loop)
plot([flake_points(1,1),flake_points(end,1)],...
    [flake_points(1,2),flake_points(end,2)],'w')
set(gca,'color',[0.1 0.1 0.1])
axis equal;
str = sprintf('Iteration: %d',iteration);
title(str, 'FontSize', 20);

Output:

Edit - This function is also required:

function p_vec = perp(vec)
% Returns the unit vector p_vec, which is perpendicular to the input
% 2D vector vec
%
% Inputs
%
%   vec : double
%       n_dimensions x 1
%       Input vector
%
% Outputs
%
%   p_vec : double
%       n_dimensions x 1
%       Perpendicular unit vector

p_vec(1) = -vec(2);
p_vec(2) = vec(1);

% Convert to unit vector
p_vec = p_vec / norm(p_vec);

end

Very nice script!

I suggest you put this into a function, that takes n as an input argument instead of hardcoding it. It requires very little alterations, and will make the function a lot easier to use. You can skip all the clearing in the beginning and substitute it with:

function [] = koch_snowflake(iterations)  % or

and add an end at the bottom of your script.

length = 1; % Original side length of triangle

This is a no no. I strongly recommend you not to use length as a variable name as it's a very commonly used builtin function. This can cause a lot of errors that are hard to find. Note that you should always use the function numel instead of length, but it's still not a good idea to overload it.

theta = 60*pi/180; % Angle of the equilateral triangle in radians

I guess you think 60*pi/180 is easier to read than pi/3, but I disagree. If you want to use the number of degrees, and convert it to radians, you might want to use deg2rad.

I suggest substituting the iterator i to for instance ii if you ever work with complex numbers, to avoid potential errors and confusion. i is by default the imaginary unit in MATLAB (so is j, 1i, 1j). This is a matter of preference though.

Also, personal preference again, but I like it when comments are aligned (if possible):

length = 1;                   % Original side length of triangle
theta = 60*pi/180;            % Angle of the equilateral triangle in radians

% Points of the original equilateral triangle
p1 = [0,0];
p2 = [1/2*length,length*sin(theta)];
p3 = [length,0];

flake_points = [p1; p2; p3];  % Vertices of the snowflake

for iteration = 1:7
    n_points = size(flake_points,1);          % No. points currently in data
    new_triangle_points = nan(n_points*3,2);  % New points to be added
...
...


Python random number generator between 1 and 9 I created this code and it works fine, but I think it's ugly. Could you show me a better way to create a list of three different random numbers from 1 to 9?

class Baseball_Engine(object):
    def __init__(self):
        self.count = 0
        self.random_number_list = [0, 0, 0]
        while self.count < 3:
            random_number = random.randint(1, 9)
            self.random_number_list[self.count] = random_number
                if self.random_number_list[self.count - 1] != random_number and self.random_number_list[self.count - 2] != random_number:
                self.count += 1
        print self.random_number_list

Without dwelling too deep into your code there are a few alternatives. After a very quick google search, I stumbled upon this question on SO.

They propose two different solutions.

import random
print random.sample(xrange(1, 10), 3)

This will create three distinct numbers with values ranging from 1 to 9. If you do not want the three numbers to be different, you could try the following

import random
print [random.randrange(1,10) for _ in range(3)]


Filter function implementations using copy_if algorithm vs. for loop I have the following method of some class, which also defines the method isAllowed:

auto filter(const auto& in)
{
    auto ret = decltype(in) {};
    for(auto x : in)
      if(isAllowed(x))
        ret.insert(x);
    return ret;
}

This is a clear case where copy_if could be used instead. I see two alternative versions:

auto filter(const auto& in)
{
    auto ret = decltype(in) {};
    copy_if(begin(in), end(in),
            inserter(ret, end(ret)),
            [this](auto i) {return this->isAllowed(i);});
    return ret;
}

or

auto filter(const auto& in)
{
    auto ret = decltype(in) {};
    copy_if(begin(in), end(in),
            inserter(ret, end(ret)),
            bind(&A::isAllowed, this, placeholders::_1));
    return ret;
}

Both seem much less obvious than the original, so I am inclined to keep the for loop. Is there a strong argument not to? (And is there a better way?)

Otherwise, I feel itchy because cases like this point to the limited usefulness of the algorithm tools, despite what best practices advise.

There are probably good arguments for choosing copy_if that are not being considered properly (performance?). This question is mainly after those, as the others seem more intuitive and easier to get.

I'll respectfully disagree with other answers that suggest using anything but the first option. The only thing the other two std-library-heavy approaches accomplish is to obfuscate the code. Never underestimate the importance of keeping it simple. Complexity is the number one enemy of any piece of software.

The first one is not only more explicit about what is happening, but it is also universal. Any high-level programming language has the concept of a for-loop, so an occasion reader coming from another language background can still clearly see what's going on over there. A JavaScript programmer would have a very hard time figuring out what an inserter or a placeholder is. Even a novice C++ programmer with limited knowledge of the library would have to waste time digging through documentation to understand a simple function that does a conditional copy.

Don't get me wrong here, I'm not making a case against the Standard Library,
I'm just pointing out that, like any programming language feature, it can be overused. In the three examples presented by the OP, the second and third clearly are overusing the std lib just for the sake of using library feature XYZ. My opinion.


Changing the width of a centered progress bar I am making a game in which the player has a certain amount of available boost. To show this, I am using a blue bar inside a black frame. The bar can grow and shrink at different rates as available boost goes up and down. It works well and looks nice, but it almost halves my FPS.

The relevant snippet:

var boostStyle = document.getElementById("available-boost").style;
var currBoost = 100;
var maxBoost = 100;

function setBoost(boostAmount) {
  boostStyle.width = (boostAmount / maxBoost) * 100 + "%";
};

function demonstrateBoostChange() {
  currBoost -= 1;
  setBoost(currBoost);
  if (currBoost < 1) {
    currBoost = maxBoost;
  }
  requestAnimationFrame(demonstrateBoostChange);
}
requestAnimationFrame(demonstrateBoostChange);
#boost-bar {
  bottom: 10px;
  position: absolute;
  right: 20%;
  left: 20%;
  height: 20px;
  border-radius: 12px;
  border-style: solid;
  border-width: 2px;
  border-color: rgb(0, 0, 0);
}

#available-boost {
  margin: 0 auto;
  height: 20px;
  width: 100%;
  text-align: center;
  font-weight: bold;
  border-radius: 10px;
  background-color: rgb(0, 225, 255);
}

Boost

(I see the slight glitch where the text moves when very little boost remains, but I can solve that later. )

The performance hit is probably not noticeable  since it is the only thing running, but I have used Chrome DevTools to see that it is the slowest part of my code. I think this is because changing the width is triggering a reflow.

How can I improve the performance of the above code?

Some considerations:

I am already using a canvas for some other things, would it be faster to just put the bar on that?
The boost can change at different rates, depending on the maxBoost.

You may put your text "Boost" in a seperate element and position it.
Check this:

var inner = document.querySelector(".inner");
var cb = function() {
  var k = 1 - Date.now() % 1500 / 1500;
  inner.style.width = (k * 100).toFixed(2) + "%";
  window.requestAnimationFrame(cb);
};
cb();
.boost {
  border: 2px solid #999;
  width: 300px;
  position: relative;
  font: 16px mono;
}

.boost .inner {
  margin: 0 auto;
  background-color: #999;
}

.boost .text {
  position: absolute;
  text-align: center;
  top: 0;
  left: 0;
  width: 100%;
}

&nbsp;

BOOST

Note how position: absolute; behaves inside position: relative;.

After all I suggest you to drop this part and code it with canvas. Even CSS animations will outperform requestAnimationFrame.

Example with CSS animations:

var inner = document.querySelector(".inner");
inner.addEventListener("animationend", function () {
  // Animation just ended
  inner.parentNode.style = 'display:none;';
})
inner.style.animationDuration = '2s';
inner.style.animationName = 'boost';
.boost {
  border: 2px solid #999;
  width: 300px;
  position: relative;
  font: 16px mono;
}

.boost .inner {
  margin: 0 auto;
  background-color: #999;
  animation-timing-function: linear;
}

.boost .text {
  position: absolute;
  text-align: center;
  top: 0;
  left: 0;
  width: 100%;
}

@keyframes boost {
  0% { width: 100%; }
  100% { width: 0; }
}

&nbsp;

BOOST


Extracting the src attribute and content of  tags in HTML files in a directory I have a program which parse HTML content to get a script src attribute and script attribute. A function have directory (contain HTML files) as input and extract to other directory as output.

However, I think my code is not clean. The following is my code.

def extract_int_js(src, dst):

    for file in os.listdir(src):
        filepath = os.path.join(src, file)
        with open(filepath) as f:
            html = f.read()
        parser = HtmlParser(html)

        for text in parser.script_text:
            yield text

def extract_src_js(src, dst):
    for file in os.listdir(src):
        filepath = os.path.join(src, file)
        with open(filepath) as f:
            html = f.read()
        parser = HtmlParser(html)

        for src in parser.src:
            yield src

Do you have any solution to improve this code since there are duplicated line in 2 functions.

You can use getattr function to merge your two functions into a single one.

def extract_attribute(src, attr_name):
    for file in os.listdir(src):
        filepath = os.path.join(src, file)
    with open(filepath) as f:
        html = f.read()
    parser = HtmlParser(html)

    yield from getattr(parser, attr_name)

Note that in Python 3.3+ there is yield from statement so you can yield values directly from iterable without writing for loop

Now if you still want to have two separate functions instead of passing attribute_name argument each time you can use function partial from functools

from functools import partial

extract_src_js = partial(extract_attribute, attr_name='src')
extract_int_js = partial(extract_attribute, attr_name='script_text')

Please also note that I've removed dst parameter from your functions since it was not used, however if it's just a part of your function you wanted us to review, just return it back.


Finding largest string palindrome I've attempted to write a program to find the largest palindrome in a sentence. Spaces are included and characters are case-sensitive.  Can you suggest better ways of doing this?

//public static void main(String args[])--

String source = "my malayalam is beautiful";
//Find the the largest palindrom in this sentence

StringBuilder sb = new StringBuilder( source);
String p = sb.reverse().toString();
System.out.println(p);
System.out.println(source);
int largest = 0;
try{
    for (int start=0; start <=source.length(); start++ ){
        for (int end=start+2; end < source.length(); end++){
            if (largest < (end-start)){
                String sub = source.substring(start, end);
                if (p.indexOf(sub) > 0){
                    largest = sub.length();
                    System.out.println(sub+" "+largest);
                }
            }
        }
    }
}
catch(Exception e){
    System.out.println("2");
    e.printStackTrace();
}

There are two things that can be criticized here: Your coding style and your algorithm.

Code Style

There are unnecessary spaces inside some parens: new StringBuilder( source), for (…; start++ ){. Make a conscious decision to follow one style and follow it through.
Improve readability by consistently putting a space around most operators like =, -, + and <=.
Never ever catch (Exception e)! If there is a specific exception you would like to catch, then please do, but specify the appropriate class or interface. But often it is better to just let the exception bubble up through the call stack.

Do not catch exceptions you are not interested in actually handling. Do not catch exceptions you have no idea where they would come from. Why are you currently catching an…

… ArithmeticException? Is there any chance e.g. for a division by zero in your code?
… ArrayIndexOutOfBoundsException? Where are you using arrays?
… ArrayStoreException? Where are you storing stuff into an array?
… ClassCastException? Your code does not contain a single cast.
… ClassNotFoundException? You have no dynamic class loading in your code.
… CloneNotSupportedException? You don't call clone on any object.
…

Use good variable names that make it clear what a certain variable means. Shouldn't p be reversed or something like that?
I would not introduce a sb variable for the StringBuilder, but rather:

String reversed = new StringBuilder(source).reverse().toString();

The palindrome finding code is not callable as a method. Extracting code into sensible methods improves testability, and can make it easier to design good programs.

Bugs

Some bounds you choose are rather weird. For example, start <=source.length() should be start < source.length() or even better start <= source.length() - largest.
The indexOf method returns an index on success, and -1 on failure. The snippet if (p.indexOf(sub) > 0) is therefore misleading. Anyway, you should be actually using if (p.contains(sub)).
Your logic just asserts that the source string contains both a substring and the reversed substring. It does not assert that it contains any palindrome. For example, computers return will cause your code to output uter. You would have to check the actual indices, or better: that the substring equals itself reversed.

Algorithm

Your current algorithm works by extracting all possible substrings of length 2 or longer, and then checks whether the substring reversed is contained in the source string. As explained above, this is wrong. It is also quite inefficient, because your algorithm runs in O(n²) (two nested loops).

The problem could be solved in a fraction of the code by a recursive regular expression, e.g. in Perl/PCRE: (.)(?:(?R)|.?)\1. Sadly, the regex implementation of the Java standard library does not support this.

According to the Wikipedia article on Longest Palindromic Substrings, it is possible to find them in linear time. You may want to implement such an existing algorithm.


two or more render/redirect in the same method I often have method like this one with two or more render to do due to catching the error for example.
I currently do something like this:

def update
  @user = current_user
  if params[:user][:camera_type].present? && params[:user][:lens_type].present? && params[:user][:photo].present?
    r = Photo.instance.Upload(camera_type: @user.id,
                              album: @user.default_album,
                              photo: params[:user][:photo],
                              lens_type: params[:user][:lens_type],
                              camera_type: params[:user][:camera_type])
    @user.camera_type = params[:user][:camera_type]
    @user.lens_type = params[:user][:lens_type]
    @user.photo = params[:user][:photo]
    @user.save!
    render action: 'edit'
    return
  end
rescue Photo => e
  redirect_to photo_path, :flash => { :alert => t(:text_photo_edit_failed) }
end

But may be there is a better way to do it ?

Some notes:

Keep your controllers lean, this is too much code for an action.
The photo upload should be done as a part of the object's save chain (after validation), so create a before_save callback in the User model that returns false if the upload fails.
You set r (cryptic name) but don't use it anywhere? does the upload raise an exception on error?

At the end the controller should look extremely simple, something like:

def update
  @user = current_user

  if @user.update_attributes(params[:user])
    render action: 'edit'
  else
    redirect_to photo_path, flash: {alert: t(:text_photo_edit_failed)}
  end
end

Any additional logic should be in User. Models are easy to test in isolation, controllers on the other hand have a lot of state to worry about.