Pascal's triangle for an interview in PHP I saw that one of the interview questions could be building a pascal triangle.
Is there anything wrong with this particular solution I came up with?

function pascal_r($r){

   $local = array();

   if($r == 1){
       return array(array(1));
   } else {
       $previous = pascal_r($r - 1);

       array_push($local, 1);

           for($i = 0; $i < $r - 2 && $r > 2; $i++){
               array_push($local, $previous[$r-2][$i] + $previous[$r-2][$i + 1]);
           }

       array_push($local, 1);
   }

   array_push($previous, $local);

   return $previous;

}

print_r(pascal_r(100));

In regards to the comments: Yes, this is a good place for this so long as it works as is and you are asking about the code and not the algorithm. If you are asking about the algorithm this should have stayed on programmers.SE. If you are asking if this will work, then you should test it first, then if you can't find a solution you should post this to SO. Now, assuming this works and you're not asking about the algorithm, here are my thoughts:

A number of these points may be nitpicking, but the interviewers are likely to nitpick as well since they have so little code to review to determine how good you are. First example of this are your variable and function names, which could stand to be a bit more descriptive. I can guess what "r" might be, but spelling it out wouldn't hurt, especially if that variable might get lost in layers of code. It makes your code "self-documenting", this means less actual documentation and easier future integration.

Unless you have to add a number of elements on to an array at the same time, don't use array_push(). There is no need to call a function to do something that can be done without one. This is also explicitly explained on the PHP documentation for this function should you need further proof.

$local[] = 1;//good

array_push( $local,
    1,
    2,
    //etc...
);//also good

array_push( $local, 1 );//bad

Watch your indentation. That for loop is needlessly indented. Needless whitespace and indentation add "weight" to your documents. Not much at any one time, but a weight that can build up steadily, especially if you are of the faction that uses spaces instead of tabs. This isn't ever likely to cause any real world difficulties, except maybe slightly larger file sizes, but again, I'm nitpicking.

Something else you are going to get tagged on is all of these magic numbers. Where are these 1's and 2's coming from? If those numbers mean something, then you should create constants or variables to define it. Constants if immutable, variables if mutable. If, as in your first return statement you are merely returning the value of $r, as it appears, then you can just typecast it.

if( $r == 1 ) {
    return array( ( array ) $r );
}

Instead of doing the same operation, $r - 2, over and over again, set the value to a variable and use that instead. Its overhead may be slight, and again a nitpick, but it still adds up if done enough. Besides, it makes for much more legible code.

