its doing multiple nested for loop. How to reduce to 1? I am writing one logic to iterate numbers first and then additional logic to putting them into particular subset of array.

What does this code do:

Code accept first $n
It creates an array of $n number from 1 to $n
Then started converting to subset of $main_array to possible one like
['1'] [1,2] [1,2,3] [2] [2,3] [3] etc. same like this
After creating subset i am counting those some subset which satisfy condition
Condition is xyz[0] should not come in subset with abc[0] and xyz[i] should not come in subset abc[i]. Example 2 and 3 is coming subset then don't count that subset, same 1 and 4 is coming then don't count.

Here is my nested for loop:

$n = 1299;
$main_array = range(1,$n);
$counter = 0;
$count = sizeof($abc);  // $abc and $xyz size will same always.
$abc = [2,1];
$xyz = [3,4];

for ($i=0; $i <$n; $i++) {
         for($j = $i;$j < $n; $j++){
            $interval_array = array();
          for ($k = $i; $k <= $j; $k++){
                array_push($interval_array,$main_array[$k]);
          }
           $counter++;
           for ($l=0; $l < $count ; $l++) {
                //if block here to additional condition using in_array() php function. which do $counter--
                if(in_array($abc[$l], $interval_array) &&
                 in_array($xyz[$l], $interval_array)){
                     $counter--;
                     break;
                 }
             }
         }
    }

$main_array I have to create on the spot after receiving $n values.

Following is cases:

when running $n = 4 its run in 4s
when running $n = 1200 or 1299 or more than 1000 its run in 60s-123s

Expected execution timing is 9s. I reduce from 124s to 65s by removing function calling inside for loop but it's not coming to point.

Expectation of code is if I have an array like:

$array = [1,2,3];

Then subset need to generate:

[1],[1,2],[1,2,3],[2],[2,3],[3]

Any help in this?

I've seen this on SO, but only realised the biggest optimisation today.

The first optimization is to not build the $interval_array array in each iteration.  This code starts with the empty array and then in each loop just adds the next number in sequence to this array.

