Convert number into words I am trying to implement a program which can convert number into words. My code can convert numbers between 0 - 999.  It uses recursive function calls and simple arithmetic operations. Can you please review it and give me your feedback?

I used three different vectors to store words such as "Six" and "Eleven" and then access there indexes according to your input.

numberToWord.h

#ifndef numberToWord_H
#define numberToWord_H

#include <vector>
#include <string>

class numbertoword
{
public:
    numbertoword();
    ~numbertoword(){};

public:
    std::vector<std::string> one2nine;
    std::vector<std::string> elevent2ninteen;
    std::vector<std::string> twoDigit;
    std::vector<std::string> threeDigit;

public:
    void initialize();
    void convert(int number);
    int countNumber(int number);
    void calculate(int number, int count);
    void display(int digit);

private:
    int xN, yN;
    int originalNumber;

};

#endif

Source.cpp

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

numbertoword::numbertoword() : xN(0), yN(0), originalNumber(0)
{
}

void numbertoword::initialize()
{
    //one to nine
    one2nine.push_back("Zero");
    one2nine.push_back("One");
    one2nine.push_back("Two");
    one2nine.push_back("Three");
    one2nine.push_back("Four");
    one2nine.push_back("Five");
    one2nine.push_back("Six");
    one2nine.push_back("Seven");
    one2nine.push_back("Eight");
    one2nine.push_back("Nine");

    //eleven to ninteen
    elevent2ninteen.push_back("Eleven");
    elevent2ninteen.push_back("Twelve");
    elevent2ninteen.push_back("Thirteen");
    elevent2ninteen.push_back("Fourteen");
    elevent2ninteen.push_back("Fifteen");
    elevent2ninteen.push_back("Sixteen");
    elevent2ninteen.push_back("Seventeen");
    elevent2ninteen.push_back("Eighteen");
    elevent2ninteen.push_back("Nineteen");

    //TwoDigit
    twoDigit.push_back("Ten");
    twoDigit.push_back("Twenty");
    twoDigit.push_back("Thirty");
    twoDigit.push_back("Forty");
    twoDigit.push_back("Fifty");
    twoDigit.push_back("Sixty");
    twoDigit.push_back("Seventy");
    twoDigit.push_back("Eighty");
    twoDigit.push_back("Ninety");

    //threeDigit
    threeDigit.push_back("Hundred");
}

void numbertoword::display(int digit)
{
    if( originalNumber < 10)
    {
        std::cout<< one2nine[digit]<<" ";
    }
    else if( originalNumber < 20)
    {
        std::cout<< elevent2ninteen[digit - 1]<<" ";
    }
    else if( originalNumber < 100)
    {
        std::cout<< twoDigit[digit - 1]<<" ";
    }
    else if( originalNumber < 1000)
    {
        std::cout<<one2nine[digit]<<" "<<threeDigit[0]<<" ";
    }
}

void numbertoword::calculate(int number, int count)
{
    if( number < 10)
    {
        display(number); return;
    }

    xN = number / count;
    yN = number - (xN * count);

    if( number < 20 && number != 10)
    {
        display (yN ); return;
    }
    else if(number < 100 && yN == 0)
    {
        display( xN ); return;
    }
    else if(number < 100 )
    {
        display( xN );
    }
    else if(number < 1000 && yN == 0)
    {
        display(xN);
        return;
    }
    else if(number < 1000 )
    {
        display(xN);
    }
    originalNumber = yN;
    count = countNumber(originalNumber);
    calculate(yN, count);

}

int numbertoword::countNumber(int number)
{
    int c = 1;
    while( number > 9)
    {
        number = number / 10;
        c *= 10;
    }

    return c;
}

void numbertoword::convert(int number)
{
    initialize();

    originalNumber = number;

    int c = countNumber(number);
    calculate(number, c);

}

int main()
{
    numbertoword n2w;
    n2w.convert(999);

    std::cin.get();
    return 0;
}

numberToWord.h

Consider renaming numbertoword to NumberToWord.  This uses a naming convention referred to as PascalCase, which is different from your variables and functions.  Also notice that the compound words have been emphasized.  This makes it easier for others to read the full name.
Since you're not using your own destructor, you can just leave it out.  The compiler will provide one for you that should be suitable.
These names make no sense to me:

int xN, yN;

Unless these shortened names are obvious in the program, they should be spelled-out entirely so that others can understand them.  It may also benefit you in case you ever forget what they mean.

Source.cpp

Elaborating on what @vnp has mentioned about initializer lists, you can use it in place of the multiple calls to push_back():

one2nine { "one", "two", "three" /* ... */ };

Moreover, consider renaming the vector to oneToNine, which is less-awkward of a name.  Apply this to the other similar names as well.

Better yet (regarding the use of vectors), you can use std::array instead.  It would be better-suited for this task as you're not needing a dynamic data structure here.
This can be simplified:

number = number / 10;

by using the /= operator:

number /= 10;

With the existing curly braces, these should be on separate lines:

display(number); return;

You should be doing input-validation with this.  If the user inputs a non-numerical value or a value below 0 or above 999, the program should display an appropriate error message and then terminate.
This program can be made more usable by accepting command line arguments:

int main(int argc, char* argv[])
{
    int number;

    // the file name is considered an argument
    if (argc > 1)
    {
        number = std::atoi(argv[1]);
    }
    // only file name given
    else if (argc == 1)
    {
        std::cin >> number;
    }

    // ...
}

(std::atoi() requires <cstdlib>)
It would make more sense to construct numbertoword objects with the original number, rather than passing it to convert().  The function has no business doing that, and this initialization should only be done by the constructor.  The function should also no longer take arguments.

numbertoword n2w(999);
n2w.convert();

In order for this to work, you should replace the default initializer list with one that takes an argument:

numbertoword::numbertoword(int originalNumber)
    : xN(0)
    , yN(0)
    , originalNumber(originalNumber)
{}

(I've rearranged the list so that it's easier to maintain the data members.)


Project Euler Problem #10 I've read other solutions about solving the 10th problem for Project Euler (finding the sum of all prime numbers under 2,000,000) but I wanted to try it on my own using the Sieve of Eratosthenes method. It runs quickly and it seems to work for all of my test increments (under 10, first 1000 primes) but it doesn't seem to work for the full 2,000,000 range. I was wondering if anyone could help point out the problem in my code. I know it's not the prettiest/most efficient code, so sorry.

#include <iostream>

using namespace std;

int main(int argc, char const *argv[])
{
    // upper limit & sum
    int sum = 0;
    int upLimit = 2000000;

    // array of isPrime check for integers
    bool isPrime[upLimit+1];

    // initialize
    for (int i=2; i <= upLimit; i++) {
        isPrime[i] = true;
    }

    for (int i=2; i<=upLimit; i++) {
        if (isPrime[i]) {
            // add to sum
            sum += i;

            // mark all multiples
            int j = i;
            while (j <= upLimit) {
                isPrime[j] = false;
                j += i;
            }
        }
    }
    cout << sum << endl;
}

You need to check for overflow before adding.

