Compare multidimensional arrays I'm writing a wordpress plugin that makes changes to the admin menu. To add some context: The wordpress global $menu is a multidimensional array. Each array in $menu represents a menu item, with the title, url, css classes etc at  specific indexes in the array. the oa_custom_menu option is a similarly structured multidimensional array, but each menu item may have additional indexes in its array to store things like wether the menu item should be hidden or not.

the oa_custom_menu array is only updated when changes are made in my custom admin page. The $menu array may have changed since the last time this happened (e.g. new plugins or themes being activated and adding menu items), so I need to check for any added or removed items in the array.

index 2 in both menu arrays are the URL and the only thing that will consistently be unique. $custom_menu_item[7] is a bool that indicates wether the item should be hidden or not, so we ignore these items if they are hidden..

I hope that all makes sense, I'm not the best at explaining things!

The code I have works perfectly well, it just seems like too many foreach loops to me, but I can not for the life of me figure out a better way to do it. Am I right?

public function compare_menu_arrays() {

    global $menu;
    $custom_menu = get_option('oa_custom_menu') ? get_option('oa_custom_menu') : $menu;

    $menu_sort_keys = array();
    $custom_menu_sort_keys = array();

    foreach ( $menu as $menu_item ) {
        $menu_sort_keys[] = $menu_item[2];
    }

    foreach ( $custom_menu as $custom_menu_item ) {
        if ( empty($custom_menu_item[7]) ) {
            $custom_menu_sort_keys[] = $custom_menu_item[2];
        }
    }

    $new_menu_items     = array_diff( $menu_sort_keys, $custom_menu_sort_keys );
    $removed_menu_items = array_diff( $custom_menu_sort_keys, $menu_sort_keys );

    /*
     * add any new $menu items to our $custom_menu
     */
    if ( !empty($new_menu_items) ) {
        foreach ( $menu as $menu_item ) {
            if ( in_array( $menu_item[2], $new_menu_items ) ) {
                $custom_menu[] = $menu_item;
            }
        }
    }

    /*
     * remove any missing $menu items from our $custom_menu
     */
    if ( !empty($removed_menu_items) ) {
        foreach ( $custom_menu as $index => $custom_menu_item ) {
            if ( in_array( $custom_menu_item[2], $removed_menu_items ) ) {
                unset( $custom_menu[$index] );
            }
        }
    }
}

There is nothing inherently wrong with foreach loops.

Alternatives might include:

Creating objects that contain the data and extend Iterator, and using that to recurse the options. The disadvantages are that it would be less efficient and would involve quite a lot more code. The advantage is it would make the foreach loops more readable; particularly whats inside them, once you give methods to each of the objects that extend Iterator that perform the action that happens within the foreach code.
use PHP array functions. This is probably the most efficient, but can make code painfully unreadable. I would not recommend this unless you desperately need greater efficiency.

By the way, one change I would make no matter what you do, is replace "magic constants" with named constants. E.g.:

