Solving Project Euler problem #48 in Scala I'm trying to learn some Scala and decided to try to tackle some Project Euler problems.

For problem #48, coming from a Python background, my solution is the following one-liner:

print ( (1 to 1000).map(i => BigInt(i).pow(i)).sum % BigInt(10).pow(10) )

Is this idiomatic? Is there a simpler/more readable solution?

Your solution is clean but doesn't scale well. A well known optimization for a^k mod m is to perform all computations modulus m. If m is sufficiently small, we furthermore can switch to native types (this is not the case here !).

val n = 1000
val m = BigInt(10).pow(10)
(for (i <- 1 to n) yield BigInt(i).modPow(i,m)).sum % m

Rounded avarage timing results (Scala 2.9.1 with -optimize) :

n                     1000     | 10000
Initial solution       4.8 ms  |  6700 ms
Fold left              4.8 ms  |  6700 ms
My humble version      0.5 ms  |     5 ms


Insert str into larger str in the most pythonic way I want to insert a small str into a larger one in the most pythonic way.

Maybe I missed a useful str method or function ? The function that insert the small str into the larger one is insert_tag.

I have tried something with reduce but it didn't work and it was too complex.

Code:

from random import choice, randrange
from typing import List

def insert_tag(text: str, offsets: List[int], tags: List[str]) -> str:
    text: str = text
    for offset, tag in zip(offsets, tags):
        # text = f'{text[:offset]}{tag}{text[offset:]}'  # solution 1
        text = text[:offset] + tag + text[offset:]  # solution 2
    return text

if __name__ == '__main__':
    tag_nbr = 30
    # Text example
    base_text = 'Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do ' \
                'eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut ' \
                'enim ad minim veniam, quis nostrud exercitation ullamco laboris ' \
                'nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor ' \
                'in reprehenderit in voluptate velit esse cillum dolore eu fugiat ' \
                'nulla pariatur. Excepteur sint occaecat cupidatat non proident, ' \
                'sunt in culpa qui officia deserunt mollit anim id est laborum.' \
        .replace(',', '').replace('.', '')
    # Random list of tag only for testing purpose
    tag_li: List[str] = [f'<{choice(base_text.split())}>' for _ in range(tag_nbr)]
    # Ordered list of random offset only for testing purpose
    offset_li: List[int] = [randrange(len(base_text)) for _ in range(tag_nbr)]
    offset_li.sort(reverse=True)
    # Debug
    print(tag_li, offset_li, sep='\n')

    # Review
    new_text: str = insert_tag(base_text, offset_li, tag_li)
    # End Review

    # Debug
    print(new_text)

Output:

"Lorem ips<consectetur>u<Lorem>m dolo<Lorem>r sit amet consecte<Lorem>tur <nostrud>adipiscing eli<aute>t sed d<anim>o eiusmod tempo<voluptate>r <exercitation>incid<anim>i<non>dunt ut l<quis>abor<do>e et dol<reprehenderit>ore magna aliqua Ut enim ad minim veniam quis nostrud exercitatio<est>n ullamco laboris nisi ut aliquip ex<nulla> ea <consectetur>commodo consequat Duis aute irure dolor in repreh<deserunt>enderit in voluptate vel<sint>it esse cillum dolore eu f<qui>ugiat nulla pariatur <magna>Excepteur sin<ex>t occaecat cup<labore>i<ut>datat non proident sun<est>t<in> in cul<fugiat>pa qui officia<enim> deserunt mollit anim i<in><anim>d est laborum"

PS: The rest of the code is not my primary concern, but if it can be improved, tell me, thanks.

The core code (I'd prefer the f-string if you can guarantee Python 3.6+, else the addition or "{}{}{}".format(...) if you need to support older Python versions) looks fine. However, you overwrite text each loop, but continue to use the originally provided offset's, which could be very confusing since the offsets will become incorrect after the first tag insertion. The code doesn't crash, but I'd argue it doesn't "work", certainly not as I'd intuitively expect:

In [1]: insert_tag('abc', [1, 2], ['x', 'y'])
Out[1]: 'axyyxbc'

I'd expect this to produce 'axbyc'. If that output looks correct to you, ignore the rest of this answer.

It would be better to break the original string apart first, then perform the insertions. As is typical in Python, we want to avoid manual looping if possible, and as is typical for string generation, we want to create as few new strings as possible. I'd try something like the following:

In [2]: def insert_tags(text: str, offsets: [int], tags: [str]):
   ...:     offsets = [0] + list(offsets)
   ...:     chunks = [(text[prev_offset:cur_offset], tag) for prev_offset, cur_offset, tag in zip(offsets[:-1], offset
   ...: s[1:], tags)]
   ...:     chunks.append([text[offsets[-1]:]])
   ...:     return ''.join(piece for chunk_texts in chunks for piece in chunk_texts)

In [3]: insert_tags('abc', [1, 2], ['x', 'y'])
Out[3]: 'axbyc'

Other stylistic considerations:

Your function supports the insertion of multiple tags, so it should probably be called "insert_tags" (plural) for clarity.
Type annotation are generally good (though recall that they lock you in to newer versions of Python 3.X), but getting them right can be a nuisance. E.g., in this case I'd suggest using Sequence instead of List, since you really just need to be able to loop on the offsets and tags (e.g., they could also be tuples, sets, or even dicts with interesting keys). I also find them to often be more clutter than useful, so consider if adding type annotations really makes this code clearer, or if something simpler, like a succinct docstring, might be more helpful.


Random number generator in C# I don't know if it is any good. I've never done this, thus, I noticed it is kinda slow.

Can you suggest improvements? I may use it in actual software in the future.

private int secgen(int minvalue, int maxvalue)
    {
        Func<int, int, int, int> modular = (a, b, mod) =>
        {
            long rem;
            Math.DivRem((long)a + (long)b, (long)mod, out rem);

            if (rem < minvalue)
                rem += minvalue;

            return (int)rem;
        };

        byte[] entropyBytes = new byte[257];
        RNGCryptoServiceProvider rng = new RNGCryptoServiceProvider();

        rng.GetBytes(entropyBytes);
        int value = entropyBytes[0];
        for (int i = 1; i < 257; i++)
        {
            value = modular(value, entropyBytes[i], maxvalue);

        }

        //MessageBox.Show(value.ToString());
        return value;
    }

You may improve performance:

I would use the modulo operator instead of a function call you aren't benefiting from 100%. Math.DivRem returns a value (quotient) that you don't use. Same result:

var rem = a + b % mod;

Saves you a few unboxing operations and method call

Func<int, int, int, int> modular = (a, b, mod) =>
{
    var rem = a + b % mod;

    if (rem < minvalue)
        rem += minvalue;

    return rem;
};

Saves half the time? http://pastebin.com/XTaLCQh4


Speeding up the process of obtaining data I created this script to obtain data from graphs online, from monavisaxmldata. I were wondering if there was a way to obtain the data faster. The program which uses this data gets presented in a presentation which takes roughly 5 minutes. But the data already takes 1.5 minutes to load. Which is quiet a bite in such a short time period.

Is the WebClient() optimized in speed or should I approach the handling of the information differently?

using UnityEngine;
using System.Collections;
using System.Collections.Generic;
using System.Net;
using System.Web;

public class PVPanels_Actueel : MonoBehaviour
{
    private static WebClient myWebClient = new WebClient();
    private List<KeyValuePair<string, List<MonavisaXmlData>>> DataList = new List<KeyValuePair<string, List<MonavisaXmlData>>>();
    private List<MonavisaGraphData> graphReadyData = new List<MonavisaGraphData>();
    public event System.Action<List<MonavisaGraphData>> PVPanelsActDataParsed = delegate { };

private void Start()
{
    myWebClient.DownloadStringCompleted += onDownloadComplete;
}
public static void StartDownload(int day, int month, int year)
{
    string dateAndTime = string.Format("{0}-{1}-{2}+23:00", day, month, year);
    MonavisaRequestForm myRequest = new MonavisaRequestForm
        (
            "foo",
            "bar",
            string.Format("http://www.monavisa.info/CreateGraphData?graphs=1&graph[0]=1226&todate={0}&period=1&step=0&b_id=194&inter=1&other_graph=true", dateAndTime),
            ref myWebClient
            );
    MonavisaFetch.instance.getData(ref myRequest);
}

private void onDownloadComplete(object sender, DownloadStringCompletedEventArgs e)
{
    DataList.Clear();
    DataList = MonavisaParse.Parse(e.Result);
    graphReadyData.Clear();

    float totalKwh = 0;
    float totalMw2 = 0;
    string latestDate = "";
    for (int i = 0; i < DataList[1].Value.Count; i++)
    {
        float f0 = float.Parse(DataList[0].Value[i].value);
        float f1 = float.Parse(DataList[1].Value[i].value);
        float f2 = float.Parse(DataList[2].Value[i].value);
        float f3 = float.Parse(DataList[3].Value[i].value);

        if (f0 > 0)
        {
            totalMw2 += f0;
            latestDate = DataList[0].Value[i].date;
        }

        if (f1 > 0 || f2 > 0 || f3 > 0)
        {
            totalKwh += f1 + f2 + f3;
        }
    }

    graphReadyData.Add(new MonavisaGraphData { date = latestDate, value = totalMw2 });
    graphReadyData.Add(new MonavisaGraphData { date = latestDate, value = totalKwh });
    PVPanelsActDataParsed.Invoke(graphReadyData);
}
}

The data is stored on a dutch site. you can go to this link use the log in info DWA-demo and password = welkom (this is a sample login given by the site itself) to see how the data structure is stored. What I currently request is a bit of a different graph, which is stored in a XML file. This file contains the values

Period where:

HOUR = 0;
DAY = 1;
WEEK = 2;
MONTH = 3;
TWO_MONTHS = 4;
THREE_MONTHS = 5;
SIX_MONTHS = 6;
YEAR = 7;
FIVE_YEAR = 8;

and a step where

HOUR = 0;
DAY = 1;
WEEK = 2;
MONTH = 3;
TWO_MONTHS = 4;
THREE_MONTHS = 5;
SIX_MONTHS = 6;
YEAR = 7;
FIVE_YEAR = 8;

And currently I am listing graph1&graph[0] which is 1 day per hour of dateAndTime, which today and the last round hour.

string.Format("http://www.monavisa.info/CreateGraphData?graphs=1&graph[0]=1226&todate={0}&period=1&step=0&b_id=194&inter=1&other_graph=true", dateAndTime),

This situation should be repeatable by putting a XML on a server/site where you first have to login, and then grab the data from the XML through requests.

It looks fast enough to me, nothing screams "major slowdown" in your code, so I would recommend putting a profiler on your code to spot any remaining hotpoints. After that, it's a case of damage control to try to reduce the impact of the data transfer time. As such, consider starting the download as early as possible.

On other matters:

Use var when the right hand side of your declaration makes the type obvious:

string dateAndTime = string.Format("{0}-{1}-{2}+23:00", day, month, year);

should be:

var dateAndTime = string.Format("{0}-{1}-{2}+23:00", day, month, year);

The reason for doing this is that should you change the type of the variable, you only have to change it in one place.

I'm also seeing a few magic strings and numbers that it might be wise to extract as constants or readonly instance variables.

I also don't like your short variable names for your float datapoints. Firstly, if there's nothing that distinguishes them other than the order in which you read them, store them as an array, although I suspect this is not the case, because f0 is given special treatment in the following code.

Secondly, to a maintenance programmer f1 means nothing, unless you're referring to a high-speed motor race. Call them dataPoints if you must, but f1 is extremely unhelpful.

I also note a couple of worrisome points about your implementation:

You don't seem to handle any potential errors at all. What if you can't make a connection, what if the connection drops half way through? Looking at your code literally nothing would happen.

You also only clear your graphReadyData variable when the download completes. If the download fails, this variable will contain the previous correctly-downloaded data. This has the potential to cause errors if not for yourself then for a maintenance programmer, so I would recommend clearing it when you begin the download.


Student sorting based on criteria This code takes id, name and marks of 3 subjects for 5 students from user base, calculates the total of 3 subjects per student, and then takes a criteria level with which to sort; level 1 sorting does sorting based on 1 criteria only, while level 2 will sort based on criteria 1 and if similar occurs (like if select level 1 criteria marks and if marks of two student are same) sorting of that pair will be done based on criteria 2, if either user have selected level 2 and criteria 2.

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

struct student
{
int id;
 int sub[3];
int total;
char name;
}s[5];
void sort1(struct student *[],int,int,int);
main()
{       int i,j,ch,c1,c2,t;
    char choice;
for(i=0;i<5;i++)
{
    s[i].total=0;
printf("Enter the id of s[%d]\n",i+1);
scanf("%d",&s[i].id);
printf("Enter the name:\n");
scanf("%s",&s[i].name);
    for(j=0;j<3;j++)
    {
    printf("Enter the marks of sub[%d] of s[%d]\n",j+1,i+1);
    scanf("%d",&s[i].sub[j]);
    s[i].total+=s[i].sub[j];
    }
}
struct student *p[5];
for(i=0;i<5;i++)
{
    p[i]=&s[i];
}
printf("You wanna sort or not?\n");
scanf("%c",&choice);
if((choice == 'y') || (choice == 'Y'))
{
    printf("Which level sorting 1 or 2 ?\n");
    scanf("%d",&ch);
    if(ch==2)
    {
        t=1;
        printf("Criteria:\t1)By name\n2)By Id\n3)By total\nEnter level1 &level 2criteria:\n");
        scanf("%d%d",&c1,&c2);
        sort1(p,t,c1,c2);
    }
    else
    {   t=0;
        printf("Enter the level 1 criteria:\n");
        scanf("%d",&c1);
        sort1(p,t,c1,10);
    }
}
}
void sort1(struct student *p[5],int t,int cri1,int cri2)
    {
int i,j;

struct student *pt;

for(i=0;i<5;i++)
{
    for(j=0;j<5;j++)
    {
        if((t==1)&&(cri1==1) && (strcmp(&p[i]->name,&p[i+j]->name)>0))
        {
            pt=p[i];
            p[i]=p[i+j];
            p[i+j]=pt;
        }
        else if((t==1) &&(cri2==2)&& (strcmp(&p[i]->name,&p[i+j]->name)==0))
        {
            if(p[i]->id<p[i+j]->id)
            {
                pt=p[i];
                p[i]=p[i+j];
                p[i+j]=pt;
            }
        }
        else if((t==1)&&(cri2==3)&&(strcmp(&p[i]->name,&p[i+j]->name)==0))
        {
            if(p[i]->total<p[i+j]->total)
            {
                pt=p[i];
                p[i]=p[i+j];
                p[i+j]=pt;
            }
        }
        else if((t==0)&&(cri1==1) && (strcmp(&p[i]->name,&p[i+j]->name)>0))
        {
            pt=p[i];
            p[i]=p[i+j];
            p[i+j]=pt;
        }
        else if((t==0) &&(cri1==2)&& (strcmp(&p[i]->name,&p[i+j]->name)>0))
        {
            if(p[i]->id<p[i+j]->id)
            {
                pt=p[i];
                p[i]=p[i+j];
                p[i+j]=pt;
            }
        }
        else if((t==0)&&(cri1==3)&&(strcmp(&p[i]->name,&p[i+j]->name)>0))
        {
            if(p[i]->total<p[i+j]->total)
            {
                pt=p[i];
                p[i]=p[i+j];
                p[i+j]=pt;
            }
        }

    }
}
for(i=0;i<5;i++)
{
    printf("Id\tName\tS1\tS2\tS3\tTotal\n\n%d\t%s\%d\t%d\t%d\t%d\n",p[i]->id,p[i]->name,p[i]->sub[0],p[i]->sub[1],p[i]->sub[2],p[i]->total);
}
}

indentation

The most obvious weirdness about your code is the inconsistent indentation. Indeed I was convinced that there was a missing } at the end until I scanned the whole thing much more carefully. Indent everything within a function one level and add additional levels at each new nested statement. (user1118321 has generously provided a version with corrected indentation)