for (int i=2; i<=upLimit; i++) {
    if (isPrime[i]) {
        // Check for overflow
        if (std::numeric_limits<decltype(sum)>::max() - sum <= i) {
            std::cerr << "Overflow" << std::endl;
            return 1;
        }

        // add to sum
        sum += i;
        …

As int is only guaranteed to hold values up to 32767, you'll find that you should use a long long for sum.


POST security with PDO? So I have some code that works as PDO. But is it secure? I'm trying to prevent SQL injection. This is my first PDO script and was hoping to see if it was done correctly. Here it is.

<?php
echo 'Your query code has been submited';

$host = "localhost";
$pass = "root";
$user= "root";
$dbname = "users";
try{
$con = new PDO("mysql:host=$host;dbname=$dbname", $user, $pass);
}
catch(PDOException $e){
    echo $e->getMessage();
 }

$code = $_POST['email'];

$DATA = $con->prepare("INSERT INTO users (code) VALUES (:code)");
$DATA->bindValue(':code', $code);
$DATA->execute();
?>

It depends on what you're looking to be safe.
If you're worried about the bobby tables case, you're fine. However, there are many risks that come with user input cases.
For such a simple script, little can go wrong. However, below is only one concern that can happen (however it is probably you're most threatening):
Cross-site Scripting (XSS)

Cross-Site Scripting (XSS) attacks are a type of injection, in which
malicious scripts are injected into otherwise benign and trusted web
sites. XSS attacks occur when an attacker uses a web application to
send malicious code, generally in the form of a browser side script,
to a different end user. The end user’s browser has no way to know
that the script should not be trusted, and will execute the script.
Because it thinks the script came from a trusted source, the malicious
script can access any cookies, session tokens, or other sensitive
information retained by the browser and used with that site. These
scripts can even rewrite the content of the HTML page. (OWASP)

Basically, without validating what comes out of your data source, someone could input something nasty, and therefore giving anyone else who sees that data something nasty.
So to best protect yourself, sanitize any output from the data source and know the various attacks. Here's a good read on SO.

Also, it'd be great if you could format your code so it's more consistent and readable. Such as giving appropriate spacing around operators, correct indentations, and simply better variable names (why shorten $dbname, why CAPS $DATA?).


Decoupling with a self-descriptive mediator function I don't have a lot of experience writing libraries and just recently started thinking a lot about structure and patterns.

I wrote a tiny module manager based on the facade and mediator design patterns and very vaguely based on require.js / AMD and libraries using facade+mediator patterns like jQuery.

My goal is not only to decouple but to provide a minimal, self-descriptive mediator function as an entrance point into modules serving as a kind of tutorial.

So if everything works out inspecting a module via console.log(), it only shows the exposed mediator function and hopefully gives the required information needed to use the module without looking up in the documentation most of the time

Here is an example module featuring all structural components:

imports(["module1","module2"],function privateModuleConstructor(moduleOne,moduleTwo){
    var privateVar = "hidden and can only be accessed through the closure";

    return exports(function longDescriptiveMediatorName(){
         var a = arguments,
             l = a.length;
             c = a.callee;
         /* ...exposed part of the facade... */
         if(l===0)                     {return privateMethod()};
         if(typeof a[0] === "string")  {return privateVar+a[0]};
         if(a[0] instanceof Array)     {return privateMethod.apply(c,a[0])}

    },[module1,module2,publicMethod],"shortAlias");

    function privateMethod(){
        return privateVar.replace(/hidden/,"now revealed");
    }
    function publicMethod(){
        return "exposing methods without mediator increases coupling but is possible"
    }
})

How intuitive does this feel to you?  Even without seeing the exact source of imports and exports, does this give you the information you would need to write a module yourself?

edit:
After using the pattern for a month it turned out to be rather tedious most of the time. Some modules just can't be reasonably decoupled this way, so it's far from being the magic formula solution that solves all my problems. But it did work out very well for some modules.

I ended up just not using the mediator function when starting out to write a new module, simply use the module and then pay attention and decouple when I ended up actually using the module in many different places and saw an opportunity to reduce the complexity of my use cases.

Odd question,

arguments.callee is considered obsolete, it's use will trigger an error in strict mode
On top you use moduleOne, but you pass module1 in the array in your exports call
You did not provide the source for exports
You have both missing & unnecessary semicolons ( jshint.com )
This

var a = arguments,
    l = a.length;
    c = a.callee;

should be

var a = arguments,
    l = a.length, //<- Comma here
    c = a.callee;

Traditionally the private things go on top, with a return/export at the bottom, still I like your way better
I'm assuming your exposed part of the facade is just a show case of what you can do and you will not actually decide which function to call based on parameter count and type.

All in all, I find this introduces complexity while not providing very much in return. Personally, I would hold off on modules until I really need them. And then, use require.js.


Node.js configuration object based on environment variables In the code below, I am building a NODE_ENV-sensitive config object from environment variables.

let username
let password
let cluster
let hosts
let databaseName
let replicaSet

if (process.env.NODE_ENV === 'production') {
  username = process.env.ATLAS_HUB_USERNAME
  password = process.env.ATLAS_HUB_PASSWORD
  cluster = process.env.ATLAS_CLUSTER
  hosts = process.env.ATLAS_HOSTS
  databaseName = process.env.ATLAS_DATABASE
  replicaSet = process.env.ATLAS_REPLICA_SET
} else {
  username = process.env.MONGO_HUB_USERNAME
  password = process.env.MONGO_HUB_PASSWORD
  cluster = process.env.MONGO_CLUSTER
  hosts = process.env.MONGO_HOSTS
  databaseName = process.env.MONGO_DATABASE
  replicaSet = process.env.MONGO_REPLICA_SET
}
const config = { username, password, cluster, hosts, databaseName, replicaSet }

In this new age of fancy spread and rest operators, I hate polluting my files with code like this which repeat the same variable name multiple times and uses let instead of const for the wrong reasons, all for something simple and frequent. I could use the ternary operator to get something way better:

const config = {
  username:     process.env.NODE_ENV === 'production' ? process.env.ATLAS_HUB_USERNAME : process.env.MONGO_HUB_USERNAME,
  password:     process.env.NODE_ENV === 'production' ? process.env.ATLAS_HUB_PASSWORD : process.env.MONGO_HUB_PASSWORD,
  cluster:      process.env.NODE_ENV === 'production' ? process.env.ATLAS_CLUSTER : process.env.MONGO_CLUSTER,
  hosts:        process.env.NODE_ENV === 'production' ? process.env.ATLAS_HOSTS : process.env.MONGO_HOSTS,
  databaseName: process.env.NODE_ENV === 'production' ? process.env.ATLAS_DATABASE : process.env.MONGO_DATABASE,
  replicaSet:   process.env.NODE_ENV === 'production' ? process.env.ATLAS_REPLICA_SET : process.env.MONGO_REPLICA_SET
}

But even this seems one step short of what modern js should be able to do. I'd now like to get rid of the repeated process.env.NODE_ENV, I just can't figure out how (apart from creating a new const with a shorter name). If I had a magic wand, I'd write something along the following lines:

const config = ({
  username: [process.env.MONGO_HUB_USERNAME, process.env.ATLAS_HUB_USERNAME],
  password: [process.env.MONGO_HUB_PASSWORD, process.env.ATLAS_HUB_PASSWORD],
  cluster: [process.env.MONGO_CLUSTER, process.env.ATLAS_CLUSTER],
  hosts: [process.env.MONGO_HOSTS, process.env.ATLAS_HOSTS],
  databaseName: [process.env.MONGO_DATABASE, process.env.ATLAS_DATABASE],
  replicaSet: [process.env.MONGO_REPLICA_SET, process.env.ATLAS_REPLICA_SET]
}).*[new Number(process.env.NODE_ENV === 'production')]

But I don't, and it's not even all that great, sooo, any suggestions?

I thought of using a function, like below, but this just introduces another dependency you need to internalize for a simple batch conditional assignment operation... And if the function is in-line, there is duplication across files and it's frankly just confusing.

function fromEach (obj, key) {
  const final = {}
  Object.keys(obj).forEach((k) => {
    final[k] = obj[k][key]
  })
  return final
}
const config = fromEach({
  username: [process.env.MONGO_HUB_USERNAME, process.env.ATLAS_HUB_USERNAME],
  password: [process.env.MONGO_HUB_PASSWORD, process.env.ATLAS_HUB_PASSWORD],
  cluster: [process.env.MONGO_CLUSTER, process.env.ATLAS_CLUSTER],
  hosts: [process.env.MONGO_HOSTS, process.env.ATLAS_HOSTS],
  databaseName: [process.env.MONGO_DATABASE, process.env.ATLAS_DATABASE],
  replicaSet: [process.env.MONGO_REPLICA_SET, process.env.ATLAS_REPLICA_SET]
}, new Number(process.env.NODE_ENV === 'production'))

Note: I use new Number(process.env.NODE_ENV === 'production') in these examples, but I don't like it, feel free to propose something better!

Ideally, I would suggest two changes the the overall design:

the environment variable names could have one consistent prefix, e.g. MONGO_ and ATLAS_
the keys of the config object could match the environment variable names, e.g. databaseName would match MONGO_DATABASE_NAME

If that for some reason is not possible, then we can hardcode the config-to-environment name mapping:

function getConfigurationFor(prefix) {
  const configMap = {
    username:     'HUB_USERNAME',
    password:     'HUB_PASSWORD',
    cluster:      'CLUSTER',
    hosts:        'HOSTS',
    databaseName: 'DATABASE',
    replicaSet:   'REPLICA_SET'
  }

  prefix = prefix.toUpperCase() + '_'
  return Object
    .entries(configMap)
    .reduce((result, [key, envKey]) => {
      result[key] = process.env[prefix + envKey]
      return result
    }, {})
}

const config = getConfigurationFor(
  process.env.NODE_ENV === 'production'
    ? 'atlas'
    : 'mongo'
)

if you can make these design changes, then we have more flexibility: we can create a function that would read all the environment variables that start with a certain prefix and return this as a plain JavaScript object:

const getEnvironmentForPrefix(prefix, source) {
  const camelCase = require('lodash.camelcase')

  return Object
    .entries(source || process.env)
    .filter([key] => key.startsWith(prefix))
    .map(([key, value]) => [key.substring(prefix.length), value])
    .map(([key, value]) => [camelcase(key), value])
    .reduce((result, [key, value]) => {
      result[key] = value
      return result
    }, {})
}

const config = getEnvironmentForPrefix(
  process.env.NODE_ENV === 'production'
    ? 'ATLAS_'
    : 'MONGO_'
)


Python - Split nth occurence of a character in string I needed this function for a project I'm currently working on. I think there is definitely a way to make this prettier and have tickle more performance out of it, but how?

def nthofchar(string: str, char: str, n: int = 2) -> list:
    l = []  # New list
    temp = ""  # temp value
    count = 0  # Counter, that get will be reset, each time the temp get`s appended to the list

    for value in string.split(char):
        temp += value + char  # Add char to have it still in the string

        # Incrementing
        count += 1
        if count >= n:
            l.append(temp[:len(char) * -1])  # Remove last char
            # Reset
            count = 0
            temp = ""

    return l

print(nthofchar("25:hellwdasd:64:da:12:yeah:1:y", ":", 2))
# Output: ['25:hellwdasd', '64:da', '12:yeah', '1:y']

print(nthofchar("1;a;2;b;3;c;4;d;5;e;6;f", ";", 2))
# Output: ['1;a', '2;b', '3;c', '4;d', '5;e', '6;f']

A python idiom for iterating over a sequence in groups it to use zip() like so:

group_iter = zip(*[iter(seq)]*n)

where seq is the sequence and n is the size of each group. Note that the last group will be dropped if it doesn't have n elements.

Using that idiom, your function could be coded:

def nthofchar(string: str, char: str, n: int = 2) -> list:
    chunks = string.split(char)
    groups = zip(*[iter(chunks)]*n)

    return [char.join(group) for group in groups]

Added a test case for n != 2:

print(nthofchar("1;a;b;2;c;d;3;e;f;4;g;h;5;i;j;6;k;l", ";", 3))
# Output: ['1;a;b', '2;c;d', '3;e;f', '4;g;h', '5;i;j', '6;k;l']


Mobile content delivery system For flexing my newly acquired Django & Python muscles, I have set up a mobile content delivery system via Wap Push (Ringtones, wallpapers, etc).

The idea is that a keyword comes in from an sms via an URL, let's say  the keyword is "LOVE1" and the program should search if this keyboard points to a Ringtone or an Image. For this I have created a parent model class called "Categoria" (Category) and two subclasses "Ringtone" and "Wallpaper". This subclasses have a variable called "archivo" (filename) which points to the actual path of the content.

Dynpath is a dynamic URL which has been created  to download the content, so it is available only for X amount of time. After that a Celery scheduled task deletes this dynamic URL from the DB.

I have a piece which has "code smell" which I would like to have some input from everyone here.

Model

class Contenido(models.Model):
    nombre = models.CharField(max_length=100)
    fecha_creacion = models.DateTimeField('fecha creacion')
    keyword = models.CharField(max_length=100)

class Ringtone(Contenido):
    grupo = models.ManyToManyField(Artista)
    archivo = models.FileField(upload_to="uploads")

    def __unicode__(self):
        return self.nombre

class Wallpaper(Contenido):
    categoria = models.ForeignKey(Categoria)
    archivo = models.ImageField(upload_to="uploads")

    def __unicode__(self):
        return self.nombre

class Dynpath(models.Model):
    created = models.DateField(auto_now=True)
    url_path = models.CharField(max_length=100)
    payload = models.ForeignKey(Contenido)
    sms = models.ForeignKey(SMS)

    def __unicode__(self):
        return str(self.url_path)

View

Here is my view which checks that the Dynamic URL exists and here is where the code (which works) gets a little suspicious/ugly:

    def tempurl(request,hash):
        p = get_object_or_404(Dynpath, url_path=hash)
        try:
            fname = str(p.payload.wallpaper.archivo)
        except DoesNotExist:
            fname = str(p.payload.ringtone.archivo)

        fn = open(fname,'rb')
        response = HttpResponse(fn.read())
        fn.close()
        file_name = os.path.basename(fname)
        type, encoding = mimetypes.guess_type(file_name)
        if type is None:
            type = 'application/octet-stream'
        response['Content-Type'] = type
        response['Content-Disposition'] = ('attachment; filename=%s') % file_name
        return response

I am talking explictitly this snippet:

      try:
            fname = str(p.payload.wallpaper.archivo)
        except DoesNotExist:
            fname = str(p.payload.ringtone.archivo)

I would have loved to do something like:

fname = p.payload.archivo

But it would not let me do that, from the docs:

Django will raise a FieldError if you override any model field in any ancestor model.

I took a look at generics, but could not make it work with them. Any ideas on a better way of doing this?

You have 2 models (Ringtone extends Contenido). As I understand you store same nombre, fecha_creacion, keyword in both models and every update/delete/insert operation on the first model must be synchronized with another one. You can avoid this, make foreign key to base model:

class Contenido(models.Model):
    nombre = models.CharField(max_length=100)
    fecha_creacion = models.DateTimeField('fecha creacion')
    keyword = models.CharField(max_length=100)

class Ringtone(models.Model):
    contenido = models.ForeignKey(Contenido)
    grupo = models.ManyToManyField(Artista)
    archivo = models.FileField(upload_to="uploads")

class Wallpaper(models.Model):
    contenido = models.ForeignKey(Contenido)
    categoria = models.ForeignKey(Categoria)
    archivo = models.ImageField(upload_to="uploads")

Then in your Views

def tempurl(request,hash):
    p = get_object_or_404(Dynpath, url_path=hash)
    try:
        obj=Wallpaper.objects.get(contenido_id=p.id)
    except Wallpaper.DoesNotExist:
        try:
            obj=Ringtone.objects.get(contenido_id=p.id)
        except Ringtone.DoesNotExist:
            raise Http404
    fname = str(obj.archivo)

    # use with statement
    with open(fname,'rb') as fn:
        response = HttpResponse(fn.read())

Uhh.. Is it still complicated? If you can retrieve content type (ringtone or wallpaper) and save it in Dynpath field, solution will be easier.

P.S. Please write your code in English not Spanish )


JettyGwtSpringSample web application I just created a sample project using Spring 3.x, Jetty 9.x, GWT 2.5.1 and Gradle here. It's a slightly modified version of the 'Greeting' GWT application from Google, the only change being that RPC calls are handled by a spring controller.

It would be really great if someone can review it, I am particularly apprehensive of injecting the GwtRpcController into the GreetingServiceImpl. Should I be worried about any thread safety issues?

Main Controller

@Controller
public class GwtRpcController extends RemoteServiceServlet {

@Autowired
private ServletContext servletContext;

@Autowired
private RemoteService remoteService;

private Class<?> remoteServiceClass;

private HttpServletRequest request;

@RequestMapping(value = "/jettygwtspringsampleapp/*.rpc")
public ModelAndView handleRequest( HttpServletRequest request, HttpServletResponse response ) throws Exception {
    super.doPost( request, response );
    return null;
}

@Override
public String processCall( String payload ) throws SerializationException {
    try {

        RPCRequest rpcRequest = RPC.decodeRequest( payload, this.remoteServiceClass );

        // delegate work to the spring injected service
        return RPC.invokeAndEncodeResponse( this.remoteService, rpcRequest.getMethod(), rpcRequest.getParameters() );
    } catch ( IncompatibleRemoteServiceException ex ) {
        getServletContext().log( "An IncompatibleRemoteServiceException was thrown while processing this call.", ex );
        return RPC.encodeResponseForFailure( null, ex );
    }
}

@Override
public ServletContext getServletContext() {
    return servletContext;
}

public void setServletContext( ServletContext servletContext ) {
    this.servletContext = servletContext;
}

public void setRemoteService( RemoteService remoteService ) {
    this.remoteService = remoteService;
    this.remoteServiceClass = this.remoteService.getClass();
}

public HttpServletRequest getThreadLocalHttpRequest() {
    return getThreadLocalRequest();
}

}

Service implementation

@Configuration
public class GreetingServiceImpl implements GreetingService {

@Autowired
GwtRpcController rpcController;

public String greetServer( String input ) throws IllegalArgumentException {
    // Verify that the input is valid.
    if ( !FieldVerifier.isValidName( input ) ) {
        // If the input is not valid, throw an IllegalArgumentException back to
        // the client.
        throw new IllegalArgumentException( "Name must be at least 4 characters long" );
    }

    String serverInfo = rpcController.getServletContext().getServerInfo();
    String userAgent = rpcController.getThreadLocalHttpRequest().getHeader( "User-Agent" );

    // Escape data from the client to avoid cross-site script vulnerabilities.
    input = escapeHtml( input );
    userAgent = escapeHtml( userAgent );

    return "Hello, " + input + "!

I am running " + serverInfo + ".

It looks like you are using:
" + userAgent;
}
.....
.....

Just some generic note about the code:

If the parameter called input contains a name you should rename it to name to express its purpose.
Method names should be verbs. I would rename greetServer to serveGreeting or doGreeting. (Check Code Conventions for the Java Programming Language, Naming Conventions and Clean Code, page 25.)
Comments like this are rather just noise:

// If the input is not valid, throw an IllegalArgumentException back to
// the client.

It's pretty obvious from the code itself, so I'd remove the comment. (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)
I'd move the complete validation logic to the FieldVerifier class (or another helper class or helper method) for better reusability and higher abstraction. It also would make the greetServer method more compact.
The isValidName could be a little bit shorter with the null-safe Apache Commons Lang StringUtils.length:

public static boolean isValidName(String name) {
    return StringUtils.length(name) > 3;
}

Usage of Google Guava would make checkName more compact:

public static void checkName(final String name) {
    checkArgument(isValidName(name),
        "Name must be at least 4 characters long");
}

(See also: Effective Java, 2nd edition, Item 47: Know and use the libraries The author mentions only the JDK's built-in libraries but I think the reasoning could be true for other libraries too.)
public String greetServer(String name) throws IllegalArgumentException {
    ...
}

IllegalArgumentException is a RuntimeException, so declaring them in the method signature is not mandatory, you could omit it:

public String greetServer(String name) {
    ...
}


Function for getting URLs This is a simple Python program I wrote.  I hope some one could help me optimize this program. If you found any bad habits, please tell me.

#!/usr/bin/python

import urllib2

def GetURLs(srcURL=""):

    if srcURL == "":
        return -1

    Content = urllib2.urlopen(srcURL, None, 6)
    oneLine = Content.readline()
    while oneLine:
        print oneLine
        oneLine = Content.readline()
    Content.close()
    return 0

GetURLs("http://www.baidu.com")

You asked for comments on coding style, so here goes:

It's Python style to use CapitalizedNames for classes and lower_case_names for functions, methods and variables.
Functions should have docstrings that describe their arguments and what they do.
Names starting with "get" are normally used for functions that return the value they get. Your function prints the content at the URL, so it would be better to call it something like print_url.
The value of 6 for the timeout argument to urlopen seems arbitrary. It would be better either to leave this out (the caller can always use socket.setdefaulttimeout if they need to set a timeout), or to allow the caller to pass it as a keyword argument.
You can pass the timeout argument as a keyword argument to avoid having to specify None for the data argument.
Having a default value for srcURL is pointless, especially since it's an invalid value! Omit this.
The return value is useless: it's -1 if the argument was an empty string, or 0 if it wasn't. If the caller needs to know this, they can look at the argument they are about to supply. (Or they can catch the ValueError that urlopen raises if passed an empty string.)
The Content object does not get closed if the readline function raises an error. You can ensure that it gets closed by using Python's try: ... finally: ... statement. (Or contextlib.closing if you prefer.)

Applying all of these improvements, plus the one suggested by Lev, yields this function:

def print_url(url, timeout=6):
    """
    Print the content at the URL `url` (but time out after `timeout` seconds).
    """
    content = urllib2.urlopen(url, timeout=timeout)
    try:
        for line in content:
            print line
    finally:
        content.close()


Implementing Haskell's `union` Learn You a Haskell shows the union function:

  union also acts like a function on sets. It returns the union of two
  lists. It pretty much goes over every element in the second list and
  appends it to the first one if it isn't already in yet. Watch out
  though, duplicates are removed from the second list!

Example:

ghci> [1..7] `union` [5..10]
[1,2,3,4,5,6,7,8,9,10]

Please critique my implementation.

import Data.List (nub)

union' :: (Eq a) => [a] -> [a] -> [a]
union' xs ys = nub (xs ++ ys)

This 1-line implementation seems easy to understand, however I'm concerned at its bad/mediocre performance. Consider that I'm appending xs to ys, and then performing nub on the whole list.

The formatting of your code is broken, so I've reproduced it here for clarity.

union' :: (Eq a) => [a] -> [a] -> [a]
union' xs ys = xs ++ union'' ys xs
    where union'' [] _         = []
          union'' (a:as) first = if (a `elem` first) then union'' as first else a : union'' as (a:first)

Your code is correct, but can be rewritten to exhibit better style.

module MyUnion where

import qualified Data.List as List (union) -- For testing with QuickCheck

union :: (Eq a) => [a] -> [a] -> [a]
union xs ys = xs ++ union' ys xs
    where union' []       skips                   = []
          union' (y':ys') skips | y' `elem` skips =      union' ys'     skips
                                | otherwise       = y' : union' ys' (y':skips)
                             -- ^ Guards instead of ifs
                                               -- ^ Aligned for readability

prop_equal xs ys = union xs ys == List.union xs ys -- QuickCheck-able property

Good Haskell style also involves using higher-level functions whenever other considerations don't require otherwise (e.g., performance, comprehensibility). Here's my version of union, note that it looks much more like your version from before the edits.

union xs ys = xs ++ filter (`notElem` xs) (nub ys)

For reference here is the definition of unionBy from base (union is defined as unionBy (==)).

unionBy                 :: (a -> a -> Bool) -> [a] -> [a] -> [a]
unionBy eq xs ys        =  xs ++ foldl (flip (deleteBy eq)) (nubBy eq ys) xs


Merge Sort Algorithm Python I started implementing algorithms to learn them in order to get better at programming. Here is a merge sort one and criticisms / optimizations are welcome.

import unittest
import random

def mergeSort(list):
    """Sorts a mutable list.

    Args:
      list: An unordered list

    Returns:
      The sorted list.
    """
    if len(list) > 1:
        middle = len(list) // 2
        left_sorted = mergeSort(list[:middle])
        right_sorted = mergeSort(list[middle:])
        i,j,k = 0, 0, 0
        merged_list = []
        while (k < len(list) and len(left_sorted) != i and len(right_sorted) != j):
            if left_sorted[i] < right_sorted[j]:
                merged_list.append(left_sorted[i])
                i+=1
            else:
                merged_list.append(right_sorted[j])
                j+=1
            k+=1
        # Handle remaining items in the list.
        if len(left_sorted) == i:
            merged_list += right_sorted[j:]
        elif len(right_sorted) == j:
            merged_list += left_sorted[i:]
        return merged_list
    else:
        return list

class test_mergeSort(unittest.TestCase):
    def test_mergeSort(self):
        random_list = [random.randrange(0, 1000) for _ in range(500)]
        self.assertEqual(mergeSort(random_list), sorted(random_list))

if __name__ == '__main__':
    unittest.main()

Reduce nesting

If you invert the condition if len(list) > 1: near the beginning and use an early return,
you can reduce the level of nesting, making the code slightly easier to read:

if len(list) <= 1:
    return list

middle = len(list) // 2
left_sorted = mergeSort(list[:middle])
right_sorted = mergeSort(list[middle:])
# ...

Eliminate k

The variable k is unnecessary, as it will never reach len(list). You can delete it and all its uses.

Style

The implementation doesn't follow recommended conventions of naming and formatting. I suggest to read and follow PEP8.

The good

It's really great that you included unit tests,
it made the review easier :-)


Function for scanning /proc I wrote a function, scan_ps(), for scanning /proc, to replace a combination of popen() and ps(1).

scan_ps() scans /proc, calling filter() on each /proc/[pid]. Entries for which filter() returns nonzero are stored in strings allocated via malloc(3), and collected in array ps_list. If /proc/[pid]/cmdline is empty, the entry will not be stored. And return value is the count of entries.

But I wonder that I have to add or remove some corner-case handlings for read(), malloc(), or realloc().

int scan_ps(struct my_proc ***ptr, int (*filter)(const struct my_proc *)) {
    DIR * dirp;
    struct dirent * dp;
    int count = 0;
    pid_t pid;
    char path[MAX_LEN_STR], buf[MAX_LEN_STR];
    int fd, n;
    unsigned int size = 16;
    struct my_proc ** ps_list, ** ps_list2;
    struct my_proc * current_ps;

    dirp = opendir("/proc");
    if (NULL==dirp)
        return -1;

    ps_list = malloc(sizeof(struct my_proc *)*size);
    if (ps_list==NULL) {
        printf("%s: malloc failed(ps_list)\n",__func__);
        goto failed_closedir;
    }

    while (NULL != (dp = readdir(dirp))) {
        pid = atoi(dp->d_name);
        if (0 == pid)
            continue;
        snprintf(path, sizeof(path), "/proc/%u/cmdline", pid);
        fd = open(path, O_RDONLY);
        if (fd < 0) {
            printf("%s: failed to open() (%d)\n",__func__, errno);
            continue;
        }
        n = read(fd, buf, MAX_LEN_STR);
        if (0 == n) {
            // cmdline is empty. maybe kernel process.
            printf("%s: pid:%u cmdline is empty!\n",__func__, pid);
            close(fd);
            continue;
        }
        n = (MAX_LEN_STR - 1) > n ? n : (MAX_LEN_STR - 1);
        buf[n] = '\0';
        close(fd);

        current_ps = malloc(sizeof(struct my_proc));
        if (current_ps==NULL) {
            printf("%s: malloc failed\n",__func__);
            goto failed_ps_list;
        }
        current_ps->pid = pid;
        strncpy_v2(current_ps->cmdline, buf, MAX_LEN_STR);
        if ( !filter(current_ps) ) {
            printf("%s: let it go. currnet_ps\n", __func__);
            free(current_ps);
            continue;
        }
        // prepare ps_list
        if (count!=0 && (count%16)==0) {
            size += 16;
            ps_list2 = realloc(ps_list, sizeof(struct my_proc *)*size);
            if (ps_list2==NULL) {
                printf("%s: malloc failed(ps_list2)\n",__func__);
                goto failed;
            }
            ps_list = ps_list2;
        }
        ps_list[count++] = current_ps;
    }
    closedir(dirp);
    *ptr = ps_list;
    return count;
failed:
    free(current_ps);
failed_ps_list:
    while(count--) {
        free(ps_list[count]);
    }
    free(ps_list);
failed_closedir:
    closedir(dirp);
    return -1;
}

scan_ps() works with below code.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <dirent.h>
#include <fcntl.h>
#include <errno.h>
#define FALSE               (0)
#define TRUE                (!FALSE)
#define MAX_LEN_STR         256

struct my_proc {
    pid_t pid;
    char cmdline[256];
};

int true_filter(const struct my_proc * a) {
    return TRUE;
}

char *strncpy_v2(char *dest, char *src, unsigned int size){
    if (size==0) return dest;
    dest[--size] = '\0';
    return strncpy(dest, src, size);
}

int main(){
    int n;
    struct my_proc ** ps_list;
    n = scan_ps(&ps_list, true_filter);
    if (n<0) {
        printf("%s: n=%d something wrong!\n", __func__, n);
        return -1;
    }
    printf("%s: n=%d\n", __func__, n);
    while (n--) {
        printf("%s: %u, %s\n", __func__, ps_list[n]->pid, ps_list[n]->cmdline);
        free(ps_list[n]);
    }
    free(ps_list);
    return 0;
}

I wonder that I have to add or remove some corner-case handlings for read(), malloc(), or realloc().

int scan_ps(struct my_proc ***ptr, int (*filter)(const struct my_proc *));

Make review easier

Allocating to the size of the type rather than the size of an object obliges a reviewer to double check - did code use the right type?  Instead, make code easier to review, code right and maintain.

// ps_list = malloc(sizeof(struct my_proc *)*size);
ps_list = malloc(sizeof *ps_list * size);

// current_ps = malloc(sizeof(struct my_proc));
current_ps = malloc(sizeof *current_ps);

Why use unsigned?

With array sizing and indexing, size_t is the Goldilocks type, neither too narrow nor too wide.

// unsigned int size = 16;
size_t size = 16;

Questionable type

Why use int when read() returns ssize_t?

// int n;
ssize_t n;
n = read(fd, buf, MAX_LEN_STR);

Questionable code

Why potentially shorted n and not report this as an error?  Just read less.

// n = read(fd, buf, MAX_LEN_STR);
// n = (MAX_LEN_STR - 1) > n ? n : (MAX_LEN_STR - 1);

n = read(fd, buf, MAX_LEN_STR - 1);
// or better
n = read(fd, buf, sizeof buf - 1);

Proper use of goto

goto is often mis-used.  This is an acceptable usage.

Why MAX_LEN_STR?

For a max path length, I'd expect PATH_MAX

// char path[MAX_LEN_STR];
char path[PATH_MAX /* maybe + 1 */];

Why 256?

Avoid naked magic numbers as in char cmdline[256];.  Perhaps char cmdline[MAX_LEN_STR];?

Why MAX_LEN_STR?

In limiting string copy, best to use the size of the destination and not some other constant.

// strncpy_v2(current_ps->cmdline, buf, MAX_LEN_STR);
strncpy_v2(current_ps->cmdline, buf, sizeof current_ps->cmdline);

Missing error report

strncpy_v2() does not indicate it failed to copy a complete string. With all the other good error checking, it is curious code does not report this.  If failure cannot occur because of the construction of code, why then have strncpy_v2() versus simple strcpy()?  Being "safe" by not over-writing memory is good, yet not reporting insufficiencies is not.

Sign and potential print specifier mis-match

A pid_t is some signed type.  Ref.  Best to use signed math and cast to a wide signed type to print.

// pid = atoi(dp->d_name);
pid = atoll(dp->d_name);

// snprintf(path, sizeof(path), "/proc/%u/cmdline", pid);
snprintf(path, sizeof(path), "/proc/%jd/cmdline", (intmax_t) pid);
// or
snprintf(path, sizeof(path), "/proc/%lld/cmdline", (long long) pid);

Other stuff

What is *ptr in failure?

When scan_ps() fails, returns -1, consider setting *ptr = NULL;

Lose a *

Further, consider using re-architect and using a return value of NULL as an error indication.  Otherwise the return value is a pointer to a NULL terminated array.  You do lose one *, but I'd say that is good.

struct my_proc **scan_ps(int (*filter)(const struct my_proc *));

Why define FALSE, TRUE?

Instead, use stdbool.h.

// #define FALSE               (0)
// #define TRUE                (!FALSE)
//   return TRUE;

#include <stdbool.h>
...
   return true;


Twelve Days of X-mas in Python Printing the lyrics of "Twelve Days of Christmas" seems to be a fairly popular programming challenge.

As I've successfully attempted it, I was wondering if the code can be simplified even further (say no loops)?

Any feedback in regards to style and/or logic would be greatly appreciated.

#!/usr/bin/env python3

verses = {
        1: ["first", "and a Partridge in a Pear Tree."],
        2: ["second", "two Turtle Doves, "],
        3: ["third", "three French Hens, "],
        4: ["fourth", "four Calling Birds, "],
        5: ["fifth", "five Gold Rings, "],
        6: ["sixth", "six Geese-a-Laying, "],
        7: ["seventh", "seven Swans-a-Swimming, "],
        8: ["eighth", "eight Maids-a-Milking, "],
        9: ["ninth", "nine Ladies Dancing, "],
        10: ["tenth", "ten Lords-a-Leaping, "],
        11: ["eleventh", "eleven Pipers Piping, "],
        12: ["twelfth", "twelve Drummers Drumming, "]
        }

def get_verses(line):
    verse = f"On the {verses[line][0]} day of Christmas " + \
        "my true love gave to me: "
    for i in range(line, 0, -1):
        verse += f"{verses[i][1]}"
    return verse.replace("and ", "") if line == 1 else verse

def recite(start_verse, end_verse):
    return "\n".join([get_verses(line) for line
            in range(start_verse, end_verse + 1)])

print(recite(1, 12))

You're storing your verses in a format that's a little more complicated than necessary. If you have a dictionary that you're keying by a continuous range of number (and you don't need to handle removals), you likely shouldn't be using a dictionary. Just use a list here:

verses = [["first", "and a Partridge in a Pear Tree."],
          ["second", "two Turtle Doves, "],
          ["third", "three French Hens, "],
          ["fourth", "four Calling Birds, "],
          ["fifth", "five Gold Rings, "],
          ["sixth", "six Geese-a-Laying, "],
          ["seventh", "seven Swans-a-Swimming, "],
          ["eighth", "eight Maids-a-Milking, "],
          ["ninth", "nine Ladies Dancing, "],
          ["tenth", "ten Lords-a-Leaping, "],
          ["eleventh", "eleven Pipers Piping, "],
          ["twelfth", "twelve Drummers Drumming, "]]

If you think about, lists are already indexed by number. Now you don't need to manually increment the number keys if you add a new verse. The change just requires tweaking some numbers to account for 0-based indexing (which I think is more appropriate anyways):

def get_verses(line):
    verse = f"On the {verses[line][0]} day of Christmas, my true love gave to me: "
    for i in range(line, -1, -1):  # -1 stop
        verse += f"{verses[i][1]}"
    return verse.replace("and ", "") if line == 0 else verse  # line 0 now

def recite(start_verse, end_verse):
    return "\n".join([get_verses(line)
                      for line in range(start_verse, end_verse + 1)])

I also changed how you're splitting up the last list comprehension. If you wanted to split it, I would keep the iteration part all on one line, and get_verses on another. I also don't think the line in get_verses needed to be split. It isn't that line.

I'd still change some more things though:

In get_verses, you named the parameter line. It isn't a line though, it's a line number. I'd change it to, at the very least, line_n.
You don't need a strict list comprehension in recite. You can get rid of a pair of brackets by making it a generator expression that uses join's brackets.
The imperative loop in get_verse could be changed to a joined generator expression.

I ended up with this:

verses = [["first", "and a Partridge in a Pear Tree."],
          ["second", "two Turtle Doves, "],
          ["third", "three French Hens, "],
          ["fourth", "four Calling Birds, "],
          ["fifth", "five Gold Rings, "],
          ["sixth", "six Geese-a-Laying, "],
          ["seventh", "seven Swans-a-Swimming, "],
          ["eighth", "eight Maids-a-Milking, "],
          ["ninth", "nine Ladies Dancing, "],
          ["tenth", "ten Lords-a-Leaping, "],
          ["eleventh", "eleven Pipers Piping, "],
          ["twelfth", "twelve Drummers Drumming, "]]

def get_verses(line):
    opener = f"On the {verses[line][0]} day of Christmas, my true love gave to me: "
    verse = "".join(str(verses[i][1]) for i in range(line, -1, -1))
    return opener + (verse.replace("and ", "") if line == 0 else verse)

def recite(start_verse, end_verse):
    return "\n".join(get_verses(line_n)
                     for line_n in range(start_verse, end_verse + 1))

print(recite(0, 11))

On the first day of Christmas, my true love gave to me: a Partridge in a Pear Tree.
On the second day of Christmas, my true love gave to me: two Turtle Doves, and a Partridge in a Pear Tree.
On the third day of Christmas, my true love gave to me: three French Hens, two Turtle Doves, and a Partridge in a Pear Tree.
On the fourth day of Christmas, my true love gave to me: four Calling Birds, three French Hens, two Turtle Doves, and a Partridge in a Pear Tree.
On the fifth day of Christmas, my true love gave to me: five Gold Rings, four Calling Birds, three French Hens, two Turtle Doves, and a Partridge in a Pear Tree.
On the sixth day of Christmas, my true love gave to me: six Geese-a-Laying, five Gold Rings, four Calling Birds, three French Hens, two Turtle Doves, and a Partridge in a Pear Tree.
On the seventh day of Christmas, my true love gave to me: seven Swans-a-Swimming, six Geese-a-Laying, five Gold Rings, four Calling Birds, three French Hens, two Turtle Doves, and a Partridge in a Pear Tree.
On the eighth day of Christmas, my true love gave to me: eight Maids-a-Milking, seven Swans-a-Swimming, six Geese-a-Laying, five Gold Rings, four Calling Birds, three French Hens, two Turtle Doves, and a Partridge in a Pear Tree.
On the ninth day of Christmas, my true love gave to me: nine Ladies Dancing, eight Maids-a-Milking, seven Swans-a-Swimming, six Geese-a-Laying, five Gold Rings, four Calling Birds, three French Hens, two Turtle Doves, and a Partridge in a Pear Tree.
On the tenth day of Christmas, my true love gave to me: ten Lords-a-Leaping, nine Ladies Dancing, eight Maids-a-Milking, seven Swans-a-Swimming, six Geese-a-Laying, five Gold Rings, four Calling Birds, three French Hens, two Turtle Doves, and a Partridge in a Pear Tree.
On the eleventh day of Christmas, my true love gave to me: eleven Pipers Piping, ten Lords-a-Leaping, nine Ladies Dancing, eight Maids-a-Milking, seven Swans-a-Swimming, six Geese-a-Laying, five Gold Rings, four Calling Birds, three French Hens, two Turtle Doves, and a Partridge in a Pear Tree.
On the twelfth day of Christmas, my true love gave to me: twelve Drummers Drumming, eleven Pipers Piping, ten Lords-a-Leaping, nine Ladies Dancing, eight Maids-a-Milking, seven Swans-a-Swimming, six Geese-a-Laying, five Gold Rings, four Calling Birds, three French Hens, two Turtle Doves, and a Partridge in a Pear Tree.

Some further things though:

The verse.replace("and ", "") if line == 0 else verse seems like a code smell. It would probably be cleaner to not have "and" in the saved verse, and to add it when needed, instead of removing it in the special case.
Instead of using range(line, -1, -1) to get reversed indices, you could probably use reversed and islice from itertools to get a sliced reversed iterator of the verses. That way you wouldn't need to index the list.

I'd elaborate, but I need to get going. Good luck.


Stacking frames of a video file horizontally into a single image I stumbled upon a post on Reddit a few days ago and I thought I make my own! It's a program to extract from a movie file (.mkv, .mp4, etc.) and stack them horizontally next to each other and make one picture of all the frames available in the movie file. how can I make it more pythonic, readable, and sufficient?
Also, THIS is the GitHub link, pull requests are welcome.
Also, the default frame rate (the number of frames to extract one from, to stack up in the output image) is 5.
from natsort import natsorted
from ffprobe import FFProbe
from time import sleep
from tqdm import tqdm
from PIL import Image
import cv2
import os

# get and format file path
path = input('All modules loaded, press enter to proceed...\nEnter file path to the file(w/ filename, w/out the last backslash):\n')

if not os.path.exists(path):
    print("Path is incorrect, quitting")
    exit(0)
if path[0] == "~":
    os.path.expanduser(path)  # replace ~ with literal home address
meta = FFProbe(path)
# get metadata
video_duration = meta.metadata["Duration"][:-3]
rate = meta.video[0].framerate
height = int(meta.video[0].width)
# one shot per 5 seconds
freq = int(rate) * 5

# info for the process bar
hours = int(video_duration[1]) * 3600
minutes = int(video_duration[3:5]) * 60
seconds = int(video_duration[-2:])
duration_seconds = hours + minutes + seconds
frames = rate * duration_seconds
output_images = frames // freq  # number of extracted images

(directory, video_name) = os.path.split(path)

dst_path = directory + "/images/"
os.system("mkdir " + dst_path)
os.system("mkdir " + dst_path + "resized")
print("Destination folder created.\n")

print("Extracting frames...")
cap = cv2.VideoCapture(path)
i = 1  # current captured frame, how many 24's has already been iterated
os.system('clear')
with tqdm(total=frames) as pbar:
    while cap.isOpened():
        frame_id = cap.get(1)  # first frame in the current stream
        ret, frame = cap.read()
        if ret != True:  # if run out of frames, video finished
            break
        if frame_id % freq == 0:  # if frame# is a multiplicand of freq=24
            filename = dst_path + str(i) + ".jpg"
            i += 1
            cv2.imwrite(filename, frame)  # create the image
        pbar.update(1)
print(str(output_images) + " images extracted successfully.")
cap.release()
del pbar

src_path = directory + "/images/"
dst_path = src_path + "resized/"

images = natsorted(os.listdir(src_path))

os.system("clear")
i = 1
max_counter = len(images) - 1  # all images count
with tqdm(total=output_images) as pbar:
    for image in images:
        if i <= max_counter:  # process until all images are done
            read_path = src_path + image
            img = cv2.imread(read_path, cv2.IMREAD_UNCHANGED)
            w = 2  # convert to 2px
            h = img.shape[1]  # unchanged
            dim = (w, h)
            resized = cv2.resize(img, dim)
            filename = dst_path + str(i) + "_re.jpg"
            cv2.imwrite(filename, resized)  # create the image
            if i == 1:  # do only once
                print(str(img.shape) + " convert to " + str(dim))
            i += 1
            pbar.update(1)
        else:
            print("Finished " + str(len(images) - 1) + " pictures.")

images_list = natsorted(os.listdir(dst_path))

images_list = [Image.open(dst_path + x) for x in images_list]

# we have the height, user input #3
width = len(images_list) * 2  # number of small pictures, each 2px
out = Image.new("RGB", (width, height))  # create an empty image

x_offset = 0
for im in images_list:
    out.paste(im, (x_offset, 0))
    x_offset += im.size[0]

out.save(directory + "/" + video_name[:-4] + ".jpg")
print("Cleaning Up...")
os.system("rm -rf " + src_path)

print("All set, finishing now.")

Some suggestions aimed at more general issues rather than specific details in
your lines of code:

Computers are not our friends, no need to chat. Get user input from the command line, not from input(). Interactive user
input is a bad usage model for the default behavior of a script -- with rare
exceptions. Use interactivity only for special circumstances, and always
provide a command-line option to bypass that interactivity. In addition,
input() is terrible for development because it requires you (the coder) to go
into a dialogue with your script every damn time you run it. Super
annoying. One option for handling command-line input is
argparse. See the script
below for the template that I usually follow in my scripts.

But functions are our friends. Organize your code into functions. Have each function do one thing. Among
other things, this approach allows you to test each small piece of behavior as
you write it.

Either do or talk. Do not print() inside your functions that do the actual work (aside from some quite rare exceptions). Functions should take arguments, calculate something or do
something, and return some meaningful data (or None). If the larger program
requires printing ultimately, use the data returned from functions to do that
printing. But keep computation and printing in separate realms.

No magical tokens in the castle. Keep literal strings and values out of your code. Instead put them in one
or more containers. In many situations, I usually just use a Constants class
and module-level global (con), as shown below. Good practice, but easy and
low-tech.

The script template:
import argparse
import sys

class Constants:

    EXIT_OK = 0
    EXIT_FAIL = 1

    NEWLINE = '\n'
    NAMES = 'names'
    BAR = 'BAR'
    BAR_PROPAGANDA = 'Sucker, should have picked --bar!'

    OPTS_CONFIG = (
        {
            NAMES: 'path',
        },
        {
            NAMES: '-b --bar',
            'action': 'store_true',
        },
    )

con = Constants()

def main(args):
    ap, opts = parse_args(args)
    if opts.bar:
        print(con.BAR, opts)
        exit()
    else:
        print(opts)
        exit(con.EXIT_FAIL, con.BAR_PROPAGANDA)

def parse_args(args):
    ap = argparse.ArgumentParser()
    for oc in con.OPTS_CONFIG:
        kws = dict(oc)
        xs = kws.pop(con.NAMES).split()
        ap.add_argument(*xs, **kws)
    opts = ap.parse_args(args)
    return (ap, opts)

def exit(code = None, msg = None):
    code = con.EXIT_OK if code is None else code
    fh = sys.stderr if code else sys.stdout
    if msg:
        nl = con.NEWLINE
        msg = msg if msg.endswith(nl) else msg + nl
        fh.write(msg)
    sys.exit(code)

def your_function_foo(opts):
    pass

def your_function_bar(opts):
    pass

# Etc.

if __name__ == '__main__':
    main(sys.argv[1:])


Is it ok to use Thread.Sleep and Thread.Interrupt for pausing and resuming Thread like this? I need to observe a ConcurrentQueue, but to minimize the resources I want to pause the Thread if the Queue is empty and resume it from another Thread if there is a new Entry in the Queue. I implemented a pausing and an resuming of a thread like this:

My Worker, where the DoWork() method is called in a new Thread, looks like this:

public static class Worker
{
    public static bool Running = true;

    public static void DoWork()
    {
        while (Running)
        {
            try
            {
                Thread.Sleep(Timeout.Infinite);
            }
            catch (ThreadInterruptedException)
            {
                DoActualWork();
            }
        }
    }

    private static void DoActualWork()
    {
        //Do something
    }
}

I start the thread like this:

Thread workerThread = new Thread(Worker.DoWork);
workerThread.Start();

I interrupt the Thread like this:

workerThread.Interrupt();

I stop the Thread like this:

Worker.Running = false;

Everything is working as expected but I'm not sure if this is how it should be implemented.

Is this best practice?
What can go wrong?
Is there a problem with the static class and the members? (It have to be static because the Thread have to be interrupted by different Threads)

This is pretty bad.

Instead why not use BlockingCollection. Then the loop will be:

try
{
    while(true)
    {
        doSomethingWith(queue.Take());
    }
}
catch(InvalidOperationException e)
{
    // ignore and cleanup
}

And to stop the thread you need to call CompleteAdding() on the queue.


Project Euler Q7 - 10001st prime The problem is (source)...

By listing the first six prime numbers: 2, 3, 5, 7, 11, and 13, we can see that the 6th prime is 13.
What is the 10001st prime number?

This is my second attempt at the problem, and gets the correct answer in about a second. I wasn't too unhappy with that until I noticed a C# solution in the PE forum that ran in about 50ms.
Anyone able to suggest how I could improve my code, either in style or in speed?
let nthPrime n =
  let primeInner n =
    let numbers = [2..(int (sqrt (float n)))] // All ints from 2 to the square root of n
    (numbers |> Seq.filter (fun n1 -> n % n1 = 0) |> Seq.sum) = 0
  let rec nthPrimeInner target counter candidate last =
    if counter = target then last
    else if (primeInner candidate) then nthPrimeInner target (counter + 1) (candidate + 1) candidate
    else nthPrimeInner target counter (candidate + 1) last
  nthPrimeInner n 0 2 2

EDIT Following John Palmer's comment, I realised that I forgot to include my prime checking function. I've added it as an inner function in the code above.
Before I get on to caching (which is a good idea, but a second stage), I thought it might be worth trying a slightly different approach for the prime checking function. The code shown above iterates through all numbers from 2 to sqrt n, and then checks to see if the resultant sequence sums to zero. This means that for an even number, where the very first check (dividing by 2) is enough to say the number isn't prime will still result in 3..sqrt n being checked. So, I tried the following instead...
let primeInner n =
  let numbers = seq {for i in 2..(int (sqrt (float n))) -> i}
  numbers |> Seq.forall (fun n1 -> n % n1 = 0)

As far as I understand it, Seq.forall will give up as soon as it finds one element in the sequence for which the function doesn't return true. Thus, in the case of an even number, it would only need to check the very first input. This should have made the computation much faster. However, when I tried timing it by passing it a million numbers to check, it didn't make any noticeable difference. When I tried to use it in my function to answer the original question, it churned away for a long time before I stopped the execution.
Anyone able to explain this? I'm still bothered that the fairly simple C# I saw did the whole thing much faster, without any caching, etc. I'm sure F# could do the same.
EDIT The 2nd The C# code mentioned seems to have disappeared from the PE forum, but here is a very slightly modified version of it, with timing added. You can just copy this as-is into LinqPad...
void Main() {
  Stopwatch sw = new Stopwatch();
  sw.Start();
  int currentPrimePosition = 1;
  long currentPrimeCandidate = 1;
  while (currentPrimePosition < 10001) {
    currentPrimeCandidate++;
    if (IsPrime(currentPrimeCandidate)) {
      currentPrimePosition++;
    }
  }
  sw.Stop();
  Console.WriteLine($"The answer is: {currentPrimeCandidate}, in {sw.ElapsedMilliseconds}ms");
}

public static bool IsPrime(long number) {
  if (number < 2) return false;
  if (number % 2 == 0) return false;
  long boundary = (long)System.Math.Floor(System.Math.Sqrt(number));
  for (long i = 3; i <= boundary; i += 2) {
    if (number % i == 0) return false;
  }
  return true;
}

This runs in about 20ms in my machine, which is still about 10 times faster than the best F# I have (using Veedrac's suggestions lower down).

This computation is wrong:

let primeInner n =
 let numbers = seq {for i in 2..(int (sqrt (float n))) -> i}
 numbers |> Seq.forall (fun n1 -> n % n1 = 0)

You need to invert the condition to n % n1 <> 0. Then you find a noticeable speed improvement of roughly 10x to the overall algorithm.

You should think of better names. isPrime instead of primeInner; findNth instead of nthPrimeInner.

A much prettier way is to filter an infinite sequence with isPrime and take the nth.

let nthPrime n =
  let isPrime n =
    seq {2..(int (sqrt (float n)))}
    |> Seq.forall (fun n1 -> n % n1 <> 0)

  let rec potentialPrimesFrom n =
    seq { yield n; yield! potentialPrimesFrom (n + 1) }

  potentialPrimesFrom 2
  |> Seq.filter isPrime
  |> Seq.nth n

This adds a little overhead, but you can get some of that back by applying nicer filters upfront to potentialPrimes

  let potentialPrimes =
    let rec oddFrom n = seq { yield n; yield! oddFrom (n+2) }
    seq { yield 2; yield! oddFrom 3 }

  potentialPrimes
  |> Seq.filter isPrime
  |> Seq.nth n

Rather than pushing performance further here, I would suggest rewriting to a more optimal algorithm like a Sieve of Eratosthenes.


Finding arin whois information I have created one method that get whois information from arin server is this any changes in this code for good code structure.

public ArinResponse GetWhoisInfo(string domainName)
{
    if (string.IsNullOrEmpty(domainName))
    {
        throw new ArgumentException("You have to supply a domainName.", nameof(domainName));
    }
    using (var clientService = new HttpClientService<ArinResponse>(arinBaseUrl))
    {
        string query = string.Format("pocs;domain={0}", domainName);
        var arinResponse = clientService.GetAPI(query);
        if (arinResponse != null)
        {
            return arinResponse;
        }
    }
    return new ArinResponse();
}

it should be very helpful to me. thanks.

HttpClientService is generic common class for calling rest api.

There is not much to comment on. In general the code looks fine.

I can only suggest making the return shorter by using the ?? (coalesce) operator

return clientService.GetAPI(query) ?? new ArinResponse();

and perhaps you should also consider using dependency injection for the HttpClientService.


Password hashing/handling on a form Does my code look secure right now? I know that I still have the password hashing and so forth, but what about SQL injections? Do I have any? Are there any other security issues?

<?php
$con=mysqli_connect("localhost","root","xxx","s");
// Check connection
if (mysqli_connect_errno())
  {
  echo "Failed to connect to MySQL: " . mysqli_connect_error();
  }

$name = $con->real_escape_string($_POST['name']);
$username = $con->real_escape_string($_POST['username']);
$email = $con->real_escape_string($_POST['email']);
$password1 = $con->real_escape_string($_POST['pass1']);
$password2 = $con->real_escape_string($_POST['pass2']);

if (empty($name) || empty($username) || empty($email) || empty($password1) || empty($password2))
{
    echo "Complete all fields";
    // you can stop it here instead of putting the curly brace ALL the way at the bottom :)
    return;
    }

if (!filter_var($email, FILTER_VALIDATE_EMAIL))
{
    echo $emailvalid = "Enter a  valid email";
    }

if (strlen($password1) <= 6)
{
echo $passlength = "Password must be at least 6 characters long";
}

// Password numbers
if (!preg_match("#[0-9]+#", $password1))
{
echo $passnum = "Password must include at least one number!";
}
if (!preg_match("#[a-zA-Z]+#", $password1))
{
echo $passletter = "Password must include at least one letter!";
}
if ($password1 <> $password2)
{
echo $passmatch = "Passwords don't match";
}

if(!(isset($emailvalid) || isset($passlength) || isset($passnum) || isset($passletter) || isset($passmatch))) {
mysqli_query($con,"INSERT INTO pass (Name,Username,Email,Password) VALUES     ('$name','$username','$email','$password1')");
 }

mysqli_close($con);
?>

Not only is there code that does this type of stuff already out there, freely available. This question has been asked and answered a couple of times before on this site already. This is one such example.

However, given that the answers currently posted here are either incomplete or, IMHO, wrong (suggesting bad practices, or providing incomplete info), I'll post a short review of mine here, too.

You have a simple, procedural script. There's nothing wrong with that, per se. Although, nowadays, people will tell you to pour your code into objects and let each class do its own, specific task.
Why should you use objects here? Well, that's really down to what kind of project you're putting together: is the site going to be a fully dynamic LAMP driven site? Is the code likely to grow, or is it probable that, as you go along, new features will be added to your site? Then yes, you should look into writing your own classes, or adopt a framework (ZendFW, Symfony2,... there are tons to choose from).

If the amount of PHP you'll be using is relatively small, and its role won't exceed simple user input checks and some DB connectivity, then OO code is just going to add clutter, and bloat.

On the code
Connecting to the DB, and executing queries alongside code that actually generates output for the user/client is one of the main reasons why PHP is said to be a badly designed language. Yes, PHP allows you to write that kind of code, but you should know that it's actually seen as bad practice. A saw doesn't prevent you from sawing off the branch you're sitting on, but that doesn't mean it's a good idea to do so... Same thing applies to programming.

If you don't require object-oriented code (yet), at least create functions to handle the DB stuff, put them in a separate file, and call the functions when you need them:

require 'db_functions.php';//or require_once
$db = getDbConnection(
    array(
        'user'  => 'username',
        'pass'  => 'yourpass'
    )
);

Perhaps look into ways to keep the actual login credentials oustide of your code.

Escaping and assigning
I cannot, for the life of me understand why you would do anything like:

$name = $con->real_escape_string($_POST['name']);

with all of your post values. All those values belong together: they represent the data the user posted to your script, to validate and process. Why would you assign them to individual variables? keep that data as a whole.
If you want to check if an array has all values/keys set, then please use isset and then compare the values to an empty string. empty has a couple of oddities you should be aware of.

In this case, validating the post data can be easily done in a single loop:

$keys = array('name', 'username', 'email', 'pass1', 'pass2');
foreach ($keys as $k)
{
    if (!isset($_POST[$k]) || $_POST[$k] == '')
        exit();//<-- more on this later
}

Code like this can be easily poured into a function, to which you can pass both the array to check ($_POST) and the keys to check ($keys). Just 2 simple arguments, and you never need to write code assigning and checking variables again, just a single function call will do.

Now, the real_escape_string calls really are a bad idea. Quote chars, slashes... they're all allowed in email addresses. calling real_escape_string means that there is a chance that you're screwing up perfectly valid user input, and rendering it invalid. don't.

Using prepared statements here would be the only proper way to go: don't process the input a user gave you. If the input is anything other than perfect, assume the input is invalid or possibly malicious.

Avoid too many regex's
Checking for a password that contains both digits and characters can be done through regex, but that might not be the best tool for the job. The actual benchmarking is your job, but a regex often is slower than a simple loop, checking if a char is numeric or not.

Either way: you are using 2 regular expressions for something that can be easily done with just one expression. I've posted an answer here which goes into more details on the matter.

re-assigning again?
After validating the input (that, as I explained above, you may have rendered invalid yourself!), you then check variables like $emailValid again. That really ought to set off alarm bells.

Just think about it: you check some things. If the very first thing you checked showed you the user sent invalid input, you still procede to check, validate, reassign all other values, only to then again check if all input is valid.
That's doing the same thing twice, isn't it? DRY (Do not Repeat Yourself) is a common expression/piece of advice in programming. Your code basically repeats the most basic input validation task. That can't be good, now can it?

That's why throwing an exception is preferable, and that is also why separating output code, and logic/db related code into (at least) functions. These functions (functional units, as their name suggests) could be written to throw exceptions, which will then determine the output message the client will get to see.
Here's a basic example to clear this up. Remeber how I mentioned a function to check if all POST params are set? Here's how that would work in your case:

function checkKeys(array $data, array $keys)
{//note the type-hints!
    foreach ($keys as $k)
    {
        if (!isset($data[$k]) || $data[$k] == '')
            throw new InvalidArgumentException($k.' parameter not set');
    }
    return true;
}
//your code:
try
{
    checkKeys($_POST, array('name', 'username', 'email', 'pass1', 'pass2'));
}
catch( InvalidArgumentException $e)
{//catch Exception works, too
    echo 'Error: ', $e->getMessage();
}

That should be enough to get you started for now ;-P


Find the closest enemy in a 2D grid with wrapping allowed I'd like feedback on my solution to the outlined programming challenge. Is numpy a good candidate module for this? What might be a more efficient or Pythonic solution?

  Closest Enemy II

  Have the function ClosestEnemyII(strArr) read the matrix of numbers stored
  in strArr which will be a 2D matrix that contains only the integers 1, 0,
  or 2. Then from the position in the matrix where a 1 is, return the number
  of spaces either left, right, down, or up you must move to reach an enemy
  which is represented by a 2. You are able to wrap around one side of the
  matrix to the other as well. For example: if strArr is
  ["0000", "1000", "0002", "0002"] then this looks like the following:

  0 0 0 0
1 0 0 0
0 0 0 2
0 0 0 2

  For this input your program should return 2 because the closest enemy (2)
  is 2 spaces away from the 1 by moving left to wrap to the other side and
  then moving down once. The array will contain any number of 0's and 2's,
  but only a single 1. It may not contain any 2's at all as well, where in
  that case your program should return a 0.

  Examples
Input: ["000", "100", "200"]
Output: 1
Input: ["0000", "2010", "0000", "2002"]
Output: 2

import doctest
import numpy as np

from typing import List

def calc_closest_enemy(matrix: List[str]) -> int:
    """Calculate the minimum number of moves to reach an enemy

    Moves allowed: left, right, up, down. All count as +1
    Wrapping allowed: if current position is on an edge, a move
    can be made to the opposite edge for the cost of 1 move.

    >>> calc_closest_enemy(matrix=["000", "100", "200"])
    1
    >>> calc_closest_enemy(matrix=["0000", "2010", "0000", "2002"])
    2
    >>> calc_closest_enemy(matrix=["0000", "0010", "0000", "0000"])
    0
    >>> calc_closest_enemy(matrix=["0000", "0100", "0000", "0020"])
    3
    >>> calc_closest_enemy(matrix=["0000", "0100", "0000", "0000", "0020"])
    3
    >>> calc_closest_enemy(matrix=["0200", "0100", "0000", "0000", "0020"])
    1
    """
    grid = np.array([list(s) for s in matrix], dtype=np.int32)

    # In case of no enemies
    if 2 not in grid:
        return 0

    # format (y, x)
    friendly_coords = tuple(int(i) for i in np.where(grid == 1))
    assert len(friendly_coords) == 2, "Only one friendly is allowed"

    # Create wrapped grid
    # Wrap the y-axis then take the y-axis wrapped grid, transpose, and wrap the
    # former x-axis (now y-axis after transposition)
    for indx in friendly_coords:
        height, _ = grid.shape

        # These slices are appended to the appropriate grid ends to produce
        # the wrapped grid
        slice_to_friendly = grid[:indx]
        slice_from_friendly = grid[indx+1:]

        grid = np.insert(
            arr=grid, values=slice_to_friendly, obj=height, axis=0)
        grid = np.insert(
            arr=grid, values=slice_from_friendly, obj=0, axis=0)

        # Transpose to reuse code in for loop
        grid = grid.T

    # format (y, x)
    friendly_coords = tuple(int(i) for i in np.where(grid == 1))

    # Gather all enemy coords into a generator
    enemy_locations = np.where(grid == 2)
    enemy_locations = ((x, y) for y, x in zip(
        enemy_locations[0], enemy_locations[1]))

    # Calculate the moves required to travel from friendly to each enemy
    # and return lowest
    moves = [sum((abs(friendly_coords[1] - enemy[0]),
                    abs(friendly_coords[0] - enemy[1])))
                for enemy in enemy_locations]
    min_moves = min(moves)

    return min_moves

if __name__ == "__main__":
    doctest.testmod()

Here

enemy_locations = np.where(grid == 2)
enemy_locations = ((x, y) for y, x in zip(
    enemy_locations[0], enemy_locations[1]))

the x/y-coordinates are swapped, and later the x-coordinate of each enemy is subtracted from the y-coordinate of the friend, and vice versa:

moves = [sum((abs(friendly_coords[1] - enemy[0]),
                abs(friendly_coords[0] - enemy[1])))
            for enemy in enemy_locations]

I find that confusing. If you don't swap the coordinates then the computation of the locations becomes simpler

enemy_locations = zip(*np.where(grid == 2))

and the distance computation becomes more logical:

moves = [sum((abs(friendly_coords[0] - enemy[0]),
                abs(friendly_coords[1] - enemy[1])))
            for enemy in enemy_locations]

Making moves a generator instead of a list would be sufficient.

Here

friendly_coords = tuple(int(i) for i in np.where(grid == 1))
assert len(friendly_coords) == 2, "Only one friendly is allowed"

you check if exactly one friend is given. But that check does not work: If there is zero or more than two 1 elements in the array then the int(i) conversion already fails with a

TypeError: only size-1 arrays can be converted to Python scalars

The error message is not appropriate if there is no friend.

Also assertions are for finding programming errors. A grid with zero or more than 2 friends is a wrong parameter and a reason to raise a ValueError:

  Raised when an operation or function receives an argument that has the right type but an inappropriate value, and the situation is not described by a more precise exception such as IndexError.

My suggestion for that part would be

friendly_locations = list(zip(*np.where(grid == 1)))
if len(friendly_locations) != 1:
    raise ValueError("There must be exactly one friend")
friendly_coord = friendly_locations[0]

In order to compute the “wrapped distances” you augment the array by additional rows and columns. The dimensions of the array increase from \$ (m, n) \$ to at most \$ (2m-1, 2n-1) \$, i.e. the number of elements is almost quadrupled if the friend is located at one of the corners.

This can be completely avoided if the computation of the distance takes the wrapping into account:

def wrapped_distance(p1: Tuple[int, int], p2: Tuple[int, int], shape: Tuple[int, int]):
    """Return the wrapped distance between two points.

    The Manhattan distance from p1 to p2 in a grid of the given shape
    is computed, taking wrapping around the edges of the grid into account.
    """
    deltax = abs(p1[0] - p2[0])
    deltay = abs(p1[1] - p2[1])
    return min(deltax, shape[0] - deltax) + min(deltay, shape[1] - deltay)

And if the array is not modified then there is no need anymore to re-compute the friends's location. Also the pre-scan for the existence of enemies

if 2 not in grid:
    return 0

can now be replaced by calling min() with a default argument.

The main function then becomes:

def calc_closest_enemy(matrix: List[str]) -> int:
    """Calculate the minimum number of moves to reach an enemy.

    (... rest of your docstring omitted ...)
    """
    grid = np.array([list(s) for s in matrix], dtype=np.int32)

    friendly_locations = list(zip(*np.where(grid == 1)))
    if len(friendly_locations) != 1:
        raise ValueError("There must be exactly one friend")
    friendly_coord = friendly_locations[0]

    enemy_locations = zip(*np.where(grid == 2))
    distances = (wrapped_distance(friendly_coord, enemy, grid.shape)
                for enemy in enemy_locations)
    return min(distances, default = 0)


Thread-safe game engine: multi-threading best practices? I'm writing a multithreaded game engine, and I'm wondering about best practices around waiting for threads. It occurs to me that there could be much better options out there than what I've implemented, so I'm wondering what you think.

wait() method gets called at the top of every other method in the class. This is my current implementation, and I'm realizing it's not ideal.

class Texture {
public:
    Texture(const char *filename, bool async = true);
    ~Texture();

    void Render();

private:
    SDL_Thread *thread;
    const char *filename;

    void wait();
    static int load(void *data);
}

void Texture::wait() {
    if (thread != NULL) {
        SDL_WaitThread(thread, NULL);
        thread = NULL;
    }
}

int Texture::load(void *data) {
    Texture *self = static_cast<Texture *>(data);

    // Load the Image Data in the Thread Here...

    return 0;
}

Texture::Texture(const char *filename, bool async) {
    this->filename = filename;
    if (async) {
        thread = SDL_CreateThread(load, NULL, this);
    } else {
        thread = NULL;
        load(this);
    }
}

Texture::~Texture() {
    // Unload the Thread and Texture Here
}

void Texture::Render() {
    wait();

    // Render the Texture Here
}

Convert the wait() method in to a function pointer. This would save my program from a jmp at the top of every other method, and simply check for thread != NULL at the top of every method. Still not ideal, but I feel like the fewer jumps, the better. (I've also considered just using the inline keyword on the function... but would this include the entire contents of the wait function when all I really need is the if (thread != NULL) check to determine whether the rest of the code should be executed or not?)
Convert all of the class' methods in to function pointers, and ditch the whole concept of calling wait() except while actually loading the texture. I see advantages and disadvantages to this approach... namely, this feels the most difficult to implement and keep track of. Admittedly, my knowledge of the inner workings on GCC's optimizations and assembly and especially memory->cpu->memory communication isn't the best, so using a bunch of function pointers might actually be slower than a properly defined class.

Anyone have any even better ideas?

I'm wondering about best practices around waiting for threads.

That's too big a question. Instead I'll just review your existing code.

  This is my current implementation, and I'm realizing it's not ideal.

You construct this with a bool async parameter, so that it loads synchronously or asynchronously. There are (at least) two alternatives.

One is to make two derived subclasses:

class Texture
{
    ... etc ...
    virtual ~Texture() {}
protected:
    Texture(const char *filename)
    {
        this->filename = filename;
    }
    static int load(void *data);
private:
    virtual void wait() = 0;
    const char *filename;
}

class SyncTexture : public Texture {
public:
    SyncTexture(const char *filename)
    : Texture(filename)
    {
        load(this);
    }
private:
    virtual void wait() {} // do nothing
};

class AsyncTexture : public Texture {
public:
    AsyncTexture(const char *filename)
    : Texture(filename)
    {
        thread = SDL_CreateThread(load, NULL, this);
    }
private:
    SDL_Thread *thread;
    virtual void wait() { SDL_WaitThread(thread, NULL); }
};

The above is similar to what you might have been suggesting when you talked about a "function pointer": i.e. wait is 'virtual' and called through a so-called vtable.

The second possibility is to do it using templates:

template <bool async = true>
class Texture {
public:
    Texture(const char *filename);
};

This may be faster at run-time, because the value of async is known/fixed at compile-time.

  Convert the "wait()" method in to a function pointer.

I don't understand what you're thinking of when you say this, and you haven't provided the source code for it.

  I've also considered just using the "inline" keyword on the function... but would this include the entire contents of the wait function when all I really need is the "if (thread != NULL)" check to determine whether the rest of the code should be executed or not?

I guess that wait is small and simple enough to be inlined.

The "rest of the code" is only the following ...

{
    SDL_WaitThread(thread, NULL);
    thread = NULL;
}

... and it wouldn't be called if thread is null: so what are you worrying about: the increase in size of your software? I doubt you have enough methods (places where you call wait), such that inlining them would make significant difference to the size of your code.


Leaving an APScheduler in a while True loop I want to run a background job on my web server to do some database maintenance. I am looking at using APScheduler to do this.

I am planning on running the below code in a separate process to my main web server. I don't really want to tie the code to my web server.

Question: Is using While True pass at the end of a cron-like scheduler considered bad practice? How should it be done? (time.sleep()?)

from apscheduler.scheduler import Scheduler

@sched.interval_schedule(days=1)
def tick():
    # do a clean up job

while True:
    pass

while True: pass will consume 100 % of one CPU which is not something you want. I'm not familiar with APScheduler, but a quick look into the docs reveals a daemonic option:

  Controls whether the scheduler thread is daemonic or not.

  If set to False, then the scheduler must be shut down explicitly when
  the program is about to finish, or it will prevent the program from
  terminating.

  If set to True, the scheduler will automatically terminate with the
  application, but may cause an exception to be raised on exit.


Shrinking and expanding square I'm a computer science student who's just starting out with algorithms and their implementation. I wrote this code for a dynamically morphing square. I was wondering if I could get a review on it.  I apologize if the comments are excessive, this is how I submitted on it at school.

import time  #imported the time module to make the loop stop for a .5 sec before the next iteration

user_input = int(input("Input a number greater than 3: ")) #gets the user input in the form of an integer

def pattern(count): #created a function here called pattern, and gave it a parameter called count
    while user_input > 3: #This loop works as long as the integer entered is > 3
        min = 0 #initialize the variable min as the starting corner of the square i.e zero point.
        max = count - 1 #initialize the variable max as "count -1". This is because this variable will be updated by the loops through each iteration.
        for run in range(0,count): #first loop to control the iterations of the square
            print(run) #This will print the number of the iteration we are in at the time. This is put here as a check to see if the loop works as intended.
            for i in range(0,count): #this loop goes from row 0 to row N, where N is the value entered by the user.
                for j in range(0,count): #this loop goes from column 0 to column N, where N is the value entered by the user. This will be responsible for printing the stars in the square, from column to column.
                    if (min <= max and min <= i and i <= max and min <= j and j <= max) and (i == min or i == max or j == min or j == max): #this branch checks if ceertain cinditions are met. then prints * if they are. The conditions are explained below.
                        print("*",end="") #prints * without a line break after every iteration of its loop
                    else:
                        print(".",end="") #prints . without a line break after every iteration of its loop
                print("") #prints a new line
            print("") #prints a new line
            if run < count / 2: #in this branch, if the iterations of the boxes < half of the user input, the variable min will be incremented by 1
                min += 1        #and the variable max will be reduced by 1
                max -= 1
            if run >= count / 2: #if the iterations of the boxes > half of the user input, min will be reduced by 1 and  max will increase by 1.
                min -= 1
                max += 1
            time.sleep(.5) #pauses for .5 secs between iterations of the square.

if user_input >= 3: #if the user input > 3, the function called pattern will be called
    pattern(user_input)
else: #if the user input < 3, there will be an error message printed
    print("Enter a number greater than 3 ")

# The first branch in this function checks if the variable min<max, if min < i <= max, if min < j <= max, if i = min, if j = min, if i = max and if j = max.
# These conditions confine the points that are printed to the space between min and max, in both the rows and columns of the square.
# The four final conditions are there to allow the square to dissappear creating a set of dots and display the first square where both rows and columns are equal to the number entered by the user.

The results are as follows:

A few things stand out:

The many levels of indentation. Consider putting the contents of one of the loops into a function, and calling that instead. For example, all the printing work could be done in a separate function. I'd suggest a maximum number of four levels of indentation
The amount of comments. Comments are to explain how something works (an algorithm), not what you're doing. The latter should be obvious from the code. And definitely not a comment per line.

For example:

def pattern(count): #created a function here called pattern, and gave it a parameter called count

is an unnecessary comment: Python programmers know def means to create a function, and the parameter (I think Pythoneers tend to use the term argument instead) is obvious as well.
The lengthy lines. The suggested maximum line length is 79 characters, though preferences and local standards may differ. A situation like here provides a good example about a limited line length: you have put code in a code block on a web site, which has set a limited viewing width. And you can immediately see why long lines can be bad, in particular: hard to read. (Hint: other than removing all those comments, consider putting comment lines on the line preceding the code).
If a user enters something that can't be converted to an int, they'll get presented with a traceback. That is not very user friendly. Consider testing this appropriately (error checking is, for some code at least, > 50% of the actual code).
You have if run < count / 2:, and below that if run >= count / 2:. Since run and count don't change in between, an else instead of the second if-statement is probably clearer.


The Blacklist - follow-up Follow-up from this question using @Toby Speight's answer:

The primary concern is jq improvement/optimization, but please detail any others.
#!/bin/sh

set -eu

sources=$(mktemp)
trap 'rm "$sources"' EXIT

curl -s -o "$sources" https://raw.githubusercontent.com/T145/packages/master/net/adblock/files/adblock.sources

for key in $(jq -r 'keys[]' "$sources")
do
    case $key in
        gaming | oisd_basic )
        # Ignore these lists
        ;;
        * )
            url=$(jq -r ".$key.url" "$sources")
            rule=$(jq -r ".$key.rule" "$sources")

            curl -s "$url" |
                case $url in
                    *.tar.gz) tar -xOzf - ;;
                    *) cat ;;
                esac |
                gawk --sandbox -- "$rule"
    esac