for ($i=0; $i <$n; $i++) {
    // Reset array in outer loop
    $interval_array = array();
    for($j = $i;$j < $n; $j++){
        // Add in new number to interval array as the key
        $interval_array[$main_array[$j]] = 0;

So the first time round it will add be [1], then on the second loop it add the next number in and will be [1,2] etc. (although they are actually [1=>0], [1=>0, 2=>0] the principle is the same)

The main difference is note that it adds the number as the key of the array and not the value.  This allows the main optimization which is when you are checking if you want to exclude particular combinations in...

        if(in_array($abc[$l], $interval_array) &&
         in_array($xyz[$l], $interval_array)){

it will look through (up to) the entire array twice to check for the values.

If instead you had the values as the key, you could use isset()...

            if(isset($interval_array[$abc[$l]]) &&
                isset($interval_array[$xyz[$l]]))   {

This is much more efficient at just checking a value exists.

The last thing I've added is that I create an array with all of the numbers you are checking against in (use array_flip() to again make these numbers the keys)...

$interest = array_flip(array_merge($abc, $xyz));

This allows me to check if the number is even something I'm interested in before running the more detailed checks...

    if ( isset($interest[$main_array[$j]] )) {

Put all of this together...

$n = 1299;
$start = microtime(true);
$main_array = range(1,$n);
$counter = 0;
$abc = [2,1];
$xyz = [3,4];
$count = sizeof($abc);  // $abc and $xyz size will same always.

// Create a combined array which lists all the numbers interested in
$interest = array_flip(array_merge($abc, $xyz));
for ($i=0; $i <$n; $i++) {
    // Reset array in outer loop
    $interval_array = array();
    for($j = $i;$j < $n; $j++){
        // Add in new number to interval array as the key
        $interval_array[$main_array[$j]] = 0;
        $counter++;
        // Only check for exclusion if interested in the number
        if ( isset($interest[$main_array[$j]] )) {
            for ($l=0; $l < $count ; $l++) {
                // if block here to additional condition using isset()
                if(isset($interval_array[$abc[$l]]) &&
                    isset($interval_array[$xyz[$l]]))   {
                        $counter--;
                        // Exit 2 levels of array as all further combinations will be excluded
                        break 2;
                }
            }
        }
    }
}

echo $counter."=".(microtime(true)-$start);

On my laptop (i7-3632QM, 8GB) the results of running this for 1299 are

original code...

841756=206.25858306885

modified code...

841756=0.34013700485229


Independent cascade model for networkx graphs Similar to this question, I implemented the independent cascade model, but for a given networkx graph (including multigraph with parallel edges). My focus is on readability, pythonic-ness, and performance (although the problem itself is NP-hard).
I cannot wait to hear your feedback!
def independent_cascade_model(G: nx.Graph, seed: list, beta: float=1.0):
    informed_nodes = {n: None for n in seed}
    updated = True

    while updated:
        for u, v, diffusion_time in G.edges(nbunch=informed_nodes, data='diffusion_time'):
            updated = False
            if informed_nodes[u] == None or informed_nodes[u] < diffusion_time:
                if random.random() < beta:
                    if v not in informed_nodes or diffusion_time < informed_nodes[v]:
                        informed_nodes[v] = diffusion_time
                        updated = True
    return informed_nodes

Correctness / Readability
I'm not sure if this is a bug, or just an unclearness of the algorithm.
    while updated:
        for ... in ...:
            updated = False
            if ...:
                if ...:
                    if ...:
                        ...
                        updated = True

If you want to loop over the edges, until no change is made, then the updated = False looks like it is in the wrong place.  As it currently stands, if the last edge processed in the for loop fails any of the 3 if conditions, the updated flag is set to False, even if a prior edge set it to True.
Wouldn't the correct implementation be:
    while updated:
        updated = False
        for ... in ...:
            if ...:
                if ...:
                    if ...:
                        ...
                        updated = True

Now, for each while loop iteration, we start by clearing the flag.  Then, if any edge results in updated = True, a change has been made and the while loop is repeated.
If the updated = False was in the correct place, then the readability of the code could be improved with comments explaining why update = True only matters for the last edge returned by the for loop.


Simple Golang TCP server I've written an itty bitty tcp server, for educational purposes.

The program outputs a file if you input the correct hash (or whatever you set the environment variable to).

I've performed tests on the application through netcat ( nc localhost 9000 ) and everything is fine, the application works as intended, both in my docker container and running "standalone".

Here's the 80 lines of code.

package main

import (
        "fmt"
        "io"
        "io/ioutil"
        "net"
        "bytes"
        "os"
        "log"
)

var OUTPUT_FILE = os.Getenv("OUTPUT_FILE")
var SECRET_HASH = os.Getenv("SECRET_HASH")

func handleError(e error) {
        if e != nil {
                log.Fatal(e)
                panic(e)
        }
}

func printFromFile(connection net.Conn) {
        /* Read file */
        data, err := ioutil.ReadFile(OUTPUT_FILE)
        handleError(err)

        /* Close connection */
        io.WriteString(connection, string(data))
        connection.Close()
}

func invalidHashMessage(connection net.Conn) {
        io.WriteString(connection, fmt.Sprint("Invalid hash\n"))
}

func handleMessage(connection net.Conn) {
        buffer := make([]byte, 1024)

        reqLen, err := connection.Read(buffer)
        if reqLen == 0 {
                connection.Close()
                return
        }

        handleError(err)

        n := bytes.Index(buffer, []byte{0})
        message := string(buffer[:n-1])

        if message != SECRET_HASH {
                invalidHashMessage(connection)
                handleMessage(connection)
        } else {
                printFromFile(connection)
        }
}

func main() {

        if OUTPUT_FILE == "" || SECRET_HASH == "" {
                fmt.Print("You need to supply output file and hash to serve application\n")
                os.Exit(1)
        }

        /* Start listener */
        ln, err := net.Listen("tcp", ":9000")
        handleError(err)
        defer ln.Close()

        for {
                conn, err := ln.Accept()
                handleError(err)

                io.WriteString(conn, "Please type unique identifier:")
                go handleMessage(conn)
        }
}

So.. my questions are.

How would you handle concurrent connections? Making it real pretty. I have a certain feeling this could be improved upon, or is this way totally ok?
Am i formatting the message correctly (the file content) so that available clients/terminals that access this little application all works the same?
When i run this application through strace i get the following message which is saying that the port is already taken (?), but the application runs just fine. Trying to understand why strace outputs this.

 accept4(3, 0xc420051c00, [112], SOCK_CLOEXEC|SOCK_NONBLOCK) = -1 EAGAIN (Resource temporarily unavailable)

main

On very first lines of main you test for provided enviroment variables and write error message to stdout if any are missing. It is more common to write error messages to stderr. stdout is for main program output (none in your case).

You can write to stderr with io.Stderr:

io.WriteString(os.Stderr, "You need to supply output file and hash to serve application.\n")
os.Exit(1)

Note: os.Exit won't call any deferred functions

You can read more about stdout and stderr usage at Stack Overflow and in this article.

In a for loop you start a routine to handle each new connection separetly, so nothing will block. But one line before the go handleMessage(conn) you write to conn with io.WriteString. This is blocking I/O operation and in some cases it may block for a long time.

I suggest you to move all interactions with new conn to a separate routine.

handleMessage

handleMessage function is an entry point for routine that handles new connections. Instead of calling conn.Close() multiple times across different functions consider using a defer statement:

function handleMessage(conn net.Conn) {
    defer conn.Close()
    // function body goes here
}

This way when handleMessage function returns or panics the conn will be closed.

buffer := make([]byte, 1024)

You allocate 1024 bytes long buffer to read provided input from conn. If selected hash value is greater then 1024 you won't be able to read it and all connections will fail with non matching hashes.

buffer := make([]byte, len(SECRET_HASH))

You call conn.Read method to read provided input. The Read documentation states only specific details, to find out more let's see io.Reader interface documentation.

Some highlights:

  Read reads up to len(p) bytes into p. It returns the number of bytes read (0 <= n <= len(p)) and any error encountered. Even if Read returns n < len(p), it may use all of p as scratch space during the call. If some data is available but not len(p) bytes, Read conventionally returns what is available instead of waiting for more.

As stated by documentation Read method can return too early. To handle this let's use io.ReadFull, which returns error only when fewer bytes were read.

_, err := io.ReadFull(conn, buffer)
if err != nil {
    // handle error
}

printFromFile

Reading whole file into memory has several pitfalls:

files larger than availiable memory won't be processed
on large number of concurent connections program will use lost of memory

To resolve this issues you may use io.Copy. It will read file in small chunks and write it directly to conn until EOF or error happens. nil error value indicates all gone fine.

func printFromFile(conn net.Conn) {
    file, err := io.Open(OUTPUT_FILE)
    if err != nil {
        // handle error
    }
    defer file.Close()

    _, err = io.Copy(conn, file)
    if err != nil {
      // handle error
    }
}

Example rewrite

package main

import(
    "io"
    "log"
    "net"
    "os"
)

var (
    outputFile = os.Getenv("OUTPUT_FILE")
    hash = os.Getenv("SECRET_HASH")
)

func handleConn(conn net.Conn) (err error) {

    defer conn.Close()

    _, err = io.WriteString(conn, "Please type unique identifier:")
    if err != nil { return }

    buf := make([]byte, len(hash))

    _, err = io.ReadFull(conn, buf)
    if err != nil { return }

    if string(buf) != hash {
        _, err = io.WriteString(conn, "Invalid hash\n")
        return
    }

    file, err := os.Open(outputFile)
    if err != nil { return }
    defer file.Close()

    _, err = io.Copy(conn, file)
    return
}

func main() {

    if len(outputFile) == 0 || len(hash) == 0 {
        io.WriteString(os.Stderr, "You need to supply output file and hash to serve application.\n")
        os.Exit(1)
    }

    ln, err := net.Listen("tcp", ":8080")
    if err != nil {
        log.Println(err)
        os.Exit(1)
    }
    defer ln.Close()

    for {
        conn, err := ln.Accept()
        if err != nil {
            log.Println(err)
            continue
        }

        go func(conn net.Conn) {
            err := handleConn(conn)
            if err != nil {
                log.Println(err)
            }
        }(conn)
    }
}

Points of interest:

simplified error handling
less functions (main, handleConn)
less memory usage
less number of imported packages


Count occurrence of words in a .txt file I'm taking an intro to programming class and although I've learned some things I didn't know before (I've been using Python for about 1.5 years) I feel like I've not progressed much in writing "beautiful" code. My professor is committed to keeping this as a general intro class, and chose Python for its "friendliness" as an initial language. I can't really tell how much I'm improving (or not) as the grades at this point seem really inflated, so I wanted to get some input from here.

We were assigned an exercise in class to take a .txt file (in our case a .txt of the Gettysburg Address) and count the number of times words occur. We were then to output our results in a neatly formatted fashion. We've been getting drilled in writing functions and are starting to work with dictionaries, so I came up with this solution with those things in mind. I want to know how I can improve my code (i.e make it more efficient, Pythonic, and embrace what Python brings to the table as a language).

from re import split

def process_line(words, word_dict):
    for word in words:
        if word in word_dict:
            word_dict[word] += 1
        else:
            word_dict[word] = 1

def process_dict(word_dict):
    temp_list = []
    for key, value in word_dict.items():
        temp_list.append((value, key))

    temp_list.sort()
    return temp_list

def format_print(input_list, reverse, word_num):
    if reverse:
        input_list.sort(reverse=True)

    print "\n", ("[Unique Words: " + str(word_num) + "]").center(35, "=")
    print "-"*35 + "\n", "%-16s %s %16s" % ("Word", "|", "Count"), "\n", "-"*35
    for count, word in input_list:
        print "%-16s %s %16d" % (word, "|", count)

def word_count(_file, max_to_min=False):
    txt = open(_file, "rU")
    word_dict = {}
    for line in txt:
        if line.replace(" ", "") != ("\n" or None):
            process_line(filter(None, split("[^a-zA-Z']+", line.lower())), word_dict)

    txt.close()
    final_list = process_dict(word_dict)
    format_print(final_list, max_to_min, len(word_dict))

word_count("Gettysburg.txt", True)

Let's take a look at word_count, which appears to be the central function:

def word_count(_file, max_to_min=False):
    txt = open(_file, "rU")
    word_dict = {}
    for line in txt:
        if line.replace(" ", "") != ("\n" or None):
            process_line(filter(None, split("[^a-zA-Z']+", line.lower())), word_dict)

    txt.close()
    final_list = process_dict(word_dict)
    format_print(final_list, max_to_min, len(word_dict))

_file is not a suitable name as according to PEP 8. It is more Pythonic to use with open(_file, "rU") as f too (known as context managers). With that, I rename _file to filename. These two points are mentioned in vnp's answer; however, I disagree with vnp's suggestion to catch the exception, as there is no need for a graceful exit. The program should crash if the file cannot be opened.

def word_count(filename, max_to_min=False):
    with open(filename, "rU") as f:
        word_dict = {}
        for line in f:
            if line.replace(" ", "") != ("\n" or None):
                process_line(filter(None, split("[^a-zA-Z']+", line.lower())), word_dict)

    final_list = process_dict(word_dict)
    format_print(final_list, max_to_min, len(word_dict))

Your function calls the process_line function:

def process_line(words, word_dict):
    for word in words:
        if word in word_dict:
            word_dict[word] += 1
        else:
            word_dict[word] = 1

There's a builtin Python class for this called Counter. It has a dictionary interface too. With that, the process_line function is no longer necessary and we can rewrite this as:

from collections import Counter
.
.
.
def word_count(filename, max_to_min=False):
    with open(filename, "rU") as f:
        counter = Counter()
        for line in f:
            if line.replace(" ", "") != ("\n" or None):
                counter.update(filter(None, split("[^a-zA-Z']+", line.lower())))

    final_list = process_dict(counter)
    format_print(final_list, max_to_min, len(counter))

Secondly, you appear to be removing all spaces from the line so as to find out if the line is just a series of whitespace and contains no actual words. This can be easily done using the strip function.

from collections import Counter
.
.
.
def word_count(filename, max_to_min=False):
    with open(filename, "rU") as f:
        counter = Counter()
        for line in f:
            line = line.strip().lower()
            if not line:
                continue
            counter.update(filter(None, split("[^a-zA-Z']+", line)))

    final_list = process_dict(counter)
    format_print(final_list, max_to_min, len(counter))

filter can be rewritten as a generator, which feels more natural to me. That also uses less parenthesis, making the code more readable.

from collections import Counter
.
.
.
def word_count(filename, max_to_min=False):
    with open(filename, "rU") as f:
        counter = Counter()
        for line in f:
            line = line.strip().lower()
            if not line:
                continue
            counter.update(x for x in split("[^a-zA-Z']+", line) if x)

    final_list = process_dict(counter)
    format_print(final_list, max_to_min, len(counter))

Now, let's take a look at process_dict.

def process_dict(word_dict):
    temp_list = []
    for key, value in word_dict.items():
        temp_list.append((value, key))

    temp_list.sort()
    return temp_list

The first few lines can be done with a lambda. The new function looks like this:

def process_dict(counter):
    temp_list = map(lambda (a, b): (b, a), counter.items())
    temp_list.sort()
    return temp_list

But was there really a need for a function on its own? In fact, I'd argue that since your function is named word_count, the function should only count words. Hence, we should just return the counter object and let the printing be handled. Also, we usually name functions as verbs, so I'll change the name to count_words.

The above change affects our whole program structure. Hence, I'll show the final code before explaining the changes I made.

from collections import Counter
from re import split

BANNER = "-" * 35

def format_print(counter, is_reverse=False):
    lst = counter.items()
    lst.sort(key=lambda (a, b): (b, a), reverse=is_reverse)
    print ("[Unique Words: %d]" % len(lst)).center(35, "=")
    print "%-16s | %16s" % ("Word", "Count")
    print BANNER
    for word, count in lst:
        print "%-16s | %16d" % (word, count)

def count_words(filename):
    counter = Counter()
    with open(filename, "rU") as f:
        for line in f:
            line = line.strip().lower()
            if not line:
                continue
            counter.update(x for x in split("[^a-zA-Z']+", line) if x)
    return counter

format_print(count_words("Gettysburg.txt"), is_reverse=False)

I've removed max_to_min=False since we no longer sort the items in count_words.

In format_print, I renamed reverse to is_reverse, assigned it to False by default and removed num_words.

Afterwards, I rewrote the function that sorts the list such that it would sort by the count, then the word, without affecting the structure of the list. This makes the later loop more intuitive.

I've also seperated print statements that had strings seperated by commas, since they were confusing. I declared BANNER as a global variable (which is alright in Python, as long as it is used as a constant). In the process, I made a few minor changes to the output; I hope you don't mind!

It took a long time, but the end result is worth it. I hope that I've managed to show you the process of clearing up your code. :)

EDIT: The code here is not yet tested; I am currently checking all of the code I posted here.

EDIT 2: Updated the fixed version.


Natural merge: mergesort that uses already sorted subarrays The code bellow is my implementation for the natural merge exercise in Robert Sedgwick's Algorithms book:

  Write a version of bottom-up mergesort that takes advantage of order
  in the array by proceeding as follows each time it needs to ﬁnd two
  arrays to merge: ﬁnd a sorted subarray (by incrementing a pointer
  until ﬁnding an entry that is smaller than its predecessor in the
  array), then ﬁnd the next, then merge them.

def merge(a, lo, mi, hi):
    aux_lo = deque(a[lo:mi])
    aux_hi = deque(a[mi:hi])

    for i in range(lo, hi):
        if len(aux_lo) and len(aux_hi):
            a[i] = aux_lo.popleft() if aux_lo[0] < aux_hi[0] else aux_hi.popleft()
        elif len(aux_lo) or len(aux_hi):
            a[i] = aux_lo.popleft() if aux_lo else aux_hi.popleft()

def find_next_stop(a, start):
    if start >= len(a)-1:
        return start

    stop = start + 1
    if a[start] < a[stop]:
        while(stop<len(a)-1 and a[stop] <= a[stop+1]):
            stop += 1
    else:
        while(stop<len(a)-1 and a[stop] >= a[stop+1]):
            stop += 1

        _stop = stop
        while(start<_stop):
            a[_stop], a[start] = a[start], a[_stop]
            start += 1
            _stop -= 1
    return stop

def natural_merge(a):
    lo = hi = 0
    while(True):
        lo = hi
        mi = find_next_stop(a, lo)
        if lo == 0 and mi == len(a) - 1:
            return
        hi = find_next_stop(a, mi)
        if mi == hi == len(a)-1:
            lo = hi = 0
            continue
        merge(a, lo, mi, hi)

I referenced this answer.

Tests and bugs

Your code looks mostly good. Before trying to go and change the code to see what can be improved. I usually try to write a few simple tests. This is even easier when you have a reference implementation that can be compared to your function. In your case, I wrote:

TESTS = [
    [],
    [0],
    [0, 0, 0],
    [0, 1, 2],
    [0, 1, 2, 3, 4, 5],
    [5, 4, 3, 2, 1, 0],
    [5, 2, 3, 1, 0, 4],
    [5, 2, 5, 3, 1, 3, 0, 4, 5],
]

for t in TESTS:
    print(t)
    ref_lst, tst_lst = list(t), list(t)
    ref_lst.sort()
    natural_merge(tst_lst)
    print(ref_lst, tst_lst)
    assert ref_lst == tst_lst

which leads to a first comment: the empty list is not handled properly and the function never returns.

Improving merge

The case elif len(aux_lo) or len(aux_hi) seems complicated as we check if aux_lo just after. Things would be clearer if we were to split in two different cases:

    if len(aux_lo) and len(aux_hi):
        a[i] = aux_lo.popleft() if aux_lo[0] < aux_hi[0] else aux_hi.popleft()
    elif len(aux_lo):
        a[i] = aux_lo.popleft()
    elif len(aux_hi):
        a[i] = aux_hi.popleft()

Also, we could reuse the fact that in a boolean context, list are considered to be true if and only if they are not empty to write:

for i in range(lo, hi):
    if aux_lo and aux_hi:
        a[i] = aux_lo.popleft() if aux_lo[0] < aux_hi[0] else aux_hi.popleft()
    elif aux_lo:
        a[i] = aux_lo.popleft()
    elif aux_hi:
        a[i] = aux_hi.popleft()

Improving find_next_stop

You don't need so many parenthesis.

You could store len(a) - 1 in a variable in order not to re-compute it every time.

The name _stop is pretty ugly. I do not have any great suggestion for an alternative but end seems okay-ish.

More tests... and more bugs

I wanted to add a few tests to verify an assumption... and I stumbled upon another issue.

Here is the corresponding test suite:

TESTS = [
    [],
    [0],
    [0, 0, 0],
    [0, 1, 2],
    [0, 1, 2, 3, 4, 5],
    [5, 4, 3, 2, 1, 0],
    [0, 1, 2, 2, 1, 0],
    [0, 1, 2, 3, 2, 1, 0],
    [5, 2, 3, 1, 0, 4],
    [5, 2, 5, 3, 1, 3, 0, 4, 5],
    [5, 2, 5, 3, 1, 3, 0, 4, 5, 3, 1, 0, 1, 5, 2, 5, 3, 1, 3, 0, 4, 5],
]

Taking into account my comments and the fix you tried to add in the question, we now have:

from collections import deque

def merge(a, lo, mi, hi):
    aux_lo = deque(a[lo:mi])
    aux_hi = deque(a[mi:hi])

    for i in range(lo, hi):
        if aux_lo and aux_hi:
            a[i] = aux_lo.popleft() if aux_lo[0] < aux_hi[0] else aux_hi.popleft()
        elif aux_lo:
            a[i] = aux_lo.popleft()
        elif aux_hi:
            a[i] = aux_hi.popleft()

def find_next_stop(a, start):
    upper = len(a) - 1
    if start >= upper:
        return start

    stop = start + 1
    if a[start] <= a[stop]:
        while stop < upper and a[stop] <= a[stop+1]:
            stop += 1
    else:
        while stop < upper and a[stop] >= a[stop+1]:
            stop += 1

        end = stop
        while start < end:
            a[end], a[start] = a[start], a[end]
            start += 1
            end -= 1
    return stop

def natural_merge(a):
    upper = len(a) - 1
    if upper <= 0:
        return
    lo = hi = 0
    while True:
        lo = hi
        mi = find_next_stop(a, lo)
        if lo == 0 and mi == upper:
            return
        hi = find_next_stop(a, mi)
        if mi == hi == upper:
            lo = hi = 0
        else:
            merge(a, lo, mi, hi)

TESTS = [
    [],
    [0],
    [0, 0, 0],
    [0, 1, 2],
    [0, 1, 2, 3, 4, 5],
    [5, 4, 3, 2, 1, 0],
    [0, 1, 2, 2, 1, 0],
    [0, 1, 2, 3, 2, 1, 0],
    [5, 2, 3, 1, 0, 4],
    [5, 2, 5, 3, 1, 3, 0, 4, 5],
    [5, 2, 5, 3, 1, 3, 0, 4, 5, 3, 1, 0, 1, 5, 2, 5, 3, 1, 3, 0, 4, 5],
]

for t in TESTS:
    print(t)
    ref_lst, tst_lst = list(t), list(t)
    ref_lst.sort()
    natural_merge(tst_lst)
    print(ref_lst, tst_lst)
    assert ref_lst == tst_lst

More improvements in find_next_stop

We have a while loop but we can compute the number of iterations we'll need: it corresponds to have the distance between start and stop. We could use a for _ in range loop to perform this. It has pros and cons but one of the key aspect is that we do not need to change start and stop, thus we don't need to copy the value in a variable.

    for k in range((1 + stop - start) // 2):
        i, j = start + k, stop - k
        a[i], a[j] = a[j], a[i]

More improvements in natural_merge

A few steps can be used to re-organise the function:

see that we can move the assignment lo = hi from the beginning of the loop to the end of the loop with no impact
realise that it is already done in the first branch of the test already so move it to the else block exclusively
see that the initialisation of hi is not required anymore
notice that the condition mi == upper is checked in 2 places (with the same value of mi and upper and that if lo != 0, we see that mi == upper directly leads to find_next(a, mi) returning upper and thus ending with mi == hi == upper and thus to the assignment lo = hi = 0.

At this stage, we have:

def natural_merge(a):
    upper = len(a) - 1
    if upper <= 0:
        return
    lo = 0
    while True:
        mi = find_next_stop(a, lo)
        if mi == upper:
            if lo == 0:
                return
            lo = hi = 0
        else:
            hi = find_next_stop(a, mi)
            merge(a, lo, mi, hi)
            lo = hi

We can go further:

the assignment hi = 0 has no effect
we can reorganise conditions

We'd get

def natural_merge(a):
    upper = len(a) - 1
    if upper <= 0:
        return
    lo = 0
    while True:
        mi = find_next_stop(a, lo)
        if mi != upper:
            hi = find_next_stop(a, mi)
            merge(a, lo, mi, hi)
            lo = hi
        elif lo == 0:
            return
        else:
            lo = 0

Interestingly, removing lo = hi leads a much more efficient code on my benchmark: the function returns much more quickly (because we always have lo == 0, we get out of the loop as soon as mi == upper) and the list is still fully sorted.

def natural_merge(a):
    upper = len(a) - 1
    if upper <= 0:
        return
    while True:
        mi = find_next_stop(a, 0)
        if mi == upper:
            return
        hi = find_next_stop(a, mi)
        merge(a, 0, mi, hi)

This looked surprising at first but thinking about it, it looks like this may be the way this algorithm is supposed to be.


Counting possible moves for a queen The challenge is this: You have a function that is passed a board length (8x8 chess board), with the coordinates of the queen's location, along with an array of obstructions (opponents). Given the location of the queen, how many moves spaces are available for her to move, total?

This is the solution I came up with, based on the matrix spiral one I did recently, and I absolutely cringe when I look at it because the code looks so reusable but every time I look at lines to reuse, there's always something preventing it.

I like this approach because it's straightforward, but it is super wet and I'm wondering if anyone has any tips for taking the same strategy and making it cleaner.

const queensMenace = (boardLength, queen, obstructions) => {
  let counter = 0;
  const x = queen[0];
  const y = queen[1];

  function isObstacle(row, col) {
    for (let o of obstructions) {
      if (o[0] === row && o[1] === col) {
        console.log(o[0] + ',' + o[1]);
        return true;
      }
    }
  }

  function checkUp(row, col) {
    if (row >= 0) {
      console.log(row);
      if (isObstacle(row,col)) {
        return;
      } else {
        counter++;
        checkUp(row-1,col);
      }
    }
  }

  function checkDown(row, col) {
    if (row < boardLength-1) {
      if (isObstacle(row,col)) {
        return;
      } else {
        counter++;
        checkDown(row+1,col);
      }
    }
  }

  function checkLeft(row, col) {
    if (col >= 0) {
      if (isObstacle(row,col)) {
        return;
      } else {
        counter++;
        checkLeft(row,col-1);
      }
    }
  }

  function checkRight(row, col) {
    if (col < boardLength-1) {
      if (isObstacle(row,col)) {
        return;
      } else {
        counter++;
        checkRight(row,col+1);
      }
    }
  }

  function checkUpLeft(row, col) {
    if (row >= 0 && col >= 0) {
      if (isObstacle(row,col)) {
        return;
      } else {
        counter++;
        checkUpLeft(row-1,col-1);
      }
    }
  }

  function checkUpRight(row, col) {
    if (row >= 0 && col < boardLength-1) {
      if (isObstacle(row,col)) {
        return;
      } else {
        counter++;
        checkUpRight(row-1,col+1);
      }
    }
  }

  function checkDownRight(row, col) {
    if (row < boardLength-1 && col < boardLength-1) {
      if (isObstacle(row,col)) {
        return;
      } else {
        counter++;
        checkDownRight(row+1,col+1);
      }
    }
  }

  function checkDownLeft(row, col) {
    if (row < boardLength-1 && col >= 0) {
      if (isObstacle(row,col)) {
        return;
      } else {
        counter++;
        checkDownRight(row+1,col-1);
      }
    }
  }

  checkUp(x-1,y);
  checkUp(x+1,y);
  checkLeft(x,y-1);
  checkRight(x,y+1);
  checkUpLeft(x-1,y-1);
  checkUpRight(x-1,y+1);
  checkDownRight(x+1,y+1);
  checkDownLeft(x+1,y-1);

  return counter;
};

queensMenace(8, [4,5], [[2,3], [7,4]]);

What about defining checkDelta?

function checkDelta(dx, dy) {
    if (0 <= x + dx && x + dx < boardSize) {
        if (0 <= y + dy && y + dy < boardSize) {
            // And so on.

This function should make your code really simple.


Validate optional input parameters with if(!empty) I have a function which accepts four parameters, which are all optional. Once they're received, it validates using if(!empty).

$subject = (empty($params['subject'])) ? '' : addslashes(trim(strip_tags($params['subject'])));
$message = (empty($params['message'])) ? '' : addslashes(htmlentities($params['message']));
$userIdArray = (!empty($params['users']) && is_array($params['users'])) ? array_map('intval', $params['users']) : array();
$messageId = (empty($params['id'])) ? null : intval($params['id']);

$validMessage = true;

$valHelper = new validation();

if(!empty($message)){
    $validMessage = ($valHelper->validateMessageTextLength($message)) ? true : false;
}

if(!empty($subject) && $validMessage){
    $validMessage = ($valHelper->validateMessageSubjectTextLength($subject)) ? true : false;
}

if($messageId && $validMessage){
    $validMessage = ($valHelper->validateMessageId($messageId)) ? true : false;
}

if ($validMessage) {

    // go to save method
    $messageDbHelper = new Message_MessageDbHelper();
    $messageDbHelper->saveMessage($subject, $message)
}  else {
    // show a common error message
}

As you can see, I'm giving a common error message and in a scenario where $message fails in validation then the rest of the if(!empty) statements become an extra overhead.

I need this code to be more optimized. Or is there a better way to do this?

EDIT-

save method is something similar to below

public function saveMessage($subject='', $message=''){
            try {
                $query = 'INSERT INTO message(subject, message) VALUES ("'.$subject.'", "'.$message.'")';
                return $this->dbAdapter->query($query)->rowCount();
            } catch (Exception $ex) {
                throw $ex;
            }
}

I am very concerned about all the mangling that you are doing with addslashes(trim(strip_tags(…))) and addslashes(htmlentities(…)).  It's almost certainly wrong, but I can't tell you what would be right without knowing exactly how those variables get used.


Longest Increasing Sub-Sequence using Dynamic Programming in Python I wrote the following function to find the longest increasing sub-sequence in an array (tuple, it doesn't work for a list since a list is mutable and it stores the results in a dictionary.). It uses Top Down dynamic programming approach.

def LIS(seq, memo={}):
    if len(seq) == 1:
        return seq

    elif seq in memo:
        return memo[seq]

    else:
        longest = (seq[-1],)
        for index in range(1, len(seq)):
            till_index = LIS(seq[:index], memo)
            till_index += (seq[-1],) if till_index[-1] < seq[-1] else ()
            longest = max(longest, till_index, key=len)
        memo[seq] = longest
        return memo[seq]

It works correctly for all the test cases I've tried. Could you please suggest some changes which can help me reduce its time and memory consumption?

It works correctly for all the test cases I've tried.

Fortunately I thought this was a different algorithm, and so tested with "dogcatcatdog".
The answer you give is "dot" where it can be "acdo" or "acdg".
This is better shown with the transformation ord(c) - 97, where yours gives (3, 14, 19) where it can be (0, 2, 3, 14) or (0, 2, 3, 6).
I'll come back to tests as my changes will not fix this.

There are a couple of ways to improve your program:

Make and use a closure.
Allow the code to work with things other than tuples.
Limit the calls to the dictionary.

I'd use a closure as then you don't need to pass the cache, or memo in your case, to the function every function call.
It makes sure that all calls to the function whilst recursing are using the same cache and there are no dodgy edge-case cache problems.
These are unlikely but it adds clarity none the less.

A simple call of tuple allows your code to work on a much larger array of data-types, all the built-in ones and any that allow the conversion to a tuple.
Your excuse that lists are mutable and so don't work is poor, as the only reason that you have to convert your code to a tuple is as you append (seq[-1],) to the output.
I'd also like to point out that your code doesn't work with strings that are immutable in the same way tuples are.

Finally if you are concerned with performance.
A simple easy way to improve usage is to use memo.get.
You use seq in memo to then use memo[seq].
Instead you can use memo.get(seq, None).
This is as whilst dictionary's are \$O(1)\$, they still take a long time to get to the correct key.
This is as they hash the object, perform a transform on that hash, then use that modified hash to extract the data.
And so can take a while.

If I were to do all the above then I'd get to:

def lin(seq):
    seq = tuple(seq)
    cache = {}
    def inner(seq):
        if len(seq) == 1:
            return seq

        ret = cache.get(seq, None)
        if ret is not None:
            return ret

        longest = (seq[-1],)
        for index in range(1, len(seq)):
            till_index = inner(seq[:index])
            till_index += (seq[-1],) if till_index[-1] < seq[-1] else ()
            longest = max(longest, till_index, key=len)
        cache[seq] = longest
        return longest
    return inner(seq)

This functions the same way as before, and it keeps most of the same logic.
It's cleaner and allows you to add more corrections to the input, without doing them every recursion.

I also tried using a generator, however it changed your programs functionality.
The code that I used is:

last = seq[-1]
sub_sequences = (
    j + (last,) if j and j[-1] < last else ()
    for j in (inner(seq[:i]) for i in range(len(seq)))
)
longest = max((last,), *sub_sequences, key=len)

This gives the correct output when you enter 'dogcatcatdog'.
But if you change the input to 2, 5, 3, 7, 11, 8, 13, 6 then your original gives the correct result, where the generator doesn't.
Here's what they return:

> dogcatcatdog
dot (original)
acdg

> 2, 5, 3, 7, 11, 8, 13, 6
2, 5, 7, 11, 13 (original)
2, 5, 6

From this we can tell that your algorithm probably isn't the best.
As it doesn't work 100% of the time, and if you make it lazy it doesn't work other times.

I'll leave you to figure out if and how you want to amend this.
But I'd point you in the way of Wikipedia if you want an efficient method to do this.
It uses a binary search to find the max, and some funky stuff to get the list.
I'd also change lo ≤ hi to lo < hi in it's version as you don't want repeating numbers.


One-dimensional Japanese puzzle I have been practicing competitive coding for a while now and am using codeforces.com. I recently encountered a problem called "One Dimensional Japanese Crossword" on that site.
The problem is as follows:

The judge provides an integer (up to 100) and then a string with that many characters being either 'B' or 'W'. We need to output how many groups of 'B' there are and how many 'B's there are in each group. For example:

Input:

6
BBWBWB

Output:

3
2 1 1

After lots of tries and lots of fails, I was finally able to get the code accepted by the judge. However, for some reason I feel that my code is really inefficient and there might be an easier and better way to solve this problem.

#include<iostream>
#include<vector>
using namespace std;

int main()
{
    int n;
    cin >> n;
    string s;
    cin >> s;

    int bGroups = 0;
    int wGroups = 0;

    char initChar = s[0];

    for (int i = 1; i < s.length(); i++)
    {
        if (initChar == 'B')
        {
            if (s[i] != initChar)
            {
                bGroups++;
                initChar = 'W';
            }
        }
        else if (initChar == 'W')
        {
            if (s[i] != initChar)
            {
                wGroups++;
                initChar = 'B';
            }
        }

    }

    if (s[n - 1] == 'B')
    {
        bGroups++;
    }
    else if (s[n - 1] == 'W')
    {
        wGroups++;
    }

    char init = s[0];
    vector<int>grpSize(bGroups);
    int counter = 0;
    int i = 0;
    while (counter < bGroups)
    {
        if (init == 'B')
        {
            while (init == 'B')
            {
                grpSize[counter]++;
                i++;
                init = s[i];
            }
            counter++;
            while (init == 'W')
            {
                i++;
                init = s[i];
            }
        }
        else
        {
            while (init == 'W')
            {
                i++;
                init = s[i];
            }
            while (init == 'B')
            {
                grpSize[counter]++;
                i++;
                init = s[i];
            }
            counter++;
        }
    }

    cout << bGroups << endl;
    for (int i = 0; i < bGroups; i++)
    {
        cout << grpSize[i] << " ";
    }
}

So basically, in the first pass over the string, I count how many groups of 'B' and 'W' there are. Then I create an int vector to store the size of each 'B' group. Then on the second pass I fill in the values into the vector and then output the results.

Code with iterators and standard algorithms, and you'll see your coding style improve dramatically:

finding the beginning of a group of Bs can be done with std::find: auto b = std::find(first, last, 'B');
finding the end of a group of Bs can also be done with std::find: auto e = std::find(b, last, 'W');
computing the distance between the two is the job of std::distance: auto nb_bs = std::distance(b, e).

So combining all that we get:

#include <vector>
#include <algorithm>

template <typename Iterator>
auto groups_of_bs(Iterator first, Iterator last) {
    std::vector<int> result;
    while (true) {
        first = std::find(first, last, 'B');
        if (first == last) break;
        auto w = std::find(std::next(first), last, 'W');
        result.push_back(std::distance(first, w));
        if (w == last) break;
        first = std::next(w);
    }
    return result;
}

Now the only thing left is to display the size of the vector, and then its elements:

#include <string>
#include <iostream>

int main() {
    std::string test{"BBWBWB"};
    auto res = groups_of_bs(test.begin(), test.end());
    std::cout << res.size() << '\n';
    for (auto i : res) std::cout << i << ' ';
}


Implementation of Trie data structure accommodating varying number of children I've used the Trie data structure to solve the following problem:

  Given an array of words, print all anagrams together. For example, if the given array is {"cat", "dog", "tac", "god", "act"}, then output may be "cat tac act dog god".

The following is my C++ implementation which can accommodate varying number of children.

#include <iostream>
#include <vector>
#include <string>
#include <list>
#include <algorithm>
using namespace std;

struct Trie_node_base
{
    vector<Trie_node_base*> child;
    int num_of_child;

    Trie_node_base(int n)
    {
        num_of_child = n;
        for(int i = 0; i < n; ++i)
            child.push_back(NULL);
    }
};

struct Trie_node : public Trie_node_base
{
    bool is_word;
    vector<vector<string>::const_iterator> head;

    Trie_node(int num):Trie_node_base(num), is_word(false){}
};

class Trie
{
public:
    Trie(int n=26)
    {
        root = new Trie_node(n);
    }
    ~Trie()
    {
        destroy(root);
    }

    void insert_trie_node(const string& w, vector<string>::const_iterator ind);
    Trie_node* get_root_node()
    {
        return root;
    }

private:
    Trie_node* root;
    void destroy(Trie_node* root);
};

void Trie::insert_trie_node(const string& w, vector<string>::const_iterator ind)
{
    Trie_node* r = root;

    string::const_iterator runner = w.begin();
    for(; runner!=w.end(); ++runner)
    {
        char cur_char = *runner;
        int i = cur_char - 'a';

        if(r->child[i] == NULL)
        {
            r->child[i] = new Trie_node(r->num_of_child);
        }
        r = (Trie_node*)r->child[i];
    }

    if(!r->is_word)
        r->is_word = true;
    r->head.push_back(ind);
}

void Trie::destroy(Trie_node* root)
{
    vector<Trie_node_base*>::iterator it = (root->child).begin();

    for(; it != (root->child).end(); ++it)
    {
        if(*it != NULL)
            destroy((Trie_node*)(*it));
    }

    delete root;
}

void traversal_trie(const vector<string>& word_arr, Trie_node* r)
{
    size_t i;

    if(r->is_word)
    {
        vector<vector<string>::const_iterator>::iterator it = (r->head).begin();
        for(; it != (r->head).end(); ++it)
        {
            cout << *(*it) << endl;
        }
    }

    for(i = 0; i < r->num_of_child; ++i)
    {
        if(r->child[i] != NULL)
            traversal_trie(word_arr, (Trie_node*)r->child[i]);
    }
}

void cluster_anagrams(const vector<string>& word_arr, size_t size)
{
    Trie* trie = new Trie(26);

    vector<string>::const_iterator it = word_arr.begin();
    for(; it != word_arr.end(); ++it)
    {
        string cur_str = *it;
        sort(cur_str.begin(), cur_str.end());
        trie->insert_trie_node(cur_str, it);
    }
    traversal_trie(word_arr, trie->get_root_node());
}

int main()
{
    const char* word_arr[] = {"cat", "dog", "tac", "god", "act", "gdo"};
    size_t size = sizeof(word_arr) / sizeof(word_arr[0]);
    vector<string> word_arr1(word_arr, word_arr+size);

    cluster_anagrams(word_arr1, size);

    system("pause");
    return 0;
}

Your code does several things more complex than required. In no particular order:

What purpose does Trie_node_base serve? You don’t need a base class here since you only inherit from it once.
Worse, your base class doesn’t have a virtual destructor.
To create an array of size n, you use a loop and push_back. Much easier would be to use the proper initialiser:

Trie_node_base(int n) :
    num_of_child(n) ,
    child(n)
    { }

In fact, you generally fail to use initialisers, and instead re-assign values inside the constructor.
Consider proper naming. In the above code snippet, it should really be number_of_children and children (plural!).
The num_of_child member is redundant – the same information is stored in the vector.
The tree node root doesn’t need to, and shouldn’t be, a pointer.
In general, your use of pointers and manual memory management makes the code vastly more complex and brittle (and requires the existence of the destroy method). If you need to use pointers, use smart pointers (std::unique_ptr in this case).
Better yet, omit pointers entirely. They are utterly unnecessary here. Unfortunately though, you cannot use std::vector with an incomplete class (the standard screwed up here). But you can (and should!) use boost::container::vector. Thus your tree node implementation becomes:

struct Trie_node
{
    boost::container::vector<Trie_node> children;
    bool is_word;

    Trie_node(int n)
        : children(n)
        , is_word(false)
    { }
};

… There’s more but these are the essential points that will help reduce code complexity.


Shortest distance between two words in the list Given a list of words and two words word1 and word2, return the shortest distance between these two words in the list.

Example: given words = ["practice","makes","perfect","coding","makes"]

Input: word1 = "coding", word2 = "practice"
Output: 3

We assume word1 does not equal to word2, and both are in the list. List can be big as length 1000.

public int shortestDistance(String[]words, String word1, String word2){
 int temp = 0;

 if(word1.equals(words[words.length - 1])) && word2.equals(words[words.length - 2])||
    word2.equals(words[words.length - 1])) && word1.equals(words[words.length - 2]){
   return 1;
 }

 for(int i = 0; i < words.length; i++){
   if(word1.equals(words[I]) || word2.equals(words[I])) && (temp == 0){
     temp++
   }else if(word1.equals(words[I]) || word2.equals(words[I])){
     return temp;
   }else if(temp > 0){
     temp++;
   }
 }
    return temp;
}

Care about final

Your parameters are never modified, so it is good practice to mark them final. Probably the method too, unless you expect it can be overridden.

public final int shortestDistance(final String[] words, final String word1, final String word2)

Compare by indices

Instead of your for loop, you can use the indices of the elements.

public final int shortestDistance(final String[] words, final String word1, final String word2) {
   final List<String> wordsList = Arrays.asList(words);

  final int word1Index = wordsList.indexOf(word1);
  final int word2Index = wordsList.indexOf(word2);

  return Math.abs(word2Index - word1Index);
}

There could be duplicates

We can solve this problem by finding the index of each occurrence. The final product:

public final int shortestDistance(final String[] words, final String word1, final String word2) {
  final List<Integer> word1Indices = IntStream.range(0, words.length).boxed().filter(i -> words[i].equals(word1)).toList();
  final List<Integer> word2Indices = IntStream.range(0, words.length).boxed().filter(i -> words[i].equals(word2)).toList();

  int result = Integer.MAX_VALUE;

  // Better way to do this but I'm tired.
  for(final Integer word1Index : word1Indices) {
    for(final Integer word2Index : word2Indices) result = Math.min(result, Math.abs(word2Index - word1Index));
  }

  return result;
}


A caesar cipher code This is code for a caesar cipher that I wrote, I would love for anyone who can to offer any improvements/corrections/more efficient methods they can think of. Thanks.
alphabet=["A","B","C","D","E","F","G","H","I","J","K","L","M","N","O","P","Q","R","S","T","U","V","W","X","Y","Z"," ","1","2","3","4","5","6","7","8","9","0",",","."]
enc_text=[]
dec_text=[]

def number_check(a):
  valid=False
  while valid==False:
    try:
      number=int(a)
      valid=True
    except ValueError:
      print("Invalid entry, You must enter a number.")
      a=input("please enter a number.\n")
  return number

def encrypt():
  for x in org_text:
    index=alphabet.index(x)
    enc_letter=(index+int(enc_key))%39
    new_let=alphabet[enc_letter]
    enc_text.append(new_let)
  return enc_text

def decipher():
  for x in message:
    index=alphabet.index(x)
    dec_letter=(index-int(enc_key))%39
    new_let=alphabet[dec_letter]
    dec_text.append(new_let)
  return dec_text

enc_key=number_check(input("Please enter your encryption key:\n"))
org_text=input("please enter the message you would like encrypted:\n").upper()
message=encrypt()
message1="".join(message)
print(message1)
decode=decipher()
decode1="".join(decode)
print(decode1)

PEP-8
White space!  Your code needs more white space!  The Style Guide for Python Code enumerates many coding conventions Python programs should follow.  One is binary operators (like =, ==, +, % should have one space character before and after the operator.
alphabet is a constant.  It never changes.  PEP-8 would recommend you changing the name to ALPHABET to indicate it is a constant.
List of characters
A string is essentially just a list of characters.  Moreover, it will behave exactly as if it is list of characters.
alphabet=["A","B","C","D","E","F","G","H","I","J","K","L","M","N","O","P","Q","R","S","T","U","V","W","X","Y","Z"," ","1","2","3","4","5","6","7","8","9","0",",","."]

could be replaced with the much less verbose ...
alphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZ 1234567890,."

... with no other changes required in your program.
One source of truth
How many characters are in your alphabet?  Did you count?  39?  Are you sure?  Great!  What if you added an apostrophe to the alphabet.  The 39 would become 40, and you'd have to change both occurrences of 39 in your code to 40.
Why are we counting and risking making a mistake?  Computers are much better at counting!
ALPHABET_LENGTH = len(alphabet)
...
    enc_letter = (index + int(enc_key)) % ALPHABET_LENGTH
...
    dec_letter = (index - int(enc_key)) % ALPHABET_LENGTH

Now if you decide additional letters should be included in the alphabet, you just add them; no other change would be required.
Global variables
alphabet is a global constant.  It never changes.  In contrast to that, enc_text, dec_text, org_text and message are global variables.  They are mutated by the program.
Avoid global variables.  They make the code harder to understand, and harder to maintain.
Consider encrypt().  You call it and org_text is processed, encrypted and added to the enc_text list.  What happens if you change org_text and call encrypt() again?  The new text gets added to enc_text.
Was that expected?
If enc_text was removed from global scope, and defined inside the encrypt() function, this unexpected behaviour would not happen.  Same with dec_text.
Additionally, global variables should not be used to pass the text to be encrypted/decrypted into the functions; these should be passed as parameters.
Bug
If you encrypt the message "This fails!", the program crashes with the exception ValueError: '!' is not in list.
This is another reason to use an alphabet string instead of a list of characters: strings have another method, .find(), which will return -1 if the target isn't found.  A list has no such method, forcing you to use try: ... except: ... to catch the exception.


Return most recent DateTime C# Entity Framework Job -> Many Quotes
Quote -> EmailLogList
EmailLogList -> Many EmailLogs

I want to return a CreatedDate from an EmailLog which has been created most recently - using a Job.Guid.

Is this the best way possible?

public DateTime? GetMostRecentEmailLog(Guid jobGuid)
{
    using (DbContext dbContext = new DbContext())
    {
        IEnumerable<Guid?> logLists = dbContext
            .Jobs
            .Include(j => j.Quotes)
            .First(j => j.Guid == jobGuid)
            .Quotes
            .Where(q => q.EmailLogListGuid != null)
            .Select(q => q.EmailLogListGuid);

        return dbContext
            .EmailLogs
            .OrderByDescending(e => e.CreatedDate)
            .Where(e => logLists.Contains(e.EmailLogListGuid))
            .Select(e => e.CreatedDate)
            .FirstOrDefault();
    }
}

This frequently used pattern...

entities.First().Property

... is hardly ever an economical way to do querying. It fetches a complete entity from the database of which subsequently all but one property is discarded. Or, when Property is a lazy loaded collection, everything is discarded.

In general a better way is...

entities.Where(e => e.Id == id).Select(e => e.Property).First()

... because this only gets the single property from the database.

Or, if Property is a collection,...

entities.Where(e => e.Id == id)
        .SelectMany(e => e.Property)
        .Select(p => p.Property1)

Applying this would turn the first statement into...

IEnumerable<Guid> logLists = dbContext.Jobs
                                      .Where(j => j.Guid == jobGuid)
                                      .SelectMany(j => j.Quotes)
                                      .Where(q => q.EmailLogListGuid != null)
                                      .Select(q => q.EmailLogListGuid.Value);

The second part is OK. Getting the most recent item from a collection always requires some ordering that defines "most recent".

Note that using the improved form, EF will translate this into one query, because logList isn't materialized yet. It is an expression tree1 that can be merged with the second expression. If you want, you can create one statement in LINQ as well:

return dbContext.Jobs
                .Where(j => j.Guid == jobGuid)
                .SelectMany(j => j.Quotes)
                .SelectMany(q => dbContext.EmailLogs
                    .Where(e => q.EmailLogListGuid == e.EmailLogListGuid))
                .OrderByDescending(e => e.CreatedDate)
                .Select(e => e.CreatedDate)
                .FirstOrDefault();

Or, if Quote has a navigation property EmailLogs (recommended):

return dbContext.Jobs
                .Where(j => j.Guid == jobGuid)
                .SelectMany(j => j.Quotes)
                .SelectMany(q => EmailLogs)
                .OrderByDescending(e => e.CreatedDate)
                .Select(e => e.CreatedDate)
                .FirstOrDefault();

1 Even though you cast the expression tree to IEnumerable, it's still an IQueryable containing an expression.


Number Of Matching Elements In Two Lists I have many sets of 2 strings. I'm trying to determine the number of matching elements in these 2 strings. The rules are if the strings share a common letter, that's a point, order does matter, but each letter in the first string can only match one of the letters in the second string. So in the strings 'aaaab', 'acccc', only 1 point is awarded because there is only one 'a' to match in the second string. Here are a few examples:

aaabb  bbaaa  5
aabbb  bbbaa  5
aaabb  aabbb  4
aaabb  ccaaa  3
aaaaa  bbbbb  0
ababa  babab  4
aabcc  babaf  3
abcde  abfgh  2
bacde  abdgh  3

Hopefully that gets across how it works.

Here is the most efficient code I've been able to come up with, but its horribly convoluted. I hoping someone could think of something better.

def Score(guess, solution):
    guess = list(guess)
    solution = list(solution)
    c = 0
    for g in guess:
        if g in solution and g != "_":
            c += 1
            solution[solution.index(g)] = "_"
    return c

Surely this isn't the best way to do this, but I haven't been able to figure anything else out. I tried creating an algorithm with Counter and doing guess&solution, which worked, but ended up being way slower. Anyone have any ideas?

As you have updated your examples, you can write some code to ensure the code we are about to write works properly :

def main():
    """Main function"""
    assert Score('aaabb', 'bbaaa') == 5
    assert Score('aabbb', 'bbbaa') == 5
    assert Score('aaabb', 'aabbb') == 4
    assert Score('aaabb', 'ccaaa') == 3
    assert Score('aaaaa', 'bbbbb') == 0
    assert Score('ababa', 'babab') == 4
    assert Score('aabcc', 'babaf') == 3
    assert Score('abcde', 'abfgh') == 2
    assert Score('bacde', 'abdgh') == 3

Now, the main issue in your code is that we are performing complicated (dirty?) string logic and checking many times if some letter is in a list. The real solution is to count letters in both string. Each letter will count for n points where n is the minimum between the number of times it appears in string 1 and the number of times it appears in string 2.

Using Counter, you can write this :

from collections import Counter

def Score(guess, solution):
    g = Counter(guess)
    s = Counter(solution)
    n = 0
    for l,c in g.iteritems():
        n+=min(c, s[l])
    return n

Abusing generator expressions, you can also write this :

def Score(s1, s2):
    c = Counter(s2)
    return sum(min(n, c[l]) for l,n in Counter(s1).iteritems())


Lua OOP and classically-styled prototypal inheritance I want to do some object-oriented programming in Lua, and I decided on something like this:

local B = {} -- in real life there is other stuff in B

B.Object = {

  constructor = function() end,

  extend = function(this, that, meta)
    if not that then that = {} end
    if not meta then meta = {} end
    meta.__index = this
    setmetatable(that, meta)
    return that
  end,

  isa = function(this, that)
    for meta in function() return getmetatable(this) end do
      if this == that then return true end
      this = meta.__index
    end
    return this == that
  end,

  new = function(this, ...)
    local that = this:extend()
    that:constructor(...)
    return that
  end,

}

All objects (including "class-like objects") extend B.Object, directly or indirectly. For example:

-- Request handler base

B.RequestHandler = B.Object:extend()

-- override these
function B.RequestHandler:accept() error "not implemented" end
function B.RequestHandler:process() error "not implemented" end

-- URL-encoded POST handler

B.PostHandler_URLEncoded = B.RequestHandler:extend()

function B.PostHandler_URLEncoded:accept()
  return self.method == "POST" and
         self.content_type == "application/x-www-form-urlencoded"
end

function B.PostHandler_URLEncoded:process()
  -- some code
end

-- Multipart POST handler

B.PostHandler_Multipart = B.RequestHandler:extend()
-- etc.

It might be used something like this:

B.request_handlers = {
  GetHandler:new(),
  URLEncodedPostHandler:new(),
  MultipartPostHandler:new(),
}

B.handle_request = function()
  for k, v in ipairs(B.request_handlers) do
    if v:accept() then
      return v:process()
    end
  end
  error "request not handled"
end

As far as I know, this is pretty much the normal way to handle inheritance in Lua. Did I miss anything? Is there anything that needs improvement?

Example Usage

The example that you provide does not really help to explain how the code should work. I trust that it does work, but the variables don't exactly match up.

For example, this code here:

B.request_handlers = {
  GetHandler:new(),
  URLEncodedPostHandler:new(),
  MultipartPostHandler:new(),
}

What is B.request_handlers before this? You define the "class" like so in the former code block: B.RequestHandler = B.Object:extend() but now it is lower case and has an underscore.  The same for the three calls inside those brackets.  What does GetHandler call? I don't see a matching method anywhere.  It's all just a little confusing.

Lack of Comments

The first code block is the most important one, because that is where you are establishing the actual Lua inheritance.  The problem with the code it that it is very dense, and there is no explanation whatsoever about how it is expected to be used.

For example, you define the extend method like this:

extend = function(this, that, meta)
  if not that then that = {} end
  if not meta then meta = {} end
  meta.__index = this
  setmetatable(that, meta)
  return that
end,

But then in your example you call it like this:

B.RequestHandler = B.Object:extend()

After reading through the method, I can understand that if extend() is called without any arguments, the arguments will be filled in inside the method. I think that it would be very helpful to have a comment explaining the valid ways of calling the extend method. In most languages, when a method has arguments they must be supplied or the compiler will complain.  Since Lua is a dynamic language, you won't get this checking, and so I believe the comments explaining how to use your OOP implementation become even more important.

This, That, and the other

This code is very hard to understand:

isa = function(this, that)
  for meta in function() return getmetatable(this) end do
    if this == that then return true end
    this = meta.__index
  end
  return this == that
end,

new = function(this, ...)
  local that = this:extend()
  that:constructor(...)
  return that
end,

All of the this, that, then, this == that, return that, etc etc just twists my brain up into a knot.  If I read the code very carefully line by line, I can eventually puzzle out what it does, but I think that this is a failure of the code.

It's true, you probably do not intend anyone to read your inheritance code.  Rather, once you have set it up, it will just work and allow someone using the code to extend objects in a classical OOP way.  However, I still think that by using better variable names, some comments, and a few more lines of code, it could be written in such a way that it could be understood immediately when reading it.

Also consider that someone who is not a native English speaker may struggle with the difference between this and that, and the then keyword does not help the situation.

Something Good

I like this code here:

-- override these
function B.RequestHandler:accept() error "not implemented" end
function B.RequestHandler:process() error "not implemented" end

The comment makes it very clear that these are the methods that should be overridden, and the string that will be returned if they are not will make it very clear what has gone wrong.


User needs to type in the password to send an email User has three attempts (locks you out for 10 seconds if you fail) to log in to send an email. Afterwards, you need to insert your username and password to send your message to someone of your will.

Any improvements that I can make?

import smtplib
import time

def send_mail() :
    try :
        content = input("Type in your content :")
        mail = smtplib.SMTP('smtp.gmail.com', 587)
        mail.ehlo()
        mail.starttls()

        username = input("Username :")
        password = input("Password :")
        mail.login(username,password)
        receiver = input("Receiver : ")
        mail.sendmail(username, receiver, content)
        mail.close()

    except smtplib.SMTPException :
        print("Error. Mail was not sent.")
    else :
        print("Success. Mail sent.")

if __name__ == '__main__' :
    password = "password123"
    num_of_tries = 3
    input_password = input("Enter password")

    '3 attempts before locking you out for 10 seconds.'
    while password != input_password :
        num_of_tries -= 1
        if num_of_tries == 0 :
            print("Try again in 10 seconds")
            time.sleep(10)
            num_of_tries = 3

        input_password = input(str(num_of_tries) + " Attempts Remaining. Password : ")

    if password == input_password :
        print("Success.")
        send_mail()

def send_mail() :

Per PEP8, you wouldn't typically include a space before those colons. You should run your code through a linter.

password = input("Password :")

This is a big no-no. Never expose a user's password content on the screen if you can avoid it. Instead, use the built-in getpass which attempts to mask your input.

mail.close()

You should avoid doing this explicitly, and instead use mail in a with statement.

'3 attempts before locking you out for 10 seconds.'

This line has no effect. Perhaps you meant to write print here. Also, don't hard-code the 3 in the string; use num_of_tries.


Extract unique words from given text and group by letter count The task is for training go-lang. The idea is to extract unique words sorted and grouped by length. Might be useful in learning new words. The program uses command line argument assuming it's a file to read; reads from stdin if no arguments were given.

package main

import (
    "bufio"
    "fmt"
    "os"
    "regexp"
    "sort"
    "strings"
)

type words = map[string]struct{}
type groupedWords = map[int]words

// Checks for fatal errors.
// If non nil error passed program will be terminated
func check(e error) {
    if e != nil {
        panic(e)
    }
}

// Reads data from bufio.Reader and returns
// a map of grouped unique words: key is letters count, value is map of words
func getGroupedWords(reader *bufio.Reader) groupedWords {

    groupedWords := make(groupedWords)

    re, err := regexp.Compile("[^a-zA-Z]")
    check(err)
    for {
        str, err := reader.ReadString(' ')
        str = strings.ToLower(strings.TrimSpace(str))
        if !re.MatchString(str) {
            val, exists := groupedWords[len(str)]
            if !exists {
                groupedWords[len(str)] = words{str: struct{}{}}
            } else {
                val[str] = struct{}{}
            }

        }

        if err != nil {
            break
        }
    }

    delete(groupedWords, 0)
    return groupedWords

}

// Extracts sorted slice of keys from words
func getSortedKeysWord(w words) []string {
    list := make([]string, len(w))

    i := 0
    for word := range w {
        list[i] = word
        i++
    }

    sort.Strings(list)
    return list
}

// Extracts sorted slice of letters count from groupedWords
func getSortedKeysGroupedWord(gw groupedWords) []int {
    list := make([]int, len(gw))

    i := 0
    for lettersCnt := range gw {
        list[i] = lettersCnt
        i++
    }

    sort.Ints(list)
    return list
}

func main() {
    var reader *bufio.Reader
    args := os.Args
    if len(args) == 1 {
        reader = bufio.NewReader(os.Stdin)
    } else {
        file, err := os.Open(args[1])
        check(err)
        reader = bufio.NewReader(file)
    }

    groupedWords := getGroupedWords(reader)

    lettersCntList := getSortedKeysGroupedWord(groupedWords)
    for _, lettersCnt := range lettersCntList {
        list := getSortedKeysWord(groupedWords[lettersCnt])
        fmt.Print(lettersCnt, "(", len(list), "):\t")
        for _, word := range list {
            fmt.Print(word, " ")
        }

        fmt.Println()
    }

}

usage

echo "The unary notation can be abbreviated by introducing different symbols for certain new values. " | go run extract_words.go

should give

2(2):   be by
3(3):   can for new
5(1):   unary
7(2):   certain symbols
8(1):   notation
9(1):   different
11(2):  abbreviated introducing

If code is not correct it is not useful and it may be harmful.

I reviewed the specification for your program,

  The idea is to extract unique words sorted and grouped by length.

the sample input,

echo "The unary notation can be abbreviated by introducing different symbols for certain new values. " | go run extract_words.go

and the sample output.

2(2):   be by
3(3):   can for new
5(1):   unary
7(2):   certain symbols
8(1):   notation
9(1):   different
11(2):  abbreviated introducing

I then ran your program

2(2):   be by
3(4):   can for new the
5(1):   unary
7(2):   certain symbols
8(1):   notation
9(1):   different
11(2):  abbreviated introducing

felix posted an answer, so I ran his program

2(2):   be by
3(4):   can for new the
5(1):   unary

Three different outputs! All wrong!

None of the earlier outputs is correct. I wrote a program to find the correct output.

Output:

2(2):   be by
3(4):   can for new the
5(1):   unary
6(1):   values
7(2):   certain symbols
8(1):   notation
9(1):   different
11(2):  abbreviated introducing

wordsbylen.go:

package main

import (
    "bufio"
    "bytes"
    "fmt"
    "io"
    "os"
    "sort"
    "strings"
    "unicode"
)

func wordsByLen(r io.Reader) ([][]string, error) {
    unique := make(map[string]bool)
    scan := bufio.NewScanner(r)
    for scan.Scan() {
        fields := bytes.FieldsFunc(
            scan.Bytes(),
            func(r rune) bool {
                return !unicode.IsLetter(r)
            },
        )
        for _, field := range fields {
            unique[string(bytes.ToLower(field))] = true
        }
    }
    if err := scan.Err(); err != nil {
        return nil, err
    }

    words := make([]string, 0, len(unique))
    for word := range unique {
        words = append(words, word)
    }

    sort.Slice(words,
        func(i, j int) bool {
            if len(words[i]) < len(words[j]) {
                return true
            }
            if len(words[i]) == len(words[j]) {
                return words[i] < words[j]
            }
            return false
        },
    )

    var byLen [][]string
    for i, j := 0, 1; j <= len(words); j++ {
        if j == len(words) || len(words[j]) != len(words[i]) {
            byLen = append(byLen, words[i:j])
            i = j
        }
    }
    return byLen, nil
}

func main() {
    f := os.Stdin
    if len(os.Args) == 2 {
        var err error
        f, err = os.Open(os.Args[1])
        if err != nil {
            fmt.Fprintln(os.Stderr, err)
            return
        }
        defer f.Close()
    }

    byLen, err := wordsByLen(f)
    if err != nil {
        fmt.Fprintln(os.Stderr, err)
        return
    }
    for _, words := range byLen {
        if len(words) > 0 {
            list := strings.Join(words, " ")
            fmt.Printf("%d(%d): \t%s\n", len(words[0]), len(words), list)
        }
    }
}

Go programmers usually like to write efficient code. I ran the code on Shakespeare:

The Complete Works of William Shakespeare by William Shakespeare
http://www.gutenberg.org/files/100/100-0.txt

For my program (correct results)

real    0m0.369s
user    0m0.356s
sys     0m0.012s

For your program (incorrect results)

real    0m0.675s
user    0m0.630s
sys     0m0.040s

For felix's program (incorrect results)

real    1m58.704s
user    1m58.600s
sys     0m0.140s

Since felix's program takes hundreds of times longer than other programs, it is unlikely to be useful.

You and felix have a narrow view of words, limiting them to ASCII letters

regexp.Compile("[^a-zA-Z]")

There is a larger world out there: The Unicode Consortium. For example, unicode.IsLetter()

The Go garbage collector manages memory, it does not manage operating system resources like files. To avoid crashing with too many open files, get into the habit of releasing OS file resources by closing files when no longer needed. See my use of defer f.Close().

Two key goals of the Go programming language are simplicity and readability.

Your text algorithms favor regular expressions, which can be complicated and error prone. See Mastering Regular Expressions, 3rd Edition, Jeffrey Friedl, ISBN-13: 978-0596528126.

Your data structures are complicated, favoring maps (dictionaries). For example, map[int]map[string]struct{}. I used simple structures: map[string]bool, []string, and, at the end, [][]string.

Your control structures look complicated. I used a simple, sequential control structure: unique words -> word list -> sort word list -> group word list -> print grouped word list.


File manager project (creates and prints file) I just finished my very simple and beginner-friendly C project and wanted to hear your thoughts and opinions about ways to improve it or something.

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

void getFile();
void createFile();

int main()
{
    createFile();
    getFile();
    return 0;
}

void createFile()
{
    char userfilename[40];
    char filecontent[1000];
    FILE *filePointer;

    printf("How do you want to name your file? ");
    scanf("%s", userfilename);
    filePointer = fopen(userfilename, "w");

    printf("Enter file content: ");
    scanf ("%999[^\n]", filecontent);
    getchar();
    fgets (&filecontent, 1000, stdin);
    fputs(filecontent, filePointer);

    fclose(filePointer);
    printf("Written file successful.\n");
}

void getFile()
{
    int c;
    char fileName[100];
    FILE *filePointer;

    printf("\nEnter filename: ");
    gets(fileName);
    printf("\n");

    filePointer = fopen(fileName, "r");
    if(filePointer == NULL)
    {
        printf("File not found.\n");
    }
    else
    {
        int count = 0;
        printf("File contents:\n");
        while ((c = getc(filePointer)) != EOF)
        {
            putchar(c);
            if(c == ' ')
            {
                count++;
            }
        }
        printf("\n");
        printf("Number of words: %d", count + 1);
        fclose(filePointer);
    }
}

Nice first question. It can definitely be expanded on for additional learning in the future.

Improve Error Handling

Both the functions createFile() and getFile() could be changed to return int. This would allow the program to exit with the correct status.

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

int createFile();                 // return EXIT_SUCCESS or EXIT_FAILURE
int getFile();                    // return EXIT_SUCCESS or EXIT_FAILURE

int main()
{
    int status = EXIT_SUCCESS;    // EXIT_SUCCESS and EXIT_FAILURE are defined in stdlib.h

    status = createFile();
    if (status != EXIT_SUCCESS)
    {
        return status;
    }

    status = getFile();

    return status;
}

In getFile() the variable filePointer is tested after fopen(). This is a good practice. In createFile() the variable filePointer is not tested, this is a bad practice. The C library function fopen() for write can fail for a number of reasons:

File exists and is protected.
File exists and it is a directory.
The program doesn't have sufficient privilege to write to the target directory.

There are more ways it can fail, this is a partial list. See full list of possible error codes. The C library function perror() can be used to report program error in the case of
I/O failures or the include file errno.h can be included and the global variable errno will contain
the error code for better error messages as shown in this stackoverflow question.

These same error messages can be used to improve the error reporting in getFile() as well.

The existing error message in getFile() might be improved by adding the file name to the error message.

fprintf(stderr, "File %s not found.\n", fileName);

Error messages should generally be reported using fprintf(stderr, FMT); rather than reporting the error
to stdout using printf().

Inconsistent File Name Size

In getFile() fileName can be up to 99 characters (strings must be null terminated),
in createFile() it can be 39 characters, this is inconsistent and may confuse the user.
The code also doesn't tell the user how long the file name can be and that could be a
problem for the user. In neither case is the length of the string checked prior to use,
and that could lead to a number of errors including buffer overflow.

A better way to handle the file name size might be to define a symbolic constant using #define.

#define MAX_FILE_SPEC_LENGHT 1024

    char userfilename[MAX_FILE_SPEC_LENGHT];

    char fileName[MAX_FILE_SPEC_LENGHT];

The file name variable should be large enough to handle a full path as well as the file name.

Many systems define the maximum file name size based on the file system implementation. On Linux the
include file linux/limits.h defines that maximum file name size using PATH_MAX. Most operating systems
will have some way of defining it.

Possible Bug

This line was flagged by my C compiler with a warning:

    fgets (&filecontent, 1000, stdin);

The warning is Incompatible pointer types passing char (*)[1000] to parameter of type char *.
Since file content is an array of chars it only needs the name filecontent rather than the address
of filecontent, to use the address, it should be &filecontent[0].

Inconsistent Variable Naming Conventions

In the function createFile() there are 3 variables, userfilename, filecontent and filePointer,
the variable filePointer is camelCase and is clear, userfilename and filecontent are not camelCase
and not quite as clear.


How to put apt and python packages in a list and check if installed or not then install I am not an expert in shell scripting,
I need help optimizing the shell script I wrote, This shell script I wrote is run by users on the client side who connect to our vpn server from the setting passed in the config.tar.
I check if pip or or tar or other packaches are installed if not isntalled than install it, I am doing it in separate if condition, what I want to achieve is I put the packages to check in a  listand then iterate if not installed than install it for all apt, pip or pip3 packages.
#!/usr/bin/env bash
set -

command=`which pip`

if [ "$command" = "" ]
then
    echo "The pip program not exist on this system."
    sudo apt-get install python-pip -y
    sudo apt install python3-pip -y
else
    echo "Pip is intalled."
fi
sudo apt install speedtest-cli
pip install -U pip
pip3 install mysql-connector-python
pip3 install pymysql
pip3 install sqlalchemy

TAR=`which tar`

if [ "$TAR" = "" ]
then
    echo "Tar is not installed,  installing now."
    sudo apt-get install tar -y
else
    echo "Tar is installed."
fi

# lets install requests
pip install requests

cd $HOME
echo "Iniating download..."
curl -sL "http://my.domain.com/config" -o "config.tar"

tar xvf config.tar
sudo chmod 604 ucc.py

if [ -e ucc.py ]
then
    echo "ucc.py installed."
else
    echo "ucc.py failed to download."
fi

if [ -e ucc.service ]
then
    echo "ucc.service installed."
else
    echo "ucc.service failed to download."
fi

sudo mv ucc.service "/lib/systemd/system/"

if [ -e client.conf ]
then
    sudo mv client.conf /etc/openvpn/
    echo "client.conf downloaded."
else
    echo "client.conf failed to download."
fi

sudo mv -f client.conf /etc/openvpn/

cd /etc/openvpn/

if [ -e client.conf ]
then
    echo "client.conf installed."
else
    echo "client.conf failed to download."
fi

sudo systemctl restart openvpn@client
sudo systemctl status  openvpn@client

any help will be greatly appreciated.

Use for loops for expandability and readability.
If you want to test for individual utilities, try using command -v pip instead of which and comparing against that.
Look up and understand the functional difference between single bracket comparisons and double bracket. [ ] vs [[ ]]
`` syntax is very out of date, use $()
If you have to call /usr/bin/env for the location of bash, but broadly assume apt is the package manager on the system, it's much safer to use /bin/bash.
Python requests is also stored as an apt package, I moved it into the first for loop statement.

#!/bin/bash

for PACKAGE in python-pip python-pip3 speedtest-cli tar python-requests; do
    if [[ ! $(dpkg -l ${PACKAGE}) ]] ; then
        echo "${PACKAGE} is not installed, installing now."
        sudo apt-get install ${PACKAGE} -y
    else
        echo "${PACKAGE} is installed."
    fi
done

pip3 install -U pip

for PACKAGE in mysql-connector-python pymysql sqlalchemy ; do
    pip3 install -U ${PACKAGE}

    if [[ $? -ne 0 ]] ; then echo "${PACKAGE} failed to installed" ; fi
done

cd $HOME
echo "Iniating download..."
curl -sL "http://my.domain.com/config" -o "config.tar"

tar xvf config.tar

sudo chmod 604 ucc.py
## I don't know why you need to use sudo to chmod an item inside your own home dir, but ill leave this here.
## Also, you can tar a package with certian perms and then unpack it to obey those specific permissions.

for ITEM in ucc.py ucc.service client.conf; do
    if [[ -e ${ITEM} ]] ; then
         echo "${ITEM} is installed."
    else
        echo "${ITEM} failed to download"
    fi
done

sudo mv ucc.service /lib/systemd/system/
sudo mv client.conf /etc/openvpn/
sudo mv -f client.conf /etc/openvpn/

sudo systemctl restart openvpn@client
sudo systemctl status  openvpn@client


Toggle menu under specified screen width I've created a simple toggle menu in jQuery, which is capable of specifying a breakpoint, but I'm afraid my code is too complicated. How can I make it simpler?

I also have a question about performance of code on window resize. I want to invoke a function only once if the window's width is smaller than a given breakpoint. Should I use some boolean variable, or is there a better solution?

Here's a pen.

$(document).ready(function() {
  var $menuBtn = $('[href="#menu"]'),
      $nav = $('#menu'),
      $window = $(window),
      breakpoint = 800;

  function changeMenuStyle() {
    if ($window.width() < breakpoint) {
      $menuBtn.show();
      $nav.addClass('toggled').hide();
    } else {
      $menuBtn.hide();
      $nav.removeClass('toggled').show();
    }
  }

  changeMenuStyle();

  $window.on('resize',function() {
    changeMenuStyle();
  });

  $menuBtn.on('click',function(e) {
    e.preventDefault();
    $nav.slideToggle();
  });
});
* {
  box-sizing: border-box;
}

body {
  background: #485373;
  font-family: "Lato", sans-serif;
  font-weight: 300;
}

.container {
  max-width: 940px;
  margin: 0 auto;
}

.menu-link {
  display: block;
  color: #fff;
  background: #202332;
  width: 100%;
  padding: 20px;
  text-decoration: none;
}
.menu-link i {
  display: inline-block;
  margin: 0 10px;
}

.menu {
  overflow: hidden;
}
.menu i {
  display: block;
  font-size: 25px;
  padding: 10px 0;
}
.menu ul {
  width: 100%;
}
.menu li {
  width: 20%;
  float: left;
  text-align: center;
}
.menu a {
  display: block;
  padding: 20px;
  width: 100%;
  background: #323A51;
  color: #fff;
  text-decoration: none;
  border-bottom: 2px transparent solid;
}
.menu a:hover {
  background: #202332;
  border-bottom: 2px #fff solid;
}
.menu.toggled i {
  display: inline-block;
  margin-right: 10px;
  padding: 0;
  font-size: 15px;
}
.menu.toggled li {
  width: 100%;
}
.menu.toggled a {
  padding: 10px;
}

main {
  padding: 50px;
  color: #fff;
}
main h1 {
  font-size: 30px;
  font-weight: 400;
  line-height: 40px;
  margin: 10px 0;
}

    Menu

Home

About

Services

Team

Contact

Toggle menu

Vestibulum tempus at ipsum a finibus. Mauris pharetra eleifend odio quis lacinia. Pellentesque ullamcorper, dolor vel congue tincidunt, mi nisl suscipit metus, nec bibendum elit ligula vitae leo. Integer volutpat semper iaculis. Mauris fermentum, enim ac finibus scelerisque, magna nibh volutpat dolor, vitae sodales mi augue molestie augue. Aliquam ultricies eget mi vel viverra. Morbi id tortor purus. Vivamus sed pretium justo. Fusce porttitor eu neque quis volutpat. Lorem ipsum dolor sit amet, consectetur adipiscing elit. Curabitur ultrices metus a aliquet tincidunt. Ut pellentesque dui nec ipsum fringilla ultricies.

Feedback

The code looks pretty good. The Javascript code waits until the DOM is ready, then caches references to DOM elements in variables before using those variables in the functions later on.

I initially thought some of the CSS could be consolidated (e.g. many common styles under .menu-link and .menu a but then I realized that might be tricky with the SCSS...

  I want to invoke a function only once if the window's width is smaller than a given breakpoint. Should I use some boolean variable, or is there a better solution?

One technique for this is to use a debounced function callback. There are jQuery plugins for that, or that article mentions ways to achieve it on your own (like I suggested in this SO answer).

Suggestions

Toggle the display using .toggle()

The .show() and .hide() calls can be replaced with a single call to .toggle(). Also, using the second form of .toggleClass(), the class name toggled can be added/removed depending on whether the menu button is being displayed or not (i.e. if the width is less than the break point). Thus the function changeMenuStyle() can be reduced to two lines (or one if you move the conditional out of a variable):

function changeMenuStyle() {
    var LTBreakpoint = $window.width() < breakpoint;
    $menuBtn.toggle(LTBreakpoint).toggleClass('toggled', LTBreakpoint);
}

Remove closure just to call function

The resize callback can simply be the name of the function - i.e.

$window.on('resize',function() {
    changeMenuStyle();
});

Can be simplified to:

$window.on('resize', changeMenuStyle);

Though be aware that the event parameter would get passed to the function, which could have adverse affects if the function is expecting other parameters.

CSS media query

I should have mentioned this, as Rotora did- CSS media queries can be used to eliminate much of the Javascript code (like you did in the CSS for this post).

Adding this CSS:

/* default settings ( width >= 800px) */
#menu{
    display: block;
}
[href="#menu"] {
    display: none;
}
/* only when screen width is less than 800px */
@media (max-width: 799px) {
    #menu{
        display: none;
    }
    [href="#menu"] {
        display: block;
    }
}

Can allow the JS to be simplified to:

$(document).ready(function() {
  var $menuBtn = $('[href="#menu"]'),
      $nav = $('#menu');

  $menuBtn.on('click',function(e) {
    e.preventDefault();
    $nav.slideToggle();
  });
});

$(document).ready(function() {
  var $menuBtn = $('[href="#menu"]'),
      $nav = $('#menu');

  $menuBtn.on('click',function(e) {
    e.preventDefault();
    $nav.slideToggle();
  });
});
* {
  box-sizing: border-box;
}

body {
  background: #485373;
  font-family: "Lato", sans-serif;
  font-weight: 300;
}

.container {
  max-width: 940px;
  margin: 0 auto;
}

.menu-link {
  display: block;
  color: #fff;
  background: #202332;
  width: 100%;
  padding: 20px;
  text-decoration: none;
}
.menu-link i {
  display: inline-block;
  margin: 0 10px;
}

.menu {
  overflow: hidden;
}
.menu i {
  display: block;
  font-size: 25px;
  padding: 10px 0;
}
.menu ul {
  width: 100%;
}
.menu li {
  width: 20%;
  float: left;
  text-align: center;
}
.menu a {
  display: block;
  padding: 20px;
  width: 100%;
  background: #323A51;
  color: #fff;
  text-decoration: none;
  border-bottom: 2px transparent solid;
}
.menu a:hover {
  background: #202332;
  border-bottom: 2px #fff solid;
}
.menu.toggled i {
  display: inline-block;
  margin-right: 10px;
  padding: 0;
  font-size: 15px;
}
.menu.toggled li {
  width: 100%;
}
.menu.toggled a {
  padding: 10px;
}

main {
  padding: 50px;
  color: #fff;
}
main h1 {
  font-size: 30px;
  font-weight: 400;
  line-height: 40px;
  margin: 10px 0;
}
/* default settings ( width >= 800px) */
#menu{
    display: block;
}
[href="#menu"] {
    display: none;
}
/* only when screen width is less than 800px */
@media (max-width: 799px) {
    #menu{
        display: none;
    }
    [href="#menu"] {
        display: block;
    }
}

    Menu

Home

About

Services

Team

Contact

Toggle menu

Vestibulum tempus at ipsum a finibus. Mauris pharetra eleifend odio quis lacinia. Pellentesque ullamcorper, dolor vel congue tincidunt, mi nisl suscipit metus, nec bibendum elit ligula vitae leo. Integer volutpat semper iaculis. Mauris fermentum, enim ac finibus scelerisque, magna nibh volutpat dolor, vitae sodales mi augue molestie augue. Aliquam ultricies eget mi vel viverra. Morbi id tortor purus. Vivamus sed pretium justo. Fusce porttitor eu neque quis volutpat. Lorem ipsum dolor sit amet, consectetur adipiscing elit. Curabitur ultrices metus a aliquet tincidunt. Ut pellentesque dui nec ipsum fringilla ultricies.


Get all points within given radius for a given vector I have a vector of ints that represents a point on a N-dimensional grid. I need to find all grid points that are within a given radius. I wrote the following, but it is rather inefficient, because it iterates through all points in a cube and checks the radius for each point:

typedef std::vector<int> VectI;

bool getNextInNSphere(VectI& x,int radius){
    int pos = 0;
    bool done = false;
    while (!done && pos < x.size()){
        x[pos]++;
        if (x[pos]>radius){ x[pos] = -radius; pos++; }
        else              { done = true; }
    }
    if (done){
        int sqsum = 0;
        for (size_t i=0;i<x.size();i++){ sqsum += x[i]*x[i]; }
        if (sqsum > radius*radius){ return getNextInNSphere(x,radius); }
    }
    return done;
}

usage:

void foo(const VectI& x,int radius){
    VectI d = VectI(x.size(),-radius);
    while (getNextInNSphere(d,radius)){
        // x+d is inside a sphere of given radius around x
    }
}

Any ideas how to get this more efficient and/or in a nicer way?

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

Use the appropriate #includes

In order to compile and link, this code requires at least following line:

#include <vector>

This too, should be part of the interface.

Provide complete code to reviewers

This is not so much a change to the code as a change in how you present it to other people.  Without the full context of the code and an example of how to use it, it takes more effort for other people to understand your code.  This affects not only code reviews, but also maintenance of the code in the future, by you or by others.  One good way to address that is by the use of comments.  The test code was helpful, but would have been slightly more so if a representative main had been include for a whole program.

Take advantage of symmetry

Given the simple case of 2D space, the "sphere" of course becomes a circle.  For a given radius \$r\$, we know that the coordinate \$(r,0)\$ is, by definition, the outer bound of the "sphere".  Also, no calculation is needed to determmine that the points \$(0, r), (-r, 0),\$ and \$(0, -r)\$ are also on the perimeter.  This principle can easily be expanded for any N-dimensional space.

Additionally, now that we know that \$(r,0)\$ and \$(-r, 0)\$ are among the desired points, it is clear that every point between them must also be inside the "sphere."  In general, one can iterate each dimension from \$r\$ to \$0\$, to follow the perimeter of the sphere (as with a modification of Bresenham's algorithm) and then simply include all points between there and the origin.

Precompute if practical

If you happen to know in advance a maximum size \$x\$ that will be used and the dimensionality \$d\$ of the space in which you'll work, you may be able to precompute the point clouds from 1 to \$x\$.  Since the point clouds are nested, (that is, the cloud for \$x=3\$ completely contains that of \$x=2\$, etc.) the points could be stored in a vector and for each integer  \$x\$, the program could store the matching index \$i\$ into the vector.  If the vector is sorted by increasing distance from the origin, then all points within a particular radius \$x\$ would be stored in indices \$[0 ... i]\$.

Consider writing an iterator

The code functions very much like an iterator but isn't quite.  Consider refactoring it so that it's an actual iterator and you may find that it's somewhat easier to use.

Reconsider the interface

Right now, the code doesn't actually return all points within the sphere.  Rather it returns the set of points, centered at the origin that are within the given radius.  It seems to me that it would be nicer to do the x+d calculation for the caller.


Function to convert a decimal number as a string to hexadecimal I am designing a cipher and need to convert between bases repeatedly in a loop.  I have optimized everything else, but I'm not too familiar with C++ code, and and am trying to figure out how to make the conversion faster.

string digits = "0123456789abcdef";
string tohex(string number) {                        // Decimal to Hexadecimal function
    long length = number.length();
    string result = "";
    vector<long> nibbles;
    for ( long i = 0; i < length; i++ ) {
        nibbles.push_back(digits.find(number[i]));
    }
    long newlen = 0;
    do {
        long value = 0;
        newlen = 0;
        for ( long i = 0; i < length; i++ ) {
            value = (value * 10) + nibbles[i];
            if (value >= 16) {
                nibbles[newlen++] = value / 16;
                value %= 16;
            } else if (newlen > 0) {
                nibbles[newlen++] = 0;
            };
        };
        length = newlen;
        result = digits[value] + result;
    } while (newlen != 0);
    return result;
}

Code Review.

If this is a set of constants you want to chech against then it should by marked as constexpr  (or const), that way nobody can actually modify it. Also why pollute the global space with another variable; declare it inside the function (as a static so it only gets initialized once).

string digits = "0123456789abcdef";

Also there is a standard function to check if a character is a hex digit std:: isxdigit().

Pass objects by reference to prevent a copy.

string tohex(string number) {

Since you are not modifying number make it a const reference.

Note: You should return by value. The optimizer will remove the need to copy out of the function so you don't need to worry about a copy here.

Declare variables as close to the point of usage as possible. Here result is declared but not used until the next section of code. So When I start seeing result being used I need to scan up the function a long way to find it.

    long length = number.length();
    string result = "";

This is an interesting way to convert characters into digits.

    vector<long> nibbles;
    for ( long i = 0; i < length; i++ ) {
        nibbles.push_back(digits.find(number[i]));
    }

But: 1) You don't check for invalid input as a result you may push back std::string::npos (a big number that is converted is probably converted to -1 when assigned to long). 2) This is O(n) for every digit. it can be done in O(1).

The standard guarantees that the character-set has all the numbers 0 -> 9 in a contiguous range. So you can convert a character (that is a digit) into a number by subtracting the value of 0 from the character.

char d = number[i];
nibbles.push_back((d >= '0' && d <= '9)
                         ? d - '0'
                         : (d >= 'a' && d <= 'f')
                                ? d - 'a' + 10
                                : (d >= 'A' && d <= 'F')
                                     ? d - 'A' + 10
                                     : -1;

PS. Don't write that. Put it in a function.

I see what you are doing. But the functionality could be a lot neater.

    long newlen = 0;
    do {
        long value = 0;
        newlen = 0;
        for ( long i = 0; i < length; i++ ) {
            value = (value * 10) + nibbles[i];
            if (value >= 16) {
                nibbles[newlen++] = value / 16;
                value %= 16;
            } else if (newlen > 0) {
                nibbles[newlen++] = 0;
            };
        };
        length = newlen;
        result = digits[value] + result;
    } while (newlen != 0);
    return result;
}

This is how I would do it.

How about:

template<class T>
string to_string(T t, ios_base & (*f)(ios_base&)) {

  ostringstream oss;
  oss << f << t;
  return oss.str();
}

string tohex(string number) {

    long value = std::stoi(number, null, 10);
    return std::to_string(value, std::hex);
}