PHP Validate JSON objects with a JSON schema I have a basic JSON API written in PHP. I want to validate incoming JSON data.

I know there are PHP JSON validators out there, here is a quick one I just rolled up.

<?php

$json_payload = json_decode('{
  "data": {
    "a_string": "short",
    "a_number": 7
  }
}');

validateJSONPayload(
  '{
    "data": {
      "a_string": "type=string,min_length=7,max_length=10",
      "a_number": "type=integer,min=8,max=50"
    }
  }',
  $json_payload
);

function validateJSONPayload($json_schema, $json_payload){

  $json_schema = json_decode($json_schema);

  function reCursiveCheck($schema_value, $schema_type, $payload_value, $payload_type){
    if($schema_type === "object" || $schema_type === "array"){
      # KEEP LOOPING
      foreach ($schema_value as $key => $key_val) {
        if(!property_exists($payload_value, $key)) echo "validation failed because the data does not match the schema
";
        reCursiveCheck($key_val, gettype($key_val), $payload_value->{$key}, gettype($payload_value->{$key}));
      }
    } else {
      # VALUE IS A STRING, NUMBER, BOOL, NULL
      $validation_param_strings = explode(",", $schema_value);

      # STORE THE PARAMS TO CALL THE VALIDATION FUNCTION
      $validation_params = array();

      foreach($validation_param_strings as $validation_param) {
        $params = explode("=", $validation_param);
        $validation_params[$params[0]] = $params[1];
      }
      // print_r($validation_params);
      validateProperty($payload_value, $payload_type, $validation_params);
    }
  }

  function validateProperty($payload_value, $payload_type, $validation_rule){
    if($payload_type !== $validation_rule["type"]) {
      echo "types do not match
";
    }
    # STRING VALIDATION
    if($validation_rule["type"] === "string"){
      if(strlen($payload_value) > $validation_rule["max_length"]) echo "string too long
";
      if(strlen($payload_value) < $validation_rule["min_length"]) echo "string too short
";
    }
    # NUMBER VALIDATION
    if($validation_rule["type"] === "integer"){
      if($payload_value > (int)$validation_rule["max"]) echo "number too large
";
      if($payload_value < (int)$validation_rule["min"]) echo "number too small
";
    }
    # NULL VALIDATION
    if($validation_rule["type"] === "null"){
      echo "found a null
";
    }
  }

  foreach($json_schema as $key => $value) {
    if(!property_exists($json_payload, $key)) echo "validation failed because the data does not match the schema
";
    reCursiveCheck($value, gettype($value), $json_payload->{$key}, gettype($json_payload->{$key}));
  }
}

?>

Your solution is very "functional programming" in approach, with nested function declarations and such.  This in of itself is not "wrong", it is just a little bit unusual in the PHP world.

I would consider moving this into an object-oriented paradigm which is more common in PHP.  I would think in this case perhaps only a class with static functions might be needed, so you could make calls against the validator like:

$result = JsonSchemaValidator::validate($jsonSchema, $jsonPayload);

If you find you have one schema that is used multiple times for validation, then perhaps it makes sense to have concrete validator objects like this:

$validator = new JsonSchemaValidator($jsonSchema);
$result1 = $validator->validate($jsonPayload1);
$result2 = $validator->validate($jsonPatload2);

This would save you the overhead of having to re-validate the schema with each validation request.

I am not sure which is more appropriate to your application, but wanted to present both options for putting this logic into a class.

Some odd naming conventions are in play:

Why name a variable $json_payload when it does not contain JSON at all, but rather an object built from JSON?  Perhaps $payload is better here.
Why are you mixing camelCase and snake_case in your code?  I know PHP is not very consistent here, but that doesn't mean you shouldn't be - at least within the confines of a single library, class, set of functions, etc.
Why uppercase first c in reCursiveCheck()?

I think your main validateJSONPayload() function name and signature is odd. As noted above, the "payload" is not JSON at all.  Why pass one parameter as a JSON string and the other as an actual data structure?  This seems to be an incongruous approach.  I would probably just pass JSON for both parameters so caller doesn't need to decode one of the parameters beforehand.

Why define validation rules in a string like "type=string,min_length=7,max_length=10"?  You are introducing complexity in your code to have to perform string manipulation to get at these values when they should be defined as appropriate properties/values on the schema.
Why not mirror the already well-established JSON Schema format?

Do not echo error/validation messages to standard output.  A validator like this should do one thing - validate, not deliver end user messaging.  Leave that up to functionality further up the call stack that is more well-positioned to understand the context for how end user messaging should be delivered.  This code might throw exceptions and/or log errors when it gets put into a bad state (i.e. invalid JSON is passed) but outside of that, it's contract to the caller should be to just deliver validation results and leave it up to the calling code to determine what action to take from there.

If you are delivering error messages, you should also remove any HTML markup from it.  What if this code is being used in a RESTful service where response is going to be JSON format and not HMTL?  Now you have to go strip out the HTML mark-up that has been added.

The takeaway here is to separate your display concerns from your business logic.

This code is incredibly fragile and strictly "happy path" in nature, as you are doing no validation of the passed dependencies and you just assume that all of the steps in the code will work properly.  Most critically, you just assume the passed schema JSON will decode properly.

What if it doesn't?

Right now, your code will just silently fail, perhaps giving the caller the perception that validation passed, since you currently do not return any validation result to the caller (you just echo out failures).

To extend on my earlier class-based example, perhaps you need to do something like:

try {
    $result = JsonSchemaValidator::validate($jsonSchema, $jsonPayload);
} catch (InvalidArgumentException $e) {
    // perhaps we had schema or payload which could not be decoded
    // and we have the code throw InvalidArgumentExpection
    // perhaps log the error
    error_log($e->getMessage());
    // then perhaps do something to recover
    $result = false;
}

or

try {
    $validator = new JsonSchemaValidator($jsonSchema);
    $result1 = $validator->validate($jsonPayload1);
    $result2 = $validator->validate($jsonPatload2);
} catch (InvalidArgumentException) {
    // do something
}

I echo the sentiment mentioned in comments to question above that I am really not quite sure why you would want to roll your own solution to this problem.


jQuery autocomplete for all place names in the UK I made an autocomplete input box of every town, city and village in the UK. Yes, every single one. The load times are painful so is there a way to reduce that?

Here is a sample:

<input type="text" id="location" name="locations" placeholder="Edinburgh, Belfast, Cardiff, London">
$( function() {
    var availableTags = [
      "Abberton",
      "Abbeytown",
      "Abbots Bromley",
      "Abbots Leigh",
      "Abbotsbury",
      "Abbotskerswell",
      "Abbotsley"
    ];
$("#location").autocomplete({
      source: availableTags
});
});

Here is a JSFiddle with the entire list, which has 7726 items.

You can improve performance by using autocomplete function with this options:

$( "#location" ).autocomplete({
      source: availableTags,
      delay: 0,
      minLength: 2,
});

This library has  a 300 ms delay by default. So setting delay to 0 disables it.
minLength is for loading the matches after the users typed at least 2 chars, I don't think there is a point to load them when you have only 1 character.

Good luck


What's in your file? Another more simple project from my CS1 class that I didn't upload while I was sick last week:

  Write a program that computes all of the following statistics for a
  file and outputs the statistics to both the screen and to another
  file: the total number of occurrences of characters in the file, the
  total number of non-whitespace characters in the file, and the total
  number of occurrences of letters in the file.

I must use at least two functions in this project. In addition, I am also supposed to count and output the number of occurrences of each character.

fileStats.cpp:

/**
 * @file fileStats.cpp
 * @brief
 * @author syb0rg
 * @date 10/23/14
 */

#include <iostream>
#include <sstream>
#include <fstream>

constexpr int ARRAY_LENGTH = 128;

struct Data
{
    size_t total = 0;
    size_t nonWhite = 0;
    size_t letters = 0;
    size_t charCount[ARRAY_LENGTH] = {0};
};

std::string constructString(Data data)
{
    std::stringstream sstm;
    sstm << "Number of characters: " << data.total << "\n";
    sstm << "Number of non-whitespace characters: " << data.nonWhite << "\n";
    sstm << "Number of letters: " << data.letters << "\n" << "\n";

    sstm << "Occurances of each character: " << "\n";
    for (int i = 0; i < ARRAY_LENGTH; ++i)
    {
        if(data.charCount[i]) sstm << (char) i << ": " << data.charCount[i] << "\n";
    }
    return sstm.str();
}

void outputToFile(std::string fileName, std::string str)
{
    std::ofstream out(fileName);
    out << str;
    out.close();
}

int main()
{
    Data data;
    std::ifstream file("test.txt");
    char input = '\0';

    while ((input = file.get()) && file.good())
    {
        data.total++;
        data.charCount[input]++;
        if(!isblank(input)) data.nonWhite++;
        if(isalpha(input)) data.letters++;
    }
    file.close();

    std::string result = constructString(data);
    outputToFile("output.txt", result);
    std::cout << result;
}

You can pass data by const& as it's not being modified.
ARRAY_LENGTH should probably be an unsigned type, such as std::size_t.
Just to stay safe, terminate the program if the file cannot be opened:

if (!file)
{
    std::cerr << "file cannot be opened";
    return EXIT_FAILURE;
}

You do not really need to call close() here.  It would only be necessary for checking for errors, otherwise the destructor will do this for you.  It shouldn't leak any resources.


Student data in a database I am working on an ASP.NET MVC 5 project, using the code-first approach for database design. I have a question about my code design.

I have an Entity in my database schema called Student:

public partial class Student
{
  public int Id { get; set; }
  public string RollNumber { get; set; }
  public string Name { get; set; }
  public string Address { get; set; }
  public string Mobile { get; set; }
  public string Email { get; set; }
}

Notice that I have declared this as partial because I extend it and include all the data access and processing functions for a student, like this:

public partial class Student
{
   [NotMapped]
   private MyDbContext context = new MyDbContext();

   public static Student Get(int id)
   {
     Student student;
     using(MyDbContext context = new MyDbContext())
     {
       student = context.Students.SingleOrDefault(s => s.Id == id)
     }
     return student;
   }

   public void Student Create()
   {
     context.Students.Add(this);
     context.SaveChanges();
   }

   //....
   //and similarly many other
}

Notice that I have declared a static Get function which takes an id and returns a Student object if it exists, otherwise null. I have declared this static because it already returns a Student object, so calling it on an object seems unreasonable to me (please correct me if I am wrong).

The second thing to notice is the NotMapped attribute which I've used to exclude this property from mapping to database when running migrations.

I am using the above mentioned design with my all entities (i.e. wrapping data and function processing over that data into a single object).

Is this design approach good? Is there any other better approach exist, or am I on the wrong path?