done |
    sed -e 's/\r//g' -e 's/^/0.0.0.0 /' | sort -u > the_blacklist.txt
# use sort over gawk to merge sort multiple temp files instead of using up limited memory

Don't use a for loop to iterate over program output. See http://mywiki.wooledge.org/BashFAQ/001
Try this -- extract the keys, urls, rules all at once:
jq -r 'keys[] as $k | [$k, .[$k].url, .[$k].rule] | @tsv' "$sources" |
while IFS=$'\t' read key url rule; do
    case $key in
       ...

If, for some reason, your shell does not understand $'\t', use
while IFS="$(printf '\t')" read ...


Huffman code generator in Haskell This is problem 50 in https://wiki.haskell.org/99_questions/46_to_50. The website has more elegant Haskell solutions, but I wanted to get feedback on how the code I wrote on my own below could be improved.

import Data.List

data Tree a = Leaf a Int | Internal (Tree a) (Tree a) Int

instance (Eq a) => Eq (Tree a) where
 (Leaf a ac)         == (Leaf b bc)  = (a == b) && (ac == bc)
 (Internal a1 a2 ac) == (Internal b1 b2 bc) = (a1 == b1) && (a2 == b2) && (ac == bc)
 _   == _  = False

instance (Eq a) => Ord (Tree a) where
 a <= b = (freq a) <= (freq b)

