Bracket balance checker in Python Implement a feature for a text editor to find errors in the usage of brackets in the code.

Input: sorted(list(zip(a, b))

Output: False

Input: ([{}, {}])

Output: True

Here's my implementation in Python:

class Stack:
    def __init__(self):
        self.items = []

    def push(self, item):
        """Push item in stack."""
        self.items.append(item)

    def top(self):
        """Return top of the stack."""
        if self.items:
            return self.items[-1]
        raise Exception('Stack empty.')

    def pop(self):
        """Remove and return top of the stack."""
        if self.items:
            return self.items.pop()
        raise Exception('Stack empty.')

    def is_empty(self):
        """Return True if stack is empty."""
        return len(self.items) == 0

    def __len__(self):
        """Return stack length."""
        return len(self.items)

    def __str__(self):
        """Print stack."""
        print(list(reversed(self.items)))

def check_balance(code):
    """Return True if brackets are balanced, False otherwise."""
    valid = {']': '[', ')': '(', '}': '{'}
    lefts = Stack()
    for char in code:
        if char in '[({':
            lefts.push(char)
        if char in '])}' and lefts.is_empty():
            return False
        if char in '])}' and not lefts.is_empty():
            if lefts.top() != valid[char]:
                return False
            lefts.pop()
    if not lefts:
        return True
    return False

if __name__ == '__main__':
    print(check_balance('[(((((({{}}))))))]'))
    print(check_balance('print(list(reversed(self.items))'))
    print(check_balance(' print(list(reversed(self.items))[]'))
    print(check_balance('print(list(reversed(self.items)))'))
    print(check_balance('print(list[(]reversed(self.items))'))
    print(check_balance('print(list(reversed(self.items[0)]))'))
    print(check_balance('print(list(reversed(self.items[7])))'))

Looks pretty good.

A couple comments:

Unless this is an assignment to make or use a Stack, a Python list would suffice.

The code checks if a char is in '[{(', then checks if it is in ']})' and the stack is empty, then checks if it is in ']})' (again) and checks if the top of the stack is the matching bracket. All characters go through all those checks.

In general, it is good to make the common case fast and less cases can be slower.

def check_balance(code):
    """Return True if brackets are balanced, False otherwise.
    """

    valid = {']': '[', ')': '(', '}': '{'}

    lefts = []

    for char in code:
        # common case
        if char not in '[](){}':
            continue

        if char in '[({':
            lefts.append(char)

        elif not lefts or lefts.pop() != valid[char]:
            return False

    return lefts == []


Java CharSequence iteration A project I am working on requires me to check the first character of an input string, to determine if it is a numeric character. I have developed the following code:

public static boolean IsLeadingCharNumfName(String fName)
{
    CharSequence[] numbs;
    numbs  = new CharSequence[10];
    numbs[0] = "0";
    numbs[1] = "1";
    numbs[2] = "2";
    numbs[3] = "3";
    numbs[4] = "4";
    numbs[5] = "5";
    numbs[6] = "6";
    numbs[7] = "7";
    numbs[8] = "8";
    numbs[9] = "9";

    if(fName.substring(0,1).equals(numbs[0]))
    {
        return true;
    }
    if(fName.substring(0,1).equals(numbs[1]))
    {
        return true;
    }
    if(fName.substring(0,1).equals(numbs[2]))
    {
        return true;
    }
    if(fName.substring(0,1).equals(numbs[3]))
    {
        return true;
    }
    if(fName.substring(0,1).equals(numbs[4]))
    {
        return true;
    }
    if(fName.substring(0,1).equals(numbs[5]))
    {
        return true;
    }
    if(fName.substring(0,1).equals(numbs[6]))
    {
        return true;
    }
    if(fName.substring(0,1).equals(numbs[7]))
    {
        return true;
    }
    if(fName.substring(0,1).equals(numbs[8]))
    {
        return true;
    }
    if(fName.substring(0,1).equals(numbs[9]))
    {
        return true;
    }
    else
    {
        return false;
    }
}

I feel that this code can be optimised for efficiency, but I'm not sure how. I'll have to do the same for checking if the name contains a number.

What I am looking for, is a way to lower the code footprint primarily, with efficiency as a secondary bonus.

Use Java's built-in functions for things like this. Specifically, Character.isDigit(char c) and String.charAt(int index).

What you have right now is a poor reimplementation that makes any maintenance programmer go "wait, what?".

It sounds mean, but consider what it actually is:

return Character.isDigit(myString.charAt(0));

That one line is all you needed.

To prevent things like this happening in the future, I suggest googling your problem ("java string starts with digit" or "java string starts with number").

You'd have arrived at this question... https://stackoverflow.com/questions/1107798/how-to-check-a-string-starts-with-numeric-number .. which also points you to Character.isDigit. A little bit of google will go a long way in your programming work.

But school said I can't use those methods!

Oh. Okay.

First, store fName.substring(0,1) in a temporary variable. No need to substring forever.
Then, instead of Character.isDigit...

String allowedChars = "0123456789";
String firstChar = fName.substring(0,1);
return allowedChars.contains(firstChar);

After all, String IS CharSequence. You can see this in the documentation of java.lang.String.

public final class String
extends Object
implements Serializable, Comparable<String>, CharSequence

See, String implements CharSequence.

Bugs

One of the things you need to be aware of is that your function can get a null string or an empty string.

You should check if the string you get is null or empty:

if (fName == null || fName.length == 0){
    return false;
}


InputStream Wrapper that closes the stream once reading is done I faced some issues in a project with spring and hibernate where the stream given to Blob is not closed.

Blob blob = Hibernate.getLobCreator(currentSession).createBlob(inputStream, size);

I can't use things like try with resources since the stream is only read after the transaction commits. One of my friends suggested a hacky way to do this. Basically, his idea was to close the stream once the reading is complete.

import java.io.IOException;
import java.io.InputStream;

public class InputStreamWrapper extends InputStream {

    private InputStream inputStream;

    public InputStreamWrapper(InputStream inputStream) {
        this.inputStream = inputStream;
    }

    @Override
    public int read() throws IOException {
        int val = this.inputStream.read();
        if (val != -1) {
            return val;
        } else {
            this.inputStream.close();
            return -1;
        }
    }
}

I've done a simple test and it works. I understand that there can be a performance issue with the if check. However, I think there are other caveats in this as well. Can someone shed some light on those?

Blob blob = Hibernate.getLobCreator(currentSession).createBlob(new InputStreamWrapper(inputStream), size);

The general idea of a wrapper stream that closes the inner stream automatically once its end is reached is fine, but your implementation leaves somewhat to be desired.

First, if you're going to wrap a stream, then do a complete job.

At minimum, do not fail to override close() properly.  I understand that you're trying to solve a problem of close() not being called, but you don't know for sure how instances will be used once you hand them off to another component.  If the wrapper is closed before the end of the inner stream is reached, then you want the inner stream closed, too.
It would be a very good idea to delegate the other InputStream methods, too, with the possible exception of read(byte[]).  That will provide the full capabilities of the underlying stream to the wrapper's consumer, including, but not limited to, efficient block reads.  For this purpose, you should consider inheriting from FilterInputStream instead of directly from InputStream.

Second, you have to consider how to handle exceptions.  With a proper close() method on the wrapper, you're not limited to having read close the inner stream when an exception occurs.  Nevertheless, that might still be a good strategy.

Third, your wrapper doesn't necessarily provide a complete solution to the problem.  No matter what you do, a situation could arise in which the wrapper is abandoned before the inner stream is closed.  If you want to attend to that, too, then your best option is to provide a cleanup strategy based on soft, weak, or phantom references.  If you don't already know how to do this then now is a good time to learn -- it's a useful skill to have.

On the other hand, you should also consider avoiding the non-closure problem differently, by using a stream for which that's a non-issue. ByteArrayInputStream is such a stream.  Using that would mean reading the original data into memory yourself, so you would have control over closing that stream.  Failing to close a ByteArrayInputStream is no big deal.


Finding out the indices of common elements in two array lists having duplicate elements I have two lists which are also  having some duplicate elements , i am converting this list to map as key value pair with element index as the value for the key - element

List 1 : [1, 2, 3, 4, 5, 6, 7, 2] = >

MAP 1 (Map, List>) : {1=[0], 2=[1, 7], 3=[2], 4=[3], 5=[4], 6=[5], 7=[6]}

List 2 : [3, 3, 2, 4, 7]

= > MAP 2 (Map, List>) : {2=[2], 3=[0, 1], 4=[3], 7=[4]}

i am trying to collect matching keys from both of the maps and club it another map

For above two it would give the result : {[2]=[0, 1], [3]=[3], [6]=[4], [1, 7]=[2]}

Post this , i am taking the cartesian product of key , values and only retaining the unique elements

Final output [[0, 2], [1, 2], [3, 3], [4, 6], [2, 1], [2, 7]] . These is the indices of common elements

Below is the code that i have come up with to achieve this , is there any better way i can achieve the same . I feel it will be too much costly at run time for larger lists .

 List<List<Integer>> cartesianProduct = new ArrayList<>();
 List<String> distinct1 = Arrays.asList("1", "2", "3", "4", "5", "6", "7", "2")
 List<String> distinct2 = Arrays.asList["3", "3", "2", "4", "7"]

 Map<String, List<Integer>> result1 = IntStream.range(0, distinct1.size()).boxed()
                     .collect(Collectors.groupingBy(i -> distinct1.get(i)));

 Map<String, List<Integer>> result2 = IntStream.range(0, distinct2.size()).boxed()
                    .collect(Collectors.groupingBy(i -> distinct2.get(i)));

 result1.entrySet().stream()
                    .filter(x -> result2.containsKey(x.getKey()))
                    .collect(Collectors.toMap(x -> x.getValue(), x -> result2.get(x.getKey())))
                    .entrySet().forEach(a1 -> a1.getValue().stream()
                    .forEach(b1 -> a1.getKey().stream().forEach(c1 -> cartesianProduct.add(Arrays.asList(b1, c1)))));

   List<List<Integer>> listWithoutDuplicates = cartesianProduct.stream()
           .distinct()
           .collect(Collectors.toList());

Firstly, try to make sure that code you post here compiles. Now you have missing semicolons on the lines

List<String> distinct1 = Arrays.asList("1", "2", "3", "4", "5", "6", "7", "2")
List<String> distinct2 = Arrays.asList["3", "3", "2", "4", "7"]

and also the last of these two lines has [...] instead of (...).

It was also a bit difficult to actually figure out what your code was supposed to do, but I think I understand it now.

It seems you want to create a list of pairs of some sort. So instead of ending with a List<List<Integer>>, you can create a Pair class

public final class Pair<S, T> {
  private final S left;
  private final T right;

  public Pair(S left, T right) {
    this.left = left;
    this.right = right;
  }

  public S left() {
    return left;
  }

  public T right() {
    return right;
  }

  // equals and hashCode
  ...
}

and have a List<Pair<Integer, Integer>> instead.

Then you can create a method which given two Lists creates the cartesian product

public List<Pair<S, T>> cartesianProduct(List<S> firstList, List<T> secondList) {
  return firstList.stream()
    .flatMap(x -> secondList.stream().map(y -> new Pair<>(x, y)))
    .collect(Collectors.toList());
}

During the construction of your cartesian list, you collect your intermediate results into a Map. This step is not needed as you can now immediately convert to a list of pairs. Also, it is usually not a good idea to have a List as keys of a Map as the List is mutable. Map expects the hash code of the keys to not change which is a possibility when using Lists as keys.

Removing the Map step in the process we get

List<Pair<Integer, Integer>> listWithoutDuplicates = result1.entrySet().stream()
        .filter(x -> result2.containsKey(x.getKey()))
        .flatMap(x -> cartesianProducts(result2.get(x.getKey()), x.getValue()).stream())
        .distinct()
        .collect(Collectors.toList());


Close Scanner if no input I'm in very early stages of learning some Java coding. I'm asking for this review because, although the code below works, I'd like to know if it's an ugly way of achieving it.
What I want the code to do is take input from Scanner and reprint it, except if the input is empty, ie, just hit the return key, in which case the Scanner should close and the program end.

import java.util.Scanner;

public class Morse {

static boolean truefalse = true;

public static void main(String[] args) {
    // TODO Auto-generated method stub
Scanner input = new Scanner(System.in);
while (truefalse)
{
String line = input.nextLine();
if (line.equals(""))
truefalse = false;
else
System.out.println(line);
}
input.close();
}

}

There are a number of ways your code can be improved:

Use nice indentation using 4 spaces. This improves the readability a lot. See the official guidelines on indentation here.

Basically every time you open a new curly brace you should start a new indentation depth. (See code sample below)
Use braces around all kinds of blocks, e.g. here even for one-lined if/else blocks. Also, for Java it is the official convention to place the braces like this:

if (line.equals("")) {
    truefalse = false;
} else {
    System.out.println(line);
}

You can use a trick to assign the value of the scanner input to a variable inside the while condition and then use the String's isEmpty() function to check if the input was empty. This way you can spare the boolean variable indicating whether something was read:

while (!(line = input.nextLine()).isEmpty())

Use try() to create instances of Types that implement java.lang.AutoCloseable to make sure they are automatically closed when the block ends (Note this only works with Java 7 or newer):

try (Scanner input = new Scanner(System.in))

See here for reference.

Putting that all together:

public class Morse {

    public static void main(String[] args) {
        try (Scanner input = new Scanner(System.in)) {
            String line;
            while (!(line = input.nextLine()).isEmpty()) {
                System.out.println(line);
            }
        }
    }
}


When to initialize infrequently used base class objects I have a business object base class which is inherited by my business objects:

public class ErrorBase
{
    public bool HasError { get; set; }
    public List<Error> ErrorList { get; set; }

    public ErrorBase()
    {
        HasError = false;
        ErrorList = new List<Error>();
    }
}

Can this be optimized? My concern is that List<Error> is initialized every time a business object is instantiated, regardless of whether an Error object is ever added.

The only alternative I can think of is initializing ErrorList = null, then checking ErrorList != null before calling ErrorList.Add(), but I'm not sure that performing that check every time before adding an Error is actually an optimization. It certainly adds steps to the process of adding an Error object to the List<>.

Is there another way I'm not thinking of? If not, is one of these approaches considered a best practice?

First: I doubt that creating a new List<> object creates enough overhead which has measurable impact on your system so unless you can prove that it's a problem don't worry about it.

That being said: The classic solution to that problem is lazy instantiation. As you have a property already this can be easily achieved by:

public class ErrorBase
{
    public bool HasError { get; set; }

    private List<Error> _ErrorList = null;
    public List<Error> ErrorList
    {
        get
        {
            if (_ErrorList == null)
            {
                _ErrorList = new List<Error>();
            }
            return _ErrorList;
        }
    }

    public ErrorBase()
    {
        HasError = false;
    }
}

If you have multiple threads adding to it then you need to add some locking in the get - but then on the other hand List<> itself is not thread-safe so I assume that's not the case.

In .NET 4.0 and later you have Lazy<> which will make the code a little bit shorter.


Tic-Tac-Toe game in JavaScript/HTML/CSS I need help with a question my teacher wants us to do by tomorrow. I would love for you to try to help me out with this.

The following functions must be implemented:

A function that initializes all variables
Variables that keep track of which player is currently playing, and which player is next
Variables that keep count of X's and O's played

var currentPlayer = prompt("Who is The Current Player X=1 O=2");

var grid=new Array(3);
grid[0]=new Array(3);
grid[1]=new Array(3);
grid[2]=new Array(3);

for (var i=0; i<=2; i++) {
  for (var j=0; j<=2; j++) {
    grid[i][j]=0;
  }
}

// Checks If Grid Is Already Clicked
function clickCell(x,y) {
  if (grid[x][y]>0) {
    alert("Dont Try To Cheat Bud!!!!!");
  }

// Clicking Of Boxes
  else {
    if (currentPlayer==1) {
      document.getElementById("cell_"+x+"_"+y).style.color="#3F88C5";
      document.getElementById("cell_"+x+"_"+y).innerHTML="X";
      grid[x][y]=1;
      currentPlayer=2;
    } else {
       document.getElementById("cell_"+x+"_"+y).style.color="#E2C290";
       document.getElementById("cell_"+x+"_"+y).innerHTML="O";
      grid[x][y]=2;
      currentPlayer=1;
    }
  }
}

// Reset Game

function reset() {
for (var i=0; i<=2; i++) {
  for (var j=0; j<=2; j++) {
    grid[i][j]=0;
    document.getElementById("cell_"+i+"_"+j).innerHTML="&nbsp;";
  }
}
 currentPlayer=1;
}
h1 {
    text-align: center;
    font-family: 'Indie Flower', cursive;
  font-size: 53px;
}
div {
  text-align: center;
  margin: 0px auto;
    text-align: center;
  margin: auto;
}
body {
    background-color: #02c39a;
}

.cell {
  width:80px;
  height:80px;
  border: 5px solid #000000;
  background: #FFFFFF;
  font-size: 46pt;
  font-family: arial;
  font-weight: bold;
  text-align: center;
  margin: auto;

}

fieldset {
  text-align: center;
  margin: auto;
}

.cell:hover {
  background: #C2DBFC;
  text-align: center;
}

    <meta charset="utf-8" />
    <title>Tic-Tac-Toe</title>
    <link rel="stylesheet" type="text/css" href="tictoe.css">
    <link href='https://fonts.googleapis.com/css?family=Indie+Flower' rel='stylesheet' type='text/css'>

Tic Tac Toe

<fieldset>

Please Reload The Page To Choose Player Who Goes First Again

    &nbsp;
    &nbsp;
    &nbsp;

    &nbsp;
    &nbsp;
    &nbsp;

    &nbsp;
    &nbsp;
    &nbsp;

<input type=button onClick="Javascript: reset();" value="Reset">

You should not use "magic numbers". Define a constant representing the size of the grid:

var GRID_SIZE = 3;

Generally new Array() can be shorted to [] (an empty literal array). Also it's not really necessary to give a length when initializing arrays, because JavaScript has dynamic arrays. Thus

var grid=new Array(3);
grid[0]=new Array(3);
grid[1]=new Array(3);
grid[2]=new Array(3);

can be shorted to

var grid = [];
grid[0] = [];
grid[1] = [];
grid[2] = [];

or by using a literal array even to

var grid = [ [], [], [] ];

This however skips the use if the GRID_SIZE constant. So you could instead integrate this with the loops filling the grid:

var grid = [];
for (var i = 0; i < GRID_SIZE; i++) {
  grid[i] = [];
  for (var j = 0; j < GRID_SIZE; j++) {
    grid[i][j] = 0;
  }
}

Your HTML table is unnecessary complex.

You don't need to encode the coordinates of the cells in the ID. You can get the coordinates (or find a cell based on coordinates) using the DOM (more about that later).

You don't need to give every single the same class. Instead give the table a class or identifier (e.g. tic-tac-toe) and style the cells using a descendant selector:

.tic-tac-toe td {
   /* ... */
}

Event handlers shouldn't be assigned using HTML attributes. And if you do, then you don't need to use javascript: in them. javascript: is only used in attributes that normally don't contain JavaScript, such as href, however you shouldn't be using that either. Event handlers should only be assigned using addEventListener.

Finally the non-breaking spaces are also unnecessary. To make sure that the empty cells are displayed use the CSS empty-cells: show.

This leaves us with:

When assigning the event handler it is not not necessary to assign each cell it's own handler. Instead assign the handler to a common parent element - in this case the table. The event object will contain a reference to the actually clicked element:

var table = document.getElementById("tic-tac-toe-1");
table.addEventListener("click", tableClick);

function tableClick(event) {
    var element = event.target;
    if (element && element.tagName === "TD") {
       var y = element.cellIndex;
       var x = element.parent.rowIndex;

       clickCell(element, x, y);
    }
}

Since you already have a reference to the clicked cell, you don't need to look for it in clickCell.

function clickCell(cell, x, y) {
  if (grid[x][y] > 0) {
    alert("Dont Try To Cheat Bud!!!!!");
  } else {
    if (currentPlayer == 1) {
      cell.className = "x";
      cell.innerHTML = "X";
      grid[x][y]=1;
      currentPlayer=2;
    } else {
      cell.className = "o";
      cell.innerHTML = "O";
      grid[x][y]=2;
      currentPlayer=1;
    }
  }
}

Also you shouldn't assign the colors to the elements. Instead define the colors in the CSS with classes.

.x { color: #3F88C5; }
.o { color: #E2C290; }

If you ever need to get an element from the coordinates, then you can use the DOM properties rows and cells:

var cell = table.rows[y].cells[x];

Some quick last things:

Identifying the current player using the numbers 1 and 2 is not good (magic numbers again). Find an alternative. Also: What happens, if the user doesn't enter 1 or 2 at the beginning?
The reset function basically has the same loop as the initialization. You should re-write both so that you can re-use the loop instead of having it twice in your code.
The grid variable isn't necessary. All information in it can be get directly from the HTML table.


Quicksort implementation I put together this implementation after playing around with various partitioning- and pivot-strategies, and it runs fine. I wrote this off a version which seemed a bit different from the majority of implementations, so I'm curious on what you guys think. It works and handles duplicates.

def quicksort(a, min=0, max=a.length-1)
  if min < max
    index = partition(a, min, max)
    quicksort(a, min, index-1)
    quicksort(a, index+1, max)
  end
end

def partition(a, l, r)

  m=(l+r)/2
  piv = a[m]
  a[m], a[r] = a[r], a[m]

  cur = l
  for i in l..r-1

    if a[i] <= piv
      a[i], a[cur] = a[cur], a[i]
      cur+=1
    end
  end
  a[cur], a[r] = a[r], a[cur]

  return cur

end

You code looks nice, my main problem with it is naming:

Don't use single letters as member names, unless they are trivial. I couldn't figure out what l and r stand for until I read your code a couple of times (length? range?).

Even more so, don't ever use the letter l as a variable name - it is much too similar to 1, and is notoriously known to obfuscate code.

Don't re-use too familiar names for different purposes - partition is a well known method in ruby enumerables. Using this name for your code, especially when it involves arrays is very confusing. Don't do that. Call it reorder_partition or pivot_partition.

Expressiveness goes a long way - a[l], a[r] = a[r], a[l] works perfectly, but is hard to read, a much more readable way would be if you had a helper method swap(a, l, r). Even better might look to implement it into Array a.swap!(l, r)

Use the power of ruby - instead of l..r-1, use non-inclusive range operator - l...r

So, my version may look a little more verbose, but I believe it is much more readable. It did teach me a lot about quicksort :-)

def quicksort(array, min=0, max=array.length-1)
  if min < max
    index = reorder_partition(array, min, max)
    quicksort(array, min, index-1)
    quicksort(array, index+1, max)
  end
end

def reorder_partition(array, left_index, right_index)
  middle_index = (left_index + right_index)/2
  pivot_value = array[middle_index]

  array.swap!(middle_index, right_index)

  less_array_pointer = left_index

  (left_index...right_index).each do |greater_array_pointer|
    if array[greater_array_pointer] <= pivot_value
      array.swap!(greater_array_pointer, less_array_pointer)
      less_array_pointer+=1
    end
  end

  array.swap!(less_array_pointer, right_index)

  return less_array_pointer
end

class Array
  def swap!(a,b)
    self[a], self[b] = self[b], self[a]
    self
  end
end


Git commit-msg URL shortener I have just written my first git hook script.  It is very simple that simply finds any URLs in the commit message and uses the Google URL shortener to rewrite the URL nicely.

it is located here.

I feel it could be improved immensely (as it is my first) and would love to have your input.

#! /bin/bash
message=`cat $1`
shorten=`sed -ne 's/.*\(http[^"]*\).*/\1/p' $1`
echo "Shortening Url $shorten ...."
new_url=`curl -s https://www.googleapis.com/urlshortener/v1/url \-H 'Content-Type: application/json' \-d "{'longUrl': '$shorten'}"`
latest=`echo $new_url | python -c 'import json,sys;obj=json.load(sys.stdin);print obj["id"]';`
final=${message/$shorten/$latest}
echo $final > $1

First of all, the `cmd` form of running commands and capturing their output is deprecated. Use the recommended, modern way everywhere: $(cmd)

Safety

Be careful with spaces in filenames. These commands will break:

message=`cat $1`
shorten=`sed -ne 's/.*\(http[^"]*\).*/\1/p' $1`

It will be safer like this:

message=$(cat "$1")
shorten=$(sed -ne 's/.*\(http[^"]*\).*/\1/p' "$1")

The pattern there in the sed is not very safe. The text "add support for http protocol" will match. I think you want to be more strict, maybe something like:

longUrl=$(sed -ne 's/.*\(https\{0,1\}:\/\/[^"]*\).*/\1/p' "$1")

Note: if you have GNU sed (you're in Linux, or have gsed), then instead of the tedious https\{0,1\} you can simplify as https\?, though it's less portable.

And what if there are multiple URLs in the script? It will fail. You probably want to loop over the results of the sed. Or take a lazier approach and just ensure that sed will always produce at most one line:

longUrl=$(sed -ne 's/.*\(https\{0,1\}:\/\/[^"]*\).*/\1/p' "$1" | head -n 1)

What if there are no matches? An if would be good, as in that case you won't need the curl call, and it's better to not overwrite a file if you don't really have to.

When you echo $somevar, whitespaces inside like tabs and newlines would be replaced by spaces. To prevent that, quote the variable:

echo "$message" > "$1"

Naming

Your variable names are not so good:

shorten is the output of sed, a long url. Something like longUrl would have been better.
newUrl is the output of curl, a json text. Something like json would have been better.

Unnecessary things

In the curl command, you don't need the backslashes in \-H and \-d flags.

And instead of saving the output of the curl and then echo-ing to python, it would be better to skip that intermediary variable and just pipe it directly, like this:

shortUrl=$(curl -s https://www.googleapis.com/urlshortener/v1/url -H 'Content-Type: application/json' -d "{'longUrl': '$longUrl'}" | python -c 'import json, sys; print(json.load(sys.stdin)["id"])')

Also notice in that python that I skipped the intermediary obj variable and just used the ["id"] directly on json.load(...).

Suggested implementation

#! /bin/bash
message=$(cat "$1")
longUrl=$(sed -ne 's/.*\(https\{0,1\}:\/\/[^"]*\).*/\1/p' "$1" | head -n 1)
if test "$longUrl"; then
    echo "Shortening Url $longUrl ..."
    shortUrl=$(curl -s https://www.googleapis.com/urlshortener/v1/url -H 'Content-Type: application/json' -d "{'longUrl': '$longUrl'}" | python -c 'import json, sys; print(json.load(sys.stdin)["id"])')
    message=${message/$longUrl/$shortUrl}
    echo "$message" > "$1"
fi

This is still not perfect, because it doesn't handle multiple urls. I might add that later, gotta go now.

Online shell checker

This site is pretty awesome: http://www.shellcheck.net/#

Copy-paste your script in there, and it can spot many mistakes that are easy fix.


Simple dynamic tree in Python I tried to make a dynamic tree in python. It is simple and i have gotten it to work so far. I created a function in main that generates some example data for the tree and then the main goal was to create a member function that uses recursion to print all the data (display_data()). The only issue with this that i have is the recursion depth problem, as well as the speed. Essentially the recursion and loop pattern builds up quite a bit on the overall run time. Also note this is a side project, I need to understand dynamic trees for a tic-tac-toe AI I am attempting(failing) to write.

tree.py

class Tree():
    """Implement a dynamic tree

                   1
                 / | \
                /  |   \
              2    3     4
             / \       / | \
            5   6     7  8  9
           /  / | \
          10 11 12 13
    """

    def __init__(self):
        self.children = []
        self.data = []

    def create_children(self, amount):
        for i in range(amount):
            self.children.append(Tree())

    def create_data(self, data):
        for datum in data:
            self.data.append(datum)

    def display_data(self):
        print(self.data, end = ' ')
        for child in self.children:
            child.display_data()

main.py

from tree import Tree

def example_data(root):
    """
        ['Child', 1] ['nChild', 1] ['nChild', 2]
        ['Child', 2] ['nChild', 1] ['nChild', 2]
        ['Child', 3] ['nChild', 1] ['nChild', 2]

    """
    root.create_data(["Root"])
    root.create_children(3)

    counter = 1
    for child in root.children:
        child.create_data(["Child", counter])
        child.create_children(2)
        counter += 1

        ncounter = 1
        for nchild in child.children:
            nchild.create_data(["nChild", ncounter])
            ncounter += 1

    return root

if __name__ == "__main__":
    root = example_data(Tree())
    root.display_data()

Specific suggestions:

It is idiomatic to wrap the stuff after if __name__ == "__main__": in a main function.
Rather than the generic data I would suggest figuring out exactly which information you want to attach to each Tree and creating fields for each of them rather than a fully generic list of stuff. This will make it much less painful to work with actual Trees because you can use for example tree.name or tree.counter instead of tree.data[0] and tree.data[1].
You can enumerate a list to loop over it without maintaining a separate index variable, as in for index, child in enumerate(root.children):

In general it'll be much easier to see how to improve this code once it's wired into a production use case rather than example code. The problem with writing code to an example "spec" is that the example inevitably doesn't completely fit the production use case - some crucial features will be missing and others will be superfluous. For example, storing the count of children separately. This information is already encoded in the length of the children list, so you are duplicating the information for no obvious reason. This could conceivably be useful if you're dealing with giant amounts of data, but if your application is sufficiently optimized that this is a real concern you probably should look into other languages or frameworks like numpy or pandas.

General suggestions:

black can automatically format your code to be more idiomatic.
isort can group and sort your imports automatically.
flake8 with a strict complexity limit will give you more hints to write idiomatic Python:

[flake8]
max-complexity = 4
ignore = W503,E203

(The max complexity limit is not absolute by any means, but it's worth thinking hard whether you can keep it low whenever validation fails. For example, I'm working with a team on an application since a year now, and our complexity limit is up to 7 in only one place. Conversely, on an ugly old piece of code I wrote without static analysis support I recently found the complexity reaches 87!)
I would then recommend adding type hints everywhere and validating them using a strict mypy configuration:

[mypy]
check_untyped_defs = true
disallow_untyped_defs = true
ignore_missing_imports = true
no_implicit_optional = true
warn_redundant_casts = true
warn_return_any = true
warn_unused_ignores = true


Extracting first non-null property inside list using Java 8's Optional I am writing a Java method to extract a String property, called number, from a Person object.

A Person object has several properties that can be null and number belongs to house, which belongs to addresses, which belongs to metadata which finally belongs to our person.

As addresses is a list of Address objects, I want to extract the first non-null number. That is what I have so far:

public class HouseNumberResolver {
    public static String numberFrom (Person person) {
        return Optional.ofNullable(person)
                .map(Person::getMetadata)
                .map(Metadata::getAddresses)
                .map(HouseNumberResolver::getFirstHouseNumber)
                .orElse("");
    }

    private static String getFirstHouseNumber(List<Address> addresses) {
        for (Address address : addresses) {
            String number = Optional.ofNullable(address)
                    .map(Address::getHouse)
                    .map(House::getNumber)
                    .orElse(null);

            if (number != null) {
                return number;
            }
        }

        return null;
    }
}

However, I feel that this is quite ugly. Ideally, I wanted to have something like:

    private static String getFirstHouseNumber(List<Address> addresses) {
        for (Address address : addresses) {
            Optional.ofNullable(address)
                .map(Address::getHouse)
                .map(House::getNumber)
                .ifPresent(number -> return number)
                .orElse(null)
        }
    }

That would return the number there, but it seems like this is not possible.

Any recommendations are welcome.

The problem here is that your Optional would be required to either return your method or to continue with the next loop iteration. This cannot be expressed that way in Java.

One solution is to convert from external iteration (using the for loop) to internal iteration using a Stream. You can find a lot of documentation for working with streams (start with the Javadoc for Stream and the java.util.stream package), so I won't go into low level details here.

addresses.stream()
    .map(Address::getHouse)
    .map(House::getNumber)
    .filter(number -> number != null)
    .findFirst();

This executes the mappings, drops all numbers that are null and returns the first non-null number. It solves the return-vs-continue problem, but it won't work as expected because Stream.map(…) will happily put a null value in the resulting stream. You would get a NullPointerException at map(House::getNumber) if some Address has no House.

You can use your original Optional chain as mapping function and your method would look like this:

private static String getFirstHouseNumber(List<Address> addresses)
{
    return addresses.stream()
            .map(address -> Optional.ofNullable(address)
                    .map(Address::getHouse)
                    .map(House::getNumber))
            .filter(Optional::isPresent)
            .map(Optional::get)
            .findFirst()
            .orElse(null);
}

This looks more concise than your first method but it is not really that obvious what is going on. You can probably guess it but to say for sure you will probably need some moments.

Maybe one can come up with something a bit more readable but I'm afraid that given the current setup it won't get much better.

To get much more benefit (not just for this case but for the whole application) you may think about refactoring.

Note that the following may or may not be useful. As I don't know the rest of your application this are just some observations and hints from the perspective of this code snippet.

I see two main problems here:

Extensive use of null
Violation of law of Demeter

Is it really the case that all of your properties may be null here?

I guess each Person should have some Metadata (even if sparsely filled). You can always get a list of known addresses for a person. This list may be empty but it should not contain null values. An Address should always have a House on it if some Person lives there. So the only thing that may be null is the house number (e.g. if you know the person lives in city X but you don't know the exact street and house number).

Does your code at this point really have to know how your person is structured internally?

From the perspective of the code above I would expect to get a list of addresses directly from the person. I don't care about the metadata. Similarly I don't care about the house.

Maybe you check the house number for null just to verify if the address is complete? Maybe what you are actually looking for is the first valid and complete address of some person?

If all this assumptions apply, your code could look like this:

public Optional<Address> getValidAddress(Person person) {
    return person.getAddresses().stream()
            .filter(Address::isComplete)
            .findFirst();
}


Python right position for a Error message in a class first of all sorry for this easy question. I have a class

class Grid(object):
    __slots__= ("xMin","yMax","nx","ny","xDist","yDist","ncell","IDtiles")
    def __init__(self,xMin,yMax,nx,ny,xDist,yDist):
        self.xMin = xMin
        self.yMax = yMax
        self.nx = nx
        self.ny = ny
        self.xDist = xDist
        self.yDist= yDist
        self.ncell = nx*ny
        self.IDtiles = [j for j in np.ndindex(ny, nx)]
        if not isinstance(nx, int) or not isinstance(ny, int):
            raise TypeError("an integer is required for nx and ny")

where

xMin = minimum x coordinate (left border)
yMax = maximum y coordinate (top border)
nx = number of columns
ny = number of rows
xDist and yDist = resolution (e.g., 1 by 1)

nx and ny need to be an integer. I wish to ask where is the most elegant position for  the TypeError. Before all self. or in the end?

class Grid(object):
    __slots__= ("xMin","yMax","nx","ny","xDist","yDist","ncell","IDtiles")
    def __init__(self,xMin,yMax,nx,ny,xDist,yDist):

Python convention says that argument should be lowercase_with_underscores

        self.xMin = xMin
        self.yMax = yMax

The same convention holds for your object attributes

        self.nx = nx
        self.ny = ny
        self.xDist = xDist
        self.yDist= yDist
        self.ncell = nx*ny
        self.IDtiles = [j for j in np.ndindex(ny, nx)]

This is the same as self.IDtiles = list(np.ndindex(ny,nx))

        if not isinstance(nx, int) or not isinstance(ny, int):
            raise TypeError("an integer is required for nx and ny")

This would be more conventialy put at the top. But in python we prefer duck typing, and don't check types. You shouldn't really be checking the types are correct. Just trust that the user of the class will pass the right type or something close enough.


Converting partial XML (Node List) to a String I have a need to extract a part of XML tree (everything under root) and convert it to a string.  (The result string will be later pasted inside XSL-FO XSLT file.)

The above functionality is a part of Java 6 application.  I use org.w3c.dom and javax.xml.transform. Still in the debugging process.

// ...

// Extract all child notes of root from foDoc
// and convert the result to String
Node root = foDoc.getElementsByTagNameNS("*","root").item(0);
NodeList body = root.getChildNodes();
String foResult;

try {
    foResult = nodeListToString(body);
} catch (TransformerException e) {
    logger.error(e);

    foResult = "";
}
// ...

private static String nodeListToString(NodeList nodes) throws TransformerException {
    StringBuilder result = new StringBuilder();
    int len = nodes.getLength();
    for(int i = 0; i < len; ++ i) {
        result.append(nodeToString(nodes.item(i)));
    }
    return result.toString();
}

private static String nodeToString(Node node) throws TransformerException {
    StringWriter buf = new StringWriter();
    Transformer xform = TransformerFactory.newInstance().newTransformer();
    xform.setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "yes");
    xform.transform(new DOMSource(node), new StreamResult(buf));
    return(buf.toString());
}

My questions:

Assuming that there will always be exactly one root node, how reliable is my code in extracting everything under it?
Am I missing any edge cases?
Anything just plain wrong?

Please disregard any exception handling inadequacies.

Edited on 10/06/2015

Since my originally posted code had a "just plain wrong" part, I've fixed it. There were no answers based on the old code.

Reading the documentation for
Transformer,
where it says:

  A Transformer may be used multiple times. Parameters and output
  properties are preserved across transformations.

, I'd say that not only can the Transformer object be reused across
multiple nodes, but you should also be able to use a single
StringWriter instead of recreating it for every node; same goes for
the DOMSource actually:

private static String nodeListToString(NodeList nodes) throws TransformerException {
    DOMSource source = new DOMSource();
    StringWriter writer = new StringWriter();
    StreamResult result = new StreamResult(writer);
    Transformer transformer = TransformerFactory.newInstance().newTransformer();
    transformer.setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "yes");

    for (int i = 0; i < nodes.getLength(); ++i) {
        source.setNode(nodes.item(i));
        transformer.transform(source, result);
    }

    return writer.toString();
}

That said, I'm only guessing that this should work.

Also, the len variable is probably going to be inlined automatically
anyway, though you could check that with a disassembler just to be sure
(it wouldn't make that much of a difference I suspect).

Otherwise looks good to me; if you've tested it with all kinds of node types and the output looks good, then I'd say that should work IMHO.


Console based password generator I am currently developing a password generator and just finished my first version. I know there is a lot to do but before I proceed with the next version I would like to have feedback on my current source code.

GitHub

The embedded source code does not contain comments to reduce characters.

Main.cpp

void main()
{
    Controller::PwGenerator* oController = new Controller::PwGenerator();
    oController->Run();
    delete oController;
}

PwGenerator.h

#pragma once
namespace View
{
    class Terminal;
}
namespace Utility
{
    class Security;
}
namespace Controller
{
    class PwGenerator
    {
        private:
            View::Terminal* oTerminal;
            Utility::Security* oSecurity;
        public:
            PwGenerator();
            void Run();
            ~PwGenerator();
    };
}

PwGenerator.cpp

#include "PwGenerator.h"
#include "Terminal.h"
#include "Security.h"
#include <fstream>
#include <ctime>
#ifdef WIN32
#include <windows.h>
#else
#include <unistd.h>
#endif
Controller::PwGenerator::PwGenerator()
{
    this->oTerminal = new View::Terminal();
    this->oSecurity = new Utility::Security();
}
void Controller::PwGenerator::Run()
{
    bool bFinish = false;
    int iMenuItem = 1;
    std::ofstream oFstream;
    std::time_t oTime = time(0);
    std::tm* timePtr = std::localtime(&oTime);
    this->oTerminal->Programminfo();
    #ifdef WIN32
    Sleep(3000);
    #else
    usleep(3000 * 1000);
    #endif
    do
    {
        this->oTerminal->Menu();
        iMenuItem = this->oTerminal->SelectMenuOption();
        switch (iMenuItem)
        {
            case 0:
                bFinish = true;
            break;
            case 1:
                this->oTerminal->Programminfo();
            break;
            case 2:
                this->oSecurity->GeneratePw( this->oTerminal->MenuePwGen() );
                if (this->oTerminal->MenueSavePw() == true)
                {
                    this->oTerminal->Print("The password has been saved in a file.");
                    oFstream.open("PasswordGenerator.txt", std::fstream::app);
                    oFstream << "\r\n[" << (timePtr->tm_year + 1900) << "." << (timePtr->tm_mon+1) << "." << timePtr->tm_mday << " " << timePtr->tm_hour << ":" << timePtr->tm_min << ":" << timePtr->tm_sec << "] " << this->oSecurity->GetPw();
                    oFstream.close();
                }
                else
                {
                    this->oTerminal->Print("Generated password:");
                    this->oTerminal->Print( this->oSecurity->GetPw() );
                }
            break;
            default:
            break;
        }
    } while (bFinish == false);
}
Controller::PwGenerator::~PwGenerator()
{
    delete this->oTerminal;
}

Security.h

#pragma once
#include <string>
namespace Utility
{
    class Security
    {
        private:
            std::string sPassword;
            std::string sAllowedCharacter;
        public:
            Security();
            void GeneratePw(int iLength);
            std::string GetPw();
            ~Security();
    };
}

Security.cpp

Utility::Security::Security()
{
    this->sPassword = std::string("");
    this->sAllowedCharacter =      std::string("abcdefghijklmnopqrstuvwxyABCDEFGHIJKLMNOPQRSTUVWXY0123456789!$%&()[]{}=?");
}
void Utility::Security::GeneratePw(int iLength)
{
    this->sPassword = this->sAllowedCharacter;
    std::random_shuffle(this->sPassword.begin(), this->sPassword.end());
    this->sPassword = this->sPassword.substr(0, iLength);
    std::random_shuffle(this->sPassword.begin(), this->sPassword.end());
}
std::string Utility::Security::GetPw()
{
    return this->sPassword;
}
Utility::Security::~Security()
{
}

It seems that you could use a little C++11 in order to improve your code:

You can abstract away the operating system you are using by replacing the platform-dependent sleep functions by std::this_thread::sleep_for. If you can use C++14, you can even benefit from the added readability gained with the standard user-defined literals in <chrono>:

using namespace std::chrono_literals;
std::this_thread::sleep_for(3s);

You can avoid using most of the this-> you are using. Generally speaking, you don't need to prepend this->: it just makes the code longer and the compiler should warn you anyway if a local variable shadows a class member (-Wshadow with GCC for example).
Declaring all the variables at the beginning of a function is very C-like and makes it hard to know where a variable is used exactly. Generally speaking, you should only declare variables when you need them, in the tightest possible scope. Have a look at this piece of your code:

if (this->oTerminal->MenueSavePw() == true)
{
    this->oTerminal->Print("The password has been saved in a file.");
    oFstream.open("PasswordGenerator.txt", std::fstream::app);
    oFstream << "\r\n[" << (timePtr->tm_year + 1900) << "." << (timePtr->tm_mon+1) << "." << timePtr->tm_mday << " " << timePtr->tm_hour << ":" << timePtr->tm_min << ":" << timePtr->tm_sec << "] " << this->oSecurity->GetPw();
    oFstream.close();
}

Here, you can clearly declare oFstream in the if block since it isn't used before nor after. Moreover, it allows you to open the stream at construction and to let the end of the scope call the destructor, which will call close for you:

if (oTerminal->MenueSavePw() == true)
{
    oTerminal->Print("The password has been saved in a file.");
    std::ofstream oFstream("PasswordGenerator.txt", std::fstream::app);
    oFstream << "\r\n[" << (timePtr->tm_year + 1900) << "." << (timePtr->tm_mon+1) << "." << timePtr->tm_mday << " " << timePtr->tm_hour << ":" << timePtr->tm_min << ":" << timePtr->tm_sec << "] " << oSecurity->GetPw();
}

By the way, it is considered bad practice to compare conditions against true. You should simply change it to:

if (oTerminal->MenueSavePw())

Your lines are quite long too. You could split them into several lines to make your code more readable. Consider the following and compare it to your one-liner:

    oFstream << "\r\n["
             << (timePtr->tm_year + 1900) << "."
             << (timePtr->tm_mon+1) << "."
             << timePtr->tm_mday << " "
             << timePtr->tm_hour << ":"
             << timePtr->tm_min << ":"
             << timePtr->tm_sec << "] "
             << oSecurity->GetPw();

Sometimes, when printing things that have a structure (like printing XML data), you can even use fancier indentation in order to reflect the structure of the data you're printing.
There are too many useless pointers and dynamic allocation in your code. In C++, whenever possible, you should use automatic variables instead of dynamically allocated ones. Here is what your main should look like:

void main()
{
    Controller::PwGenerator oController;
    oController.Run();
}

Terser, safer and even probably faster (unnoticeable in this case though). Whenever you use new, your program may throw an std::bad_alloc, and dynamically allocated memory doesn't free itself when an exception occurs while automatic variables are correctly destructed when an exception is thrown. Bottom line: use pointers when you have to and regular variables whenever you can.
I said that this is what main should look like, but it's actually not true: your main has a void return type, which makes the program ill-formed in regards to the standard. While most compilers will accept it, you better have an int return type for your main to be standard-compliant. You don't have to return anything from it though since it is guaranteed that the compiler will add a return 0; at the end of it if it didn't encounter any return statement.
std::random_shuffle has been deprecated in C++14 and will be removed from the standard library in C++17 because it might be unsafe. You should consider using std::shuffle instead, with a random number generator from the C++11 header <random>:

std::random_device rd;
std::mt19937 g(rd());

std::shuffle(sPassword.begin(), sPassword.end(), g);


Returning the prime factors of a number I am trying to create a function prime-factors that returns the prime factors of a number. To do so, I created is-prime function, and prime-factors-helper that will do a recursive check of the prime factors.

(defun is-prime (n &optional (d (- n 1)))
  (if (/= n 1) (or (= d 1)
          (and (/= (rem n d) 0)
               (is-prime  n (- d 1)))) ()))

(defun prime-factors-helper (x n)
   (if (is-prime x)
       (list x)
       (if (is-prime n)
            (if (AND (= (mod x n) 0) (<= n (/ x 2)))
                (cons n (prime-factors-helper (/ x n) n))
                (prime-factors-helper x (+ 1 n)))
            (prime-factors-helper x (+ 1 n)))))

(defun prime-factors (x)
    (prime-factors-helper x 2))

I have a problem of optimisation. When I have a big number such as 123456789, I get this error message "Stack overflow (stack size 261120)". I believe because the correct answer of (prime-factors 123456789) is (3 3 3607 3803), my program will take so long to find the next prime factor once it constructs the list with the two first elements (3 3). How can I optimise my code?

 CL-USER 53 > (prime-factors 512)
 (2 2 2 2 2 2 2 2 2)

 CL-USER 54 > (prime-factors 123456789)

 Stack overflow (stack size 261120).
   1 (abort) Return to level 0.
   2 Return to top loop level 0.

 Type :b for backtrace or :c <option number> to proceed.
 Type :bug-form "<subject>" for a bug report template or :? for other options

There are several problems with your code.

Style

is-prime is C/Java style. Lispers use primep or prime-number-p.
zerop is clearer than (= 0 ...).
Lispers use indentation, not paren counting, to read code.  Your
code is thus virtually unreadable.  Please use Emacs if you are
unsure how to format lisp properly.

Stack overflow

is-prime is tail-recursive, so if you compile it, it should become a
simple loop and there should be no stack issues.

However, do not rush with it yet.

Algorithm

Number of iterations

Let us use trace to see the
problems:

> (prime-factors 17)
1. Trace: (IS-PRIME '17)
2. Trace: (IS-PRIME '17 '15)
3. Trace: (IS-PRIME '17 '14)
4. Trace: (IS-PRIME '17 '13)
5. Trace: (IS-PRIME '17 '12)
6. Trace: (IS-PRIME '17 '11)
7. Trace: (IS-PRIME '17 '10)
8. Trace: (IS-PRIME '17 '9)
9. Trace: (IS-PRIME '17 '8)
10. Trace: (IS-PRIME '17 '7)
11. Trace: (IS-PRIME '17 '6)
12. Trace: (IS-PRIME '17 '5)
13. Trace: (IS-PRIME '17 '4)
14. Trace: (IS-PRIME '17 '3)
15. Trace: (IS-PRIME '17 '2)
16. Trace: (IS-PRIME '17 '1)
16. Trace: IS-PRIME ==> T
15. Trace: IS-PRIME ==> T
14. Trace: IS-PRIME ==> T
13. Trace: IS-PRIME ==> T
12. Trace: IS-PRIME ==> T
11. Trace: IS-PRIME ==> T
10. Trace: IS-PRIME ==> T
9. Trace: IS-PRIME ==> T
8. Trace: IS-PRIME ==> T
7. Trace: IS-PRIME ==> T
6. Trace: IS-PRIME ==> T
5. Trace: IS-PRIME ==> T
4. Trace: IS-PRIME ==> T
3. Trace: IS-PRIME ==> T
2. Trace: IS-PRIME ==> T
1. Trace: IS-PRIME ==> T
(17)

You do 17 iterations when only (isqrt 17) = 4 iterations are necessary.

Recalculations

Now compile is-prime to turn recursion into a loop and see:

> (prime-factors 12345)
1. Trace: (IS-PRIME '12345)
1. Trace: IS-PRIME ==> NIL
1. Trace: (IS-PRIME '2)
1. Trace: IS-PRIME ==> T
1. Trace: (IS-PRIME '12345)
1. Trace: IS-PRIME ==> NIL
1. Trace: (IS-PRIME '3)
1. Trace: IS-PRIME ==> T
1. Trace: (IS-PRIME '4115)
1. Trace: IS-PRIME ==> NIL
1. Trace: (IS-PRIME '3)
1. Trace: IS-PRIME ==> T
1. Trace: (IS-PRIME '4115)
1. Trace: IS-PRIME ==> NIL
1. Trace: (IS-PRIME '4)
1. Trace: IS-PRIME ==> NIL
1. Trace: (IS-PRIME '4115)
1. Trace: IS-PRIME ==> NIL
1. Trace: (IS-PRIME '5)
1. Trace: IS-PRIME ==> T
1. Trace: (IS-PRIME '823)
1. Trace: IS-PRIME ==> T
(3 5 823)

You are checking the primality of the same numbers several times!

Extra optimization

primep can find a divisor, not just check primality.

Optimized algorithm

(defun compositep (n &optional (d (isqrt n)))
  "If n is composite, return a divisor.
Assumes n is not divisible by anything over d."
  (and (> n 1)
       (> d 1)
       (if (zerop (rem n d))
           d
           (compositep n (- d 1)))))

(defun prime-decomposition (n)
  "Return the prime decomposition of n."
  (let ((f (compositep n)))
    (if f
        (nconc (prime-decomposition (/ n f))
               (prime-decomposition f))
        (list n))))

Note that one final optimization is possible -
memoization of
compositep:

(let ((known-composites (make-hash-table)))
  (defun compositep (n &optional (d (isqrt n)))
    "If n is composite, return a divisor.
Assumes n is not divisible by anything over d."
    (multiple-value-bind (value found-p) (gethash n known-composites)
      (if found-p
          value
          (setf (gethash n known-composites)
                (and (> n 1)
                     (> d 1)
                     (if (zerop (rem n d))
                         d
                         (compositep n (- d 1)))))))))

or, better yet, of prime-decomposition:

(let ((known-decompositions (make-hash-table)))
  (defun prime-decomposition (n)
    "Return the prime decomposition of n."
    (or (gethash n known-decompositions)
        (setf (gethash n known-decompositions)
              (let ((f (compositep n)))
                (if f
                    (append (prime-decomposition (/ n f))
                            (prime-decomposition f))
                    (list n)))))))

note the use or append instead of
nconc.

Another interesting optimization is changing the iteration in
compositep from descending to ascending.
This should speedup it up considerably as it would terminate early more
often:

(let ((known-composites (make-hash-table)))
  (defun compositep (n)
    "If n is composite, return a divisor.
Assumes n is not divisible by anything over d."
    (multiple-value-bind (value found-p) (gethash n known-composites)
      (if found-p
          value
          (setf (gethash n known-composites)
                (loop for d from 2 to (isqrt n)
                  when (zerop (rem n d))
                  return d))))))

PS. This is identical to https://stackoverflow.com/a/49369108/850781


Improve performance of Gaussian kernel function evaluation I need to improve the performance of a function that calculates the integral of a two-dimensional kernel density estimate (obtained using the function stats.gaussian_kde) where the domain of integration is all points that evaluate below a given value.

I've found that a single line takes up pretty much 100% of the time used by the code:

insample = kernel(sample) < iso

(see the code below). This line stores in the list insample all the values in sample that evaluate in kernel to less than the float iso.

I made this question (slightly modified) 6 months ago over at Stack Overflow and the best answer involved parallelization. I'm hoping to avoid parallelization (gives way too much trouble as can be seen in the question linked above), but I'm open to any improvement in performance achieved by means of virtually any other way (including Cython and any package available).

Here's the minimum working example (MWE):

import numpy as np
from scipy import stats

# Define KDE integration function.
def kde_integration(m1, m2):

    # Perform a kernel density estimate (KDE) on the data.
    values = np.vstack([m1, m2])
    kernel = stats.gaussian_kde(values, bw_method=None)

    # This list will be returned at the end of this function.
    out_list = []

    # Iterate through all floats in m1, m2 lists and calculate for each one the
    # integral of the KDE for the domain of points located *below* the KDE
    # value of said float eveluated in the KDE.
    for indx, m1_p in enumerate(m1):

        # Compute the point below which to integrate.
        iso = kernel((m1_p, m2[indx]))

        # Sample KDE distribution
        sample = kernel.resample(size=100)

        # THIS TAKES ALMOST 100% OF THE COMPUTATION TIME.
        # Filter the sample.
        insample = kernel(sample) < iso

        # Monte Carlo Integral.
        integral = insample.sum() / float(insample.shape[0])
        # Avoid 'nan' and/or 'infinite' values down the line.
        integral = integral if integral > 0. else 0.000001

        # Append integral value for this point to list that will return.
        out_list.append(round(integral, 2))

    return out_list

# Generate some random two-dimensional data:
def measure(n):
    "Return two coupled measurements."
    m1 = np.random.normal(size=n)
    m2 = np.random.normal(scale=0.5, size=n)
    return m1+m2, m1-m2

# Random data.
m1, m2 = measure(100)
# Call KDE integration function.
kde_integration(m1, m2)

Any help/suggestions/ideas will be very much appreciated.

I am never forget the day I first meet the great Lobachevsky.
In one word he told me secret of success in NumPy:
Vectorize!
Vectorize!
In every loop inspect your i's
Remember why the good Lord made your eyes!
So don't shade your eyes,
But vectorize, vectorize, vectorize—
Only be sure always to call it please 'refactoring'.

def kde_integration(m1, m2, sample_size=100, epsilon=0.000001):
    values = np.vstack((m1, m2))
    kernel = stats.gaussian_kde(values, bw_method=None)
    iso = kernel(values)
    sample = kernel.resample(size=sample_size * len(m1))
    insample = kernel(sample).reshape(sample_size, -1) < iso.reshape(1, -1)
    return np.maximum(insample.mean(axis=0), epsilon)

Update
I find that my kde_integration is about ten times as fast as yours with sample_size=100:
>>> m1, m2 = measure(100)
>>> from timeit import timeit
>>> timeit(lambda:kde_integration1(m1, m2), number=1) # yours
0.7005664870084729
>>> timeit(lambda:kde_integration2(m1, m2), number=1) # mine (above)
0.07272820601065177

But the advantage disappears with larger sample sizes:
>>> timeit(lambda:kde_integration1(m1, m2, sample_size=1024), number=1)
1.1872510590037564
>>> timeit(lambda:kde_integration2(m1, m2, sample_size=1024), number=1)
1.2788789629994426

What this suggests is that there is a "sweet spot" for sample_size * len(m1) (perhaps related to the computer's cache size) and so you'll get the best results by processing the samples in chunks of that length.
You don't say exactly how you're testing it, but different results are surely to be expected, since scipy.stats.gaussian_kde.resample "Randomly samples a dataset from the estimated pdf".


LinkedHashMap as LRU cache I have a simple implementation for a LRU cache using LinkedHashMap.

I want it to be as generic as   possible.
This is not for production use, just practice, so I don't care
if its thoroughly robust as far as it
is correct. However, I will welcome
any comments, especially the ones
which might make this better with
simple changes :)
Are there any other ways of doing this?

class LRUCache<E> {

    @SuppressWarnings("unchecked")
    LRUCache(int size)
    {
        fCacheSize = size;

        // If the cache is to be used by multiple threads,
        // the hashMap must be wrapped with code to synchronize
        fCacheMap = Collections.synchronizedMap
        (
            //true = use access order instead of insertion order
            new LinkedHashMap<Object,E>(fCacheSize, .75F, true)
            {
                @Override
                public boolean removeEldestEntry(Map.Entry eldest)
                {
                    //when to remove the eldest entry
                    return size() > 99 ;   //size exceeded the max allowed
                }
            }
        );
    }

    public void put(Object key, E elem)
    {
        fCacheMap.put(key, elem);
    }

    public E get(Object key)
    {
        return fCacheMap.get(key);
    }

    private Map<Object,E> fCacheMap;
    private int fCacheSize;
}

First, use generics properly.

Second, why do you track the cache size if when you need it, you get a hardcoded 99? I will assume that was some sort of mistake.

Third, if you really want to annoy most truely professional java developers and course instructors (i.e, not these that write Java code as if they were programming in C#, C++ or Pascal), prefix your class and variable names. If you don't want to do so, do not make this.

Fourth, I would personally prefer to handle synchronization myself. This way, if the class is expanded to include some method that makes two or more operations in the map, I will not break it. I added the atomicGetAndSet method to show this. If I relied in the synchronization provided by the Collections.synchronizedMap instead of my own, the atomicGetAndSet method would not be atomic and it would break if LRUCache instances are used concurrently.

With this in mind, your code became this:

public class LRUCache<K, V> {

    private final Map<K, V> cacheMap;

    public LRUCache(final int cacheSize) {

        // true = use access order instead of insertion order.
        this.cacheMap = new LinkedHashMap<K, V>(cacheSize, 0.75f, true) {
            @Override
            protected boolean removeEldestEntry(Map.Entry<K, V> eldest) {
                // When to remove the eldest entry.
                return size() > cacheSize; // Size exceeded the max allowed.
            }
        };
    }

    public synchronized void put(K key, V elem) {
        cacheMap.put(key, elem);
    }

    public synchronized V get(K key) {
        return cacheMap.get(key);
    }

    public synchronized V atomicGetAndSet(K key, V elem) {
        V result = get(key);
        put(key, elem);
        return result;
    }
}


TPL Async Await in Console Apps I just picked up Async Await and I am trying to use Async Await in Console App along with TPL for querying webservices. The code works fine and I am able to get the speed using HttpClient and async. Wanted to know if I am overusing async/await and if there is anything that I should avoid and can be simplified further.

I have read Albahari's threading e-book and the following articles to understand the usage.

http://www.tugberkugurlu.com/archive/how-and-where-concurrent-asynchronous-io-with-asp-net-web-api

http://blogs.msdn.com/b/pfxteam/archive/2011/01/13/10115163.aspx

https://scalablenotions.wordpress.com/2015/05/02/tpl-and-async-await-best-practices-for-the-busy-developer/

class Program
{
    public class Member
    {
        public string N { get; set; }
        public string E { get; set; }
    }

    static void Main()
    {
        List<Member> list = new List<Member>();
        Task.Run(async () => { await SubscribeMembersusingAPI(list); }).GetAwaiter().GetResult();

    }

    // Define other methods and classes here
    public static async Task SubscribeMembersusingAPI(List<Member> members)
    {
        List<string> updateemailstosubscribed = new List<string>();
        HttpClient client = new HttpClient();

        try
        {
            await Task.WhenAll(members.Select(m => SubscribeusingAPI(client, m)));
        }
        catch (Exception e)
        {
            //logger.Error("Error in Async task", e);
        }
        return;
    }

    public static async Task<bool> SubscribeusingAPI(HttpClient client, Member member)
    {
        try
        {
            List<KeyValuePair<String, String>> listkvp = new List<KeyValuePair<string, string>>();
            listkvp.Add(new KeyValuePair<string, string>("e", member.E.Trim()));
            listkvp.Add(new KeyValuePair<string, string>("n", member.N.Trim()));
            listkvp.Add(new KeyValuePair<string, string>("boolean", "true"));
            FormUrlEncodedContent formpost = new FormUrlEncodedContent(listkvp);
            var response = await client.PostAsync("", formpost);
            string responsebody = await response.Content.ReadAsStringAsync();
            if (responsebody == "1" || responsebody == "Member present")
                return true;
            else
                Console.WriteLine(string.Format("FAIL, Error while subscribing e: {0}, n: {1}, response: {2}", member.E, member.N, responsebody));
        }
        catch (Exception e)
        {
            Console.WriteLine(string.Format("FAIL, Error while subscribing e: {0}, n: {1} ", member.E, member.N), e);
        }
        return false;
    }
}

Let’s try to apply SRP, and get rid of mutable state to increase code maintainability. It inflates C# code a little bit, but usually makes sense.

Demo:

  IEnumerable<Member> members = new Member[] { new Member("n","e") };
  await members
      .SubscribeAsync()
      .LogAsync(Console.Out);

Where Member could be immutable:

public class Member
{
    public Member(string n, string e)
    {
        N = n.Trim();
        E = e.Trim();
    }

    public string N { get; }
    public string E { get; }
    public override string ToString() => $"N={N}, E={E}";
}

We will use this universal helper class to log task exceptions:

static class Logging
{
    public static async Task LogAsync(this Task task, TextWriter writer)
    {
        try
        {
            await task;
        }
        catch (AggregateException ex)
        {
            foreach (var exx in ex.Flatten().InnerExceptions)
                await writer.WriteLineAsync(exx.Message);
        }
        catch(Exception ex)
        {
            await writer.WriteLineAsync(ex.Message);
        }
    }
}

Now subscription helpers:

static class Subscription
{
    public static Task SubscribeAsync(this IEnumerable<Member> members) =>
        Task.WhenAll(from member in members
                     select member.SubscribeAsync());

    public static Task<SubscriptionResponse> SubscribeAsync(this Member member) =>
        new SubscriptionRequest(member).ExecuteAsync();
}

Subscription request:

public class SubscriptionRequest
{
    const string RequestUrl = "";

    public SubscriptionRequest(Member member)
    {
        Member = member;
    }

    public async Task<SubscriptionResponse> ExecuteAsync()
    {
        using (var client = new HttpClient())
            try
            {
                var response = await client.PostAsync(RequestUrl, Content);
                var responseBody = await response.Content.ReadAsStringAsync();
                return new SubscriptionResponse(responseBody);
            }
            catch(Exception ex)
            {
                throw new SubscriptionException(Member, ex);
            }
    }

    FormUrlEncodedContent Content => new FormUrlEncodedContent(Kvps);
    IEnumerable<KeyValuePair<string, string>> Kvps => new[]
    {
        new KeyValuePair<string, string>("e", Member.E),
        new KeyValuePair<string, string>("n", Member.N),
        new KeyValuePair<string, string>("boolean", "true")
    };

    Member Member { get; }
}

And response:

public class SubscriptionResponse
{
    public SubscriptionResponse(string body)
    {
        Body = body;
        if (!Success)
            throw new InvalidOperationException(body);
    }

    public bool Subscribed => Body == "1";
    public bool AlreadySubscribed => Body == "Member present";
    bool Success => Subscribed || AlreadySubscribed;
    string Body { get; }
}

The only missing part:

public class SubscriptionException : Exception
{
    public SubscriptionException(Member member, Exception error) :
        base($"{member} subscription failed, error: {error.Message}", error)
    {
    }
}

P.S. Generally speaking, C# is a pretty verbose language, so it is often punishing good programming practices…


(Project Euler #1) Find the sum of all the multiples of 3 or 5 below 1000 I have decent experience in programming before (mostly C++), but I am very, very, very new to Python, and I decided to try out Project Euler as an exercise.  Here's the description of Problem 1:

  If we list all the natural numbers below 10 that are multiples of 3 or
  5, we get 3, 5, 6 and 9. The sum of these multiples is 23.

  Find the sum of all the multiples of 3 or 5 below 1000.

Here's my solution:

sum = 0
for n in range(0, 1000):
    if n % 3 == 0 or n % 5 == 0:
        sum += n
print(sum)

The program works fine.  What I want to get from a code review:

whether I did anything non-Pythonic;
whether I did anything outdated;
whether I did anything against the PEP 8;
whether there's a better way of doing this;
...

The keyword sum is a built in function and you shouldn't name variables from within the reserved keywords.

Here's a list of the most common used keywords which you shouldn't be naming any of your variables:

[False, class, finally, is, return, None, continue, for, lambda, try, True, def, from, nonlocal, while, and, del, global, not, with, as, elif, if   or, yield, assert, else, import, pass, break, except, in, raise]

And for the built-in functions check https://www.programiz.com/python-programming/methods/built-in

for n in range(0, 1000):

Can be written for n in range(1000):

The range() function as well as the whole Python is zero-indexed.

You might also want to use comprehension syntax (which is much more efficient than explicit loops) and enclose it inside a function like this:

def get_multiples(upper_bound):
    """Return sum of multiples of 3 and multiples of 5 within specified range."""
    return sum(number for number in range(upper_bound) if not number % 3 or not number % 5)


Sieve of Eratosthenes and twin prime finder I made this program to find primes and twin pairs. I'd like feedback on anything from formatting to content to technique. In short: how would this be different if it was written by an experienced professional?

The program runs five methods:

getMax asks for an integer input which is the maximum number below which to find primes and twin prime pairs. It also has an error message to catch unacceptable input and ask again.
sieve checks every number between 0 and max and returns a boolean array which says whether each index is prime or not.
primesCount counts many primes there are, and prints it.
primesList creates an integer array of all primes, and prints it.
pairsCount counts how many twin prime pairs there are, and prints it.
pairsList creates a string array of all twin prime pairs, and prints it.

Code:

public static void main(String[] args) {

    Scanner in = new Scanner(System.in);
    int max = getMax(in);

    boolean[] primesSieve = sieve(max);
    int primesCount = primesCount(primesSieve);
    int[] primesList = primesList(primesSieve, primesCount);
    int pairsCount = pairsCount(primesList);
    pairsList(primesList, pairsCount);

}

// User input for maximum number to sieve
public static int getMax(Scanner in) {
    System.out.print("Maximum number to sieve: ");
    while (!in.hasNextInt()) {
        String input = in.next();
        System.out.println("Error: \"" + input + "\" is not a number. Try again.");
        System.out.print("Maximum number to sieve: ");
    }
    int max = in.nextInt();
    return max;
}

// Create boolean array of prime status of integers less than max
public static boolean[] sieve(int max) {

    // Create initial array and assume all are primes
    boolean[] arePrimes = new boolean[max];
    for (int i = 0; i<max; i++) {
        arePrimes[i] = true;
    }

    // Mark multiples of each prime as non-prime
    for (int i = 2; i<max; i++) {
        if (arePrimes[i]) {
            for (int m = 2; i * m < max; m++) {
                arePrimes[i*m] = false;
            }
        }
    }
    return arePrimes;
}

// Count and print how many primes are less than max
public static int primesCount(boolean[] primes) {
    int count = 0;
    for (boolean prime : primes) {
        if (prime) {
            count++;
        }
    }
    System.out.println("Number of primes: " + count);
    return count;
}

// Create and print array listing all primes less than max
public static int[] primesList(boolean[] sievedPrimes, int numberOfPrimes) {
    int[] listPrimes = new int[numberOfPrimes-2];
    int n = 0;

    for (int i=2; i<sievedPrimes.length; i++) {
        if (sievedPrimes[i]) {
            listPrimes[n] = i;
            n++;
        }
    }
    System.out.println("List of primes: " + Arrays.toString(listPrimes));
    return listPrimes;
}

// Count and print the number of twin prime pairs
public static int pairsCount(int[] listOfPrimes) {
    int pairsCount = 0;
    for (int i=0; i<listOfPrimes.length-1; i++) {
        if (listOfPrimes[i+1] - listOfPrimes[i] == 2) {
            pairsCount++;
        }
    }
    System.out.println("Number of twin prime pairs: " + pairsCount);
    return pairsCount;
}

// Create and print array listing all pairs of twin primes less than max
public static String[] pairsList(int[] listOfPrimes, int pairsCount) {
    String[] twinPrimes = new String[pairsCount];
    int n = 0;
    for (int i=0; i<listOfPrimes.length-1; i++) {
        if (listOfPrimes[i+1] - listOfPrimes[i] == 2) {
            twinPrimes[n] = listOfPrimes[i] + " and " + listOfPrimes[i+1];
            n++;
        }
    }
    System.out.println("List of twin prime pairs: " + Arrays.toString(twinPrimes));
    return twinPrimes;
}

Implementation

// Mark multiples of each prime as non-prime
for (int i = 2; i<max; i++) {
    if (arePrimes[i]) {
        for (int m = 2; i * m < max; m++) {
            arePrimes[i*m] = false;
        }
    }
}

Pretty close to the perfect implementation.

However...

Consider the Transitive relation of the multiplication operation: 5 times 2 is 10. 2 times 5 is 10. It doesn't matter in which order you multiply things.

So when you multiply, say, 3 by 2, you're just multiplying 2 by 3 again. And the same goes for 5 by 2, 7 by 2, 11 by 2... but also 5 by 3, 7 by 3, 7 by 5, etc.

What you'd do in this case is start m off at the value of i. This solves the issue where you're multiplying 2 with 3 and 3 with 2, thus checking the same thing twice.

There's another issue: Now that we're effectively checking (i * i) < max in the inner loop on the first iteration, it doesn't make sense for the outer loop to check for on i < max. Instead, you'd want to check for (i * i) < max there as well.

// Mark multiples of each prime as non-prime
for (int i = 2; i * i < max; i++) {
    if (arePrimes[i]) {
        for (int m = i; i * m < max; m++) {
            arePrimes[i*m] = false;
        }
    }
}

I was also a bit worried about getMax willing to accept negative numbers, but Java will throw a java.lang.NegativeArraySizeException, and as such, the error should be pretty clear. For use in a larger application, you might want to handle the exception, rather than the program forcibly exiting.

Documentation

You should use Javadoc style comments. Simple //comments before each function generally don't get picked up by IDE's. By making use of Javadoc, you can provide descriptions of what a method does and what the meaning of each parameter is. Like that, not only will other programmers easily be able to search for information in a method's documentation, it can also be automated - like an IDE showing a tooltip for each method parameter as you type them.


An arithmetic_mean Function For Various Type Arbitrary Nested Iterable Implementation in C++ This is a follow-up question for A recursive_count Function For Various Type Arbitrary Nested Iterable Implementation in C++ and A Summation Function For Boost.MultiArray in C++. I am trying to implement an arithmetic_mean template function for calculating the arithmetic mean value of various type arbitrary nested iterable things. The recursive_reduce function (thanks to G. Sliepen's answer) and the recursive_size function are used here. Because both recursive_reduce function and recursive_size function are needed in the arithmetic_mean function here, there are two new concepts is_recursive_reduceable and is_recursive_sizeable created as follows.
template<typename T>
concept is_recursive_reduceable = requires(T x)
{
    recursive_reduce(x, 0.0);
};

template<typename T>
concept is_recursive_sizeable = requires(T x)
{
    recursive_size(x);
};

Next, the main part of arithmetic_mean template function:
template<class T> requires (is_recursive_reduceable<T> && is_recursive_sizeable<T>)
auto arithmetic_mean(const T& input)
{
    return (recursive_reduce(input, 0.0)) / (recursive_size(input));
}

Some test cases of this arithmetic_mean template function.
//  std::vector<int> case
std::vector<int> test_vector = {
    1, 2, 3
};
auto arithmetic_mean_result1 = arithmetic_mean(test_vector);
std::cout << arithmetic_mean_result1 << std::endl;

//  std::vector<std::vector<int>> case
std::vector<decltype(test_vector)> test_vector2 = {
    test_vector, test_vector, test_vector
};
auto arithmetic_mean_result2 = arithmetic_mean(test_vector2);
std::cout << arithmetic_mean_result2 << std::endl;

// std::deque<int> case
std::deque<int> test_deque;
test_deque.push_back(1);
test_deque.push_back(1);
test_deque.push_back(1);
auto arithmetic_mean_result3 = arithmetic_mean(test_deque);
std::cout << arithmetic_mean_result3 << std::endl;

// std::deque<std::deque<int>> case
std::deque<decltype(test_deque)> test_deque2;
test_deque2.push_back(test_deque);
test_deque2.push_back(test_deque);
test_deque2.push_back(test_deque);
auto arithmetic_mean_result4 = arithmetic_mean(test_deque2);
std::cout << arithmetic_mean_result4 << std::endl;

A Godbolt link is here.
All suggestions are welcome.
The summary information:

Which question it is a follow-up to?
A recursive_count Function For Various Type Arbitrary Nested Iterable Implementation in C++ and
A Summation Function For Boost.MultiArray in C++

What changes has been made in the code since last question?
The arithmetic_mean template function is the main part here.

Why a new review is being asked for?
I am not sure if it is a good idea to add is_recursive_sizeable and is_recursive_reduceable concepts here. Moreover, I am wondering that the name of used concept is understandable.

Instead of simply printing output, it would be better to have self-checking unit tests, so you can fail your build as soon as you break something, rather than having to notice a change in the output.
You probably want to include some tests with very large (positive and negative) values to make sure you get accurate results with these, and don't fall foul of integer overflow (which will cause gross inaccuracy with unsigned types, and is Undefined Behaviour with signed integer types).
It's disappointing that there are no tests using primitive types other than int.


Project Euler #1 in Haskell Looking for feedback/critiques/alternatives to my solution for the 1st Project Euler question:

{-|
If we list all the natural numbers below 10 that are multiples of 3 or 5, we get 3,5,6 and 9. The sum of these multiples is 23. Find the sum of all the multiples of 3 or 5 below 1000.
-}

import Data.List

-- Finds all multiples of a factor below set limit, by computing factor*n and adding to a list, until factor*n > limit
f_multiple :: [Int] -> Int -> Int -> Int -> [Int]
f_multiple multiples current limit factor
        | (current * factor) >= limit   = multiples
        | otherwise         = f_multiple (multiples ++ [current * factor]) (current + 1) limit factor

sum_multiples :: Int -> Int -> Int -> Int
sum_multiples max first_factor second_factor = foldr (+) 0 ( (f_multiple [] 0 max first_factor) `union` (f_multiple [] 0 max second_factor))

There are several approaches to Project Euler Question 1.  The one you have chosen is not particularly efficient, as it involves creating two lists, then merging them.  I'm not going to present alternative solutions here.  If you are going with this approach, you can write it much more simply:

sum_multiples :: Int -> Int -> Int -> Int
sum_multiples max factor1 factor2 = sum $ multiples1 `union` multiples2
  where
    multiples1 = [factor1, 2 * factor1 .. max - 1]
    multiples2 = [factor2, 2 * factor2 .. max - 1]

In particular,

foldr (+) 0 is just the built-in sum function.
f_multiple can be replaced with ranges.

In terms of style, your last line is hard to read due to its length.  To shorten it,

Use shorter variable names, e.g. factor1 instead of first_factor.
Give names to complex subexpressions using a where clause.
Use the $ operator to form a "pipeline", instead of nesting parentheses.


PDO Dynamic query builder I'm trying to improve my php oop programming skills. As an exercise I wrote this class that will generate and execute dynamic sql queries. I've also writed a class to call various sanitization filters that php has built in.

QueryBuilder.php class

<?php

class QueryBuilder{

private $db = null;
private $stmt;
private $table;
private $param;
private $cols, $columns;
private $holders, $placehold;
private $fields, $field;

public $data;
public $results;

public function __construct(\PDO $db){
    $this->db = $db;
}

public function insert($table, array $data, array $columns){
    $holders = $this->setHolders($columns);
    $cols = $this->setColumns($columns);
    $stmt = $this->db->prepare("INSERT INTO $table ($cols) VALUES ($holders)");
    return $stmt->execute($data);
}

public function select($table, array $columns, $field, $param){
    $cols = $this->setColumns($columns);
    $stmt = $this->db->prepare("SELECT $cols FROM $table WHERE $field = ?");
    $stmt->execute(array($param));
    $result = $stmt->fetch();
    return json_encode($result);
}

public function edit($table, array $columns, array $data, $param){
    $fields = $this->setFields($columns);
    $stmt = $this->db->prepare("UPDATE $table SET $fields WHERE $param = ?");
    return $stmt->execute($data);
}

public function delete($table, array $data, $param){
    $stmt = $this->db->prepare("DELETE FROM $table WHERE $param = ?");
    return $stmt->execute($data);
}

private function setColumns(array $columns){
    $cols = implode(', ', array_values($columns));
    return $cols;
}

private function setFields(array $columns){
    $fields = implode(' = ?, ', array_values($columns));
    return $fields.' = ?';
}

private function setHolders(array $columns){
    $holders = array_fill(1 ,count($columns),'?');
    return implode(', ',array_values($holders));
}

}

?>

DataSanitizer.php class

<?php

class DataSanitizer{

private $value;
private $sanitized_value;

public function intSanitize(int $value){
    $sanitized_value = filter_var($value, FILTER_SANITIZE_NUMBER_INT);
    return $sanitized_value;
}

public function stringSanitize(string $value){
    $sanitized_value = filter_var($value, FILTER_SANITIZE_STRING);
    return $sanitized_value;
}

public function floatSanitize(float $value){
    $sanitized_value = filter_var($value, FILTER_SANITIZE_NUMBER_FLOAT);
    return $sanitized_value;
}

public function emailSanitize(string $value){
    $sanitized_value = filter_var($value, FILTER_SANITIZE_EMAIL);
    return $sanitized_value;
}

public function validateEmail(string $value){
  $sanitized_value = filter_var($value, FILTER_VALIDATE_EMAIL);
  return $sanitized_value;
}

}

?>

Classes usage example:

<?php
// Usaually i use the spl_autoloader_register();
require_once 'QueryBuilder.php';
reqiure_once 'DataSanitizer.php';

// this file holds the PDO connection stored inside the $db variable
require_once 'Config.php';

$query = new QueryBuilder($db);
$sanitize = new DataSanitizer;

$table = 'test_table';
$data = array($sanitize->stringSanitize('hello'),$sanitize->stringSanitize('world'));
$col = array('col1','col2');

$query->insert($table, $data, $col);

?>

The attempt is quite good, you managed to elude the most critical faults. There are only rather minor and architectural nitpicks.

First of all, I don't know what does DataSanitizer to do here but it shouldn't be any part of a query builder. Data sanitization is tangential to data persistence and they have nothing in common. So it should be just taken away.

As of the query builder, there are two main problems

a potential vulnerability which I explained recently in the other answer. At the very least all identifiers must be formatted according to database rules.
select is unusable in the real life. Nobody have seen such a method in the wild, it lurks only in such educational builders. If you want a query builder for SELECT, then either create a real one, with distinct methods for each SQL clause, or stick to a free-form query() method. Trust me, SQL is a precious and powerful programming language, do not diminish it to a crippled gibberish.

Also

Looks like class variables beside $db are just for decoration, cause they never used in the code? And for the good, so just ditch them away.
consider implementing PSR-4 Autoload for your classes.
usually, the data format for such methods as insert and edit is like

'col1' => value1,
'col2' => value2,

so you have to provide only one array with data. However it has other drawbacks with security already explained above and so must be used with caution

All in all, I believe you would benefit from my article on the common mistakes in database wrappers even if yours is a query builder, not a wrapper.


Function to copy characters from one buffer to another given the maximum size #include <iostream>
#include <vector>
#include <algorithm>

template <typename T>
int copy_max(T& to, T& from, unsigned char delim, size_t limit)
{
    typename T::iterator pos = std::find(from.begin(), from.end(), delim);
    typename T::iterator prev_pos = pos;

    if(pos == from.end())
    {
        return -1;
    }

    size_t index = std::distance(from.begin(), pos);

    while(pos != from.end()
    && index <= limit)
    {
        prev_pos = pos;
        pos = std::find(pos + 1, from.end(), delim);
        index = std::distance(from.begin(), pos);
    }

    index = std::distance(from.begin(), prev_pos);

    if(index == 0)
    {
        return -1;
    }
    to.insert(to.begin(), from.begin(), prev_pos);
    from.erase(from.begin(), prev_pos);
    return index;
}

void output(const std::vector<unsigned char>& va, const std::vector<unsigned char>& vb )
{
    std::string str(va.begin(), va.end());
    std::cout << "To: " << str << std::endl;
    str.assign(vb.begin(), vb.end());
    std::cout << "From: " << str << std::endl;
}
static int case_num = 1;
#define expect(a, b) \
std::cout << "[" << case_num ++ << "] "; \
if(a == b) \
std::cout << #a << " == " << #b << " TRUE " << std::endl;\
else {\
std::cout << "FAIL " << #a << " != " << #b << " FALSE " << std::endl;\
throw new int;\
}

int main() {
    std::string b = "Hi\x24The\x24HowAreYou\x24?";
    std::vector<unsigned char> va;
    std::vector<unsigned char> vb(b.begin(), b.end());

    int index = copy_max(va, vb, '\x24', 5);
    expect(index, 2);
    output(va, vb);

    va.clear();
    index = copy_max(va, vb, '\x24', 5);
    expect(index, 4);

    va.clear();
    index = copy_max(va, vb, '\x24', 5);
    expect(index, -1);

    return 0;
}

Don't mind the main() function and testing macros, this is just something I threw together in a matter of minutes.

The function copy_max() is mis-named, No one maintaining this code would know from the name of the function that the from string is being shortened by the from.erase(from.begin(), prev_pos). Copy does not imply destruction of the source, move might be more appropriate.

You may also want to consider the Single Responsibility Principle and break copy_max() into multiple functions. I see at least 2 functions there. This loop is complex enough to be it's own function and can be called from
copy_max()

    while(pos != from.end() && index <= limit)
    {
        prev_pos = pos;
        pos = std::find(pos + 1, from.end(), delim);
        index = std::distance(from.begin(), pos);
    }

Doing so makes the logic of copy_max() clearer.

I'm fairly sure that you can calculate index only once. I'm not sure why it's being calculated in 3 different places in copy_max().

The title is miss leading as well, since this will copy more than just characters.


TicTacToe game in Python I'd just like to ask if there's anyway to make the functions play_Game() or is_move_valid() anymore efficient. For example, using a loop of some sort, I haven't been able to figure out how and would like any help in trying to make those functions shorter.

#TicTacToe
import sys
rows=[[0,1,2],[3,4,5],[6,7,8]]
columns =[[0,3,6],[1,4,7],[2,5,8]]
diagonals=[[0,4,8],[2,4,6]]
count=0

def display_Grid():
    print("\n")
    print(grid[0][0], "|" , grid[0][1], "|" , grid[0][2])
    print("---------")
    print(grid[1][0], "|", grid[1][1], "|", grid[1][2])
    print("---------")
    print(grid[2][0], "|", grid[2][1], "|", grid[2][2])

def check_turn():
    if count%2 == 0:
        player_1()
    else:
        player_2()

def game_over(shape):
    try:
        sys.exit()

    except SystemExit:
        print(shape," won")
        win = True
        quit()
def player_1():
    p_number = ("Player one")
    p_shape = p1
    play_Game(p_number, p_shape)

def player_2():
    p_number = ("Player two")
    p_shape = p2
    play_Game(p_number, p_shape)

def checkDraw():
    try:
        if count==10:
            sys.exit()

    except SystemExit:
        print("Draw")
        quit()

def check_win(shape):
    directions=[rows,columns,diagonals]
    for i in directions:
        for j in i:
            status = map(lambda val: True if grid[int(val/3)][val%3] == shape else False, j)
            if all(status):
                return True

    return False

def play_Game(p_number, p_shape):
    global count
    checkDraw()
    print(p_number, ",enter where you would like to move on the grid: ")
    move = int(input(""))
    if move in (1, 2, 0):
        valid = is_move_valid(move)
        if valid:
            count+=1
            grid[0][move] = p_shape
            display_Grid()
        else:
            check_turn()
    elif move in (3, 4, 5):
        valid = is_move_valid(move)
        if valid:
            count+=1
            move2 = move-3
            grid[1][move2] = p_shape
            display_Grid()
        else:
            check_turn()
    elif move in (6, 7, 8):
        valid = is_move_valid(move)
        if valid:
            count+=1
            move2 = move-6
            grid[2][move2] = p_shape
            display_Grid()
        else:
            check_turn()

    else:
        print("Enter one of the numbers visible on the grid: ")

    if check_win(p_shape):
        game_over(p_shape)
    else:
        None
    if count%2 == 0:
        player_1()
    else:
        player_2()

def is_move_valid(move):
    print(move)
    if move in(0, 1, 2):
        if grid[0][move] in ("X", "O"):
            print("This place has already been picked...")
            return False
        else:
            None
    elif move in(3, 4, 5):
        move2 = move-3
        if grid[1][move2] in ("X", "O"):
            print("This place has already been picked...")
            return False
        else:
              None
    elif move in(6, 7, 8):
        move2 = move-6
        if grid[2][move2] in ("X", "O"):
            print("This place has already been picked...")
            return False
        else:
              None

    return True

win = False
grid = [[0,1,2],[3,4,5],[6,7,8]]
p_number = (" ")
display_Grid()
choose = input("Player one, would you like to be 'X' or 'O'?: ")
while win != True:
    try:
         if choose == "X":
             p2 = "O"
             p1 = choose
             player_1()
         elif choose == "O":
             p1 = "O"
             p2 = "X"
             player_1()
         else:
             print("Enter x or o: ")
             choose = input("Player 1, would you like to be 'X' or 'O'?: ")
    except ValueError:
        print("Please enter X OR O")

Divide Logic and User Intereaction

Why should a function to decide if a move is valid print to the screen? What if you want to change the message? Will you look inside a logic function to change a user-interface message?

I like to keep as many as possible of my functions "pure" so that they only make a calculation and return a result and build a thin interaction skin over it.

Long story short, is_move_valid should not print anything.

Simplification

As far as simplification is concerned, you can just flatten the board, and the function becomes:

def is_move_valid(move):
    return flatten(board)[move] not in ("X", "O")