Program to find the maximum number/factor in a list I have written the following basic assembly program to find the maximum number in a list divisible by a number. Here is what I have thus far:
# Program: given a list or integers and a factor
# Find the max number in the list that the factor divides
# For example: INPUT: [3,9,50,27], factor: 3 | OUTPUT: 27
.section .rodata
nums:   .long 3,9,50,27,-1
factor: .long 3

.section .data
cur_value:  .long -1

# first three args: %edi, %esi, %edx
.section .text
.globl _start
_start:

    # SETUP
    # %r8 will store the array index
    # %r11 will store the max value
    # %esi will store the factor/divisor.
    mov $0,     %r10d
    mov $0,     %r11d
    mov factor, %esi

loop:

    # get current value and store it in %rdi
    # we'll also update our variable for `cur_value`
    mov nums(, %r10d, 4), %edi
    cmp $-1, %edi
    je exit
    movl %edi, cur_value

    # Call the function and increment the aray index
    call is_divisible_by
    inc %r10d

    # if it was NOT divisible (rax = False or 0) jump back to the beginning
    cmp $0, %rax
    je loop

    # if it was divisible, check to make sure it's larger than the current max
    cmp %r11d, cur_value
    jl loop
    mov cur_value, %r11d
    jmp loop
  exit:
    mov %r11d, %edi
    mov $60, %eax
    syscall

is_divisible_by:

    # Return 0 (false) if not divisible; 1 (true) if divisible

    # A (dividend, %eax) / B (divisor)
    # dividend needs to first be moved into eax
    mov %edi, %eax

    # divide by a register, immediate, or memory address
    # this is unsigned (positive), use idiv for signed
    div %esi

    # the resultant integer quotient goes in %eax, and the remainder goes in %edx
    # if %rdx is zero it means A is divisible by B: we don't care about %eax
    mov $0, %eax

    cmp $0, %edx
    jne end
    mov $1, %rax

  end:
    ret

It's compiled using:
$ as file.s -o file.o && ld file.o -o file
$ ./file; echo $?
# 27

Here are a few particular questions related to this:

Is it common to use named variables (such as cur_value in .section .data) or not? I use them while learning a bit so it's easier to view the value of an easily-rememberable entity, i.e., I can just do x &cur_value in gdb to see what it is.

What is the suggested way to handle an if statement. I've tried to do this in the is_divisible_by function -- setting it to $0 by default and then 'overwriting' it if the true condition is met. -- but this seems pretty hacky. I suppose another way to do it would be something like:
  cmp $0, %edx
  je set_true
set_false:
  mov $0, %eax
  jmp clean_up
set_true:
  mov $1, %eax
  jmp clean_up
clean_up:
  ret

Is it common to have end-labels on functions and such? I find myself often adding an end or whatever to be able to 'exit' things easily.

For labels within a main label (such as exit or end or set_true etc.), what is a good way to name these? I see gcc uses something like .L1029 but that seems not-too-friendly when writing my own and having to remember then.

Would any of the above be better done 'on the stack' rather than using registers or named variables? I find it a bit more difficult to use the stack than registers as you cannot do something like mov mem1, mem2

Finally, how could I extract the is_divisible_by function into another file and call it from within this main file.s file?

You have two independent conditions that must be satisfied before updating cur_val.

The number must be divisible by factor
The number must be larger than cur_val

If the first test fails, you don't bother doing the second test.
How many instruction (or better cycles) does it take to do each test?
Given a long non-monotonic list, you may save considerable time simply by reversing these two tests.


Reading input from stdin I read user input from stdin in plain C.
The problem is that I want a sane implementation that is robust to errors and restricts the user to a certain input and doesn't suck in terms of complexity.
The function get_strings() reads input char by char as long there is no new line (\n), no EOF and all chars are passing the isalpha() test. But I want to keep spaces.

Some points that (I think) deserve special attention during the review:

- I would love to get rid of the outer while loop which is basically testing wether the user just pressed Enter without any meaningful input.

 - Do I even need to use ferror()? fgetc already returns EOF when something did go wrong, but I will just stop reading from the stream instead of telling the user something went wrong.

- It doesn't happen everytime but it happens: A \n stays in the stdin stream and the next time I want to get meaningful input fgetc() just gets skipped. It doesn't matter here, but it does when I am asking single character Yes/No questions. I can only get rid of this problem with a construct that clears out all previous things that are left in stdin. See the second codeblock for this.

So, am I handling this totally wrong? Are there better practices to embrace? It looks really clunky to me, and clunky is always bad.

/** @brief Contains the dictionary */
static char **strings = NULL;

/** @brief Helps with containing the dicionary */
static char *string;

/* Reads input char by char with fgetc() */
static char *get_strings()
{
    char *string = NULL;
    char ch;
    size_t len = 0;
    while (string == NULL && ch != EOF) {
    while (EOF != (ch = fgetc(in_stream)) && ch != '\n') {
        if (ch != ' ' && isalpha((int)ch) == 0) {
            fprintf(stderr, "Only [a-z] is a valid input. | \t"
                                "| Input another or end with CTRL+D: ");
            continue;
        }
        string  = (char*) realloc(string, len+2);
        if (string == NULL) {
            bail_out(EXIT_FAILURE, "realloc(3) failed");
        }
        string[len++] = toupper(ch);

        if (len >= MAX_DATA) {
            bail_out(EXIT_FAILURE, "Input too long\n");
        }
    }
    if (ferror(in_stream)) {
        bail_out(EXIT_FAILURE, "Error while reading from stream");
    }
    }

    if(string) {
        string[len] = '\0';
    } else {
        printf("\nFinished dictionary...\n");
    }
    printf("Added string: %s | Input another or end with CTRL+D: ", string);
    return string;
}

/* Saves the returned strings from get_strings() in a linked list */
static void read_dict()
{
    int index;
    for (index = 0; (string = get_strings()); ++index) {
        if (string[0] == '\0') continue;
        strings = (char**) realloc(strings, (index+1)*sizeof(*strings));
        if (strings == NULL) {
            bail_out(EXIT_FAILURE, "realloc(3) failed");
        }
        strings[index] = string;
    }

    /* Take a note of how many entries we have yet. */
    dict_size = index;
}

Second CodeBlock with a more simple case:

while(1) {
    char tmp;
    printf("Please enter your guess [a-z]: ");
    guess = fgetc(stdin);
    /* Jump back to start of loop */
    if (guess == '\n') {
        continue;
    }

    /* HERE IS THE CLEAR FOR STDIN
       This part really just eats all remaining \ns from the user,
       so that later inputs can start uninterrupted. Can I get rid of
       it in some better way? */
    while((tmp = getchar()) != '\n' && tmp != EOF);

    if(!isalpha(guess)) {
        fprintf(stderr, "Enter a valid letter [a-z]!\n");
        continue;
    }
}

Architecture

stdin is usually line buffered.  So nothing is given to fgetc() until the user hits Enter.  OP code will give multiple error messages with input like "Hello 123".  Better to separate user input from input validation.  Read the line of user input with fgets() or some version of your own as fgets() does have some weaknesses.  Then validate the input.

char *input;
while ((input = my_gets()) != NULL) {
  if (valid_phrase(input)) {
    foo(input);
  } else {
    fprintf(stderr, "Invalid input\n");
  }
  free(input);
}

Concerning "I would love to get rid of the outer while loop".  That loop exists to silently consume '\n'.  If you want a loop to do that, just preceded inner loop with

int ch;
while ((ch = fgetc()) == '\n')
  ;
ungetc(ch, stdin);

char ch

The ch is not the best type.  fgetc() returns typically 257 different values [0-255] and EOF.  To properly distinguish them, save the result in an int.