freq :: Tree a -> Int
freq (Leaf _ c) = c
freq (Internal _ _ c) = c

leafMap :: [(a, Int)] -> [Tree a]
leafMap list = map (\(a, count) -> Leaf a count) list

extractMinFreq :: (Eq a) => [Tree a] -> (Tree a, [Tree a])
extractMinFreq list = (mf , delete mf list)
                      where mf = minimum list

collapseLowest :: (Eq a) => [Tree a] -> [Tree a]
collapseLowest list = (collapsePair s1 s2):xs
                        where
                        (s1,y) = extractMinFreq list
                        (s2, xs) = extractMinFreq y

treeBuilder :: (Eq a) => [Tree a] -> Tree a
treeBuilder list = case len of
                        0 -> error "Empty"
                        1 -> list !! 0
                        _ -> treeBuilder $ collapseLowest list
                    where len = length list

collapsePair :: Tree a -> Tree a -> Tree a
collapsePair x y = Internal x y $ (freq x) + (freq y)

prefixBuilder :: Tree Char -> String -> [(Char, String)]
prefixBuilder (Leaf a _) prefix = [(a, prefix)]
prefixBuilder (Internal a b _) prefix = (prefixBuilder a $ prefix ++ "0") ++
                                        (prefixBuilder b $ prefix ++ "1")

