Exporting data from PostgreSQL as CSV to S3 bucket I have a general question regarding DRY. As you can see there are several parts such as io.StringIO(), csv.writer, io.BytesIO which are repeated. I now wonder if there is a better way, where I don't seem to repeat myself. Or is the way how I use it here a fair approach and not considered as DRY?

What does your code do?

With the code, I create two CSV-files in the buffer and directly upload them to an AWS S3 bucket. The data for these files are from my Postgres database. The framework I use is Django. That's why I am able to access the database with commands such as Event.objects.filter

What does the rest of your program look like?

That's basically all of the program. After the CSV-files are available on my bucket an AWS Lambda function proceeds with the data and a machine learning model.

What Python version did you write this for?

I use Python 3.

class Forecast:
    @classmethod
    def export_data_for_forecast(cls):
        s3 = boto3.client(
            's3',
            aws_access_key_id=settings.ML_AWS_ACCESS_KEY_ID,
            aws_secret_access_key=settings.ML_AWS_SECRET_ACCESS_KEY,
        )

        BUCKET_NAME = 'fbprophet'
        EVENT_DATA_OBJECT = 'event_data_test.csv'
        ORDER_DATA_OBJECT = 'orders_order_test.csv'

        events = Event.objects.filter(status=EventStatus.LIVE).prefetch_related(
            'orders'
        )

        # Prepare buffer for csv files
        event_data_buffer = io.StringIO()
        event_data_writer = csv.writer(event_data_buffer)
        event_data_writer.writerow(["Event PK", "Name", "Start date"])

        order_data_buffer = io.StringIO()
        order_data_writer = csv.writer(order_data_buffer)
        order_data_writer.writerow(["Event PK", "Created", "Total Gross"])

        active_events = filter(lambda event: not event.is_over, events)
        for event in active_events:
            event_data_writer.writerow([event.pk, event.name, event.start_date])

            for order in event.orders.all():
                order_data_writer.writerow(
                    [order.event.pk, order.created, order.total_gross]
                )

        # Prepare buffer and transform to binary
        event_data_buffer_to_binary = io.BytesIO(
            event_data_buffer.getvalue().encode('utf-8')
        )
        order_data_buffer_to_binary = io.BytesIO(
            order_data_buffer.getvalue().encode('utf-8')
        )

        # Upload to S3
        s3.upload_fileobj(event_data_buffer_to_binary, BUCKET_NAME, EVENT_DATA_OBJECT)
        s3.upload_fileobj(order_data_buffer_to_binary, BUCKET_NAME, ORDER_DATA_OBJECT)

The code's straightforward enough at a glance, I wouldn't have worried
too much about the duplicate bits, especially since it's only double and
not multiple times for each part!  Overdoing abstraction can hurt readability too.  See below for some thoughts on how I'd approach this with that warning in mind.

First of I'd try and move some of the functionality out of the function.
The constants ... should be constants and not right in the middle of the
function body.  Plus, some of them look like constants, while some are
inline ('orders').  I can't tell why the distinction is made here,
maybe there was a reason.

Also consider making all of this not a class method, but a regular one,
then you could for example start creating helper objects beforehand
(like the s3 client) and/or pass in preconstructed ones, e.g. for
testing purposes (like with mock objects).

I'm not going to do these changes right here because you might have had
a reason for the @classmethod.

Instead, let's do a few different changes to make the flow a little bit
easier to comprehend for a reader not already familiar with the code
base:

Moving construction and usage of an object closely together will let
readers just continue reading while not having to jump up the block
again to find out where a particular name was defined ... unless
there are pressing reasons like failing early if no connection could
be made (c.f. the s3 client, though I'm guessing it doesn't actually
establish a connection until the upload_fileobj call is made).
Breaking out functionality into more functions (or even local helper
functions, no one's stoppiing you from creating more abstractions.
Alternatively, and I think that's better suited for this case, create
a new class to wrap things together that belong together.  The hint
for me here is event_data_... appearing with two suffixes and then
order_data_... with the same suffixes.  That begs to be an object
with two attributes.

E.g. like this:

BUCKET_NAME = 'fbprophet'
EVENT_DATA_OBJECT = 'event_data_test.csv'
ORDER_DATA_OBJECT = 'orders_order_test.csv'

class ForecastBuffer:
    def __init__(self, columns):
        # Prepare buffer for csv files
        self.buffer = io.StringIO()
        self.writer = csv.writer(self.buffer)
        self.writer.writerow(columns)

    def write_row(self, row):
        self.writer.writerow(row)

    def upload(self, s3, bucket_name, object_name):
        # Prepare buffer and transform to binary
        to_binary = io.BytesIO(self.buffer.getvalue().encode('utf-8'))
        s3.upload_fileobj(to_binary, bucket_name, object_name)

class Forecast:
    @classmethod
    def export_data_for_forecast(cls):
        event_data = ForecastBuffer(["Event PK", "Name", "Start date"])
        order_data = ForecastBuffer(["Event PK", "Created", "Total Gross"])

        events = Event.objects.filter(status=EventStatus.LIVE).prefetch_related(
            'orders'
        )

        for event in filter(lambda event: not event.is_over, events):
            event_data.write_row([event.pk, event.name, event.start_date])

            for order in event.orders.all():
                order_data.write_row(
                    [order.event.pk, order.created, order.total_gross]
                )

        # Upload to S3
        s3 = boto3.client(
            's3',
            aws_access_key_id=settings.ML_AWS_ACCESS_KEY_ID,
            aws_secret_access_key=settings.ML_AWS_SECRET_ACCESS_KEY,
        )

        event_data.upload(s3, BUCKET_NAME, EVENT_DATA_OBJECT)
        order_data.upload(s3, BUCKET_NAME, ORDER_DATA_OBJECT)

Okay, so, the methods still a bit long in the middle.

One thing that came to my mind was that order.event.pk is probably the
same as event.pk itself, right?  Maybe check and simplify that.

Then, the filter call with the lambda feels a bit weird to me, could
that perhaps already be filtered out in the Event.objects.filter...
chain above it?

You could also move one last bit to make things clearer:  Don't expose
the "raw" writerow call, instead just feed objects to the buffers:

        for event in filter(lambda event: not event.is_over, events):
            event_data.write(event)

            for order in event.orders.all():
                order_data.write(order)

However, that would require to have either a method on the object being
written that specifies the serialisation output (well, which values to
select for the rows), or the same information on the ForecastBuffer
object (you could pass in a function that formats an object for CSV).
Depends entirely on the rest of the code if that's worth it / feasible
at all.

Btw. I just saw that itertools has a complement to filter called
filterfalse (arguably that could be a better name, keep comes to my
mind):

        # from itertools import filterfalse

        for event in filterfalse(Event.is_over, events):
            event_data.write(event)

Assuming that you can get the unbound method Event.is_over that way.

Coming back to that @classmethod, the method here is difficult to test
due to the global variables.  Depending on how it's called it might be
worth to make events a parameter so that the functionality of this
method is simply: format to CSV and upload.  Same goes for settings -
the s3 client most definitely should be passed in so this function
doesn't have to deal with configuration on top of formatting and
uploading.

Actually that's still two things, but it's short enough that that's
probably okay, splitting the formatting into another method might also
be worth it actually:

class Forecast:
    @classmethod
    def format_data_for_forecast(cls, events):
        event_data = ForecastBuffer(["Event PK", "Name", "Start date"])
        order_data = ForecastBuffer(["Event PK", "Created", "Total Gross"])

        for event in filterfalse(Event.is_over, events):
            event_data.write(event)

            for order in event.orders.all():
                order_data.write(order)

        return event_data, order_data

    @classmethod
    def export_data_for_forecast(cls):
        events = Event.objects.filter(status=EventStatus.LIVE).prefetch_related(
            'orders'
        )

        event_data, order_data = cls.format_data_for_forecast(events)

        # Upload to S3
        s3 = boto3.client(
            's3',
            aws_access_key_id=settings.ML_AWS_ACCESS_KEY_ID,
            aws_secret_access_key=settings.ML_AWS_SECRET_ACCESS_KEY,
        )

        event_data.upload(s3, BUCKET_NAME, EVENT_DATA_OBJECT)
        order_data.upload(s3, BUCKET_NAME, ORDER_DATA_OBJECT)


Creating an optimized, fully functional TextureManager in SDL After I've discussed pointer semantics with Loki Astari previously, I finally managed to code the TextureManager class using auto_pointers. But the problem is that I need to pass an SDL_Renderer object to my class' constructor as an auto_ptr. But the object I want to pass is a raw pointer.

How should I do it? Also, I'm not so sure about the aout_pointers I've written. Can you help me, please?

TextureManager.h

#pragma once //using visual C++

#include <unordered_map>
#include "Texture.h"
#include <memory>

/**
    This class helps the user to manage large
    amount of texture at the same time (disposing them, setting
    their color, their renderer, ...). The dispose method of this class
    should be called before the end of the program (there will be no issues
    if it is not called : the destrctor of the TextureManager class will call
    the destructor of the @Texture class ! But it is better to call it, since
    it will release all ressources when you want !).This class contains a dynamic
    array, so the user can add @Texture pointers threw out the programs
    evolution.
*/

typedef std::unordered_map<int, Texture*> atlasType;//cannot use auto_ptr in STL container !

class TextureManager
{
public:
    TextureManager(std::auto_ptr<SDL_Renderer> pRenderer);
    ~TextureManager();

    void setRenderer(std::auto_ptr<SDL_Renderer> pRenderer);
    bool LoadFromFile(unsigned int ID, const char* fileName);

    bool unloadTexture(unsigned int ID);
    void dispose();

    Texture& getTexture(unsigned int ID);
    Texture getTextureCopy(unsigned int ID);

private:
    std::auto_ptr<SDL_Renderer> m_pRenderer;
    atlasType m_textureAtlas;
};

TextureManager.cpp

#include "TextureManager.h"

TextureManager::TextureManager(std::auto_ptr<SDL_Renderer> pRenderer)
    : m_pRenderer(pRenderer)
{}

TextureManager::~TextureManager(){
    dispose();
}

void TextureManager::dispose(){
    for (atlasType::iterator it = m_textureAtlas.begin(); it != m_textureAtlas.end(); ++it){
        delete (it->second);
    }
    m_textureAtlas.clear();
}

bool TextureManager::unloadTexture(unsigned int ID){
    Texture *pTempTexture = m_textureAtlas[ID];

    if (!pTempTexture)
        return false;

    delete pTempTexture;//dispose texture
    pTempTexture = nullptr;
    m_textureAtlas.erase(ID);

    return true;
}

Texture& TextureManager::getTexture(unsigned int ID){
    return *(m_textureAtlas[ID]);
}

Texture TextureManager::getTextureCopy(unsigned int ID){
    return *(m_textureAtlas[ID]);
}

bool TextureManager::LoadFromFile(unsigned int ID, const char* fileName){
    Texture *pTexture = m_textureAtlas[ID];

    if (pTexture == nullptr){
        pTexture = new Texture();
        pTexture->setRenderer(m_pRenderer.get());//Texture uses raw pointer;

        m_textureAtlas[ID] = pTexture;//will be deleted in dispose;
    }
    if (!pTexture->LoadFromFile(fileName))
        return false;

    return true;
}

void TextureManager::setRenderer(std::auto_ptr<SDL_Renderer> pRenderer){
    m_pRenderer = pRenderer;
}

Here's where I can not call the constructor (raw pointer <-> auto_ptr):

/* ......Declarations........... */

SDL_Renderer * m_pRenderer = nullptr;

/* etc..........*/

m_pRenderer = SDL_CreateRenderer(m_pWindow, -1, 0);

/*Trying to create TextureManager*/

m_textureManager = TextureManager(m_pRenderer);//fail to compile : is not of type auto_ptr<SDL_Renderer> ! what shall I do ?

You don't need a texture copy.

Texture& getTexture(unsigned int ID);
// Texture getTextureCopy(unsigned int ID);

This works:

Texture&  t1 = tm.getTexture(1);  // Gets a reference.
Texture   t2 = tm.getTexture(2);  // Gets a reference but because t2
                                  // is an object copies it from the
                                  // original into `t2`

auto_ptr is deprecated

std::auto_ptr<SDL_Renderer>

Prefer the modern version std::unique_ptr it understands move semantics.

std::unique_ptr<SDL_Renderer, TypeThatKnowsHowToDeleteSDLRendor>  renderer;

Also std::unique_ptr and std::auto_ptr just call delete on the pointer. I am betting that the SDL library has a special function to delete render objects. In which case you need to add a class that knows what function to call.

Also std::unqiue_ptr can be used in containers:

typedef std::unordered_map<int, Texture*> atlasType;//cannot use auto_ptr in STL container !

But you don't even need to be using pointers. Texture is not polymorphic. Just create the object in the container (not the pointer).

std::unordered_map<int, Texture>  AtlasType;

Naming conventions.

It is usall to give User defined types an initial capitol letter. It is usual to give methods and member variables an initial lower case letter.

std::unordered_map<int, Texture>  AtlasType;
             //                   ^  Its a user defined type.
AtlasType                         textureAtlas;
             //                   ^ its an addressable object.

Prefer the new range based for

for (atlasType::iterator it = m_textureAtlas.begin(); it != m_textureAtlas.end(); ++it){
    delete (it->second);
}

As mentioned last time. Prefer auto, prefer std::begin/std::end over method calls. and prefer range based for over all of them:

//
for (auto it = std::begin(m_textureAtlas); it != std::end(m_textureAtlas); ++it){
    delete (it->second);
}

//
for(auto item: m_textureAtlas) {
    delete item.second;
}

//
std::for_each(std::begin(m_textureAtlas), std::end(m_textureAtlas),
               [](AtlasType::value_type& v){delete v.second;});

Compilation Failure:

 m_textureManager = TextureManager(m_pRenderer);
                               //  ^^^^^^^^^^^ that is not an auto_ptr

 // You have to create the auto_ptr object.
 std::auto_ptr<SDL_Renderer>   rend(m_pRenderer)
 m_textureManager = TextureManager(rend);

Note. When you give up ownership of the object you should not be using it from the original pointer again. The only way to access the render object should not be through the TextureManager that owns it. If you have kept another pointer to the renderer that you are cheating ownership semantics.

Writting a shard render object

class CRendWrap
{
    public:
        CRendWrap(SDL_Window* w, int x, int y)
            : SDL_CreateRenderer(w, x, y)
        {}
        ~CRendWrap()
        {
            if (rend != nullptr) {
                SDL_DestroyRenderer(rend);
            }
        }
        // Disable Copy
        CRendWrap(CRendWrap const&)             = delete;
        CRendWrap& operator=(CRendWrap const&)  = delete;
        // Allow Move
        CRendWrap(CRendWrap&& move)
            : rend(nullptr)
        {
            using std::swap;
            swap(rend, move.rend);
        }
        CRendWrap& operator=(CRendWrap&& move)
        {
            using std::swap;
            swap(rend, move.rend);
        }
    prviate:
        SDL_Renderer*   rend;
};

typedef std::shared_ptr<CRendWrap>   CRender;

// Usage.
CRender  renderer = std::make_shared<CRendWrap>(win, 10, 20);


Heapsort is not modular For the time being, I am not interested in any recursive solution. The code is not modularized; the whole body is inside the main function.

#include<iostream.h>
#include<stdlib.h>
/*This code is written by NF between any time of 120711 and 210711;modified at 231211*/

int main()
{
    int length=0,min_heapified=0,start_length=0,index=0;
    cout<<"Enter Array Length:";
    cin>>length;
    int array[length];
    for(int i=0;i<length;i++)
    {
        array[i]=rand()%100;
    }
    cout<<"Array is filled now with random elements";
    cout<<"\nSorted Array:"<<endl;
    do
    {
            do
            {
                min_heapified=0;
                for(int root=0;root<length/2;root++)/*As the leaf nodes have no child they should not be in a consideration while swapping so looping upto length/2 is enough*/
                {

                    int left=((2*root)+1);
                    int right=((2*root)+2);

                    if(left<length)
                    {
                           if(array[left]<array[root])
                           {
                                int swap=array[left];
                                array[left]=array[root];
                                array[root]=swap;
                                min_heapified=1;
                           }
                     }
                    if(right<length)
                     {
                            if(array[right]<array[root])
                            {
                                 int swap=array[right];
                                 array[right]=array[root];
                                 array[root]=swap;
                                 min_heapified=1;
                             }
                      }

                  }

             }while(min_heapified>0);

            int swap=array[0];
            array[0]=array[--length];/*modification done at this point to avoid keeping the sorted elements into another array;and swapping done between the 0th and last element*/
            array[length]=swap;
            cout<<array[length]<<" ";

 }while(length>0);

  return 0;

}

int length=0,min_heapified=0,start_length=0,index=0;

I prefer one declaration per line because it's easier to read and find the type of variables. Furthermore, min_heapified is only used inside the do-while loop, so declare it there:

do {
    int min_heapified = 0;
    do {
...

Change

if(x<=(length-1))

to

if (x < length)

It's more common and simple.

Instead of

int swap=array[left];
array[left]=array[root];
array[root]=swap;

you should create a swap function. Maybe it already exists in a library.

int swap(array, index, rootIndex) {
    int swap = array[index];
    array[index] = array[rootIndex];
    array[root] = swap;
}

If I'm right you could extract the following to a new function:

if(array[left]<array[root])
{
    int swap=array[left];
    array[left]=array[root];
    array[root]=swap;
    min_heapified=1;
}

It looks for me that the other if do the same. The only difference is the used index, so I'd write something like this:

int swapIfLess(array, index, rootIndex) {
    if (array[index] < array[rootIndex]) {
        swap(array, index, rootIndex)
        return 1;
    }
    return 0;
}

if (left < length) {
    if (swapIfLess(array, left, root)) {
        min_heapified=1;
    }
}
if (right < length) {
    if (swapIfLess(array, right, root)) {
        min_heapified=1;
    }
}

The i variable looks unnecessary:

int root=i;

Use the root variable in the for loop:

for (int root = 0; root < length; root++)

Maybe you should rename it to rootIndex.

Extract more functions. The for loop (inside the do-while loops) looks as a good candidate for this. Additionally, you should have a readInputArray function and a printResults function too.


Linux/Windows Text Editor in Tkinter Over the past couple of days I have been working on a super-basic text editor with Tkinter.

It can:

Open new files
Open files (general)
Save files

Obviously you can edit any file you open or make. This is very similar to this question, though I did not see that until I checked for duplicates of this question.

I did not use classes.

from Tkinter import *
import sys, os
import tkMessageBox
import error_mes
main = Tk()
#Variables that are globally needed
file_input = "" #whats put into the text box
_FILE_= "" #File the user wants to open; readapt to be synonymous with save?
open_a_file = "" #will be the entry field for opening a file
target = ""
new_file_ = ""
new_file_name = ""
isnewfile = "no"
def get_from_text():
    global file_input
    try:
        file_input = my_text_box.get("1.0", END)
        print file_input
    except:
        file_input = 'UHOH'
        print file_input

def save(): #This function can definitely be improved
    global file_input, target, _FILE_, my_text_box, new_file_name
    try:
        file_input = my_text_box.get("1.0", END)
        target = open(_FILE_, "r+w")
        target.truncate()
        target.write(file_input)
    except:
        file_input = my_text_box.get("1.0", END)
        target = open(new_file_name, "r+w")
        target.truncate()
        target.write(file_input)

def exit_application():
    sys.exit(0)

def menu_open_file():
    global _FILE_, open_a_file, save, my_text_box
    try:
        open_a_file = Entry()
        open_a_file.grid(row = 3, column = 0)
        open_a_file.insert(0, "Path to File to Open")
        #save.grid_forget()
        Button(main, text = "Click to Open", command = get_file).grid(row = 4,
                                                                    column = 0)
    except:
        error_mes.error()

def get_file():
    global _FILE_, open_a_file, my_text_box
    try:
        _FILE_ = open_a_file.get()
        target = open(_FILE_, "r+w")
        opened_file = target.read()
        try:
            my_text_box.insert(INSERT, opened_file)
        except:
            error_mes.error()
    except:
        error_mes.error()

def new_file():
    global new_file_, my_text_box
    my_text_box.delete("1.0", END)
    try:
        new_file_ = Entry()
        new_file_.grid(row = 3, column = 0)
        Button(main, text = "Click to Save", command = save_new_file).grid(row = 4,
                                                                    column = 0)
    except:
        error_mes.error()

def save_new_file():
    global new_file_, new_file_name, my_text_box, target
    new_file_name = new_file_.get()
    target = open(new_file_name, "w")
    target.write(my_text_box.get("1.0", END))

my_text_box = Text(main, bg = "black", fg = "white", insertbackground = "white",
                                                                tabs = ("1c"))
my_text_box.grid(row = 0, column = 0)
#The Menu
menu = Menu(main)
main.config(menu = menu)
filemenu = Menu(menu)
menu.add_cascade(label = "File", menu = filemenu)
filemenu.add_command(label = "New...", command = new_file)
filemenu.add_command(label = "Open...", command = menu_open_file)
filemenu.add_command(label = "Save", command = save)
filemenu.add_separator()
filemenu.add_command(label = "Exit", command = exit_application)
main.mainloop()

This code works fine in Linux, but when testing in Windows, the save command throws an error for some reason, but aside from that it works fine in both OSes.

My questions are:

How can my style be improved?
Is the code understandable? Did I use acceptable variable and function names?
Building off of that, are the FILE, FILE_, new_file etc, variables confusing?
Are classes necessary with a small program like this? Should I think about rewriting the code using object oriented programming if I want to add more features (such as syntax highlighting, python console, preset formats, etc)
Any suggested edits that would make my code either easier to read or more efficient?

3.x Compatibility

If you want to make this compatible with Python 3.x, you'd have to do a few things. First off, in Python 3.x, Tkinter has been renamed to tkinter, so at the top of your file you might want to do something like this:

try:
    import tkinter as tk
except ImportError:
    import Tkinter as tk

You should also use parentheses in print, like this:

print( ... )

Style and other nitpicks

Generally, you shouldn't be using wildcard imports like this:

from Tkinter import *

If you don't like prefixing everything with Tkinter, you can just do this:

import Tkinter as tk

Rather than importing multiple modules on the same line, like this:

import sys, os

It's better to import them all on separate lines, like this:

import sys
import os

It's also very bad practice to have a bare except clause, like this:

try:
    ...
except:
    ...

Any program error that you don't want to catch will go un-noticed . This is not good. You should be doing something like this instead:

try:
    ...
except TheErrorYouWantToCatch:
    ...

You should also have two blank lines between functions, not one, like this:

def a():
    ...

def b():
    ...

On the note of functions, you should also have docstrings describing the purpose of your functions. A typical docstring looks something like this:

def my_function( ... ):
    """Brief description.

    More detailed description.

    Description of arguments.
    """
    ...

Your naming is also, not the greatest. For example, I'd rename the following variables:

file_input to input_file_path.
_FILE_ to file_contents.


Little chess game with python I am pretty sure my code does not follow PEP 8 and would definitely want someone to point out my errors specifically. I am open any form of criticism or suggestions.
import chess
import chess.engine
import time
import stockfish

engine = chess.engine.SimpleEngine.popen_uci("stockfish")
board = chess.Board()

def pact():  # Player's Action
    print('----------------\n' + 'The legal moves are: \n' +
          str(board.legal_moves) + '\n' + str(board))
    Action = input('Declare your moves: ')
    try:
        board.push_san(Action)
    except ValueError:
        # output: "integer division or modulo by zero"
        print('Invalid move, try again')

def cact():  # Computer's Action
    result = engine.play(board, chess.engine.Limit(time=2.0))
    board.push(result.move)

    print(board)

print(board)

for index in range(3, 0, -1):
    print('The game starts in ' + str(index) + 'seconds')
    time.sleep(1)
while(board.is_checkmate() == False):
    pact()
    cact()
    # print(engine.play.move(board, limit)

Ideally I would like to incorporate sound recognition and making it a hands-free chess playing with Stockfish or other player. For now, I just want to get the basics right to avoid any future bugs and improve overall tidiness so I can have a good habit in my future coding progress.

The game logic is broken in many respects.
First, the game may reach an "insufficient material" condition, in which checkmate is impossible. Infinite loop right away.
Second, the game may get into a stalemate. There is no legal moves in a stalemate, so pact would reject anything the user tries to enter. Yet another infinite loop.
Finally, the end of game condition shall be tested after each half-move. BTW, chess module conveniently offers a board.is_game_over() method, which encompasses everything above, plus three-fold repetition, plus 50 moves rule.

Board is printed only after a computer move. This is strange.

Listing the legal moves unconditionally is annoying. It should only be printed when the player enters an illegal one.

When you feel compelled to put a comment like
  def pact():  # Player's Action

it means that you fail to express yourself clearly in the code. Giving the function a meaningful name, like
  def player_action():

eliminates the need to comment it.


Seeking 5 primes such that concatenating any two of them produces another prime I have a program which solves Project Euler problem 60:

The primes 3, 7, 109, and 673, are quite remarkable. By taking any two primes and concatenating them in any order the result will always be prime. For example, taking 7 and 109, both 7109 and 1097 are prime. The sum of these four primes, 792, represents the lowest sum for a set of four primes with this property.Find the lowest sum for a set of five primes for which any two primes concatenate to produce another prime.

Unfortunately the program is too slow. For finding the result of 792 in the question it takes around 20 seconds. And for five primes it takes 12 hours, which is - although I was very happy that it printed the answer around 20:00 today after I started it 08:00 - too slow.
For the solution I was helped tremendously by the Stack Overflow community. Especially the users Neil Slater, Renzo and Chris Jester-Young who answered questions regarding the algorithm, the creation of a fast prime checker and the generation of prime pairs (see: 1, 2, 3, 4, 5). I find it great that although I know no one in real life who could help me with this I was helped so much anyway. Thanks for that!
Now for the performance: after generating all primes and pairs relatively quickly, the execution spends most of the time in a function get-all-combinations-that-satisfy-constraints. This function takes as input a list of primes and a list of allowed-pairs that consist of possible combinations of primes that combined lead to a new prime.
get-all-combinations-that-satisfy-constraints tries to create sets of primes with satisfy the constraints of the question. It starts a set with a prime in the list of primes and then checks if other primes can be concatenated to the front and back of every prime in the set so far (i.e., the resulting pairs are both in the list of pairs). Once it finds a prime it is added to a resulting set and the search is continued to try add a next prime, if this is not possible the previous prime is discarded and the next prime of the list is tried. This stops once we went through all starting primes. The result is a list of all combinations of 5 primes for which any concatenation of two results in new prime.
Disclaimer: I am not completely sure if the program does what I describe above.
Here is the function:
(define (get-all-combinations-that-satisfy-constraints allowed-pairs primes)

  (define (can-be-added? prime set)
    (cond ((null? set) #t)
          ((and (contains? allowed-pairs (list (car set) prime))
                (contains? allowed-pairs (list prime (car set))))
           (can-be-added? prime (cdr set)))
          (else #f)))

  (define (get-combination start-prime)
    (let loop ((primes (delete start-prime primes))
               (acc (list start-prime)))
      (cond ((null? primes) acc)
            ((can-be-added? (car primes) acc)
             (loop (cdr primes) (cons (car primes) acc)))
            (else (loop (cdr primes) acc)))))

  (let loop ((primes primes)
             (result '()))
    (if (null? primes) result
        (let ((combo (get-combination (car primes))))
          (if (= 4 (length combo))
              (loop (cdr primes) (cons combo result))
              (loop (cdr primes) result))))))

The results:

The program which uses this function and finds the smallest sum of four primes which concatenate to a new prime for each combination takes ~ 20 seconds. It tries combining primes up to 1.000. Full code: 4primes-20seconds.scm.

The program which uses this function and finds the smallest sum of five primes which concatenate to a new prime for each combination takes ~ 12 hours. It tries combining primes up to 10.000. Full code:  5primes-12hours.scm. (The memory limit needs to be set to 1024 MB.)

Why is the code so slow and is there any way execution time can be improved?
Note: general remarks on my Scheme style are very welcome as well!

I looked at your 4primes-20seconds.scm file and noticed a potential improvement.

    (define (get-prime-pairs lst)
  (define (find-all-prime-pairs prime lst)
    (let loop ((prime prime)
               (lst lst)
               (acc '()))
      (if (null? lst) (if (>= (length acc) 3) acc '())
          (if (prime? (list->number (append prime (car lst))))
              (loop prime (cdr lst) (cons (list prime (car lst)) acc))
              (loop prime (cdr lst) acc)))))
  (append-map (lambda (x) (find-all-prime-pairs x lst)) lst))

You may be able to  filter quite a bit more out here because you don't just want every combination of primes that can be concatenated, you should only keep your acc if it's more than length three (for 4 prime and 4 for 5 primes)

I notice you map number->list over your primes and check every combination for a match.  Just this function is n^2 time proportional to lst

You can improves your time of search from n^2 to N*log(p) (where p is the value of the prime being tested

instead of searching low to high for each possible append of primes, search high to low for each possible decomposition

(decompose '(4 6 7)) -> '(((4) (6 7)) ((4 6) (7)))
(decompose '(2)) -> '()
(define (decompose L)
 (let loop ((A L) (B '()) (acc '()))
    (cond ((null? A) acc)
          (else (let ((A (cdr A))
                      (B (append B (list (car A)))))
                  (if (null? A) acc
                  (loop A B (cons (list A B) acc))))))))

(define (get-prime-pairs lst)
  (define (find-all-prime-pairs prime)
    (if (or (null? prime) (null? (cdr prime)))
        '()
        (let ((decomps (decompose prime)))
         (filter (lambda (x) (and (prime? (list->number (car x)))
                                  (prime? (list->number (cadr x)))
                                  (prime? (list->number
                                            (apply append x)))))
                 decomps))))
  (append-map (lambda (x) (find-all-prime-pairs x)) lst))

We make sure both decomposed parts are primes and we also make sure the other combination is prime as well. (we don't have to add it to acc as it's already showed up or will show up later in lst)

You have this even worse n^3 thing going in get-all-combinations-that-satisfy-constraints . @hen you recur down the cars of primes in the get-combinations subdefine and then recur does the cars of the set in can-be-added?, and then in contains? you recur down the cars of allowed-pairs.

My suggestion is to change allowed-pairs from a list of pairs to a hash-map where the key is a prime, and the value is a list of valid pairs. For the pair (3 7), the key 3 should have 7 as a member in it's list, and pair (7 3) key 7 should have 3 in it's list. http://sicp.ai.mit.edu/Fall-2004/manuals/scheme-7.5.5/doc/scheme_12.html#SEC105

Then get-combinations and above would compress to log time. In get-combinations loop initialize primes  to lookup into the above map.

Therefore can-be-added? uses a much shorter N. Instead of allowed-pairs

  (define (can-be-added? prime set) ;psuedo-code use the hash function of your impentation
(let ((friends (hask-lookup prime allowed-pairs-hash-table))
  (let loop ((set set))
(cond ((null? set) #t)
      (contains? friends (car set))
        (can-be-added? prime (cdr set)))
      (else #f)))

I'm not 100% sure this will find all combos, you may have to have get-combination return a list of combos from trying and not-trying the next prime, but if it worked for this problem last time, not doing it should work this time.


Vue.js component for adding/removing items Simple Vue.js component, where you can add a wrestler via an AJAX endpoint, and remove a wrestler from an AJAX endpoint.

Any notes on whether I’m doing this the “Vue.js way” or tips for improvement are greatly welcomed.

The HTML template portion of the component:

<template>

Loading&hellip;

                    <button type="button" class="btn btn-outline-danger" v-on:click.prevent="removeWrestler(wrestler, index)">
                        {{ wrestler.name }}
                        &times;
                    </button>

                    <input type="text" class="form-control" autocomplete="name" required ref="name" v-bind:disabled="busy" v-model="name" v-on:keydown.enter.prevent="addWrestler" />

                        <button type="button" class="btn btn-primary" v-on:click.prevent="addWrestler" v-bind:disabled="busy">

                        </button>

</template>

The JavaScript portion of the component:

const confirm = require('./../mixins/confirm.js');

export default {
    computed: {
        submitText() {
            return this.busy ? 'Submitting&hellip;' : 'Add wrestler';
        },
        wrestlersSortedByName() {
            return this.wrestlers.sort((a, b) => {
                return a.name.localeCompare(b.name);
            });
        }
    },
    data() {
        return {
            busy: false,
            loading: true,
            name: '',
            wrestlers: []
        };
    },
    methods: {
        addWrestler() {
            this.busy = true;

            axios.post(this.action, { name: this.name })
                .then((response) => {
                    this.wrestlers.push(response.data.data);
                })
                .catch((response) => {
                    alert('Error adding wrestler to video.');
                })
                .then(() => {
                    this.busy = false;
                    this.name = '';
                    this.$refs.name.focus();
                });
        },
        loadWrestlers() {
            axios.get(this.action).then((response) => {
                this.wrestlers = response.data.data;
                this.loading = false;
            });
        },
        removeWrestler(wrestler, index) {
            const message = `Are you sure you wish to remove ${wrestler.name} from this video?`;

            this.confirm(message, () => {
                axios.delete(`${this.action}/${wrestler.id}`)
                    .then((response) => {
                        this.wrestlers.splice(index, 1);
                    })
                    .catch((response) => {
                        alert('Error removing wrestler.');
                    });
            });
        }
    },
    mixins: [
        confirm
    ],
    mounted() {
        this.loadWrestlers();
    },
    props: {
        action: {
            required: true,
            type: String
        }
    }
}

And finally, the contents of ./mixins/confirm.js:

module.exports = {
    methods: {
        confirm(message, callback) {
            if (window.confirm(message)) {
                callback();
            }
        }
    }
};

Style notes: I’m aware of the @ shorthand for v-on:, but prefer the longer form as it looks more HTML-y to me.

It looks like the code makes good use of basic VueJS concepts and practices- using computed properties, binding values, conditional display with v-if, etc.

I also like the application of arrow functions, promise handling, etc.

I see there is a key attribute bound to the list items (i.e. v-bind:key="wrestler.id"). Correct me if I am wrong but that doesn't appear to be an attribute of <li>. What is that used for? Edit: Oh yes, thanks for reminding me about the key attribute for list editing

Then in the addWrestler() method, there are two .then() callbacks. Is there a reason not to move the three lines in the second callback into the first?

axios.get(this.action, { name: this.name })
    .then((response) => {
        this.wrestlers.push(response.data.data);
        this.busy = false;
        this.name = '';
        this.$refs.name.focus();
    })
    .catch(...)

If you really wanted to shorten the code for the promise callbacks, you could abstract those into methods.


Simple hangman game from beginner This is my first project I am attempting in Python, and my first time on Stack Exchange, so please be kind.
The game uses a text file with lots of words. I was wondering how to improve the game by having different difficultly levels. E.g. easy for 1-4 letters, medium for 5-8 letters, hard for 9+ letters in the word.
All the code in print_graphics() was pre-made, so I have to figure out a way to incorporate it into the code I was writing.
I was also thinking it might look neater if it didn't keep printing new iterations of the hangman after each body part is added, but I'm not sure how I could do that without massively changing the text.
I couldn't figure out how to attach the input file so I've linked it instead.
import random
from IPython.display import clear_output
from termcolor import colored

def print_graphics(wrong_guesses):
    # list of possible body parts
    body_parts = ['  O     |', '  |     |',' /|     |', ' /|\    |', ' /      |', ' / \    |']

    # how many lines to print
    lines = 4 if wrong_guesses != 0 else 5

    # check number provided is usable
    if 0 <= wrong_guesses <= 6:
        print('  +-----+')  # print top of frame
        print('  |     |')

        # print the correct body parts for current state
        if wrong_guesses > 0:
            if wrong_guesses > 1:
                print(body_parts[0])
                lines -= 1
            if wrong_guesses > 4:
                print(body_parts[3])
                lines -= 1
            print(body_parts[wrong_guesses-1])

        for i in range(lines):
            print('        |')  # print the lines

        print('==========')  # print the floor

with open("wordlist.txt") as file:
  words = file.read().split()
  word = random.choice(words)

guessed_word = []
guessed_letters = []
max_guesses = 5
length_word = len(word)
alphabet = "abcdefghijklmnopqrstuvwxyz"
letter_storage = []
body_parts = ['  O     |', '  |     |',' /|     |', ' /|\    |', ' /      |', ' / \    |']

def start():
  print("\033[1;34m"+"Welcome to hangman! You have 6 tries to guess the correct word, enter a letter to get started", "\033[0;30m")
start()

def intro():
  for character in word:
    guessed_word.append("_")
  print("The word you are guessing has", length_word, "characters")
  print_graphics(0)
intro()

def play(word):
  wrong_guesses = 0
  while wrong_guesses <6:
    guess = input("Guess a letter or word: ")

    if guess in guessed_letters:
      print("\033[1;31m"+"Whoops! You have already guessed this letter. Don't worry, you still have", 6-wrong_guesses, "guesses remaining" "\033[0;30m")
    if guess not in alphabet:
      print("\033[1;31m"+"Character invalid. Please enter a letter", "\033[0;30m")

    elif guess in alphabet and guess not in guessed_letters:
      if guess in word:
        print("\033[1;3;32m"+"Well done", guess.upper(), "is correct!", "\033[0;30m")
        guessed_letters.append(guess)
        for i in range(0, length_word):
          if word[i] == guess:
            guessed_word[i] = guess
        print(guessed_word)
        if not "_" in guessed_word:
          print("\033[1;3;32m"+"Congratulations, you beat the Hangman!", "\033[0;30m")
          break

      elif guess not in word:
        wrong_guesses += 1
        print(print_graphics(wrong_guesses))
        print(guessed_word)
        print("\033[1;31m"+guess.upper(), "is not correct. You have", 6-wrong_guesses, "guesses left", "\033[0;30m")
        guessed_letters.append(guess)
    if wrong_guesses == 6:
      print("\033[1;31m"+"Bad luck! You have been beaten by the Hangman. The word you were trying to guess was", word, "\033[0;30m")

play(word)
print("\033[1;3;35m"+"Game over, thank you for playing :)")

You write

from termcolor import colored

but then ignore that and write non-portable escape sequences directly in the strings:

  print("\033[1;34m"+"....", "\033[0;30m")

Doesn't that defeat the point of termcolor, which is supposed to adapt to the connected terminal?


2D Game Framework in C++ and SDL I'm making a simple 2D framework / engine for the first time based on two books and before the project gets too big I would like to know if I'm ordering things the right way. Maybe there are better ways of doing this. I tried to apply patterns so I could decouple as much as possible and keep things abstract.
For example this is the Game class:
#include "Game.h"

void Game::Create(int width, int height, const char * title, int window_flag, int renderer_flag){
      display.CreateWindow(800,600,"Game", SDL_WINDOW_SHOWN, SDL_RENDERER_ACCELERATED);
}

void Game::Start(){
      Uint32 first_frame_ticks; float delta_time = 0;
      SDL_Renderer * renderer = display.GetRenderer();
      while(input_handler.GetLastEvent() != SDL_QUIT){
            first_frame_ticks = SDL_GetTicks();

            bool state_manager_empty = state_manager.isEmpty();
            //update
            input_handler.Update();
            if(!state_manager_empty) state_manager.GetGameState()->onUpdate(delta_time);

            //render
            SDL_RenderClear(renderer);
            if(!state_manager_empty) state_manager.GetGameState()->onRender();
            SDL_RenderPresent(renderer);

            delta_time = (SDL_GetTicks() - first_frame_ticks) / 1000.0f;
      }

      display.Clear();
}

void Game::AddState(GameState* state){
      state->Set(display.GetRenderer(), &input_handler);
      state_manager.Add(state);
}

Or the Drawer class (wich I have some doubts about, the DrawEntity method is static)
#include "Drawer.h"

void Drawer::DrawEntity(Entity& entity, TextureManager* texture_manager, SDL_Renderer * renderer){
      texture_manager->Draw(entity.GetSprite(), entity.GetPosition(), renderer);
}

It acts like a mediator between TexureManager(wich draws texures) and Entity wich has all the information like position, scale, etc.
Finally the example input class
#include "InputHandler.h"
#include <cstdio>

InputHandler::InputHandler(){
      keyboard_state = SDL_GetKeyboardState(NULL);
      printf("Event handler initialized\n");
}

void InputHandler::Update(){
      SDL_Event ev;
      while(SDL_PollEvent(&ev)){
            if(ev.type == SDL_KEYUP || ev.type == SDL_KEYDOWN){
                  keyboard_state = SDL_GetKeyboardState(NULL);
            }else last_event = ev.type;
      }
}

bool InputHandler::isKeyPressed(SDL_Scancode key){
      if(keyboard_state[key]) return true;
      else return false;
}

Uint32 InputHandler::GetLastEvent(){
      return last_event;
}

I'm going to upload a dependency diagram as well as the GitHub repo:

Github Repo with all the Code
https://github.com/grazianobolla/monke-engine

Update:
Well, if someone wants to know, I've done some changes for good,
one of the main problems I had was that I couldn't really "connect" the GameStates to the main Engine itself without passing a pointer to the engine (wich is messy) or individual pointers to the InputHandler, Display, etc.
The thing was that for example the InputHandler handled the Window events and the keyboard input, so for a GameState to have access to it, I would have to create a pointer to Game::InputHandler, so I did a few things:
First I created a KeyboardHandle class wich is totally independent from the Engine itself and its only function its to get keyboard input, that way I can make every game state to have an independent KeyboardHandle, also every game state has a pointer to the engine Display class, that means that from the game state you can get access to information like windows size or the SDL_Renderer without having much trouble, you add states from the Engine so the Display pointer is set at that moment.
The GameState class is the base for all games, now it looks like this:
#pragma once
#include "KeyboardHandle.h"
#include "TextureManager.h"
#include "Display.h"

class GameState{
protected:
      KeyboardHandle keyboard;
      TextureManager texture_manager;
      Display * screen_display;
public:
      GameState(Display * display){ this->screen_display = display; }
      virtual void onStart() = 0;
      virtual void handleInput() = 0;
      virtual void onUpdate(float) = 0;
      virtual void onRender() = 0;
};

As you can see is almost all independent from the Engine, it just shares the display so we can render the graphics.
Here is an example state (drawing an image on the center of the screen):
#include "GameState.h"
#include "Entity.h"

class DefaultState : public GameState{
      Entity splash_logo;
public:
      DefaultState(Display * display) : GameState(display){ }

      void onStart(){
            texture_manager.Load("textures/splash_logo.png", "splash_logo", screen_display->GetRenderer());
            splash_logo = Entity("splash_logo", Vector2i(0, 0), Vector2i(336, 336), 1.0f);

            float center_x = (screen_display->GetWindowWidth() / 2) - splash_logo.GetCollisionBox().w / 2;
            float center_y = (screen_display->GetWindowHeight() / 2) - splash_logo.GetCollisionBox().h / 2;
            splash_logo.SetPosition({center_x, center_y});
      }

      void handleInput(){

      }

      void onUpdate(float delta_time){

      }

      void onRender(){
            texture_manager.Draw(splash_logo.GetSprite(), splash_logo.GetPosition(), screen_display->GetRenderer());
      }
};

And just for fun here is the dependency diagram:


Custom Comparison at the object This question is NOT about passing a Comparison to a Sort.

The question is how to pass a Comparison to an object for object to object  (1:1) Comparison.   Some users must be able to pass in a Comparison.  They want 2 as want be able to tell when they give different comparison.

The way this solution approaches it is to pass a Comparison delegate to the constructor.

static int CompareMotorPrice(Motorcycle motorcycle1, Motorcycle motorcycle2)
{
    return motorcycle1.Price.CompareTo(motorcycle2.Price);
}
static int CompareMotorSpeed(Motorcycle motorcycle1, Motorcycle motorcycle2)
{
    return motorcycle1.Speed.CompareTo(motorcycle2.Speed);
}
public class Motorcycle : IComparable
{
    public string Name { get; }
    public int Speed { get; set; }
    public int Price { get; set; }
    public Comparison<Motorcycle> CompareDelegate { get; set; }
    public Comparison<Motorcycle> CompareDefault { get; set; }
    public int CompareViaDelegate(Motorcycle motorcycle2)
    {
        if (CompareDelegate == null || motorcycle2 == null)
        {
            throw new ArgumentNullException();
        }
        return (CompareDelegate(this, motorcycle2));
    }
    public int CompareTo(object obj)
    {
        if (obj == null) return 1;

        Motorcycle otherMotorcycle = obj as Motorcycle;
        if (otherMotorcycle != null)
        {
            if (CompareDefault == null)
            {
                return (this.Name.CompareTo(otherMotorcycle.Name));
            }
            else
            {
                return CompareDefault(this, otherMotorcycle);
            }
        }
        else
        {
            throw new ArgumentException("Object is not a otherMotorcycle");
        }
    }
    public Motorcycle(string name, int price, int speed, Comparison<Motorcycle> compareDefault = null, Comparison<Motorcycle> compareDelegate = null)
    {
        Name = name;
        Price = price;
        Speed = speed;
        CompareDelegate = compareDelegate;
        CompareDefault = compareDefault;
    }
}

test:

Motorcycle motorcycle1 = new Motorcycle("harley", 1000, 120, CompareMotorSpeed, CompareMotorPrice);
Motorcycle motorcycle2 = new Motorcycle("yamaha", 1900, 130, CompareMotorSpeed, CompareMotorPrice);
Motorcycle motorcycle3 = new Motorcycle("susuki", 1800, 150, CompareMotorSpeed, CompareMotorPrice);
Debug.WriteLine(motorcycle1.CompareViaDelegate(motorcycle2));
List<Motorcycle> Motorcycles = new List<Motorcycle>() { motorcycle1, motorcycle2, motorcycle3 };
foreach (Motorcycle m in Motorcycles)
{
    Debug.WriteLine($"name {m.Name}   price {m.Price}   speed {m.Speed}");
}
Motorcycles.Sort(CompareMotorPrice);
foreach (Motorcycle m in Motorcycles)
{
    Debug.WriteLine($"name {m.Name}   price {m.Price}   speed {m.Speed}");
}
Motorcycles.Sort();
foreach (Motorcycle m in Motorcycles)
{
    Debug.WriteLine($"name {m.Name}   price {m.Price}   speed {m.Speed}");
}

1)

    if (CompareDelegate == null || motorcycle2 == null)
    {
        throw new ArgumentNullException();
    }

This exception is misleading. First, it does not specify which parameter is null. Second, strictly speaking CompareDelegate is not a parameter.

2) I would set the default value for CompareDefault:

public Comparison<Motorcycle> CompareDefault { get; set; } =
    other => Name.CompareTo(other.Name);

this way you can probably remove some of the null checks and simplify CompareTo implementation.

3) Overall, I don't like this approach.

it scales poorly.
it hides comparison logic: there is no way to learn how two objects are going to be compared just by reading the code, the comparison logic is hidden inside some delegate.
most importantly, you break asymmetric property of comparison. If a.CompareTo(b) > 0 then I expect that b.CompareTo(a) < 0. But it is not the case, since nothing in your code prevents two objects from using different delegates for comparison. This alone looks like a huge source of problems to me.

IMHO, just write a bunch of regular IComparer<T> implementations and be done with it. It has none of the problems I mentioned above:

//custom comparers are implemented as stand alone classes,
//they are not part of Motorcycle class.
public class Motorcycle
{
    public string Name { get; }
    public int Speed { get; set; }
    public int Price { get; set; }

    public Motorcycle(string name, int price, int speed)
    {
        Name = name;
        Price = price;
        Speed = speed;
    }
}

//It also guarantees asymmetry:
//if SpeedComparer.Compare(a,b) < 0 then SpeedComparer.Compare(b,a) > 0 is also true
class SpeedComparer : IComparer<Motorcycle>
{
    public int Compare(Motorcycle first, Motorcycle second)
    {
        //add null checks

        return first.Price.CompareTo(second.Price);
    }
}

class NameComparer : IComparer<Motorcycle>
{
    public int Compare(Motorcycle first, Motorcycle second)
    {
        //add null checks

        return first.Name.CompareTo(other.Name);
    }
}

Usage:

public void Test()
{
    var motorcycle1 = new Motorcycle("harley", 1000, 120);
    var motorcycle2 = new Motorcycle("yamaha", 1900, 130);
    var motorcycle3 = new Motorcycle("susuki", 1800, 150);
    var motorcycles = new List<Motorcycle>() { motorcycle1, motorcycle2, motorcycle3 };

    IComparer<Motorcycle> selectedComparer = new NameComparer();
    //compares by name
    TestComparison();

    selectedComparer = new SpeedComparer();
    //selectedComparer is changed, so now speeds are compared
    TestComparison();

    void TestComparison()
    {
        Debug.WriteLine($"Using {selectedComparer.GetType().Name}...");
        Debug.WriteLine(selectedComparer.Compare(motorcycle1, motorcycle2));
        motorcycles.Sort(selectedComparer);
        motorcycles.ForEach(m => Debug.WriteLine($"name {m.Name}   price {m.Price}   speed {m.Speed}"));
    }
}


Removing duplicate audio files with different UTC timestamps in filenames, using PowerShell 5 I have a drive with many thousands of sound effects.  The files have been stored on different operating systems and NAS devices over time and there are now duplicates of many files, but the filenames contain different UTC timestamps.  For example:

1-14 Crowded Bowling Alley (2016_06_28 02_14_41 UTC).aif

1-14 Crowded Bowling Alley (2016_02_18 05_56_59 UTC).aif

I would like to remove the duplicate files.  There are also junk files left over from audio software and operating system indexing that I would like to remove. Using a copied subset of the data I have tested the following PowerShell script under PS version 5.1, it appears to give me what I'm after:

# Remove files left over from audio editors
Get-ChildItem -Name -Recurse -Filter "*.clip*" | Remove-Item
Get-ChildItem -Name -Recurse -Filter "*.ptf" | Remove-Item
Get-ChildItem -Name -Recurse -Filter "*.wfm" | Remove-Item
Get-ChildItem -Name -Recurse -Filter "*.repeaks" | Remove-Item

# Remove old indexing files
Get-ChildItem -Name -Recurse -Filter "*.DS_STORE" | Remove-Item
Get-ChildItem -Name -Recurse -Filter "._.*" | Remove-Item
Get-ChildItem -Name -Recurse -Filter "Thumbs*.db" | Remove-Item
Get-ChildItem -Name -Recurse -Filter "*.ini" | Remove-Item

# Remove UTC stamp from all files (will fail on duplicates)
Get-ChildItem -Recurse -Filter "*(?????????? ???????? UTC).*" | Rename-Item -newname { $_.basename.substring(0,$_.basename.length-26) + $_.Extension }

# Remove files that still have the UTC stamp (they were the duplicates)
Get-ChildItem -Name -Recurse -Filter "*(?????????? ???????? UTC).*" | Remove-Item

However, I would love to get a critique of the script and learn a few things.  In particular it seems a little hacky to use an expected failed command as part of the workflow.  It also seemed wise to get notes before I run it on the entire set of files.

While -Filter is usually the fastest way as it handled by the provider,
I'm not sure this is true compared to one -Include with an array which only recurses once instead of 8 times.

$Include = '*.clip*','*.ptf','*.wfm','*.repeaks','*.DS_STORE','._.*','Thumbs*.db','*.ini'

Get-ChildItem  -Recurse -Name -Include $Include | Remove-Item

As for the duplicates, your approach will only eliminate dupes in the same folder.
An alternative would be grouping the output with a calculated property which strips off the date in parentheses. And then iterating the groups and sorting by whatever means, keeping only the first/last - removing the others.


Rounding ints division result to certain "granularity" In my program I call "granularity" the "minimum step" for a number. For example, if granularity == 4 then I only work with numbers "0, 4, 8, 12 etc" (so normal integer has granularity == 1).

I need a function to round an integer number to a certain granularity. So for example, if granularity == 3, I want this map:

// I don't use negative number so don't care about negative results
0 -> 0
1 -> 0
2 -> 3
3 -> 3
4 -> 3
5 -> 6
6 -> 6
7 -> 6
8 -> 9
and so on

This is my implementation:

#include <iostream>
#include <stdint.h>

using namespace std;

typedef int64_t myDecimal;

myDecimal Round(myDecimal val, myDecimal granularity)
{
    myDecimal rest = val % granularity;
    myDecimal candidat = val / granularity;
    if (2 * rest < granularity) {
        return candidat * granularity;
    } else {
        return (candidat + 1) * granularity;
    }
}

int main()
{
    for (int i = -10; i < 10; i++){
        std::cout << "i = " << i << " " << Round(i, 3) << std::endl;
    }
    return 0;
}

How can you further improve it for better latency as I'm using this in HFT trading?

Let me start with the usual:

Naming

myDecimal: Usually, the prefix "my" does not bear any sensible information. You should think about the information you want to pass by the types name. (For example Decimal is often the name for decimal fractions and the way in which numbers are stored in it!)
rest: in the context of a modulus this is called remainder
candidat: this is a candidat but where is the other one and which one is it? You should call this something like: flooredValue

Reusability

Everytime that you use this function you have to use the exact type or must cast, which is slow. You should think about making it a template function.

Performance

Most of this is micro optimizations but you seem to be interested in every last bit of time. I don't know how much of this is automatically done by your optimizing compiler:

go to assembler: This is very easy code that can be written in assembler. Especially the division and modulus are two functions that are slow (compared to others) and (at least) on x86 architectures the assembler instruction for division gives you the modulus at the same time. (likely optimized by the compiler)
multiplication by 2: this can be replaced by a left shift by one (very likely optimized by the compiler)
multiplication in return: instead of using a multiplication you can use an addition to return the results (return val - remainder; and return val + granularity - remainder;) (maybe not optimized by the compiler)

Note that all of those "optimizations" could be done automatically by the compiler and even more important might not be optimizations on modern architectures anymore. When it comes to performance you should compare on the assembler level and do extensive profiling.


Cast inside the method or let the client code cast, which one is better? I have two choices of implementing a method, the first one is a generic type where the client code does not have to cast

    public T GetControl<T>(string controlName) where T : Control
    {
        return (T)ControlManager.GetControl( controlName);
        //I simplify the logic here, in my case this is a bit
        //complicated than this, but the object we get is of the Type Control
    }

client code:

MyControl control = GetControl<MyControl>("foo");

The second choice is the non-generic one. Here the client code will have to cast

    public Control GetControl(string controlName)
    {
        return ControlManager.GetControl(controlName);
        // I simplify the logic here, in my case this is a bit
        // complicated than this, but the object we get is of the Type Control
    }

client code:

    MyControl control = (MyControl)GetControl("foo");

Is one of them better than the other? At first I thought first one is better, because casting (which I think is a bit dirty) is refactored in one method, and don't have to be repeated in all client codes. But since the generic type has to be specified by the client code anyway (generic methods can't infer return type), basically the client code only shift (MyControl) in front to <MyControl> in the generic parameter, so it's probably not better anyway.

What is your opinion?

In my opinion the generic method is better for two reasons. First, it's cleaner, both for the client calling it because they don't have to cast and for the process of casting because it is only done in one place. Secondly, the client that's asking for the control knows the type of the control in this instance so it's very sensible, and good practice, to have the client specify that rather than sending back an object just so the client has to cast it. That's why generics exist.

However, I recommend in your code in the method you use the as operator rather than a cast because even though the client calling you specified the type, the as operator will simply return null instead of throwing an exception.


TweetResolver class, to be used in a graphql project This is my module TweetResolver (tweet-resolver.js) :

import Tweet from '../../models/Tweet';
import { requireAuth } from '../../services/auth';

export default {
    getTweet: async (_, { _id }, { user }) => {
        try {
            await requireAuth(user);
            return Tweet.findById(_id);
        } catch (error) {
            throw error;
        }
    },
    getTweets: async (_, args, { user }) => {
        try {
            await requireAuth(user);
            return Tweet.find({}).sort({ createdAt: -1 });
        } catch (error) {
            throw error;
        }
    },
    createTweet: async (_, args, { user }) => {
        try {
            await requireAuth(user);
            return Tweet.create(args);
        } catch (error) {
            throw error;
        }
    },
    updateTweet: async (_, { _id, ...rest }, { user }) => {
        try {
            await requireAuth(user);
            Tweet.findByIdAndUpdate(_id, rest, { new: true });
        } catch (error) {
            throw error;
        }
    },
    deleteTweet: async (_, { _id }, { user }) => {
        try {
            await requireAuth(user);
            await Tweet.findByIdAndRemove(_id);
            return {
                message: 'Tweet has been deleted'
            }
        } catch (error) {
            throw error;
        }
    }
}

It is used in my graphql project like this:

import GraphQLDate from 'graphql-date';
import TweetResolvers from './tweet-resolvers';
import UserResolvers from './user-resolvers';

export default {
    Date: GraphQLDate,
    Query: {
        getTweet: TweetResolvers.getTweet,
        getTweets: TweetResolvers.getTweets
    },
    Mutation: {
        createTweet: TweetResolvers.createTweet,
        updateTweet: TweetResolvers.updateTweet,
        deleteTweet: TweetResolvers.deleteTweet,
        signup: UserResolvers.signup,
        login: UserResolvers.login
    }
}

Correct me if I am wrong, but I feel there is a lot of code duplication in my tweet-resolver.js module.

Almost every method signature on this module is identical (as I only want authenticated users to be able to do anything via graphql).

Every execution is inside a try/catch block.

Is there a way to remove code duplication in this. Or is this style of coding acceptable?

If all your catch is doing is re-throwing the error, there is no reason to have the try/catch in the first place.
If you find yourself using export default {}; to export an object, it is a huge sign that you should be using named exports instead, since you're essentially emulating them. This means you'll want to do import * as Foo from instead of import Foo from on these "bag of functions" modules.
You can make a wrapper function to remove the repetitive requireAuth call.
A wrapper function also allows you to remove the params you don't care about in the implementation, like the first and third parameters to these functions.

To update your code with these, I'd do

import Tweet from '../../models/Tweet';
import { requireAuth } from '../../services/auth';

let withAuth = callback => async (_, args, data) => {
    await requireAuth(data.user);
    return callback(args);
};

export const getTweet = withAuth(({ _id }) => Tweet.findById(_id));
export const getTweets = withAuth(() => Tweet.find({}).sort({ createdAt: -1 }));
export const createTweet = withAuth((args) => Tweet.create(args));
export const updateTweet = withAuth(({ _id, ...rest }) => Tweet.findByIdAndUpdate(_id, rest, { new: true }));
export const deleteTweet = withAuth(async ({_id}) => {
    await Tweet.findByIdAndRemove(_id);
    return {
        message: 'Tweet has been deleted'
    };
});

with

import GraphQLDate from 'graphql-date';
import * as TweetResolvers from './tweet-resolvers';
import UserResolvers from './user-resolvers';

export const Date = GraphQLDate;
export const Query = {
    getTweet: TweetResolvers.getTweet,
    getTweets: TweetResolvers.getTweets,
};
export const Mutation = {
    createTweet: TweetResolvers.createTweet,
    updateTweet: TweetResolvers.updateTweet,
    deleteTweet: TweetResolvers.deleteTweet,
    signup: UserResolvers.signup,
    login: UserResolvers.login,
};


Functional method for storing a query string as a simple key-value object I'm working on an isomorphic javascript module (query-hash) to handle query strings and base64 tokens. It's essentially a simple key-value object with methods for taking in data and giving it back in the format you ask for.

function queryToObject(queryString) {
    return queryString.split('&')
        .map(kv => kv.split('='))
        .reduce((p, kv) => {
            p[kv[0]] = decodeURIComponent(kv[1] || '').replace(/\+/g, ' ');

            return p;
        }, {});
}

For the "functional approach", mapping the array created by split() seems like unnecessary function calls. Should I be splitting the string in the reduce() method? Is there a better approach?

For this simple use case, you are correct that you could do all this work in a reduce step saving yourself an additional iteration over the array created by the split() operation.   Now whether this is enough of a performance impact for your expected use cases to move away from the (probably clearer-to-understand) map-reduce pattern is something that perhaps you would need do some testing to decide how you want to approach the problem.

If for example, you ever foresee yourself adding a filter() step, then perhaps keeping separation between map - filter - reduce steps makes sense.

I do think there are some edge cases around your basic query-split approach which might not be handled well here.  For example, what about parameters with array notation (i.e. param[]=foobar)?  You can certialny do plenty of search on the internet to gets lots of ideas on how to approach the basic query string parsing logic.


Sending an SMS message in C Recently I've been fiddling with the idea of sending a text message using C.  Looking at a few options, I thought that using libcurl to send an SMTP email using TLS for some security would be the best and easiest option.  Here's what I would like reviewed:

Method: Is there a better/easier way I could be sending an SMS?
Security: Is my current security enough? How could I add more security to make sure my authentication details aren't being snooped on?
SMS Format: Is there a way I could format my payload to look more like what it would if I sent it off using a normal email client?  The normal email client sends something that looks like the image on the left, with my first name being blacked out; the image on the right is what it looks like sent from my program with my email being censored.

Conversations: With my program, a new conversation is generated each time an SMS is sent, with the numbers at the top (950-9 in this case) being randomly chosen anew each time.  Could an improvement be made to the program so that each new SMS message stays in the same conversation?

#include <stdio.h>
#include <string.h>
#include <curl/curl.h>

typedef struct
{
    int lines_read;
} UploadStatus;

size_t payload_source(void *ptr, size_t size, size_t nmemb, void *userp)
{
    if((size == 0) || (nmemb == 0) || ((size * nmemb) < 1)) return 0;

    UploadStatus *upload = (UploadStatus*) userp;
    const char *payload[] =
    {
        "Subject: SMS Test\r\n",
        "\r\n", /* empty line to divide headers from body, see RFC5322 */
        "Test",
        NULL
    };
    const char *data = payload[upload->lines_read];

    if(data)
    {
        size_t len = strlen(data);
        memcpy(ptr, data, len);
        upload->lines_read++;
        return len;
    }
    return 0;
}

int main(void)
{
    CURLcode res = CURLE_OK;
    struct curl_slist *recipients = NULL;
    UploadStatus upload_ctx = {0};
    const char *from = "test@gmail.com";
    const char *to = "UrTxtEmail"; // see http://www.emailtextmessages.com/
    CURL *curl = curl_easy_init();

    if(curl)
    {
        // set username and password
        curl_easy_setopt(curl, CURLOPT_USERNAME, from);
        curl_easy_setopt(curl, CURLOPT_PASSWORD, "password");
        curl_easy_setopt(curl, CURLOPT_URL, "smtp://smtp.gmail.com:587/");
        // start with normal connection, and upgrade to TLS using STARTTLS
        curl_easy_setopt(curl, CURLOPT_USE_SSL, (long) CURLUSESSL_ALL);
        curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, 0L);
        curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, 0L);
        curl_easy_setopt(curl, CURLOPT_MAIL_FROM, from);
        recipients = curl_slist_append(recipients, to);
        curl_easy_setopt(curl, CURLOPT_MAIL_RCPT, recipients);
        curl_easy_setopt(curl, CURLOPT_READFUNCTION, payload_source);
        curl_easy_setopt(curl, CURLOPT_READDATA, &upload_ctx);
        curl_easy_setopt(curl, CURLOPT_UPLOAD, 1L);
        // useful for debugging encryped traffic
        curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);

        // send the message
        res = curl_easy_perform(curl);
        if(res != CURLE_OK) fprintf(stderr, "curl_easy_perform() failed: %s\n", curl_easy_strerror(res));
        curl_slist_free_all(recipients);
        curl_easy_cleanup(curl);
    }

    return (int) res;
}

A few notes:

First off, your UploadStatus structure only contains one variable, which is somewhat useless IMO.  Instead, I would make some sort of email structure that contains all the necessary parts of an email in order for your program to send it off.  Here's a structure that I came up with to use:

 typedef struct
 {
     const char* to;
     const char* from;
     const char** email;
     size_t pos;
 } EmailData;

You may note that I changed your lines_read to pos and made it a size_t since I doubt you will ever read a negative number of lines.
The payload_source() method looks to be your CURLOPT_READFUNCTION, but it doesn't have the right format.  It should look like this prototype according to the documentation:

size_t read_callback(char *buffer, size_t size, size_t nitems, void *instream);

Also, the current name of your function doesn't really convey to me that it is a callback when reading your code.  To aid readability, I would change it to something such as readCallback().
Right now you have your email hardcoded within your code... something I would definitely look to change to be more dynamic.  I would suggest creating a sort of message constructing function that would fulfill such a purpose, store it in your EmailData struct, and then pass that structure to your callback function as the instream parameter.
To fix your formatting I would look at the original HTML email that is sent from a Gmail client and then dissect that.  Here is an example of one that I sent off:

MIME-Version: 1.0
Received: by 10.37.194.194 with HTTP; Tue, 11 Aug 2015 18:42:25 -0700 (PDT)
Date: Tue, 11 Aug 2015 20:42:25 -0500
Delivered-To: person@gmail.com
Message-ID: <CJYf78Vnxbma4wdV1mZuBXHPeEptOzyjH1vLMzLwnhoXsPw@mail.gmail.com>
Subject: Gmail Test
From: syb0rg <person@gmail.com>
To: 5555555555@tmomail.net
Content-Type: multipart/alternative; boundary=001a11c04f6ca3e68e051d1354cc

--001a11c04f6ca3e68e051d1354cc
Content-Type: text/plain; charset=UTF-8

Body content

--001a11c04f6ca3e68e051d1354cc
Content-Type: text/html; charset=UTF-8

Body content

--001a11c04f6ca3e68e051d1354cc--

The actual dissection of each line of this email is done in my final code with comments

Final Code

Please note that this code leaks memory and doesn't contain a unique ID generation method.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include <curl/curl.h>

typedef struct
{
    const char* to;
    const char* from;
    const char** email;
    size_t pos;
} EmailData;

EmailData* initEmailData(EmailData* data, const char* to, const char* from, const char* subject, const char* input)
{
    data->to = to;
    data->from = from;

    // the added numbers to the length of the string are the pre-computed
    // lengths of the strings to be concatenated + 1 (for termination char)
    size_t htmlSize = strlen(input) + 24;
    char *html = malloc(htmlSize);
    snprintf(html, htmlSize, "%s%s%s", "
", input, "
\r\n");

    size_t headerSize = strlen(subject) + 12;
    char *header = malloc(1);
    snprintf(header, headerSize, "%s%s%s", "Subject: ", subject, "\r\n");

    size_t receiverSize = strlen(to) + 7;
    char *receiver = malloc(receiverSize);
    snprintf(receiver, receiverSize, "%s%s%s", "To: ", to, "\r\n");

    size_t senderSize = strlen(from) + 9;
    char *sender = malloc(senderSize);
    snprintf(sender, senderSize, "%s%s%s", "From: ", from, "\r\n");

    size_t timeSize = 40;
    char* timeString = malloc(timeSize);
    time_t rawtime = 0;
    time(&rawtime);
    struct tm *timeinfo = localtime(&rawtime);
    strftime (timeString, timeSize, "Date: %a, %d %b %Y %T %z\r\n", timeinfo);

    const char **payload = malloc(sizeof(char*) * 17);
    const char* setup[] =
    {
        // extends the format of email to support more data (vidoes, audio, etc)
        "MIME-Version: 1.0\r\n",
        // when the email was sent
        timeString,
        // who the email is to
        // TODO: multiple recipient support
        receiver,
        // who the email is from
        sender,
        // https://en.wikipedia.org/wiki/Message-ID
        //"Message-ID: <CJYf78Vnxbma4dV1mZuBpXPeEptOzyjHvLMBzLwnhoXsPw@mail.gmail.com>\r\n"
        // email subject, optional
        header,
        // describe the data contained in the body
        // https://tools.ietf.org/html/rfc2046#section-5.1.1
        "Content-Type: multipart/alternative; boundary=border\r\n",
        // empty line to divide headers from body, see RFC5322
        "\r\n",
        // all boundaries must start with two hyphens
        "--border\r\n",
        // let recipient interpret displaying text, send plain text & HTML
        // specify the plain text encoding
        "Content-Type: text/plain; charset=UTF-8\r\n",
        "\r\n",
        input,
        "\r\n",
        "--border\r\n",
        // specify the HTML encoding
        "Content-Type: text/html; charset=UTF-8\r\n",
        "\r\n",
        html,
        "\r\n",
        // final boundary ends with two hyphens as well
        "--border--\r\n",
        NULL
    };
    for (int i = 0; setup[i]; i++)
    {
        *(payload + i) = setup[i];
    }
    data->email = payload;
    data->pos = 0;
    return data;
}

size_t readCallback(char *ptr, size_t size, size_t nmemb, void *userp)
{
    if((size == 0) || (nmemb == 0) || ((size * nmemb) < 1)) return 0;

    EmailData *upload = (EmailData*) userp;
    const char *email = upload->email[upload->pos];

    if(email)
    {
        size_t len = strlen(email);
        if (len > size * nmemb) return CURL_READFUNC_ABORT;
        memcpy(ptr, email, len);
        upload->pos++;
        return len;
    }
    return 0;
}

int main(void)
{
    CURLcode res = CURLE_OK;
    struct curl_slist *recipients = NULL;
    const char *from = "person@gmail.com";
    const char *password = "password";
    const char *to = "5555555555@tmomail.net"; // see http://www.emailtextmessages.com/
    EmailData data = {0};
    initEmailData(&data, to, from, "Test Subject", "Test content");
    CURL *curl = curl_easy_init();

    if (curl)
    {
        // set username and password
        curl_easy_setopt(curl, CURLOPT_USERNAME, from);
        curl_easy_setopt(curl, CURLOPT_PASSWORD, password);
        curl_easy_setopt(curl, CURLOPT_URL, "smtp://smtp.gmail.com:587/");
        // start with normal connection, and upgrade to TLS using STARTTLS
        curl_easy_setopt(curl, CURLOPT_USE_SSL, (long) CURLUSESSL_ALL);
        curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, 0L);
        curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, 0L);
        curl_easy_setopt(curl, CURLOPT_MAIL_FROM, from);
        recipients = curl_slist_append(recipients, to);
        curl_easy_setopt(curl, CURLOPT_MAIL_RCPT, recipients);
        curl_easy_setopt(curl, CURLOPT_READFUNCTION, readCallback);
        curl_easy_setopt(curl, CURLOPT_READDATA, &data);
        curl_easy_setopt(curl, CURLOPT_UPLOAD, 1L);
        // useful for debugging encryped traffic
        curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);

        // send the message
        res = curl_easy_perform(curl);
        if(res != CURLE_OK) fprintf(stderr, "curl_easy_perform() failed: %s\n", curl_easy_strerror(res));
        curl_slist_free_all(recipients);
        curl_easy_cleanup(curl);
    }

    return (int) res;
}


Socket namespace for a chat module I finally managed to work with socket.io namespace stuff which I'm using for building a chat module. Here, employees of multiple organizations can join and vhat with other employees of the respective organization. What I'm doing at here is creating separate namespaces for each organization. So, it'll be easier for me to manage all employees of different organizations.

Here is my server side code:

var express = require('express'),
http = require('http'),
app = express(),
server = http.createServer(app),
io = require('socket.io').listen(server);

var nsp_1005 = io.of('/nsp_bucket_1005');
nsp_1005.on('connection', function(socket){
    console.log('someone connected to namespace bucket 1005');

    socket.on('addEmp', function(login_org_id, login_emp_id, login_emp_name){
        console.log('addEmp - Org_Id : '+login_org_id);
        console.log('addEmp - Emp_Id : '+login_emp_id);
        console.log('addEmp - Emp_Name : '+login_emp_name);
    });

    socket.on('disconnect', function(){
        console.log('Someone disconnected from namespace bucket 1005.');
    });
});

var nsp_1010 = io.of('/nsp_bucket_1010');
nsp_1010.on('connection', function(socket){
    console.log('someone connected to namespace bucket 1010');

    socket.on('addEmp', function(login_org_id, login_emp_id, login_emp_name){
        console.log('addEmp - Org_Id : '+login_org_id);
        console.log('addEmp - Emp_Id : '+login_emp_id);
        console.log('addEmp - Emp_Name : '+login_emp_name);
    });

    socket.on('disconnect', function(){
        console.log('Someone disconnected from namespace bucket 1010.');
    });
});

Those 1005, 1010 codes are Organization IDs. Sorry for the wired naming scheme. But, one thing right now I'm feeling the way I've made this code is not so good. Because I'm duplicating the code at the time of creating namespace for each organization. Can anyone suggest a better way to arrange this code?

Extract the common logic to a function,
where the varying parts are parameters.
Unless I'm missing something, this looks trivially easy to do:

function setup_namespace(org_id) {
    var nsp = io.of('/nsp_bucket_' + org_id);
    nsp.on('connection', function(socket){
        console.log('someone connected to namespace bucket ' + org_id);

        socket.on('addEmp', function(login_org_id, login_emp_id, login_emp_name){
            console.log('addEmp - Org_Id : ' + login_org_id);
            console.log('addEmp - Emp_Id : ' + login_emp_id);
            console.log('addEmp - Emp_Name : ' + login_emp_name);
        });

        socket.on('disconnect', function() {
            console.log('Someone disconnected from namespace bucket ' + org_id);
        });
    });
    return nsp;
}

var nsp_1005 = setup_namespace(1005);
var nsp_1010 = setup_namespace(1010);

If you don't need to retain those variables,
then you could remove the variables and just leave the calls:

setup_namespace(1005);
setup_namespace(1010);


Find duplicates in List where all the numbers in between is <= than the duplicates My function is supposed to go through a given List<Integer> and find integers of same value, but only count them if the integers between them is <= the the integers that are equal:

public static int findPairs(List<Integer> arr) {
        int total = 0;

        for(int i = 0; i < arr.size(); i++) {
            for(int j = i+1; j < arr.size(); j++) {
                total += (arr.get(i) == arr.get(j)) ? 1 : 0;
                j = (arr.get(j) > arr.get(i)) ? arr.size() : j;
            }
        }
        return total;
    }

The array size can be between \$1 \le N \le 3 *10^5\$.

The numbers in the array can be between \$1 \le N \le 10^6\$.

This works, but is there any way to achieve the same thing, only a better solution performance-wise?

Firstly, note that you should use .equals not == to compare two Integer values. Also, setting j to equal arr.size() is an odd way of doing a break.

However the main problem with your algorithm is that it has worst case performance O(n^2). If you run it on a descending List like [1000000, 999999, 999998, ..., 0], it is very slow because j is never set to be arr.size().

However, for random Lists (i.e. Lists where every item is chosen using random.nextInt(1000000) for example), it is in general very quick. This is because j generally doesn't have to get very far before we break out of the j loop, so it essentially has O(n) performance for random Lists.

The fact that the performance can vary so drastically depending on the nature of the List makes it very difficult to compare two algorithms. However, for many cases the following algorithm is quicker. It only traverses the List once, keeping track of the occurrences of the numbers found. Because it uses a TreeMap I think the worst case performance is O(n log n).

private static int countPairs(List<Integer> list) {
    int total = 0;
    NavigableMap<Integer, Integer> map = new TreeMap<>();
    for (int a : list) {
        map.headMap(a).clear();
        Integer m = map.get(a);
        if (m == null)
            map.put(a, 1);
        else {
            map.put(a, m + 1);
            total += m;
        }
    }
    return total;
}


Inserting an entire form into MySQL database Please review this.

public int InsertQuery(string Query)
{
    int LastInsertedID = -1;
    MySqlCommand MySqlCmd = new MySqlCommand(Query, this.sqlConn);
    try
    {
        MySqlCmd.ExecuteNonQuery();
        LastInsertedID = int.Parse(MySqlCmd.LastInsertedId.ToString());
    }
    catch (Exception excp)
    {
        Exception myExcp = new Exception("Could not execute INSERT Query.
" + Query + "
 Error:
" + excp.Message, excp);
        throw (myExcp);
    }
    return LastInsertedID;
}

public int AddChannel(NameValueCollection FormValues)
{
    string Keys = string.Join(",",FormValues.AllKeys);

    string Values = string.Join(",", FormValues.AllKeys.Select(key => String.Format("\"{0}\"", HttpContext.Current.Server.HtmlEncode(FormValues[key]))));
    return InsertQuery("INSERT INTO channels (" + Keys + ") VALUES (" + Values + ");");
}

Don't take shortcuts. Build a proper insert statement using a parameterized query and validate each of the form inputs that they're also of the proper type (ie., not sending a string into an integer field).

Do this to protect the integrity of your data, both from improper types and from malicious (or even unintended) attacks.

See this and other related questions on SO for constructing proper parameterized queries with MySql.

More than that, you don't want your class that deals with data inserts to also interact with your UI. These concerns should be seperated. Seperate your UI code from your business logic code from your data access code. Don't let these things mingle, because your UI could change, your logic could change, and your data strategy could change, and they could change independently.


Web job logger implementation Interface for logger:

 public interface ILogger
        {
            void Log(Exception ex, string contextualMessage = "");

            void Log(string message);
        }

Interface for web job logger:

public interface IJobLogger : ILogger
{
    void SetWriter(TextWriter logWriter);
}

Web job logger:

 public class WebJobLogger : IJobLogger
    {
        private TextWriter writer;

        public WebJobLogger()
        {
            writer = TextWriter.Null;
        }

        public void SetWriter(TextWriter logWriter)
        {
            writer = logWriter;
        }

        public void Log(Exception ex, string contextualMessage = "")
        {
            writer.WriteLine("Logger: Exception thrown with message {0}, exception : {1}", contextualMessage, ex);
        }

        public void Log(string message)
        {
            writer.WriteLine("Logger: {0}", message);
        }
    }

Usage:

public async Task TestJob(
    [QueueTrigger(QueueName.StorageExcelImport)] StartImportCommand blobInfo,
    [Blob(Global.BlobContainerName + BlobReferenceName, FileAccess.Read)] Stream input,
    TextWriter logWriter)
{
    await logWriter.WriteLineAsync("Start");
    try
    {
        //Implemetation
    }
    catch (Exception ex)
    {
        //Set writer
        log.SetWriter(logWriter);
        //Log exception
        log.Log(ex);
    }
    await logWriter.WriteLineAsync(string.Format("File: '{0}' successfuly processed from blob to the database. ({1})", blobInfo.OriginalFileName, DateTime.UtcNow));
}

Dependency injection:

container.Register<ILogger, WebJobLogger>();

The problem with this is that I always have to call log.SetWriter(logWriter); before call  log.Log(ex);. If I forget that, then will not work. The whole point of this is to mock logger in unit test. What do you think?

void Log(Exception ex, string contextualMessage = "");

This isn't bad, but seems perhaps a bit redundant, since exceptions already have messages. A more idiomatic option might be to wrap the exception in another, with the custom message you want. However, if a "contextual message" conceptually serves a different purpose than the message on an exception, what you have may be fine.

void SetWriter(TextWriter logWriter);

You're right that this is problematic. What it means is that your class can be in a "not fully initialized" state. Like you mentioned, that means you have to remember to initialize it. It also means that if you ever pass it around, you can't rely on your configuration staying the same- some other code might change the logWriter without you realizing.

There's a pretty common solution to this- just pass it into the constructor. That way a WebJobLogger always has one TextWriter for its entire lifetime, which won't be changed. I'm not immediately sure which IoC container you're using, but any mainstream one will have support for constructors with parameters- you just need to make sure the dependencies are bound too.

await logWriter.WriteLineAsync("Start");

Shouldn't you be using the ILogger.Log method for this too? It's confusing to access the underlying writer from the same different place at two different abstraction levels.


Stable radix sort in-place using O(1) space I am trying to implement a stable radix sort in place with O(1) space. I've seen Wikipedia, and I notice the C implementation is using a temporary variable to hold the sorted value for each pass, and the section of "in-place" MSD radix sort implementation is quite confusing.

I took the C code and tried to implement it in C++ for the stable in-place radix sort version:

#include <iostream>
#include <vector>
#include <cstring>

using namespace std;

void radix_sort(vector<int>& v) {
    //search for maximum number
    int max_number = v[0];
    for(int i = 1;i<v.size();i++) {
        if(max_number < v[i])
            max_number = v[i];
    }

    int bucket[10]; // store first index for that digit
    int bucket_max_index[10]; // store maximum index for that digit
    int exp = 1;

    while(max_number / exp > 0) {
        memset(bucket, 0, 10 * sizeof(int));
        memset(bucket_max_index, 0, 10 * sizeof(int));
        for(int i=0;i<v.size();i++) {
            bucket[(v[i] / exp) % 10]++;
        }

        int index = v.size();
        for(int i=9;i>=0;i--) {
            index -= bucket[i];
            bucket_max_index[i] = index + bucket[i];
            bucket[i] = index;
        }

        index = 0;
        vector<int> temp(v.size());
        for(int i=0;i<v.size();i++) {
            int digit = (v[i] / exp) % 10;
            temp[bucket[digit]] = v[i];
            bucket[digit]++;
        }

        for(int i=0;i<v.size();i++) {
            v[i] = temp[i];
        }

        exp *= 10;
    }
}

EDIT:

Thanks to the comments, I checked and found out that I made a mistake. The original idea of the question is that I want to implement a stable in-place radix sort.

I thought I succeeded in implementing it when I tested it with a several test cases (one of them is this test case: {1, 3, 2, 5, 8, 2, 3, 1}) .

I changed the code to a stable radix sort with O(n) space and changed my questions to:

Is this O(kn) time ?  (with k as the maximum number of digits)
Can we improve the above code to be a stable radix sort with O(1) space ?

Yes, your radix sort appears to be correct, and runs in O(k n) time.

bucket_max_index is written to, but otherwise never used.  You can eliminate it completely.

Before the creation of the temp vector, you set index = 0.  It would be clearer to change that to assert(index == 0), since that should have been the result of v.size() - Σ bucket[i] in the previous loop.


String replacement using dictionaries I've always been bothered by the fact that there weren't any built-in functions that could replace multiple substrings of a string in Python, so I created this function. Essentially, you supply it with a string, a dictionary of keys (substrings) and values (replacements), and then a few additional options.

def keymap_replace(
        string: str,
        mappings: dict,
        lower_keys=False,
        lower_values=False,
        lower_string=False,
    ) -> str:
    """Replace parts of a string based on a dictionary.

    This function takes a string a dictionary of
    replacement mappings. For example, if I supplied
    the string "Hello world.", and the mappings
    {"H": "J", ".": "!"}, it would return "Jello world!".

    Keyword arguments:
    string       -- The string to replace characters in.
    mappings     -- A dictionary of replacement mappings.
    lower_keys   -- Whether or not to lower the keys in mappings.
    lower_values -- Whether or not to lower the values in mappings.
    lower_string -- Whether or not to lower the input string.
    """
    replaced_string = string.lower() if lower_string else string
    for character, replacement in mappings.items():
        replaced_string = replaced_string.replace(
            character.lower() if lower_keys else character,
            replacement.lower() if lower_values else replacement
        )
    return replaced_string

Here's some example usage:

print(keymap_replace(
    "Hello person. How is your day?",
    {
        "hello": "goodbye",
        "how": "what",
        "your": "that",
        "day": ""
    },
    lower_keys=False,
    lower_values=False,
    lower_string=True
))

Mostly this looks good, but I’ll make a few suggestions:

Boolean arguments in your function signature

These control whether certain strings/mappings are lowercased first. I think this behaviour should be removed from keymap_replace(). It's scope creep: it’s gone from being string replacements to more complicated string manipulations. I’d leave that sort of manipulation to the caller.

(And you could continue to add options like this: what about uppercasing the string, or reversing it, or another pattern. Rather than muddling up your function with this, I’d just take it out.)

That makes the arguments simpler, and simplifies the function definition as well.

But if you really want those parameters in there, then I’d suggest making them keyword-only. In Python 3, you can insert an asterisk as so:

def keymap_replace(
    string: str,
    mappings: dict,
    *,
    lower_keys=False,
    lower_values=False,
    lower_string=False,
) -> str:

and when the function is called, those boolean arguments have to be as keyword arguments, not positional. There can never be ambiguity in how the function is being used.

Dictionaries are unordered

There’s a possibility for ambiguity, or for substitutions to be chained. For example:

keymap_replace('Hello world', {
    'J': 'K',
    'H': 'J'
})

Should this call return "Jello world" or "Kello world"?

I think the first is more natural – I don’t expect substitutions to be chained – but when I tried the function, I got the latter. You should try to make this less confusing.

One possibility that springs to mind:

replaced_string = ''.join(mappings.get(char, char) for char in string)

Each character in the original string is subject to at most one substitution.


Best approach for converting list of nested dictionaries to a single dictionary with aggregate functions I've looked through a lot of solutions on this topic, but I have been unable to adapt my case to a performant one. Suppose I have a list of dictionaries stored as:

db_data = [
  {
    "start_time": "2020-04-20T17:55:54.000-00:00",
    "results": {
      "key_1": ["a","b","c","d"],
      "key_2": ["a","b","c","d"],
      "key_3": ["a","b","c","d"]
    }
  },
  {
    "start_time": "2020-04-20T18:32:27.000-00:00",
    "results": {
      "key_1": ["a","b","c","d"],
      "key_2": ["a","b","e","f"],
      "key_3": ["a","e","f","g"]
    }
  },
  {
    "start_time": "2020-04-21T17:55:54.000-00:00",
    "results": {
      "key_1": ["a","b","c"],
      "key_2": ["a"],
      "key_3": ["a","b","c","d"]
    }
  },
  {
    "start_time": "2020-04-21T18:32:27.000-00:00",
    "results": {
      "key_1": ["a","b","c"],
      "key_2": ["b"],
      "key_3": ["a"]
    }
  }
]

I am trying to get a data aggregation from the list output as a dictionary, with the key values of the results object as the keys of the output, and the size of the set of unique values for each date for each key.

I am attempting to aggregate the data by date value, and outputting the count of unique values for each key for each day.

Expected output is something like:

{
  "key_1": {
    "2020-04-20": 4,
    "2020-04-21": 3
  },
  "key_2": {
    "2020-04-20": 6,
    "2020-04-21": 2
  },
  "key_3": {
    "2020-04-20": 7,
    "2020-04-21": 4
  }
}

What I have tried so far is using defaultdict and loops to aggregate the data. This takes a very long time unfortunately:

from datetime import datetime
from collections import defaultdict

grouped_data = defaultdict(dict)

for item in db_data:
  group = item['start_time'].strftime('%-b %-d, %Y')
  for k, v in item['results'].items():
    if group not in grouped_data[k].keys():
      grouped_data[k][group] = []
    grouped_data[k][group] = v + grouped_data[k][group]
for k, v in grouped_data.items():
  grouped_data[k] = {x:len(set(y)) for x, y in v.items()}

print(grouped_data)

Any help or guidance is appreciated. I have read that pandas might help here, but I am not quite sure how to adapt this use case.

Try:

from collections import defaultdict
from datetime import date, datetime
from typing import DefaultDict, Set, List, Dict

DefaultSet = DefaultDict[date, Set[str]]

def default_set() -> DefaultSet:
    return defaultdict(set)

aggregated: DefaultDict[str, DefaultSet] = defaultdict(default_set)

for entry in db_data:
    start_date: date = datetime.fromisoformat(entry['start_time']).date()
    result: Dict[str, List[str]] = entry['results']
    for k, v in result.items():
        aggregated[k][start_date].update(v)

grouped_data: Dict[str, Dict[date, int]] = {
    k: {gk: len(gv) for gk, gv in group.items()}
    for k, group in aggregated.items()
}

Notes:

I do not know if this is faster, but it's certainly simpler
If you're able, maintain the output with actual date keys
Your data are better-modeled by a defaultdict of defaultdicts of sets.
I used a bunch of type hints to make sure that I'm doing the right thing.