long lines

Very long lines are hard to read. This one, requires me to scroll to read the whole thing. So I can't even see the whole function call all-at-once.

    printf("Id\tName\tS1\tS2\tS3\tTotal\n\n%d\t%s\%d\t%d\t%d\t%d\n",p[i]->id,p[i]->name,p[i]->sub[0],p[i]->sub[1],p[i]->sub[2],p[i]->total);

You can fold this nicely over multiple lines.

    printf("Id\tName\tS1\tS2\tS3\tTotal\n\n%d\t%s\%d\t%d\t%d\t%d\n",
           p[i]->id,p[i]->name,
           p[i]->sub[0],p[i]->sub[1],p[i]->sub[2],
           p[i]->total);

You can even break-up long strings and let the compiler concatenate them for you.

    printf("Id\tName\t" "S1\tS2\tS3\t" "Total\n\n"
           "%d\t%s\t"   "%d\t%d\t%d\t" "%d\n",
           p[i]->id,p[i]->name,
           p[i]->sub[0],p[i]->sub[1],p[i]->sub[2],
           p[i]->total);

Incidentally, I found a missing "t" in there while doing this.

index origin

For output you add 1 to the index, but for input you do not subtract 1. This is bizarre. It would be more natural in a C program to use 0 throughout. The first element is element 0. If you need to, you can document it for your users or even announce it at program startup.

interactive/batch

A more useful program would be one that operates on data-files and accepts command-line arguments as instructions. This saves you lots of typing as you test your program. You can type all the data just once and run your program many times. You can test different options by writing a shell-script or batch-file which runs the program different ways.

You could define a text-representation of your data. A common way is to use : as a separator

Student,Lastname:25:37:44:55
Student,Noname:0:0:0:99
Studentka,Foxy:101:110:150:1000

Then your program could accept the filename and a code or option as an argument.

int main(int argc, char **argv){
    if (argc < 3) {
        printf("too few arguments\n"
               "usage: %s filename option\n"
               "where option is 0 1 2 or 3\n");
    }

    struct student *data = loadfile(argv[1]);
    perform_sort(data, strtol(argv[2],NULL,10));
    print_data(data);
}

It would be good to separate the different parts into different functions, but this is easier to see when you try to make it work with files. You could also accept - as a special file-name that means stdin, and still run your program
interactively. You can trigger an End-Of-File condition with ctrl-D (Unix, OSX) or ctrl-Z (Windows) to tell your program where the data ends. This is a common behavior for command-line utilities.

dynamically resizing arrays

In order to approach what I'm describing here, it would be good to build the student database dynamically.

student *data = NULL;
int data_n = 0;

data = malloc((data_n=10) * sizeof *data);

A couple of things to note. I assign the size and allocate the array in the same statement. I don't know if everybody does this, but it helps me make sure that the two variables always change together. It would be good to check the result of malloc for a NULL and do something sensible such as exit(1), but for very simple programs you can usually get away with just assuming malloc will return the memory. This is a fragile behavior and the program is not correct per se, but memory-management is a complicated subject to learn, so it's also good to keep things simple until there is a need to make them complicated.

So what happens if you're reading line number 11? Every time the loadfile function will read a line, it will first check that i < data_n and if not,
then it must first

data = realloc(data, (data_n *= 2) * sizeof *data);

to make more room. When you finish reading data, you should make sure there is still some room (i < data_n) and then add a sentinel record to indicate the end of data for the other functions. This could be a student with the name "\0" perhaps.

char ... pointer?

char name;

//...

scanf("%s",&s[i].name);

Unless the name is 0 characters long, that's not enough memory for a name. You could set a maximum size, which is perhaps the easiest way.

char name[100];

Or you could use a pointer to a string, but you must allocate memory for it, and for that it is still easiest to use a fixed-size buffer and then use the POSIX function strdup to make a dynamic copy of the string.


Salt generation in C# I have designed a small method to generate a salt for a password. All this does is create a random salt and does nothing to add it to the password. I have a few questions regarding my very simple method:

Is this a secure generator?
Currently, it encodes in base64. Is this an issue in any way?
Are there any other potential issues? How could I improve this in terms of security, speed, etc...

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using System.Text;
using System.Security.Cryptography;

namespace Test
{
    public class Program
    {
        // Here is a method to generate a random password salt
        private static string getSalt()
        {
            var random = new RNGCryptoServiceProvider();

            // Maximum length of salt
            int max_length = 32;

            // Empty salt array
            byte[] salt = new byte[max_length];

            // Build the random bytes
            random.GetNonZeroBytes(salt);

            // Return the string encoded salt
            return Convert.ToBase64String(salt);
        }

        static void Main(string[] args)
        {
            System.Console.WriteLine(getSalt());
            System.Console.ReadKey();
        }
    }
}

var
You should use them with max_length and salt too. If the type is obvious from the right hand side of the assignment you should use var.
From here

Under what circumstances is it necessary for a variable's type to be clearly understood when reading the code? Only when the mechanism of the code -- the "how it works" -- is more important to the reader than the semantics -- the "what its for".

So basically in a line like
byte[] salt = new byte[max_length];

or
int max_length = 32;