if ( in_array( $menu_item[2], $new_menu_items ) ) {

becomes

define ('MEANINGFUL_NAME', 2);

// ....

if ( in_array( $menu_item[MEANINGFUL_NAME], $new_menu_items ) ) {

Otherwise, when you come back to look at the code after a long time, you will have no idea if each time you see a 2, whether they all refer to the same "thing" or not; in other words, say you want to change an aspect of it to be "3" instead, you will have to painstakingly work out what each bit of code does to see if it is a 2 that you need to change or not. (Using comments is a bad idea for this, as comments are easily forgotten to be changed, and it would have to be commented each time you have a magic number - the more you have, the more likely changes in code will not have corresponding comments changed

One final change I would make is perhaps splitting up the function into sub-functions, since doing that would mean you can give the functions names that describe the sub-process being performed. Putting that method and sub methods into an appropriate class could nicely tidy things up. While OOP can lead (IMO) to the very best code, if it's implemented badly it can be a nightmare to manage. Plenty of books on it, which I could recommend if you like. I have 5-10 books on it that I always keep by my side.

"Design Patterns" Gamma / Helm / Jhnson / Vlissides (1995)
"Patterns of Enterprise Application Architecture" Martin Fowler (2003)
"The pragmatic programmer" Hunt / Thomas (2000)
"Refactoring to patterns" Kerievsky (2005)
"Refactoring" - Martin Fowler (1999)
"OO Design Heuristics" - Arthur Riel (1996)
"Growing Object-Oriented Software Guided by Tests" - Freeman / Pryce (2009)

I would actually suggest first getting Freeman/Pryce, even if you find TDD doesn't really agree with you - their bibliography was very useful to me, and I picked and choose what suited me. There's a lot of controversy about TDD, but they'll show you why refactoring is such a powerful principle, and you can then try one or two books on patterns and refactoring. Choose one that seems to have patterns you think you'll be most likely to use.


Beginner's pygame Conway's Game of Life Here's my implementation of Game of Life. I'm quite new to Python and would like to know how could I improve that code, especially in terms of performance, compactness and readability.

import copy
import pygame
from pygame import *
import random

#constants
red = (255, 0, 0)
black = (0, 0, 0)
white = (255, 255, 255)
#neighbour coordinates
neighbours = [[-1,-1],[-1,0],[-1,+1],
              [0,-1],        [0,+1],
              [+1,-1],[+1,0],[+1,+1],]

class cell(object):
        def __init__(self, ngb, state):
                self.state = state
                self.ngb = 0

#2d array for storing cells
cells = [[i for i in range(50)] for i in range(50)]

#random field generation
def generate():
        print "Generating"
        for y in xrange(50):
                for x in xrange(50):
                        cells[x][y] = cell(0, random.randint(0, 1))
        print "DoneGen"

#neighbour processing
def update():
        global cells2
        #saving this turn's state
        cells2=copy.deepcopy(cells)
        for y in xrange(50):
                for x in xrange(50):
                        cellv2=cells2[x][y]
                        cellv2.ngb=0
                        cellv = cells[x][y]
                        #processing
                        for i in neighbours:
                                #offsetting neighbour coordinates
                                dy=i[0]+y
                                dx=i[1]+x
                                if dy < 0:
                                        dy = 49
                                if dy > 49:
                                        dy = 0
                                if dx < 0:
                                        dx = 49
                                if dx > 49:
                                        dx = 0
                                if cells2[dx][dy].state==1:
                                        cellv2.ngb+=1
                        #updating field
                        if cellv2.state==1 and 2<=cellv2.ngb<=3:
                                cellv.state=1
                        else:
                                cellv.state=0
                        if cellv2.state==0 and cellv2.ngb==3:
                                cellv.state=1

#main game function
def play():
        #initialization
        pygame.init()
        scrn = pygame.display.set_mode((500, 500))
        mainsrf = pygame.Surface((500, 500))
        mainsrf.fill(white)
        generate()
        #game cycle
        while 1:
                #tracking quitting
                for event in pygame.event.get():
                        if event.type == QUIT:
                                pygame.quit()
                                sys.exit()
                #drawing
                for y in xrange(50):
                        for x in xrange(50):
                                if cells[x][y].state==1:
                                        pygame.draw.rect(mainsrf, black, (x*10, y*10, 10, 10))
                                else:
                                        pygame.draw.rect(mainsrf, white, (x*10, y*10, 10, 10))
                                if cells[x][y].ngb==3:
                                        pygame.draw.rect(mainsrf, red, (x*10, y*10, 10, 10))
                update()
                scrn.blit(mainsrf, (0, 0))
                pygame.display.update()

#running the game
if __name__ == "__main__":
                                play()

Functions good practices: Documentation, Parametrization, Single Purpose, Outside State Independence

Documentation

#random field generation
def generate():

Documentation is usually written as a docstring in triple quotes under the function definition (this allows programmatic access via help:

def generate():
    """ Random field generation. """

It also usually is a complete phrase, I would write it a little different:

def generate():
    """ Generates a random game of life board. """

Parametrization

Your function can only generate boards of size \$50 * 50\$, may I be interested in other sizes I would need to modify the definition accordingly.

I suggest asking x and y sizes as parameters:

def generate(x_size, y_size):
        print "Generating"
        for y in xrange(x_size):
                for x in xrange(y_size):
                        cells[x][y] = cell(0, random.randint(0, 1))
        print "DoneGen"

This way you gain more re-usability for your function (and you can test it simpler by giving small sizes).

Single Purpose

    print "Generating"
    ...
    print "DoneGen"

Your function prints to std-out in addition to building the board and this behaviour cannot be turned off. If printing out is not desired the user will not be able to use this function. I would just delete these printing statements after the debugging is complete.

Outside state independence

You need a 2d list called board for this function to work. You may instead create such list and return it.

Write functions to encapsulate logical units of action

wrap_around(dx, dy, x_size, y_size)

                            if dy < 0:
                                    dy = 49
                            if dy > 49:
                                    dy = 0
                            if dx < 0:
                                    dx = 49
                            if dx > 49:
                                    dx = 0

This 8 lines of code provide the program with wrap-around functionality (for example: going too far to the right leaves you all the way back to the left), it would ideal if you wrote a function for this.

next_state(cell, neighbours)

The same can be said for this block of code calculates the next state of a given cell:

                    if cellv2.state==1 and 2<=cellv2.ngb<=3:
                            cellv.state=1
                    else:
                            cellv.state=0
                    if cellv2.state==0 and cellv2.ngb==3:
                            cellv.state=1

decide_colour(cell)

                            if cells[x][y].state==1:
                                    pygame.draw.rect(mainsrf, black, (x*10, y*10, 10, 10))
                            else:
                                    pygame.draw.rect(mainsrf, white, (x*10, y*10, 10, 10))
                            if cells[x][y].ngb==3:
                                    pygame.draw.rect(mainsrf, red, (x*10, y*10, 10, 10))

The 3 options differ only in the colour of the drawing so you could also cut down code duplication by using such a function.

Minor: use ALL_CAPS for constants

As a widely accepted convention constants are written ALL CAPS to quickly discern them from variable variables.

RED = (255, 0, 0)
BLACK = (0, 0, 0)
WHITE = (255, 255, 255)
#neighbour coordinates
NEIGHBOURS = [[-1,-1],[-1,0],[-1,+1],
              [0,-1],        [0,+1],
              [+1,-1],[+1,0],[+1,+1],]

Give meaningful names: ngb?

self.ngb = 0

I cannot understand what the purpose of the ngb field is even after reading the code.

This is a bad sign indicating that a more descriptive name should be adopted.


Finding duplicate files using md5sum Unix command This is an exercise from Think Python: How to Think Like a Computer Scientist

Here's its description:

  In a large collection of MP3 files, there may be more than one copy of
  the same song, stored in different directories or with different
  filenames. The goal of this exercise is to search for duplicates.

  Write a program that searches a directory and all of its subdirectories, recur‐ sively, and returns a list of complete paths
  for all files with a given suffix (like .mp3). Hint: os.path provides
  several useful functions for manipulating fileand path names.
  To recognize duplicates, you can use md5sum to compute a “checksum” for each files. If two files have the same checksum, they probably
  have the same contents.
  To double-check, you can use the Unix command diff.

Here's my solution:

import os

def run_command(cmd):
    """Runs a command in a shell.

       cmd: a string specifies a Unix command.

       Returns: a string specifies the result
       of executing the command.
       """
    filepipe = os.popen(cmd)
    result = filepipe.read()
    status = filepipe.close()

    return result

def md5_checksum(filepath):
    """Returns a string specifies the MD5 checksum of
       a given file using md5sum Unix command.

       filepath: a string specifies a file.
       """
    command = 'md5sum ' + filepath

    return run_command(command)

def md5_checksum_table(dirname, suffix):
    """Searches a directory for files with a given
       file format (a suffix) and computes their
       MD5 checksums.

       dirname: a string specifies a directory.

       suffix: a file format (e.g. .pdf or .mp3).

       Returns: a dictionary mapping from string
       works as a MD5 checksum to list of strings
       work as pathes of files have this checksum.
       """
    table = {}

    for root, sub, files in os.walk(dirname):
        for file in files:
            if file.endswith(suffix):
                filepath = os.path.join(root, file)
                checksum, filename = md5_checksum(filepath).split()
                table.setdefault(checksum, []).append(filename)

    return table

def are_identical(files_names):
    """Returns whether files in files_names
       are identical using diff Unix command.

       files_names: a list of strings specify pathes of files.
       """
    index = 1

    for filename1 in files_names:
        for filename2 in files_names[index:]:
            command = 'diff %s %s' % (filename1, filename2)
            result = run_command(command)

            if result:
                return False

        index += 1

    return True

def print_duplicates(checksums):
    """Prints pathes of files have the same MD5 checksum.

       checksums: a dictionary mapping from MD5 checksum (string) to
       list of pathes of files (strings) have share this checksum.
       """
    for checksum, filepathes in list(checksums.items()):
        if len(filepathes) > 1:
            print('Files have the checksum %s %s' % (checksum, 'are: '))

            for filepath in filepathes:
                print(filepath)

            if are_identical(filepathes):
                print('\nThey are indentical. \n')

def main():
    table = md5_checksum_table('/media/sf_Shared/', '.pdf')
    print_duplicates(table)

if __name__ == '__main__':
    main()

How can it be refactored and optimized?

Notes:

It wasn't like that, but it worked. After reading the author's solution I refactored it and edited a lot of it.
I'm no expert at MD5, just got the basic idea.
I'm no expert at Unix or Linux. I use Windows, and I've tested this script using a virtual machine running Ubuntu.
I'm a hobbyist and beginner in Python and programming.

Well this was a very satisfying problem, thanks for sharing!

First of all, calling external resources is expensive, there for not optimized, which you ask for. Else wise, calling external resources can be preferable if the external resource is something like shell on a platform you have control over. That's the reasons I removed them and substituted them with python built-ins. It's pretty much the only reason this code is slightly faster then yours.

I found one small error in your code. What if a file you try to hash has spaces? The problem occurs when you split the return from md5_checksum, it splits to as many values as there are white spaces.

The most time consuming function of both our code is walk. It's easy to check these where cpu-time went with profilers. And python has a builtin I like, but there are many. It's the cProfiler, check my code for usage.

The biggest change was refactoring the function are_identical for

if any(cmp(x, y) for x in paths for y in paths if y != x):
                print('\nThey are identical\n')

They do the same thing, but the any() builtin, is.. as well faster, then iterating over lists.

I did remove your function comments, as they can be substituted for good function names and annotations. Do you agree?

from os import walk
from os.path import join
from hashlib import md5
from filecmp import cmp
from base64 import b64encode
from time import time
import cProfile

def md5_checksum(file_path: str) -> (bytes, str):
    """ Returns the raw MD5 bytes here used as checksum a given files content """

    with open(file_path, "rb") as f:
        file = f.read()
    m = md5()
    m.update(file)
    return m.digest(), file_path

def md5_checksum_table(dir_name: str, suffix: str) -> {bytes: [str]}:
    """
    Searches a directory for files with a given file format (a suffix) and
      computes their MD5 checksums.
    """
    table = {}
    for root, sub, files in walk(dir_name):
        for file in files:
            if file.endswith(suffix):
                checksum, filename = md5_checksum(join(root, file))
                table.setdefault(checksum, []).append(filename)
    return table

def print_duplicates(checksums: {bytes: [str]}):
    """ Prints paths of files have the same MD5 checksum and are identical. """

    for checksum, paths in checksums.items():
        if len(paths) > 1:
            print('Files have the checksum {0} are:\n {1}'.format(b64encode(checksum),
                                                                "\n".join(paths)))
            if any(cmp(x, y) for x in paths for y in paths if y != x):
                print('\nThey are identical\n')

def main():
    start = time()
    table = md5_checksum_table('/media/sf_Shared/', '.pdf')
    print_duplicates(table)
    print("Time {:.3f}s".format(time()-start))
    cProfile.run("md5_checksum_table('/home/cly/', '.pdf')")
    cProfile.run("print_duplicates({})".format(table))

if __name__ == '__main__':
    main()

That being said, they problem statement seems foggy. The MD5 function does not yield the same hash for two different sets of data, when concerned with these kinda problems. That is way it is called a hash function or a one way function. If the hashes is identical the content is identical.

The last thing I will say, is that even the very fast hash function MD5, is slower then a efficient comparing of the content. So I criticize the problem not your solution.

Thanks! Good work.


Use reflection to check for class methods with a custom attribute This code is within a ViewModelBase class.  I have a custom attribute defined that is meant to trigger methods when a property is changed.  The following runs as the ViewModel is being constructed.  The idea is to catalog the methods that should be triggered upon a property changed event based on the property name.

It works but I was wondering if there was a way to more efficiently identify methods with the custom attribute, since there are a lot of methods to loop through and generally only one or two methods that have the sought-after attribute.

ExecuteOnPropertyChangedMap = new Dictionary<string, List<MethodInfo>>();

foreach (var method in this.GetType().GetMethods(
    BindingFlags.Instance | BindingFlags.Static | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.FlattenHierarchy))
{
    var execAttributes = method.GetCustomAttributes(typeof(ExecuteOnPropertyChangedAttribute), true);
    foreach (ExecuteOnPropertyChangedAttribute execAttribute in execAttributes)
    {
        if (execAttribute == null) continue;

        foreach (string propertyName in execAttribute.PropertyNames)
        {
            if (!ExecuteOnPropertyChangedMap.ContainsKey(propertyName))
                ExecuteOnPropertyChangedMap.Add(propertyName, new List<MethodInfo>());

            ExecuteOnPropertyChangedMap[propertyName].Add(method);
        }
    }
}

It's true that reflection is fairly slow, but for one-time operations the performance isn't usually a deal-breaker.

This method runs once-per-type, "some time before the view model is instantiated"

// optional type alias for clarity in this post
using MethodMap = System.Collections.Generic.Dictionary<System.String, System.Collections.Generic.List<System.Reflection.MethodInfo>>;

public class ViewModelBase : ...
{
    protected readonly MethodMap ExecuteOnPropertyChangedMap;

    const BindingFlags Flags = BindingFlags.Instance | BindingFlags.Static | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.FlattenHierarchy;
    protected static MethodMap GetMethodMap<T>() where T : ViewModelBase
    {
        // slight variation on your code here
        var t = typeof(T);
        var map = new MethodMap();
        // This is up to personal preference, but I recommend using 'var' and a constant for Flags to make your code as human readable as possible
        foreach (var method in t.GetMethods(Flags))
        {
            foreach (var execAttribute in method.GetCustomAttributes(typeof(ExecuteOnPropertyChangedAttribute), true))
            {
                if (execAttribute == null) continue;
                foreach (string propertyName in execAttribute.PropertyNames)
                {
                    if (!map.ContainsKey(propertyName))
                        map.Add(propertyName, new List<MethodInfo>());
                    map[propertyName].Add(method);
                }
            }
        }
        return map;
    }

    // ...

    protected ViewModelBase(MethodMap map)
    {
        ExecuteOnPropertyChangedMap = map;
    }
}

Then in each of your view model types, you could add

public class MyViewModel : ViewModelBase
{
    private static readonly MethodMap map = GetMethodMap<MyViewModel>();

    public MyViewModel : ViewModelBase(map)
    {
        // ...
    }
}

You can put the logic for reflection and building your map in your base class - but your base class doesn't know about its sub types. So each of your sub types is responsible for passing info about itself up to the base class to provide it with an instance-level reference to the sub types map.

By setting a private static readonly in the sub type we ask C# to 'calculate this some time before we need it but we don't care exactly when',
as opposed to putting it in your constructor. If you put the call to the reflection code in your constructor it happens every time you construct the object (like you pointed out). With this it only happens once, 'some time' before you instantiate the first MyViewModel. In practice, the field will usually be set right before the first MyViewModel constructor is called.


Vertex cover approximation in JavaScript I believe this runs in \$O(V+E)\$ but I wouldn't be able to explain the reasons why. I was looking for some general code review and any help on understanding the runtime.

var Graph = function() {
    this.adj = {};
}

Graph.prototype.addEdge = function(v, w) {
    if (!Array.isArray(this.adj[v])) this.adj[v] = [];
    if (!Array.isArray(this.adj[w])) this.adj[w] = [];

    this.adj[v].push(w);
    this.adj[w].push(v);
}

var vertexCover = function(G) {
    var visited = {};
    var cover = [];

    var u;
    for (var v in G) {
        if (!visited[v]) {
            for (var i = 0; i < G[v].length; i++) {
                u = G[v][i];
                if (!visited[u]) {
                    visited[u] = true;
                    visited[v] = true;
                }
            }
        }
    }

    for (var x in visited) {
        cover.push(x);
    }
    return cover;
}

var ex = new Graph();
ex.addEdge(0, 1);
ex.addEdge(0, 2);
ex.addEdge(1, 3);
ex.addEdge(3, 4);
ex.addEdge(4, 5);
ex.addEdge(5, 6);
console.log('graph', ex.adj);

console.log(vertexCover(ex.adj));

As you can see after adding edges to the graph, G[v] has the list of neighbours of v, correct?
Therefore the length of G[v] for the vertex v is equal to the number of edges joining v.
Notice that each edge u-v appears twice in G, once in the neighborhood set of u, and once in the neighborhood of v (G[u] contains v and G[v] contains u both referring to the single edge u-v).
So these being said let's have a look at the code.

We iterate over all vertices in the outermost for loop;
for each vertex v in the set of vertices, we iterate over all edges connected to it.

Since each edge has appeared twice in the list (as mentioned above), we are basically counting (or visiting) each edge twice using these two nested loops.
Hence, the complexity of these two for loops could be O(E) where E is the number of edges in the graph.

Notice that the outer loop, iterates through all vertices any way. So, the complexity of the runtime cannot be less than O(V). This being said the complexity of the algorithm is the maximum of O(E) and O(V), which is exactly what O(V+E) means.


Dynamic programming to solve two printers with different speeds I solved this problem using a Class, but thought I might try to figure out this memoization thing.

Problem
There are two printers that print pages at different speeds (X, Y).
What is the minimum amount of time it takes to print (N) pages?
Input Data First line contains the number of test cases Each test case
is on a new line in the form of X Y N
Output The minimum printing time for each test case separated by spaces

e.g.

input data:
2
1 1 5
3 5 4

output:
3 9

The math here is trickier than it appears, so make the assumption that my math is correct (it is).
Code
Option Explicit On
Option Strict On
Option Infer On
Option Compare Text

Module Module1

    <STAThread>
    Sub Main()
        Const inputPath As String = "C:\Temp\printers.txt"
        Dim delimiters As String = " " & System.Environment.NewLine
        Dim data() As Integer = System.IO.File.ReadAllText(inputPath).Split(delimiters.ToCharArray(), StringSplitOptions.RemoveEmptyEntries) _
                                .Select(Function(i) Integer.Parse(i)).ToArray
        Dim result As String

        For i As Integer = 0 To data(0) - 1
            result = result & BestTime(data(i * 3 + 1), data(i * 3 + 2), data(i * 3 + 3)) & " "
        Next

        result = result.Trim()
        System.IO.File.AppendAllText(inputPath, Environment.NewLine & result)
    End Sub

    Public Function BestTime(ByVal X As Double, ByVal Y As Double, ByVal N As Integer) As Double
        Dim myTimes(1) As Double
        For nextpage As Integer = N To 1 Step -1
            If myTimes(0) + X <= myTimes(1) + Y Then
                myTimes(0) = myTimes(0) + X
            Else
                myTimes(1) = myTimes(1) + Y
            End If
        Next
        Return myTimes.Max
    End Function

End Module

For some reason I couldn't get the delimiters to work as a constant. I'm using doubles because I kept getting overflow.

Additional test data
Input
16
206 120 2925144
5680 2268 173606
443 144 2231807
1336 1966 235314
251936 215499 755
1 1 610653060
27229864 68593621 7
13 11 58925803
25 13 39786943
936 301 970391
6811369 9145334 90
3070457 3923385 93
111 126 4757640
16350510 10897861 9
171127 91014 3379
2 2 373415582

Output
221808480 281383956 242540784 187180894 87708093 305326530 137187242
351099580 340283073 221013936 354191188 160858785 280761012 65387166
200776884 373415582

Your BestTime method looks great. The nextpage variable is never used, so it doesn't really matter which direction the loop goes. I'd be curious why you are using a decrementing loop, but it doesn't matter which direction you use in the grand scheme of things.

Choosing Double because Integer gives you overflow seems misleading. At first I thought "oh good, it handles decimal rates, like 2 1/2 pages per minute". The algorithm looks like it should work for floating point, though.

If you only want to support integral types, you can use Long or ULong. Based on the test cases you provided, Long is sufficient.

myTimes(0) + X and myTimes(1) + Y are calculated twice. For this scale of number it's not a huge performance loss. But if you were extending this to BigInteger levels, you could calculate them both once and assign the number if needed.

This is the kind of optimization that would require benchmarking before deciding if it is worthwhile.

A Const for delimeters won't work because System.Environment.NewLine is read only property that is not available at compile time. There are a couple of alternatives:

use a ReadOnly field instead of Const
use vbNewLine instead of System.Environment.NewLine

Note, this will (possibly) be different functionality.

However, you only use delimeters in one spot, so a method variable like you currently have is fine.

You did a good job separating the algorithm from input/output. You could take it further by wrapping input handling in its own function outside of Main.

If this weren't for a "competition" style of programming, I would recommend a parameter for the input file name so that the user can change it. I'd also strongly encourage a separate parameter for output. Right now, the user needs to perform an extra step before running your program a second time.

Additionally, you could use more descriptive variable names than X, Y, and N. It matches the competition specs so it's perfect in this instance. But for a long term program I would recommend something like printerXSpeed or numberOfPagesToPrint. The caveat is you never know when that one off function you write gets put in production for years.


Find group of available stalls in an aisle An aisle is defined as two rows of six stalls each.  A stall is adjacent to another if it is directly next to it or across from it.  My method finds out how many stalls can be found in a chain from the stall currently being worked with that are available for use.  For example, * is a stall that is already occupied, and . is a stall that is available.  With an aisle like this:

...*..
......

the get_available_adjacents method of the top left stall would return a set of eleven stalls, every stall except the occupied one.  With

...*..
...*..

the top left stall would return a set of six stalls because the occupied ones form a break in the line.  One of the occupied ones, however, would return a set of ten stalls because it is connected to both groups of available stalls.  My solution below seems inefficient.  With only twelve stalls per aisle, it is figured out quickly enough, but I would like to know if there is a more efficient solution.

It works by creating a set found to store all of the stalls that we find.  It also creates a set used that contains all stalls for which the get_available_adjacents method has been called.  It then loops through each adjacent of the current stall, and if it is available, adds it to the used set and updates found by calling the adjacent's get_available_adjacents method.

class Stall:
    def __init__(self, occupant=None):
        self.occupant = occupant
        self.available = True

    def get_available_adjacents(self, used=None):
        if used is None:
            used = {self}

        found = set()
        if self.available:
            found.add(self)

        for adjacent in self.adjacents:
            if adjacent in used or not adjacent.available:
                continue
            used.add(adjacent)
            found.update(adjacent.get_available_adjacents(used))
        return found

For testing:

class Aisle:
    def __init__(self, row_length=6):
        self.top = [Stall() for _ in range(row_length)]
        self.bottom = [Stall() for _ in range(row_length)]

        self.top[0].adjacents = (self.top[1], self.bottom[0])
        self.top[-1].adjacents = (self.top[-2], self.bottom[-1])
        self.bottom[0].adjacents = (self.bottom[1], self.top[0])
        self.bottom[-1].adjacents = (self.bottom[-2], self.top[-1])

        for i, (t, b) in enumerate(zip(self.top[1:-1], self.bottom[1:-1]), 1):
            t.adjacents = (self.top[i-1], self.top[i+1], b)
            b.adjacents = (self.bottom[i-1], self.bottom[i+1], t)

aisle = Aisle()
print(aisle.top[0].get_available_adjacents()) # gives 12 results
aisle.top[2].available = False
print(aisle.top[0].get_available_adjacents()) # gives 11 results
aisle.bottom[2].available = False
print(aisle.top[0].get_available_adjacents()) # gives 4 results
print(aisle.top[2].get_available_adjacents()) # gives 10 results

There are no docstrings. What does an object belonging to the Stall class represent? What do the methods do? How should I call them?
The get_available_adjacents method relies on the adjacents attribute. But there is no method on the class that sets this attribute. This makes it hard to reason about the behaviour of this method.

Update: I recognize that you can't supply the adjacent stalls at initialization time (because maybe they haven't been created yet), so I recommend initializing the adjacents attribute to an empty collection in the __init__ method, and providing a method to extend the collection.

Alternatively (as suggested by Sjoerd Job Postmus in comments), since aisles have a standard layout, it might make sense to move the adjacency computation to the aisle class. If you do the latter then you'll find that the Stall class becomes redundant — all it represents is whether there is an occupant or not, and that could be represented by an Occupant object, or None if there is no occupant. (Assuming that no occupant can occupy more than one stall.)
I don't like the way that get_available_adjacents has been implemented as a method on the Stall class. That's because finding an available set of stalls in an aisle is an operation on an aisle, not an operation on an individual stall. Methods should be placed where they make logical sense, not just attached to any old class.
The algorithm you have implemented is known as flood fill. If you read the Wikipedia article, you'll see a couple of approaches for making it faster: (i) use a queue instead of a stack, to avoid function call overhead; (ii) run as far as you can along horizontal rows to reduce the amount of queue management. (But neither of these approaches improves the efficiency: all algorithms take time proportional to the size of the flooded region.)


Grouping and summing values for food takeout I have 4 properties which I calculate using LINQ:

public decimal TotalEatin { get; set; }
public int EatinAmount { get; set; }

public decimal TotalTakeout { get; set; }
public int TakeoutAmount { get; set; }

The way I generate the values are done correctly this way, but I think the code is a bit cumbersome because of the repeated iteration that is needed.

Because I need to generate the values from different DataTables, I have no idea how I can improve this using one "large" query. My knowledge of LINQ / SQL-like syntax is simply way too low.

Is it somehow possible to put these properties in a separate class, and use proper LINQ to generate one instance of this class with the right property values?

TotalEatin = Data.kassaOrders.AsEnumerable().Where(x => x.Field<bool>("btwticket") && !x.Field<bool>("terugname") && !x.Field<bool>("teruggenomen"))
    .SelectMany(x => Data.orderDetails.AsEnumerable().Where(d => d.Field<int>("orderid") == x.Field<int>("serial") && !d.Field<bool>("takeout")))
    .Sum(x => x.Field<decimal>("price"));

TotalEatin += Data.kassaOrders.AsEnumerable().Where(x => x.Field<bool>("btwticket") && !x.Field<bool>("terugname") && !x.Field<bool>("teruggenomen"))
    .SelectMany(x => Data.orderDetails.AsEnumerable().Where(d => d.Field<int>("orderid") == x.Field<int>("serial") && !d.Field<bool>("takeout")))
    .SelectMany(x => Data.MenuDetails.AsEnumerable().Where(d => d.Field<int>("orderid") == x.Field<int>("orderid")))
    .Sum(x => x.Field<decimal>("prijs"));

EatinAmount = Data.kassaOrders.AsEnumerable().Where(x => x.Field<bool>("btwticket") && !x.Field<bool>("terugname") && !x.Field<bool>("teruggenomen"))
    .SelectMany(x => Data.orderDetails.AsEnumerable().Where(d => d.Field<int>("orderid") == x.Field<int>("serial") && !d.Field<bool>("takeout")))
    .GroupBy(x => new { takeout = x.Field<bool>("takeout"), orderid = x.Field<int>("orderid") }).Distinct().Count();

TotalTakeout = Data.kassaOrders.AsEnumerable().Where(x => x.Field<bool>("btwticket") && !x.Field<bool>("terugname") && !x.Field<bool>("teruggenomen"))
    .SelectMany(x => Data.orderDetails.AsEnumerable().Where(d => d.Field<int>("orderid") == x.Field<int>("serial") && d.Field<bool>("takeout")))
    .Sum(x => x.Field<decimal>("price"));

TotalTakeout += Data.kassaOrders.AsEnumerable().Where(x => x.Field<bool>("btwticket") && !x.Field<bool>("terugname") && !x.Field<bool>("teruggenomen"))
    .SelectMany(x => Data.orderDetails.AsEnumerable().Where(d => d.Field<int>("orderid") == x.Field<int>("serial") && d.Field<bool>("takeout")))
    .SelectMany(x => Data.MenuDetails.AsEnumerable().Where(d => d.Field<int>("orderid") == x.Field<int>("orderid")))
    .Sum(x => x.Field<decimal>("prijs"));

TakeoutAmount = Data.kassaOrders.AsEnumerable().Where(x => x.Field<bool>("btwticket") && !x.Field<bool>("terugname") && !x.Field<bool>("teruggenomen"))
    .SelectMany(x => Data.orderDetails.AsEnumerable().Where(d => d.Field<int>("orderid") == x.Field<int>("serial") && d.Field<bool>("takeout")))
    .GroupBy(x => new { takeout = x.Field<bool>("takeout"), orderid = x.Field<int>("orderid") }).Distinct().Count();

Scope

If all you need is to calculate some values from a set of data tables, it would suffice to use LINQ. However, if you prefer to provide an API with common functionality concerning orders and order details, you should really create your own domain classes and map the data tables to your domain classes. For the sake of this question, I would opt for using LINQ directly on the data tables.

Review

You are iterating your 3 data tables kassaOrders, orderDetails and menuDetails over and over again with each order detail having a link to a kassa order and each menu detail having a link to a kassa order. We can easily optimize performance and reduce code redundancy by caching these tables in lookup collections.

Lookup Tables

Start by caching the kassa orders in a dictionary by order id (called serial in the orders). This way, fast lookup is possible to join the details.

Data.kassaOrders.AsEnumerable()
    .Where(x => x.Field<bool>("btwticket") &&
          !x.Field<bool>("terugname") && !x.Field<bool>("teruggenomen"))

 var orders = Data.kassaOrders
    .AsEnumerable()
    .Where(x =>
         x.Field<bool>("btwticket") &&
        !x.Field<bool>("terugname") &&
        !x.Field<bool>("teruggenomen"))
    .ToDictionary(order => order.Field<int>("serial"));

We can then use lookup tables - which are dictionaries that store a sequence of values instead of a value - to cache the order and menu details by order id. This code shows how to do it for eating in, equivalent LINQ could be written for take out.

Data.orderDetails.AsEnumerable()
   .Where(d => d.Field<int>("orderid") == x.Field<int>("serial")
       && !d.Field<bool>("takeout"))

 var orderDetailsEatIn = Data.orderDetails
    .Where(order => !order.Field<bool>("takeout"))
    .ToLookup(detail => detail.Field<int>("orderid"));

and

Data.MenuDetails.AsEnumerable()
   .Where(d => d.Field<int>("orderid") == x.Field<int>("orderid"))

var menuDetailsEatIn = Data.MenuDetails
   .Where(order => !order.Field<bool>("takeout"))
   .ToLookup(detail => detail.Field<int>("orderid"));

Calculate Amount

You're calculating the amount using a peculiar GroupBy. There is no reasong to group by the take out variable, because you filter on a specific value right before !d.Field<bool>("takeout"). The call to Distinct also doesn't make sense since details are already grouped by key and keys are distinct anyway.

GroupBy(x => new {
    takeout = x.Field<bool>("takeout"),
    orderid = x.Field<int> ("orderid") }).Distinct().Count();

Using our lookup tables, we can easily calculate the amount as below. To stay within your spec, we count the number of orders that have at least one order detail available. Note that an empty collection is returned orderDetailsEatIn[key] is the key is not present in an ILookup.

 var EatinAmount = orders
    .Select(order => orderDetailsEatIn[order.Key])
    .Count(details => details.Any());

Calculating Total

Your spec is a bit strange to me. I would expect Order 1-->* OrderDetail 1-->* MenuDetail. Instead the cardinality is Order 1-->* OrderDetail, Order 1-->* MenuDetail with the price for each menu detail multiplied by the number of order details to calculate the subtotal by menu detail.

We could calculate the subtotal for the order details as follows:

var subtotalOrderDetailEatIn = orders
    .SelectMany(order => orderDetailsEatIn[order.Key])
    .Select(detail => detail.Field<decimal>("price"))
    .DefaultIfEmpty(0).Sum();

and the subtotal for menu details:

var subtotalMenuDetailEatIn = orders
    .SelectMany(order => orderDetailsEatIn[order.Key]
        .SelectMany(detail => menuDetailsEatIn[order.Key]))
    .Select(detail => detail.Field<decimal>("prijs"))
    .DefaultIfEmpty(0).Sum();

yielding a grant total of:

var TotalEatin = subtotalOrderDetailEatIn + subtotalMenuDetailEatIn;

Note that this is just one possible way to use LINQ to calculate these values from data tables. Other ways could include:

Create intermediate classes, value tuples or domain classes to store strongly typed properties to facilitate querying readability.
A more advanced LINQ approach using GroupJoin to build complex classes given several flattened collections and join selectors.

It's really up to you to see how comfortable you are exploring the realms of LINQ.


Visualizing Newton-Raphson method for finding zeroes of a function I have created a program to visualize the working of Newton-Raphson method to find the zeroes of a function:

newton.m:

% Dummy statement to avoid writing function in the first line and making it a 'function file' instead of a 'script file'
1;

% The function to find zeroes of.
% The function is specifically chosen to not have any zeroes
% so as to show the weakness of Newton's method.
function y = f(x)
    y = (x - 5).^2 + 5;
endfunction

% The derivative of f(x)
function y = fd(x)
    y = 2 * (x - 5);
endfunction

% Initial guess
x0 = 1.5;

% Max number of iterations
itermax = 20;

% Epsilon value initialized to a very large value
eps = 1;

% A vector for storing the history of the approximate roots
xvals = x0;

% Number of iterations done
itercount = 0;

% Required for plotting f(x) vs x
x = linspace(0, 10, 100);

% Create a figure whose output is not rendered on the screen
% Not working currently; supposedly a bug in Octave
% A workaround is to use gnuplot instead of qt - `graphics_toolkit gnuplot`
% but this is very slow.
% Uncomment the following to activate the feature once the bug is fixed
% figure('Visible','off');

% The main loop
while eps >= 1e-5 && itercount <= itermax
    % x1 = New value of root
    % x0 = Current value of root
    x1 = x0 - f(x0) / fd(x0);

    % Plot f(x)
    % Plot a line passing through points [x0, f(x0)] and [x1, 0]
    % Plot a line passing through points [x1, 0] and [x1, f(x1)]
    % Plot a line passing through points [x0, 0] and [x0, f(x0)]
    plot(x, f(x), ";f(x);", [x0 x1], [f(x0) 0], "-r;f'(x);", [x1 x1], [0 f(x1)], ":r", [x0 x0], [0 f(x0)], ":r");
    title('f(x) = (x-5)^2 + 5');

    % Set limits for the axes shown in the plots
    xlim([0 10]);
    ylim([0 30]);

    % Label the two consecutive zeroes on the X-axis
    text(x0, -2, sprintf('x%d', itercount), 'color', 'red');
    text(x1, -2, sprintf('x%d', itercount+1), 'color', 'red');

    % Print the plot to a file
    filename = sprintf('output/%05d.jpg', itercount);
    print(filename)

    % Append the zero to the array of zeroes calculated so far
    xvals = [xvals; x1];

    % Calculate the epsilon value
    eps = abs(x1-x0);

    x0 = x1;
    itercount = itercount+1;
end

% Print the result of the iteration
xvals
f_zero = f(xvals(end))
eps
itercount

This program produces a series of images which I have combined to create an animation:

The command I used to convert the images is:

convert -delay 100 -loop 0 *.png animation.gif

This code is also stored here.

I would like to get it reviewed on the following:

Octave conventions and coding patterns (This is my first programming attempt using Octave)
Any improvement on the algorithm itself
Readability and performance of the code
Method and quality of producing animations

Conventions, coding patterns and readability

Overall, your code looks nice! I have never seen a code where you insert a line in the beginning to avoid the script being interpreted as a function. It works, but I wouldn't do it like that.

The two functions you have are very simple. There is 1: no recursion. 2: no loops 3: no handling of invalid input. For such functions you can simply create anonymous functions. This way you can avoid the first line of code in your script.

% The function to find zeroes of.
% The function is specifically chosen to not have any zeroes
% so as to show the weakness of Newton's method.
f = @(x) (x - 5).^2 + 5;

% The derivative of f(x)
fd = @(x) 2 * (x - 5);

You have very descriptive variable names. However, eps is not one of them, as eps is a built-in function and variable.

You have a growing vector in your loop xvals = [xvals; x1];. This is not recommended, as it is very slow. It's faster to pre-allocate memory for a vector that will be "large enough", and trim it down afterwards:

xvals = zeros(20,1);
while ...
...
end
xvals = xvals(1:iter);  % or xvals(iter+1:end) = [];

Also, if you don't want to pre-allocate memory, you can increase performance by substituting xvals = [xvals; x1] with:

xvals(end+1) = x1;

You should use < itermax instead of <= itermax in the loop condition. Otherwise you'll end up with itercount = itermax + 1.

Note: Always try to keep compatibility with MATLAB when programming in Octave.

Don't use itercount ++ instead of itercount = itercount + 1, because it's easier.
Don't do a = b = c = 0 because it's easier.
Don't do inline assignments b = (a=3) * a just because you can.
And it might not be recommended by everyone, but I recommend you to use end instead of endfunction, endif etc to keep compatibility with MATLAB.

This is all I can do for now... Good luck!


Construct a monochromatic graph flipping nodes of the edges Given a letter (A or B) representing the color for N nodes of a graph and the set of M edges. Its possible to make all the graph have the letter A by flipping the nodes connected by an edge ?

Example:

  Given the following letter graph:
  A - B

  Its impossible to make all the letters A by flipping the nodes connected by some edge.

  With this graph is possible to make it (Just flip the second edge ):
  A - B - B

To solve this problem i've tried brute force. For every edge the program make two paths (flip the ith edge or not flip). I've tried to memoize some paths that was visited in the search to make it more faster.

Description of the input:

  There will be several test cases. Each test case begins with two integers: N (1 ≤ N ≤ 1000) and M (1 ≤ M ≤ 4000). The next line contains N letters, indicating the starting letter of each node. Then follows M lines, each with two integers, a and b (1 ≤ a,b ≤ N and a != b), describing that there is an edge from a to b. Input ends with EOF.

Description of the output:

  For each test case you should output Y if it is possible to meet the requirements or N otherwise.

Above example input:

2 1
A B
1 2
3 2
A B B
1 2
2 3

Above example output:

N
Y

#include <cstdio>
#include <vector>
#include <utility>
#include <unordered_map>
#include <algorithm>

#define A 0
#define B 1

using edge = std::pair<int, int>;

std::unordered_map<unsigned int , int > visited_state;
std::vector<edge> edge_list;
std::vector<char> node_letters;

bool circuit;

bool isAllA(){
    for (int i = 0; i < node_letters.size(); ++i)
        if(node_letters[i] == B)
            return false;
    return true;
}

//flip the color of the nodes in the eth edge
void flipColors( int e ){
    int u = edge_list[e].first;
    int w = edge_list[e].second;

    node_letters[u] = !node_letters[u];
    node_letters[w] = !node_letters[w];
}

unsigned int getHashState(){
    unsigned int hash1 = 63689 , hash2 = 378551;
    unsigned int hash_state = 0;

    for (int i = 0; i < node_letters.size(); ++i){
        hash_state = hash_state * hash1 + node_letters[i];
        hash1 *= hash2;
    }

    return hash_state;
}

void brute_search( int i ){

    //if its possible ends the recursion
    if ( circuit )
        return;

    unsigned int hash = getHashState();

    if (  visited_state[ hash ] ) {

        //this state was visited on higher depth (dont need to search again)
        if ( visited_state[ hash ] >= i + 1){
            //puts("test repeated problems");
            return;
        }

    }

    visited_state[ hash ] = i + 1;

    if(i == edge_list.size() ){
        circuit = isAllA();
        return;
    }

    //search without change the nodes on the ith edge
    brute_search(i + 1);

    //test with the changes
    flipColors(i);
    brute_search(i + 1);
    flipColors(i);

}

int main ( void ){
    int n,m;

    while( scanf("%d %d ",&n,&m) != EOF ) {

        //init and reset global variables
        circuit = false;
        node_letters.clear();
        edge_list.clear();
        visited_state.clear();

        //read the letter of each node
        for ( int i = 0 ; i < n ; ++i ) {
            node_letters.push_back(getchar() - 'A');
            getchar();//remove space
            //printf("%d\n",c[i]);
        }

        //read the edges
        for (int i = 0; i < m; ++i) {
            int u,w;
            scanf("%d %d",&u,&w);
            edge_list.push_back(edge(u - 1,w - 1));
            //printf("%d %d\n",v[i].first,v[i].second);
        }

        brute_search(0);

        putchar(circuit ? 'Y' : 'N');
        putchar('\n');
    }
}

This is not a review, but a comment too long to be a comment.

Brute force is almost always wrong. In the tasks like this it is always wrong.

Let the graph has a nodes colored A, and b nodes colored B.

First notice that flipping an edge doesn't change the parity of neither a nor b. From this, it follows immediately that the if both a and b are odd, the graph cannot be made monochromatic.

It is a bit harder to prove the opposite, that is if at least one of a and b is even, a connected graph can be made monochromatic. Prove that given two vertices of a given color there is a sequence of A-B flips so that two vertices of a given color become adjacent. Then use induction.

Summing up, the solution is to identify the connected components of the graph, and for each of them count the color's parities.


Potential uninitialized access in zlib The zlib library is a venerable piece of C software. The copyright for the decompression routine goes back to 1995, with the last rewrite in 2002. It is used daily by millions.

To be perfect, the decompression routine should not invoke undefined behavior, even if passed a maliciously crafted buffer. I have the opportunity to detect a number of places where it is not obvious that the library does not invoke undefined behavior on some input. Would users of this site help me in confirming these issues? We would improve an already great piece of Open Source software, and there is always something to learn in reading code that has passed the test of time.

Assuming this is okay, for my first post, I would like to suggest looking at function inflate() in version 1.2.7 of the library.

int ZEXPORT inflate(strm, flush)
z_streamp strm;
int flush;
{
    struct inflate_state FAR *state;
    state = (struct inflate_state FAR *)strm->state;
/*** strm->state was previously allocated with malloc(), apparently.
Function malloc(), when it succeeds,
returns an uninitialized block of memory. */
...
    for (;;)
        switch (state->mode) {
...
        case MATCH:
            if (left == 0) goto inf_leave;
            copy = out - left;
            if (state->offset > copy) {         /* copy from window */
/*** It is unclear if state->offset always contains initialized contents
at this point, line 1125 in inflate.c.
*/
...

So my question is, is it possible to reach line 1125 in inflate.c without having initialized state->offset?

I linked the library with the following main() function to identify the possible issue, so I guess that an input vector, if one exists, could be based on this main(), simply initializing array in[] to values that cause the issue. This main() is using the library correctly, I hope, otherwise the flagged issue may be meaningless.

#include "zlib.h"

#define CHUNK 100

z_stream strm;
unsigned char in[CHUNK];
unsigned char out[CHUNK];
int ret;

main(){
  int i;

  /* allocate inflate state */
  strm.zalloc = Z_NULL;
  strm.zfree = Z_NULL;
  strm.opaque = Z_NULL;
  strm.avail_in = 0;
  strm.next_in = Z_NULL;
  ret = inflateInit(&strm);
  if (ret != Z_OK)
    return ret;

  for (i=0; i<CHUNK; i++)
    in[i] = any();

  strm.next_in = in;
  strm.avail_in = CHUNK;
  strm.next_out = out;
  strm.avail_out = CHUNK;

  ret = inflate(&strm, Z_NO_FLUSH);
}

Of course, if the apparently dangerous access at line 1125 is in fact safe, then a great answer would be an explanation why.

I am not an expert in the zlib source tree nor am I familiar with the coding style of its authors, but I found this to be an interesting question.  I'd say it looks safe and my guess is the original authors are expecting offset to be initialized to zero. Update: OK, I misread an if statement; looking at it again it looks like it might be unsafe without a custom allocator, so we'd have to look at inflateInit et al. and the actual inflate logic to know for sure.

Explanation follows:

It looks like inflate_state is allocated with the ZALLOC macro.

That macro resolves to:

#define ZALLOC(strm, items, size) \
       (*((strm)->zalloc))((strm)->opaque, (items), (size))

According to comments in zlib.h, correct use of the library requires the caller to initialize the zalloc member being called:

The application must initialize zalloc, zfree and opaque before
   calling the init function.

If strm->zalloc is NULL, which I'd expect most callers to do, the library uses its own default implementation, zcalloc.

zcalloc, with some exceptions for ancient MS-DOS compilers and so long as sizeof(unsigned int) is greater than 1, ends up calling calloc.

calloc by definition zeroes out its data.

So, I'd say it's safe under the following conditions:

If you use a custom allocator, make sure it zeroes out the data.

2. sizeof(unsigned int) must be > 1.  (I'm guessing the standard guarantees that but I'm not certain.)

After the update: Now realizing that the allocations are not necessarily zeroed, I'd say it all depends on whether or not the local variable here can be used uninitialized, as the only assignment I see is based on that value.

Note that inflateInit, which calls inflateReset, which calls inflateResetKeep, seems to initialize the members that the assignments to here depends on, notably lenbits.  It looks like there are lots of size checks for overflow as well, eg: if ((unsigned)(here.bits) <= bits) break;.  I haven't given it a thorough audit but I'd say everything I've seen so far looks safe.


CoffeeScript 2 + 2 program I tested this program on Smooth-CoffeeScript

I just wanted to have as many eyes on this as I could to learn about coffeescript;

Is there anything I should do to improve it?

Am I using the tools available to me adeptly? (i.e. good variable naming, layout, commenting, etc.)

userQuestion   = '¿ what is 2 + 2 ?'
correctLength = 1
correctAnswer = '4'
correctAnswerMsg = "That's correct!"
closeMsg = "Almost there!"
digitsOnlyMsg = "Digits only!"
singleDigitMsg = "Single digit!"
tryAgainMessage = "Try again!"

prompt userQuestion, '', (answer) ->  # the answer can not be set as a variable here
                                      # (answer) grabs the answer from the console
  userEntry = answer                  # -> casts that answer down to the rest of the indent to be used plainly
                                      # at least that's how I think it works :/

  show userEntry  # the user's answer always shows on the first line

  if userEntry.length isnt correctLength  # .length only works when comparing the length of a string to an integer
    show singleDigitMsg

  userEntry = (Number) userEntry
  correctAnswer = (Number) correctAnswer  # now we convert our user entry and our correct answer into integers so the math functions work

  if userEntry is correctAnswer
    show correctAnswerMsg
    exit program

  else if userEntry is correctAnswer - 1 or userEntry is correctAnswer + 1
    show closeMsg

  else if isNaN(userEntry)
    show digitsOnlyMsg

  else  # this only shows when the user types in 1, 2, or 6-9. Or multiple characters with a digit, like '-4'
    show tryAgainMessage

This is a a very, um, thorough way of doing things. Not necessarily a bad thing, and it does help learn some CoffeeScript, so yay for that, but still it's a little much. I'll roll with it though.

Before talking about the code proper, though, I have to point out that prompt, show and exit program aren't part of CoffeeScript or JavaScript. That would appear to be something wholly custom to that smooth-coffeescript site you mention. In other words, those only work on that site, so don't think it's something you can use anywhere else. In the code below I'll use alert instead of show (since works in browsers), and I'll get to the exit program thing in a second.

prompt is another matter, since that does exist in browsers, but it works differently. Instead of taking a callback function, it simply returns a string (or null). So this is actually pretty bad of the smooth-coffeescript author(s), since it doesn't merely add custom functionality, but takes an existing, well-documented feature of browser-based JavaScript and makes it behave differently, confusing everyone.

So just know that the stuff you write on that site, while it might teach you CoffeeScript, it can also mislead you.

Now, some notes on the code:

Put the strings and other variables inside the prompt function; don't pollute the global namespace. The only one you have to keep outside is the question itself, as that's used when calling prompt to begin with.
userEntry = answer does nothing useful. It just gives you two variables both containing what the user entered. Nothing gained.
This part has some weirdness:

if userEntry is correctAnswer
  show correctAnswerMsg
  exit program

As mentioned exit program is not standard. In Node.js you might use process.exit to explicitly quit/stop the entire script and runtime, but that the nearest thing that's actually common to see. However, you don't need to exit the script/program. Since you're inside a function (the callback for prompt), you can just say return when you want to stop. Since there's no other code after the call to prompt, the script will exit on its own.

However, you don't even need to return. You've written an if.. else if.. else structure, so if the user's answer is correct, none of the other else/else if branches will be run. So there's nothing to stop or return from.
Checking for both length and the number itself is a hassle. Both because if the number's wrong, who cares if the length was right? Secondly, it requires you to specify both length and value of the correct answer, meaning you have two things to change if you want to make the question what is 3 + 11? instead. And you'd have to change the singleDigitMsg text too - and the name of the singleDigitMsg variable itself - since neither of those make sense anymore.

I'd instead check "did the user enter only digits?" and "did they enter the correct number?". Checking "did the user enter something that's the same length as the string representation of the correct number?" is somehow both too precise and too vague to be useful.
For checking if the user's just-1-off from the correct answer, don't check if it's correct-1 or correct+1; check if the difference is 1.

Here's what I'd write (run the snippet, and open the developer console to see the output)

# a less confusing wrapper for the browser-native `prompt` function
promptWithCallback = (message, callback) ->
  response = prompt message
  callback response unless response is null

# the actual thing
promptWithCallback "What's 2 + 2?", (answer) ->
  correctAnswer = 4

  # complain if answer contains non-digit character
  if /\D/.test answer
    alert "Only digits, please"
    return # stop here

  # instead of casting to a Number, use parseInt with a radix/base argument
  # this avoids the string "08" being converted to NaN because JS thinks it's
  # an octal (base 8) number (due to the leading zero) that's invalid (due to
  # it containing an 8). Doesn't happen in most runtimes, but still
  number = parseInt answer, 10

  if number is correctAnswer
    alert "Yup, correct!"

  else if Math.abs(number - correctAnswer) is 1
    alert "Nope - but you were really close though. Try again"

  else
    alert "Nope. Try again"

If you want to try it in your browser (couldn't make it work with stack snippets due to a http/https conflict), go to coffeescript.org, click the "Try CoffeeScript" button at the top, paste in the code, and click "run".

You'll note that I didn't add variables for all the different strings. Frankly, I just didn't want to bother with that, since it's all pretty simple. It's not necessarily a bad thing to do (it puts all the text in one place, making it easier to change), but it's not a huge piece of code, so I just wrote it in place.


Merge sort efficiency I've written this Java code to implement merge sort:

import java.util.*;

public class MergeSort {

    int[] a;

    MergeSort() {
        Scanner scan = new Scanner(System.in);
        System.out.println("Enter the number of items: ");
        a = new int[scan.nextInt()];

        for (int i = 0; i < a.length; ++i) {
            a[i] = scan.nextInt();
        }

    }

    int[] mergeSort2(int beg, int end) {
        if (beg == end) {
            int []result = new int[]{a[beg]};         /****new array allocation****/
            return result;
        }

        int[] left = mergeSort2(beg, (beg + end) / 2);
        int[] right = mergeSort2((beg + end) / 2 + 1, end);
        int[] result = new int[end - beg + 1];        /****new array allocation****/
        System.out.println("left.length = "+left.length+", right.length = "+right.length+", result.length = "+result.length);

        int i, l, r;
        for (i = 0, l = 0, r = 0; l < left.length && r < right.length; ++i) {
            if (left[l] >= right[r]) {
                result[i] = right[r];
                r++;
            } else if (left[l] < right[r]) {
                result[i] = left[l];
                l++;
            }
        }

        if (l == left.length) {
            for (; r < right.length; ++r, ++i) {
                result[i] = right[r];
            }
        } else if (r == right.length) {
            for (; l < left.length; ++l, ++i) {
                result[i] = left[l];
            }
        }
        return result;
    }

    public static void main(String[] args) {
        MergeSort ms = new MergeSort();
        int[] result = ms.mergeSort2(0, ms.a.length - 1);
        System.out.println();
        for (int i = 0; i < result.length; ++i) {
            System.out.print(result[i] + " ");
        }
    }
}

Every time I want to return left and right to merge into result, I've to allocate result again (same for cases where length == 1). How can I avoid this allocation during recursion?

You can use a temporary array, see for example here or here.

As this would change quite a lot, I'm not going to rewrite your code to match those examples.

But I do have some additional comments which might improve the readability and ease of use of your code:

Constructor

A constructor should only construct an object. Your constructor also gathers user input. This makes it extremely hard to test your class.

Just let MergeSort accept an array, which you created in a ConsoleInput class (or main).

Or pass the array directly to the sort method and just use an empty constructor.

Merging

Your merging code is a bit confusing, because you are using for loops when while would be a better fit:

    int l = 0;
    int r = 0;
    int arrayPosition = 0;
    while (l < left.length && r < right.length) {
        if (left[l] >= right[r]) {
            result[arrayPosition] = right[r];
            r++;
        } else {
            result[arrayPosition] = left[l];
            l++;
        }
        arrayPosition++;
    }

    // copy rest
    while (l < left.length) {
        result[arrayPosition] = left[l];
        l++;
        arrayPosition++;
    }
    while (r < right.length) {
        result[arrayPosition] = right[r];
        r++;
        arrayPosition++;
    }

With this code, you can probably also better see the similarities to the code that uses a temporary array.

And to copy the rest, you could also use arraycopy, which is probably faster:

    if (l == left.length) {
        System.arraycopy(right, r, result, arrayPosition, right.length - r);
    } else if (r == right.length) {
        System.arraycopy(left, l, result, arrayPosition, left.length - l);
    }

Other

naming: mergeSort2 why 2? And just write beginning instead of beg, it's not that long.
private: fields should be private (unless there is a good reason to make them public), same with methods.
create a merge method, it makes your code easier to read.
the caller shouldn't need to know with what parameters to call mergeSort2. I would create a public method without arguments (or which accepts the array) which then calls mergeSort2 with the correct parameters.


Prevent rewriting multiple vars Taking a look at the code, is there a better way in JavaScript to prevent so much repetition. In particular, re declaring the variables like moduleOptions, removeBtn, moveUp?

I tried putting them in their own key values pairs. Unfortunately, that causes them to be called right away, and the selectors that I'm querying are not ready until the toggleModuleOptions.show() and toggleModuleOptions.hide() functions are called.

  var toggleModuleOptions = {
    show: function() {
      var moduleOptions = visualModules.activeModuleContainer.querySelector('.module-options');

      moduleOptions.classList.add('module-options--active');

      removeBtn = visualModules.activeModuleContainer.querySelector('.js-remove');
      removeBtn.addEventListener('click', deleteModule);

      moveUp = visualModules.activeModuleContainer.querySelector('.js-index-up');
      moveUp.addEventListener('click', move);
      moveUp.direction = 'up';

      moveDown = visualModules.activeModuleContainer.querySelector('.js-index-down');
      moveDown.addEventListener('click', move);
      moveDown.direction = 'down';
    },
    hide: function() {
      var moduleOptions = visualModules.activeModuleContainer.querySelector('.module-options');

      if(moduleOptions) {
        moduleOptions.classList.remove('module-options--active');
      }

      removeBtn = visualModules.activeModuleContainer.querySelector('.js-remove');
      removeBtn.removeEventListener('click', deleteModule);

      moveUp = visualModules.activeModuleContainer.querySelector('.js-index-up');
      moveUp.removeEventListener('click', move);

      moveDown = visualModules.activeModuleContainer.querySelector('.js-index-down');
      moveDown.removeEventListener('click', move);
    }
  }

This version tries not to change functionality. Added two functions that reduce code duplication, both methods make use of javascript's [](square bracket) property accessor syntax which allows the functions to call different functions based on the parameter passed in.

var addListener = function(selector, method, listener) {
    var selected = visualModules.activeModuleContainer.querySelector('.js-' + selector);
    selected[method + 'EventListener']('click', listener);
    return selected;
};
var showOrHide = function (method, direction) {
    var moduleOptions = visualModules.activeModuleContainer.querySelector('.module-options');

    if (moduleOptions) {
        moduleOptions.classList[method]('module-options--active');
    }

    removeBtn = addListener('remove', method, deleteModule);

    moveUp = addListener('index-up', method, move);

    moveDown = addListener('index-down', method, move);

    if (direction) {
        moveUp.direction = 'up';
        moveDown.direction = 'down';
    }
};
var toggleModuleOptions = {
    show: function() {
        showOrHide('add', true);
    },
    hide: function() {
        showOrHide('remove', false);
    }
};

This version could change some things but very minor.  This one always sets the direction properties even on the one that wasn't being set before to avoid needing conditional logic for setting those properties. It also does not assign values to variables from outside the scope of the code shown which was only left in above in case it was necessary.

var addListener = function(selector, method, listener, direction) {
    var selected = visualModules.activeModuleContainer.querySelector('.js-' + selector);
    selected[method + 'EventListener']('click', listener);
    selected.direction = direction;
};
var showOrHide = function (method) {
    var moduleOptions = visualModules.activeModuleContainer.querySelector('.module-options');

    if (moduleOptions) {
        moduleOptions.classList[method]('module-options--active');
    }

    addListener('remove', method, deleteModule);

    addListener('index-up', method, move, 'up');

    addListener('index-down', method, move, 'down');
};
var toggleModuleOptions = {
    show: function() {
        showOrHide('add');
    },
    hide: function() {
        showOrHide('remove');
    }
};


Star Rating in Swift So I needed a a star rating feature to appear in my app - it doesn't need to be interactive.

I have my app connected to a database I've set up in Parse, and I've made a column for OurRatingof type Number, and I just set a number between 0.5 and 5.

In my custom cell, I have 5 images which will display my star images. The code I've done is as follows:

//I define my images at the top

var fullStarImage: UIImage = UIImage(named: "starFull.png")!
var halfStarImage: UIImage = UIImage(named: "starHalf.png")!
var emptyStar: UIImage = UIImage(named: "starEmpty.png")!

Then further down in my table method, I have:

//Star Rating System
    if let ourRating = object?["OurRating"] as? Double {

    if ourRating == 0.5 {
        cell?.star1.image = halfStarImage
    }
    if ourRating == 1 {
        cell?.star1.image = fullStarImage
    }
    if ourRating == 1.5 {
        cell?.star1.image = fullStarImage
        cell?.star2.image = halfStarImage
    }
    if ourRating == 2 {
        cell?.star1.image = fullStarImage
        cell?.star2.image = fullStarImage
    }
    if ourRating == 2.5 {
        cell?.star1.image = fullStarImage
        cell?.star2.image = fullStarImage
        cell?.star3.image = halfStarImage
    }
    if ourRating == 3 {
        cell?.star1.image = fullStarImage
        cell?.star2.image = fullStarImage
        cell?.star3.image = fullStarImage
    }
    if ourRating == 3.5 {
        cell?.star1.image = fullStarImage
        cell?.star2.image = fullStarImage
        cell?.star3.image = fullStarImage
        cell?.star4.image = halfStarImage
    }
    if ourRating == 4 {
        cell?.star1.image = fullStarImage
        cell?.star2.image = fullStarImage
        cell?.star3.image = fullStarImage
        cell?.star4.image = fullStarImage
    }
    if ourRating == 4.5 {
        cell?.star1.image = fullStarImage
        cell?.star2.image = fullStarImage
        cell?.star3.image = fullStarImage
        cell?.star4.image = fullStarImage
        cell?.star5.image = halfStarImage
    }
    if ourRating == 5 {
        cell?.star1.image = fullStarImage
        cell?.star2.image = fullStarImage
        cell?.star3.image = fullStarImage
        cell?.star4.image = fullStarImage
        cell?.star5.image = fullStarImage
    }

}

This code WORKS! Nothing wrong with what it produces, I'm using wondering if there was a better way I could have written this? Maybe to reduce the amount of code?

Any advice is appreciated.

You could create a helper routine getStarImage:forRating: and use that to set up each star:

let fullStarImage:  UIImage = UIImage(named: "starFull.png")!
let halfStarImage:  UIImage = UIImage(named: "starHalf.png")!
let emptyStarImage: UIImage = UIImage(named: "starEmpty.png")!

func getStarImage(starNumber: Double, forRating rating: Double) -> UIImage {
    if rating >= starNumber {
        return fullStarImage
    } else if rating + 0.5 == starNumber {
        return halfStarImage
    } else {
        return emptyStarImage
    }
}

if let ourRating = object?["OurRating"] as? Double {
    cell?.star1.image = getStarImage(1, forRating: ourRating)
    cell?.star2.image = getStarImage(2, forRating: ourRating)
    cell?.star3.image = getStarImage(3, forRating: ourRating)
    cell?.star4.image = getStarImage(4, forRating: ourRating)
    cell?.star5.image = getStarImage(5, forRating: ourRating)
}

If the last part looks a bit redundant, you could do it in a loop.  This probably isn't worth it for 5 items, but it demonstrates what you can do in Swift:

for (index, imageView) in [cell?.star1, cell?.star2, cell?.star3, cell?.star4, cell?.star5].enumerate() {
    imageView?.image = getStarImage(index + 1, forRating: ourRating)
}


Role-based access control query I use SQL Server, and though not pertinent here, Railo's CFML engine.

I'm not great at SQL Joins, but I worked through this one and got the result set that I want. This particular SQL will only ever return 10 to 20 records, so even if it's a weak method, I'd probably never see a lot of grief from it since it's on an admin page to edit user profiles.

However, obviously it's best to learn early to improve, before habits set in. It's conceivable, and likely, that I'll encounter a similar situation in the future where the potential size of the result could be several times larger.

It's also possible that there's a faster way to retrieve the same results.

The goal of this page is to list out the user details, and a list of roles along the side with checkboxes. That's easy to code. I'm not asking for help with that.

This is the SQL that I came up with, and I'll explain my db layout below it.

select r.roleID, r.roleTitle, pu.*, 'constant' as qc from roles r
    left outer join (select u.*, p.roleID as xrole from permissions p
         left join users u on p.userID = u.userID
              and p.userID = 2) pu on pu.xrole = r.roleID
         and pu.userID = 2
order by len(username) desc

(Both occurrences of the number 2 are merely to test, this will naturally be replaced with a variable in my .cfm page)

I have three tables:

Users - typical users table (UserID, Username, other personal details
Permissions Table (UserID, RoleID)

Permissions, likely obvious, links the other two tables. If a User doesn't have an entry matching their userID and the role they're trying to access, can't access the role.

Roles (RoleID, RoleTitle)

Notes
- I'm not sure if I would naturally always get a row with userdata first, so I added an order by clause.
- 'constant' as qc is added so that I have one column that has a reliable value so that I can always group by that (cfoutput grouping, not sql grouping, I then cfoutput-group by roleID for the checkboxes for roles)

No, this s not the best way for several reasons:.

First, with this CTE test data common to all three runs:

with Users as ( select * from (values
        (1,'Pieter')
       ,(2,'Betty')
       ,(3,'Robert')
       ,(4,'Fred')
       ,(5,'George')
       ,(6,'Anne')
   )users(UserID,Username)
),
Roles as ( select * from (values
        (100,'Guest')
       ,(101,'Public')
       ,(  1,'Admin')
       ,(102,'Editor')
   )Roles(RoleID,RoleTitle)
),
Permissions as ( select * from (values
        (1,101)
       ,(2,101),(2,102)
       ,(3,101),(1,102)
       ,(4,101)
       ,(5,100)
       ,(6,1)
       ,(7,101),(1,102)
       ,(8,101)
   )Permissions(UserID,RoleID)
)

Your suggested SQL with results is:

select r.roleID, r.roleTitle, pu.* from roles r
    left outer join (select u.*, p.roleID as xrole from permissions p
         left join users u on p.userID = u.userID
              and p.userID = 2) pu on pu.xrole = r.roleID
         and pu.userID = 2

roleID      roleTitle UserID      Username xrole
----------- --------- ----------- -------- -----------
100         Guest     NULL        NULL     NULL
101         Public    2           Betty    101
1           Admin     NULL        NULL     NULL
102         Editor    2           Betty    102

Note that the same result is obtained if we tidy the SQL somewhat to this:

select
    r.roleID, r.roleTitle,
    u.UserID, u.UsrName
from permissions p
join users       u on u.UserID = p.UserID
                  and p.UserID = 2
right join roles r on p.RoleID = r.RoleID
;

roleID      roleTitle UserID      Username
----------- --------- ----------- --------
100         Guest     NULL        NULL
101         Public    2           Betty
1           Admin     NULL        NULL
102         Editor    2           Betty

Ordering your joins can make your code much easier to read, and often avoid nested subqueries.
Always list all field names (except possibly hen referencing a subquery which already lists them). This helsp to prevent your code from breaking with minor schema changes, such as additional fields or a re-ordering of the physical layout of the fields.


Extracting emails from a file and writing them to another file The code below works fine for me.  Is there any way that I can improve this code more? Note: delimiter is single whitespace only for email address.

getmail.py

#!/usr/bin/python

import re

handleone = open("scratchmail.txt", "r")
f1 = handleone.readlines()

handletwo = open("mailout.txt", "a+")

match_list = [ ]

for ArrayItem in f1:

    match_list = re.findall(r'[\w\.-]+@[\w\.-]+', ArrayItem)

    if(len(match_list)>0):

         handletwo.write(match_list[0]+"\r\n")

Input file scratchmail.txt:

tickets.cgi:5141|5141Z4063442957942364067088508|1219588377|1219588377||PND||abc@AABBCC.com|Mjpqafml|JgALKGXCuasMph|
tickets.cgi:358236|358236Z24989798132452492828304439|1433071308|1433071308||PND||xyz.abc@example.com|Edison|CpwxPARuQaqPR|
tickets.cgi:86805|86805Z25422507290694218605033173|1232345784|1232345784||PND||pytest@test.com|Agfaodki|jPdSNVlbXi|
…

Output file mailout.txt:

abc@AABBCC.com
xyz.abc@example.com
pytest@test.com

Working with file handles

You should always close the file handles that you open.
You didn't close any of them.
In addition,
you should use with open(...) as ... syntax when working with files,
so that they will be automatically closed when leaving the scope.

Checking if a list is empty

There is a much simpler way to check if a list is empty, instead of this:

if(len(match_list)>0):

The Pythonic way to write is simply this:

if match_list:

Unnecessary initializations and data storage

There's no need to initialize match_list before the loops.
You reassign it anyway inside.

You don't need to store all the lines from the first file into a list.
You can process the input line by line and write output line by line.
That can save a lot of memory, as you only need to keep one line in memory at a time,
not the entire input file.

Poor naming

The file handles are very poorly named.
handleone and handletwo don't convey that one is for input the other is for output.

ArrayItem doesn't follow the recommended naming convention of snake_case (see PEP8),
and it doesn't describe what it really is.

But f1 wins the trophy of worst name in the posted code.
lines would have been better.

Minor optimization

Instead of re-evaluating regular expressions in every loop,
I use re.compile to compile them first, to make all uses efficient.
However, this might not be necessary,
as it seems recent versions of Python have a caching mechanism.

Suggested implementation

With the above suggestions applied, the code becomes simpler and cleaner:

import re

re_pattern = re.compile(r'[\w\.-]+@[\w\.-]+')

with open("input.txt") as fh_in:
    with open("mailout.txt", "a+") as fh_out:
        for line in fh_in:
            match_list = re_pattern.findall(line)
            if match_list:
                fh_out.write(match_list[0]+"\r\n")


`sfinae` proposal... useful or unnecessary? I am trying to learn what SFINAE is. I believe I understand it quite well; however, I do not have a lot of practice in the subject. I mean I would find it difficult to list all the cases in which SFINAE should or could be used, or it would be difficult for me to immediately understand what a given sample of an SFINAE code should do. However, I believe I understand the core concept well enough to use it effectively.

Nonetheless looking through examples of SFINAE usage I came to the few conclusions.

One of which is: it is extremely pointless to use a chain of std::enable_if_t, std::void_t, std::is_same, etc, etc, etc, to define a single type using type = int *. IMHO (of a "newbie" with untrained eyes) such chains make the code less (extremely less) readable.

The second conclusion was: wouldn't it be useful to have the ability to check few conditions at the same time?

So I created this alias: (well "created" is waaay too big word... I just wrote 4 lines of code...)

template<typename T, typename ...>
using sfinae = T;

template<typename T, typename ...>
using SFINAE = T;

Usage examples

// 1) within class declaration:
template <typename T,
    sfinae<bool,
        std::enable_if_t<std::is_integral_v<T>>,
        std::enable_if_t<std::is_arithmetic_v<T>>
    > = true /* provide default option, bc reasons */>
class foo;

// 2) within function declaration:
template <typename T,
    sfinae<bool,
        std::enable_if_t<std::is_integral_v<T>>,
        std::enable_if_t<std::is_arithmetic_v<T>>
    > = true /* provide default option, bc reasons */>
void foo;

// 3) as a function return type:
template <typename T>
sfinae<T,
    std::enable_if_t<std::is_integral_v<T>>,
    std::enable_if_t<std::is_arithmetic_v<T>>
> const & foo(T const & val);

// 4) To define other SFINAE-conditions as aliases:
template <typename T>
using check_xyz = sfinae<void,
    std::enable_if_t<std::is_integral_v<T>>,
    std::enable_if_t<std::is_arithmetic_v<T>>
>;

template <typename T, typename = check_xyz<T>>
class foo;

And my main concern/question is: would a professional c++ developer deem sfinae</*...*/> (as I proposed it) useful or extremely unnecessary? Are there any changes that could be introduced to the idea?

I just wrote four lines of code...

template<typename T, typename ...>
using sfinae = T;

template<typename T, typename ...>
using SFINAE = T;

You should have written just two lines of code — either the first two or the last two. Not both. "There's more than one way to [spell] it" is the Perl way, not the C++ way. ;) (At least not intentionally! Not intentionally pre-C++2a! Now we're getting both std::filesystem and std::fs, that's a less airtight position.)

As you show, your thing can be used for

template<class T,
    sfinae<bool,
        std::enable_if_t<std::is_integral_v<T>>,
        std::enable_if_t<std::is_arithmetic_v<T>>
    > = true>
void foo();

However, this is pretty verbose; surely we'd rather write std::enable_if_t<std::is_integral_v<T> && std::is_arithmetic_v<T>> than std::enable_if_t<std::is_integral_v<T>>, std::enable_if_t<std::is_arithmetic_v<T>>.

So personally I have a different alias that accomplishes the same goal: bool_if_t.

template<bool B>
using bool_if_t = std::enable_if_t<B, bool>;

template<class T,
    bool_if_t<
        std::is_integral_v<T> &&
        std::is_arithmetic_v<T>
    > = true>
void foo();

This has the advantage that the inner expression is written using normal C++ syntax rules. We don't have to remember some arbitrary rule like "x, y means both x and y must be true"; we just write x && y. And if we want to enable this function when either x or y is true, we don't have to invent a new primitive; we just write bool_if_t<x || y>. bool_if_t is much more composable than your sfinae<Ts...>, because bool_if_t can exploit the existing expression syntax of C++.

Recommended viewing: "A Soupçon of SFINAE" (Arthur O'Dwyer, CppCon 2017). (Yes, that's me.)

You picked examples that don't really show off the power of sfinae, by the way.
Consider this example:

// 3) as a function return type:
template <typename T>
sfinae<T,
    std::enable_if_t<std::is_integral_v<T>>,
    std::enable_if_t<std::is_arithmetic_v<T>>
> const & foo(T const & val);

You take a "value-space" boolean (is_integral_v<T>), lift it up into "SFINAE-space" with enable_if_t, and then apply a logical AND operation in SFINAE-space using your sfinae alias.

template <typename T>
std::enable_if_t<
    std::is_integral_v<T> &&
    std::is_arithmetic_v<T>, T
> const & foo2(T const & val);

Here I take the same "value-space" boolean, perform the logical AND in value space where we have a dedicated operator && for maximum expressiveness; and I lift it up into SFINAE-space after doing the AND. This is clearer and also probably more efficient in terms of compile time.

Where your version helps is when your values start in SFINAE-space!

template<class T>
sfinae<T,
    decltype(std::declval<T>() + std::declval<T>()),
    T*,
    T&
> const & foo3(T const & val);

If we were trying to replicate this code's behavior with the standard tools, we'd say something like

template<class, class=void> struct has_plus : std::false_type {};
template<class T> struct has_plus<T, decltype(void(std::declval<T>() + std::declval<T>()))> : std::true_type {};
template<class T> inline constexpr bool has_plus_v = has_plus<T>::value;

template<class T>
std::enable_if_t<
    has_plus_v<T> &&
    not std::is_reference_v<T> &&
    not std::is_void_v<T>, T
> const & foo4(T const & val);

That is, we start with a value in SFINAE-space (decltype(std::declval<T>() + std::declval<T>())), lift it into value-space (has_plus_v<T>), do the logical AND in value-space, and lift the result back into SFINAE-space.

Whereas with your foo3, you start in SFINAE-space, do the logical AND via sfinae<...> without leaving SFINAE-space, and then you're done. Much simpler! But harder to read, I think.

A simple way to improve readability is to pick a good name. sfinae_space_and<Ts...> might be clearer. Can you think of a way to write sfinae_space_or<Ts...>? How about sfinae_space_not<T>? Does it make sense, maintainability-wise, to provide one without the others?


Ad page optimization? Any suggestions on how to shorten this would be greatly appreciated. This code takes three parameters through the URL and displays a page with targeted ads. I especially need help on how to cut down the three sections where I replace %20 with spaces. Also, here is the URL with sample parameters - http://timtechsoftware.com/ad.html?file_url=URL?file_name=FILE?keyword=KEY

This is the code of ad.html:

<!DOCTYPE html>

<title>Download File</title>

Your getParam code has a bug and cannot possibly work..

function getParam(paramName){                //Look for parameters
  var prmstr = window.location.search.substr(1);
  var prmarr = prmstr.split ("?"); //This has to be ampersand!!
  var params = {};

  for ( var i = 0; i < prmarr.length; i++) {
     var tmparr = prmarr[i].split("=");
     params[tmparr[0]] = tmparr[1];
  }
  return params[paramName];                      //Return the requested parameter
}

This function could be shorter if you skipped a few intermediary and some unnecessary steps:

There is no need to assign anything to an object, and hence to create that object
You only split out prmstr, no need to put that in a var
You could return early out of the for loop

That would give something more like

//Look for parameters
function getParam( s )
{
  var a = window.location.search.substr(1).split("&"), i = a.length, pair;
  while(i--)
  {
    pair = a[i].split("=");
    if( pair[0] == s )
      return pair[1];
  }
}

or, more Golfic

function getParam( s )
{
  return location.search.substr(1).split(s+"=")[1].split("&")[0]
}

Then, someone clearly copy pasted 3 times the same piece of code and made minor changes.
You should have 1 generic function, called 3 time instead. Or, realize that this code is a bad replacement of unescape and just call that. The reason the replacement is bad is that there are lot more characters out there than just %20.

var keyword = unescape( getParam( 'keyword' ) ),
    file_name = unescape( getParam( 'file_name' ) ),
    file_url = unescape( getParam( 'file_url' ) );

Furthermore, you assign google_ad_client = "ca-pub-1582371570610820"; and google_ad_slot = "9662168393"; twice.

Be aware , document.write, I have been told by reliable sources, should only be used when standing inside a protective summoning circle, lest greater demons devour you. Instead you should have the link tag in your HTML and then find the link in your js and fill in the anchor and caption.

Finally, this is not very good code, I hope the server side code that downloads the file is of higher quality and will not allow path manipulation or path traversal.


Three numbers calculator Edit: I made a new calculator after taking your input New Calculator

I made this three-function calculator. I have been learning C for a couple of months, and I learned C++ through the internet.
This is my first "real" program in C++, it started really messy but I did a lot of refining and debuging and deleting unnecessary code and this is the final result.
If you see any way to improve it or something I did wrong please tell me.

// Three numbers Calculator
#include "stdafx.h"
#include<iostream>
using namespace std;
int main()
{
    cout << "Enter action as # to exit program" << endl;
    cout << "Possible actions:+,-,*,/\n" << endl;
    while (1) //loop
    {
             long double num1, num2, num3, total;
         char action1, action2;
         cin >> num1 >> action1 >> num2 >> action2 >> num3;
         if (action1 == '/' && num2 == 0 || action2 == '/' && num3 == 0)
             cout << "You can't divide by zero" << endl;
         else if ((action2 == '*' || action2 == '/') && (action1 == '-' || action1 == '+'))  //action2 will be preformed before action1 (Order of operation)
         {
             switch (action2)  //I didn't include the options for '+' or '-' because the if statement requires action2 to be '*' or '/'.
             {
             case('/'):
                 total = num2 / num3;
                 break;
             case('*'):
                 total = num2*num3;
                 break;
             default:
                 cout << "Input not recognized";
                 break;
             }
             switch (action1)   //I didn't include the options for '*' or '/' because the if statement requires action1 to be '+' or '-'.
             {
             case('+'):
                 cout << num1 << "+" << num2 << action2 << num3 << "=" << total + num1;
                 break;
             case('-'):
                 cout << num1 << "-" << num2 << action2 << num3 << "=" << num1 - total;
                 break;
             default:
                 cout << "Input not recognized";
                 break;
             }
         }
         else //action1 will be performed before action2 (Order of operation)
         {
             switch (action1)
             {
             case('+'):
                 total = num1 + num2;
                 break;
             case('-'):
                 total = num1 - num2;
                 break;
             case('/'):
                 total = num1 / num2;
                 break;
             case('*'):
                 total = num1*num2;
                 break;
             case('#'):
                 system("PAUSE");
                 return 0;
                 break;
             default:
                 cout << "Input not recognized";
                 break;
             }
             switch (action2)
             {
             case('+'):
                 cout << num1 << action1 << num2 << "+" << num3 << "=" << total + num3;
                 break;
             case('-'):
                 cout << num1 << action1 << num2 << "-" << num3 << "=" << total - num3;
                 break;
             case('/'):
                  cout << num1 << action1 << num2 << "/" << num3 << "=" << total / num3;
                 break;
             case('*'):
                 cout << num1 << action1 << num2 << "*" << num3 << "=" << total*num3;
                 break;
             case('#'):
                 system("PAUSE");
                 return 0;
                 break;
             default:
                 cout << "Input not recognized";
                 break;
             }
         }
         cout << "\n\n";
     }
}

Update:

  Input: 7+8/2
  Output: 7+8/2=11

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

Fix your formatting

The code as posted has inconsistent indenting which makes it hard to read.  Choose a particular style and apply it consistently to make your code easier to read.

Don't abuse using namespace std

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

Validate the input

The code does not seem to be able to handle negative numbers as in the expression:

-3+2

If it's not intended to handle negative numbers, it would be good to let the user know.  If it is, then there's a bug that should be fixed.

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream.  This can be time-consuming in a program with a lot of I/O and is rarely actually needed.  It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one.  Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Don't use system("PAUSE")

There are two reasons not to use system("cls") or system("PAUSE").  The first is that it is not portable to other operating systems which you may or may not care about now.  The second is that it's a security hole, which you absolutely must care about.  Specifically, if some program is defined and named PAUSE or pause, your program will execute that program instead of what you intend, and that other program could be anything.  First, isolate these into a seperate functions pause() and then modify your code to call those functions instead of system.  Then rewrite the contents of those functions to do what you want using C++.  For example:

void pause() {
    getchar();
}

General portability

This code could be made portable if you omit the Windows-only include files #include "stdafx.h".  If you must have stdafx.h, consider wrapping it so that the code is portable:

#ifdef WINDOWS
#include "stdafx.h"
#endif

Separate input, output and calculation

To the degree practical it's usually good practice to separate input, output and calculation for programs like this.  By putting them in separate functions, it isolates the particular I/O for your platform (which is likely to be unique to that platform or operating system) from the logic of the program (which does not depend on the underlying OS).  So for example, the main program might call a function to fetch and validate the input, a second function to actually perform the mathematical operations and then a third to emit the results.

Omit spurious parentheses

The argument to a case statement or return statement does not need parentheses, so instead of this:

 case('#'):

One could write this:

 case '#':

Consolidate strings

Right now, the string "Input not recognized" appears four times in the program.  I'd recommend creating a const variable instead and then using that instead of repeating the string.