I'm no big fan of using partial classes because I find it less readable. Like the other commenter I think you should look at the repository pattern for a cleaner solution.
It seems a bit odd that you give Student the responsibility to save itself to the context.

Something in the same neighbourhood is the UnitOfWorkPattern. That said the DbContext is an implementation of UnitOfWork and Repository Pattern. You could use the DbContext directly or with a wrapper of your own, but if your wrapper doesn't add value to the DbContext you could skip it.

You are initializing your DbContext multiple times which seems redundant. Most likely you get the same instance since it implements the UnitOfWork pattern, but the "new" and private variables combined with static methods will make it very hard to write unit-tests. For testability you should avoid having MyDbContext in a private variable. You can use constructor injection to make it available to the class, or keep it in a property that you can substitute in tests later.


Return true from enumerable that calls function unless that function ever returns false Is there a tighter way to write this code?

foreach(var processName in processNames)
{
    bool launched = launch(processName);
    if (!launched)
    {
        return false;
    }
}
return true;

I'm not sure what this kind of approach is called. But basically if any false the whole thing is false, otherwise it's true. The closest I can get is:

return !processNames.Any(process => !launch(process));

but I think the first approach is better because it's easier to understand.

Usually positive conditions are much easier to understand than negative ones and even easier then two negatives in the same expression. You can flip it and get rid of both negations by using All instead:

return processNames.All(process => launch(process));

Now that the nagations are gone you can further shorten this by removing the lambda:

return processNames.All(launch);

This is possible because the launch method signature matches the delegate  Func<string, bool> required by All.


Converting recordsets to POCO The goal this method was to emulate the way EF or Dapper return to you concrete POCO classes instead of recordsets when querying your db. Our current code is riddled with iterations over recordsets and I want to consolidate to an extension method that can convert the recordset into an Enumerable<T>.

Review

I'm concerned about how efficient this will be. When querying the db for a single or handful of records I'm not concerned but we sometimes query for an entire table (i.e. ~100,000 records) and in that case will the iterations in this start to cause performance issues? Is there a better way to resolve the recordsets to classes without iterating? Any other constructive remarks appreciated.

public static IEnumerable<T> AsList<T>(this Recordset rs) where T : new()
{
    IEnumerable<PropertyInfo> props = new List<PropertyInfo>(typeof(T).GetProperties()).Where(p => p.CanWrite);
    List<T> results = new List<T>();
    while (!rs.Eof)
    {
        T obj = new T();
        foreach (PropertyInfo property in props)
        {
            var val = rs[property.Name].Value;
            if (val != null) property.SetValue(obj, Convert.ChangeType(val, property.PropertyType));

        }
        rs.MoveNext();

        results.Add(obj);
    }

    return results.AsEnumerable();
}

used as...

Recordset rs = new Recordset();
rs.Open("select * from Users", connection)
IEnumerable<User> results = rs.AsList<User>();

Post Review Update

After great feedback from Heslacher and t3chb0t the following code has been finalized. Note there was a change in the requirements and sister method was created As<T>() to create a single class instance from a recordset. The feedback from the review has been applied to it as well.

public static IList<T> AsList<T>(this Recordset rs) where T : class, new()
{
    var results = new List<T>();

    if (rs == null) throw new ArgumentNullException("rs");
    if (rs.Eof) return results;

    IList<PropertyInfo> properties = GetProperties<T>();

    while (!rs.Eof)
    {
        results.Add(CreateInstance<T>(properties, rs));
        rs.MoveNext();
    }

    return results;
}

public static T As<T>(this Recordset rs) where T : class, new()
{
    if (rs.Eof) return default(T);
    IList<PropertyInfo> properties = GetProperties<T>();

    return CreateInstance<T>(properties, rs);
}

private static T CreateInstance<T>(IEnumerable<PropertyInfo> properties, Recordset rs) where T : new()
{
    T obj = new T();
    foreach (PropertyInfo property in properties)
    {
        var val = rs[property.Name].Value;
        if (val != null && !Convert.IsDBNull(val)) property.SetValue(obj, Convert.ChangeType(val, property.PropertyType));
    }

    return obj;
}

private static IList<PropertyInfo> GetProperties<T>()
{
    return new List<PropertyInfo>(typeof(T).GetProperties().Where(p => p.CanWrite));
}

public static IEnumerable<T> AsList<T>(this Recordset rs) where T : new()
{
    ..
    List<T> results = new List<T>();
    ..
    return results.AsEnumerable();
}

This might be very inefficient because when I see that a method returns an IEnumerable<T> I assume its execution is deferred so I automatically do AsList(..).ToList() to execute it but what it actually does, is to iterate the list twice. First time to create the first list and then to create the second list. You should either create a real deferred method or return the List<T> so there is no doubt about the result.

IEnumerable<PropertyInfo> props = new List<PropertyInfo>(typeof(T).GetProperties()).Where(p => p.CanWrite);