the type of the variables does not add any value to the code. It is too obvious what the assigned type is, using the type instead of var only adds noise to the code without any real value.
Disposing
If an object implements IDisposable you should either call its Dispose() method or enclose it in an using block.
Naming
Also variables local to a method aren't mentioned in the naming guidelines I would suggest to use camelCase casing for naming them instead of snake_case casing.
For naming private methods you should use PascalCase casing.
Comments
Comments should only describe why something is done. Let the code itself explain what is done by using meaningful and readable names.
A very good answer about comments can be found here: https://codereview.stackexchange.com/a/90113/29371
getSalt()
You should allow to pass the max_length to the method instead of hardcoding it. This has the advantage that a change to this value won't need the class class/method to be changed. If you make it an optional parameter you can still call it like it whould have no parameter.
Edit
Based on the valid comment from Johnbot you should better use a overloaded GetSalt() method instead of using an optional parameter.
I don't see the need for converting to base64. Encryption algorithms use byte[] arrays, so you should better just return the byte[].

Applying the above would lead to
private static int saltLengthLimit = 32;
private static byte[] GetSalt()
{
    return GetSalt(saltLengthLimit);
}
private static byte[] GetSalt(int maximumSaltLength)
{
    var salt = new byte[maximumSaltLength];
    using (var random = new RNGCryptoServiceProvider())
    {
        random.GetNonZeroBytes(salt);
    }

    return salt;
}


Four algorithms to find the Nth Fibonacci number I'm implementing some basic algorithms in Go as an introductory exercise. Here are four different algorithms to find the Nth Fibonacci number.

I'm looking for general feedback, but I'm specially interested in the Go idiom. Are the algorithms implemented idiomatically? If not, how can I correctly use the Go idiom to implement them?

Any other feedback you can think of is also welcome.

// Algorithms to calculate the nth fibonacci number

package main

import (
    "fmt"
    "math"
)

func main() {
    for i := 0; i <= 20; i++ {
        fmt.Println(fibonacci(i))
        fmt.Println(fibonacciRecursive(i))
        fmt.Println(fibonacciTail(i))
        fmt.Println(fibonacciBinet(i))
        println()
    }
}

// Iterative
func fibonacci(n int) int {
    current, prev := 0, 1
    for i := 0; i < n; i++ {
        current, prev = current + prev, current
    }
    return current
}

// Recursive
func fibonacciRecursive(n int) int {
    if n < 2 {
        return n
    }
    return fibonacciRecursive(n - 1) + fibonacciRecursive(n - 2)
}

// Tail recursion
func fibonacciTail(n int) int {
    return tailHelper(n, 1, 0)
}
func tailHelper(term, val, prev int) int {
    if term == 0 {
        return prev
    }
    if term == 1 {
        return val
    }
    return tailHelper(term - 1, val + prev, val)
}

// Analytic (Binet's formula)
func fibonacciBinet(num int) int {
    var n float64 = float64(num);
    return int( ((math.Pow(((1 + math.Sqrt(5)) / 2), n) - math.Pow(1 - ((1 + math.Sqrt(5)) / 2), n)) / math.Sqrt(5)) + 0.5 )
}

For tailHelper(), the term == 1 case is superfluous and should be eliminated.

In fibonacciBinet(), the quantity \$\frac{1 + \sqrt{5}}{2}\$ appears twice.  Since that quantity is known as the Golden Ratio, I would define an intermediate value

var phi = (1 + math.Sqrt(5)) / 2;

However, none of these four solutions is particularly idiomatic for Go.  The most expressive way to write a Fibonacci sequence in Go is as an iterator, such as this one.


List of variables, split based on pattern In my application i keep repeating writing this pattern. I just cant figure out how to write it better. Coming to you for suggestions:

        if ($_POST['x'] = 1) {add_remove($one, array($two,$three,$four,$five));}
        if ($_POST['x'] = 2) {add_remove($two, array($one,$three,$four,$five));}
        if ($_POST['x'] = 3) {add_remove($three, array($one,$two,$four,$five));}
        if ($_POST['x'] = 4) {add_remove($four, array($one,$two,$three,$five));}
        if ($_POST['x'] = 5) {add_remove($five, array($one,$two,$three,$four));}

You can extract the value from the array before hand with something like:

$arr = array($one, $two,$three,$four,$five);

&first = $arr[$_POST['x']-1];

unset($arr[$_POST['x']-1]);
$arr = array_values($arr);
add_remove($first, $arr);


CS50 Pset1 Greedy, change algorithm The purpose of the code is to calculate the least amount of coins needed to produce correct amount of change given a dollar or cent user input. I'm just starting out and appreciate any pointers in how this code could/should be optimized.

The code works but I feel there is some redundancy with all those "while" loops. Using modulo math was another suggestion in order to solve the problem but I felt more comfortable using this solution for now.

#include <stdio.h>
#include <cs50.h>
#include <math.h>

int main (void)
{

// Variables//
float change;
int num = 0;
int quarter = 25;
int dime = 10;
int nickel = 5;
int penny = 1;

// Input of change//
do
{
    printf("How much change is owed?  ");
    change = get_float();
}
while (change <= 0);

printf("%.2f\n",change);

// conversion from dollars to cents (need to look over use of rounding)

int cents = (change * 100);

//Coin count//

while ( cents >= quarter)
{
cents -= quarter;
num++;
}

while (cents >= dime)
{
cents -= dime;
num++;
}

while (cents >= nickel)
{
cents -= nickel;
num++;
}

while (cents >= penny)
{
cents -=penny;
num++;
}

printf("%d\n", num);
}

Be careful with that algorithm.
It looks pretty intuitive, but it would not work for some other coin values.
For example, if we need to deal 20 cents using coins of 17, 10 and 1, this greedy approach would give (17, 1, 1, 1) - 4 coins, whereas (10, 10) is just 2 coins.
Figuring out the difference - when your algorithm works and when it does not - would be an interesting exercise in itself.
Wasteful loops
while ( cents >= quarter) {
  cents -= quarter;
  num++;
}

You can replace it with something like:
num += cents / quarter;
cents = cents % quarter;

Note that if you put coin values in array, this can be done in a loop.
Generally, your code would spend most of its time waiting for user to type the number and will give answer immediately, there's little point in optimisation.
You should adopt some code style though, your indentation is inconsistent. Indentation makes code structure more immediately apparent. I don't think it's a problem for you now, but it would be a problem for you on bigger assignments and it would be a problem for other people who would have to read (and grade) your code. Take a look at one of these if your course does not have one.


Directly accessing class attributes in Rock Paper Scissors I am reading a beginning Python programming book and in it the author says that you want to avoid accessing a class' attributes directly, but instead to create methods that return attribute values. However, I am finding there are times this seems a little redundant and I am wondering if am just going about this in the wrong way.

An example of this is in the battle function where I access player1.hand directly. Is this a bad idea? Also, does everything else look okay in this or are there any bad practices I should be nipping in the bud?

#a game of rock paper scissors for two computers

#imports
import random

class Hand(object):
    """A hand with which to rock paper scissor"""

    HANDS = ("rock", "paper", "scissors")

    def __init__(self, name, wins = 0, hand = None):
        self.name = name
        self.wins = wins
        self.hand = hand

    def choose_hand(self):
        """Chooses hand for player"""
        self.hand = random.choice(self.HANDS)
        print self.name + " picks " + self.hand + "."

    def increase_wins(self):
        """Increases players wins and declares them this rounds winner"""
        self.wins += 1
        print self.name + " wins."

    def total_wins(self):
        """shows total wins of all rounds played"""
        print self.name + " won " + str(self.wins) + " rounds."

def battle(player1, player2):
    """plays one round of rock paper scissor"""
    #players choose their hands
    player1.choose_hand()
    player2.choose_hand()

    #see who wins the battle
    if player1.hand == "rock" and player2.hand == "scissors":
        player1.increase_wins()
    elif player1.hand == "paper" and player2.hand == "rock":
        player1.increase_wins()
    elif player1.hand == "scissors" and player2.hand == "paper":
        player1.increase_wins()
    elif player1.hand == player2.hand:
        print "It's a tie."
    else:
        player2.increase_wins()

def main():
    player1 = Hand("HAL")
    player2 = Hand("WOPR")

    rounds = 10

    for i in range(rounds):
        print "\nRound " + str(i + 1) + ":"
        battle(player1, player2)

    print "\nFinal Score:"
    player1.total_wins()
    player2.total_wins()
    if player1.wins > player2.wins:
        print player1.name + " wins."
    elif player1.wins == player2.wins:
        print "It's a tie."
    else:
        print player2.name + " wins."

#play the game
main()

As has been pointed out, overriding the __call__ method is not the best way to improve the readability of your code.

Using attributes is not, in itself, a problem.

What you may want to consider is a trick you can play with Rock/Paper/Scissors.

If you create an array in the order:

Rock -> Scissors -> Paper -> Rock

Then, if you have two hands, and they select a random 'hand' from the three options Rock, Scissors, Paper.... then, you can find the first index of the first hand's value in the size-4 array above.... then if the next value is the opponent's hand, you win. If it is the same as your hand, it's a draw. Otherwise it's a loss.

winconditions = ['Rock', 'Scissors', 'Paper', 'Rock']
p1index = winconditions.index(player1.hand)
p2index = winconditions.index(player2.hand)
if p1index == p2index:
    # draw
elif p1index + 1 == p2index:
    # p1 wins!
else
    # p2 wins.

Since this is probably the most common method to call when accessing the player, it may make sense to make the hand the result of the __call__ function ..... and it would make the code read pretty nicely....

def __call__(self):
    return self.hand

and then you can use it like:

def battle(player1, player2):
    """plays one round of rock paper scissor"""
    #players choose their hands
    player1.choose_hand()
    player2.choose_hand()

    #see who wins the battle
    if player1() == "rock" and player2() == "scissors":
        player1.increase_wins()
    elif player1() == "paper" and player2() == "rock":
        player1.increase_wins()
    elif player1() == "scissors" and player2() == "paper":
        player1.increase_wins()
    elif player1() == player2():
        print "It's a tie."
    else:
        player2.increase_wins()


NASM tutorial (Linux syscalls, 64 bit) code with CI-build and test According to this question,  I have made an agreement with Peter Cordes to create such a question here.

I think that I should prepare the tutorial using the modern technologies as the Continuous Integration (hereinafter referred to as "CI").

Here is the public repo (branch tests) and here is the successful CI-build with the tests.

The tests were written in C#/.NET Core, using the MSTest framework, on Ubuntu (CI also uses Ubuntu docker image).

The source code may be found here.

It was divided into 4 files:

functions.asm, the main source, which is working with the 64-bit
syscalls
sys_write.asm, the program, which using the SYS_WRITE
syscall
sys_write.loop.asm, the program, which using the SYS_WRITE
syscall repeatedly (10 times)
sys_read.asm, the program, which is using
the SYS_READ syscall