$r2 = $r - 2;//horrible name, don't use this
for( $i = 0; $i < $r2 && $r > 2; $i++ ) {

This next suggestion is similar to one I've already given. Watch your indentation. You have an if/else statement when you could really get away with just an if statement. Since it has an early return, you can drop the else statement, this removes one layer of indentation across six lines of code. In larger systems this could easily add up to quite a bit more and is easily one of the easiest, most visual, refactoring you can do.

The last bit of advice would be to use var_dump() over print_r(). While this may be a matter of preference, I find the type output beneficial, especially in cases where you could somehow get an empty array or FALSE value.

EDIT

Interesting coincidence here, but I'm taking a course on Coursera (Functional Programming in Scala), and one of the things we were required to do was find the value of a given coordinate on a pascal triangle. This assignment reminded me of this post so I thought I'd revisit it, but I wanted to wait until the deadline had been reached before posting this. Below is the code I used to functionally find the value.

Note: I'm using spoilers to give you a chance to work it out. Try and do these yourself before peeking.

      function pascal( $col, $row ) {
        if( $col == 0 || $col == $row ) {
            return 1;
        } else if( $col == 1 || $col == $row ) {
            return $row;
        } else {
            return pascal( $col, $row - 1 ) + pascal( $col - 1, $row - 1 );
        }
    }

Note: I stressed functional, but there are other ways of doing this. I'm merely keeping with my assignment's guidelines.
Note: I know I'm violating some of my own advice. The instructions I was given in my assignment stressed not to use variables, so I'm not.

So, given we can now find a value at any given coordinate, we should easily be able to use this to print a pascal triangle too. All we need is to determine how big we want our triangle to be and we can loop for it. I'm going to use 10, but you can change that amount to any value you want and it will still work. Again, no peeking!

      for( $i = 0; $i < 10; $i++ ) {
        for( $j = 0; $j <= $i; $j++ ) {
            echo pascal( $j, $i ) . ' ';
        }
        echo '<br />';
    }

There you have it. Not only have you printed a pascal triangle, but you've also created a function that can be reused to serve a better purpose, namely finding a coordinate value.


Implementation of IpV4Address For legacy reason (guys here LOVE immutable object), I need a struct that contain an IpV4.

I can't use System.Net.IPAddress because it's not a struct.

So far here what I got:

public struct IpV4Address
{
    readonly byte[] address;

    public IpV4Address(byte[] address)
    {
        Throw.IfArgumentIsNull("address", address);

        if (address.Length != 4)
        {
            throw new ArgumentException("Wrong address length", "address");
        }

        this.address = new byte[4];
        Array.Copy(address, this.address, 4);
    }

    public override string ToString()
    {
        return string.Join(".", address);
    }

    /// <summary>
    ///     Parse a string representing an IPV4.
    ///     <example>xxx.yyy.zzz.www</example>
    /// </summary>
    /// <param name="quadDotted">The string to parse.</param>
    /// <returns>The IPV4</returns>
    public static IpV4Address Parse(string quadDotted)
    {
        Throw.IfArgumentIsNull("quadDotted", quadDotted);

        var bytes = quadDotted.Split('.').Select(byte.Parse).ToArray();

        return new IpV4Address(bytes);
    }

    /// <summary>
    ///     Try parse a string representing an IPV4.
    ///     <example>xxx.yyy.zzz.www</example>
    /// </summary>
    /// <param name="quadDotted">The string to parse.</param>
    /// <param name="result">The IPV4</param>
    /// <returns>A value indicating whether the parse succeed.</returns>
    public static bool TryParse(string quadDotted, out IpV4Address result)
    {
        if (quadDotted == null)
        {
            goto Fail;
        }

        var elements = quadDotted.Split('.');
        if (elements.Length != 4)
        {
            goto Fail;
        }

        var address = new byte[4];
        for (var i = 0; i < 4; i++)
        {
            if (!byte.TryParse(elements[i], out address[i]))
            {
                goto Fail;
            }
        }

        result = new IpV4Address(address);
        return true;

        Fail:
        result = default(IpV4Address);
        return false;
    }

It's the first time I use a goto in a C# code. And I don't feel ashamed !

I find it more readable if you write the method like the following:

static bool TryParse(string quadDotted, out IpV4Address result)
{
    result = default(IpV4Address);

    if (quadDotted == null)
    {
        return false;
    }

    var elements = quadDotted.Split('.');
    if (elements.Length != 4)
    {
        return false;
    }

    var address = new byte[4];
    for (var i = 0; i < 4; i++)
    {
        if (!byte.TryParse(elements[i], out address[i]))
        {
            return false;
        }
    }

    result = new IpV4Address(address);
    return true;
}

or:

static bool TryParse(string quadDotted, out IpV4Address result)
{
    bool canParse = false;

    if (quadDotted != null)
    {
        var elements = quadDotted.Split('.');
        if (elements.Length == 4)
        {
            var address = new byte[4];

            // the following part can also be transformed in a do-while cycle
            canParse = byte.TryParse(elements[0], out address[0]);
            for (var i = 1; i < 4 && canParse; i++)
            {
                canParse = byte.TryParse(elements[i], out address[i]);
            }
        }
    }

    result = canParse ? new IpV4Address(address) : default(IpV4Address);
    return canParse;
}


Is catching null pointer exception to handle it instead of null check beforehand always a bad practice or it depends? I wrote a static method for an enum that returns the corresponding enum instance based on the source string, trimming any whitespaces and ignoring case as:

/**
 * Convert the passed {@code source} string value to respective {@link SomeEnum} instance.
 * Ignore any white-spaces present in the beginning/end of the passed {@code source} string.
 * Ignore casing of the {@code source} string.
 *
 * @param source {@link String}
 * @return converted {@link SomeUnit} instance if conversion is possible, null otherwise.
 */
public static SomeEnum fromString(String source) {
    try {
        return source == null ? null : SomeEnum.valueOf(source.trim().toUpperCase());
    } catch (IllegalArgumentException e) {
        return null;
    }
}

Then considering the fact that there is only one source for a NPE here, I changed the above to:

/**
 * Convert the passed {@code source} string value to respective {@link SomeEnum} instance.
 * Ignore any white-spaces present in the beginning/end of the passed {@code source} string.
 * Ignore casing of the {@code source} string.
 *
 * @param source {@link String}
 * @return converted {@link SomeUnit} instance if conversion is possible, null otherwise.
 */
public static SomeEnum fromString(String source) {
    try {
        return SomeEnum.valueOf(source.trim().toUpperCase());
    } catch (NullPointerException | IllegalArgumentException e) {
        return null;
    }
}

Question:

Is there any difference (significant/insignificant) between the two in terms of efficiency, best practices, coding style, readability or any other possible factor that I am unaware of? Or am I just overthinking?

I personally feel that instead of relying on exceptions (NPE here) to take some action, better to test it beforehand if possible, which favors the first approach. But then I feel that in this particular case I am anyway having a catch block here for IAE so better to club the exceptions together instead of performing a null check.

Update:
This code is a piece of already written application, and I have to return null in case there is no match, so as to respect the various client calls.

Edit:
This code is fully owned by me. When I stated its a piece of already written application, I meant, way earlier I wrote the code as first approach(where again returning null was needed), and just a few days back going through this I refactored it to second approach.

Also this is no pseudo code, this is the exact code being used with just the class and variables name changed.

The correct behavior of your code depends on the related code around it.

If that code doesn't expect nullvalues in strings, you should not even check for null. And, to be consistent, you should not return null either but throw an exception instead. Therefore, you should remove the whole catch clause.

On the other hand, if you follow the Apache Commons style of expecting null everywhere and passing it through unmodified, your first code is the way to go. Since null is an expected value, it should not throw exceptions, for performance reasons. The second code is nicer to read, but it will be slower unless the optimizer were really fancy and would transform the second code into the first.


Parsing data from big json field and geocode services then store to PostgreSQL using psycopg2 instead django-orm This code works, but my boss told me that it's very bad code, and don't want to deploy it. It is my first time working as a programmer and I have at least 3 months of experience.

# -*- coding: utf-8 -*-

import psycopg2.extras
import time
import json
from psycopg2.extensions import AsIs
from methods_grabber import get_district, get_city,\
                            get_street, get_house, get_postcode
time_start = time.time()

conn = psycopg2.connect("dbname=gym user=gyms")

def get_data():

    global conn

    dict_cur = conn.cursor(cursor_factory=psycopg2.extras.DictCursor)
    dict_cur.execute("""
                    SELECT *
                    FROM dblgis_grabber_firm;
                    """
                    )

    res = dict_cur.fetchall()
    dict_cur.close()
    return res

def update_data(id_row, row, data):
    global conn

    cur = conn.cursor()

    query = "UPDATE dblgis_grabber_firm SET %s=%s WHERE id=%s;"

    cur.execute(query, (AsIs(row), data, id_row))

    conn.commit()
    cur.close()
    return cur.rowcount

data = get_data()

def get_from_contacts(contact_group, row_name, representation, id_row):
    result = []
    for contacts in contact_group:
        for contact in contacts.get('contacts'):
            if contact.get('type') == representation:
                result.append(unicode(contact.get('text')))
    result = '; '.join(result)
    update_data(id_row, row_name, result)

def fill_data(instance):

    longitude = instance.get('longitude')
    latitude = instance.get('latitude')

    id_row = instance.get('id')

    contact_group = json.loads(instance.get('all_data', {})).get('contact_groups', {})

    if not instance.get('street'):
        street = get_street(longitude, latitude)
        if street:
            update_data(id_row, 'street', street)

    if not instance.get('street_number'):
        street_number = get_house(longitude, latitude)
        if street_number:
            update_data(id_row, 'street_number', street_number)

    if not instance.get('postcode'):
        postcode = get_postcode(longitude, latitude)
        if postcode:
            update_data(id_row, 'postcode', postcode)

    get_from_contacts(contact_group, 'phone_number', 'phone', id_row)
    get_from_contacts(contact_group, 'site', 'website', id_row)
    get_from_contacts(contact_group, 'email', 'email', id_row)

for d in data:
    fill_data(d)

conn.close()

print 'Timeit %s' % (time.time() - time_start)

It use my methods from methods, which resolves data from yandex and geopy lib.

After parsing big JSON data, it writes changes to the DB. It's tested for 4k elements and works for 230sec. I think it's normal, but why is it so bad?

Avoid globals

There is no need in using the global keyword here since you’re not modifying your conn. But there is also no need to access this variable at global scope, you can easily pass it as parameter.

One of the advantages of using parameters instead of a global variable being that you can more easily test each function with a custom connection when you need to.

Avoid top-level code

conn = psycopg2.connect("dbname=gym user=gyms")
data = get_data(conn) # Adding parameter from previous advice
for d in data:
    fill_data(conn, d) # Adding parameter from previous advice
conn.close()

Is better within its own function: you get the workflow at first glance. You can also use the function parameters to pass the connection credentials.

Using a function here lets you easily write:

if __name__ == '__main__':
    time_start = time.time()
    check_data("dbname=gym user=gyms")
    print 'Timeit %s' % (time.time() - time_start)

You clearly see there what is computation and what is timming. This is also best practice to wrap the executable part of your code under if __name__ == '__main__'.

But you are usually better off using timeit for performances measurement.

Avoid managing database state yourself

psycopg2 provide context managers to wrap connection or cursor objects. For the former it handles auto-commit/rollback based on how things went within the context; and for the latter it auto-closes it.

You can thus write

def get_data(conn):
    with conn.cursor(cursor_factory=psycopg2.extras.DictCursor) as cursor:
        cursor.execute("""
                       SELECT *
                       FROM dblgis_grabber_firm;
                       """)
        return cursor.fetchall()

and for update_data, you’ll need to auto-commit/rollback the connection:

def update_data(conn, id_row, row, data):
    with conn:
        with conn.cursor() as cursor:
            cursor.execute(
                "UPDATE dblgis_grabber_firm SET %s=%s WHERE id=%s;",
                (AsIs(row), data, id_row)
            )

I removed the return statement since you never make use of it.

Improve get_from_contacts

First of, the name is misleading. It doesn't get anything, it set data in the database. It probably should be named update_from_contacts. Alternatively, you could keep the name get_from_contacts and have that function actually do what it claims to do; and nothing more:

def get_from_contacts(contact_group, representation):
    result = []
    for contacts in contact_group:
        for contact in contacts.get('contacts'):
            if contact.get('type') == representation:
                result.append(unicode(contact.get('text')))
    return '; '.join(result)

You will need to have update_data calls near your get_from_contacts; but it separate concerns. And you can save on number of parameters.

You could also improve the overall execution: initialyzing a list to the empty list and appending inside a for loop is often an anti-pattern in python and better served with a list comprehension. Here the join call calls to using a generator expression:

def get_from_contacts(contact_group, representation):
    return '; '.join(
        unicode(contact.get('text'))
        for contacts in contact_group
        for contact in contacts.get('contacts')
        if contact.get('type') == representation
    )

Avoid redundancy

In fill_data, there are several constructions that look alike. You should be able to extract the structure from the data and use iterations to achieve the desired result without copy/pasting similar blocks of code:

GEO_FETCH = {
    'street': get_street,
    'street_number': get_house,
    'postcode': get_postcode,
}

CONTACT_FIELDS = {
    'phone_number': 'phone',
    'site': 'website',
    'email': 'email',
}

def fill_data(conn, instance):
    longitude = instance.get('longitude')
    latitude = instance.get('latitude')

    id_row = instance.get('id')

    contact_group = json.loads(
        instance.get('all_data', {})
    ).get('contact_groups', {})

    for field, fetcher in GEO_FETCH.iteritems():
        if not instance.get(field):
            geo_data = fetcher(longitude, latitude)
            if geo_data:
                update_data(conn, id_row, field, geo_data)

    for row_name, field_name in CONTACT_FIELDS.iteritems():
        row_data = get_from_contacts(contact_group, field_name)
        update_data(conn, id_row, row_name, row_data)

Bringing it all together

# -*- coding: utf-8 -*-

import json
import psycopg2.extras
from psycopg2.extensions import AsIs
from methods_grabber import (get_district, get_city,
                             get_street, get_house, get_postcode)

GEO_FETCH = {
    'street': get_street,
    'street_number': get_house,
    'postcode': get_postcode,
}

CONTACT_FIELDS = {
    'phone_number': 'phone',
    'site': 'website',
    'email': 'email',
}

def fill_data(conn, instance):
    longitude = instance.get('longitude')
    latitude = instance.get('latitude')

    id_row = instance.get('id')

    contact_group = json.loads(
        instance.get('all_data', {})
    ).get('contact_groups', {})

    for field, fetcher in GEO_FETCH.iteritems():
        if not instance.get(field):
            geo_data = fetcher(longitude, latitude)
            if geo_data:
                update_data(conn, id_row, field, geo_data)

    for row_name, field_name in CONTACT_FIELDS.iteritems():
        row_data = get_from_contacts(contact_group, field_name)
        update_data(conn, id_row, row_name, row_data)

def get_from_contacts(contact_group, representation):
    return '; '.join(
        unicode(contact.get('text'))
        for contacts in contact_group
        for contact in contacts.get('contacts')
        if contact.get('type') == representation
    )

def update_data(conn, id_row, row, data):
    with conn:
        with conn.cursor() as cursor:
            cursor.execute(
                "UPDATE dblgis_grabber_firm SET %s=%s WHERE id=%s;",
                (AsIs(row), data, id_row)
            )

def get_data(conn):
    with conn.cursor(cursor_factory=psycopg2.extras.DictCursor) as cursor:
        cursor.execute("""
                       SELECT *
                       FROM dblgis_grabber_firm;
                       """)
        return cursor.fetchall()

def check_data(db_credentials):
    conn = psycopg2.connect(db_credentials)
    for data in get_data(conn):
        fill_data(conn, data)
    conn.close()

if __name__ == '__main__':
    import timeit
    timeit.timeit('check_data("dbname=gym user=gyms")',
                  number=1, globals={'check_data': check_data})

Beyond readable code

Good production code is not only clear but also correctly documented to be maintainable. You should spend some time writing some docstrings to help any future reader understand what your code does. Comments in the code can also help understand how the code accomplishes its goals.


Combobox with search suggestions I created a combobox of which the list of options updates whenever you type in the box. Just a bit like entering a search in google works with suggestions.
I followed this tutorial to do so: https://trumpexcel.com/excel-drop-down-list-with-search-suggestions/

There are 4 columns of data in the sheet that make this work, combined with a named range, and a short piece of VBA to update the combobox. The combobox is in linked to cell B3. And data is in E:H starting at row 3.

First column includes the options for the combobox. There are 5619 possible options to choose from.
Second column has the following formula to determine if the entry of the combobox is in the text of the same row in the first column: =--ISNUMBER(IFERROR(SEARCH($B$3,E3,1),""))
Third column has the following formula to establish the amount of matches found: =IF(F3=1,COUNTIF($F$3:F3,1),"")
Fourth column has the following formula to return the list of possible options based on the value typed in the combobox: =IFERROR(INDEX($E$3:$E$22,MATCH(ROWS($G$3:G3),$G$3:$G$22,0)),"")

The code to update the combobox is as follows:

Private Sub ComboBox1_Change()
ComboBox1.ListFillRange = "DropDownList"
Me.ComboBox1.DropDown
End Sub

With DropDownList Being a named range that selects all the names in the fourth column.

The problem I face here is purely performance. It is really slow and unresponsive on a PC. Let alone deployed on a mobile device, which I intend to.

The problem is ofcourse within the fact that there are 5600+ options for the combobox, and therefore 4*5600 formula's to be recalculated at every character entered in the combobox. I was wondering if there is a way to do with less helper columns, and maybe more VBA to reduce the calculation times required?

To combine the second and third column into only 1 helper column I deviced the following formula's:

In the first cell of the second column I put the following formula: =IF(ISNUMBER(SEARCH($B$3,E3,1))=FALSE,"",--ISNUMBER(SEARCH($B$3,E3,1))

In the second cell and down I wrote the following formula: =IF(--ISNUMBER(IFERROR(SEARCH($B$3,E4,1),""))=0,"",COUNT($E$3:$E3)+1)


PHP e-mail contact form with AJAX I have just started learning how to build websites, and I have been experimenting by creating a contact form.

Here is what I currently have - can anyone suggest and recommend ways enhance my current contact form system?

JavaScript:

$("#contactForm").submit(function (event) {

    /* stop form from submitting normally */
    event.preventDefault();

    /* get some values from elements on the page: */
    var $form = $(this),
        $submit = $form.find('button[type="submit"]'),
        name_value = $form.find('input[id="name"]').val(),
        email_value = $form.find('input[id="email"]').val(),
        message_value = $form.find('textarea[id="message"]').val(),
        url = $form.attr('action');

    /* send the data using post */
    var posting = $.post(url, {
        name: name_value,
        email: email_value,
        message: message_value,
        ajax: 1
    });

    posting.done(function (data) {
        $form.find('span.error').remove();

        if (data == "1") {

            /*Change the button text.*/
            $submit.text('Sent. Thank You!');
            $submit.addClass("sent");

        } else {
            $submit.after('Failed to send the message, please check your entries and try again.');
            /*Change the button text.*/
            $submit.text('Try Again');
        }
    });
});

PHP

<?php

    if(isset($_POST['name']) && $_POST['email'] && $_POST['message']) {

    $name = $_POST['name'];
    $email = $_POST['email'];
    $message = $_POST['message'];

    $to = "email@website.com";
    $subject = "New Message From: $name";
    $message .= "$messege";

    $headers  = 'MIME-Version: 1.0' . "\r\n";
    $headers .= 'Content-type: text/html; charset=utf-8' . "\r\n";
    $headers .= 'From: '.$email . "\r\n";
    $mailed = ( mail($to, $subject, $message, $headers) );

    if( isset($_POST['ajax']))
        $response = ($mailed) ? "1" : "0";
    else
        $response = ($mailed) ? "
Success!
" : "
Error! There was a problem with sending.
";
        echo $response;
        }
    else
        {
        echo "Form data error!";
    }
?>

HTML:

<form id="contactForm" action="email.php" method="post" name="contactForm">

        Your Name
        <input id="name" type="text" placeholder="Enter Name" required>

        Email Address
        <input id="email" type="email" placeholder="Enter Email" required>

        Message
        <textarea id="message" required></textarea>

    <button id="submit" type="submit">Send</button>
</form>

Php

I'm not a big Php guy so I can't be too thorough in this section. The biggest thing is that you seem to be writing ... well ... a php script. While that absolutely works it's incredibly old-school and doesn't do anything as far as guiding you into the pit of success. So first bit of advice: use a framework. At time of writing, the two I've been hearing most buzz about are Laravel and Codeigniter. This is constantly changing but just pick one which you've heard about and use it, that will help you learn enough concepts to make an educated choice next time you need to select one.

Second, at the bottom you seem to have some invalid php. You have some missing braces in that if statement toward the bottom and it has two else clauses in a row. I'm not sure if that will cause an error but it will definitely never run the third clause.

Finally, your design leaves open a huge security hole if the site is ever opened to the public. You accept the message as an input parameter, much worse, you accept the email. Think of what happens when someone does a query to yoursite.com/yourpage.php?email=yourgrandma&message=Send%20me%20all%20the%20moneyand again, and again. Spammers love finding unsecured services like this!

A much better approach is to have the emails (and maybe message) stored on the server already in the database or a text file. Then you could supply parameters like email=1 indicating "email the person with id=1".

Having this behind a login page is not a (good) defense either. Someone could easily put the above url in the src of an  element and every time someone who had recently logged into your site visits a page they put that on (like a comment or forum page) you will be sending spam! To defend against this make sure that page does not accept the Http GET verb (in this situation POST would be appropriate). This is exactly the kind of error that a framework shoudl help defend you from.

Finally, I see what you're doing with the ajax flag but that's not the typical thing to do. Whether the operation succeeded or not should be determined by the returned Http code (200 for success and maybe 400 for error). Whether you should be returning json or html should be determined by a concept called content negotiation. This is again, something that your framework should be handling for you.

Html

name is not a valid attribute for the <form> element. But also don't put ids on things unless you absolutely know there will be only one of these elements on the page. The spec says ids must be unique across the page and lots of javascript libraries cause errors when the assumption is broken. In this case can you guarantee there will not be two contactForm elements on the page at once? What if you have 2 offices and each one wants its own contact form? What about elements with id=message or id=submit? A better approach I think in this case is to use a class. That is not to say that you should never use ids but in most cases the benefit isn't worth the risk.

Next, you don't technically need <button type=submit>, it's optional but submit is the default behavior.

Finally, I'm not 100% sure of the spec for  but I don't think it works by associating the label with the following <input>. Instead you have to use the for attribute and ids (which I've already argued is bad). Or do what I recommend, something like:

        Email Address
        <input id="email" type="email" placeholder="Enter Email" required>

Javascript

Javascript naming convention are to use camelCase with no underscores. It's not a big deal but it's pretty standard. The only exception is to use PascalCase for variables that are intended to be used with the new keyword (which you don't have here).

To a lesser extent than php, but with the javascript community where it is, its probably time to start considering choosing a javascript framework (like angular, backbone, knockout, or the like rather than using jquery directly.

In my opinion you are over-specifying your selectors, no need to do input[id=foo] when id is already specified as unique, no need to span.blah when span holds no semantic nor structural meaning. Consider that all this stuff is indexed in the browser so by adding more selectors you might actually be slowing things down however slightly.

You don't need to use $('id=something') to select, you can just say $('#something')

Your scope management looks pretty good, I'm happy to see you using find - another syntax which achieves the same purpose and you might find flows better (or not) is $('[name=email]', $form).

You've got inline styling and html. Why? Just put the element on your html page and show or hide it from javascript.

Finally, just about everything you're doing in js is handled in a very elegant manner by the jquery.form plugin. Just use that.


Designing a binary tree structure from scratch in C I'm a beginner and self learning programming students. While learning binary tree data types, for the sake of understanding graph theory and other interesting programming problems, I've writen the below implementation. Would you please suggest if there's anything wrong or either this can be improved.

It may seem reinventing the wheel, as there's a nice STL in C++, but I do implement data structure and algos from scratch for the sake of a better understanding and visualization for me

Here's my code, and thanks in advance

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

typedef struct tree {
      int value;

      struct tree *left_child;
      struct tree *right_child;
} bin_tree;

void create_tree(int value, bin_tree **root);
void print_tree(bin_tree *root);
void free_mem(bin_tree *root);

int main() {
    bin_tree *root;

    root = (bin_tree *) malloc (sizeof(bin_tree));
    root = NULL;

    int val;
    while (scanf("%d", &val) != EOF) {
           create_tree(val, &root);
    }

    printf("root : %d\n", root->value);
    print_tree(root);
    free_mem(root);
    //printf("root: %d\n", root->value);
    return 0;
   }

void create_tree(int val, bin_tree **root) {
     if (*root == NULL) {
        bin_tree *cache = (bin_tree *) malloc(sizeof(bin_tree));
        cache->value = val;
        cache->left_child = NULL;
        cache->right_child = NULL;
        *root = cache;
     }
    /*this tree is built avoiding duplicates*/
   else if(val < (*root)->value){ create_tree(val, &(*root)->left_child);}
   else if(val > (*root)->value){ create_tree(val, &(*root)->right_child);}
  }

  void print_tree(bin_tree *root) {
       if (root != NULL) {
           print_tree(root->left_child);
           print_tree(root->right_child);
           printf("%d ", root->value);
         }
       }

   void free_mem(bin_tree *root) {
        if (root) {
           free_mem(root->left_child);
           free_mem(root->right_child);
           free(root);
   }
 }

Here are some things that may help you improve your code.

Don't leak memory

The main routine begins with these two curious lines of code:

root = (bin_tree *) malloc (sizeof(bin_tree));
root = NULL;

That's a guaranteed memory leak because we no longer have any way to refer to the memory allocated by malloc.  Since your create_tree function will automatically allocate space for the root node, I'd change that to this, combining declaration and initialization:

bin_tree *root = NULL;

Check return values and handle errors

The code calls malloc but never checks for error return values.  This is a serious problem that must be addressed.  Also, scanf can return EOF if there was an error, but that condition is neither checked nor handled.

To be more precise, the way that the underlying operating system lets a program know that it cannot allocated the requested memory is by returning NULL from a call to malloc.  Robust programs should always check for that condition before using the returned value as a pointer or they will risk a crash by dereferencing a NULL pointer.

Similarly, scanf returns the number of arguments successfully scanned.  In your case it should be 1 because each call is scanning one integer value.  If it's anything other than 1, it's an indication of a problem that should probably be handled gracefully by the program, perhaps by emitting a message to the user that only integer values are allowed and asking again.

Use "in-order" tree traversal

The binary tree you've constructed maintains the order of entries, so to take advantage of this, we should use in-order.  It's as simply as changing the order of two lines of code so the print_tree code looks like this:

void print_tree(bin_tree *root) {
    if (root != NULL) {
        print_tree(root->left_child);
        printf("%d ", root->value);
        print_tree(root->right_child);
    }
}

So for example, when I feed the program this input:

  10 5 33 2 18 17 15 20 22 1 8

The program then constructs this tree:

And an in-order traversal produces this output:

  1 2 5 8 10 15 17 18 20 22 33

Put each statement on a single line

It detrimental to the readability of your code to put multiple things one the same line. So instead of this:

else if(val < (*root)->value){ create_tree(val, &(*root)->left_child);}

I would recommend writing this:

else if (val < (*root)->value) {
    create_tree(val, &(*root)->left_child);
}

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments:  "I didn't know that." or "That's bad advice!"  My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard.  For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

  [...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

  If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea.  We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function.  Reasons against omitting seem to boil down to "it looks weird".  If, like me, you're curious about the rationale for the change to the C standard read this question.  Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!)  In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.


Review a simple PHP OOP model I wanted to write the best implementation I could think of the following really, super simple and stupid OOP model in php.

class Number {
    private $number = false;

    function __construct($n) {
        if(is_int($n)) {
            $this->number = $n;
        }
    }

    public function get() {
        return $this->number;
    }
}

class Numbers {
    public static function multiply(Number $a, Number $b) {
        return ($a->get() * $b->get());
    }
}

////
// Example
////
$first = new Number(45);
$second = new Number(80);

echo Numbers::multiply($first, $second);

Anything you guys would change? I was thinking of having Numbers extend Number, but really, the type of operations (functions) Numbers does do not need to extend the Number class. Is there a better object oriented pattern for this model?

Here's a few things that caught my eye:

I'm assuming this is just a sketch of an unfinished class, or perhaps a learning excersize? As it stands, your class is unnecessary, and I would advise against using it. Since it just wraps a built in int, built in ints might as well be used. Various functionalities could indeed make it a useful class though (for example, arbitrary precision capabilities).
If the constructor's argument can't be used, throw an exception. Objects should never exist in an incomplete or meaningless state. What does a false number mean? What does multiply($n1, $n2) mean if one of the numbers is false? Just throw an exception instead.
I wouldn't bother having Numbers exist. Just have the operations be members of Number. Static methods tend to be regretted down the line. It's always better to have a non-static that could be static than to have a static that you later realize shouldn't be static. Also, having the method be internal to Number would be nice as it would allow you access to Number's internals without having to expose them to the world.
multiply should return a Number, not an int. Why bother abstracting into an object if you're not going to stick with it?
Name your argument to the constructor something meaningful. Rather than $n name it $value (or something along those lines).
Consider expanding your number to handle more than ints. Using the BC Math or GMP extensions, you could write a class that handles arbitrary precision numbers very easily.

I would probably go with BCM just because it's more commonly available. It's statically linked on Windows and thus always enabled, whereas GMP is bundled in the binary archives, but not enabled by default. I'm not sure what the situation is on linux.
Since BCM works with ASCII encoded base 10 numeric strings and GMP works with handles, GMP is probably more memory efficient. No idea about general performance though since I'm not very familiar with GMP or BCM's internals or features.
I belive GMP is limited to integers whereas BC Math is not. That could be good or bad depending on the needs/expectations.

You missed the public in front of your constructor.
Depending on what you end up doing with the class, I probably wouldn't expose the internal representation. For example, if you ended up using GMP and wanted to switch to BMP, you couldn't if you had previously exposed the internals. Rather than expose them directly, I would implement methods like toString, toInt, toFloat, toBinary, etc. Those methods woud then either do the conversion if possible, or, if precision would be lost (like trying to convert a value to large for an int to an int), an exception would be thrown (or you could return false depending on how you expected to use the methods and if you consider a failure to convert a truly exceptional situation).
As it is right now, Number is a misnomer. 5 and 5.0 are both numbers, but your class only supports 5. I might rename it to Integer or something similar.


N-Queens without recursion (but with one goto) Compared to the recursive version e.g. on Rosetta, this is 5-10% faster and uses 50% less instructions (for N=12).
Only problem is the goto. Can it be avoided without using a function?
The two outer for-loops don't do much in terms of loop control. But this is how I could make it work.
Is there a more convincing non-recursive version? I searched quite a bit and most are recursive and/or longer.
/* N-Queens, non-recursive, with goto */
#include <stdio.h>
//const int N = 8;  //92 solutions
const int N = 12;   //14200 in 0.100 s

/* Print excluding last row, plus last col */
void pr_solution(int *bq, int col) {
    for (int i = 0; i < N - 1; i++)
        printf("%d:", bq[i]);
    printf("%d\n", col);
}
void init_board(int bq[], int v) {
    for (int i = 0; i < N; i++)
        bq[i] = v;
}
int main() {
    int bq[N];
    init_board(bq, -1);
    int row, col, rd, old;
    int cnt = 0;
    for (row = 0; row >= 0;) {

        for (col = bq[row];;) {
            NEXT_col:
            if (++col == N)
                bq[row--] = -1;
            else {
                for (rd = 1; rd <= row; rd++)
                    if ((old = bq[row-rd]) == col || old == col+rd || old == col-rd)
                        goto NEXT_col;
                if (row == N - 1) {
                    cnt++;
                    row--;
                    //pr_solution(bq, col);
                }
                else
                    bq[row++] = col;
            }
            break;
        }
    }
    printf("%d SOLUTIONS (%dx%d)\n", cnt, N, N);
    return 0;
}

Only problem is the goto. Can it be avoided without using a function?

Candidate goto replacement
    for (col = bq[row];;) {
        //NEXT_col: // Delete
        if (++col == N)
            bq[row--] = -1;
        else {
            for (rd = 1; rd <= row; rd++)
                if ((old = bq[row-rd]) == col || old == col+rd || old == col-rd)
                    // goto NEXT_col;  // Delete
                    break; //ADD
            if (rd <= row) continue; // ADD
            if (row == N - 1) {
                cnt++;
                row--;
                //pr_solution(bq, col);
            }
            else
                bq[row++] = col;
        }
        break;
    }

if (rd <= row) continue; takes advantage that for (col = bq[row];;) lacks a test and increment.

Is there a more convincing non-recursive version?

I'd re-write the loop (perhaps a state machine)  - need some time to think about it.

Later:
Code as written is quite tight.  A very nice simple non-recursive solution.
Yet (at the risk of the ire of OP) lacks clarity making it unnecessarily  difficult to optimize.
The style uses terse variable names like rd and bq which do not well convey their meaning.  It is as if short variable names make faster code (it does not).  init_board() does not initialize a board, but a single dimension of data.  Use more meaningful names.
Use of incomplete for loops like for (col = bq[row];;) seems like a code golf goal than a clearer col = bq[row]; while (1).
Helper functions like pr_solution() rely on the global N, which instead could be passed in, facilitating clearer function flow and allowing use with more than one size.
Local variable instantiation would help see their limited impact.
For clarity, use {} even for one-line blocks.
Consider rectangles, not only squares.  It adds clarity to code and offers other investigations for eventual optimization.
Some other ideas in code below too.
A first round cut:
int main(void) {
  for (int N = 1; N <= 8; N++) {
    const int ROW_N = N;
    const int COL_N = ROW_N;
    int queen_column[ROW_N];
    init_queen_column(ROW_N, queen_column, -1);
    long long count = 0;
    int parity[2] = { 0,0};
    int row = 0;
    while (row >= 0) {
      int col = queen_column[row];
      while (1) {
        if (++col == COL_N) {
          queen_column[row--] = -1;
        } else {
          int row_offset;
          for (row_offset = 1; row_offset <= row; row_offset++) {
            int old = queen_column[row - row_offset];
            int diff = old - col;
            if ((diff == 0) || diff == row_offset || diff == -row_offset) {
              break;
            }
          }
          if (row_offset <= row) { // From early break above
            continue; // ADD
          }
          if (row == ROW_N - 1) {
            count++;
            // print_solution(ROW_N, queen_column, col); // Print, then change state.
            row--;
          } else {
            parity[(row ^ col)&1]++; // Count parity for later optimization (stub code)
            queen_column[row++] = col;
          }
        }
        break;
      }
    }
    printf("%lld SOLUTIONS (%dx%d)\n", count, ROW_N, COL_N);
  }
  return 0;
}
1 SOLUTIONS (1x1)
0 SOLUTIONS (2x2)
0 SOLUTIONS (3x3)
2 SOLUTIONS (4x4)
10 SOLUTIONS (5x5)
4 SOLUTIONS (6x6)
40 SOLUTIONS (7x7)
92 SOLUTIONS (8x8)

Other idea to speed things up

Keep track of the count of queen placement on black (and/or white) squares.  Once that exceeds about half of N, time to unwind as subsequent forward iterations will all fail.

Look for patterns.  Example: Symmetry.  A solution that starts with the first queen on column 0 will have as many solutions as column N-1.  Looks like run time can be halved in some fashion.


Project Euler 35: Circular primes below 1 million I'm doing another Project Euler problem, number 35:

  The number, 197, is called a circular prime because all rotations of
  the digits: 197, 971, and 719, are themselves prime.

  There are thirteen such primes below 100: 2, 3, 5, 7, 11, 13, 17, 31,
  37, 71, 73, 79, and 97.

  How many circular primes are there below one million?

import math
num = 0
primeList = []

def isprime(test):
    if test == 2:
        primeList.append(2)
        return True
    elif test < 2:
        return False
    else:
        for i in primeList:
            if i > math.sqrt(test):
                break
            elif test % i == 0:
                return False
        primeList.append(test)
        return True

def rotations(n):
    answer = []
    rotation = n
    while not rotation in answer:
        answer.append(rotation)
        rotation = int(str(rotation)[1:] + str(rotation)[0])
    return answer

for i in range(2,1000000):
    numList = rotations(i)
    valid = True
    for j in numList:
        if not isprime(j):
            valid = False
    if valid:
        num += 1

    print(num)

I need help improving my code speed.  What I really think is happening is the rotations() function is slowing the whole thing down.

Trial division repeatedly can get very expensive, and is not scalable in terms of computation time. Especially when you are running your rotation check during the process.

I would suggest getting a list together, then filtering that out. That way it is more efficient, and also when you rotate a number you just check if it is in the list, rather than rechecking it for primality slowly. Lookup tables are usually a good recommendation for these things, as the rotated elements overlap in spots. (such as 79 & 97, having to check if they are prime twice. It gets worse for longer numbers!)

To get all primes below 1,000,000 you can implement the Sieve of Eratosthenes to quickly get the list of primes.

from math import sqrt

def sieve(n):
    primes = [True] * (n+1)
    primes[0] = primes[1] = False

    for x in range(4,n+1,2):
        primes[x] = False

    for x in range(3,int(sqrt(n))+1,2):
        if(primes[x]):
            for y in range(x*x,n+1,x):
                primes[y] = False

    return primes

Now you just go through the returned list, and for each True entry, you can check if it's rotatable.

And here is a simpler way to right a rotation. This one shifts left.

def rotate(n):
    n=str(n)
    return n[1:]+n[:1]

And with that rotation, and the sieve available to us, we now just rotate the number, and check if it is True in the sieve! Return false if any rotation of n is False, else return True if we can't find any non-primes.

def isRotatable(n, primes):
    for x in range(2,n):
        n = rotate(n)
        if not primes[int(n)]:
            return False
    return True

Now for a main method to just go through and print the results for us! I'm using main because I'm not sure what else to name it.

def main(n):
    primes = sieve(n)
    for x in range(2, len(primes)):
        if(isRotatable(x,primes)):
            print(x)

Altogether these pieces should work rather quickly. The longest part I found was it actually outputting to the interpreter, possibly writing to a file may be quicker.


Same setup for many properties in Moq I'm trying to use Moq to mock a rather large set of interfaces in an object. I'm trying to capture the behavior of retaining a dirty flag when properties are set and I'm hoping there is a cleaner way to do this because I have many properties that need this setup.

    public static void SetupDirtyProperty<T, TProp>(this Mock<T> mock, Expression<Func<T, TProp>> getterExpression, Action<T> setterExpression, ref bool isDirty)
    {
        TProp p = default(TProp);

        mock.SetupGet(getterExpression).Returns(() => p);
        mock.SetupSet(setterExpression).Callback((TProp value) =>
        {
            if (p != value)
            {
                p = value;
                isDirty = true;
            }
        });
    }

and to use it it's called like this:

        bool isDirty = false;
        var product = new Mock<IProduct>();

        product.As<ISku>().SetupDirtyProperty(x => x.Price, x => x.Price= It.IsAny<decimal?>(), () => isDirty = true);

The things I would like to simplify about this approach are:

I have to specify a property getter and setter, but it seems like I should be able to invert the property getter expression into a setter with It.IsAny<TProp> that will work correctly with Moq.
I have to specify the closure around the isDirty flag every time.

It'd be nice if I could set this up in a way that is easy to apply this to dozens of properties, and it'd be really nice if it was something like SetupAllProperties where it would just handle all the properties automatically.

Keeping code DRY is a noble goal, even for test code, but it's less important for tests. Our first priority when writing test code is being precise and clear. The tests are the specs and the specs are the tests. It's important that it be immediately clear what the state of the mocked object is. We can expect to duplicate a certain amount of code.

Looking at your test, in isolation from your extension, it's difficult to tell exactly what is going on here.

bool isDirty = false;
var product = new Mock<IProduct>();

product.As<ISku>().SetupDirtyProperty(x => x.Price, x => x.Price= It.IsAny<decimal?>(), () => isDirty = true);

If you need to create this same Moq for many tests, it would be better to use the [TestInitialize] attribute and have it set up prior to each test method being run. See this for more information.


document click vs element click I am wondering if the following is a good concideration.
There might be several links on a page with a data-rel attribute that could be static or dynamically added with javascript. Instead of adding an eventlistener to each of those I am thinking about adding an eventlistener to the document. I don't want to use jQuery.

Is the following good, or is there a better do add events to dynamically added ellements?

var api = {
    function addEvent (evnt, el, func) {
        if (el.addEventListener) { // W3C DOM
            el.addEventListener(evnt,func,false);
        } else if (el.attachEvent) { // IE DOM
            el.attachEvent('on' + evnt, func);
        } else { // No much to do
            el[evnt] = func;
        }
    }
  , removeEvent : function removeEvent (evnt, el, func) {
        if (el.removeEventListener) { // W3C DOM
            el.removeEventListener(evnt,func,false);
        } else if (el.detachEvent) { // IE DOM
            el.detachEvent('on' + evnt, func);
        } else { // No much to do
            el.splice(evnt, 1);
        }
    }
  , checkWhatIsClicked: function checkIfDialog (e) {
        var el = e.srcElement || e.target
          , dataCallback = el.getAttribute('data-callback')
          , dataRel = el.getAttribute('data-rel');

        if (dataRel !== null) {
            switch (dataRel) {
                case 'alert':
                    e.preventDefault();

                    dialogBox.alert();
                    break;
                case 'dialogBox':
                    e.preventDefault();

                    dialogBox.dialog();
                    break;
                case 'prompt':
                    e.preventDefault();

                    dialogBox.prompt();
                    break;
            }
        } else if (dataCallback !== null && el.className.indexOf('dialog-answer') > -1) {
            if (typeof window[dataCallback] === 'function') {
                var result = el.getAttribute('data-result');

                window[dataCallback](result, el);
            }

        }
    }
};

api.addEvent('click', window.document, api.checkWhatIsClicked);

What's wrong with jQuery?  It doesn't get much easier than:

$(document).delegate("<identifier>", "<event>", function (e) { do stuff; });

However, the following link might help you with base javascript event listening and delegation.
https://mattandre.ws/2014/08/small-beautiful-dom-delegation/


Implementation of a sparse Markov Chain I need to create a sparse Markov chain. It is supposed to receive text, so the number of rows or columns can easily go up to 20000. Besides, if I want to consider higher orders of the Markov chain (creating pairs of consecutive words) the dimension can become much bigger. Hence the need to have something sparse.

I added the constraint to have a "uniform prior" on the transitions (so as to avoid having infinite log likelihood).

I am not sure this is the cleanest way to proceed.

using System.Collections.Generic;
using System;

namespace rossum.Machine.Learning.Markov
{
    public class SparseMarkovChain<T>
    {
        private Dictionary<T, Dictionary<T, int>> _sparseMC = new Dictionary<T, Dictionary<T, int>>();
        private Dictionary<T, int> _countEltLeaving = new Dictionary<T, int>();
        private int _size = 0;

        public double GetTransition(T p1, T p2)
        {
            if (_sparseMC.ContainsKey(p1))
            {
                if (_sparseMC[p1].ContainsKey(p2))
                    return (1f + _sparseMC[p1][p2]) / (_countEltLeaving[p1] + _size);
                else
                    return 1f / (_countEltLeaving[p1] + _size);
            }
            else
                return 1f / _size;
        }

        public void AddTransition(T p1, T p2)
        {
            if (_sparseMC.ContainsKey(p1))
            {
                _countEltLeaving[p1]++;
                if (_sparseMC[p1].ContainsKey(p2))
                    _sparseMC[p1][p2] += 1;
                else
                    _sparseMC[p1].Add(p2, 1);
            }
            else
            {
                _size++;
                if (!_sparseMC.ContainsKey(p2))
                    _size++;
                Dictionary<T, int> nd = new Dictionary<T, int>();
                nd.Add(p2, 1);
                _sparseMC.Add(p1, nd);
                _countEltLeaving.Add(p1, 1);
            }
        }

        public double LogLikelihood(T[] path)
        {
            double res = 0;
            for (int i = 1; i < path.Length; i++)
                res += Math.Log(GetTransition(path[i - 1], path[i]));
            return res;
        }
    }
}

If you're using ContainsKey, you're probably doing it wrong and you need to use Dictionary<TKey, TValue>.TryGetValue Method (TKey, TValue).

Your first method would then become:

public double GetTransition(T p1, T p2)
{
    Dictionary<T, int> p1Value;
    if (!_sparseMarkovChain.TryGetValue(p1, out p1Value))
    {
        return 1f/_size;
    }

    int p2Value;
    if (p1Value.TryGetValue(p2, out p2Value))
    {
        return (1f + p2Value)/(_countEltLeaving[p1] + _size);
    }

    return 1f/(_countEltLeaving[p1] + _size);
}

Your naming could be improved. The "MC" in _sparseMC makes sense in context, but why not write "MarkovChain" in full? p1 and p2 aren't clear to me, but I don't know the convention.

These three lines could be a single one:

Dictionary<T, int> nd = new Dictionary<T, int>();
nd.Add(p2, 1);
_sparseMC.Add(p1, nd);

versus:

_sparseMC.Add(p1, new Dictionary<T, int>{{ p2, 1 }});

Use braces everywhere to avoid introducing bugs, even for code like this:

if (_sparseMC[p1].ContainsKey(p2))
{
    _sparseMC[p1][p2] += 1;
}
else
{
    _sparseMC[p1].Add(p2, 1);
}

_sparseMC and _countEltLeaving can be made readonly.


Classical model for database connection provider Let's forget about Spring Singleton Beans and about other frameworks in Java. We have one or more simple HttpServlets. And we should make database connection. (doesn't matter what is it,  hibernate session factory or JDBC connection). Always I write the following pattern:

class DatabaseService {

    private static DatabaseService instance;

    public static void init() {
        instance = new DatabaseService();
    }

    public static void destroy() {
        //.. destroy
    }

    public static DatabaseService getInstance() {
        return instance;
    }

    private Connection conn;

    private DatabaseService() {
        conn = DriverManager.getConnection(DB_URL, USER, PASSWORD);
    }

    public Connection getConnection() {
        return conn;
    }
}

And every my servlet gets database connection from singleton. But what you think about efficiency that solution? Where I should initialize my DatabaseService singleton? Are there a better solution?

Like in Spring where you have not defined lazy loading of bean, the singleton beans are created when the context is started up.

3.3.4. Lazily-instantiated beans

  By default, ApplicationContext implementations eagerly create and
  configure all singleton beans as part of the initialization process.
  Generally, this pre-instantiation is desirable, because errors in the
  configuration or surrounding environment are discovered immediately,
  as opposed to hours or even days later.

So assuming you have some entrypoint like main() method - there is the best place where you can create singletons etc. main() is basically the beginning of your application and since this method is called you should be able to use DatabaseService.

In my opinion DatabaseService is something like DataSource in spring so it should not be configured as lazy loading.

I would change getInstane to singleton pattern:

public static DatabaseService getInstance() {
    if(instance == null) {
        instance = new DatabaseService();
    }
    return instance;
}

So I will go for:

public static void main(String... args){
    DatabaseService.getInstance() != null ? LOG.info("DatabaseService initialized!") : throw new SystemException("Failed DatabaseService initilization");
    ...
}

or you can use your init method but it would be better to change void to DatabaseService and check whether instance was created successfully.


Browser and version detection Based off of another SO reply by kennebec, I had problems following the code so I rewrote it to give myself a better understanding of what is being accomplished.

I would be very grateful if you could critique it or point out any problems I have caused or could modify to improve it.

Tested with IE8/9/10/11, FF, Chrome, Opera & Safari.

Original JSFiddle link
Updated with changes from thriggle JSFiddle link

function getBrowser() {
    var userAgent = navigator.userAgent,
        matched = userAgent.match(/(chrome|firefox|opera|safari|msie|trident(?=\/))\/?\s*(\d+)/i) || [],
        version,
        browser = { name: '', version: '' };

    // IE Shells; I gaurantee that you will see this with Dell branded OEM IE 11 installs.
    if(/trident/i.test(matched[1])) {
        version = /\brv[ :]+(\d+)/g.exec(userAgent) || [];

        return { name: 'Internet Explorer', version: (version[1] || '') };
    }

    // Not Chrome; at first glance this looks like a block for chrome identification, but
    // its actually Opera.
    if(matched[1] === 'Chrome') {
        version = userAgent.match(/\b(OPR|Edge)\/(\d+)/);

        if(version !== null) {
            return { name: version[1].replace('OPR', 'Opera'), version: version[2] };
        }
    }

    // The rest; Chrome, Safari, etc
    matched = matched[2] ? [matched[1], matched[2]] : [navigator.appName, navigator.appVersion, '-?'];

    if((version = userAgent.match(/version\/(\d+)/i)) !== null) {
        matched.splice(1, 1, version[1]);
    }

    browser.name = (matched[0] !== "") ? matched[0] : "Unknown";
    browser.version = (matched[1] !== "") ? matched[1] : "Unknown";

    return browser;
}

Like Kennebec's original answer, the code is inconsistent in its identification of different versions of browsers. For example, Internet Explorer 9 will appear as MSIE 9.0 while Internet Explorer 11 (which despite forsaking the family name still wields the trident of its ancestors) will appear as Internet Explorer 11. Since you have direct control over the name returned for a trident-detected version of IE, why not return "MSIE" instead of "Internet Explorer"?

Super minor nitpick: you misspelled "guarantee" in the code comments.


Connecting to a database and running CRUD operations I have a database class that connects to a database and runs CRUD operations. I need a critique of how well this class adheres to SOLID principles. A class to run all database operations seems like 'Single Responsibility' to me. Is there any need for further abstraction, such as an interface of some kind?

<?PHP

class Database
{

  private $pdo='';
  public $dbConn='';

  private $dbName='';
  private $dbHost='';
  private $dbUser='';
  private $dbPass='';

public function __construct($database)
{
  $this->dbName=$database;
}

private function dbConnect()
{
  //place connection in try catch, so errors can be displayed or sent to log
    try
    {
    $this->pdo= new PDO('mysql:dbname='.$this->dbName.';host=xxx.xxx.xx.xxx','abcdef','vwxyz');
    $this->pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

    $this->dbConn=true;

    return true;

    } catch(PDOException $e){

      return $e;

    }

} //End dbConnect

public function connect(){

    if ($this->dbConnect()) {

         if ($this->dbConn) {

           $myPDO=$this->pdo;

           return $myPDO;
         }

    } else { return false; }

}

function tableExists()
{

   $findTable="SHOW TABLES FROM ".$this->dbName." WHERE tables_in_".$this->dbName." LIKE 'prod%' ";

   $query=$this->pdo->query($findTable);
   $tablesFound=$query->fetch();

   return $tablesFound;

} //END Function tableExists.

public function select($table,$rows='*', $where=null,$order=null,$limit=null)
{

  is_array($rows) ? $rows=implode(',', $rows) : $rows=$rows;

   $stmt='SELECT '.$rows.' FROM '.$table;

  if ( !is_null($where) )
    $stmt .=' WHERE '.$where;

   if (! is_null($order) )
    $stmt .=' ORDER BY '.$order.' DESC ';

  if (! is_null($limit) )
    $stmt .=' Limit '.$limit;

    //if table exists proceed with Query
    $query=$this->pdo->query($stmt);

    $data=$query->fetchAll(PDO::FETCH_ASSOC);

   return $data;

} //End Function Select

public function insert($table,$values,$rows=null)
{
      $ins='INSERT INTO '.$table;

      if ($rows != null) {

        $ins .= ' ('.implode(',', $rows).')';
      }

      for ($i=0; $i<count($values);$i++ )
      {
         $bindingVals[]=':val'.$i;
      }

        $bindVals=implode(',', $bindingVals);
        $ins .=' VALUES ('.$bindVals.') ';

        $params=array_combine($bindingVals, $values);
        $insert=$this->pdo->prepare($ins);

        foreach ($params as $key => &$value) {
          $insert->bindParam($key, $value,PDO::PARAM_STR);
         }

         try {
           $insert->execute();
         } catch (Exception $e) {
           $error=$e->getMessage();

           echo $error.'

' ;
         }

   }//END Function insert

public function setUpdateVals(array $vals)
{
  foreach ($vals as $k => $v)

  {
    $values[]=$k.'='.$this->pdo->quote($v);
  }

  $values=implode(',',$values);

  return $values;
}

public function update($table,$rows,$where=null)
{

  $sql='UPDATE '.$table.' SET '.$rows;
  if ($where != null)
  {
    $sql.=' WHERE '.$where;
  }

  $stmt=$this->pdo->prepare($sql);
  if ($stmt->execute()) {

    return true;
  }

}//END Function update

public function delete($table,$where=null)
{

     if ( is_null($where) )
     {

        if (confirmTableDelete())
        {
          $sql="DELETE $table";
        }

     } else{

      $sql='DELETE FROM '.$table.'WHERE'.$where;
    }

    $stmt=$this->pdo->prepare($sql);
    $stmt->bindParam(':Value',$Value,$Data_Options);
    $stmt->execute();

 }

}

I think the database class as you wrote it has no use. You write a database class to provide an easier interface with the database. Instead you split up queries into multiple parts, but are still somewhat writing sql. A good database class allows you to use it without having to worry about escaping data, or even if the query was executed (instead of loaded from a cache).
Whitespace
Take some time to fix the whitespace in your code. The whitespace around several operators is inconsistent, such as the spaces around the assignment operator, and the spaces after a comma.
The indentation of your code is incorrect. Make sure that all your code is consistently indented. This makes the code a lot more readable, and thus more maintainable.
Documentation
Your code lacks any documentation or comments. This makes it hard to understand and hard to use, because it is unclear what code can return at any time.
Methods and variables
connect(..)
I am not sure what you are trying to accomplish splitting your code between connect(..) and dbConnect(..). There are several issues with this code. If dbConnect(..) throws, you return the exception, which is not falsy. This means that the if-statement in connect(..) will never be false. This results in connect(..) returning either true or nothing at all.
If connect(..) is called multiple times, you simply connect multiple times instead of just ignoring future calls based on $dbConn.
$dbConn
Why is this variable public? The naming could be better, and you probably don't want external code to modify this variable. Instead provide access to it via a getter.
table_exists(..)
I am unsure why this function exists. The name suggests that it would take an argument with a table name, and would return true or false. Instead it gives an array with table names.
select(..)
This code is vulnerable to sql injection. I would expect a database class to sanitize anything you pass to it. Why would surrounding code do that for the Database class? Check each of the arguments against what you expect the arguments to contain.
PDO::query returns a PDOStatement or false. You do not check for false, resulting in warnings and errors in the following code.
PDOStatement::fetchAll returns false on failure. Since your code lacks documentation I can't determine if this is the intended behaviour, but usually when you build a Database class you do this to encapsulate the PDO interface and provide easier to use methods.
insert(..)
This code is vulnerable to sql injection. See the previous section.
This method prints an error message. I do not expect a database class to print anything! Either provide a different way of accessing the error if it is required or log it. Make the method return true or false based on if it succeeds.
Don't use a catch all catch block. Instead figure out what the code can throw, and act accordingly. According to the documentation, that method does not throw.
setUpdateVals(..)
This function should be removed. It is not the responsibility of other code to escape values. In any case, you should not use PDO::quote and instead use prepared queries.
update(..)
While an update query without a where clause is possible, it is extremely unlikely you need it. Instead, the chances are much greater that you instead forgot to pass a value or did not properly check if a variable existed, or a return value was set.
You probably are tired of hearing it, but the code in this method is vulnerable to sql injection. A prepared query without any placeholders could as well be a normal query. You know how to bind variables to a variable number of placeholders, as you used it before. Use it here.
This method either returns true or nothing at all. Instead just return $stmt->execute(). The same problem where you do not check if PDO::prepare returned a boolean exists here too.
delete(..)
confirmTableDelete(..) does not exist. In any case, it is not the responsibility of a database class to interact with the user. It provides an interface between the database and the rest of the code. Assume that when delete is called, it is intended to be used.
You bind to :Value, but no such placeholder exists. $Data_options does not exist either.
The method does two separate things. I think you should split it into two distinct methods, each doing exactly one thing. This prevents unfortunate mistakes with missing tables.


Is this code an efficient implementation of Reservoir Sampling? I posted this an answer this to a question about drawing a random line from a file too large to put into memory. I hacked the below code together. In essence, this is what Reservoir Sampling does, in pseudo-code:
Scan over the 'tape'
    Put the first 'n' samples in a reservoir(of size n)
    // samples are lines of a document, numbers, whatever
    After the first 'n' :
        Pick a random number between 1 and NumberOfLinesCounted
        if the number is between 1 and n
            replace an existing line with a 1/n chance

Here is the code designed to scan over a document, with said sampling method:
import java.io.FileNotFoundException;
import java.io.IOException;
import java.util.List;

public class reservoirSampling {

    public static void main(String[] args) throws FileNotFoundException, IOException{
        Sampler mySampler = new Sampler();
        List<String> myList = mySampler.sampler(10);
        for(int index = 0;index<myList.size();index++){
            System.out.println(myList.get(index));
        }
    }
}

import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Random;
import java.util.Scanner;

public class Sampler {

    public Sampler(){}
    public List<String> sampler (int reservoirSize) throws FileNotFoundException, IOException
    {
        String currentLine=null;
        //reservoirList is where our selected lines stored
        List <String> reservoirList= new ArrayList<String>(reservoirSize);
        // we will use this counter to count the current line number while iterating
        int count=0;

        Random ra = new Random();
        int randomNumber = 0;
        Scanner sc = new Scanner(new File("Open_source.html")).useDelimiter("\n");
        while (sc.hasNext())
        {
            currentLine = sc.next();
            count ++;
            if (count<=reservoirSize)
            {
                reservoirList.add(currentLine);
            }
            else if ((randomNumber = (int) ra.nextInt(count))<reservoirSize)
            {
                reservoirList.set(randomNumber, currentLine);
            }
        }
        return reservoirList;
    }
}

I'm using something very similar on a project I am on at the moment (i.e. the code is close enough I can transfer any changes easily by hand).
Is this a) efficient, and b) actually reservoir sampling (with equal odds of any line being drawn)?

Reviving an old, but really interesting post, that did not get a great opportunity for review the first time around....
What you've done well:
This code, for the most part, is excellent. As for your specific questions:
Is this:

efficient
Mostly, yes. The inefficiencies in here are all related to the IO, and the Scanner, not your code. A more low-level API may net you some performance benefits, but not likely. You are also (re)using the Random class well, and there should not be a problem there.

actually reservoir sampling (with equal odds of any line being drawn)?
Yes. This is a good implementation of the reservoir algorithm. The random arguments are good, and the substitutions should be fine.

What's concerning

The most significant beef is that you do not close the Scanner. Scanners can hold on to open file-handles, and, if the file really is big, this may have system-wide consequences. A try-catch-finally block would be appropriate (or, a try-with-resources if you're on Java7 now).

A second (smaller) issue is that reservoir sampling is designed to handle huge data in a streaming way. I know it is perhaps unrealistic to consider in your use-case, but multi-gigabyte files are not unexpected, and the likelihood of overflowing the count variable is not unrealistic. That would cause problems....

You should consider converting count and randomNumber to be long values, and only convert it back to an int if it is less-than reservoirSize

Finally....
Nice code to review. Thanks. Obviously, the new language features in more recent Java versions may be interesting to consider....


2D Platformer Collision Detection/Resolution I am working on a 2D tile based platformer and was hoping to get someone to review my collision detection. I've only included the X collision check since the Y is essentially the same. Any suggestions/criticisms are appreciated.

    // Corrects X movement if a collision occurs.
    private void checkXCollision()
    {
        // Find the range of tiles the player intersects
        Point topLeft = new Point(((int)(xtemp + dx) - collisionWidth/2) / map.getTileSize(), ((int)ytemp - collisionHeight/2) / map.getTileSize());
        Point bottomRight = new Point(((int)(xtemp + dx) + collisionWidth/2) / map.getTileSize(), ((int)ytemp + collisionHeight/2) / map.getTileSize());

        // Left
        if(dx < 0)
        {
            boolean collision = false;
            for(int row = topLeft.y; row <= bottomRight.y; row++)
            {
                if(map.getType(row, topLeft.x) == Tile.SOLID)
                {
                    collision = true;
                    break;
                }
            }

            if(collision)
            {
                dx = 0;

                // Move player to edge of solid tile
                int col = ((int)(xtemp + dx) - collisionWidth/2) / map.getTileSize();
                xtemp = col * map.getTileSize() + collisionWidth / 2;
            }
            else
            {
                xtemp += dx;
            }
        }

        // Right
        if(dx > 0)
        {
            boolean collision = false;
            for(int row = topLeft.y; row <= bottomRight.y; row++)
            {
                if(map.getType(row, bottomRight.x) == Tile.SOLID)
                {
                    collision = true;
                    break;
                }
            }

            if(collision)
            {
                dx = 0;

                // Move player to edge of solid tile
                int col = ((int)(xtemp + dx) + collisionWidth/2) / map.getTileSize();
                xtemp = (col + 1) * map.getTileSize() - collisionWidth / 2 - 1;
            }
            else
            {
                xtemp += dx;
            }
        }
    }

You have some code duplication there

for(int row = topLeft.y; row <= bottomRight.y; row++)
{
    if(map.getType(row, topLeft.x) == Tile.SOLID)
    {
        collision = true;
        break;
    }
}

which should be extracted to a method like

private boolean hasCollision(int start, int stop, int x){

    for(int row = start; row <= stop; row++)
    {
        if(map.getType(row, x) == Tile.SOLID)
        {
            return true;
        }
    }
    return false;
 }

If a collision is found you have this

dx = 0;

// Move player to edge of solid tile
int col = ((int)(xtemp + dx) - collisionWidth/2) / map.getTileSize();

Do you on purpose set dx = 0 and calling then (xtemp + dx) ?
You are also doing a lot of collisionWidth/2 which should be extracted to a variable.
you should extract the left and right checking to separate methods.
in Java the convention is to place the opening brace { on the same line.
using a guard clause for dx == 0 well remove the creation of the Point's
comments should describe why something is done. **What is done should be described by the code itself by using meaningful names.

Implementing most the points above leads to

// Corrects X movement if a collision occurs.
private void checkXCollision()
{
    if (dx == 0) {
        return;
    }

    Point topLeft = new Point(((int)(xtemp + dx) - collisionWidth/2) / map.getTileSize(), ((int)ytemp - collisionHeight/2) / map.getTileSize());
    Point bottomRight = new Point(((int)(xtemp + dx) + collisionWidth/2) / map.getTileSize(), ((int)ytemp + collisionHeight/2) / map.getTileSize());

    if(dx < 0){

        checkLeftXCollision(topLeft, bottomRight);

    } else {

        checkRightXCollision(topLeft, bottomRight);

    }
}

private void checkLeftXCollision(Point topLeft, Point bottomRight) {

    if(hasCollision(topLeft.y, bottomRight.y, topLeft.x)) {
        dx = 0;

        int col = ((int)(xtemp + dx) - collisionWidth/2) / map.getTileSize();
        xtemp = col * map.getTileSize() + collisionWidth / 2;
    } else {
        xtemp += dx;
    }
}

private void checkRightXCollision(Point topLeft, Point bottomRight) {

    if(hasCollision(topLeft.y, bottomRight.y, bottomRight.x)) {
        dx = 0;

        int col = ((int)(xtemp + dx) + collisionWidth/2) / map.getTileSize();
        xtemp = (col + 1) * map.getTileSize() - collisionWidth / 2 - 1;
    } else {
        xtemp += dx;
    }
}

private boolean hasCollision(int start, int stop, int x){

    for(int row = start; row <= stop; row++)
    {
        if(map.getType(row, x) == Tile.SOLID)
        {
            return true;
        }
    }
    return false;
}


Critique of Pascal's Triangle Class I'm working my way through The Java Programming Language, Fourth Edition - The Java Series.  This is Exercise 7.3:

  Write a program that calculates Pascal's triangle to a depth of 12, storing each row of the triangle in an array of the appropriate length and putting each of the row arrays into an array of 12 int arrays. Design your solution so that the results are printed by a method that prints the array of arrays using the lengths of each array, not a constant 12. Now change the code to use a constant other than 12 without modifying your printing method.

Is the following an adequate solution? Is my use of exception handling reasonable for this use case?

/**
 * Defines Pascal's Triangle
 */
public class PascalsTriangle {
    private final long[][] triangle;

    /**
     * Sole constructor
     *
     * @param depth the depth of the triangle
     */
    PascalsTriangle(int depth) {
        triangle = new long[depth][];
        long upperLeft;
        long upperRight;

        for (int i = 0; i < depth; i++) {
            triangle[i] = new long[i + 1];

            for (int j = 0; j < i + 1; j++) {
                if(i == 0) {
                    triangle[i][j] = 1; //seed the triangle
                }
                else {
                    try {
                        upperLeft = triangle[i - 1][j - 1];
                    } catch (ArrayIndexOutOfBoundsException e) {
                        upperLeft = 0; //upperLeft is 0 if not found
                    }

                    try {
                        upperRight = triangle[i - 1][j];
                    } catch (ArrayIndexOutOfBoundsException e) {
                        upperRight = 0; //upperRight is 0 if not found
                    }

                    triangle[i][j] = upperLeft + upperRight;
                }
            }
        }
    }

    /**
     * Prints the triangle to System.out
     */
    public void printTriangle() {
        for (long[] iTriangle : triangle) {
            for (long jTriangle : iTriangle) {
                System.out.printf("%d ", jTriangle);
            }
            System.out.println();
        }
    }

    /**
     * Main method - create and print a triangle
     */
    public static void main(String[] arg) {
        PascalsTriangle triangle = new PascalsTriangle(10);
        triangle.printTriangle();
    }
}

There are 2½ things wrong with your code, otherwise it's fine.

Use exception handling with try/catch for exceptional circumstances, not for trivial bounds checking which you can do! The snippet

try {
      upperLeft = triangle[i - 1][j - 1];
} catch (ArrayIndexOutOfBoundsException e) {
     upperLeft = 0; //upperLeft is 0 if not found
}

throws an error if j == 0 – you already handled the i == 0 case. You can avoid this by starting the loop one index later, and just putting a 1 in the first field. The same considerations hold for the last array entry.

When we initialize the bounds outside of the loops, we get something like:

// init first row
triangle[0] = new long[]{ 1 };

for (int i = 1; i < depth; i++) {
    triangle[i] = new long[i + 1];

    // init first and last element
    triangle[i][0] = 1;
    triangle[i][i] = 1;

    for (int j = 1; j < i; j++) {
        long left  = triangle[i - 1][j - 1];
        long right = triangle[i - 1][j    ];
        triangle[i][j] = left + right;
    }
}

By handling these edge cases seperately, I can radically shorten the code, and can omit exception handling.
In- and Output should happen in the main method, not in helpers. You have written a printTriangle method, which hardcodes the System.out stream. It would be better to assemble a string representation in a toString() method, and then print out the result from that:

public String toString() {
  Formatter formatter = new Formatter(new StringBuilder(), null);
  for (long[] row : triangle) {
    for (long i : row) {
      formatter.format("%d ", i);
    }
    formatter.format("\n");
  }
  return formatter.toString();
}

Then in main: System.out.print(triangle.toString()).

What I haven't addressed until now is your usage of variables. You should declare your variables as close to their point of usage as possible, and in the tightest possible scope. Your upperLeft etc. is only relevant inside the else branch of your original code – if you look at my version, you see I declared the corresponding variable inside the loop as well.

Oh, and your names for the loop variables in printTriangle() are weird. The letters i and j usually denote integers in loops, a convention taken from mathematics into programming. It is better to use names that are actually connected to the problem you are solving. If you look at my snippet, you'll see that I iterate over each row in the triangle, and then over each long integer i in that row.


Detecting gyroscope data using promises In a plugin that I am currently authoring, I am using a function to check whether the device provides gyroscopic data. Although intuitively this can be easily resolved by checking the value of window.DeviceOrientationEvent, i.e.:

if (window.DeviceOrientationEvent) {
    // Ok, device orientation event listening is supported!
}

…this statement also resolves true when on a device with no functional gyroscope, i.e. Google Chrome on a laptop. The event data returned for alpha, beta and gamma are, however, null. Basically, the window.DeviceOrientationEvent does not tell me if there is gyroscopic data available, but only informs me if the browser/user-agent supports the event.

Therefore, not only do I have to check for support for the event, but also have to cross verify the value of alpha, beta or gamma such that they are not null. The best way I could check is using promises, which I feel a bit cumbersome and long-winded:

var check = {
    // Does it have a working gyroscope?
    gyroscope: function() {
        var d = new $.Deferred(),
            handler = function(e) {
                d.resolve({
                    alpha: e.alpha,
                    beta: e.beta,
                    gamma: e.gamma
                });
                // Listen to device orientation once and remove listener immediately
                window.removeEventListener('deviceorientation', handler, false);
            };
        window.addEventListener('deviceorientation', handler, false);
        return d;
    }
}

// Wait for gyroscope data
$.when(check.gyroscope()).done(function(r) {
    if(r.alpha === null && r.beta === null && r.gamma === null) {
        console.log('Device has no functional gyroscope.');
    } else {
        console.log('Device has functional gyroscope.')
    }
});

I am wondering if there is a cleaner way to write this code?

I imagine you're still checking window.DeviceOrientationEvent? Because if that doesn't exist, you can presumably skip everything else.
When using $.Deferred you should always return its .promise() - not the deferred object itself. The deferred object itself is "read/write" (i.e. you can call resolve and reject on it), while the promise it creates is "read-only" which is what you want to pass on to other code.
You don't need $.when unless you're waiting for several promises to resolve. You can simple call promise.done() (or .then or .fail or .always)
Since the idea is get a yes/no answer, I'd simply use the resolve/reject callbacks of the deferred and do all the checking in one place.
I don't know if it's relevant, but checking strictly against null will give you a false positive if alpha etc.are actually completely undefined. It might be better to check typeof alpha === 'number'.
Of course, you don't really need promises; a simple callback would do.

For instance:

var check = {
  gyroscope: function (callback) {
    function handler(event) {
      var hasGyro = typeof event.alpha === 'number'
                 && typeof event.beta  === 'number'
                 && typeof event.gamma === 'number';
      window.removeEventListener('deviceorientation', handler, false);
      callback(hasGyro);
    }
    window.addEventListener('deviceorientation', handler, false);
  }
};

check.gyroscope(function (hasGyroscope) {
  if(hasGyroscope) {
    console.log('Device has functional gyroscope.')
  } else {
    console.log('Device has no functional gyroscope.');
  }
});

If you do want to use $.Deferred, I'd recommend something like:

var check = {
  gyroscope: function () {
    var deferred = $.Deferred();

    function handler(event) {
      var hasGyro = typeof event.alpha === 'number'
                 && typeof event.beta  === 'number'
                 && typeof event.gamma === 'number';
      window.removeEventListener('deviceorientation', handler, false);
      hasGyro ? deferred.resolve() : deferred.reject();
    }

    window.addEventListener('deviceorientation', handler, false);
    return deferred.promise();
  }
};

check.gyroscope().then(function () {
  console.log('Device has functional gyroscope.')
}, function () {
  console.log('Device has no functional gyroscope.');
});


Speeding up create CRC function I have a function like this:

BOOL CGameData::GetCheckSum(BYTE o_byObjCheckSum[32], int *o_pnFileSize, char* pFilePath)
{
memset(o_byObjCheckSum, 0x00, 32);
*o_pnFileSize   = 0;

UINT uiCheckSum = 0;
if(strlen(pFilePath) <=0)
{
return FALSE;
}
FILE *fp;
fp=fopen(pFilePath, "rb");
if(NULL == fp)
{
    return FALSE;
}
fseek( fp, 0L, SEEK_END );
long lFileSize = ftell( fp );
*o_pnFileSize = lFileSize;
fseek( fp, 0L, SEEK_SET );

BYTE *pFileData = new BYTE [lFileSize];
fread(pFileData, lFileSize, 1, fp);
sha256_encode(pFileData, lFileSize, o_byObjCheckSum);

fclose(fp);
delete [] pFileData;

return TRUE;
}

How can I make this faster?

If it will help, this function is called here:

    STRNCPY_MEMSET(szResDirectoryPath, RESOBJ_DIRECTORY_PATH, MAX_PATH);
    if(FALSE == GetAllFileNameList(&vectFileNameList, szResDirectoryPath))
    {
        g_pFieldGlobal->WriteSystemLogEX(TRUE, "[ERROR] LoadResObjCheckList_ error !!, Directory(%s)\r\n", szResDirectoryPath);
        return FALSE;
    }

    nCnt = vectFileNameList.size();
    for(i=0; i < nCnt; i++)
    {
        SRESOBJ_CHECKSUM    resObjCheckSum;
        CGameData           tmMapGameData;
        char                szFileFullPath[MAX_PATH];
        MEMSET_ZERO(&resObjCheckSum, sizeof(resObjCheckSum));

        STRNCPY_MEMSET(resObjCheckSum.szResObjFileName, vectFileNameList[i].c_str(), SIZE_MAX_RESOBJ_FILE_NAME);
        _strupr(resObjCheckSum.szResObjFileName);
        sprintf(szFileFullPath, "%s/%s", szResDirectoryPath, resObjCheckSum.szResObjFileName);

        if(tmMapGameData.GetCheckSum(resObjCheckSum.byDigest, &resObjCheckSum.nFileSize, szFileFullPath))
        {
            m_mapResObjCheckSumList.insert(pair<string,SRESOBJ_CHECKSUM>(resObjCheckSum.szResObjFileName, resObjCheckSum));
        }
    }

In general, the file input takes the most amount of processing time.

Your CRC algorithm may also be consuming a lot of the processing time.

Here are some suggested optimizations:

Don't clear the checksum.  The CRC will write to it anyway.  This is
a wasted function call.
The strlen function searches the string to determine length.  Use
std::string instead, because it maintains the length of the string
and can return the string length faster.
Don't read the entire file into memory.  Pick a chunk size and use
that.  A seek to the end of file may consume a lot of time.  If you
must get the file size, use an OS API that will return the size of
the file; hopefully it will be faster than seeking to the end of the
file.
If your OS supports memory mapped files, you may want want to use
this.  The memory mapped files allows the OS to handle reading the
file into memory.
Don't keep allocating from the heap.  Create a large array once and
use that.  Memory allocation may become a bottleneck when the memory
becomes fragmented.
Turn on compiler optimizations to highest level.
Try creating multiple threads for processing.  For example, you could
spawn two threads, the main program would pass different filenames to
the threads as they complete their processing.
Reduce the number of branches; branches may force instruction
pipeline reloading, which wastes time that could be spent processing
data.  Research "loop unrolling".
Optimize data to fit into the cache.  Reloading of the cache for
items outside the cache wastes time.
If you can, do whatever it takes to keep the hard drive spinning.
There is an overhead associated with starting up the hard drive
motors.  Keeping the drive spinning reduces the need to restart the
motors.  See also "double buffering".
Optimize the CRC by performing it in parallel, if possible.


Dependency injection with asp.net identity and property injection with dbcontext I configure asp.net identity using Simple Injector

  var container = new Container();
  container.Options.DefaultScopedLifestyle = new WebRequestLifestyle();

  //Identity
  container.RegisterPerWebRequest<AppDbContext>();
  container.RegisterPerWebRequest<IUserStore<User, int>>(() => new UserStore<User, Role, int, UserLogin, UserRole, UserClaim>(new AppDbContext()));
  container.RegisterPerWebRequest<IRoleStore<Role, int>>(() => new RoleStore<Role, int, UserRole>(new AppDbContext()));
  container.RegisterPerWebRequest<AppRoleManager>();
  container.RegisterPerWebRequest<AppUserManager>();
  container.RegisterPerWebRequest<AppSignInManager>();

  container.RegisterPerWebRequest(() =>
  {
    if (HttpContext.Current != null && HttpContext.Current.Items["owin.Environment"] == null && container.IsVerifying)
    {
      return new OwinContext().Authentication;
    }
    return HttpContext.Current.GetOwinContext().Authentication;

  });

And configure a property injection for BaseController and my DbContext

My BaseController

 public class BaseController : Controller
  {
    public ISessionContext SessionContext { get; set; }
  }

My register in simple injector

  container.Register<ISessionContext,ClaimsSessionContext>();
  container.RegisterInitializer<BaseController>(ctrl =>
  {
    ctrl.SessionContext = container.GetInstance<ISessionContext>();
  });

My DbContext

  public class AppDbContext : IdentityDbContext<User, Role, int, UserLogin, UserRole, UserClaim>
  {
    public ISessionContext SessionContext { get; set; }
    public AppDbContext()
      : base("AppConnection")
    {
    }
    public static AppDbContext Create()
    {
      return new AppDbContext();
    }

And my simple injector register for property injection

   container.RegisterInitializer<AppDbContext>(db =>
      {
        db.SessionContext = container.GetInstance<ISessionContext>();
      });

I wonder if there would be no problem injecting  SessionContext property next to my DbContext, for every request it is also instantiated

Suggestions? This ok ?

There is any specific need to register those per single request ?

IUserStore
IRoleStore
AppRoleManager
AppRoleManager
AppRoleManager

Weird implementations apart they should be fine as "per application" removing a lot of overhead

Also, you probably can add the sessioncontext to AppDbContext constructor and make a single registration


Python Tkinter countdown app with a freezing GUI So I want to create a very basic constantly changing text bvariable with a counter function. I created it, and it is working well, but once I press the "countdown" button, the GUI freezes, and the only way to close the window is from PyCharm.

Any suggestons about what causes the freeze and how to fix it?

The program unfreezes when the countdown is finished, so it's just a hanging interface, not a crash. Written for Python 3.8.

from tkinter import *

class MyFirstGUI:
    def __init__(self, master):
        self.master = master
        master.title("A simple GUI")

        self.label = Label(master, textvariable=time)
        self.label.pack()

        self.greet_button = Button(master, text="Greet", command=self.greet)
        self.greet_button.pack()

        self.close_button = Button(master, text="Countdown", command=self.countdown)
        self.close_button.pack()

    def greet(self):
        print("Greetings!")

    def countdown(self):
        remaining = 100
        for i in range(100):
            time.set(remaining)
            remaining -= 1
            self.master.after(1000)
            self.master.update_idletasks()

root = Tk()
time = StringVar()

my_gui = MyFirstGUI(root)

root.mainloop()

Tkinter is already in a loop. The window handler gets all messed up because of your own loop, which is called synchronously. Effectively you're running a loop in a loop and wonder why the outer loop takes so long to get to the next step. That's because of the inner loop taking so much time.

Long story short, your next window update will only commence once the countdown loop is finished.

There's mainly 2 ways of fixing that.

Don't loop.
Use asynchronous (or threaded) loops instead.

Basically, you'll need a function start_countdown which starts a threaded version of countdown. That would probably look something like this:

import threading

class MyFirstGUI:
    # Init and rest of methods snipped for brevity
    def refresh(self):
        self.root.update()
        self.root.after(1000,self.refresh)

    def start_countdown(self):
        self.refresh()
        threading.Thread(target=countdown).start()

Now, the countdown will be calculated in a thread that doesn't halt the current loop (the Tkinter main window). With that as basis, see if you can rebuild your countdown method. You'll see it becomes more focussed on the actual calculations and less on the screen.

Please see this answer on Stack Overflow for the complete story on how to work with Tkinter and calls that would ordinarily block.


C++ Range based stream Hoping somebody has knowledge of the range based additions going into the standard. So they can point out all the things I need to add to make it compliant.
IterableStream.h
#include <iostream>
#include <iterator>

namespace ThorsAnvil::Util
{

template<typename T>
class IterableStream
{
    std::istream&      stream;
    public:
        IterableStream(std::istream& stream)
            : stream(stream)
        {}
        operator std::istream&()            {return stream;}

        std::istream_iterator<T> begin()    {return std::istream_iterator<T>(stream);}
        std::istream_iterator<T> end()      {return std::istream_iterator<T>();}
};

}

Example usage
#include "IterableStream.h"
#include <iostream>
#include <fstream>

namespace TaU = ThorsAnvil::Util;
int main()
{
    std::ifstream x("plop");

    for(auto const& y: TaU::IterableStream<int>(x)) {
        std::cout << y << "\n";
    }
}

I don't like the name - it ought to be more explicit that it's an input-only range.

Otherwise, it looks just as a range should, with the observation that we omit the usual const iterators, as they don't make sense here.

I think you can avoid repetition of the return type:

std::istream_iterator<T> begin()    {return {stream};}
std::istream_iterator<T> end()      {return {};}