You are executing this Where query for each record because the list wrapps only the GetProperties part (which is an array anyway so I don't understand why you convert it into a list in the first place - linq can work with arrays). You need to call the ToList extension after the Where

var properties = typeof(T).GetProperties().Where(p => p.CanWrite).ToList();

I also suggest moving the object creation into a separate method so that you don't have any nested loops like this:

public static IEnumerable<T> AsList<T>(this Recordset rs) where T : new()
{
    var properties = typeof(T).GetProperties().Where(p => p.CanWrite).ToList();
    while (!rs.Eof)
    {
        yield return CreateInstance<T>(properties, rs);
        rs.MoveNext();
    }
}

private static T CreateInstance<T>(IEnumerable<PropertyInfo> properties, Row row) where T : new()
{
    var obj = new T();
    foreach (var property in properties)
    {
        var value = row[property.Name].Value;
        if (value != null) property.SetValue(obj, Convert.ChangeType(value, property.PropertyType));
    }
    return obj;
}


Pure methods for updating java objects I've been reading about pure methods and immutability in Java. I have a current application and want to convert all the methods to pure methods.

My app takes payment from users. I first create a payment in the DB:

Payment payment = new Payment();
payment = createPayment(orderId, status, address1, address2, country, state, price);

// createPayment() creates a row in the database table, saving orderId, status, address1, address2, country, state, price
// state is the inital state of the payment, in this cased INITIATED
createPayment() returns a Payment model

// make the payment with the payment processor
PaymentResult paymentResult = paymentService.pay(payment);

PaymentResult contains properties such as authCode, pspReference, resultCode, refusalReason.

The Payment model also contains authCode etc. properties. I now want to update my Payment model, including its status. I currently have:

paymentService.updatePaymentFromPaymentResultOnAuthorise(paymentResult, payment);

And updatePaymentFromPaymentResultOnAuthorise is:

public void updatePaymentFromPaymentResultOnAuthorise(PaymentResult paymentResult, Payment payment) throws PaymentException {

    try {

        boolean success = !isNullOrEmpty(paymentResult.getAuthCode());

        if (success) {
            payment.setLastStatus(payment.getStatus());
            payment.setStatus(PaymentStatus.PAID);
        }

        payment.setPspReference(paymentResult.getPspReference());
        payment.setAuthCode(paymentResult.getAuthCode());
        payment.setResultCode(String.valueOf(paymentResult.getResultCode()));
        payment.setRefusalReason(paymentResult.getRefusalReason());

        paymentDAO.updatePayment(payment);

    } catch (Exception e) {
        throw new PaymentException(String.format("there was an error updating the payment with paymentId %s", payment.getId()));
    }
}

As you can see, updatePaymentFromPaymentResultOnAuthorise is not a pure method as it changes the argument payment. This pattern is repeated throughout the app.

Is the only way to make this method pure to create a copy of payment within updatePaymentFromPaymentResultOnAuthorise and return this new object? This seems expensive and it will need to be done in many places throughout my app. But is that my only option?

The biggest problem I see is that the objects you pass around in EJB3 are part of the transaction. If you try to reload the payment again from the EntityManager, you will get back the exact same instance you are trying to avoid changing. You will have to jump through hoops in order to reload the payment as a completely new object and that pretty much negates anything that makes pure methods less error prone.

Following best practises is a valid reason but in this case I get the feeling that the baby gets thrown out with the bath water. Don't feel bad, though. EJB3 transaction model is not intuitive at all, needs very good project documentation and guidelines and it still needs constant care taking anyway.


Website monitor - if site is down, kill the process, contact admin and restart server My Python-based web server (Tornado module) is pretty complex and keeps going down (seems like usually in the middle of the night), so here's a bash script that kills the process, restarts and emails and txt messages me:

#!/bin/bash
NOTIFYEMAIL=<my_email_address>
SMSEMAIL=<cell_phone_number>@<carrier> # ie. pm.sprint.com
SENDEREMAIL=admin@loclahost
SERVER=http://the_website.com/
PAUSE=60
FAILED=0
DEBUG=0

while true
do
/usr/bin/curl -sSf --max-time 10 $SERVER > /dev/null 2>&1
CS=$?
# For debugging purposes
if [ $DEBUG -eq 1 ]
then
    echo "STATUS = $CS"
    echo "FAILED = $FAILED"
    if [ $CS -ne 0 ]
    then
    echo "$SERVER is down"

    elif [ $CS -eq 0 ]
    then
    echo "$SERVER is up"
    fi
fi

# If the server is down and no alert is sent - alert
if [ $CS -ne 0 ] && [ $FAILED -eq 0 ]
then
    PID=`ps -eaf | grep 'python -m fore.server' | grep -v grep | awk '{print $2}'`
    FAILED=1
    if [ $DEBUG -eq 1 ]
    then
    echo "$SERVER failed"
    if [[ "" !=  "$PID" ]]
    then
        echo "killing $PID"
        kill -9 $PID
        echo "Starting the server."
        python -m fore.server
    else
        echo "No Python Process Running Locally"
    fi
    fi
    if [ $DEBUG = 0 ]
    then
    echo "$SERVER failed"
    PID=`ps -eaf | grep 'python -m fore.server' | grep -v grep | awk '{print $2}'`
    if [[ "" !=  "$PID" ]]
    then
        kill -9 $PID
        echo "$SERVER went down $(date)" | /usr/bin/mail -s "$SERVER went down" "$SENDEREMAIL" "$SMSEMAIL"
        echo "$SERVER went down $(date)" | /usr/bin/mail -s "$SERVER went down" "$SENDEREMAIL" "$NOTIFYEMAIL"
        python -m fore.server
    else
        echo "No Python Process Running Locally"
    fi
    fi

# If the server is back up and no alert is sent - alert
elif [ $CS -eq 0 ] && [ $FAILED -eq 1 ]
then
    FAILED=0
    if [ $DEBUG -eq 1 ]
    then
    echo "$SERVER is back up"
    fi
    if [ $DEBUG = 0 ]
    then
    echo "$SERVER is back up $(date)" | /usr/bin/mail -s "$SERVER is back up again" "$SENDEREMAIL" "$SMSEMAIL"
    echo "$SERVER is back up $(date)" | /usr/bin/mail -s "$SERVER is back up again" "$SENDEREMAIL" "$NOTIFYEMAIL"
    fi
fi
sleep $PAUSE
done

I have to execute my server with sudo in order to use port :80 so I am executing the script like:

sudo ./outage.sh

Of course, warnings, suggestions, improvements are welcome.

One improvement I'm interested in implementing is to iterate through a list of servers in order to keep track of multiple web sites.

Indent code blocks

To improve readability,
it's good to indent code blocks in loops and if statements.
For example:

while true
do
    /usr/bin/curl -sSf --max-time 10 $SERVER > /dev/null 2>&1
    CS=$?

You indented some if statements, but not all. Indent all.

Replace elif with else if possible

In these conditions:

    if [ $CS -ne 0 ]
    then
    echo "$SERVER is down"

    elif [ $CS -eq 0 ]
    then
    echo "$SERVER is up"

If $CS is not-not-zero, then when can it be? Zero, of course!
So the elif can be a simple else.

Use $(...) instead of `...`

`...` is old-fashioned and error prone. Use $(...) instead.

ps ... | grep ... | grep -v grep

Instead of:

ps -eaf | grep 'python -m fore.server' | grep -v grep

A common workaround is:

ps -eaf | grep '[p]ython -m fore.server'

This works, because this way in the output of ps, the process python -m fore.server will still match, but grep '[p]ython -m fore.server' will not match.

However, in this particular example you should go a bit further, see the next point.

Look suspiciously at grep in a pipeline ending with awk

awk is fast and powerful,
and very often can easily do grep's job too when it's in the same pipeline.

For example instead of this:

ps -eaf | grep 'python -m fore.server' | grep -v grep | awk '{print $2}'

You can rewrite the whole thing using awk alone:

ps -eaf | awk '/python -m fore.server/ {print $2}'

This is not only shorter and less troublesome (no need for the grep [p]ython trick),
and there are less processes executed in the pipeline.

Don't repeat yourself

You do some things twice, with identical or very similar commands:

Finding the PID of the Python process
Sending an email with the message that the server is down
Sending an email with the message that the server is back up

It would be better to move common operations to helper functions.

Other simplifications

Instead of this:

    if [[ "" !=  "$PID" ]]

You can write as:

    if [[ $PID ]]


Program to print a Python list as "a, b, and c" I am a beginner (with zero Python experience) working through the book Automate the boring stuff with Python. At the end of Chapter 4, the author provides a review problem.

I was supposed to write a function that takes a list value as an argument and returns a string with all the items separated by a comma and a space, with and inserted before the last item. Ex: 'apples, bananas, tofu, and hot dogs'. My function should be able to work with any list value passed to it.

I pulled together working code for the problem, but I cannot help but feel like there are concepts I am not quite understanding.  My solution is long, compared to the answers for a related Stack Overflow question. I feel that there must be redundant code in my answer. Could I bother someone to please point out obvious logical errors or areas where I use redundant code?

That said, I did include an error checking function (slightly outside the scope of the question) since we briefly learned about try and except usage in the previous chapter, and because I don't like the idea that a capital letter at the beginning of the input is able to break the program.

# defining possible lists
lists = ['cars','favorite foods','family members']
x = ['Hyundai','Acura','Honda','Suzuki','Ford']
y = ['apples','bananas','tofu','hot dogs']
z = ['Mother','Father','Dog','Cat','Grandmother','Grandfather','Cousins','Host family']

# defining functions
def listselection():
    print('The following lists exist within the system. Which would you like to see?')
    print(lists)

def errorchecking():
    global option

    while True:
        try:
            option = str(input())
        except:
            print('Please type letters, not numbers.')
            continue

        if option == 'cars':
            break
        elif option == 'favorite foods':
            break
        elif option == 'family members':
            break
        else:
            print('Please re-enter your selection')

    return option

def listconcatenation():
    global spam
    global listitem

    listitem = 'Here are the items in my list: '

    if option == 'cars':
        spam = x
    elif option == 'favorite foods':
        spam = y
    elif option == 'family members':
        spam = z

    for i in spam:
        if spam.index(i) < (len(spam)-1):
            listitem = listitem + i + ', '
        else:
            listitem = listitem + 'and ' + i + '.'

    return listitem

# run program
listselection()
errorchecking()
listconcatenation()
print(listitem)

# defining possible lists

Don't write comments that describe what the code does; we can see the code, we know what it's doing. If you ever need comments, it's where it's otherwise unclear why the code's doing it.

lists = ['cars','favorite foods','family members']

Per the style guide, you should put spaces after the commas:

lists = ['cars', 'favorite foods', 'family members']

x = ['Hyundai','Acura','Honda','Suzuki','Ford']

x is a terrible name for this list (see also: y, z). Names should be more descriptive:

cars = ['Hyundai', 'Acura', 'Honda', 'Suzuki', 'Ford']

But given that lists is supposed to refer to the three lists below it, why not use a dictionary instead:

lists = {
    'cars': ['Hyundai', 'Acura', 'Honda', 'Suzuki', 'Ford'],
    ...
}

def listselection():
    print('The following lists exist within the system. Which would you like to see?')
    print(lists)

The style guide recommends snake_case names for functions. It's helpful to include a docstring when you define a function. Given the above switch to a dictionary, we need to change the second line slightly:

def list_selection():
    """Show the user the selection of available lists."""
    print('The following lists exist within the system. Which would you like to see?')
    print(list(lists))

Note that by default you get a dictionary's keys when you iterate over it, so list(lists) == ['cars', ...].

def errorchecking():
    global option

    while True:
        try:
            option = str(input())
        except:
            print('Please type letters, not numbers.')
            continue

        if option == 'cars':
            break
        elif option == 'favorite foods':
            break
        elif option == 'family members':
            break
        else:
            print('Please re-enter your selection')

    return option

Using a while True loop to take and validate input is a good idea, but there are a few problems here:

Why do you use global option? You return option at the end of the function, which is the way you ought to provide it to other functions. Using global state is a bad pattern, get out of the habit of doing it.
Your error checking doesn't actually do anything. The result of input(...) is always a string, str(input(...)) will never cause an exception. Also your error checking is too broad; bare except is a bad idea, you should be specific about the kind of errors you expect and allow unexpected exceptions to get raised up. "Errors should never pass silently. Unless explicitly silenced."
This part:

if option == 'cars':
    break
elif option == 'favorite foods':
    break
elif option == 'family members':
    break
else:
    print('Please re-enter your selection')

is not very DRY (Don't Repeat Yourself); we already have those options, they're the keys to our dictionary (or the values of your list in the original). Therefore we could write:

if option in lists:
    break
else:
    print('Please re-enter your selection')

With all of the above changes the function is now a lot more concise:

def error_checking():
    """Take valid input from the user."""
    while True:
        option = str(input())
        if option in lists:
            return option  # this will also end the loop
        print('Please re-enter your selection')

Now it's clear that error_checking doesn't really describe what this function does; that's only part of its job, but the name should tell you how to use it. I would call it something like get_valid_input.

def listconcatenation():
    global spam
    global listitem

    listitem = 'Here are the items in my list: '

    if option == 'cars':
        spam = x
    elif option == 'favorite foods':
        spam = y
    elif option == 'family members':
        spam = z

    for i in spam:
        if spam.index(i) < (len(spam)-1):
            listitem = listitem + i + ', '
        else:
            listitem = listitem + 'and ' + i + '.'

    return listitem

Same things again: put underscores in the names; remove the global state (make option a parameter instead, you already return listitem). But there are three more specific improvements:

The dictionary we introduced above means you can go from this:

if option == 'cars':
    spam = x
elif option == 'favorite foods':
    spam = y
elif option == 'family members':
    spam = z

to this:

item_list = lists[option]  # note also a more descriptive name

That said, why do this inside list_concatenation? That makes that function very tightly coupled to the place where you're currently using it, it's not reusable at all. Instead, why not pass in the selected list, rather than the option name, as a parameter?

Similarly including things like 'Here are the items in my list: ' and the trailing full stop '.' within the function reduce its reusability.
In terms of looping in Python:

for i in item_list:
    if item_list.index(i) < (len(item_list)-1):
        listitem = listitem + i + ', '
    else:
        listitem = listitem + 'and ' + i + '.'

The i would usually suggest an index, perhaps item would be a better name. If you want the index, rather than using spam.index(item) you can enumerate over the list:

for index, item in enumerate(item_list):
    if index < (len(item_list) - 1):

This is more efficient as you're only passing over the list once, rather than every time you need the index. Note the additional whitespace, again per the style guide.

With all of the above, we have to change the invocation of the functions:

list_selection()
item_list = lists[take_valid_input()]
result = list_concatenation(item_list)
print('Here are the items in my list: {}.'.format(result))

Now the data is moving through the functions in a logical way, rather than being updates to the global module state, and list_concatenation is a very generic, reusable function.

I might also be inclined to move list_selection inside take_valid_input, or even write a more generic function for user input that:

Takes a dictionary of options;
Displays the option names to the user;
Loops until they've entered one of the option names; and
Returns the one they've entered.


Object oriented hangman game I'm a hobbist trying to become a professional software developer. I do have a lot interest in Data Science in general and my goal is to master Python before diging deeper into specific data science topics.

I created this game when I started studying object oriented programming. Thank you all in advance.

import os
import getpass

class Hangman:
    winner = False
    word = ''
    mask = ''
    mistakes = []

    def __init__(self, word):
        self.word = word
        self.mask = '_ ' * len(word)

    def make_move(self, letter):
        letter_match = self.word.find(letter)
        if letter_match >= 0:
            for i, w in enumerate(self.word):
                if w == letter:
                    new_mask = list(self.mask)
                    new_mask[i * 2] = letter
                    self.mask = ''.join(new_mask)

            self.check_winner()
        else:
            self.mistakes.append(letter)
            print('Wrong!')

    def check_winner(self):
        if self.mask.find('_') < 0:
            self.show_board()
            print('You won!')
            self.winner = True

        return self.winner

    def check_looser(self):
        if len(self.mistakes) > 4:
            print('You loose! The word was %s' % self.word)
            quit()

    def show_board(self):
        os.system('cls')
        print(self.mask)
        print(self.mistakes)

    def play(self):
        while not self.winner:
            self.show_board()

            letter_input = input('Choose a letter: ').upper()
            self.make_move(letter_input)
            self.check_looser()

secret_input = getpass.getpass('Secret Word: ').upper()

game = Hangman(secret_input)
game.play()

It might be slightly cleaner and more performant to keep self.mask as a list, and just join it before printing:

# Basically what you had before, without the space
# Just "list multiplication" instead of "string multiplication"
self.mask = ['_'] * len(word)

. . .

if w == letter:
    mask[i] = letter

. . .

if '_' not in self.mask:
    self.show_board()
    print('You won!')

. . .

print(' '.join(self.mask))

Now there's less converting back and forth between strings and lists. This also saves you from needing to do i * 2. The spaces can just be added in using join before being printed, so you don't need to worry about them.

print(self.mistakes) currently prints out a normal list representation to the end user without any context. I'd probably prefix a message to that, and format it a bit before printing:

print(f"Already tried: {', '.join(self.mistakes)}")
# Already tried: R, Q

I'd probably get rid of self.winner and change how you're checking for winning. Notice what you're using it for: you assign it in check_winner, then return that same value that you just assigned, but never use the return value. I'd have check_winner just return the value, move the call to check_winner to after the call to make_move, and use the return value:

def check_winner(self):
    if '_' not in self.mask:
        self.show_board()
        print('You won!')
        return True

    return False

. . .

def play(self):
    won = False  # Just track this locally instead

    while not won:
        self.show_board()

        letter_input = input('Choose a letter: ').upper()
        self.make_move(letter_input)

        won = self.check_winner()  # And assign it here
        self.check_loser()

I'd also probably change check_loser (As a heads up, it's spelled "loser". "Looser" means that one thing is "less tight" than something else). I'd maybe have it return a bool saying whether or not the player lost, and just return from that function instead. As I noted in a previous review (at the bottom), quit can cause problems when testing code. Just returning from the main loop to allow control to pass back to the REPL is much cleaner.


Parameterizing a common template I need to execute the same set of statements but with a different name of the field each time. I was wondering if there is a better way to write this?

int returnCode = collectValue("empName", record);
if(returnCode <0)
{
return false;
}

returnCode = collectValue("empNum", record);
if(returnCode < 0)
{
return false;
}

returnCode = collectValue("empSalary", record);
if(returnCode < 0)
{
return false;
}

There are two primary ways I can think of to change this:

Iterate over an array

String[] fieldNames = new String[]{ "empName", "empNum", "empSalary" };
for (String field : fieldNames) {
    if (collectValue(field, record) < 0) {
         return false;
    }
}

Boolean short-circuiting

boolean returnFalse = collectValue("empName", record) < 0 || collectValue("empNum", record) < 0 || collectValue("empSalary", record) < 0;
if (returnFalse) {
    return false;
}

I would personally recommend the first version, as the second version creates a long statement and is a bit more difficult to both read and debug.


Ruby methods to enable and disable attributes I have a Ruby code segment which I need to DRY up. Can it be done without meta programming?

def disable_attribute
  statement1
  @response = Client::Service.disable_attribute(
    param1: param1value,
    param2: param2value
  )
  statement2
end

def enable_attribute
  statement1
  @response = Client::Service.enable_attribute(
    param1: param1value,
    param2: param2value
  )
  statement2
end

How about:

def toggle_attribute
  statement1
  @response = yield(
    param1: param1value,
    param2: param2value
  )
  statement2
end

def disable_attribute
  toggle_attribute { |params| Client::Service.disable_attribute(params) }
end

def enable_attribute
  toggle_attribute { |params| Client::Service.enable_attribute(params) }
end

If you want to be more succinct, you could:

def toggle_attribute(method_name)
  statement1
  @response = Client::Service.send(method_name,
    param1: param1value,
    param2: param2value
  )
  statement2
end

def disable_attribute
  toggle_attribute(:disable_attribute)
end

def enable_attribute
  toggle_attribute(:enable_attribute)
end

And if you want to really impress your friends, you could write:

def toggle_attribute(method_name)
  statement1
  @response = Client::Service.send(method_name,
    param1: param1value,
    param2: param2value
  )
  statement2
end

def method_missing(method_name, *args)
  %i[disable_attribute enable_attribute].include?(method_name) ?
    toggle_attribute(method_name) : super
end


sportsRunning and sportsWalking There are two functions, sportsRunning and sportsWalking, that will render different text. For this, I am using an if condition at the bottom to display corresponding div:

{sportsDescription.sportsDate && sportsDescription.sportsDate.length > 0 && sportsDescription.sportsDate !== '2199-01-01' ?
                this.sportsWalking() : this.sportsRunning()}`

Can you tell me how I can refactor the code?

sportsRunning() {
    const sportsFire = this.props.profile.firstName;
    return (

{`Want some advice${sportsFire && sportsFire.length > 0 ? `, ${sportsFire}` : ''}?`}

Stay focused by setting a target date for completion of your sports.

                Set Target Date

    );
}

sportsWalking() {
    const sportsFire = this.props.profile.firstName;
    return (

{`Want a helpful tip${sportsFire && sportsFire.length > 0 ? `, ${sportsFire}` : ''}?`}

Add some momentum to your account by setting up automatic recurring deposits.

                Set Up Automatic Deposits

    );
}

render() {
    const {sportsDescription, data} = this.props;
    const sportsAmount = parseFloat(sportsDescription.sportsAmount);
    const accumulatedAmount = data.summary.total || 0;

    return (

                {
                    sportsDescription.error ?
                        this.renderProgressError() :
                        this.renderProgress({
                            accumulatedAmount,
                            sportsAmount
                        })
                }

Your Goal Progress

You have accumulated = 0 ? null : 'negative'}>
                        {moneyFormat(accumulatedAmount)} towards your sports of
                        {moneyFormat(sportsAmount)}.

                    {
                        sportsDescription.sportsDate && sportsDescription.sportsDate.length > 0 &&
                        sportsDescription.sportsDate !== '2199-01-01' && this.renderGoalDate(sportsDescription.sportsDate)
                    }

                        Edit Goal Details

            {sportsDescription.sportsDate && sportsDescription.sportsDate.length > 0 && sportsDescription.sportsDate !== '2199-01-01' ?
                this.sportsWalking() : this.sportsRunning()}

    );

This is what I mean with pureRenderComponent:

const SportsMoving = ({editHandler, sportsDescription, ...props }) => {

   const isRunning = sportsDescription.sportsDate && sportsDescription.sportsDate.length > 0 && sportsDescription.sportsDate !== '2199-01-01';
   const sportFire = props.profile.firstName || "";

   return (

            {
              isRunning
                ? `Want some advice ${sportFire}?`
                : `Want a helpful tip ${sportsFire}?`
            }

            {
              isRunning
                ? "Stay focused by setting a target date for completion of your sports."
                : "Add some momentum to your account by setting up automatic recurring deposits."
            }

               {
                 isRunning
                   ? "Set Target Date"
                   : "Set Up Automatic Deposits"
               }

   )
}

now in your parentComponent you can use it like so:

     ...

     <SportsMoving editHandler={this.editHandler} ...props />


Making a webapp using Bottle Here is a screenshot of my setup:

Here is the actual code:

#activate python virtualenv
activate_this = 'C:/Users/D/Dropbox/_mypyapp/learnbottle/learnbottle/scripts/activate_this.py'
execfile(activate_this, dict(__file__=activate_this))

#import dependencies
import os
from bottle import route, run, template

#this is what will become the page html
index_html = '''My First web app! By {{ author }}'''

@route('/:anything')
def something(anything = ''):
    return template(index_html, author = anything)
@route('/')
def index():
    return template(index_html, author = 'your name here')

if __name__ == '__main__':
    port = int(os.environ.get('PORT', 8080))
    run(host = '0.0.0.0', port = port, debut = True)

Couple things I should note:

This is my first time using virtualenv
This is the first time I am using git
This is my first time writing any sort of web app or using a framework

I'm following along with a tutorial on realpython.com. The tutorial is written for a "Unix-based environment" so I had to do some things to get it working on Windows. Not sure if I should go the route of setting up a VM as it seems unnecessarily complicated.

So, am I making any newbie mistakes and is there anything I should do differently?

Don't activate the virtualenv inside the script. If you do, the script won't be portable, it won't run on other computers, as that absolute path is specific to your computer. The virtualenv is part of the runtime execution environment, should not be part of the source code. Add a wrapper script that will activate the virtualenv and then run your app.

@route('/:anything')
def something(anything = ''):
    return template(index_html, author = anything)

I don't know bottle, but it seems to me that the default value setting anything = '' is pointless, because the path / should not match this route.

PEP8 dictates to put 2 empty lines before every global function declaration. So put 2 empty lines right before this code:

@route('/')
def index():
    return template(index_html, author = 'your name here')

This looks like a typo:

run(host = '0.0.0.0', port = port, debut = True)

You probably meant debug instead of "debut".


Sencha touch with XML Could you please review the following?  My concern is XML parsing is hardcoded:

    Ext.Ajax.request({
        url: storageDataItem.contentlink,
        success: function (response) {
            var store;
            try {
                var parser = new DOMParser();
                // TODO: remove extra lines or ignore on parsing. How? :)
                var xml = parser.parseFromString(response.responseText, "text/xml");
                var mediaContentURL = response.responseXML.childNodes[0].childNodes[0].childNodes[1].lastChild.attributes[0].nodeValue;
                var title = xml.getElementsByTagName('title')[1].childNodes[0].nodeValue;
            } catch (e) {
                // ignore exception
            }
            console.log("storage: ")
        }
    });
    this.fireEvent("playButtonTapped", storageDataItem);
    this.fireEvent("playAudioFile", title, mediaContentURL);

The actual feed

From looking a little into this

You derive mediaContentURL through response.responseXML and title through xml, I would stick in both cases to xml
As you guessed, your approach to getting the mediaContentURL is pretty terrible.
Use getElementsByTagName and attributes, do not hardcode array indexes
Do not use console.log in production code

I would use something like this:

function getItemTitle( xml , itemNumber )
{
  //Skip the title number of the playlist
  return xml.getElementsByTagName('title')[itemNumber+1].textContent
}

function getItemURL( xml , itemNumber )
{
  return xml.getElementsByTagName('content')[itemNumber].attributes.getNamedItem('url').value;
}


Multiple jQuery promises I'm starting to reuse this pattern, and was wondering if there was a more succinct/clear way to write it.

Given a function foo taking a callback argument:

function foo(..., cb) { ... }

And function bar which calls foo with several different arguments taking a callback function of its own to be called when all the 'dependent' callbacks have been called, I write:

function bar(data, cb) {
    var promises = [];
    function createPromise() {
         var p = $.Deferred();
         promises.push(p);
         return function() { p.resolve(); };
    };
    for (var i = 0; i < data.length; i++) {
         foo(data[i], createPromise());
    }
    $.when.apply($, promises).then(cb);
}

I'm quite happy with the pattern, but the apply and closure-returning-a-function make me suspect that there might be a simpler way of doing the same... Ideas?

Well, first of all, your closure doesn't have to create a function. This will work just as well:

function createPromise() {
     var p = $.Deferred();
     promises.push(p);
     return p.resolve; // Doesn't create an anonymous function
};

Second of all, if you're doing this pattern a lot, I would make a generic method that lets you convert any function that accepts a callback to one that returns a promise:

function callbackToPromise(func) {
    return function() {
        var args = $.makeArray(arguments);

        var p = $.Deferred();
        args.push(p.resolve); // Add the deferred as a callback

        func.apply(this, args);

        return p.promise();
    };
}

var improvedFoo = callbackToPromise(foo);

function bar(data, cb) {
    var promises = [];
    for (var i = 0; i < data.length; i++) {
         promises.push(improvedFoo(data[i]));
    }
    $.when.apply($, promises).then(cb);
}

$.when really should accept an array of Deferred's as an argument, but until that feature is added, I don't think there's a way to avoid using .apply here, except to add another helper function:

function when(promises) {
    return $.when.apply($, promises);
}

function bar(data, cb) {
    var promises = [];
    for (var i = 0; i < data.length; i++) {
         promises.push(improvedFoo(data[i]));
    }
    when(promises).then(cb);
}

A slightly less-readable version can eliminate the promises variable entirely, though I prefer the more-readable version above:

function bar(data, cb) {
    when($.map($.makeArray(data), function(item) {
        return improvedFoo(item);
    })).then(cb);
}


Toggling checkboxes based on certain conditions I have a bunch of checkboxes that need to be toggled based on certain conditions. My function is called by multiple buttons on an onclick event. The certain condition is expressed as a string (check_all, uncheck_all, check_all_virtual). The problem that I see is that if the function would be called as toggleCheckboxes('typo'), that the error wouldn't be so easy to see as there would be no error message. Such problems should probably yield more information. How could this be done better?

    <input name="site_ids[]" data-is-virtual="0" value="1" class="checkbox"/> SG3
    <input name="site_ids[]" data-is-virtual="1" value="2" class="checkbox"/> SG1
    <input name="site_ids[]" data-is-virtual="0" value="3" class="checkbox"/> SG2

    <button onclick="toggleCheckboxes('check_all')">Check All</button>
    <button onclick="toggleCheckboxes('uncheck_all')">Uncheck All</button>
    <button onclick="toggleCheckboxes('check_all_virtual')">Check All Virtual</button>

The functionality is correct. Checking all virtual shouldn't uncheck non-virtual.

I may have misunderstood something but if you just want to be able to detect case where the input string is none of expected, you may do something like that:

default:
  console.log("This operation '"+operation+"' is not supported, it will be ignored");

at the end of your switch\case construction


WPF canvas drawing very slow I am building a map editor to easily create 2d levels. To visualize how my level looks like I am using a canvas. On this canvas I can select which block is filled with a certain type ( like solid ). I did add click and drag but because I am live drawing on the canvas it goes very slow and if you draw at a decent speed it skips several block which needs to be drawn.

I already have tried several options ( doing it in another thread. Putting it first in a queue ) but none had a good result. It works now but not with the perfomance I prefer. Because it skips blocks it needs to draw.

I use the following code to draw my rectangles:

 if (currentMap != null)
    {
        mapCanvas.Children.Clear();
        mapCanvas.Width = (currentMap.Breedte * blockScale);
        mapCanvas.Height = (currentMap.Hoogte * blockScale);
        for (int i = 0; i < currentMap.Hoogte; i++)
        {
            for (int j = 0; j < currentMap.Breedte; j++)
            {
                Rectangle blok = new Rectangle();

                blok.Stroke = new SolidColorBrush(Colors.Black);
                blok.StrokeThickness = 0.3;
                blok.Width = blockScale;
                blok.Height = blockScale;
                switch (currentMap.GetElement(j, i))
                {
                    case 0:
                        blok.Fill = new SolidColorBrush(Colors.LightGray);
                        break;
                    case 1:
                        blok.Fill = new SolidColorBrush(Colors.Red);
                        break;
                    case 2:
                        blok.Fill = new SolidColorBrush(Colors.Green);
                        break;
                    case 3:
                        blok.Fill = new SolidColorBrush(Colors.Orange);
                        break;
                    case 4:
                        blok.Fill = new SolidColorBrush(Colors.Yellow);
                        break;
                    default:
                        blok.Fill = new SolidColorBrush(Colors.Black);
                        break;

                }
                blok.SetValue(Canvas.LeftProperty, (double)(blockScale * j));
                blok.SetValue(Canvas.TopProperty, (double)(blockScale * i));

                mapCanvas.Children.Add(blok);
            }
        }
    }

I did came across a solution to draw faster on a canvas on this link: https://stackoverflow.com/questions/23976163/speed-up-adding-objects-to-canvas-in-wpf

But the problem I cannot seem to get it working in my current project or a test project.

I hope you guys can help me to speed the drawing up or at least that it can recognize every block it needs to draw.

For further question please ask I will try to give as much detail as needed

I can't tell from the code sample where the loop is being called from. It seems like it's in the update method, in which case you're updating the entire map of rectangles with every change? This would be very slow. In general when dealing with GDI, GDI+, and WPF, you want to avoid doing any unnecessary work, since it is just slow.

Only update the rectangles that have actually changed between update calls.

You can do this by having a separate map initialisation function that sets up all the rectangles once, and an update function which takes a list of cell references for cells that have changed, and updates just the colour of those cells.


Better way to load Mysql query in PHP Array Here is my code:

$sql = <<<END
Select Sum(p)/Sum(w)*100 as present FROM attendance group by month
END;
$query = mysql_query($sql) or die($sql . ' - ' . mysql_error());
$names = array();
while ($row = mysql_fetch_array($query)) {
   $names[] = $row[0];
}

Regardless of how well you know the mysql_* functions, you shouldn't use them. There are counterparts within PDO. See this answer on stackoverflow for why.

This is how you'd do that in PDO:

$db = new PDO("mysql:localhost;dbname=database", $user, $password);

// set the default for ->fetch() and ->fetchAll() to associative array
$db->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_ASSOC);
// throw an exception instead of an error on fail
$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

try {
    $sql = "(select sum(p) / sum(w) * 100) as present from attendance group by month";

    // make sure you have php5-mysqlnd installed.
    // (MySQL native driver. It makes upgrading PHP easy as there's no library conflicts).
    // You can also work with different versions of MySQL without errors.
    $names = $db->query($sql)->fetchAll();

    // array_column will take an associative array and a key and flatten it into an array
    return array_column($names, "present");
} catch (PDOException $e) {
    echo "<pre>" . $e->getTraceAsString() . "</pre>";
}

For error-handling, you should really use exceptions. Otherwise, check the value of $db->errorInfo()


C# - Standard 52 card deck I made a 52 standard deck generator for practicing my C#. Just to let you know I am using the unity editor.
I would your feedback as to whether my code is good/bad or whatever you may desire.
using System;
using System.Collections.Generic;
using UnityEngine;

public class Standard52Deck {
    List<Card> deck = new List<Card>();

    enum Ranks {
        Ace,
        Two,
        Three,
        Four,
        Five,
        Six,
        Seven,
        Eight,
        Nine,
        Ten,
        Jack,
        Queen,
        King
    }

    enum Suits {
        Hearts,
        Spades,
        Clubs,
        Diamonds,
    }

    class Card {
        readonly Ranks rank;
        readonly Suits suit;

        public Card(Ranks rank, Suits suit) {
            this.rank = rank;
            this.suit = suit;
        }

        public override string ToString() {
            return $"{rank} of {suit}";
        }
    }

    IEnumerable<Card> GetSortedCardsBySuits() {
        var rankCount = Enum.GetValues(typeof(Ranks));
        var suitCount = Enum.GetValues(typeof(Suits));
        var deckCount = rankCount.Length * suitCount.Length;

        var index = 0;
        var rankIteration = 0;

        while (index < deckCount) {
            var rank = (Ranks)(index % 13);
            if (rank == 0 && index != 0) {
                rankIteration++;
            }
            var suit = (Suits)(rankIteration);

            index++;
            yield return new Card(rank, suit);
        }
    }

    void CreateSortedCards() {
        foreach (var card in GetSortedCardsBySuits()) {
            deck.Add(card);
        }
    }

    void Start() {
        CreateSortedCards();
    }
}

class Card {
    readonly Ranks rank;
    readonly Suits suit;
    // ...
}

In this type definition, there is a grammatical mismatch: Ranks rank has a plural type name but a singular variable name.
The types Ranks and Suits should be renamed to Rank and Suit, to make the code more consistent.


Can my string to instance construction be improved? I have a Dictionary<string, object> (named tableDict) that has been parsed from JSON. The string describes the intended construction of a class T which is defined within the housing class of this code. The object is the numerical probability value that pairs up with the generated reference of T.

The string can be in either of two formats. Something encased in brackets, i.e. "(arg0, arg1, arg2, ...)", will call the constructor with these arguments. Anything else is assumed to be a name of a property of type T that returns a predefined T, i.e when T is of type Color, "Black" would lead to looking for Color.Black.

I managed to get something to work however I pieced this together from googling each problem I encountered. A lot of what I have used is new to me, it's highly likely I have gone about it in an inefficient roundabout fashion.

for (int i = 0; i < tableDict.Count; i++)
{
    KeyValuePair<string, object> kvp = tableDict.ElementAt(i);
    string s = kvp.Key;

    if (Regex.IsMatch(s, @"\(([^\)]+)\)", RegexOptions.None))
    // Key is "( something )" - entry is constructor
    {
        string sCon = s.WhitespaceRemoved();
        sCon = sCon.Substring(1, sCon.Length - 2); // Remove brackets

        string[] argsAsStrings = sCon.Split(',');
        object[] args = argsAsStrings.Select(num => (object)int.Parse(num)).ToArray();
        // I have only implemented this for the Color class.
        // I assume the constructor is comprised of ints.

        T item = (T)Activator.CreateInstance(typeof(T), args);
        double prob = Convert.ToDouble(kvp.Value);

        var entry = new RawChanceTableEntry<T>(item, prob);

        list.Add(entry);
    }
    else // Parse string as a property of T instead
    {
        string sProp = s;
        Type type = typeof(T);
        PropertyInfo propInfo = type.GetProperty(sProp);

        object obj = Activator.CreateInstance(type);

        T item = (T)propInfo.GetValue(obj, null);
        double prob = Convert.ToDouble(kvp.Value);

        var entry = new RawChanceTableEntry<T>(item, prob);

        list.Add(entry);
    }
}

Note the struct RawChanceTableEntry is just a container for the item / probability pair.

Code in class: http://pastebin.com/bvJaS8Se (class is incomplete)

Parsed JSON: http://pastebin.com/HS6eSABQ

I'd compact the main loop as such:

        for (var i = 0; i < tableDict.Count; i++)
        {
            var kvp = tableDict.ElementAt(i);
            var s = kvp.Key;

            // Key is "( something )" - entry is constructor
            if (brackets.IsMatch(s))
            {
                var constructor = s.WhitespaceRemoved();

                constructor = constructor.Substring(1, constructor.Length - 2); // Remove brackets

                var argsAsStrings = constructor.Split(',');
                var args = argsAsStrings.Select(num => (object)int.Parse(num)).ToArray();

                // I have only implemented this for the Color class.
                // I assume the constructor is comprised of ints.
                list.Add(new RawChanceTableEntry<T>(
                    (T)Activator.CreateInstance(typeof(T), args),
                    Convert.ToDouble(kvp.Value)));
            }
            else
            {
                // Parse string as a property of T instead
                var type = typeof(T);

                list.Add(new RawChanceTableEntry<T>(
                    (T)type.GetProperty(s).GetValue(Activator.CreateInstance(type), null),
                    Convert.ToDouble(kvp.Value)));
            }
        }

I also create a specific RegEx for the bracket matching that should be faster as it's compiled at the class level:

    private static readonly Regex brackets = new Regex(@"\(([^\)]+)\)", RegexOptions.Compiled);


Shell script to calculate the total space consumed by a user by file-size I'm working with a small bash code which is working fine but i'm just looking if there is better way to formulate this, In this code i'm looking for the Files between year 2002 and 2018 on the 7th column.

Below is the working code,

Script:

#!/bin/bash
# scriptName: Ftpcal.sh
FILE="/home/pygo/Cyberark/ftplogs_3"
AWK="/bin/awk"
GREP="/bin/grep"
USERS="`"$AWK" '$7 >= "2002" && $7 <= "2018"' $FILE | "$AWK" '{print $3}' | sort -u`"
for user in $USERS;
do
echo "User $user " | tr -d "\n";
"$AWK" '$7 >= "2002" && $7 <= "2018"' "$FILE" | "$GREP" "$user" | "$AWK" '{ total += $4}; END { print "Total Space consumed: "  total/1024/1024/1024 "GB"}';
done | column -t
echo ""
echo "=============================================================="
"$AWK" '$7 >= "2002" && $7 <= "2018"' "$FILE" | "$AWK" '{ total += $4}; END { print "Total Space consumed by All Users: "  total/1024/1024/1024 "GB"}';
echo ""

Actual data Result:

$ sh Ftpcal.sh

User  16871                   Total  Space  consumed:  0.0905161GB
User  253758                  Total  Space  consumed:  0.0750855GB
User  34130                   Total  Space  consumed:  3.52537GB
User  36640                   Total  Space  consumed:  0.55393GB
User  8490                    Total  Space  consumed:  3.70858GB
User  tx-am                   Total  Space  consumed:  0.18992GB
User  tx-ffv                  Total  Space  consumed:  0.183137GB
User  tx-ttv                  Total  Space  consumed:  17.2371GB
User  tx-st                   Total  Space  consumed:  0.201205GB
User  tx-ti                   Total  Space  consumed:  58.9704GB
User  tx-tts                 Total  Space  consumed:  0.0762068GB

------------ snipped output --------------
==============================================================
Total Space consumed by All Users: 255.368GB

Sample data:

-rw-r--r-- 1 34130 14063436 Aug 15  2002 /current/focus-del/files/from_fix.v.gz
-rw-r--r-- 1 34130 14060876 Jul 12  2007 /current/focus-del/files/from1_fix.v.gz
-rw-r--r-- 1 34130 58668461 Feb 23  2006 /current/focus-del/files/from_1.tar.gz
-rw-r--r-- 1 34130 14069343 Aug  7  20017 /current/focus-del/files/from_tm_fix.v.gz
-rw-r--r-- 1 34130 38179000 Dec  7  20016 /current/focus-del/files/from_tm.gds.gz
-rw-r--r-- 1 34130 15157902 Nov 22  20015 /current/focus-del/files/from_for.tar.gz
-rw-r--r-- 1 34130 97986560 Nov  4  20015 /current/focus-del/files/from_layout.tar

Sample Result:

$ sh Ftp_cal.sh
User  34130  Total  Space  consumed:  0.0808321GB

==============================================================
Total Space consumed by All Users: 0.0808321GB

I'm okay with any better approach as a review process to make it more robust.

Thanks.

AWK="/bin/awk"

It's easier and more readable if you just set your PATH to something appropriate.

USERS="`"$AWK" '$7 >= "2002" && $7 <= "2018"' $FILE | "$AWK" '{print $3}' | sort -u`"

Backticks should almost always be replaced by $( … ), which is faster because it does not invoke a subshell.

Literal numbers should not be quoted.  It happens to still do what you want in awk; in some languages it won't.  A bad habit, easily avoided.

There's no need to invoke awk a second time to extract the third field.  Simply pair the action {print $3} with the condition ($7 >= …) that's already there.

It's good form to indent the body of a for block (or any other block).

echo "User $user " | tr -d "\n";

To suppress a newline on echo, use echo -n.

column -t

This has some awkward consequences, like tabs inside of labels ("TotalTABSpace") and unaligned numbers.  printf will give much prettier results.  Both bash and awk provide it.

total/1024/1024/1024

Nothing wrong with this, as such, but 2**30 is useful shorthand for gigabyte.

==============================================================

Bash can generate sequences like this with the idiom printf "=%.0s" {1..62}.  The = is the character and 62 is the count.

You're traversing the file three times and extracting the same information each time.  This is going to get slow as the file grows.  Awk has associative arrays: you can store a subtotal for each user, then iterate and print those subtotals at the end of the awk script, accomplishing the whole thing in one go.

Putting it all together:

/bin/awk -vusrfmt="User %-20s Total Space consumed: %11.6f GB\n" \
         -vsumfmt=$( printf "=%.0s" {1..62} )"\nTotal Space consumed by All Users: %.6f GB\n"  '
  $7 >= 2002 && $7 <= 2018 {
    subtot[$3]+=$4
    tot+=$4
  }
  END {
    for (u in subtot) printf usrfmt, u, subtot[u] / 2**30
    printf sumfmt, tot / 2**30
  }'


Accept either a existing stream or a filename (to be opened) in a constructor Context:

In a larger project, I am trying to build an utility class to encapsulate the parsing of input data. And I want it to be able to process either an already existing input stream, or a file given by its name.

The general idea is to build a stream converter. The input file contains a variable number of (possibly multi-line records). The program parses the file, retrieves one item, convert it and write it to the output file. So the structure of the program (pseudo code here) is:

Parser parser(input_stream);
Writer writer(output_stream);

while (StopRecord != (record = parser.getRecord())) {
    writer.write(record);
}

My first idea was to have a ref to a std::istream as a class member, and initialize it in constructor, either from an existing ref or to a newly opened file

class Parser {
    std::istream &in;
public:
    Parser(std::istream& in): in(in) {}   // fine
    Parser(std::string file): in(std::ifstream(file)) {}   // compile error

    Record getRecord();
};

The problem is that std::ifstream(file) is a temporary, and can only initialize a const lvalue reference or a rvalue reference. A const istream would be pretty useless, so I tried a rvalue ref.

class Parser {
    std::istream &&in;
public:
    Parser(std::istream& in): in(in) {}   // compile error
    Parser(std::string file): in(std::ifstream(file)) {}   // fine

    Record getRecord();
};

But now a std::istream& cannot be used to initialize a std::istream&&, and I would not dare to use Parser(std::istream&& in): in(in) {} and later p = Parser(std::cin); with a move from a standard stream!

Current code:

I finally decided to dynamically allocate a std::ifstream in my class when I have to process a file to have an lvalue to initialize my ref:

class Parser {
    std::ifstream *fin;
    std::istream& in;   // MUST be after fin declaration!
public:
    Parser(std::istream& in): fin(nullptr), in(in) {}
    Parser(std::string file): fin(new std::ifstream(file)), in(*fin) {}
    ~Parser() {
        if (fin) {
            fin.close();
            delete fin;
        }
    }

    Record getRecord();
};

It seems to work (even if in case of copies all will share the same underlying stream), but requires fin to be declared before in, and the raw pointer initialization looks rather ugly.

Question:

As this design come from my former C experience, I wonder whether this follows modern C++ best practices and how I could make it better, more robust and easier to maintain

Very close

You're actually one foot off the finish.

Reduce

You could store a pointer to the istream (note that std::ifstream is its child class, so new std::ifstream(file) will automatically be converted to istream*), then store a bool flag if it is owning or not.

Bigger issue

There is yet greater issue here, and it is ownership semantics. When ownership semantics are weird design starts becoming fragile. I'd recommend letting the caller to take ownership of the stream, so that Parser wouldn't mess with it and keep rule of zero. Callers will have much more power that way. Otherwise Parser would also require implementing either of rule of 0/3/5.

Code review

fin.close();

is redundant in the destructor. Invoking it explicitly opens possibilities for exceptions, thus making your fin leak. Just delete it, destructor will take care of closing.

Code never checks if the file is opened. Code could start on non-opened stream, which is, I believe undefined behavior.


Determine playing card I would like a code review on the coding I did for my assignment. The reason I have asked here is because my lecturer has no intention of going through (Don't ask me why, I asked before and he said no..) and I have no idea if I am doing it right or not.. All my other classmates seems to have different answers too, some have deck class, shuffle function etc..

Anyway the question is asking the user to design and implement a class that represent a playing card in which it can be used to play card games. Consider what information you needs to be stored in a card and what you man want to do with a card (accessors/mutators).

In this assignment, only 2 files are required - 1 header and 1 implementation file.
Kindly see the following:

Header file

#ifndef  CARD_H
#define  CARD_H
#include<string>

using namespace std;

class card
{
    private:
        string cardSuit;
        string cardValue;

    public:
        // Constructor
        string displayCard(string, string);

        // Member Functions
        string getSuit();
        string getValue();

        // Default
        card();

};
#endif

Implementation File

#include <iostream>
#include <string>
#include <ctime>
#include <cstdlib>
#include "card.h"

using namespace std;

card::card()
{
    srand(time(0));
    cardSuit = getSuit();
    cardValue = getValue();
}

string card::displayCard(string suit, string value)
{
    return cardSuit + " of " + cardValue;
}

string card::getSuit()
{

    string arrSuits[] = {"HEARTS", "DIAMONDS", "CLUBS", "SPADES"};

    // 1 out of 4 suits is choose randomly
    int randSuits = rand() % 4;
    return arrSuits[randSuits];
}

string card::getValue()
{
    string arrValues[] = {"ACE", "ONE", "TWO", "THREE", "FOUR", "FIVE", "SIX", "SEVEN", "EIGHT", "NINE", "TEN", "JACK", "QUEEN", "KING"};

    // 1 out of 13 values is choose randomly
    int randValues = rand() % 13;

    return arrValues[randValues];
}

As I have only started out in C++, do pardon me if my coding format is wrong etc.

In object-oriented programming, a card object is supposed to represent a card.  Your card, though… I'm not sure what it represents!  If anything, it's the ultimate chameleon card: every time you call .getSuit() or .getValue() on it, it reports a different value!

The card's identity, therefore, must be determined in the constructor, and it should never change thereafter.  All three methods should be declared const.

Furthermore, it's a bad idea to have the card's identity be randomly assigned in the constructor.  How would you create a deck of 52 different cards, if you couldn't instantiate each one deterministically?

The displayCard() method makes no sense — why should you pass in suit and value as parameters, which will then be ignored?

using namespace std; defeats the purpose of the std namespace.  It's sloppy practice in general.  In particular, you should never do that in a header file, because any .cpp file that includes that header will have its namespace contaminated as well.

Your comments are wrong: displayCard() is not a constructor.

So, the header file should look more like this:

#ifndef  CARD_H
#define  CARD_H
#include <string>

class Card
{
    public:
        // Constructor
        Card(const std::string &suit, const std::string &value);

        // Member Functions
        std::string getSuit() const;
        std::string getValue() const;
        std::string display() const;

    private:
        std::string suit, value;
};
#endif

I'll leave it to you to figure out what the implementation should look like.


Finding the maximum pairwise difference in a collection of colors Note that this problem is equivalent to finding the longest line segment defined by any two points in a collection of 3D coordinates, which may be an easier way to visualize the problem, and is almost certainly a version that has received more research.

Essentially, given a List<Color> and this method...

public static int ColorDiff(Color a, Color b)
{
    int rDiff = a.R - b.R;
    int gDiff = a.G - b.G;
    int bDiff = a.B - b.B;
    return (int)Math.Sqrt(rDiff * rDiff + gDiff * gDiff + bDiff * bDiff);
}

... I'd like to get the highest ColorDiff value for any pair of points in the list.

Informed Sequential Approach

int max = 0;
for (int i = 0; i < colors.Count - 1; i++)
{
    for (int j = i + 1; j < colors.Count; j++)
    {
        int temp = ColorDiff(colors[i], colors[j]);
        if (temp > max)
            max = temp;
    }
}

Naive LINQ Parallel Approach

int max = colors.AsParallel().Max(a => colors.Max(b => ColorDiff(a, b)));

Informed Parallel Approach

int max = 0;
Object locker = new Object();
System.Threading.Tasks.Parallel.For(0, colors.Count - 1, () => 0,
    (i, unused, localMax) =>
    {
        int temp = 0;
        for (int j = i + 1; j < colors.Count; j++)
            temp = Math.Max(temp, ColorDiff(colors[i], colors[j]));
        return Math.Max(localMax, temp);
    }, localMax =>
    {
        lock (locker)
        {
            if (localMax > max)
                max = localMax;
        }
    });

My testing indicates the naive LINQ approach takes about 60-70% of the time the sequential approach takes. However, I'm pretty sure it's naively doing the comparisons twice (ColorDiff(colors[i], colors[j]) and ColorDiff(colors[j], colors[i])), and that the speedup is entirely due to the parallelization across 4 cores. The performance of the informed parallel approach supports that; it takes approximately 30-40% of the time the LINQ approach takes, making it about 5 times faster than the informed sequential approach.

The LINQ approach is taking approximately 1 minute on my trial datasets (about 60 thousand elements in the list), and a little over 15 minutes on a larger dataset (230 thousand elements). I don't even want to know how long it will take on a full size dataset (about 1.1 million elements in the list).

Can I improve the brute force approach any further? Or better yet, is there an algorithm I can easily implement in C# that's better than O(n2)? A good approximation of the max value (within 5% of the true max) would probably be acceptable.

Extra information:

Because these are colors rather than points, their R, G, and B values are guaranteed to be in the range 0 to 255. I don't know if knowing the area the points will be inside helps, though; the way the colors are generated results in them often being clustered in a small sub-space of the total range.
Because of the way the colors are generated, every color will have N exact duplicates in the list (N >= 0, usually < 4). The value of N will be known at this point, but the ordering of the list must be preserved and is not guaranteed to keep duplicates near each other. A shallow copy of the list can be made and ordered if that helps (and is worth the extra CPU time & memory).

Ramos (2001) gives a \$O(n \log n)\$ algorithm, but the paper is not freely available, and apparently the algorithm is very complex. So take a look instead at Malandain and Boissonnat (2004) who give a simple algorithm that is \$O(n^2)\$ in the worst case, but apparently performs well for distributions found in practice.

E. A. Ramos (2001). "An Optimal Deterministic Algorithm for Computing the Diameter of a Three-Dimensional Point Set". Discrete and Computational Geometry, 26:2, pp. 233–244.
Grégoire Malandain and Jean-Daniel Boissonnat (2004). "Computing the Diameter of a Point Set". INRIA research report.


Decoupled chain of responsibility in Python I've started to learn Python today. I'm trying to write decoupled middleware or chain of responsibility pattern.

In my app I have some two classes:

abstract class or interface named Processable
class ProcessorChain, which must locate implementations of the Processable, create objects of these implementations and call methods of these objects

In client code, user can:

create his own class based on Processable
write full path to his class to some global setting
and be sure what it will be processed in queue by my app processor chain :-)

So, the same thing we got using service container, when we call services by tag. In Symfony, for example, we use service tags and CompilerPass to process a group of decoupled objects which implement one interface.

The question is: how to get same in Python?

I've already learnt about __subclasses__ method, but it works only if sub-classes was imported. So, I've learnt about importlib.importmodule, then I got what I wanted.

The folder structure is:

patterns

main.py
settings.py
Chain

chain.py
processor.py

patterns.Chain.chain.py

import importlib
from abc import ABCMeta, abstractmethod
from patterns import settings

class Processable(metaclass=ABCMeta):
    """Meta class define required properties and methods"""
    __metaclass__ = ABCMeta

    @abstractmethod
    def execute(self) -> str:
        return 'Base class processing...'

class ProcessorChain:
    @staticmethod
    def process_directly():

        # load processors from settings list
        for definition in settings.CHAIN_PROCESSORS:

            # parse module and package
            m, p = definition.rsplit('.', 1)
            module = importlib.import_module(m, p)

            # get class
            cls = getattr(module, p)

            # check if the class is a child of Processable
            if issubclass(cls, Processable):

                # create an instance of the class
                processor = cls()

                # and do something
                print(processor.execute())

    @staticmethod
    def process_subclass():

        # still requires make dynamic imports
        for definition in settings.CHAIN_PROCESSORS:
            # parse module and package
            m, p = definition.rsplit('.', 1)
            # import a module
            importlib.import_module(m, p)

        # load all subclasses of Processable
        for processor in Processable.__subclasses__():
            # and do something
            print(processor().execute())

patterns.Chain.processor.py

from .chain import Processable

class FirstProcessor(Processable):
    """Provides required properties and methods"""
    context = 1

    def execute(self):
        return super(FirstProcessor, self).execute() + ' (implementation from meta class)'

class SecondProcessor(Processable):
    """Provides required properties and methods"""
    context = 2

    def execute(self):
        return str(self.context) + ' processing...' + ' (its own implementation)'

patterns.settings.py

CHAIN_PROCESSORS = [
    'patterns.Chain.processor.FirstProcessor',
    'patterns.Chain.processor.SecondProcessor'
]

patterns.main.py

from patterns.Chain.chain import ProcessorChain

ProcessorChain().process_directly()

The run of main.py as expected prints:

Base class processing... (implementation from meta class)
2 processing... (its own implementation)

https://github.com/xcono/py3-trials/tree/master/patterns

But, actually, I don't know is what pythonic way? May be there is a better way to acheive decoupled chain? May be better to use event listeners/dispatchers?

Dear Pythonists, please, tell the truth, how do you cook it?

This looks unfinished to me and far from ready for review.

The docstring for the class Processable doesn't describe the class. What kind of thing is an instance of Processable?
The class Processable is misleadingly named — its instances are things that can process not things that can be processed so a name like Processor would be better.
The execute method has no docstring. What does it do?
The purpose of the execute method is surely to process a request, but in that case shouldn't it take the request as a parameter?
Why does the execute method return a string? This seems to be an unnecessary limitation: a processing chain might need to return a more complex object.
In the chain of responsibility pattern, a processor's default behaviour is to forward the request to the next processor in the chain. But this seems to be omitted from the implementation here. Where is the next processor stored? How is the request forwarded?
It is not clear what happens if none of the processors in the chain want to handle a request.
staticmethod is nearly always an anti-pattern in Python. In some languages (like Java) all functions have to belong to a class. But in Python you can just write a function, and there is no need to make a class whose only job is to act as a container for some functions.
There are no docstrings for process_directly and process_subclass. What do these functions do?
process_directly is coupled tightly to settings.CHAIN_PROCESSORS. It would be better for this to be a parameter, in case the application needs to construct its processor chain in some other way.
process_directly constructs a Processable instance and calls its execute method. But doesn't there need to be a request somewhere?
process_directly throws away the Processable instance after it is finished. This means that if you call it again it has to go through the construction logic again, which is a waste of time, and it makes it hard to implement stateful processors (because the objects don't survive from one execution to the next).
process_directly calls print on the result. It seems unlikely that this is what real processor chains will want to do with the result.
If the test issubclass(cls, Processable) fails, nothing happens. This means that typos and other mistakes in the setting will be silently ignored. It would be better to raise an exception.
I don't understand the purpose of process_subclass.
There's duplicated code (for loading a class from a module) between process_directly and process_subclass.


Custom sorting function implemented as a jQuery plugin I was asked to create a jQuery plugin that would sort homogeneous DOM elements in a given DOM container element.

I came up with the idea to attach data-priority attribute to the elements that are to be sorted, e.g.:

..

, and later use that attribute in a jQuery plugin.

The algorithm that I came up with works like this:

Iterate over all elements that are to be sorted to find the element with the highest data-priority;
Append that element to the container;
Iterate over the remaining elements of the collection and find the element with the highest data-priority;
Append the element to the container;
...continue like this recursively until the collection of elements is empty.

My plugin is initiated like this: $('.col-sm-6').sortItemsInContainer('.offer-item'); (it essentially says "sort all .offer-item items in .col-sm-6 container").

This is the code:

(function($) {
    $.fn.sortItemsInContainer = function(itemsSelector) {

        // Recursively sort DOM elements - highest priority on top
        var positionElements = function($items, $container) {
            if (0 === $items.length) {
                return;
            }
            var $highestEle = findHighestElement($items);

            $container.append($highestEle);
            $items = $items.not($highestEle);

            return positionElements($items, $container); // Recursive call

        };

        // Find DOM element with highest priority
        var findHighestElement = function($items) {

            var firstItem = $items.get(0);
            var $highest = $(firstItem);

            $.each($items, function() {
                var $current = $(this);
                if ($current.data('priority') > $highest.data('priority')) {
                    $highest = $current;
                }
            });

            return $highest;
        };

        return this.each(function() {
            var $container = $(this);
            var $itemsToSort = $container.children(itemsSelector);
            positionElements($itemsToSort, $container);
        });
    };
})(jQuery);

Now my questions are:

Is my algorithm is optimal in terms of performance, or is it just terrible?
Is there any other obvious and substantially more efficient way to accomplish this?

and lastly,

I'd like to create some automated tests for this feature. Is this code testable? What would be the best way/technology to use in order to test it?

Your code is nice and easy to understand. I'm going to break my review down into two sections: in the first section, I will suggest a different implementation, and in the second section, I will answer your questions.

New solution

In case you were not aware, JavaScript has a built-in function for sorting lists: Array.prototype.sort.

This function, basically, is used to take a list of data and sort it based on either it's built-in function, or a simple function you provide. This function takes two items from the array and returns either 1, 0, or -1 to tell sort how to place them in the array (the original array is mutated).

So, that being said, a much simpler way to sort these elements would be to do this:

// Sorts the items from greatest to least priority
$items.sort(function(a, b) {
    var aPriority = a.data("priority");
    var bPriority = b.data("priority");

    if(aPriority > bPriority) {
        // Note this is -1 despite aPriority being greater; -1 moves aPriority earlier in the list, sorting it greatest to least
        return -1;
    }
    if(aPriority < bPriority) {
        return 1;
    }
    return 0;
}

Not only is this simpler (and probably faster), it is also more idiomatic and easier to understand what is being done here.

Now that the native sort method is being used, there is no need for recursive placement. Just use a loop:

var positionElements = function($items, $container) {
    if (0 === $items.length) {
        return;
    }

    // ... $items sorting ...

    while($items.length > 0) {
        $container.append($items[0]); // sorted greatest to least
        $items = $items.not($items[0]);
    }
};

This is much simpler now, and is a bit faster now that there is no recursion because there is only two places where the call stack is touched, rather than two places for every item in the list.

Questions

  Is my algorithm is optimal in terms of performance, or is it just terrible?

There are a few places where this code is not quite optimal. Look at this flowchart of your code:

Iterate through entire list to find highest element
Place that element and remove it from the list.
Go to 1 until list is empty.

It's not very efficient to be iterating through the entire list every time as long as there are still elements inside it. If, for example, there were 10 items in the list, the first you would iterate through 10 items, then 9 items, then 8 item... until you've iterated through 55 elements.

And that's why sorting the array first before working with it will be much faster (in fact, the highest voted question on Stack Overflow is about why processing a sorted array is faster).

  I'd like to create some automated tests for this feature. Is this code
  testable? What would be the best way/technology to use in order to
  test it?

There are many unit testing frameworks out there. Honestly, I'm not that familiar with jQuery, so my suggestion may not actually be your best bet (although, the jQuery website does make a reference to a unit testing framework: QUnit).

A framework I would recommend is Cheerio. My main reason for this is because it allows to easily load HTML into a virtual DOM and perform any normal DOM operations on it. This means you could load some test components, run your sorting code on them, and use Cheerio to read the DOM and make sure they are properly sorted.


Yraglac's transit woes I decided to dive into the world of functional programming since my programming so far has been mostly in C/C++, Java, and Python. I've gone through a couple Haskell tutorials and played around in GHCi so I decided to try my first real program. I decided to try one of the easier problems on Kattis.com, Transit Woes. You can get the full problem description here.

Essentially you are given a list of times that it takes to walk between bus stops, the transit times on the buses, and the intervals at which the buses come to the stops and you need to calculate the total travel time. It was really hard for me to get into a FP mindset, and I kept thinking how easy it would be if I could just write a for loop but I finally got it. I just wanted to get some advice on good FP coding practices, and what kinds of things I should stop doing or start doing so I can get into good habits from the beginning.

import System.IO
import Control.Monad

main = do
    lines <- replicateM 4 getLine
    let inputs = (map . map) read (map words lines) :: [[Integer]]
    let a = inputs !! 0 !! 0 + inputs !! 1 !! 0
    let b = zipWith (+) (tail $ inputs !! 1) (inputs !! 2)
    let c = inputs !! 3
    let finalTime = calcTime a b c
    putStrLn $ resultStr finalTime (inputs !! 0 !! 1)

calcTime :: Integer -> [Integer] -> [Integer] -> Integer
calcTime current travel intervals
        | length travel == 0 = current
        | otherwise = calcTime (firstStop + head travel) (tail travel) (tail intervals)
        where firstStop = head [n*i | n <- [1..], n*i >= current]
              i = head intervals

resultStr :: Integer -> Integer -> String
resultStr a b
        | a <= b = "yes"
        | otherwise = "no"

You have the right idea here by replacing an imperative loop with recursion.

The main improvement I would suggest is that in firstStop you are simulating waiting for the bus i minutes at a time instead of calculating how long to wait. The formula I came up with for that is (renaming i to interval as below):

where
  remainder = current `rem` interval
  firstStop =
    case remainder of
        0 -> remainder
        x -> interval - remainder

e.g. if we're at time 26 and interval = 5 then remainder is 1 and interval - remainder is 4 which correct since we need to wait until time 30. The exception is for when the wait time is 0, in which case interval - remainder would tell us 5 instead of 0, so I wrote out a case expression for that.

There are some Haskell practices you could do better. For example, head and tail are discouraged when you can use pattern matching instead. Here's one refactoring you could do:

calcTime :: Integer -> [Integer] -> [Integer] -> Integer
calcTime current [] intervals = current
calcTime current (travelTime:travelTimes) (interval:intervals) =
    calcTime (firstStop + travelTime) travelTimes intervals
        where firstStop = ...


Calculate the Hamming difference between two DNA strands Write a program that can calculate the Hamming difference between two DNA strands.

  GAGCCTACTAACGGGAT
     CATCGTAATGACGGCCT
     ^ ^ ^  ^ ^    ^^

  The Hamming distance between these two DNA strands is 7.

import java.util.Optional;

public class Hamming {
  public static int compute(String s1, String s2) {
    validateInputs(s1, s2);

    int hammingDistance = 0;
    int stringLength = s1.length();

    for (int i = 0; i < stringLength; i++) {
      if (s1.charAt(i) != s2.charAt(i)) {
        hammingDistance++;
      }
    }
    return hammingDistance;
  }

  private static void validateInputs(String s1, String s2) {
    if (s1.length() != s2.length()) {
      throw new IllegalArgumentException();
    }
  }
}

Test suite:

import static org.hamcrest.CoreMatchers.*;
import static org.junit.Assert.*;

import org.junit.Test;

public class HammingTest {

    @Test
    public void testNoDifferenceBetweenIdenticalStrands() {
        assertThat(Hamming.compute("A", "A"), is(0));
    }

    @Test
    public void testCompleteHammingDistanceOfForSingleNucleotideStrand() {
        assertThat(Hamming.compute("A", "G"), is(1));
    }

    @Test
    public void testCompleteHammingDistanceForSmallStrand() {
        assertThat(Hamming.compute("AG", "CT"), is(2));
    }

    @Test
    public void testSmallHammingDistance() {
        assertThat(Hamming.compute("AT", "CT"), is(1));
    }

    @Test
    public void testSmallHammingDistanceInLongerStrand() {
        assertThat(Hamming.compute("GGACG", "GGTCG"), is(1));
    }

    @Test(expected = IllegalArgumentException.class)
    public void testValidatesFirstStrandNotLonger() {
        Hamming.compute("AAAG", "AAA");
    }

    @Test(expected = IllegalArgumentException.class)
    public void testValidatesOtherStrandNotLonger() {
        Hamming.compute("AAA", "AAAG");
    }

    @Test
    public void testLargeHammingDistance() {
        assertThat(Hamming.compute("GATACA", "GCATAA"), is(4));
    }

    @Test
    public void testHammingDistanceInVeryLongStrand() {
        assertThat(Hamming.compute("GGACGGATTCTG", "AGGACGGATTCT"), is(9));
    }

}

Questions:

Am I using the correct data structures?
Can I improve performance? Right now it seems to be \$Θ(n)\$ since it has to check all the characters in the string.

Your code is really all that there is to it. It is clear and concise: one method to validate the input (validateInputs) and the rest of the method to calculate the Hamming distance with a simple loop.

A couple of comments:

In case of inputs of different length, you are (rightfully) throwing a IllegalArgumentException like this:

if (s1.length() != s2.length()) {
  throw new IllegalArgumentException();
}

This is a generic IllegalArgumentException with no message. You might want to add a message so that it is clearer to the caller what went wrong; it would also help the debugging (imagine a bigger application).
To resemble what is done by the existing Objects class, you could rename your validating method to requireSameLength and make it return the length:

private static int requireSameLength(String s1, String s2) {
    if (s1.length() != s2.length()) {
        throw new IllegalArgumentException();
    }
    return s1.length();
}

This has the advantage that the method name is now self-documenting and it is returning the correct value, which means we can chain the result (just like requireNonNull which returns the non-null value or throws an exception).
import java.util.Optional; is not needed, you aren't using it.
Consider making the class final since it looks like a utility class (only public static methods).

If you're using Java 8, you could write this a bit shorter using the Stream API:

public static int compute(String s1, String s2) {
    int length = requireSameLength(s1, s2);
    return IntStream.range(0, length).map(i -> s1.charAt(i) == s2.charAt(i) ? 0 : 1).sum();
}

private static int requireSameLength(String s1, String s2) {
    if (s1.length() != s2.length()) {
        throw new IllegalArgumentException();
    }
    return s1.length();
}

It maps each index to 0 or 1 based on whether the two input Strings have equal characters at that index, and sums the result.