The corresponding tests (in C#) are located here.

Main source file (functions.asm):

SECTION .data
inputTemplate  db  "User input: ", 0h
maxBytesToRead equ 255

sprint:
    push rdx
    push rdi
    push rax
    push rsi

    mov  rax, rsi
    pop  rsi
    jmp  next_char
    ret

sprint_linefeed:
    call sprint

    push rsi
    mov  rsi, 0Ah
    push rsi
    mov  rsi, rsp

    call sprint
    pop  rsi
    pop  rsi
    ret

read_input:
    push rsi
    call kernel_fn_read_input
    mov  r8 , rsi
    mov  rsi, inputTemplate
    call sprint
    mov  rsi, r8
    call sprint
    pop rsi
    ret

quit:
    call kernel_fn_quit
    ret

next_char:
    cmp byte [rax], 0
    jz  count_length
    inc rax
    jmp next_char

count_length:
    sub rax, rsi
    mov rdx, rax
    jmp kernel_fn_print_string

kernel_fn_print_string:
    mov rax, 1
    mov rdi, 1
    syscall

    pop rax
    pop rdi
    pop rdx
    ret

kernel_fn_read_input:
    push rax
    push rdx
    push rdi

    mov rax, 0
    mov rdx, maxBytesToRead
    mov rdi, 0
    syscall

    pop rdi
    pop rdx
    pop rax
    ret

kernel_fn_quit:
    push rax
    push rdi

    mov rax, 60
    mov rdi, 0
    syscall

    pop rdi
    pop rax
    ret

Here, I'm providing the several functions for handling the SYS_WRITE and SYS_READ syscalls in 64-bit mode. The 1st remark, which I can make for myself, that I didn't use the namespaces, which act like the private methods in high-level languages, like:

subroutineA:
    mov     rcx, firstMsg
    call    sprint
    jmp     .finished

.finished:
    mov     rax, secondMsg
    ...

Peter has asked me:

  Why you designed it that way (with push / pop of all registers you use, instead of letting it clobber rax, rcx, rdx, rsi, rdi, and r8-r11 like the x86-64 SystemV ABI allows for the standard function calling convention?

My answer:

  I'm pushing it to the stack for safe keeping. When the function has finished it's logic, these registers should have their previous values, restored via the POP instruction. I'm achieving the result, that any values in the registers will be the same before and after. Also, I didn't read the SystemV ABI at that moment, but reading right now.

I'm new to x86/assembler/NASM world, so I want to hear your constructive critique, because later I shall post an answer to Peter Cordes' question in beginner-friendly manner.

Intended purpose of the future tutorial:

  The simple application with the CI service, which may help the newbie to learn the correct use of the int 0x80 (for 32-bit) & syscall (for 64-bit) Linux ABI, as I promise Peter. That's why CI-service IS IMPORTANT, because it will show the beginner the conflict/exception. I want to prepare the failure tests, which will show the beginner, what happens if to use the 32-bit int 0x80 Linux ABI in 64-bit code with the deep diagnostics. Repo is public, so the beginner may execute the job and view the results in live mode.

When the function has finished it's logic, these registers should have their previous values

The normal x86-64 System V calling convention allows functions to clobber RAX, RCX, RDX, RSI, RDI, and R8-R11.  Most of these registers are also used for passing args to functions, or returning them (RAX and sometimes RDX).
For a more readable guide to calling conventions, see Agner Fog's calling conventions PDF which covers the differences between calling conventions on Windows/non-Windows and how to deal with it.  (His optimization manuals are essential reading, too if you want to write efficient asm.)
Also, the standard calling convention passes the first arg in rdi.  You seem to be using rsi.  This needs to be documented with a comment.  (Your whole code is under-commented, even though most of it is straightforward).
All libc functions like strlen follow the normal calling convention, so writing code that takes advantage of the fact that your functions don't clobber some registers requires remembering the non-standard calling convention they use.
Using a non-standard calling convention that doesn't clobber any registers might be good for some obscure use-cases like optimizing for code-size in a function with many callers, or for a debug-print function.  You're making your functions bloated by forcing them to save/restore all registers.  Also, restoring even rax stops you from returning an error code (possible if stdout is closed).  I think your function is intended for use in toy programs that don't check for errors, though.  But still, allowing your functions to clobber rax is pretty sensible.
BTW, in most stack-args calling conventions (including args that don't fit in registers in the x86-64 System V convention used on Linux), functions are allowed to modify their args on the stack.  So it's not weird that functions are allowed to clobber the registers their args are in.
Also beware that inserting a debug-print function call isn't necessarily safe even if it restores all registers.  x86-64 System V defines a red zone of 128B below rsp that a function can assume isn't clobbered asynchronously (by signal handlers if you have any), so a function might have valuable data stored at [rsp - 8] or something.  Inserting a debug-print function call into a function that was previously a leaf function might break it (because call pushes a return address, overwriting memory below the old rsp).  This is why you should just use a debugger instead of writing debug-print functions.
Anyway, if you insist on save/restoring all registers, don't forget that syscall itself clobbers rcx and r11.  See What are the calling conventions for UNIX & Linux system calls on i386 and x86-64?.  Or maybe you're intentionally not saving r8-r11?  read_input writes r8 without saving it first.  That's fine, you can use whatever non-standard calling convention you want, but document it with comments.
Also, implement it efficiently: in sprint, you push / pop RSI around a mov rax, rsi.  You're only reading it, which doesn't modify rsi.
Also, you should wait until the end of the function to restore RSI.  If you're going to eventually save/restore registers inside your function, you might as well do it early.

Keep read-only string constants in section .rodata
section .data is for modifiable strings that go in read-write memory.  Use section .rodata for your inputTemplate.

Indentation / formatting style:
It's nice to indent the operands to a consistent column, so different length mnemonics don't make your code as ragged.  I've been doing this in code blocks in this answer.
Also don't forget to use : after labels even in .data / .rodata sections.

Make function definitions contiguous and use local labels for function internals.
next_char is not a usable entry-point to your code.  It's part of sprint.  So what you've done is put the first part of sprint early in memory, then code for another function, then the rest of sprint.  This is not efficient for hardware to run (I-cache locality and a jmp instruction to reach the 2nd half of the function), or easy for anyone to read.
Putting a ret after a jmp is particularly silly:
jmp  next_char
ret

because nothing jumps back to execute the ret instruction.  Anyway, much better to just put the next_char code right below the first part of sprint so execution falls into it.
Similarly, sys_exit is guaranteed not to return, so there's no point putting some pop and ret instructions after it.  Or push instructions before it.
If some of the blocks were helper functions, it could make some sense, but they're not.  (Although sprint_linefeed should use kernel_fn_print_string, see below.  Except that you can't really use it because it pops more than it pushes.  Having the save/restore built-in to blocks of code makes it hard to reuse them.)
For labels internal to a function, use .name instead of name to give it local scope.
You don't need to jmp over blank lines in the source
Execution continues to the next instruction on its own.  This is an example:
count_length:
    sub rax, rsi
    mov rdx, rax
    jmp kernel_fn_print_string

kernel_fn_print_string:
    mov rax, 1
    ....

The jmp instruction is completely useless and a waste of space.  You should have done this:
.count_length:
    sub    rax, rsi
    mov    rdx, rax          ; should have used rdx as the loop counter in the first place to save this mov
.kernel_fn_print_string:
    mov    eax, 1
    ....

Use more efficient instructions
mov eax,1 is smaller than mov rax,1, but exactly equivalent.  NASM will actually optimize it to mov eax,1, but the other NASM-syntax assembler (YASM) won't, and will emit the 7-byte mov r/m64, imm32 form of the instruction (with a REX prefix) instead of 5-byte mov eax, imm32.
xor eax,eax is the most efficient way to zero eax.  Use that instead of mov eax, 0.
Use push imm8 for small constants.  mov  rsi, 0Ah / push rsi can be just push 0xa.  (I prefer C-style 0xDEADBEEF hex constants, not MASM-style trailing-h).  sprint_linefeed is a good example of how bloated your calling convention forces your code to be.
sprint_linefeed:
    call   sprint

    push    rsi          ; this and the 2nd pop aren't needed if you can clobber caller's RSI
    push    0Ah
    mov     rsi, rsp
    call   sprint
    pop     rsi          ; clear the newline from the stack
    pop     rsi          ; restore RSI
    ret

Of course, you know you want to sys_write exactly one byte, so could just do that with a few instructions instead of calling sprint on a known-length string.  Or you could push the right registers and jmp to kernel_fn_print_string directly.  But that doesn't work because you need it to return to your code to pop rsi, and you can't call it because it pops more than it pushes before running a ret.
I guess you could make sprint do extra work (like save/restore rsi) so you could use it from other functions that use just the tail of the function to restore rsi and return.  This whole thing is a mess, and without knowing what you're aiming for (simple code, small code-size, fast?) I have no idea what to recommend.
Use do{}while() loop structure in asm whenever possible.
A normal idiomatic asm loop has one taken conditional branch at the bottom instead of a not-taken branch at the top and a jmp at the bottom.  Why are loops always compiled into “do…while” style (tail jump)?.  (Also, prefer calling strlen because it's a lot faster than a naive byte-at-a-time loop for strings more than a few bytes long.  glibc's strlen uses SSE2 pcmpeqb to look for the zero byte, and this)
.next_char:
    inc     rdx
    cmp     byte [rdx], 0
    jnz     .next_char
   dec     rdx       ; un-count the last byte.  Happens once outside the loop.  Or init rdx to -1 instead of 0 to avoid this.

When a loop might need to run zero times, you can put a test/branch outside the loop.  But strlen always looks at at least one byte.

Use global to export functions in the symbol table
global sprint makes it possible to use extern sprint and call sprint from a separately-assembled file.
Align your function entry-points for performance
If you're really going for code-size, then sure leave everything packed.  But normally you want to align 16 before a function so there's a full fetch-block of code to decode.  Some CPUs may do a 16B-aligned fetch that contains the target address when jumping, and if the branch target is near the end of that 16B then there are only a couple bytes to decode in the first cycle.

Comment your code, especially any magic numbers.
mov  eax, 1       ; __NR_write
mov  edi, 1       ; stdout
syscall           ; sys_write(1, buf, len)

You can optimize by using mov edi, eax instead of another mov-immediate for a constant that happens to be the same.
read_input requires a fixed-size 255 byte buffer instead of having the caller pass a size?!?
This is just weird.  Write a function with 2 args: pointer + length, like a normal person.
read_input assumes that it can strlen() the buffer
read_input uses sprint to print the buffer as an implicit-length string.  But sys_read doesn't append a trailing zero, so this is only safe if the caller zeroed the buffer so there's a terminating zero somewhere.
If your intent was to print was you got from sys_read, you should use the length return value in rax instead of examining memory.  If your intent was to print out the buffer as an implicit-length string, then document that carefully (because it's weird).
Things would be simpler if you wrote a wrapper function that printed explicit-length strings.  You could use that here, and from sprint_linefeed.
Return the sys_read result from read_input
Otherwise the caller can't tell how many bytes were read.  The input bytes could be the same as what was already in the buffer.  (Even on an interactive TTY, a user can type ^V to escape any literal byte including a zero byte.)  (See this SO Q&A for example.

sys_write may return early for very large buffers
Nothing guarantees that sys_write will write all of the characters you pass it.  For small strings this is usually fine, but see When does the write() system call write all of the requested buffer versus just doing a partial write?.  e.g. if writing to a pipe, you're unlikely to be able to write more than the size of the kernel's pipe buffer in one system call.
If you want to be robust, check the sys_write return value to see if it's == the length, and if not advance the pointer and decrease the length and try again.  (If the return value wasn't an error code: -4096 to -1).

My version of your code
SECTION .rodata
inputTemplate:  db  "User input: ", 0h

;;maxBytesToRead equ 255

;;; void sprint(char *str) // in RSI
;;; {  sys_write(stdout, str, strlen(buf); }

;;; clobbers RAX and R11, everything else unmodified
;;; doesn't attempt to retry on interrupted or incomplete writes.

;;; returns sys_write return value in RAX unless string length was zero, then leaves RAX unmodified.

ALIGN 16
global sprint
sprint:
    cmp     byte [rsi], 0  ; don't even make a syscall at all with zero length
    jz      .no_print

    push    rdx
    push    rdi
    mov     rdx, rsi
.next_char:              ; first byte already checked earlier
    inc     rdx
    cmp     byte [rdx], 0
    jnz     .next_char

    sub rdx, rsi          ; string length (excluding terminator)

 kernel_fn_print_string:
    push    rcx          ; syscall clobbers RCX and R11.

    mov     eax, 1       ; __NR_write
    mov     edi, eax     ; stdout = 1 as well
    syscall              ; sys_write(stdout, buf, rdx)

     ;; TODO: check return value and retry if not all the bytes were written.

    pop     rcx
    pop     rdi
    pop     rdx
.no_print:
    ret

ALIGN 16
global sprint_linefeed
;;; sprint(rsi);  sprint("\n");
sprint_linefeed:
    call    sprint

    ; calling  kernel_fn_print_string  is inconvenient here because it wants to pop more than it pushes.
    ; could just inline that code
    ; or for small strings, would be more optimal to copy to a local buffer and append the newline to make one sys_write() call
    push    rsi
    push    0xa          ; '\n'
    mov     rsi, rsp
    call sprint
    pop     rsi          ; pop the newline
    pop     rsi          ; restore caller's RSI
    ret

ALIGN 16
global  read_input
;;; size_t read_input(buf=RSI, len=RDX)
;;; { length = sys_read(0, buf, len);
;;;   print it out
;;;   return length;
;;;  }
;;; clobbers RAX, R8-R11
read_input:
    push    rdi
    push    rcx          ; syscall clobbers RCX and R11

    xor     eax, eax     ; __NR_read
    xor     edi, edi     ; stdin = 0
    ; rsi = buf
    ; rdx = len
    syscall              ; Linux syscalls preserve everything but RCX and R11

    ;; TODO: check for error (rax = -1 to -4095)
    mov     r8, rax      ; save write return value
    mov     r9, rsi

    mov     esi,  inputTemplate    ; static data is in the low 32 bits
    call    sprint       ; print constant string (TODO: length is known, don't waste time on strlen)
    mov     rsi, r9
    call    sprint       ; length is known here, too, if buffer was zeroed before.

    pop     rcx
    pop     rdi
    mov     rax, r8
    mov     rsi, r9      ; restore caller's RSI

    ret

ALIGN  8        ; tiny function, almost fits in 8 bytes
global quit
quit:
kernel_fn_quit:
    mov   eax, 60       ; __NR_exit
    xor   edi, edi
    syscall             ; sys_exit(0)

As I said earlier, this is still a mess.  IDK what to recommend because IDK what this code is supposed to be useful for.
This still has some code-paths that save / restore and then save/restore the same register twice, e.g. sprint_newline, because it uses sprint internally multiple times.  If you care more about code-size than speed, then it can make sense to do it that way.


Constructing an odd-even mergesort sorting network I was searching for a non-recursive odd-even mergesort implementation and found this one:

Sedgewick R.

The resulting sorting network is not an odd-even-merge sort network. When one draws a diagram of the pairs, it can be seen that too many pairs are generated and others are missed.

So here is an image of a correct network with 32 inputs. A vertical line between 2 horizontal lines means compare value a[x] with a[y], if greater then swap the values in the array.

 (clickable)

The code from the book translated into C:

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

void sort(int l, int r)
{ int n = r-l+1;

  for (int p=1; p<n; p+=p)
    for (int k=p; k>0; k/=2)
      for (int j=k%p; j+k<n; j+=(k+k))
        for (int i=0; i<n-j-k; i++)
          if ((j+i)/(p+p) == (j+i+k)/(p+p))
              printf("%2i cmp %2i\n", l+j+i, l+j+i+k);
}
int main(char* argv, int args)
{ const int COUNT = 8;
  sort(0, COUNT-1);
}

See my StackOverflow question for more details and a diagram for the falsly generated network.

So I took the sorting network images for length = 1 << i and tried to write an algorithm that generates 2 indices x and y. These are needed to calculate the resource consumption of the sorting network in hardware.

I checked my function with length = 2, 4, 8, 16.

Here is my C code:

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

int log2ceil(int a)
{ int i;
  int cmp = 1;
  for (i = 0; cmp < a; i++)
  { cmp <<= 1; }
  return i;
}

void sort(int length)
{ int G = log2ceil(length);                      // number of groups
  for (int g = 0; g < G; g++)                    // iterate groups
  { int B = 1 << (G - g - 1);                    // number of blocks
    for (int b = 0; b < B; b++)                  // iterate blocks in a group
    { for (int s = 0; s <= g; s++)               // iterate stages in a block
      { int d = 1 << (g - s);                    // compare distance
        int J = (s == 0) ? 0 : d;                // starting point
        for (int j = J; j+d < (2<<g); j += 2*d)  // iterate startpoints
        { for (int i = 0; i < d; i++)            // shift startpoints
          { int x = (b * (length / B)) + j + i;  // index 1
            int y = x + d;                       // index 2
            printf("%2i cmp %2i\n", x, y);
          }
        }
      }
    }
  }
}
int main(char* argv, int args)
{ const int COUNT = 8;
  sort(COUNT);
}

Is it correct?
Can it be improved? E.g. just 4 loops.
It need not be very fast, because it's a hardware generation and check routine.

In general your code is quite hard to comprehend due to the bracing style and the single letter variable names. A bit more spaces and more meaningful names would do quite a bit for readability (I've kept your 2-indent spacing) and also remove the need for most of the comments:

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

int log2ceil(int value)
{
  int i;
  int cmp = 1;
  for (i = 0; cmp < value; i++)
  {
    cmp <<= 1;
  }
  return i;
}

void sort(int length)
{
  int groups = log2ceil(length);
  for (int group = 0; group < groups; group++)
  {
    int blocks = 1 << (groups - group - 1);
    for (int block = 0; block < blocks; block++)
    {
      for (int stage = 0; stage <= group; stage++)
      {
        int distance = 1 << (group - stage);
        int startPoint = (stage == 0) ? 0 : distance;
        for (int j = startPoint; j + distance < (2 << group); j += 2 * distance)
        {
          for (int i = 0; i < distance; i++)            // shift startpoints
          {
            int x = (block * (length / blocks)) + j + i;
            int y = x + distance;
            printf("%2i cmp %2i\n", x, y);
          }
        }
      }
    }
  }
}

int main(char* argv, int args)
{
  const int COUNT = 8;
  sort(COUNT);
}


Culling strings from a list that are substrings of other list elements I'm bothered by my answer to this SO question.
I'm pretty sure it's more efficient than the sort-and-cull implementations, but I'm having trouble expressing that in a way that I trust.
Also, it's rather convoluted by python standards.
Is there a way to express the same process more clearly, without pages of comments?
The problem statement:

Given a subject list of strings, make a list containing every element of the subject that isn't a substring of a different element of the subject.

Original Current solution:
from itertools import groupby
from operator import itemgetter
from typing import Dict, Generator, Iterable, List, Union

# Exploded is a recursive data type representing a culled list of strings as a tree of character-by-character common prefixes. The leaves are the non-common suffixes.
Exploded = None
Exploded = Dict[str, Union[Exploded, str]]

def explode(subject:typing.Iterable[str])->Exploded:
    heads_to_tails = dict()
    for s in subject:
        if s:
            head = s[0]
            tail = s[1:]
            if head in heads_to_tails:
                heads_to_tails[head].append(tail)
            else:
                heads_to_tails[head] = [tail]
    return {
        h: prune_or_follow(t)
        for (h, t) in heads_to_tails.items()
    }

def prune_or_follow(tails: List[str]) -> Union[Exploded, str]:
    if 1 < len(tails):
        return explode(tails)
    else: #we just assume it's not empty.
        return tails[0]

def implode(e: Exploded) -> Generator[str, None, None]:
    for (head, continued) in e.items():
        if isinstance(continued, str):
            yield head + continued
        else:
            for tail in implode(continued):
                yield head + tail

def cull(subject: List[str]) -> List[str]:
    return list(implode(explode(subject)))

print(cull(['a','ab','ac','add']))
print(cull([ 'a boy ran' , 'green apples are worse' , 'a boy ran towards the mill' ,  ' this is another sentence ' , 'a boy ran towards the mill and fell']))
print(cull(['a', 'ab', 'ac', 'b', 'add']))

I know that it works for the two three test cases.

All in all this looks good already.

A few tips:

continue

If you change the test in explode from if s: to

if not s:
    continue

You can save a level of indentation for the rest of the loop

collections.defaultdict

checking whether a key is in a dict, and adding it if it isn't is why there is collections.defaultdict which can simplify explode a lot:

def explode(subject: Iterable[str]) -> Exploded:
    heads_to_tails = defaultdict(list)
    for s in subject:
        if not s:
            continue
        heads_to_tails[s[0]].append(s[1:])
    return {h: prune_or_follow(t) for (h, t) in heads_to_tails.items()}

tuple unpacking

A matter of style, but in most situations where you use my_list[0] and my_list[1:], you can use tuple unpacking. You can use this in prune_or_follow

def prune_or_follow(tails: List[str]) -> Union[Exploded, str]:
    start, *rest = tails
    if rest:
        return explode(tails)
    else:
        return start

Whether this is more clear is a matter of taste.

This tuple unpacking does not work as clean in explode because it converts the string into a list of 1-character strings.

Exploded

You can use a string in typing so you can do:

Exploded = Dict[str, Union["Exploded", str]]

without the Exploded = None

cull typing

cull can accept any iterable. For the result, I don't think the order of strings is important, so there is no need to limit you to a list. The signature can then be def cull(subject: Iterable[str]) -> Collection[str]:. Whether you return a list or a set is a matter of implementation.

If the result is just used for iteration, you can even forgo the list call, and just return the implode generator, and let the caller decide whether he needs in in a list, set or whatever data structure he wants.

implode

now you use e as argument name. One-letter variable names are unclear in general, and to be avoided most of the time*. Since it is a kind of a tree, I would change this variable.

A slightly different take in implode which doesn't use string concatenation but tuple unpacking:

def implode(tree: Exploded, previous=()) -> Generator[str, None, None]:
    for root, branches in tree.items():
        if isinstance(branches, str):
            yield "".join((*previous, root, *branches))
        else:
            yield from implode(branches, *previous, root)

or equivalently:

def implode(tree: Exploded, previous=()) -> Generator[str, None, None]:
    for root, branches in tree.items():
        if isinstance(branches, str):
            yield "".join((*previous, root, *branches))
        else:
            yield from implode(branches, (*previous, root))

The choice between your version and this is a matter of taste, but I wanted to present the different possibilities.

Another variation, is using try-except ArgumentError instead of the isinstance:

def implode_except(tree: Exploded, previous=()) -> Generator[str, None, None]:
    for root, branches in tree.items():
        try:
            yield from implode(branches, (*previous, root))
        except AttributeError:
            yield "".join((*previous, root, *branches))

'* I find i, j etc acceptable for a counter or index, and x and y for coordinates, but in general I try not to use one-letter variable names.


Counting number of unique elements in a sorted container Similar to this question, but in C++. Although my priority is performance (and correctness!) rather than readability.

#include <algorithm>
#include <iterator>
#include <cstddef>

template <typename Container>
size_t count_unique(const Container& elements)
{
    size_t result {};

    for (auto it = std::cbegin(elements), last = std::cend(elements); it != last;) {
        ++result;
        it = std::upper_bound(it, last, *it);
    }

    return result;
}

Worst case is \$O(n \log n)\$

If we pass an array of completely unique elements, we get a pretty bad runtime. Your solution is good if we expect a lot of duplicates, but bad if we don't.

As an optimization for the worst case, we could add some exponential bounding to find the next element. Basically, replace

it = std::upper_bound(it, last, *it);

with:

std::ptrdiff_t bound = 1;
while (std::distance(it, last) < bound && *std::next(it, bound) == *it) {
    bound *= 2;
}

it = std::upper_bound(
    std::next(it, bound/2),
    std::next(it, std::min(bound, std::distance(it, last))),
    *it);

Of course is now pretty bad for non-random access iterators since we have to keep doing lots of distance and advance related calls, so I would simply default to the trivial comparison algorithm in that case. That is:

template <class C>
size_t count_unique(C const& container) {
    using It = decltype(std::cbegin(container));
    return count_unique(container, std::iterator_traits<It>::iterator_category{});
}

template <class C>
size_t count_unique(C const& container, std::random_access_iterator_tag ) {
   // what I suggested
}

template <class C>
size_t count_unique(C const& container, std::forward_iterator_tag ) {
    auto first = std::cbegin(container), last = std::cend(container);
    if (first == last) return 0;

    size_t count = 1;
    for (auto next = std::next(first); next != last; ++first, ++next)
    {
        count += !(*next == *first);
    }
    return count;
}

Additional Requirements

The binary-search optimization requires that the type the container holds is ordered. You may want to fallback to the simple forward-iterator version if the type is only equality-comparable but not less-than-comparable.

Initialization

I would prefer:

size_t result = 0;

to

size_t result {};

No reason to hide the 0.


Multiple criterias in a VLOOKUP in Excel VBA I have made the following custom defined function in Excel. It works somehow like VLOOKUP(), but it takes two criteria. I think the code is a bit of a mess. Does anyone has any comments, suggestions, or improvements?

Public Function VLOOKUPMC(ByVal return_col_num As Long, ByRef table_array_1 As Range, ByVal lookup_value_1 As Variant, ByRef table_array_2 As Range, ByVal lookup_value_2 As Variant) As Variant
    Dim rCell1 As Range

    For Each rCell1 In table_array_1
        With rCell1
            If .Value = lookup_value_1 Then
                If .Offset(0, table_array_2.Column - .Column) = lookup_value_2 Then
                    VLOOKUPMC = .Offset(0, return_col_num - .Column)
                    Exit Function
                End If
            End If
        End With
    Next rCell1
    VLOOKUPMC = CVErr(xlErrNA)
End Function

Here's a different version, though I won't claim that it is significantly better. The code seemed to work just fine and was mostly easy to understand (see my comment on the original question). My only big suggestion would be to add some comments to better describe what you are doing and how this works.

For a brief description of my changes, see the comments in the code. I can elaborate more if necessary.

Public Function VLOOKUPMC(ByVal return_col_num As Long, _
    ByRef table_array_1 As Range, ByVal lookup_value_1 As Variant, _
    ByRef search_column_2 As Long, ByVal lookup_value_2 As Variant) As Variant
    'Changed table_array_2 to search_column_2 because that's all that was used in the code below.
    Dim rCell1 As Range
    VLOOKUPMC = CVErr(xlErrNA)
    'Left the For loop as-is, but maybe you want it to look only in the first column, like the normal VLOOKUP would?
    For Each rCell1 In table_array_1
        'Modified logic to all fit on one line/remove nesting.
        'This organization may not be preferred, but it's one way to clean up the so-called 'mess'.
        VLOOKUPMC = _
            IIf(rCell1.Value = lookup_value_1 And _
            rCell1.Offset(0, search_column_2 - rCell1.Column) = lookup_value_2, _
            rCell1.Offset(0, return_col_num - rCell1.Column), _
            VLOOKUPMC)
        If VarType(VLOOKUPMC) <> vbError Then Exit Function
    Next rCell1
End Function


Implementing Haskell's `unwords` Learn You a Haskell explains the unwords function.

  $unwords ["hey","there","mate"]
    "hey there mate"

Here's my implementation.

unwords' :: [String] -> String
unwords' [] = []
unwords' (x:xs)
  | null xs   = x
  | otherwise = x ++ " " ++ unwords' xs

Is there a possible implementation with fold? I tried the following, but the " " got appended to the end.

$foldr (\x acc -> x ++ " " ++ acc) [] ["hey", "there", "mate"]
"hey there mate "

Also, how can I append a String([Char]) to another String without ++?

Lastly, please critique in general.

Your foldr solution doesn't work because it's using the empty list you're passing in the final step. You can use foldr1 instead, it uses the final element of the list in place of being passed an accumulator value.

Looking to the folds to implement unwords isn't a bad idea, but there are other high-level functions that you can use to write a more terse or readable version. Let's start from a verbal description of what unwords is doing.

  Insert a space character between every String in a list, then join the resulting Strings together.

The latter half of that description is easy, we know that String is really [Char], and we can easily flatten doubly-nested lists with concat. The former portion we could implement on our own, or search Hoogle for to see if anything already exists in the Prelude or other modules that could help us out. In this case, we're looking for a function with the type a -> [a] -> [a], that is, we want to pass it a value and a list and have it return a list with that value inserted between each pair of elements. As luck would have it, there's a function in Data.List that does exactly what we're looking for called intersperse that comes up as the first result if we perform that search.

Using these two functions, we can write a very short version of unwords that reads almost like prose.

import Data.List (intersperse)
import Prelude hiding (unwords)

unwords :: [String] -> String
unwords = concat . intersperse " "

Besides the aesthetic appeal of this solution, to me this illustrates the power of thinking about what you want to do in Haskell, instead of thinking about how it's going to be done.


javascript - create an object containing objects based on JSON data Based on JSON data I receive, I am trying to track a team's home and away games. The JSON data is stored in $scope.gameSchedules and has "team1" for home games and "team2" for away games. I need to initially create an object with each team's name and the only way I can get all the teams' names is via the "team1/team2" values in the initial JSON data (thus the two loops below). This was my working solution to create an object that looks like this (called homeVsAwayGames):

The initial JSON data response:

Here is my code. Is there a better way to do it, or more concise, perhaps using underscore (not necessary)? I feel that perhaps I have too many lines of code and it can be refactored:

 $scope.getSchedules = function() {
        return TeamsScheduleService.getTeamSchedules().then(function(response) {
            $scope.gamesSchedule = response.data.games;
        }, function(err) {
            console.log("there was an error getting the schedules");
        })
    }
    $scope.homeVsAwaySchedules = {};
    $scope.getSchedules().then(trackHomeVsAway).then(function(){
        console.log($scope.homeVsAwaySchedules);
    })
    //TODO: ng-repeat over the homeVsAwaySchedules Object to display in the view.

    function trackHomeVsAway() {
        //initialize homeVsAwaySchedules object in separate loop for efficiency and readability, instead of trying to initialize + tally at the same time.
        $scope.gamesSchedule.forEach(function(gameData) {
          var homeTeam = gameData.team1;
          var awayTeam = gameData.team2;
            if (homeTeam) {
                if (!$scope.homeVsAwaySchedules[homeTeam]) {
                    $scope.homeVsAwaySchedules[homeTeam] = {
                        "Home Games": 0,
                        "Away Games": 0
                    };
                }
            }
            if (awayTeam) {
                if (!$scope.homeVsAwaySchedules[awayTeam]) {
                    $scope.homeVsAwaySchedules[awayTeam] = {
                        "Home Games": 0,
                        "Away Games": 0
                    };
                }
            }
        })

        $scope.gamesSchedule.forEach(function(gameData) {
          var homeTeam = gameData.team1;
          var awayTeam = gameData.team2;
            if (homeTeam) {
                if ($scope.homeVsAwaySchedules[homeTeam]) {
                    $scope.homeVsAwaySchedules[homeTeam]["Home Games"]++;
                }
            }
            if (awayTeam) {
                if ($scope.homeVsAwaySchedules[awayTeam]) {
                    $scope.homeVsAwaySchedules[awayTeam]["Away Games"]++;
                }
            }
        })
    };
}

You could dry up this code by adding some helper functions.

For example instead of this:

$scope.gamesSchedule.forEach(function(gameData) {
  var homeTeam = gameData.team1;
  var awayTeam = gameData.team2;
    if (homeTeam) {
        if (!$scope.homeVsAwaySchedules[homeTeam]) {
            $scope.homeVsAwaySchedules[homeTeam] = {
                "Home Games": 0,
                "Away Games": 0
            };
        }
    }
    if (awayTeam) {
        if (!$scope.homeVsAwaySchedules[awayTeam]) {
            $scope.homeVsAwaySchedules[awayTeam] = {
                "Home Games": 0,
                "Away Games": 0
            };
        }
    }
})

You could write using a helper function:

$scope.gamesSchedule.forEach(function(gameData) {
    function checkTeam(team) {
        if (team) {
            if (!$scope.homeVsAwaySchedules[team]) {
                $scope.homeVsAwaySchedules[team] = {
                    "Home Games": 0,
                    "Away Games": 0
                };
            }
        }
    }
    checkTeam(gameData.team1);
    checkTeam(gameData.team2);
})

Similarly, instead of this:

$scope.gamesSchedule.forEach(function(gameData) {
  var homeTeam = gameData.team1;
  var awayTeam = gameData.team2;
    if (homeTeam) {
        if ($scope.homeVsAwaySchedules[homeTeam]) {
            $scope.homeVsAwaySchedules[homeTeam]["Home Games"]++;
        }
    }
    if (awayTeam) {
        if ($scope.homeVsAwaySchedules[awayTeam]) {
            $scope.homeVsAwaySchedules[awayTeam]["Away Games"]++;
        }
    }
})

You could write using a helper function:

$scope.gamesSchedule.forEach(function(gameData) {
    function updateTeamSchedule(team, games) {
        if (team) {
            if ($scope.homeVsAwaySchedules[team]) {
                $scope.homeVsAwaySchedules[team][games]++;
            }
        }
    }
    updateTeamSchedule(gameData.team1);
    updateTeamSchedule(gameData.team2);
})


How clean is my close_account code? Hello i'm coding a social site and I need to know how clean this code is. I read other answers people told me from my previous questions like this and used it as guidelines. So now I wanna know if anything could be improved. Thank you.
<?php

mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
error_reporting(-1);

include 'includes/header.php';

$userLoggedIn = $_SESSION['thesocial_username'];

$post_id = isset($_POST['cancel']) ? header("Location: settings.php") : '';

if(isset($_POST['close_account'])) {

    $close_query = $con->prepare('UPDATE users SET user_closed = "yes" WHERE username = ?');
    $close_query->bind_param("s", $userLoggedIn);
    $close_query->execute();

    session_destroy();
    header("Location: register.php");
}

?>

Close Account

    Are you sure you want to close your account ?

    Closing your account will hide your profile and activity from users.

    You can reopen your account at anytime by logging in.

    <form action="close_account.php" method="POST">

        <input type="submit" name="close_account" id="close_account" value="Yes, Close my account.">
        &nbsp;&nbsp;
        <input type="submit" name="cancel" id="update_details" value="Nevermind.">

    </form>

The code is overall good basic PHP and above the average. However, I would like it more structured.
For example, why you are setting the error reporting manually in this file? Apparently you have sort of a bootstrap file as includes/header.php, why not to put these lines there?
Besides, i don't see any point in the line assigning $post_id variable. What does a post id to do with deleting an account? And yes, such a syntax structure looks embarrassing.
It is also a good rule of thumb to follow Location header with an explicit exit call. Although it is not critical in this particular case, sometimes the lack of the script termination could pose a serious threat, as the header itself doesn't stop the script execution.
Some notes on the database structure

The computer world has its own representation for "yes" and "no", it's 1 and 0 respectively. So consider making the field of tinyint type instead of varchar.
it is considered a good practice to address all database entities by the unique id. It's better to store the user id in the session and use it for all the database operations.

The code structure
One big issue I overlooked at first is the structure of your script. It seems that includes/header.php is not just a bootstrap file but it also starts some HTML output. that's a big NO. An HTTP header such as Location cannot be sent after HTML. Your PHP lets you to do it due to some permissive setting, but it is nevertheless wrong, from the logical and technological points of view. It just makes no sense to start any output when your script is not going to display anything but would only redirect to another page.
Split your includes/header.php file into two: bootstrap.php and header.php. The former should contain all the configuration options such as database connection, error reporting etc, but not a byte should it output. And the latter would contain all the HTML.
Than make your code like this
<?php

include 'includes/bootstrap.php';

$id= $_SESSION['user_id'];

if(isset($_POST['close_account']))
{
    $stmt = $con->prepare('UPDATE users SET user_closed = 1 WHERE id = ?');
    $stmt->bind_param("s", $id);
    $stmt->execute();

    session_destroy();
    header("Location: register.php");
    exit;
}
include 'includes/header.php';
?>
<center>


Fibonacci prefix code This one is kind of involved. Is there a simpler way to implement this prefix code in base python, my existing code goes below but any help in modification or review will be helpful.

# Lagged fibonacci numbers the sequence starts: 1,2,3,5,8,11
def fibonacci(n):
    a = 1
    b = 1
    out = []
    for i in range(n):
        out.append(a)
        a,b = a+b,a
    return out

# There has to be a better way to do this, right?
# Go through the list for the first few fibonacci numbers from greatest to
# make a list of fibonacci numbers that add up to each code number (1,2,3...)
# The code is then the indexes of those fibonacci numbers, with leading zeroes
# removed, reversed, with an additional "1" at the end.
def makeCodebook(decode=False):
    F = fibonacci(10)
    F.reverse()
    codes = []
    for x in range(1,27):
        while x != 0:
            code = []
            for f in F:
                if f <= x:
                    code.append("1")
                    x = x-f
                else:
                    code.append("0")

            while code[0] == "0":

                code.pop(0)
            code.reverse()
            code.append("1")
            codes.append("".join(code))

    # Matchup codes with letters
    D = {}
    alpha = "ETAOINSRHLDCUMFPGWYBVKXJQZ"
    if decode == False:
        for a,b in zip(alpha,codes):
            D[a] = b

    if decode == True:
        for a,b in zip(alpha,codes):
            D[b] = a
    return D

def prefixCode(text,decode=False):

    # Build the codebook
    D = makeCodebook(decode=decode)

    # When encoding we simply translate each letter to its code
    if decode == False:
        out = []
        for letter in text:
            out.append(D[letter])
        return "".join(out)

When you need to loop a fixed number of time, but don't actually need the loop index, it is customary to use _ as the loop index.  So, in fibonacci(n), you would write for _ in range(n):.

Your fibonacci() generator is generating too many values.  Your last code value is 26, so any fibonacci value greater than 26 is unnecessary.  Instead of asking for a fixed number of values, ask for values up to a specific limit.

def fibonacci_up_to(last):
    a, b = 1, 1
    out = []
    while a <= last:
        out.append(a)
        a, b = a+b, a
    return out

And use F = fibonacci_up_to(26)

The following is an incorrect comment:

# Lagged fibonacci numbers the sequence starts: 1,2,3,5,8,11

The fibonacci sequence doesn't contain an 11; the number, following after 5 and 8 would be 5+8=13!

In makeCodebook():

It is usually clearer to write x = x-f as x -= f

Instead of always appending "1" to the end of code after reversing it, you could simply initialize code = ["1"] at the start.

The while x != 0: loop is unnecessary.  After the for f in F: loop, x will have become zero, or there is a logic error in constructions of the code for x.  Remove while x != 0:, and optionally add an assert if you are unsure x actually reaches zero.

Construction your code dictionary D:

D = {}
for a,b in zip(alpha, codes):
    D[a] = b

can be re-written with list-comprehension as:

D = { a: b for a, b in zip(alpha, codes) }

Or, as pointed out by @Graipher in the comments, simply:

D = dict(zip(alpha, codes))

Using if decode == False: followed by if decode == True: is unnecessary verbose.  if decode == False: can simply be if not decode:, and the reverse condition should simply be else:.  Or better, eliminate the negated logic, by swapping the statement order:

if decode:
    D = { code: letter for letter, code in zip(alpha, codes) }
else:
    D = { letter: code for letter, code in zip(alpha, codes) }

Or, leveraging @Graipher's simplification from the comments and Python's x if cond else y expression, the above can be written in one line.  Whether this is clearer or more obfuscated is a matter of taste:

D = dict(zip(codes, alpha)) if decode else dict(zip(alpha, codes))

In prefixCode():

Constructing the out array can be done with list comprehension:

out = [ D[letter] for letter in text ]

but this out array is just a temporary used as an argument in the next statement, so you should combined them:

return "".join( D[letter] for letter in text )

Your function has (or will have) two entirely different execution paths with almost no common functionality, one for encoding, and (eventually) one for decoding.  Write this as two functions, not as one function with a decode parameter to choose between the paths:

def encode(text):
    # ...

def decode(text):
    # ...

Use pylint or similar to check your code for PEP-8 compatibility.  Including things such as:

use spaces after commas: zip(alpha, codes) not zip(alpha,codes)
use spaces around operators: x - f not x-f
use lower_case_function_names(), not mixedCaseFunctionNames()
use lower_case_variables not uppercase identifiers like D and F

Generating your codebook:

This can be simplified by (for the moment, ignore the final terminating 1) noting that the code for a Fibonacci number is simply a number of placeholder 0's followed by a 1, and the code for a number like 17 (=13+4) will be the code for 4 (101), followed by a number of placeholder 0's, and then a 1 for the largest Fibonacci value smaller than the number (13).  After building all of the codes, add the final 1 terminator to each.

def codebook(key):
    codes = [""]
    a, b = 1, 1
    digits = 0

    for x in range(1, len(key)+1):
        if x >= a:
            a, b = a+b, a
            digits += 1

        prefix = codes[x-b]
        codes.append(prefix + "0"*(digits - len(prefix) - 1) + "1")

    return { letter: code+'1' for letter, code in zip(key, codes[1:]) }

letter_to_code = codebook("ETAOINSRHLDCUMFPGWYBVKXJQZ")

code_to_letter = { code: letter for letter, code in letter_to_code.items() }


Different function test result inquiry I decided to run a few test on js functions on jspref to see which method of using function is better suited for this small particular example, to get a better understanding.

Page with tested functions

Problem:

Getting different results each time, Cold Start, Re-test, Re-Re Test are all coming with different results

First function - is just embedded in normally

$(function() {
  var up = $('#horiz_line').offset().top + $('#horiz_line').height() + 25,
      triga = $('.trigger');
  triga.css("display", "none");
  $(window).scroll(function() {
    var down = $(this).scrollTop();
    (down > up) ? triga.fadeIn('fast') : triga.fadeOut('fast')
  });
});

Second function - calling a global function

$(function() {
  triga.css("display", "none");
  $(window).scroll(function() {
    scrollMenu($(this).scrollTop())
  });
});

function scrollMenu(down) {
  var up = $('#horiz_line').offset().top + $('#horiz_line').height() + 25,
      triga = $('.trigger');
  (down > up) ? triga.fadeIn('fast') : triga.fadeOut('fast')
}

Third function - registering/calling variable function (to test/learn: I have no idea what I'm doing)

$(function() {
  $(window).scroll(function() {
    scrollMenu($(this).scrollTop())
  });
  var scrollMenu = function(down) {
      var triga = $('.trigger'),
          up = $('#horiz_line').offset().top + $('#horiz_line').height() + 25;
      (down > up) ? triga.fadeIn('fast') : triga.fadeOut('fast')
      }
});

Fourth Func - Same as Third except Up is called before hand, and registered as a global func. Which i'm guessing is not a good idea if the project gets bigger over time.

// Variable Up Takes up The Global Namespace. Is it better to pre-define it OR have it         inside the scroll Menu anonymous func and get calculated each time the function is run?
$(function() {
  var up = $('#horiz_line').offset().top + $('#horiz_line').height() + 25;
  $(window).scroll(function() {
    scrollMenu(up, $(this).scrollTop())
  });
  var scrollMenu = function(up, down) {
      var triga = $('.trigger');
      (down > up) ? triga.fadeIn('fast') : triga.fadeOut('fast')
      }
});

These four functions are doing different things:

The 1st and 4th set up immediately and down when the user scrolls.

The 2nd and 3rd set both up and down when the user scrolls. (The 2nd seems unlikely to work properly because triga is not defined in the 1st function, so the line triga.css("display", "none"); will result in an error. Only in the 1st will the triga element be initially hidden.)

So they will behave differently if the element #horiz_line changes position between the function being set and the user scrolling.

I'm guessing what you want is to check the position of the horizontal line and the scroll top when the user scrolls. In this case something like the 2nd and 3rd function should work, but it is needlessly complicated to pass the values as parameters and call a function from within a function. Instead you can do either:

$(function() {
  var triga = $('.trigger');
  triga.css("display", "none");
  $(window).scroll(function() {
    var down = $(this).scrollTop(),
        up = $('#horiz_line').offset().top + $('#horiz_line').height() + 25;
    triga[down > up ? 'fadeIn' : 'fadeOut']('fast');
  });
});

or

$(function() {
  var triga = $('.trigger');
  triga.css("display", "none");
  $(window).scroll(scrollMenu);
  function scrollMenu() {
    var down = $(window).scrollTop(),
        up = $('#horiz_line').offset().top + $('#horiz_line').height() + 25;
    triga[down > up ? 'fadeIn' : 'fadeOut']('fast');
  }
});

If, on the other hand, up never changes, then there will be a performance advantage to either defining up as a global variable or passing it as a parameter to the scrolling function (because accessing the DOM is usually costly in terms of performance.)

I think the other aspect of your question is whether there is a performance difference in passing a value as a parameter to a function vs. having it in the scope of an outer function; I don't know but suspect it makes a negligible difference, especially if the parameter is first defined as a variable in the outer function anyway.

More generally: focus on understanding how the code works before worrying about performance optimisation!


Bitmasking and searching consecutive 1's I have written code to count the continuous ones in an array or you can use binary string for convenience. Any solution using the binary string is acceptable.

My Solution is:

#include<bits/stdc++.h>
using namespace std;
#define MAX 100000
int main()
{
  int n,q,k,count;
  string str;
  cin>>n>>q>>k;
  bitset<MAX>s,c;
  //inserting bits in array
  for(int i=0;i<n;i++)
  {
    int temp;
    cin>>temp;
    s[i]=temp;
   }
   cin>>str;
  for(int i=0;i<q;i++)
  {
     //making duplicate bitset
     c=s;
     if(str[i]=='?')
      {
         count=0;
         while(c!=0)
         {
            //using bitmask to count maximum no of continuous 1's-O(1's bit)
            c=(c&(c<<1));
            count++;
          }
      if(count>k)
         cout<<k<<"\n";
      else
         cout<<count<<"\n";
   }
   else
   {
       //shifting each bit to right and updating first bit with previous last
      // bit
       bool lb=s[n-1];
       s=s>>1;
       s[n-1]=lb;
    }
  }
}

<bits/stdc++.h> (like everything in your compiler's bits/ subtree) is not a standard header and therefore not portable.  Even if you're willing to sacrifice portability, it's a poor choice, as it will slow compilation down compared to simply including what you use.

using namespace std; is poor practice.  It makes your code less clear, and it may even silently change its meaning.

Don't use the preprocessor to name constants.  Use a properly scoped, strongly typed C++ constant:

constexpr std::size_t max = 100000;

When using input streams, always check that operations succeed before using their results.

Variable names should be more descriptive.  I have no idea what n, q, and k are supposed to be storing.  In fact, these names are so useless that I gave up reading at this point - it's not at all clear what this is supposed to be doing.


Managing batch runs of Fortran programs I have a program which basically manages batch runs of some underlying Fortran programs, and at the end, I want to move all the related files into a different directory.

At the moment, there is something along the lines of this:

private void finalClean() throws Exception{
  Script.move(file1, calcDir);
  Script.move(file2, calcDir);
  Script.move(file3, calcDir);
  ...
}

Now, the nature of these jobs is that they do not always run to completion, but the finalClean method is in the finally clause, so it is always run. The problem is, that depending on where the run fails, not all of the files are present, which means that if one of those files isn't present, it throws an exception, causing none of the remaining files to be moved. The files are not always created in the same order either, which means I can't just order them as they would be created and then not have to worry about remaining files, as they're not created.

My current solution to this is:

ArrayList<String> files_to_move = new ArrayList<String>();
files_to_move.add("file_name_1");
files_to_move.add("file_name_1");
files_to_move.add("file_name_1");
...

for(String file_name : files_to_move){
  try {
    Script.move(QNFiles.getProject(), file_name, calcDir);
  } catch (Exception ex) {
    Logger...blah.blah.blah;
  }
}

But I'm not overly pleased with how it looks. Is there a way to have a some kind of construct around the whole loop which just grabs all the exceptions, ignores them, and then throws an exception containing info on all of the others afterwards? Maybe a way I can temporarily ignore a subset of exceptions and deal with them all later, without having the try catch in the loop?

I don't use Java as much as Python anymore, so I suspect my wanting to neaten this up is a symptom of becoming used to that, but still: is there a better way to do this?

If what worries you is readability use a function that takes care of the exception:

boolean error = false;
for(String file_name : files_to_move){
    error |= moveTheFile(...);
}
if (error) {
    // throw your exception here
}

The function would be something like:

private boolean moveTheFile(...) {
    try {
         Script.move(QNFiles.getProject(), file_name, calcDir);
    } catch (Exception ex) {
        Logger...blah.blah.blah;
        return true;
    }
    return false;
}


Container for fast deletion and addition of elements I need a container that is able to delete and add elements as fast as possible, without the need of reallocation that usually takes place when we push_back elements in an std::vector or when we delete elements.

By looking around on the internet I have found that a good way to achieve this is to use a fixed-size container. The class is supplied with a counter which accounts for the number of "active elements".
Deletion of an element consists just of swapping said element with the last active one of the container and decreasing the counter of active elements by one.

What do you think of this implementation? How can I improved it?

#include <vector>

template <class Element>
class ObjectPool
{

private:
    size_t            activeElements;
    std::vector<Element> elements;

public:
    ObjectPool(size_t maxElements)
    {
        activeElements = 0;
        elements.resize(maxElements);
    }

    size_t size() const
    {
        return activeElements;
    }

    Element& at(size_t index)
    {
        if (index >= activeElements) {
            throw std::out_of_range("System: element requested is not active.");
        }
        return elements.at(index);
    }

    const Element& at(size_t index) const
    {
        if (index >= activeElements) {
            throw std::out_of_range("System: element requested is not active.");
        }
        return elements.at(index);
    }

    Element& back()
    {
        if (activeElements <= 0) {
            throw std::out_of_range("System: number of active elements is zero.");
        }
        return elements.at((size_t)(activeElements - 1));
    }

    const Element& back() const
    {
        if (activeElements <= 0) {
            throw std::out_of_range("System: number of active elements is zero.");
        }
        return elements.at((size_t)(activeElements - 1));
    }

    void addBackElement()
    {
        if (activeElements < elements.size()) {
            activeElements++;
        }
        else
        {
            throw std::out_of_range("System: maximum limit of elements in the system exceeded.");
        }
    }

    void deleteElement(size_t index)
    {
        if (index >= activeElements) {
            throw std::out_of_range("System: element requested is not active.");
        }
        activeElements--;
        swapElements(activeElements, index);
    }

    void resize(size_t maxElements)
    {
        elements.resize(maxElements);
    }

private:
    void swapElements(size_t i, size_t j)
    {
        std::swap(elements.at(i), elements.at(j));
    }
};

struct Laugh {
  Laugh() {
    std::cout << "Ha";
  }
};

int main() {
  ObjectPool<Laugh> pool(3);
}

Do you hear the evil laughter?

"active" vs the lifetime of an object

What happened?  When you resize the vector from its initial size 0 to some larger size in the constructor, then the vector will be filled with default-inserted objects.  So directly after construction of your object pool, it is already completely filled with alive objects.  These objects stay alive all the time (unless you resize).  Your notion of an "active" object basically just revolves around whether one can access (through at) that object.  "deleting" just moves one object out of the "active" (accessible) region of your pool.  "adding" enlarges this "active" region.

This is very confusing and not what one does expect from an "object pool"!

Further, it puts up some requirements to that Element type, namely that it must be default-insertable.  With the standard allocator (which you use) this means that Element has to be constructible through value initialization.  That's a pretty tough requirement, as it basically means that Element needs to have a zero argument constructor.  Now, if Element is a bit more than a simple value / data aggregation type, then its objects will probably depend on some objects of other types.  To follow the SRP one wouldn't let an Element construct the other objects then, but rather pass it the requirements upon its own construction (dependency injection).

Your object pool design does not allow this, though.  In short:  You sign yourself up for lots of future trouble.

To fix this, use reserve for your vector in the constructor, and actually construct new objects when adding (make emplace_back of the vector accessible) and destruct (terminology is important!) them when you want to get rid of them.  To keep that later operation fast, swap the last element and the one you want to get rid of and then erase the last. (Of course this only works when order doesn't matter.  Moreover, it invalidates possibly obtained references to the last element.)

Swapping done right

Calling std::swap directly is almost always wrong.  Bring it into scope with using std::swap;, and then just do an unqualified call: swap(a, b).  This allows ADL to kick in and find potentially useful user-defined swap functions.

Provide a complete interface

std::vector provides at, which - like yours - does bounds checking.  It also provides operator[] which does not.  Thus, I'd hope for (expect) your class to provide this feature, too.

Furthermore, I'd follow the terminology used by the standard std::vector for adding / removing elements here: push_back (or emplace_back) and erase.

Expose the allocator

std::vector allows to use a different allocator than the standard one.  You should expose this with your pool, too.  That allows for object pools whose objects have automatic storage duration ("are on the stack") when used with such an allocator.