// bad
char ch;
..
while (string == NULL && ch != EOF) {
while (EOF != (ch = fgetc(in_stream)) && ch != '\n') {

// better
int ch;
..
while (string == NULL && ch != EOF) {
  while (EOF != (ch = fgetc(in_stream)) && ch != '\n') {

Same for char tmp;

realloc()

Cast not need.
Modify for out of memory to free string - not needed if code will simply exits, yet good practice to put your toys (code's pointer) away.

// string  = (char*) realloc(string, len+2);
char * new_string  = realloc(string, len+2);
if (new_string == NULL) {
  free(string);
  bail_out(EXIT_FAILURE, "Out of memory");
}
string = new_string;

Good use of of sizeof(*strings) below.  Recommend simplification.

strings = (char**) realloc(strings, (index+1)*sizeof(*strings));
strings = realloc(strings, sizeof *strings * (index+1));

size_t len

Good use of size_t to represent an array size.  Curiously code does not do  the same with int index;.  Recommend size_t index;

is...()

With using int ch, no need for cast.  Since this is a logical test, recommend using ! rather than arithmetic == 0.

// if (ch != ' ' && isalpha((int)ch) == 0) {
if (ch != ' ' && !isalpha(ch)) {

The following may be easier to understand - fewer negations.  (Style issue)

if (!(ch == ' ' || isalpha(ch))) {

ferror()

Nice check of if (ferror(in_stream))

Variable names

string, strings is as useful as calling an integer integer.  Maybe phrase, dictionary instead.

// OK
/** @brief Contains the dictionary */
static char **strings = NULL;

// Better
// comment not truly needed
static char **dictionary = NULL;

get_strings() is mis-named.  It sounds general, yet code restricts input to letters and space.  Maybe get_words()?


Inserting employee records There is nothing wrong, but is there a nicer way to re-write this program?

public IEnumerable<Employee> InsertEmployee(dynamic employees)
        {
            var newEmployeeRecord = new List<Employee>();

            foreach (dynamic employee in employees)
            {
                newEmployeeRecord.Add(

                    new Employee
                    {
                        EmployeeId = Convert.ToInt32(employee.EmployeeId)
                        ,
                        EmployeeName = Convert.ToString(employee.EmployeeName)
                        ,
                        Age = Convert.ToInt32(employee.Age)
                        ,
                        Salary = Convert.ToInt32(employee.Salary)
                        ,
                        DepartmentId = Convert.ToInt32(employee.DepartmentId)
                    }
                );
            }
            return EmployeeRecords.Concat(newEmployeeRecord);
        }

Also, can we avoid typecasting? (I think it's not possible, so it could require an extension method.)

Looking this over a second time, I see a few more problems, one of which was my mistake.

I believe that InsertEmployee is a bad name, since the number of employees inserted depends on the input parameter. A better name is InsertEmployees. Similarly, newEmployeeRecord is a bad name since it's also about an arbitrary set of employees, but it's extraneous anyway.

I notice that this code throws an exception if any of the Convert.To*() fails, maybe due to the CEO making $3 billion a year, or employeee IDs changing to have 10 digits. you mention wanting to avoid the type conversion, was this the main idea or was it speed?

I had missed the line about EmployeeRecords. What is it? It's not defined anywhere in the code provided; we can determine from context (though we shouldn't have to) that it is IEnumerable<Employee> or a derived class and that it's either a global, or a member of the same class that InsertEmployee() is, and that no matter what its protection status is, it's available to anyone who can call InsertEmployee(), which is everybody. Which brings up two questions: (a) why does it need to be returned from InsertEmployee() if it's accessible everyhwere that InsertEmployee() is? Just make it public. (b) Why wouldn't InsertEmployees just .Add the Employees directly to it?

public void InsertEmployees(dynamic employees)
{
    foreach (dynamic employee in employees)
    {
        EmployeeRecords.Add(new Employee
        {
            EmployeeId = Convert.ToInt32(employee.EmployeeId)
            ,
            EmployeeName = Convert.ToString(employee.EmployeeName)
            ,
            Age = Convert.ToInt32(employee.Age)
            ,
            Salary = Convert.ToInt32(employee.Salary)
            ,
            DepartmentId = Convert.ToInt32(employee.DepartmentId)
        });
    }
}

But let's get to the real underlying issue, why this whole code stinks. Several commenters hinted at the absurdity of using dynamic here. It's clear that you know the database schema; thus there's no reason to be using dynamic. .NET has good language features for working with database natively and in a strongly typed manner; it's called LINQ-to-SQL. Use it; you'll do less grunt work and have better code. Please see https://msdn.microsoft.com/en-us/library/bb386976%28v=vs.110%29.aspx

Old answer (incorrect, given that I missed what the function was returning):

Since you're returning an IEnumerable<>, you can simplify things greatly by using yield return to give elements back one at a time, rather than creating a temporary List and then throwing it away:

    public IEnumerable<Employee> InsertEmployee(dynamic employees)
    {
        foreach (dynamic employee in employees)
        {
            yield return new Employee
                {
                    EmployeeId = Convert.ToInt32(employee.EmployeeId)
                    ,
                    EmployeeName = Convert.ToString(employee.EmployeeName)
                    ,
                    Age = Convert.ToInt32(employee.Age)
                    ,
                    Salary = Convert.ToInt32(employee.Salary)
                    ,
                    DepartmentId = Convert.ToInt32(employee.DepartmentId)
                }
            );
        }
    }


Test finder PowerShell cmdlet I am learning PowerShell, and created the following script cmdlet.

It reads one or more specified script files, to discover special functions (UnitTests, or functions with the Verb 'UnitTest' or 'DataTest'.

I designed the parameters to work like Get-Content, as it reads content from files.

I would like general feedback on this cmdlet (design, workings), but i am also intrested in correctness.

<#
.SYNOPSIS
    Gets UnitTests from a UnitTestFile.
.DESCRIPTION
    UnitTests are PowerShell script functions, which are identified with the verb 'UnitTest' or 'DataTest'.
#>
function Get-UnitTest {
    [CmdletBinding(DefaultParameterSetName = "Path")]
    [OutputType([System.Management.Automation.FunctionInfo])]
    Param(
        <# Specifies a path to one or more UnitTestFiles. Wildcards are permitted. #>
        [Parameter(Position = 0, ParameterSetName = "Path", Mandatory = $true)]
        [string[]] $Path,

        <# Specifies a LiteralPath to one or more UnitTestFiles. Wildcards are permitted. #>
        [Parameter(ParameterSetName = "LiteralPath", Mandatory = $true, ValueFromPipeline = $true, ValueFromPipelineByPropertyName = $true)]
        [Alias("FullName")]
        [string[]] $LiteralPath,

        <# Specifies the Verb to use to identify UnitTests. The default verbs are 'UnitTest' and 'DataTest'. This means that functions with a name like 'UnitTest-Get-ChildItem' or 'DataTest-Invoke-WebRequest' are found. #>
        [String[]] $Verb = @( "UnitTest", "DataTest" )
    )

    Process {

        foreach($curPath in ($LiteralPath + $Path | Where {$_} | Resolve-Path)) {
            if(Test-Path $curPath -PathType Leaf) {

                Write-Trace "Getting UnitTests from '$curPath'."
                . $curPath

                # find and return all unit tests (from the requested file)
                Get-Command -Verb $Verb `
                    | Where { $_.CommandType -eq "Function" } `
                    | Where { ( $_.ScriptBlock.File ) -eq $curPath }
            }
        }
    }
}

This cmdlet can be called as follows:

Get-UnitTest -Path .\somefile.ps1
Get-UnitTest -Path .\somefile.ps1 -Verb 'OtherTest'
Get-ChildItem -Recurse *.ps1 | Get-UnitTest

Looks good.
Personally, I would take:

foreach($curPath in ($LiteralPath + $Path | Where {$_} | Resolve-Path)) {

and break out the collection generation code.
Specifically into something like:

$PathList = Join-Path $literalPath $Path | Resolve-Path

You shouldn't need to worry about a Where-Object to validate that it isn't $null.  If there is no result it won't go up the pipeline.

If you Dot Include all of your Scripts you can wait until all are included then check your commands.  This will run anything else you have in the script though.

It may be safer to use the parser like this sample below.  In this code I looked for catch blocks to insert a breakpoint , but you could have it filter for your test prefixes too.

$ParseError = $null
$Tokens = $null
$AST = [System.Management.Automation.Language.Parser]::ParseInput($psISE.CurrentFile.Editor.Text, [ref]$Tokens, [ref]$ParseError)
if($ParseError) {
    foreach($er in $ParseError) { Write-Error $er }
    throw "There was an error parsing script (See above). We cannot expand aliases until the script parses without errors."
}
$a = $ast.FindAll({($args[0] -is [System.Management.Automation.Language.CatchClauseAst])},$true)

Josh


Traceroute implementation in Rust I made this simple traceroute implementation using the libpnet library, which sends ICMP echo probes with an increasing time-to-live, and then outputs the IPs of the hosts replying.

I'm new to Rust, so any style/performance tips would be appreciated!

extern crate pnet;

use std::env;
use std::error;
use std::net;
use std::str::FromStr;

use pnet::packet::icmp::echo_request::MutableEchoRequestPacket;
use pnet::packet::icmp::IcmpTypes;
use pnet::packet::ip::IpNextHeaderProtocols;
use pnet::packet::ipv4::MutableIpv4Packet;
use pnet::packet::MutablePacket;
use pnet::transport::{icmp_packet_iter, transport_channel, TransportChannelType::Layer3};
use pnet::util;

type Result<T> = std::result::Result<T, Box<error::Error>>;

static IPV4_HEADER_LEN: usize = 21;
static ICMP_HEADER_LEN: usize = 8;
static ICMP_PAYLOAD_LEN: usize = 32;

fn main() {
    std::process::exit(match run_app() {
        Ok(_) => 0,
        Err(error) => {
            eprintln!("Error: {}", error);
            1
        }
    });
}

fn run_app() -> Result<()> {
    let args: Vec<String> = env::args().collect();
    match args.len() {
        2 => {
            let protocol = Layer3(IpNextHeaderProtocols::Icmp);
            let (mut tx, mut rx) = transport_channel(1024, protocol)
                .map_err(|err| format!("Error opening the channel: {}", err))?;

            let ip_addr = net::Ipv4Addr::from_str(&args[1]).map_err(|_| "Invalid address")?;
            let mut rx = icmp_packet_iter(&mut rx);
            let mut ttl = 4;
            let mut prev_addr = None;
            loop {
                let mut buffer_ip = [0u8; 40];
                let mut buffer_icmp = [0u8; 40];
                let mut icmp_packet =
                    create_icmp_packet(&mut buffer_ip, &mut buffer_icmp, ip_addr, ttl)?;
                tx.send_to(icmp_packet, net::IpAddr::V4(ip_addr))?;
                if let Ok((_, addr)) = rx.next() {
                    if Some(addr) == prev_addr {
                        return Ok(());
                    }
                    prev_addr = Some(addr);
                    // This is not quite ideal as replies may arrive in different order
                    // than they were sent in, but using the identification field
                    // would make this simple example more complex
                    println!("TTL: {} - {:?}", ttl, addr.to_string());
                }
                ttl += 1;
            }
        }
        _ => Err((format!("Usage: {} ip", args[0])).into()),
    }
}

fn create_icmp_packet<'a>(
    buffer_ip: &'a mut [u8],
    buffer_icmp: &'a mut [u8],
    dest: net::Ipv4Addr,
    ttl: u8,
) -> Result<MutableIpv4Packet<'a>> {
    let mut ipv4_packet = MutableIpv4Packet::new(buffer_ip).unwrap();
    ipv4_packet.set_version(4);
    ipv4_packet.set_header_length(IPV4_HEADER_LEN as u8);
    ipv4_packet.set_total_length((IPV4_HEADER_LEN + ICMP_HEADER_LEN + ICMP_PAYLOAD_LEN) as u16);
    ipv4_packet.set_ttl(ttl);
    ipv4_packet.set_next_level_protocol(IpNextHeaderProtocols::Icmp);
    ipv4_packet.set_destination(dest);

    let mut icmp_packet = MutableEchoRequestPacket::new(buffer_icmp).unwrap();
    icmp_packet.set_icmp_type(IcmpTypes::EchoRequest);
    let checksum = util::checksum(&icmp_packet.packet_mut(), 2);
    icmp_packet.set_checksum(checksum);
    ipv4_packet.set_payload(icmp_packet.packet_mut());
    Ok(ipv4_packet)
}

I love your program’s entry point. Nearly all of my F# CLIs have an entrypoint that look nearly identical. However, Rust will automatically convert a Result into an exit code for you.

You can get rid of your run_app and wrapper by inclining it and modifying main’s signature.

fn main() -> Result<(), Box<dyn Error>> {
   // contents of `run_app` method
}

I believe this will even automatically print out the error.


Python ASCII-Art Table As a part of a console utilities module, I created a function that takes a table in the form of an array of arrays, and generates an ASCII table with the given contents.

I've also added the options of adding a header, determining the in-cell alignment, and adding a border to the table.

Table cells have auto computed lengths by the longest cell of their column, rows that have a small amount of elements get filled by blank cells to adjust their length.

I'm quite new to Python, so I wanted to know about whether my writing style is OK. Can I improve or shorten some parts of the code? Any other advice?

def ascii_table (table, **k):
    header = k.get('header', [])
    align = k.get('align', 'left')
    border = k.get('border', False)

    widths = []
    for i in range(max(map(len, table))): widths.append(max(max(map(len, [row[i] for row in table if len(row) > i])), len(header[i]) if len(header) > i else 0))

    printable = []

    if border:
        printrow = []
        for i in range(max(map(len, table))):
            if i > 0 and i < max(map(len, table)) - 1: printrow.append('─' * (widths[i] + 2))
            else: printrow.append('─' * (widths[i] + 1))
        printable.append('┌─' + '┬'.join(printrow) + '─┐')

    # header formatting
    if len(header) > 0:
        printrow = []
        for i in range(len(header)):
            assert header[i]
            if align == 'center': printrow.append(header[i].center(widths[i]))
            elif align == 'left': printrow.append(header[i].ljust(widths[i]))
            elif align == 'right': printrow.append(header[i].rjust(widths[i]))

        if border: printable.append('│ ' + ' │ '.join(printrow) + ' │')
        else: printable.append(' │ '.join(printrow))

        printrow = []
        for i in range(len(header)):
            if i > 0 and i < len(header) - 1: printrow.append('─' * (widths[i] + 2))
            else: printrow.append('─' * (widths[i] + 1))

        if border: printable.append('├─' + '┼'.join(printrow) + '─┤')
        else: printable.append('┼'.join(printrow))

    # table formatting
    for row in table:
        printrow = []
        for i in range(len(widths) - len(row)):
            row.append('')
        for i in range(len(row)):
            if align == 'center': printrow.append(row[i].center(widths[i]))
            elif align == 'left': printrow.append(row[i].ljust(widths[i]))
            elif align == 'right': printrow.append(row[i].rjust(widths[i]))

        if border: printable.append('│ ' + ' │ '.join(printrow) + ' │')
        else: printable.append(' │ '.join(printrow))

    if border:
        printrow = []
        for i in range(max(map(len, table))):
            if i > 0 and i < max(map(len, table)) - 1: printrow.append('─' * (widths[i] + 2))
            else: printrow.append('─' * (widths[i] + 1))
        printable.append('└─' + '┴'.join(printrow) + '─┘')

    return '\n'.join(printable)

Demo:

>>> from asciiart import ascii_table
>>> table = list(map(lambda x: x.split(), ['Jon Doe 20', 'Mark Waine 35', 'Donald Rory 43']))
>>> header = ['First Name', 'Last Name', 'Age']
>>> ascii = ascii_table(table, header=header, align='center', border=True)
>>> print(ascii)
┌────────────┬───────────┬─────┐
│ First Name │ Last Name │ Age │
├────────────┼───────────┼─────┤
│    Jon     │    Doe    │  20 │
│    Mark    │   Waine   │  35 │
│   Donald   │    Rory   │  43 │
└────────────┴───────────┴─────┘

In no particular order:

Your approach to default keyword arguments with the **k is unusual. I’d expect to see something more like:

def ascii_table(table, header=None, align='left', border=False):
    if header is None:
        header = []

Note that I’ve dropped the space between the function name and the opening parens (per PEP 8 style), and also used None as the default for the header parameter – this is to avoid quirks around mutable default arguments.

This style is more in keeping with other Python code, and makes it easier for somebody to see what parameters this function takes. It also makes it easier to spot when somebody is calling this with entirely wrong arguments.
Newlines are cheap! Don’t put stuff on a single line, especially if … else blocks. For example, this block:

if border: printable.append('│ ' + ' │ '.join(printrow) + ' │')
else: printable.append(' │ '.join(print row))

would usually be written more like:

if border:
    printable.append('│ ' + ' │ '.join(printrow) + ' │')
else:
    printable.append(' │ '.join(print row))

That style is more conventional, and makes it easier to see the structure of the code.
Loop over the elements of a list directly, not its indices. For example, this loop:

for i in range(len(header)):
    assert header[i]
    if align == 'center':
        printrow.append(header[i].center(widths[i]))
    ....
    # do stuff with header[i]

could be better written as something like:

for h, w in zip(header, widths):
    if align == 'centre':
        printrow.append(h.center(w))
    ...

Iterating directly over lists is more Pythonic, and generally makes for cleaner code.
Don’t cram too much into a single expression; it becomes difficult to parse and hurts readability. This is on the edge of what I’d consider acceptable:

for i in range(max(map(len, table))):

This is unreadable:

for i in range(max(map(len, table))): widths.append(max(max(map(len, [row[i] for row in table if len(row) > i])), len(header[i]) if len(header) > i else 0))

You should break it across multiple lines.
You can tidy up some of your logical conditions. For example, this:

if i > 0 and i < max(map(len, table)) - 1:

can be reduced to:

if 0 < i < max(len(t) for t in table) - 1

which is a bit easier to read.
This could potentially choke on a large table, because you’re building all the rows in a list printable. I would consider rewriting this as a generator – essentially, replace every printable.append(foo) with yield foo.

Then your function just lazily computes the rows, and passes them to the caller. It’s up to them how they want to use it – whether they want to save it, do some more processing, join it into a string (and then they can choose the newline).

Generators and iteration are a very powerful feature of Python – if you’re not familiar with them already, this PyCon talk is a good introduction.


Example x86-64 program I wrote the following x86 program to make sure I'm following the correct practices in calling a function and then exiting to the OS:
.globl _start

_start:

    # Calculate 2*3 + 7*9 = 6 + 63 = 69
    # The multiplication will be done with a separate function call

    # Parameters passed in System V ABI
    # The first 6 integer/pointer arguments are passed in:
    #   %rdi, %rsi, %rdx, %rcx, %r8, and %r9
    # The return value is passed in %rax

    # multiply(2, 3)
    # Part 1 --> Call the parameters
    mov $2, %rdi
    mov $3, %rsi
    # Part 2 --> Call the function (`push` return address onto stack and `jmp` to function label)
    call multiply
    # Part 3 --> Handle the return value from %rax (here we'll just push it to the stack as a test)
    push %rax

    # multiply(7, 9)
    mov $7, %rdi
    mov $9, %rsi
    call multiply

    # Add the two together
    # Restore from stack onto rdi for the first function
    pop %rdi
    # The previous value from multiply(7,9) is already in rax, so just add to rbx
    add %rax, %rdi

    # for the 64-bit calling convention, do syscall instead of int 0x80
    # use %rdi instead of %rbx for the exit arg
    # use $60 instead of 1 for the exit code

    movq $60, %rax    # use the `_exit` [fast] syscall
                      # rdi contains out exit code
    syscall           # make syscall

multiply:
    mov %rdi, %rax
    imul %rsi, %rax
    ret

Does the above follow the x86-64 conventions properly? I know it is probably as basic as it comes, but what can be improved here?

To elaborate on some comments you got on the SO version of this question, the main thing you're missing is stack alignment, a requirement of the SysV ABI calling conventions that's often overlooked by beginners.
The requirement is (ABI 3.2.2):

The end of the input argument area shall be aligned on a 16 (32 or 64, if
__m256 or __m512 is passed on stack) byte boundary.

So that means that, at the instant before you execute a call instruction, the stack pointer %rsp needs to be a multiple of 16.  In your case you have a push of 8 bytes without a pop in between your two calls to multiply, so they can't both have correct alignment.
Some wrinkles are introduced here by the fact that your parent function is _start instead of main or another function called by C code:

The conditions on entry to _start are described in 3.4 of the ABI.  In particular, the stack is aligned to 16 bytes at the instant _start gets control.  Also, since you cannot return from _start (there is no return address on the stack), you have to exit with a system call as you do, and so there is no need to save any registers for the caller.

For main or any other function, the stack would have been aligned to 16 bytes before your function was called, so the extra 8 bytes for the return address mean that on entry to your function, the stack is now "misaligned", i.e. the value of rsp is 8 more or less than a multiple of 16.  (Since one would normally only manipulate the stack in 8-byte increments, it's only really ever in two possible states, which I'll call "aligned" and "misaligned".)  Also, in such functions, you would need to preserve the contents of the callee-saved registers %rbx, %rbp, %r12-r15.

So as it stands, your first call to multiply has correct stack alignment, but your second does not.  Of course, it's only of academic interest in this case, because multiply doesn't do anything that needs stack alignment (it doesn't even use the stack at all), but it's good practice to do it right.
One way to fix it would be to subtract another 8 bytes from the stack pointer before the second call, either with sub $8, %rsp or (more efficiently) by simply pushing any random 64-bit register.  But why should we bother to use the stack at all to save this value?  We could simply put it in one of the callee-saved registers, say %rbx, which we know multiply must preserve.  Normally this would require us to save and restore the contents of this register, but since we are in the special case of being _start, we don't have to.

A separate comment is that you have a lot of instructions like mov $7, %rdi where you operate on 64-bit registers.  This would be better to write as mov $7, %edi.  Recall that every write to a 32-bit register will zero the upper half of the corresponding 64-bit register, so the effect is the same as long as your constant is unsigned 32 bits, and the encoding of mov $7, %edi is one byte shorter as it doesn't need a REX prefix.
So I'd revise your code as
.globl _start

_start:

    # Calculate 2*3 + 7*9 = 6 + 63 = 69
    # The multiplication will be done with a separate function call

    # Parameters passed in System V ABI
    # The first 6 integer/pointer arguments are passed in:
    #   %rdi, %rsi, %rdx, %rcx, %r8, and %r9
    # The return value is passed in %rax

    # multiply(2, 3)
    # Part 1 --> Load the parameters
    mov $2, %edi
    mov $3, %esi
    # Part 2 --> Call the function (`push` return address onto stack and `jmp` to function label)
    call multiply
    # Part 3 --> Save the return value
    mov %rax, %rbx   # could also do mov %ebx, %eax if you know the result fits in 32 bits

    # multiply(7, 9)
    mov $7, %edi
    mov $9, %esi
    call multiply

    # Add the two together
    add %rbx, %rax
    mov %rax, %rdi

    # for the 64-bit calling convention, do syscall instead of int 0x80
    # use %rdi instead of %rbx for the exit arg
    # use $60 instead of 1 for the exit code

    mov $60, %eax    # use the `_exit` [fast] syscall
                      # rdi contains out exit code
    syscall           # make syscall

multiply:
    mov %rdi, %rax
    imul %rsi, %rax
    ret

If you want to rely on the result of multiply fitting in 32 bits, you could replace mov %rax, %rbx with mov %eax, %ebx to save one byte.  And likewise, the "Add the two together" could use 32-bit instructions instead to save two more bytes.
Finally, there's a stylistic point on whether to use the AT&T-syntax operand size suffixes, like addq versus add.  They are optional when one operand is a register, since the operand size can be deduced from the size of that register (e.g. 32 bits for %eax, 64 bits for %rax, etc).  My personal preference is to always use them, as a little extra verification that you're really writing what you mean, but omitting them as you (mostly) did is also common and fine; just be consistent.  You did have one instance of movq $60, %rax where it wasn't needed, so for consistency I omitted the suffix there. (I also changed it to %eax for the reasons noted above.)


K-Nearest Neighbors in pure Python I want a general criticism on this code. Using external modules is not an option, I can only use what comes with CPython.

from collections import Counter
from typing import Sequence, NamedTuple

Coordinates = Sequence[float]

class KNNPoint(NamedTuple):
    coords: Coordinates
    classif: str

def predict(target: Coordinates, points: Sequence[KNNPoint], k: int) -> str:
    '''
    Applies the K-Nearest Neighborhood algorithm in order to find the
    classification of target.

    - target: Single data to be classified.
    - points: Collection of data which classifications are known.
    - k: The number of closest neighbors to be used.
    '''
    def distance(p: KNNPoint) -> float:
        return sum((a - b) ** 2 for a, b in zip(target, p.coords))

    neighbors = sorted(points, key=distance)
    counter = Counter(x.classif for x in neighbors[:k])

    return counter.most_common(1)[0][0]

If you'd like to run it, this gist has everything ready. it uses a dataset of mobile phones. (This gist shall not be reviewed)

Assumption: k << N, where N = len(points)

There is no need to sort the entire list of points!

Instead, take the first k points, and determine their distance values, and sort them.  Then, for each success point:

determine its distance,
if it is smaller than the maximum,

discard the maximum, and insert the new point in the sorted list.

Sorting N points by distance is O(N log N); creating and maintaining a sorted list of k smallest elements is only O(N log k), which should be considerably faster.

I'm not sure if heapq.nsmallest() is built into CPython or not ...

k_neighbours = heapq.nsmallest(k, points, key=distance)
counter = Counter(x.classif for x in k_neighbours)

Well, I'm disappointed to see heapq.nsmallest() performed up to 40% worse that sorted on CPython, but I'm happy to see PyPy validates my assertion that you don't need to sort the entire list.

Continuing with that thought, bisect.insort() may be used to maintain a list of the k-nearest neighbours so far:

    neighbours = [(float('inf'), None)] * k
    for pnt in points:
        dist = distance(pnt)
        if dist < neighbours[-1][0]:
            neighbours.pop()
            bisect.insort(neighbours, (dist, pnt))

    counter = Counter(pnt.classif for dist, pnt in neighbours)

This gave me 4% speedup over sorted()[:k] with your gist sample set.

Significant, but not impressive.  Still, it was enough encouragement to press on an look for other inefficiencies.

How about the distance() code.  It gets called a lot; can we speed it up?  Sure!

def predict(target: Coordinates, points: Sequence[KNNPoint], k: int, *,
            sum=sum, zip=zip) -> str:

    def distance(p: KNNPoint) -> float:
        return sum((a - b) ** 2 for a, b in zip(target, p.coords))

    # ...

Instead of searching the global scope for the sum and zip functions, they are saved as variables sum, zip in the local scope, along with target, for use in distance().  Total improvement: 6%.

Applying the same sum=sum, zip=zip change to the original code, without the bisect.insort() change, also speeds it up by 2%.

Further, adding insort=bisect.insort to the function declaration, and using insort(neighbours, (dist, pnt)) in the function body also provides a minor improvement.

Finally, I was concerned about neighbours[-1][0].  Looking up the first tuple of the last element in the array seemed inefficient.  We could keep track of this in a local threshold variable.  Final total speedup: 7.7%.

neighbours = [(float('inf'), None)] * k
threshold = neighbours[-1][0]
for pnt in points:
   dist = distance(pnt)
   if dist < threshold:
      neighbours.pop()
      insort(neighbours, (dist, pnt))
      threshold = neighbours[-1][0]

YMMV


Improvements for ugly reflection I have a project that uses a library to save serialized objects in items and loads them again, when needed.
To do the de-/serialization, this library uses Gson.

Due to how Gson works and how the library is set up, I can't add support for custom types easily and thus have to rely heavily on reflection.
I don't like this, but it has to be like this, sadly.

I am asking, whether there is a way to make this code use more Kotlin idioms.
My issue is that because the fields I am accessing are private, the usual reflection using :: doesn't work.

Anyways, here is my code:

/**
 * Registers a custom GSON handler to handle the [Location] class.
 */
private fun registerGsonHandlers() {
    logger.entering(LobbyMain::class, "registerGsonHandlers")
    // Gets private static final Gson gson = new Gson();
    val fGson = NBTReflectionUtil::class.java.getDeclaredField("gson")
    fGson.isAccessible = true

    val modifiersField = Field::class.java.getDeclaredField("modifiers")
    modifiersField.isAccessible = true
    modifiersField.setInt(fGson, fGson.modifiers and Modifier.FINAL.inv())

    // Gets  private final List<TypeAdapterFactory> factories;
    val fFactories = Gson::class.java.getDeclaredField("factories")
    fFactories.isAccessible = true
    val factories = fFactories.get(fGson.get(null)) as List<*>

    // Sets private static final Gson gson
    fGson.set(null, GsonBuilder().apply {
        registerTypeAdapter(Location::class.java, LocationTypeAdapter)
        factories.forEach { this::registerTypeAdapterFactory }
    }.create())
    logger.exiting(LobbyMain::class, "registerGsonHandlers")
}

It pulls the Gson object out of the NBTReflectionUtil class, makes it modifiable and gets its list of TypeAdapterFactories.
I then create a new Gson object with all the old TypeAdapters and mine added and set it as the new Gson object for the library.

I'd group your code to make it more readable. Something like this:

fun getGson(): Field {
    val fGson = NBTReflectionUtil::class.java.getDeclaredField("gson")
    fGson.isAccessible = true
}

fun removeFinalModifier(field: Field) {
    val modifiersField = Field::class.java.getDeclaredField("modifiers")
    modifiersField.isAccessible = true
    modifiersField.setInt(fGson, fGson.modifiers and Modifier.FINAL.inv())
}

// etc


TicTacToe game in Python 3 I wrote this program in Python 3.I would like to know if there is some more functionality that could be added/removed/is missing.Any comments would be helpful!

#Steps from user :
"""
Enter Input
P1 and P2
Each time for input will be preceded by a printed board.
The game will continue until an input matches a win combination.
"""
win_combinations = [
    [1,2,3],
    [4,5,6],
    [7,8,9],
    [1,4,7],
    [2,5,8],
    [3,6,9],
    [1,5,9],
    [3,5,7]]

tot_list = [1,2,3,4,5,6,7,8,9]

def board():
    print ("{}|{}|{}".format(tot_list[0],tot_list[1],tot_list[2]))
    print ("{}|{}|{}".format(tot_list[3],tot_list[4],tot_list[5]))
    print ("{}|{}|{}".format(tot_list[6],tot_list[7],tot_list[8]))

p1 = 0
p2 = 0

def player_input():
    global p1
    global p2
    while True:
        try:
            print("\n\n")
            board()
            print("\n")
            p1 = int(input("P1(X): Enter your Input:"))
            print("\n")
            p2 = int(input("P2(O): Enter your Input:"))
            if 0 < p1 < 10 and 0 < p2 < 10 :
                assign_values(p1,p2)
                break
            print("Re-enter valid input")
        except:
            print("Invalid input")
            continue

def assign_values(player1,player2):
    if type(tot_list[player1-1]) == str() or type(tot_list[player2-1]) == str():
        print("Re-enter valid input")
        player_input()
    if player1 in tot_list:
        tot_list[player1-1] = "X"
    if player2 in tot_list:
        tot_list[player2-1] = "O"

p1_win = False
p2_win = False
tie = False

def default_values():
    global tot_list
    global p1
    global p2
    global p1_win
    global p2_win
    global tie
    tot_list = [1,2,3,4,5,6,7,8,9]
    p1 = 0
    p2 = 0
    p1_win = False
    p2_win = False
    tie  = False

def play():
    global p1_win
    global p2_win
    global tie
    global tot_list
    while True:
        player_input()
        for i in win_combinations:
            if tot_list[i[0]-1] == "X" and tot_list[i[1]-1] == "X" and tot_list[i[2]-1] == "X":
                p1_win = True
                break
            elif tot_list[i[0]-1] == "O" and tot_list[i[1]-1] == "O" and tot_list[i[2]-1] == "O":
                p2_win = True
                break
        if all(isinstance(i,str) for i in tot_list) == True:
            tie == False
        if p1_win==True:
            print("P1 has won!")
            break
        elif p2_win==True:
            print("P2 has won!")
            break
        elif tie == True:
            print("Its a tie!")
            break

    play_again = input("Play Again?(y/n)")
    if play_again == "y":
        default_values()
        play()
    else:
        print("Ciao")
if __name__ == '__main__':
    play()

Your code doesn't work like in real life. For example, when I play:

1|2|3
4|5|6
7|8|9

P1(X): Enter your Input:1

P2(O): Enter your Input:1

X|2|3
4|5|6
7|8|9

There is no error generated. Both players cannot request the same location. Also, playing a new game, this scenario:

P2(O): Enter your Input:5

X|O|3
X|O|6
7|8|9

P1(X): Enter your Input:7

P2(O): Enter your Input:

This scenario P2(O): should never have been reached. Player 2 doesn't get another move after Player 1 wins the game. Recommend fixing these problems.

Onto your code -

Firstly, you have an entry point - that's good, however it doesn't do anything other than call play().

You should move the control loop into the entry point, and remove duplicate code scattered amongst your code. In default_values(), you scatter those pieces across the code.

tot_list = [1, 2, 3, 4, 5, 6, 7, 8, 9]
p1 = 0
p2 = 0
p1_win = False
p2_win = False
tie = False

Yet you have them inside default_values(). Inside your play() function, you have play() in the loop, but the first time you call it, you specify default_values() - notice the inconsistency there?
You should have realized that when you were reviewing your code.

So, we fix that, and this is what we come up with:

if __name__ == '__main__':

    win_combinations = [
        [1, 2, 3],
        [4, 5, 6],
        [7, 8, 9],
        [1, 4, 7],
        [2, 5, 8],
        [3, 6, 9],
        [1, 5, 9],
        [3, 5, 7]]

    while True:
        default_values()
        play()
        choice = input("Play Again? ('n' to exit):")
        if choice == "n":
            break

    print("Ciao")

You also have lots of other issues with your code, but let me give you a few pointers for the rest (I don't want to rewrite your entire code).

if p1_win == True:
You don't need to specify "True" as this is the same: if p1_win:

You have global statements all throughout your code. This is bad - and the reason is - you don't know 100% where your variables are being changed. The #1 source of all bugs when coding is when the state changes in variables and the change was unintended. Does that make sense? Please review it by googling.
What you should do is pass variables around. Having a piece of code like:

active_player = not active_player
player_input = get_player_input()
board_state = update_board(player_input, board_state)
if check_for_a_winner(board_state):
    print("We have a winner! {} wins!".format(active_player))

shows you how we inject the input as well as the current board status into the update_board function, and receive the updated board status back from it. We can then present the current board status into the winner check function.
This examples shows that state changes only in very specific places, and if you're hunting down a bug, you can find it quite simply.

Typically, with several pieces of related state, you'll need an object which can represent all of them at once. I recommend reading a book which explains about classes, for instance having multiple dog and cat classes and how you can represent their attributes.

Please attempt to incorporate these changes into you code and repost your updated solution (with a link to this page) and demonstrate your code working better, with those bugs removed.

Good Luck!


Check if the user is 18 years old (upto days) This code is returning 'true' for any date which are later than Feb 24, 1998.

//minimumAge is number of years i.e., 18
public boolean isUser18Older (DateTime userDob, int minimumAge){
    DateTime minAge = new DateTime();
    Days days = Days.daysBetween(userDob, minAge.minusYears(minimumAge));

    return days.getDays()>=0 ;
}

I am not sure about my minAge.minusYears code.

There's a potential bug in your method if the user is born just the day after the age requirement. Your method will return true when it should in fact return false. For example the following code prints true:

public static void main(String[] args) {
    DateTime test = new DateTime().minusYears(1).plusDays(1);
    System.out.println(isUser18Older(test, 1));
}

public static boolean isUser18Older (DateTime userDob, int minimumAge){
    DateTime minAge = new DateTime();
    Days days = Days.daysBetween(userDob, minAge.minusYears(minimumAge));

    return days.getDays()>=0 ;
}

although the user was born the day after the requirement. This is because you're not taking the time into account.

The name of the method is misleading with regard to what it does. It is named isUser18Older, which implies that the method checks whether a user has at least 18 years-old. However, the method takes a minimumAge parameter, which is actually the minimum age requirement. Therefore, I suggest renaming that method hasAgeRequirement.

The code itself could be written more simpler and to-the-point. Instead of getting the number of days between the two dates, just retrieve the number of years between the date of birth and today and compare it with the age requirement:

public static boolean hasAgeRequirement(DateTime userDob, int minimumAge) {
    DateTime now = new DateTime();
    Years age = Years.yearsBetween(userDob, now);
    return age.getYears() >= minimumAge;
}

This is easier to read since it directly codes the intent of the method.


Angular - reusing an observable for different queries This is just a dummy example, but in my real world application I get a lot of data from the web API and I'd like to manipulate it on the client's side. So, I kind of stored the observable I get from the HTTP request locally and change the values with map:

@Injectable()
export class UsersApiService {

  private readonly baseUrl: string = 'https://reqres.in/api/users';
  resource$: Observable<any>;

  constructor(private http: HttpClient) {
    this.resource$ = this.http.get<IUserDetails[]>(this.baseUrl).pipe(
      tap((data) => {
        console.log('"getUsers" successfully called!');
      }),
      map((data: any) => {
        return data.data;
      })
    ).publishReplay(1).refCount();
  }

  getUsers(): Observable<IUser[]> {
    return this.resource$.pipe(
      map((data: IUserDetails[]) => {
        return <IUser[]>data.map((u) => {
          return {
            id: u.id,
            name: `${u.first_name} ${u.last_name}`
          };
        });
      })
    );
  }

  getUserById(id: number): Observable<IUserDetails> {
    return this.resource$.pipe(
      map((data) => {
        return <IUserDetails>data.find(x => x.id === id);
      })
    );
  }

}

Here's an example plunker

Is this the correct approach at reusing an observable? What would be better? Am I creating any memory leaks and what are the general drawbacks of my approach? Thanks!

There's nothing (obviously) wrong with your code. You deal with observables the efficient way, IMO. Cache is good. The fact you're constructing a "base" observable in constructor without subscribing to it there is good as well...

There are only a few things I can point out:

Design: Be careful with caching, if your user information changes often, the cache may bite you back. You may want to implement some kind of cache invalidation policy.
Design: If your system has thousands of users, you may want to not load all of them into memory at once. That would require your API support pagination though. If the users are few, it's totally okay to do what you're doing, I think.
RxJs: not sure why pipe() and tap() are used the way are used. I think, it's easier to use do() and map() as shown in the code below. Chaining looks much more readable (as "step-by-step" explanation of how the data is transformed into the desired result).
Idioms: TypeScript is all about types. Do specify a known type when possible instead of using any. E.g. Observable<IUserDetails[]> is better than Observable<any[]>.
Style: Variable naming is important. Do not use one-letter names (u), they are pure evil. Names like data are also evil, in your case it's better to call it response. The resource$ can probably be nicer to the reader if called userDetails$. You get the idea.

@Injectable()
export class UsersApiService {

  private readonly baseUrl: string = 'https://reqres.in/api/users';
  userDetails$: Observable<IUserDetails[]>;

  constructor(private http: HttpClient) {
    this.userDetails$ = this.http
      .get<IUserDetails[]>(this.baseUrl)
      .do(response => console.log('"getUsers" successfully called!'))
      .map(response => response.data)
      .publishReplay(1)
      .refCount();
  }

  getUsers(): Observable<IUser[]> {
    return this.userDetails$
      .map(userDetailsList =>
        userDetailsList.map(userDetails =>
          <IUser>{ id: userDetails.id, name: `${userDetails.first_name} ${userDetails.last_name}` }
        )
      );
  }

  getUserById(id: number): Observable<IUserDetails> {
    return this.userDetails$
      .map(userDetailsList => <IUserDetails>userDetailsList.find(userDetails => userDetails.id === id));
  }
}


Storing temporary data in ETS - for example, using ETS in a web wizard I'm learning the Elixir language and the Phoenix web framework. And I had an idea. What if, in a web wizard (multi-step form) scenario, I stored the intermediate data in a ETS table instead of the relational database?

So I wrote this controller:

defmodule PlayPhoenixWeb.ThingsController do
  use PlayPhoenixWeb, :controller
  require Logger
  @moduledoc false

  @form_ets :things_forms

  alias PlayPhoenixWeb.Router.Helpers, as: Routes
  alias PlayPhoenixWeb.Endpoint

  def step_1_display(conn, _params) do
    render(conn, "form_1.html")
  end

  def step_1_process(conn, params) do
    data = Map.take(params, ["name", "vcode"])
    key = make_key()
    write_data(key, data)
    Logger.debug inspect(data)
    redirect(conn, to: Routes.thing_form_2_path(Endpoint, :step_2_display, key))
  end

  def step_2_display(conn, %{"key" => key}) do
    render(conn, "form_2.html", key: key)
  end

  def step_2_process(conn, %{"key" => key} = params) do
    {:ok, data} = get_data(key)
    full_data = Map.take(params, ["description", "value"]) |> Map.merge(data)
    # In production, processing full data would happen here
    Logger.debug inspect(full_data)
    redirect(conn, to: Routes.page_path(Endpoint, :index))
  end

  # This method is called by the Application on startup
  def init_ets(), do: :ets.new(@form_ets, [:public, :named_table])

  defp make_key(), do: :crypto.hash(:md5, DateTime.utc_now() |> DateTime.to_iso8601) |> :base64.encode

  defp get_data(key) do
    case :ets.lookup(@form_ets, key) do
      [{^key, value}] -> {:ok, value}
      _ -> {:not_found}
    end
  end

  defp write_data(key, data) do
    :ets.insert(@form_ets, {key, data})
  end

  defp delete_data(key), do: :ets.delete(@form_ets, key)

end

(It's purely for the sake of playing/research, and currently doesn't store or process any data).

Is this the Elixir way/the Phoenix way?

Thank you very much.

Yes, using ETS for this sort of temporary data is totally fine, as long as you realize of course that all that intermediate state is lost whenever you restart your server. In fact, if you dig into Phoenix and Elixir a bit, you will see a lot of ETS usage - it's not so much a thing you turn to in exceptional circumstances, but more a tool in the extensive toolbox at the same level of, say, GenServer.


Users' timetable database I wish to create a database for a webpage where users are able to add their own events to a timetable. Users will be able to decide if the events should be recurring (weekly) or not. Users should be able to query other groups of users to organize a time for a meeting. That is it.

What I have so far is as follows:

Users (
  userID INT NOT NULL AUTO_INCREMENT,
  username VARCHAR(80),
  PRIMARY KEY (user_id)
);

Groups {
  GroupID INT NOT NULL AUTO_INCREMENT,
  GroupName NVARCHAR(100)
  private BOOLEAN
}

Membership {
  UserID (FOREIGN KEY, UNIQUE),
  GroupID( FOREIGN KEY, UNIQUE)
 }

Events (
    event_id INT NOT NULL AUTO_INCREMENT,
    title VARCHAR(80) NOT NULL,
    description VARCHAR(200),
    start_time DATETIME,
    end_time DATETIME,
    group_id INT NOT NULL,
    recurring BOOLEAN
);

Group (
    group_id INT NOT NULL,
    user_id INT NOT NULL
);

When a user wants to arrange a meeting using my database, I will be forced to use 3 queries and 2 loops, is there a better way than this? It all seems extremely complicated.

int i = 0;
String[] names = { Request.Form["usernames"]Split(' ') }; //retrieving names from form
List<int> user_ids = new List<int>();
foreach(string name in names){
    int user_id = db.QueryValue("SELECT user_id FROM users WHERE username = name");
    user_ids.Add(user_id); //now I have a list of all user_ids
}
db.Execute("INSERT INTO group(groupName) values(Request.Form["Group name"])");
int groupID = db.QueryValue("SELECT groupid from group where            groupname="Request.Form["Group name "]");
foreach (string user_id in user_ids) {
      db.Execute("INSERT INTO membership(userid,groupid) values(user_id, groupID);"
}

From looking at your database description . . .

No unique constraint on Users.username. You can have 1000 users named "Fred".
No unique constraint on Groups.groupname. You can have 1000 groups named "Hamburger". You probably don't want to allow that.
If you intend to build Membership like UserID integer not null unique references Users (userID), and GroupID integer not null unique references Groups (GroupID) then each user and each group can appear in that table only once. That means that if I have membership in the group Hamburger, you cannot also have membership in the group Hamburger. I think you probably want to drop the UNIQUE constraint from both those clauses, and add PRIMARY KEY (UserID, GroupID).
It's not clear what the difference is between the two tables Membership and Group.

From looking at your code . . .

db.QueryValue("SELECT user_id FROM users WHERE username = name"); You're selecting user identifiers given the users' names. But names aren't unique in your database. If I select one "Fred", and there are 1000 Freds in your database, I'm going to end up with 1000 different user identifiers. You probably don't want to do this.
db.Execute("INSERT INTO group(groupName) There's no column named "groupname" in that table.
int groupID = db.QueryValue("SELECT groupid from group where groupname = You're selecting group identifiers given the group's name. Same problem as with selecting user identifiers above.

Unless I misunderstand you, and that's always possible, you should be able to insert existing users and existing groups into Memberships with a single INSERT statement.


Packet Scout (show traffic on specified port) I've just made a github repository for a simple packet scout in C#. It is my first time using C# (other than Neoaxis 3D engine logic) since I am more familiar with C#. Below is the main bulk of the code that I'd like to improve. You can see all of the code here. If you have any suggestions please share them.

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Windows.Forms;
using System.Net.Sockets;
using System.Net;

namespace Packet_Scout
{
    public partial class Form1 : Form {
        private const string endl = "\n";
        private UdpClient socket;
        private bool listening = false;
        private static string buff = "";

        public Form1() {
            InitializeComponent();
        }

        private void downloadbuffer() {
            rblog.Text += buff;
            buff = "";
        }

        private void btnstart_Click(object sender, EventArgs e) {
            if (!listening) {
                rblog.Text = "";
                socket = new UdpClient((int)ndoor.Value);
                tmbuf.Start();
                bgw.RunWorkerAsync();
                btnstart.Text = "Stop";
                listening = true;
            } else {
                tmbuf.Stop();
                downloadbuffer();
                bgw.CancelAsync();
                socket.Close();
                rblog.Text += "Stopped!";
                listening = false;
                btnstart.Text = "Start";
            }
        }

        private void bgw_DoWork(object sender, DoWorkEventArgs e) {
            buff = "Listening on port " + ndoor.Value + endl;

            while (socket != null) {
                if (socket.Available > 0) {
                    IPEndPoint ipep = new IPEndPoint(IPAddress.Any, 0);
                    Byte[] recvbytes= socket.Receive(ref ipep);

                    buff += "Client (" + ipep.Address + ":" + ipep.Port + "): ";
                    foreach (Byte b in recvbytes) buff += b.ToString() + " ";
                    buff += endl;
                }
            }
        }

        private void tmbuf_Tick(object sender, EventArgs e) {
            downloadbuffer();
        }
    }
}

Naming

Methods should be named using PascalCase casing and should reflect by its name what the responsibility of the method is. So instead of downloadbuffer() a more descriptive name would be AppendToRichtextBox() or something similiar.

Talking about responisibilties, a class should usually have only one responsibility, so you should separate the UI related code and the listening code into separate classes.

BackgroundWorker

The BackgroundWorker has an event to report the current progress, so no timer is needed.
The call to CancelAsync() isn't working like you guess it is.

  CancelAsync submits a request to terminate the pending background operation and sets the CancellationPending property to true.

  When you call CancelAsync, your worker method has an opportunity to stop its execution and exit. The worker code should periodically check the CancellationPending property to see if it has been set to true.

Now you will say, but it stops the worker. This is only partial true, because after the said call you are closing the socket which will lead to a ObjectDisposedException at retrieving the socket.Available property.
This exception will be passed to the RunWorkerCompleted eventhandler see the DoWork() event documentation

  If the operation raises an exception that your code does not handle, the BackgroundWorker catches the exception and passes it into the RunWorkerCompleted event handler, where it is exposed as the Error property of System.ComponentModel.RunWorkerCompletedEventArgs. If you are running under the Visual Studio debugger, the debugger will break at the point in the DoWork event handler where the unhandled exception was raised. If you have more than one BackgroundWorker, you should not reference any of them directly, as this would couple your DoWork event handler to a specific instance of BackgroundWorker. Instead, you should access your BackgroundWorker by casting the sender parameter in your DoWork event handler.

Instead of adding to the static buffer variable I would suggest to use the ReportProgress() event and instead of using the string variable you should use a StringBuilder object and passing the built string to the event. By doing so you omit the fact that for each string concatination a new string is created because strings in .NET are immutable.

Like

private void bgw_DoWork(object sender, DoWorkEventArgs e) {
    StringBuilder sb = new StringBuilder();

    sb.Append("Listening on port ")
      .Append(ndoor.Value)
      .Append(endl);

    while (socket != null) {
        if (socket.Available > 0) {
            IPEndPoint ipep = new IPEndPoint(IPAddress.Any, 0);
            Byte[] recvbytes= socket.Receive(ref ipep);

            sb.Append("Client (")
              .Append(ipep.Address)
              .Append(":")
              .Append(ipep.Port)
              .Append("): ");

            foreach (Byte b in recvbytes)
            {
                sb.Append(b.ToString())
                  .Append(" ");
            }
            sb.Append(endl);

            bgw.ReportProgress(0, sb.ToString());
            sb.Length = 0;
        }
    }

    private void AppendToRichtextBox(string value)
    {
        rblog.AppendText(value);
        rblog.ScrollToCaret();
    }

    private void bgw_ProgressChanged(object sender, ProgressChangedEventArgs e)
    {
        AppendToRichtextBox(e.UserState.ToString());
    }

because the Append() method of the StringBuilder is returning a StringBuilder you can fluently call this method like sb.Append("first ").Append("and second !");.

If you want to add a carriage return + new line you can simply call one of the overloaded AppendLine() methods.

You can also use the var type if the right hand side of the assignment makes the type obvious

  Beginning in Visual C# 3.0, variables that are declared at method scope can have an implicit type var. An implicitly typed local variable is strongly typed just as if you had declared the type yourself, but the compiler determines the type.

Like

var sb = new StringBuilder();


Implementing range from_to with step #include <vector>

std::vector<int> Range(int from, int to, int step = 1) {
    std::vector<int> range;

    if (step > 0) {
        for(int i = from; i < to; i += step) {
            range.push_back(i);
        }
    } else if (step < 0) {
        for(int i = from; i > to; i += step) {
            range.push_back(i);
        }
    }

    return range;
}

I don't like code duplication in my implementation, because I have two almost identical blocks.
Is it possible to avoid it?

I think the easiest way to solve this is to first calculate how many steps it takes to go from from to to, and then just build an array of that many elements. That also has the advantage that you can reserve the right amount of elements in the vector up front. You also have to make sure that step has the correct sign.
if (from <= to != step > 0)
    step = -step;

size_t distance = std::abs(to - from);
size_t stepsize = std::abs(step);
size_t nsteps = (distance + stepsize - 1) / stepsize;

std::vector<int> range;
range.reserve(nsteps);

for (size_t i = 0; i < nsteps; ++i)
    range.push_back(from + i * step);

You also might want to add some check that step is not zero.


Is this an appropriate use of abstract classes and inheritance? This is a simple app that starts the webcam either in color or in grayscale mode depending on what the user wants:

The grayscale camera is implemented via the GrayVideo class and the color camera through ColorVideo. I wanted to have a common show_current_frame for both the ColorVideo and GrayVideo objects. That show_current_frame gets the current frame from the process method and displays it in the GUI. Since the color and the grayscale videos need different frame processing, I needed to have different process methods for ColorVideo and GrayVideo. So, I thought of having a Video class with an abstract process method.

Is this a decent use of abstract classes and inheritance?
What other problems and possible improvements do you see in my code?
Or is the design wrong in the first place?

import cv2
from abc import ABC, abstractmethod

class Video(ABC):

    def __init__(self, source):
        self.capture = cv2.VideoCapture(source) # If source = 0 the webcam starts

    def get_raw_frame(self):
        # Get a bolean and current frame (numpy array) from the webcam
        ret, frame = self.capture.read()
        if ret:
            return frame

    @abstractmethod
    def process(self):
        # Method to process frames.
        # Method will be overwritten by subclasses
        pass

    def show_current_frame(self):
        # Get processed frame and show it in the GUI
        current_frame = self.process()
        cv2.imshow('Live', current_frame)

    def end(self):
        # Releases webcam and closes GUI window
        self.capture.release()
        cv2.destroyAllWindows()

class ColorVideo(Video):

    def process(self):
        # Since raw frames are in color, there's no processing needed
        return self.get_raw_frame()

class GrayVideo(ColorVideo):

    def process(self):
        # Grayscaling the raw frames
        raw_frame = self.get_raw_frame()
        gray = cv2.cvtColor(raw_frame, cv2.COLOR_BGR2GRAY)
        return gray

user_preference = input('Enter "c" for color, or "g" for grayscale: ')

if user_preference == 'c':
    video = ColorVideo(source=0)
if user_preference == 'g':
    video = GrayVideo(source=0)

while True:
    video.show_current_frame()

    if cv2.waitKey(1) & 0xFF == ord('q'):
        video.end()

Is this a decent use of abstract classes and inheritance?

Yes, it's decent. My personal preference is, rather than using ABC, simply

def process(self):
    # Method to process frames.
    # Method will be overwritten by subclasses
    raise NotImplementedError()

You can also simplify this somewhat by

Renaming Video to ColorVideo
Deleting the class that is now called ColorVideo
Rather than making process abstract, make it "virtual" (in C++ parlance): make it take the contents of what is now get_raw_frame and delete get_raw_frame
In the child GrayVideo, override process to call its super().process and convert the results

If you're concerned that this more minimal representation is confusing (i.e. why is a GrayVideo a ColorVideo?) then you can keep Video as a conceptually abstract class, and simply

class ColorVideo(Video):
    pass

Other stuff:

end should actually be the __exit__ of a context manager;
it looks like your source argument should take a default of 0;
make a main function called by standard __name__ guard.


Finding the length of the longest streak of ones in any array I need help reviewing my code snippet copied below.
The below code counts the number of continuous ones in a given array of Size 11.

#define size 11
#define ONE 1
using namespace std;

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

    int arr[11] = {1, 0, 1, -1, 0, 0, 1, 1,0, 1, 1};
        int sequence_count = 0;
        int loop_count;
        bool continuity = true;
        for(loop_count = 0; loop_count<size; loop_count++)
        {
            if((arr[loop_count]&ONE) == ONE)
            {
                if(continuity)
                    sequence_count++;
                else
                {
                    continuity = true;
                    sequence_count++;
                }
            }
            else
            {
                continuity = false;
                sequence_count = 0;
            }
        }
        cout << sequence_count << endl;

I see a number of things that could help you improve your code.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Use only required #includes

The code has a line #include "stdio.h"s that is not needed.  This clutters the code and makes it more difficult to read and understand.  Only include files that are actually needed.

Provide main

It may just have been a cut and paste error, but this appears to be the contents of a main function, even though main does not appear.

Use appropriate names for constants

The constants size and ONE are a bit too generic and likely to be used elsewhere.  In fact, this code won't even compile using a current version of gcc because of a name clash with size.

Prefer constexpr to old-style #define

Rather than using a #define for size the code could use a constexpr:

constexpr std::size_t ARRAY_SIZE = 11;

It doesn't make a lot of difference here, but generally the advantage is that the value has an associated type.

Declare variables as late as possible

Rather than using the old C-style of declaring all variables at the top of a function, use the modern C++-style and declare variables as late as possible.  Doing so can sometimes help the compiler figure out register allocation, resulting in faster, smaller code.

Fix your formatting

There are inconsistent spaces at the beginning of lines, inconsistent indentation and inconsistent use of curly braces {}.  Being consistent helps others read and understand your code.

Be careful with signed and unsigned

The code compares an int loop_count with the size of the array.  Is the size of the array ever going to be a negative number?  Probably not.  Therefore, it would be better to declare loop_count to be of type std::size_t.

Fix the bugs

The description says that this code counts ones but the code includes this line:

if((arr[loop_count]&ONE) == ONE)

The use of the operator & in there is doing a binary "and" operation.  That expression would be true if the value is -1 or 5 or any other odd number.  What you probably really want is

if (arr[loop_count] == ONE)

Also, your program only counts the length of the last sequence of contiguous ones.  If you want the maximum length, you'll have to save it.

Simplify your code

The code contains this sequence:

if(continuity)
    sequence_count++;
else
{
    continuity = true;
    sequence_count++;
}

But it could be replaced with this simpler sequence:

sequence_count++;

And then the continuity variable isn't even needed.

An alternative

Here's the code as it might look with all of these suggestions applied:

#include <iostream>

int main()
{
    constexpr std::size_t ARRAY_SIZE {11};
    constexpr int target_value {1};

    int arr[ARRAY_SIZE] = {1, 0, 1, -1, 0, 1, 1, 1, 0, 1, 1};
    unsigned sequence_count = 0;
    unsigned max_sequence = 0;
    for(std::size_t i = 0; i<ARRAY_SIZE; i++) {
        if (arr[i] == target_value) {
            if (++sequence_count > max_sequence) {
                max_sequence = sequence_count;
            }
        } else {
            sequence_count = 0;
        }
    }
    std::cout << max_sequence << std::endl;
}


Find labels in string in Haskell I wrote code that parses a table header string in order to obtain two string tags, one for variable name (like GDP) and another for unit of measurement (like bln_rub). The tags collectively make a unique identifier for the table data.

Perhaps something can be done better/shorter/more easily readable?

import Data.List (isInfixOf)

type Label = Maybe String

data Variable = Variable
    { name :: Label,
      unit :: Label
    } deriving (Show, Eq)

makeVariable :: String -> String -> Variable
makeVariable name unit = Variable (Just name) (Just unit)

isDefined:: Variable -> Bool
isDefined var = (name var /= Nothing) && (unit var /= Nothing)

isIdentical:: Variable -> String -> String -> Bool
isIdentical var name unit = (makeVariable name unit) == var

-- Map allows a readable view of tuple-like
-- associative structure.
data Map = Map
    { label :: String,
      texts :: [String] -- note: can use non-empty List
    } deriving (Show)

nameMaps = [
      Map "GDP" ["Gross domestic product"]
    , Map "INDPRO" ["Industrial production"]
    ]

unitMaps = [
      Map "bln_rub" ["bln rub", "billion ruble"]
    , Map "rog" ["% change to previous period"] -- rate of growth
    ]

-- COMMENT: code below converts nameMaps and unitMaps
--          to list of tuples which are used for searching a header
asTuples :: Map -> [(String, String)]
asTuples (Map label texts) = [(text, label) | text <- texts]

findAllKeys :: [(String, String)] -> String -> [String]
findAllKeys mapper header = [key | tup@(text, key) <- mapper,
                                text `isInfixOf` header]

getLabel :: [Map] -> String -> Label
getLabel maps' header = case findAllKeys (flatten' maps') header of
        [] -> Nothing
        (x:_) -> Just x
    where flatten' = concatMap asTuples

getName = getLabel nameMaps
getUnit = getLabel unitMaps
parseHeader text = Variable (getName text) (getUnit text)

x = parseHeader "Gross domestic product, bln rub"
flag = (isDefined x) && (isIdentical x "GDP" "bln_rub")
raise x = error ("Something wrong with: " ++ show x)

main = do
    if flag then putStrLn $ show x else
      raise x
    return ()

Perhaps something can be done shorter

A trick that I often use to avoid creating "filled" objects is simply matching on Just:

isIdentical:: Variable -> String -> String -> Bool
isIdentical (Variable (Just n) (Just u)) n' u' = n == n' && u == u'
isIdentical _ _ _ = False

You can even do the actual check inside the pattern guard:

(Variable (Just n) (Just u)) n' u' | n == n' && u == u' = True

But that seems like an overkill.

Not saying that this change is particularly warranted in this specific case, but it's a nice tool to have, I think.


Finding consecutive numbers in a vector I need to write a program to print the line numbers, in ascending order, of a particular word in a file. Example output of the program would be:

 Hello 1, 3, 5-6, 8, 10-15.
// 5-6 represents 5, 6 (consecutive numbers)

The line numbers are stored in a sorted vector<int> with no duplicates. I have created a solution, but my instincts tell me I can do better. I would appreciate some feedback. Further, the following code only processes the line numbers; the word is found in another method. I will remark that intToString() is a custom method that does as the name implies. It returns a static string.

void IndexPager::createLines(vector<int>& vec, string& line)
{
  int start = *vec.begin(), end = -1, offset = 0; // initial offset to start

  for (vector<int>::const_iterator itr = vec.begin();
       itr != vec.end() + 1; itr++)
  {
    if (*itr == start + offset && itr != vec.end())
    {
      end = *itr;
      ++offset;
    } // check if line numbers are consecutive and not reading at end of vector
    else // not consecutive
    {
      if ((end != -1) && (end != start))
      {
        line.append(intToString(start) + "-");
        line.append(intToString(end));
      } // if there existed consecutive numbers, display with dash
        // must be difference of at least 1
      else // else, there were no consecutive numbers
        line.append(intToString(start));

      if (itr != vec.end()) // check if not at end of vector
        line.append(", ");
      else // reached end of vector
        line.append(".");

      start = *itr; // set start to next line number. Soft reset.
      offset = 1; // change default offset to 1. 0 for first case.
      end = -1;
    } // not consecutive
  } // Get all line numbers and format for proper output
} // createLines()

First use ++it instead it++ with iterators.
You can replace this loop:

for (vector<int>::const_iterator itr = vec.begin();
     itr != vec.end() + 1; itr++)`

with range-based for:

for (auto i : v)

vec.end() returns iterator representing element after the last element in vector
if (itr != vec.end()) loop also does this check
I think you can rewrite your code to be clearer and with less ifs
also you can use std::to_string instead of intToString
also if you want to append . at the end of string you can do it outside of the loop

Here is the code with few things fixed; still I think you can do even better than that if you rethink your conditions.

void createLines(vector<int>& vec, string& line)
{
  int start = *vec.begin(), end = -1, offset = 0; // initial offset to start

  for (auto i : vec)
  {
    if (i == start + offset)
    {
      end = i;
      ++offset;
    } // check if line numbers are consecutive and not reading at end of vector
    else // not consecutive
    {
      if ((end != -1) && (end != start))
      {
        line.append(to_string(start) + "-");
        line.append(to_string(end));
      } // if there existed consecutive numbers, display with dash
        // must be difference of at least 1
      else // else, there were no consecutive numbers
        line.append(to_string(start));

    line.append(", ");

  start = i; // set start to next line number. Soft reset.
  offset = 1; // change default offset to 1. 0 for first case.
  end = -1;
} // not consecutive
} // Get all line numbers and format for proper output
 if ((end != -1) && (end != start))
 {
   line.append(to_string(start) + "-");
   line.append(to_string(end));
 }
 else
   line.append(to_string(start));
line.append(".");
} // createLines()


Blocking queue implementation with std::unique_ptr I implemented, somewhat closely based on this implementation, a blocking queue with emphasis on filling up first. So if one producer and one consumer thread are using the queue, the producing queue gets prioritized.

// BQueue.hpp
//#pragma once
#ifndef _BQUEUE_HPP
#define _BQUEUE_HPP

#include <condition_variable>
#include <mutex>
#include <queue>

template <class T> class BQueue {
public:
  BQueue(size_t size);
  void push(T item);
  bool pop(T &item);
  /* ALTERNATIVE 1
  void push(std::unique_ptr<T> item);
  bool pop(std::unique_ptr<T> &item);
  */

private:
  std::mutex _mutex;
  //std::queue<std::unique_ptr<T>> _queue; // ALTERNATIVE 1
  std::queue<T> _queue;
  size_t _size;
  std::condition_variable _condition_full;
  std::condition_variable _condition_empty;
};
#include "BQueue.hxx"
#endif // _BQUEUE_HPP

implementation

// BQueue.hxx
#include "BQueue.hpp"
#include <condition_variable>
#include <cstdlib>
#include <iostream>
#include <mutex>
#include <queue>

template <class T> BQueue<T>::BQueue(size_t size) : _size(size) {}

// ALTERNATIVE 1
// template <class T> void BQueue<T>::push(std::unique_ptr<T> item) {
template <class T> void BQueue<T>::push(T item) {
  std::unique_lock<std::mutex> lock(_mutex);
  while (_queue.size() >= _size) {
    _condition_full.wait(lock, [&]() { return (_queue.size() < _size); });
  }
  _queue.push(std::move(item));
  // _queue.push(item); // ALTERNATIVE 1

  // if queue is full, notify consumation part first
  if (_queue.size() >= _size) {
    _condition_empty.notify_one();
  }
  _condition_full.notify_one();
}

// template <class T> bool BQueue<T>::pop(T &item) {
template <class T> bool BQueue<T>::pop(T &item) {
  std::unique_lock<std::mutex> lock(_mutex);
  while (_queue.empty()) {
    if (!_condition_empty.wait_for(lock, std::chrono::seconds(1),
                                   [&]() { return !_queue.empty(); })) {
      // waited too long for input.
      return false;
    }
  }
  item = std::move(_queue.front());
  // item = _queue.fron(); // ALTERNATIVE
  _queue.pop();
  /* THIS FOLLOWING CODE MAY BE NEEDED;
   * KEEP IT IN CASE
   * _condition_empty.notify_one();
   * // if queue is empty, notify production
   * if (_queue.empty()) { //*/
  _condition_full.notify_one();
  // assert(_queue.size() < _size); //*/
  return true;
}

main

// alternatively: class Resource
struct Resource {
  int x;
};

int main() {
  BQueue<std::unique_ptr<struct Resource>> q{40};
  //BQueue<struct Resource> q_alternative1[40};

  std::unique_ptr<struct Resource> res1{new struct Resource};
  res1->x = 42;
  q.push(std::move(res1));

  q.push(std::move(std::unique_ptr<struct Resource>{new struct Resource}));

  for (size_t i = 0; i < 30; i++) {
    std::unique_ptr<struct Resource> res{new struct Resource};
    res->x = i;
    q.push(std::move(res));
  }
  for (size_t i = 0; i < 15; i++)
    q.pop(res1);
  return 0;
}

Now, I came across the question of resource handling. What does make more sense? Using the shown code or implement the whole thing using std::unique_ptr where necessary (see ALTERNATIVE 1)? std::move even works with non-resource wrapped types, apparently.

And, is the main function leak-free or did I miss anything? Does it work for shared_ptr types as well, or do I have to specialize because of pop(T item &) being a reference?

So if one producer and one consumer thread are using the queue, the producing queue gets prioritized.

I don't see anything in your code that corresponds to this sentence in your explanation. I think you have no "prioritization" going on here. The only thing determining which threads get to run when is the OS thread scheduler. And there's nothing at the algorithm level that could be described as "priorities"; this isn't like a rwlock where one side can "starve" the other. Contrariwise — each side here is "feeding" the other!

std::unique_ptr<struct Resource> res1{new struct Resource};

Two or three things about this line:

Don't use raw new and delete; use std::make_unique here instead.
Don't Repeat Yourself: use auto to avoid repetition here.
In C++, writing struct (or class or union) in a type-name is not idiomatic.

Thus this line should become:

auto res1 = std::make_unique<Resource>();

and your main routine in total becomes:

int main() {
    BQueue<std::unique_ptr<Resource>> q{40};
    q.push(std::make_unique<Resource>(42));
    q.push(std::make_unique<Resource>());
    for (int i = 0; i < 30; ++i) {
        q.push(std::make_unique<Resource>(i));
    }
    for (int i = 0; i < 15; ++i) {
        std::unique_ptr<Resource> res1;
        q.pop(res1);
    }
}

Writing return 0; at the end of main is unnecessary and unidiomatic (unless you want to be explicit because you return a non-zero value by some other codepath).

About unique_ptr inside the container versus outside: Instinctively prefer abstractions that are composable. So, if your options are:

create BQueue<T> that deals internally in std::unique_ptr<T>, or
create BQueue<T> that deals internally in T,

you should obviously prefer the latter. As a bonus, this means that your code will be useful ("reusable") even for people who are still stuck on boost::unique_ptr — they won't have to switch all their code from boost to std just to use your queue class.

while (_queue.size() >= _size) {
  _condition_full.wait(lock, [&]() { return (_queue.size() < _size); });
}

This loop is extremely confusing. You're looping with one condition outside and a different condition in the inner lambda. I strongly recommend removing one or the other of these loops. Specifically, I would remove the inner loop.

while (_queue.size() >= _size) {
  _condition_full.wait(lock);
}

The nice thing about removing the inner loop (as above) is that it's easily readable by a non-expert (say, someone coming from Java, C#, or Python). But if your target audience is C++ experts, this would also be okay:

_condition_full.wait(lock, [&]() { return (_queue.size() < _size); });

Your condition variables have mildly confusing names, IMO. Is _condition_full the thing I should wait on when the queue is full? Or is it the thing that gets notified when the queue is full? A little thought shows that it must be the former; but I don't like thinking, even a little bit. :)

Notice that this line —

_queue.push(std::move(item));

— runs under the mutex lock. If the item's move-constructor does something sketchy, this could be dangerous.

If you generalize your BQueue far enough that the user-programmer is allowed to customize the underlying container (e.g. change std::queue<T> to std::queue<T, std::list<T, A>>), then be aware that the user-defined memory allocator will also be running under the mutex lock. This probably isn't a problem at your current level of caring-about-metaprogrammy-bits, but it's something to consider.

if (!_condition_empty.wait_for(lock, std::chrono::seconds(1),
                               [&]() { return !_queue.empty(); })) {
  // waited too long for input.
  return false;
}

This (a function that blocks, but only for 1 second) is weird and dangerous. Don't do this.

If you want a pop function, write one. If you want a try_pop function (that can fail if the queue is empty), write one. If you want a try_pop_for function (that can fail by timing out), write one. But what you did was

write a try_pop_for_1s function (?!)
misname it pop (?!)

/* THIS FOLLOWING CODE MAY BE NEEDED;
 * KEEP IT IN CASE
 * _condition_empty.notify_one();

I believe this code is not needed. Consider:

The queue is empty. Thread C1 enters pop and blocks on _condition_empty.
Thread C2 enters pop and blocks on _condition_empty.
Thread P enters push, adds an item to the queue, and notifies-one on _condition_empty.
Thread P enters push again, adds a second item, and notifies-one on _condition_empty.

At this point, if either thread C1 or C2 is still waiting, it's a bug in the standard library. They should both be awake and waiting their turns to grab the mutex lock and process one item each off of the queue.

Similarly, the _condition_full.notify_one() at the end of push should not be needed, because if you've got two threads waiting on _condition_full, you might as well wait for the second pop before you unblock the second producer thread.

So:

template <class T>
void BQueue<T>::push(T item) {
  std::unique_lock<std::mutex> lock(_mutex);
  _condition_full.wait(lock, [&]() { return (_queue.size() < _size); });
  _queue.push(std::move(item));  // DANGER: running user code under a lock
  _condition_empty.notify_one();  // the queue is no longer empty
}

template <class T>
bool BQueue<T>::pop(T &item) {
  std::unique_lock<std::mutex> lock(_mutex);
  if (_condition_empty.wait_for(lock, std::chrono::seconds(1),
                                 [&]() { return !_queue.empty(); })) {
    item = std::move(_queue.front());  // DANGER: running user code under a lock
    _queue.pop();  // DANGER: running user code under a lock
    _condition_full.notify_one();  // the queue is no longer full
    return true;
  }
  // waited too long for input; the queue is still empty
  return false;
}


Break a full name into a dictionary of its parts It seems like I'm repeating myself here. Is there a standard way to deal with code like this?

def getNameParts(name):
    """Return a dictionary of a name's parts.
    name: e.g. 'JOHNSON, John Roberts, Jr. (Jack)'
    Ignores nicknames.
    """
    csSplit = name.split(', ')
    try:
        last = csSplit[0]
    except:
        last = ''
    try:
        first = csSplit[1].split(' ')[0]
    except:
        first = ''
    try:
        middle = ' '.join(csSplit[1].split(' ')[1:])
    except:
        middle = ''
    try:
        suffix = csSplit[2].split(' ')[0]
    except:
        suffix = ''
    partsDict = {'first': first,
                 'last': last,
                 'middle': middle,
                 'suffix': suffix}
    return(partsDict)

You have some unnecessary try blocks.  Watch this:

>>> 'this'.split(', ')
['this']
>>> ''.split(', ')
['']

Look, Ma!  No empty lists!  Since a split will always have at least one item in the result, you don't need to try to get the first item.  It will always be there for you.  Watch this:

>>> [][1:]
[]
>>> ' '.join([])
''

Look, Ma!  No errors!  A slice never throws errors, and a ' '.join() will return '' when the argument is empty.  That means that you need only one try block for first and middle.

An except block should specify what it is expecting.  A bare except is dangerous. I have found myself caught when I have done that because I can't even interrupt the script with Ctrl+C.  If it isn't an IndexError that trips the except, we want to know about it.  We don't want to hide it under an except.

Your naming does not comply with PEP 8 (the Python style guide).  It says to use lowercase_with_underscores for function and variable names.  Besides that, csSplit is a little hard to understand.  Sure, one can recognize cs as being short for comma-separated if one thinks about it,  but I prefer something a little bit easier to understand.  Keeping the same spirit, why not comma_split?  I don't really like that name, but it seems a little easier to understand that csSplit.

It doesn't make much difference with short strings, but first and middle both perform the same split operation.  To speed things up, make the split only once by assigning a variable to it.

Why the intermediate variable partsDict?  We know that the function returns a dictionary of parts by its name and doc string.  Therefore, the variable cannot be for clarity.  It is longer to use than a simple return, so it isn't for line length.  Why then?  Just return directly.

The code ends up looking like this:

def get_name_parts(name):
    """Return a dictionary of a name's parts.
    name: e.g. 'JOHNSON, John Roberts, Jr. (Jack)'
    Ignores nicknames.
    """
    comma_split = name.split(', ')
    last = comma_split[0]

    try:
        first_mid = comma_split[1].split(' ')
    except IndexError:
        first_mid = ['']

    first = first_mid[0]
    middle = ' '.join(first_mid[1:])

    try:
        suffix = comma_split[2].split(' ')[0]
    except IndexError:
        suffix = ''

    return {'first': first,
            'last': last,
            'middle': middle,
            'suffix': suffix}

Is it shorter?  Not really.  It is only two or three lines shorter, but it is clearer, less repetitive, and safer.  Happy coding!


Const wrapper for iterator Edit: added clarification for why I want this, and updated the code since I don't have any answers yet
I have a C++11 array-like class which (can be) a wrapper around a random-access iterator.  Index-based access and .begin()/.end() can just pass through to the iterator, but there's a bit of complication when the object is const:
    template<class Size, typename DataIterator>
    class Storage : public Size {
        DataIterator iterator;
    public:
        Storage(const DataIterator &iterator, const Size &size) : Size(size), iterator(iterator) {}

        auto operator[](index_t i)
                -> decltype(iterator[i]) {
            return iterator[i];
        }
        auto operator[](index_t i) const
                -> MakeConst<decltype(iterator[i])> {
            return iterator[i];
        }

        DataIterator begin() {return iterator;}
        ConstWrapper<DataIterator> begin() const {return iterator;}
        DataIterator end() {return iterator + this->size();}
        ConstWrapper<DataIterator> end() const {return iterator + this->size();}
    };

If we simply returned DataIterator from the const version of .begin() and .end(), then people holding a Storage const & would (incorrectly!) be able to modify the array through that iterator.
Containers like std::vector have two separate iterators (::iterator and ::const_iterator), but our Storage class only has the read-write one, so we synthesise one using ConstWrapper:
    template <typename Iterator>
    class ConstWrapper {
        Iterator iterator;

        using traits = std::iterator_traits<Iterator>;
    public:
        using difference_type = typename traits::difference_type;
        using value_type = typename traits::value_type;
        using pointer = ConstWrapper;
        using reference = MakeConst<typename traits::reference>;
        using iterator_category = typename traits::iterator_category;

        ConstWrapper() {}
        ConstWrapper(const Iterator &iterator) : iterator(iterator) {}

        // The problematic cases:
        auto operator[](index_t i) const
                -> MakeConst<decltype(iterator[i])> {
            return iterator[i];
        }
        auto operator*() const
                -> MakeConst<decltype(*iterator)> {
            return *iterator;
        }

        bool operator!= (const ConstWrapper& other) const {
            return iterator != other.iterator;
        }
        /** All the other random-access iterator methods **/
    };

    // Specialisation to prevent infinite loops
    template <typename Iterator>
    class ConstWrapper<ConstWrapper<Iterator>> : ConstWrapper<Iterator> {
    public:
        using ConstWrapper<Iterator>::ConstWrapper;
    };

The implementation forwards all the relevant methods, and has a specialisation for ConstWrapper<ConstWrapper<...>> so that it can't wrap itself.
A key part is MakeConst, which turns types into the correct const variant (unlike simply adding const, which has no effect on references):
    // Converts (T & -> T const &), and (T -> const T)
    using MakeConst = typename std::conditional<
        std::is_reference<T>::value,
        typename std::remove_reference<T>::type const &,
        const T
    >::type;

Does this make sense, and is it OK?  Is there something else I could be doing which is more readable/efficient/etc.?
Thanks!

I don't think your specialization is doing what you think it's doing. https://godbolt.org/z/Gfh7sv
You don't want ConstWrapper<ConstWrapper<Iterator>> to inherit from ConstWrapper<Iterator>; that would mean that a CW<CW<I>> "is-a-kind-of" CW<I>, which isn't true. You don't want an inheritance relationship here. What you want is simply for specializations of Storage to not wrap DataIterators that happen to be specializations of ConstWrapper already. The way you do that is with an extra layer of alias indirection:
template<class T> struct maybe_constwrap { using type = ConstWrapper<T>; };
template<class U> struct maybe_constwrap<ConstWrapper<U>> { using type = ConstWrapper<U>; };

template<class Size, typename DataIterator>
class Storage : public Size {
public:
    using iterator = DataIterator;
    using const_iterator = typename maybe_constwrap<DataIterator>::type;
    ~~~
};

Now, if DataIterator is already a ConstWrapper, then both iterator and const_iterator will be literally the same type... which is what you want.

Your MakeConst seems to be predicated on the dubious idea that const _Bit_reference is not going to be assignable-to. That's likely to change in C++23. See https://stackoverflow.com/questions/63412623/should-c20-stdrangessort-not-need-to-support-stdvectorbool for some context.

You repeat some metaprogramming in the return types of operator* and operator[]. They should just return the type reference which you have already computed above.
reference operator*() const { return *it_; }

Incidentally, I'm changing your data member's name from iterator to it_, because I suspect it is a terrible idea, Concepts-wise, to have a container-ish class type with a member named iterator which member is (A) not public, and (B) not a type.

Your operator!= should probably use the hidden friend idiom:
friend bool operator==(ConstWrapper a, ConstWrapper b) { return a.it_ == b.it_; }
friend bool operator!=(ConstWrapper a, ConstWrapper b) { return a.it_ != b.it_; }

In C++20 you could theoretically omit operator!= and provide operator== only. I don't know yet if that's a good idea.
Passing ConstWrapper by value should be fine, because it holds only an iterator, and iterators are cheap to copy.