huffman :: [(Char, Int)] -> [(Char, String)]
huffman list = prefixBuilder (treeBuilder $ leafMap list) ""

main = print $ huffman [('a',45),('b',13),('c',12),('d',16),('e',9),('f',5)]

Specify imports

Do you know the names of all functions in Data.List? Me neither. Specify what you import like

import Data.List (delete)

In my final version, this becomes

import Data.List (minimumBy,partition)
import Data.Ord (comparing)

Ord vs Eq

Your Ord and Eq instances do not agree! For Ord, only the frequency matters, for Eq, both frequency and payload have to be equal. This leads to oddities like this:

> let i c x = Internal (Leaf c 0) (Leaf ' ' 0) x -- helper
> i 'a' 1 >= i 'b' 1
False
> i 'b' 1 >= i 'b' 1
True

To solve the issue, I'd get rid of the Ord instance by

where mf = minimumBy (comparing freq) list

Use deriving

You implement a custom Eq instance for Tree, where deriving Eq would suffice. It takes a second to figure out what deriving Eq does, to read yours takes longer and does nothing different.

use pattern matches

length and indexing with !! 0 look like a direct translation from imperative programming. Use pattern matching instead:

treeBuilder []   = error "Empty"
treeBuilder [x]  = x
treeBuilder list = treeBuilder $ collapseLowest list

The use of length causes an unnecessary traversal of the list and can cause performance issues: In the worst case you lose list fusion.

Getting rid of Eq constraint

Just as minimumBy allowed to get rid of the Ord instance, deleteBy allows to get rid of the Eq constraint.

extractMinFreq list = (mf , deleteBy ((==) `on` freq) mf list)

Now you can compress uncomparable things like functions or IO actions after Char has been replaced by a.

Eta reduction

leafMap allows eta reduction, the lambda can be written with uncurry. (The latter is of dubious value)

 leafMap = map (uncurry Leaf)

Github

You can see all steps at https://github.com/Frank-Siebert/codereview86061