Upload a photo with a unique name using PHP I have written a script for basic uploading of a single image to a web server using PHP. This is an example that is intended for educational purposes in a basic PHP course. It does not take extensive consideration to security as it is not the focus.

Key points to focus on the review:

If the code comply with the PSR-1 and PSR-2 requirements
If the code can be more efficiently written.
Recommendations for presenting success/error messages. Errors are already in an array.

<?php
    // 1 MB = 1048576 bytes
    $max_file_size = 1048576;

    // Accepted file types
    $image_types = array('gif', 'jpg', 'png');

    // Folder for uploaded images
    $upload_dir = realpath(dirname(__FILE__)) . '/images/';

    // Error codes for file upload
    // See: http://www.php.net/manual/en/features.file-upload.errors.php
    $upload_errors = array(
        UPLOAD_ERR_OK         => 'No errors.',
        UPLOAD_ERR_INI_SIZE   => 'Larger than upload_max_filesize.',
        UPLOAD_ERR_FORM_SIZE  => 'Larger than form MAX_FILE_SIZE.',
        UPLOAD_ERR_PARTIAL    => 'Partial upload.',
        UPLOAD_ERR_NO_FILE    => 'No file.',
        UPLOAD_ERR_NO_TMP_DIR => 'No temporary directory.',
        UPLOAD_ERR_CANT_WRITE => 'Can not write to disk.',
        UPLOAD_ERR_EXTENSION  => 'File upload stopped by extension.'
    );

    // Checks form request method
    if ($_SERVER['REQUEST_METHOD'] == 'POST') {

        // Return if no file has been submitted
        if (!isset($_FILES['photo'])) {
            return;
        }

        // Checks if image has been submitted
        if (isset($_FILES['photo'])) {

            // Creates array for containing errors
            $errors     = array();

            // Checks for errors in upload
            if ($_FILES['photo']['error'] > 0) {
                $error_message = $_FILES['file_upload']['error'];
                $errors[] = $upload_errors[$error_message];
            }

            // Declares file information
            $file_tmp    = $_FILES['photo']['tmp_name'];
            $file_name   = $_FILES['photo']['name'];
            $file_size   = $_FILES['photo']['size'];
            $file_sha1   = sha1_file($file_tmp);
            $file_ext    = pathinfo($file_name, PATHINFO_EXTENSION);

            // Creates unique name for uploaded file
            $target_name = $file_sha1 . '.' . $file_ext;

            // Checks if file is an image
            if (!getimagesize ($file_tmp)) {
                $errors[] = 'Ensure you are uploading an image.';
            }

            // Checks if image is a valid type
            if (!in_array($file_ext, $image_whitelist)) {
                $errors[] = 'Not a valid image. (.jpg, .png or .gif).';
            }

            // Checks if image exceed file size
            if ($file_size > $max_file_size) {
                $errors[] = 'Exceeded file size limit (1 MB).';
            }

            // Checks if file already exists
            if (file_exists($upload_dir . $target_name)) {
                $errors[] = 'File already exists.';
            }

            // Checks if there are any errors
            if (empty($errors)) {
                // Checks if upload file can be moved
                if (move_uploaded_file($file_tmp, $upload_dir . $target_name)) {
                    $success = "File uploaded successfully.";
                }
                else {
                    $errors[] = 'Error moving file to directory.';
                }
            }
        }
    }
?>
<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <title>send.php</title>
  </head>
  <body>
    <?php if(!empty($success)) { echo $success; } ?>
    <form action="send.php" method="post" enctype="multipart/form-data">
      Choose photo: <input type="file" name="photo">
      <input type="submit" name="submit" value="Submit">
    </form>
  </body>
</html>

// Return if no file has been submitted
        if (!isset($_FILES['photo'])) {
            return;
        }

        // Checks if image has been submitted
        if (isset($_FILES['photo'])) {

The last line there is redundant.  The first if has already done the complementary check.  You don't need a second if.  At that point, you can just run the code, as it will always run.

                $error_message = $_FILES['file_upload']['error'];
                $errors[] = $upload_errors[$error_message];

The $errors array holds error messages.  The $error_message variable holds an error type or error code.  It should probably be named in accordance with that.  Of course, you don't actually need a variable at all, since you never use it again.  You could just say

                $errors[] = $upload_errors[$_FILES['file_upload']['error']];


Shader parameters in managed OpenGL I'm working on a managed OpenGL game engine for C#, and here's how I'm managing shader parameters (uniforms). Is it "alright"? Could it be better? I'm a bit unsure about using generics in this case.

public abstract class ShaderParameter
{
    public readonly string Name;
    internal readonly int Location;
    internal readonly Shader Parent;

    internal ShaderParameter(string name, int location, Shader parent)
    {
        Name = name;
        Location = location;
        Parent = parent;
    }
    //Ensures that the value is set on the shader,
    //this should be called before using the parent shader
    internal abstract void EnsureSet();
    //Gives a value to this shader paramerter
    public abstract void SetValue<T>(T value);
}

//A shader paramater of type float
public sealed class ShaderParamFloat : ShaderParameter
{
    float value = 0;
    bool valueChanged = false; //If the value has changed since the last time it was set on the shader

    internal ShaderParamFloat(string name, int location, Shader parent)
        : base(name, location, parent)
    { }
    internal override void EnsureSet()
    {
        if (valueChanged)
        {
            //Hands over a single value to OpenGL.
            GL.Uniform1(Location, value);
            valueChanged = false;
        }
    }
    //Gives a value to this shader parameter
    public override void SetValue<T>(T value)
    {
        if (typeof(T) != typeof(float))
            throw new ArgumentException("Value is of type: " + typeof(T).ToString() + ", expected: float", "value");

        this.value = (float)(object)value;
        valueChanged = true;
    }
}

The reason for using inheritance is because other ShaderParameter classes behave differently in EnsureSet(). For example, ShaderParamVector3 uses GL.Uniform3(...), and ShaderParamTexture needs to ensure that the texture is valid and set on the graphics card.

The biggest thing here is that you should make the ShaderParam class itself generic, not just its SetValue method.  This will get rid of your type check and double cast (any time you have a type check or a double cast, chances are you're using the type system incorrectly).  It will also get rid of the ShaderParamFloat class completely.

For example:

public class ShaderParam<T>
{
    private T value;

    ... (other code here)

    public void SetValue(T value)
    {
        this.value = value;
        ...
    }
}

You can now use this for floats by instantiating a ShaderParam<float>, plus it works just as well for other types.

Some other comments:

Don't write comments that just duplicate the code.  A variable named valueChanged doesn't need a comment saying "If the value has changed" - that much is obvious from the name.  More importantly in this code, the method named EnsureSet() should have a comment explaining its purpose and why it should be called, not just a comment that expands the name into a longer sentence.  I'm not an OpenGL user so I don't know what GL.Uniform1() does, but that code is surprising to me and a comment to explain why it's written that way would be useful.
I take it you have good reasons for making some things public and others internal.  To me, those choices seem pretty arbitrary.  If it works in your situation, consider making the class itself internal and all members public.
I don't personally like making fields public/internal, though given that they are readonly, I could live with it.  I might personally still change this to use properties and a readonly backing field.  That gives you the added benefit of being able to set breakpoints later if you ever need to debug.
Not relevant if you make the first change above, but why is ShaderParamFloat still abstract?
The private fields in ShaderParamFloat should be explicitly marked private.
There is no need to initialize value to 0 or valueChanged to false.  Those are their default values.
I would personally call the class ShaderParameter instead of ShaderParam.  Your abbreviation is pretty obvious in this case, but I tend to stay away from all but the most commonly used abbreviations.  In particular, if using the abbreviation improves the readability of the code, then it makes sense.  I don't think that applies here.

EDIT:

Based on your updated question, I would still recommend a solution like I mentioned above, where the base ShaderParameter is a generic class.  However, it does make sense for you to derive classes from that with different implementations of EnsureSet.  For example:

public abstract class ShaderParameter<T>
{
    private T value;

    public abstract void EnsureSet();

    public void SetValue(T value)
    {
        this.value = value;
    }
}

public class FloatShaderParameter : ShaderParameter<float>
{
    public override void EnsureSet()
    {
        ...
    }
}

And you can then derive similar classes for the other types of parameters.  Note that you only need a single implementation of SetValue in the base, and it works for all derived classes.


Collections of classes in VBA I have noticed this question https://stackoverflow.com/questions/49389392/nested-classes-and-collections-vba in StackOverflow, but it got closed faster than I could have answered. However, it was concerning a best practice for usage of collections of classes.

I usually use Array and I add to it like this:

Public Sub AddToTeam(emp As Employee)

    Dim cnt As Long
    cnt = UBound(pTeam)

    If Not pTeam(0) Is Nothing Then
        ReDim Preserve pTeam(cnt + 1)
        cnt = cnt + 1
    End If
    Set pTeam(cnt) = emp

End Sub

Furthermore, I initialize the Array like this in the class:

Private Sub Class_Initialize()
    ReDim pTeam(0)
End Sub

This is the working code, feel free to give any ideas:

Main Module

Option Explicit

Public Sub TestMe()

    Dim EmpA    As New Employee
    Dim EmpB    As New Employee
    Dim ManA    As New Employee
    Dim TeamA   As New Team

    ManA.Name = "John Doe Top Manager"
    EmpA.Name = "Peter"
    EmpB.Name = "George"

    Set EmpB.Manager = ManA
    TeamA.Name = "The best team!"

    TeamA.AddToTeam ManA
    TeamA.AddToTeam EmpA
    TeamA.AddToTeam EmpB

    TeamA.PrintNames
    Debug.Print TeamA.Name
    TeamA.PrintInfoForManagers

End Sub

Class Employee

Option Explicit

Private pName           As String
Private pManager        As Employee
Private pAge            As Long
Private pTeam           As String
Private pHasManager     As Boolean

Public Property Get HasManager() As Boolean
    HasManager = pHasManager
End Property

Public Property Let HasManager(Value As Boolean)
    pHasManager = Value
End Property

Public Property Get Manager() As Employee
    Set Manager = pManager
End Property

Public Property Set Manager(Value As Employee)
    Set pManager = Value
    HasManager = True
End Property

Public Property Get Name() As String
    Name = pName
End Property

Public Property Let Name(Value As String)
    pName = Value
End Property

Public Property Get Team() As Employee
    Team = pTeam
End Property

Public Property Let Team(Value As Employee)
    pTeam = Value
End Property

Class Team

Option Explicit

Private pTeam()         As Employee
Private pName           As String

Public Sub PrintInfoForManagers()

    Dim emp As Variant
    For Each emp In pTeam
        If emp.HasManager Then
            Debug.Print emp.Name & " is managed by " & emp.Manager.Name
        Else
            Debug.Print emp.Name & " has no manager."
        End If
    Next emp

End Sub

Public Sub PrintNames()

    Dim emp As Variant
    For Each emp In pTeam
        Debug.Print emp.Name
    Next emp

End Sub

Public Property Get Name() As String
    Name = pName
End Property

Public Property Let Name(Value As String)
    pName = Value
End Property

Public Sub AddToTeam(emp As Employee)

    Dim cnt As Long
    cnt = UBound(pTeam)

    If Not pTeam(0) Is Nothing Then
        ReDim Preserve pTeam(cnt + 1)
        cnt = cnt + 1
    End If
    Set pTeam(cnt) = emp

End Sub

Private Sub Class_Initialize()
    ReDim pTeam(0)
End Sub

So how would you review/improve it?

I don't see the need for the HasManager property. Either a Manager is assigned, or not. It wouldn't make sense for you to set it to True when there isn't one, or False when there is.

Then just change your Print function to check -

If Not emp.Manager Is Nothing Then
    Debug.Print emp.Name & " is managed by " & emp.Manager.Name
 Else
    Debug.Print emp.Name & " has no manager."

If it was easy to overload the class initialization for employee, I'd say make a name value obligatory, but it's really not a simple task in VBA.

These two properties:

Public Property Get Team() As Employee
    Team = pTeam
End Property

Public Property Let Team(Value As Employee)
    pTeam = Value
End Property

Are passing Objects - why not pass Strings given the team information is handled by a string.


Remove repetitive strings from a given sentence efficiently I recently gave a test on HackerRank:

  Given a string, return the most concise string that can be formed from it.

  For example:

string = 'watson  Represents|watson represents|Watson represents a first step into cognitive systems, a new era of computing.|first step into  Cognitive|Cognitive Systems; a new era|what does watson represent'

  The following string contains many duplicates like "Watson represents." We have to ignore extra spacing between characters or lower/upper case. "watson  Represents" and "watson represents" are the same thing.
  Semicolons and commas represent the same thing. For example, "Cognitive Systems; a new era" is present inside "Watson represents a first step into cognitive systems, a new era of computing."
  Your final string should not contain any duplicates. Ignore lowercase/uppercase or extra space if you have any.

My answer:

watson = 'watson  Represents|watson represents|Watson represents a first step into cognitive systems, a new era of computing.|first step into  Cognitive|Cognitive Systems; a new era|what does watson represent'

import re

watson = re.sub(r';', r',', watson)  #replace the semicolon with colon
watson = watson.strip().split('|')
removeWatson = list(watson)

for i in range(len(watson)):

    for j in range(len(watson)):

        if j == i:
            continue

        if " ".join(watson[i].strip().lower().split()) in " ".join(watson[j].strip().lower().split()):
            removeWatson[i] = ''

print "|".join(filter(None, removeWatson))

My answer is definitely inefficient and I am wondering if you can suggest an alternative way of solving this problem.

Most concise string:

  Watson represents a first step into cognitive systems, a new era of computing.|what does watson represent

Here's my shot at this puzzle.

Benchmark of original code, over 1,000,000 iterations (with print() commented out):

  real    1m8.486s
  user    0m0.000s
  sys     0m0.000s

Benchmark of my code, over 1,000,000 iterations (with print() commented out):

  real    0m16.226s
  user    0m0.000s
  sys     0m0.015s

Performance gain: 422.07%

It looks like you want to preserve the string order and uppercase/lowercase, so I took that into account. Otherwise this could be even faster/shorter.

The strategy here is to preprocess the strings, instead of going through strip(), lower(), and split() each time you need to do a comparison.
Additionally, sort the strings by length, so you only perform comparisons you need to perform. See comments for more details.

WATSON_RAW = 'watson  Represents|watson represents|Watson represents a first step into cognitive systems, a new era of computing.|first step into  Cognitive|Cognitive Systems; a new era|what does watson represent'

# 1. Preprocess the string.
#    Remove extraneous whitespace, replace semicolon with comma.
modified = ' '.join(WATSON_RAW.split())
modified = modified.replace(';', ',')

# 2. Split string by delimiter.
split = modified.split('|')

# 3. Generate string metadata, sorted by length
#    Keep track of the string contents, its index in the "split" variable, and its length.
#    Tuple has a speed advantage over list.
metadata = [(i, len(s), s.lower()) for i, s in enumerate(split)]
metadata = sorted(metadata, key=lambda x: x[1])

# 4. Uniqify the strings.
indexes_of_unique_strings = []

for index, _, string_lowercase1 in metadata:
    # Only search from current index on, skipping comparisions with earlier indexes.
    for _, _, string_lowercase2 in metadata[i:]:
        if string_lowercase1 in string_lowercase2 and string_lowercase1 != string_lowercase2:
            # This string is redundant.
            break
    else:
        # This string is unique.
        indexes_of_unique_strings.append(index)

# 5. Map indexes back to original, correctly capitalized strings, in original order.
indexes_of_unique_strings = sorted(indexes_of_unique_strings)
final = '|'.join(split[i] for i in indexes_of_unique_strings)

print(final)


Automation script for creating NuGet packages I've been creating some NuGet packages recenly and to automate the process I created the following script that:

reads the package id from the script name
reads the package version from the nuspec file
can rebuild the solution, create a package and upload it by specifying the jklparameters where:

j - 0/1 - disables/enables project rebuild
k - 0/1 - disables/enables package creation
l - 0/1 - disables/enables package upload to the server

Usage:

./MyPackage.ps1 110

This would create a package called MyPackage with rebuilding the solution but not uploading it to the server yet

param([String]$cmd)

$build = $cmd.Substring(0,1) -eq "1"
$pack = $cmd.Substring(1,1) -eq "1"
$push = $cmd.Substring(2,1) -eq "1"

$packageId=[System.IO.Path]::GetFileNameWithoutExtension($MyInvocation.MyCommand.Definition)

[xml]$nuspec = Get-Content $PSScriptRoot\$packageId.nuspec
$version = $nuspec.SelectSingleNode("//package/metadata/version").InnerText

if ($build)
{
    msbuild `
        /t:Rebuild `
        /nologo `
        /p:Configuration=Release `
        /p:TargetFrameworkVersion=v4.5.2 `
        /p:Platform="Any CPU" `
        /p:OutDir="$PSScriptRoot\lib\net452" `
        `"$PSScriptRoot\..\Foo.sln`"
}

if ($pack)
{
    nuget pack `
        $PSScriptRoot\$packageId.nuspec `
        -properties configuration=release `
        -outputdirectory C:\NuGet\packages\
}

if ($push)
{
    nuget push `
        C:\NuGet\packages\$packageId.$version.nupkg `
        -configfile $PSScriptRoot\NuGet.config
}

Parameters

You could achieve something similar by making real parameters with boolean values such that you can type MyPackage.ps1 1 1 0; feeding 3 positional, named, parameters at a cost of two presses of the space bar.

It's not that much slower to type and makes it a lot easier to understand the code (in my opinion).

In addition, you're easily able to add parameter help.

param(
    [Parameter(Mandatory = $true, Position = 1)]
    [Boolean]$Build,

    [Parameter(Mandatory = $true, Position = 2)]
    [Boolean]$Pack,

    [Parameter(Mandatory = $true, Position = 3)]
    [Boolean]$Push
)

Make $Build default to true perhaps and you have a "Control+Shift+B"-able script.

Ticks

I'd consider killing off those ` you have everywhere. A misplaced space after one would break those very easily.

I would generate arguments as an array and feed the call operator. For calls to internal PowerShell commands I would favour splatting for long parameter lists.

$packArgs = @(
    'pack'
    "$PSScriptRoot\$packageId.nuspec"
    "-properties", "configuration=release"
    "-outputdirectory", "C:\NuGet\packages\"
)
& nuget $packArgs

XML handler

Perhaps consider using Select-Xml as it can read directly from a file.

GetFileNameWithoutExtension

There's an alternative perhaps:

([System.IO.FileInfo]$pscommandpath).BaseName

Or, of course, the original but derived from $pscommandpath.

Alternative approaches

This is a simple build script, but had you looked at psake? https://github.com/psake/psake.


Best Algorithm for solving USACO: Photoshoot - Python I was trying to solve this problem with Python 3.8. I couldn't find a way other than to brute-force calculate all the possible permutations then check which permutation was valid. The program works fine, but as it's O(n!) time complexity (I think) it takes extraordinarily long when the input has a length of, say, 1000.
My code:
from itertools import permutations as permute

fin = open("photo.in", "r")
fout = open("photo.out", "w+")

useless, line = fin.readlines()

nums = [int(i) for i in line.split(" ")]

def main(nums):
    foo = [i+1 for i in range(len(nums)+1)]
    bar = permute(foo)

    for permutation in bar:
      boo = False
      for i in range(1, len(permutation)):
          if permutation[i]+permutation[i-1] != nums[i-1]:
              boo = False
              break
          else:
            boo = True

      if boo:
          return permutation

final = " ".join([str(i) for i in main(nums)])
fout.write(final)

fin.close()
fout.close()

I'm not sure how to optimize this code. Should I create my own permutation generator function and then check along the way instead of getting the whole thing at once? Or is there an algorithm that I'm not seeing?
A short problem description:
Given a list N of length n integers, find a permutation of the numbers 1 through n+1 in which every element of the permutation plus the next one is equal to the corresponding number of N.
Edit: The generator approach did not work.

Mathematical approach
Let us assume the following notation:
$$ \mathbb{B} = \sum_{i = 1}^{n - 1} b_i = b_1 + b_2 + \cdots + b_{n-1} $$
As mentioned earlier in comments, you have set of interdependent equations as follows:
$$
\begin{align}
b_1 &= a_1 + a_2 \\
b_2 &= a_2 + a_3 \\
b_3 &= a_3 + a_4 \\
\vdots \\
b_{n - 1} &= a_{n - 1} + a_n \\
\end{align}
$$
The above is \$ n - 1 \$ sets of equations, and the last equation you can derive from those is:
$$
\begin{align}
\sum_{i = 1}^{n - 1} b_i &= \sum_{i = 1}^{n} a_i + \sum_{j = 2}^{n - 1} a_j \\
\implies \mathbb{B} + (a_1 + a_n) &= 2 \times \left( \sum_{i = 1}^{n} a_i
\right) \\
\implies a_1 + a_n &= 2 \times \left( \dfrac{n (n + 1)}{2} \right) - \mathbb{B} \\
&= \left( n \cdot (n + 1) \right) - \mathbb{B} \tag{1}
\end{align}
$$

Now, let's see what happens if we try the summation with a slight variation:
$$
\begin{align}
\sum_{i = 1}^{n - 1} \left[ (-1)^{i + 1} \cdot b_i \right] &= b_1 - b_2 + b_3 - \cdots \cdots (-1)^{n - 2} b_{n - 1} \\
&= (a_1 + a_2) - (a_2 + a_3) + (a_3 + a_4) - \cdots \cdots + (-1)^n (a_{n - 1} + a_n) \\
&= a_1 + (-1)^{n} a_n \tag{2}
\end{align}
$$
Now, there would be 2 cases; 1 is fairly straight to solve:
Case 1: \$ n \$ is odd
The equation 2 becomes: $$ a_1 - a_n = \sum_{i = 1}^{n - 1} \left[ (-1)^{i + 1} \cdot b_i \right] $$
giving you the direct result (adding equation to above):
$$
a_1 = \dfrac{1}{2} \left( \sum_{i = 1}^{n - 1} \left[ (-1)^{i + 1} \cdot b_i \right] + n^2 + n - \mathbb{B} \right)
$$
Case 2: \$ n \$ is even
This gives you the same result as equation 1, therefore the replacement operation from SylvainD's answer comes into play. I'll leave this for the programming specific solution.

Program/code review

Use the names of variables which are closer to words used in problem statement. This makes it easier to go through code and question at the same time.

The photo.in serves no purpose once it has been read, use a contextual open for this:
with open("photo.in", "r") as file_in:
    num_cows = int(file_in.readline())
    summations = map(int, file_in.readline().split(" "))

Same as above, the photo.out serves no purpose until you want to write:
with open("photo.out", "w+") as file_out:
    file_out.write(" ".join(output_string))


Finding data on XML using Python's LXML Using Python's LXML I must read an XML file and print from each "basic" and "expert" tag, the name and email text from it. I've done a script that works but I don't think is the best way of doing this. Is there a better (simpler) way for getting the data of the XML without having to make 2 iterations on it?

Python so far:

from lxml import etree

myXML = "data.xml"
tree = etree.parse(myXML)
root = tree.getroot()
for node in root:
    if node.tag == "basic" or node.tag == "expert":
        user = [None] * 4
        for i, child in enumerate(node):
            if child.tag == "name":
                user[0] = i
                user[1] = child.text
            if child.tag == "email":
                user[2] = i
                user[3] = child.text
        print user
        if user[3].startswith('_'):
            # do some other things with data if email begins with _ ...

Will print:

[0, 'f.bar', 1, 'foobar@me.com']
[0, 'm.bob', 3, 'm.bob@email.com']
[0, 'm.bab', 3, 'm.bab@email.com']

XML sample:

<?xml version="1.0"?>
<users>
    <id>11111</id>
    <checked>True</checked>
    <version>A12</mode>
    <basic>
        <name>f.bar</name>
        <email>foobar@me.com</email>
        <forename>Foo</forename>
        <surname>Bar</surname>
    </basic>
    <expert>
        <name>m.bob</name>
        <forename>Mak</forename>
        <surname>Bob</surname>
        <email>m.bob@email.com</password>
    </expert>
    <expert>
        <name>m.bab</name>
        <forename>Mak</forename>
        <surname>Bab</surname>
        <email>m.bab@email.com</password>
    </expert>
    <guru>
        <name>e.guru</name>
        <forename>Nick</forename>
        <email>nick@email.com</password>
        <surname>Gru</surname>
    </guru>
</users>

Currently, you are overlooking one of the advantages in using lxml with its fully compilant W3C XPath 1.0 (even XSLT 1.0) language modules.

Right now, your code really follows the syntax of Python's built-in etree, without any xpath() calls that can run dynamic parsing off node names.

Below iterates through all <basic> and <expert> tags and retrieves their child <name> and <email> all in one loop or list comprehension. And to retrieve their position we count their preceding siblings with count(preceding-sibling::*).

from lxml import etree

myXML = "data.xml"
tree = etree.parse(myXML)

user = []

# FOR LOOP
for i in tree.xpath("//*[name()='basic' or name()='expert']"):
    user.append([i.xpath("count(name/preceding-sibling::*)"),
                 i.find("name").text,
                 i.xpath("count(email/preceding-sibling::*)"),
                 i.find("email").text])
print(user)
# [[0.0, 'f.bar', 1.0, 'foobar@me.com'],
#  [0.0, 'm.bob', 3.0, 'm.bob@email.com'],
#  [0.0, 'm.bab', 3.0, 'm.bab@email.com']]

# LIST COMPREHENSION
user = [[i.xpath("count(name/preceding-sibling::*)"),
         i.find("name").text,
         i.xpath("count(email/preceding-sibling::*)"),
         i.find("email").text]
        for i in tree.xpath("//*[name()='basic' or name()='expert']")]

print(user)
# [[0.0, 'f.bar', 1.0, 'foobar@me.com'],
#  [0.0, 'm.bob', 3.0, 'm.bob@email.com'],
#  [0.0, 'm.bab', 3.0, 'm.bab@email.com']]


Read comma separated values and insert in SQL Server 2016 table using String_Split I know that this question has been asked many times over many years and I have read every one of them but I wanted to get a modern solution since software has changed and String_Split has been introduced in SQL Server 2016.

I have a stored procedure that splits a comma-separated string and inserts the values into a table after it deletes the value if it is already there. The code works but sometimes I write code that is totally convoluted.

ALTER PROCEDURE [dbo].[Admin_Save_PMIDS]
    @MemberID INT,
    @PMIDList VARCHAR(MAX)
AS
BEGIN
    SELECT Split.a.value('.', 'VARCHAR(100)') AS PMID
    INTO #pmids
    FROM
        (SELECT
             CAST('<M>' + REPLACE(@PMIDList, ',', '</M><M>') + '</M>' AS XML) AS String) AS A
    CROSS APPLY
        String.nodes('/M') AS Split (a)

    DELETE FROM [dbo].[PublicationTable]
    WHERE (MemberID = @MemberID)
     AND  PMID IN (SELECT PMID FROM #pmids)

    INSERT INTO [dbo].[PublicationTable] (PersonID, PMID, MemberID)
        SELECT
            @MemberID, PMID, @MemberID
        FROM
            [#pmids]
        WHERE
            (LEN(PMID) > 0)

    DROP TABLE #pmids
END

As you can see I'm creating a temporary table and using an XML structure as a cross apply. The code works but since I'm doing a lot of updating in the application I figured I'd get some current opinions.

The following are a few suggestions on how I'd write the stored procedure.

Source Control

If you don't already have a database project, create one in Visual Studio. Then check it in to source control. Microsoft Azure DevOps Services is free & private for teams of 5 or less (this is per project, so 5 developers per project). Then you'll be able to track changes you make to your stored procedures, views, tables, etc.

Error Handling

Use Try, Catch blocks to handle your errors. You can then write them to a log file or table.

Tranactions

Use transactions for rolling back your queries during testing. If a transaction is successful, all of the data modifications made during the transaction are committed and become a permanent part of the database. If a transaction encounters errors and must be cancelled or rolled back, then all of the data modifications are erased.

Common Table Expressions (CTE)

CTE's in your SQL help with documentation. The expression name can then let other developers know why you used that expression e.g. current_suppliers or active_projects.

Merge

If I need to perform more than one DML query on a table, I usually will use a MERGE statement.

Formatting

I would download the following tool for SSMS and Visual Studio, Poor Man's T-Sql Formatter and on GitHub. I use it when I have to edit other developer's code. It's a great way to standardize your SQL. I find it does most of the formatting for me, but I'll still make a few changes after.

Here are the settings I used:

Commas

I would put the commas in front to clearly define new columns. Versus code wrapped in multiple lines. It also makes trouble-shooting code easier.

e.g. , S.[PersonID]

Estimated Execution Plan

It's a good idea to check the Estimated Execution Plan. The shortcut in Microsoft SQL Server Management Studio (SSMS) is Ctrl + L. You can even run 2 queries in the same tab to compare the plans. Then you can see differences in the query plans using STRING_SPLIT.

Revised SQL

Without table definitions and sample records I was unable to test this, but it should give you a good start.

ALTER PROCEDURE [dbo].[Admin_Save_PMIDS]
      @MemberID INT
    , @PMIDList VARCHAR(MAX)
AS
BEGIN

    BEGIN TRY

        SET XACT_ABORT ON;
        BEGIN TRANSACTION;

            WITH
            pm_ids
            AS
            (
                SELECT
                      [PMID] = Split.a.value('.', 'VARCHAR(100)')
                    , [PersonID] = @MemberID
                    , [MemberID] = @MemberID
                FROM
                    (
                    SELECT CAST('<M>' + REPLACE(@PMIDList, ',', '</M><M>') + '</M>' AS XML) AS String
                    ) AS A
                CROSS APPLY String.nodes('/M') AS Split(a)
            )
            MERGE [dbo].[PublicationTable] AS T
            USING pm_ids AS S
            ON T.[PMID] = S.[PMID] AND T.[MemberID] = S.[MemberID] --< Update the join to the correct unique key for the table
            WHEN NOT MATCHED BY TARGET
            THEN INSERT
            (
                  [PMID]
                , [PersonID]
                , [MemberID]
            )
            VALUES
            (
                  S.[PMID]
                , S.[PersonID]
                , S.[MemberID]
            )
            WHEN NOT MATCHED BY SOURCE --< You may want to add a WHERE clause here
            THEN DELETE
            --OUTPUT @@SERVERNAME AS [ServerName], DB_NAME() AS [DatabaseName], $action, inserted.*, deleted.* --< show the changed records here
            ;

        ROLLBACK TRANSACTION; --< use the rollback for testing
        --COMMIT TRANSACTION; --< use the commit for production

    END TRY
    BEGIN CATCH
        --you can write this to a log table
        SELECT
              [ServerName] = @@SERVERNAME
            , [DatabaseName] = DB_NAME()
            , [ErrorProcedure] = ERROR_PROCEDURE()
            , [ErrorNumber] = ERROR_NUMBER()
            , [ErrorSeverity] = ERROR_SEVERITY()
            , [ErrorState] = ERROR_STATE()
            , [ErrorLine] = ERROR_LINE()
            , [ErrorMessage] = ERROR_MESSAGE()
            , [ErrorDateTime] = GETDATE();

        -- Test whether the transaction is uncommittable.
        IF (XACT_STATE()) = -1
        BEGIN
            PRINT
                N'The transaction is in an uncommittable state.' + 'Rolling back transaction.';
            ROLLBACK TRANSACTION;
        END;

        -- Test whether the transaction is committable.
        IF (XACT_STATE()) = 1
        BEGIN
            PRINT
                N'The transaction is committable.' + 'Committing transaction.';
            COMMIT TRANSACTION;
        END;

    END CATCH

END


Compile-time Lagrange polynomials in C++ I have implemented Lagrange polynomials, i.e.

\$l_j(x) := \prod_{\begin{smallmatrix}0\le m\le k\\ m\neq j\end{smallmatrix}}\frac{x-x_m}{x_j-x_m} = \frac{(x-x_0)}{(x_j-x_0)} \cdots \frac{(x-x_{j-1})}{(x_j-x_{j-1})} \frac{(x-x_{j+1})}{(x_j-x_{j+1})} \cdots \frac{(x-x_k)}{(x_j-x_k)}\$

where the product is supposed to be expanded at compile time. The \$x_m\$ are chosen to be evenly spaced on the interval \$[-1, 1]\$.

I've used boost fusion for this, mainly because it seemed (and turned out to be) helpful, but I'm not entirely sure if it's the best choice (out of the multiple TMP libraries in boost and elsewhere).

Generally, I'm new to such an extensive use of types and templates, so general advice in this regard is appreciated. And now, without further ado:

#include <iostream>

#include <boost/fusion/sequence.hpp>
#include <boost/fusion/include/accumulate.hpp>

namespace fusion = boost::fusion;

template <int Order, int NodeId>
struct Location
{
    static constexpr double coord = 2.0 * NodeId / Order - 1.0;
};

template <class BaseNode, class OtherNode>
struct Term
{
    static double F(double x)
    {
        return (x - OtherNode::coord) / (BaseNode::coord - OtherNode::coord);
    }
};

template <int Order, int BaseNodeId, int NodeCounter = Order>
struct RemainingNodes
{
    using tail = typename RemainingNodes<Order, BaseNodeId, NodeCounter - 1>::list;
    using currentNode = Location<Order, NodeCounter>;
    using full =
        typename fusion::result_of::as_vector<typename fusion::result_of::push_back<tail, currentNode>::type>::type;
    using list = typename std::conditional<BaseNodeId == NodeCounter, tail, full>::type;
};

template <int Order, int BaseNodeId>
struct RemainingNodes<Order, BaseNodeId, 0>
{
    using list = typename boost::fusion::vector<Location<Order, 0>>;
};

template <int Order>
struct RemainingNodes<Order, 0, 0>
{
    using list = typename boost::fusion::vector<>;
};

template <class BaseNode>
struct GetTerm
{
    template <class OtherNode>
    Term<BaseNode, OtherNode> operator()(OtherNode);
};

template <int Order, int BaseNodeId>
struct Terms
{
    using Nodes = typename RemainingNodes<Order, BaseNodeId>::list;
    using BaseNode = Location<Order, BaseNodeId>;
    using transformed_list = typename fusion::result_of::transform<Nodes, GetTerm<BaseNode>>::type;
    using value = typename fusion::result_of::as_vector<transformed_list>::type;
};

struct evaluate
{
    double _x;

    evaluate(double x)
        : _x(x)
    {
    }

    template <class _Term>
    double operator()(double product, const _Term&) const
    {
        return product * _Term::F(_x);
    }
};

template <int Order, int BaseNodeId>
struct LagrangePolynomial
{
    using _Terms = Terms<Order, BaseNodeId>;

    static double F(double x)
    {
        return boost::fusion::accumulate(typename _Terms::value(), 1.0, evaluate(x));
    }
};

int main()
{
    using secondOrder0 = LagrangePolynomial<2, 0>;

    std::cout << "2nd order Lagrange polynomial of 1st node, evaluated at -1, 0, 0.5, 1: ";
    std::cout << secondOrder0::F(-1.0) << " " << secondOrder0::F(0.0) << " " << secondOrder0::F(0.5) << " "
              << secondOrder0::F(1.0) << std::endl;

    return 0;
}

struct evaluate ⋯

Compile-time computation is much easier today using constexpr!  Rather than going for a pre-2014 TMP library, start by writing normal code and see if there is any reason why it can’t simply be declared as constexpr.  Since it is a pure math function, it really should be.

I really think you can write this with no TMP whatsoever.  Edit: Thanks papagaga for doing so!


Interface for server requests and responses I have an MVC framework I've written. I'm trying to abstract out ASP.NET specific bits as well as make it more testable. Previously, I relied on HttpContext.Current in many places, which proves to be very unfriendly to unit testing. So, I started designing my own minimalistic interface for everything I'd need from there. However, it seems to keep on growing.

/// <summary>
/// This is a very simplified context for server requests and responses
/// It should not cover "everything", but should instead have only what BarelyMVC needs to function
/// As a result, it will usually only have the most commonly used things in it
/// All "get" methods should normally return null if a given name/key doesn't exist
/// </summary>
public interface IServerContext
{
    /// <summary>
    /// Gets a cookie from the current request
    /// </summary>
    HttpCookie GetCookie(string name);
    /// <summary>
    /// Sets a cookie to send with the current response
    /// </summary>
    void SetCookie(HttpCookie cookie);
    /// <summary>
    /// Kills the current request. Kills it with fire! This function should never return(for security purposes)
    /// </summary>
    void KillIt();
    /// <summary>
    /// Gets the user agent sent by the current request
    /// </summary>
    string UserAgent{get;}
    /// <summary>
    /// Maps a relative URL path to an absolute path on the host (is this needed?)
    /// </summary>
    string MapPath(string path);
    /// <summary>
    /// Gets the client's IP address
    /// </summary>
    string UserIP{get;}
    /// <summary>
    /// Gets an HTTP header by name from the current request
    /// </summary>
    string GetHeader(string name);
    /// <summary>
    /// Sets an HTTP header to be sent back in the response to the current request
    /// </summary>
    void SetHeader(string name, string value);
    /// <summary>
    /// Performs a server-side transfer to another URL to service the current request (is this needed?)
    /// Should perform a KillIt() afterwards and never return
    /// </summary>
    void Transfer(string url);
    /// <summary>
    /// Send a 302 redirect back to the client for the given URL and use KillIt() to end the request
    /// </summary>
    void Redirect(string url);
    /// <summary>
    /// Gets a generic item from a key/value store associated with the current request only
    /// </summary>
    object GetItem(string name);
    /// <summary>
    /// Sets a generic item to a key/value store associated with the current request only
    /// </summary>
    object SetItem(string name, object value);
    /// <summary>
    /// Gets or sets the HTTP status code
    /// </summary>
    string HttpStatus{get;set;}
    /// <summary>
    /// Gets a TextWriter which can be used to send content back as the response
    /// </summary>
    TextWriter Writer{get;}
    /// <summary>
    /// Gets a parameter dictionary corresponding to the FORM values passed in by a POST request
    /// </summary>
    ParameterDictionary Form{get;}
    /// <summary>
    /// Gets the URL of the current request
    /// </summary>
    Uri RequestUrl{get;}
}

Is this interface already too big? I know I could abstract out the Request and Response parts, but I tend to think it make it a bit harder to use, especially because of my limits on "get" and "set" being stuck to request and response respectively.

What do you think? Refactoring into Request/Response interfaces, or is it OK how it is?

You have to keep in mind that the earlier version of the .NET framework suffered in applying the interface segregation principle. In this case, the HTTP context includes methods associated with the request (ex. URL), the response (ex. HTTP status) and server utility functions (ex. map path).

With that in mind, the interface looks OK and familiar in terms of usage in comparison to the original HttpContext (System.Web) and HttpContextBase (System.Web.Mvc) and should be fine.

One aspect in regards to MVC that is hard to manage is the multiple data sources (query parameters, URL parameters, HTTP post Form values, routing parameters, session and the per request key/value store (HttpContext.Current.Items)) - perhaps it would be worthwhile to create an intermediary to manage these.


Game where two players take turns picking numbers until a target sum (part 2) Here is a continued discussion (Game where two players take turns picking numbers until a target sum) from here since my previous code has bug, I fixed it with totally new algorithm implementation.

Working on the problem of sum game. Suppose we have consecutive numbers from 1 to n, and an expected sum number, there are two players, and each player tries to select one number in turn (once a number is selected by one player, the other player cannot select it again), and the selected numbers are summed together. The player who first get a sum which is >= expected sum wins the game. The question is trying to find if first player has a force win solution (means no matter what numbers 2nd player will choose each time, the first player will always win), and also try to find all possible force win solutions of player 1.

For example, if we have numbers from 1 (inclusive) to 5 (inclusive) and expected sum is 7, if the first player select 1, no matter what the 2nd player select, in the final the first player will always win.

Areas which I want to improve, but do not know how in my below code, asking for advice,

I think the use of first_player_initial_choice is a bit ugly, but I do not know how to remove it;
I check len(path) == 1 to see if it is first choice by player 1, so that I can record when player 1 force win, what is the first selected number -- which could be used for further filter purpose, it seems a bit ugly as well;
I tried to find all possible winning solution of player 1, when try to filter (by using if result[0] in first_player_initial_choice to see if winning soluton of player 1 is actually a player 1 force win solution.) It seems a bit ugly as well, but I cannot figure out a way which pick-up only player 1 force win solution only in an efficient recursive solution way.

My code,

results = []
first_player_initial_choice = set()
def force_win(numbers, flags, current_value, expected_value, path):
    current_win = False
    for i in range(len(flags)):
        if not flags[i]:
            if numbers[i] + current_value >= expected_value:
                path.append(numbers[i])
                if len(path) % 2 != 0:
                    results.append(path[:])
                if len(path) == 1:
                    first_player_initial_choice.add(numbers[i])
                path.pop(-1)
                current_win = True
            else:
                flags[i] = True
                path.append(numbers[i])
                opponent = force_win(numbers, flags, current_value+numbers[i], expected_value, path)
                if len(path) == 1 and not opponent:
                    first_player_initial_choice.add(numbers[i])
                path.pop(-1)
                flags[i] = False
                if not opponent:
                    current_win = True

    # for
    return current_win

if __name__ == "__main__":
    win = force_win([1,2,3,4,5],[False]*5, 0, 8, [])
    print win
    if win:
        final_result = []
        print results
        print first_player_initial_choice
        for result in results:
            if result[0] in first_player_initial_choice:
                final_result.append(result)

        print final_result

About the algorithm

If you want to check for a force win on the first turn, you just need to check what happens when player one picks the lowest number and player two picks the remaining highest number.
If you want to check for a force win on the other turns you can repeat the check by removing numbers from the list and check the remaining highest and lowest numbers.

About the code

It took me a while to get how your code worked, I think it's overly complicated. You have flags to determine if a number has been picked, but if you really want to keep a copy of the original numbers (which you don't actually need) you can just, well, copy it.
I still don't really get the path variable. You use that to determine whose turn it is, but you don't need to know that, you only need to know if player one has a forced win.

This is how I'd go about it for a solution. Keep in mind that I didn't test every corner case, but it should give you an idea about what I mean:

def force_win(numbers, current_sum, expected_value):
    # Numbers are assumed to be positive integers
    # For simplicity I assume there are no duplicates
    # Otherwise be careful if min_value == max_value
    min_value = min(numbers)
    max_value = max(numbers)
    current_sum += min_value + max_value

    if (current_sum >= expected_value):
        return False

    numbers.remove(min_value)
    if (max_value in numbers):
        numbers.remove(max_value)
    if (len(numbers) == 0):
        return False

    if (min_value + max_value < expected_value):
        if ((current_sum + min_value) >= expected_value):
            return True
    force_win(numbers, current_sum, expected_value)

    return False

if __name__ == "__main__":
    print force_win([1,2,3,4,5], 0, 7)


Customized class crawler to harvest names and phone numbers from a webpage I've written some code using python to scrape names and phone numbers from a webpage which is working nice at this moment.

However, the way I've written my class crawler is unconventional. As I've hardly seen any crawler which have been written the way I did here, I'm little dubious as to whether this style I should keep up. It serves the purpose, though! Expecting reviews to do the betterment of this crawler. Thanks in advance.

Here is the script:

import requests

from lxml import html

class YelPage:

    base_url = "https://www.yellowpages.com/search?search_terms=pizza&geo_location_terms=Los+Angeles%2C+CA"
    url = "https://www.yellowpages.com"

    def __init__(self):

        self.vault = []

    def parse(self):

        self.get_urls(self.base_url)
        self.process_links(self.base_url)

    # Going to the main page for collecting links leading to the target page
    def get_urls(self, link):

        response = requests.get(link)
        tree = html.fromstring(response.text)

        for items in tree.xpath("//div[@class='info']"):
            item_link = items.xpath(".//a[@class='business-name'][not(@itemprop='name')]/@href")
            for page in item_link:
                self.target_page(self.url + page)

    # Creating links leading to the next page
    def process_links(self, link):

        response = requests.get(link)
        tree = html.fromstring(response.text)
        next_page = tree.xpath("//div[@class='pagination']//li/a/@href")

        for n_page in next_page:
            self.get_urls(self.url + n_page)

    # Scraping required data reaching this target page
    def target_page(self, urls):

        print(urls)
        response = requests.get(urls)
        tree = html.fromstring(response.text)

        for posts in tree.xpath("//*[@id='main-header']"):
            name = posts.xpath(".//div[@class='sales-info']/h1/text()")[0] if posts.xpath(".//div[@class='sales-info']/h1/text()") else ""
            phone = posts.xpath('.//p[@class="phone"]/text()')[0] if posts.xpath('.//p[@class="phone"]/text()') else ""
            records = name, phone
            if records not in self.vault:
                self.vault.append(records)

    def __str__(self):

        return self.vault

if __name__ == '__main__':

    crawler = YelPage()
    crawler.parse()
    for item in crawler.vault:
        print(item)

In some sense, you are reinventing what Scrapy web-scraping framework provides from out-of-the-box. Take a look how Scrapy spiders are written and you may find something similar, but better organized with less boilerplate code. Since, you've been doing quite a lot of web-scraping, I would go ahead and give Scrapy a try here.

Some notes about the code:

naming. Remember: code is much often read than written. YelPage can be replaced with a more explicit YellowPageScraper; next_page actually means next_pages since you locate multiple of them - etc. (please refer to the code below for the other improved variable names)
as I've mentioned before, you can reuse a session by having a session instance as your instance variable
comments before each of the methods should be transformed to proper method documentation strings

Improved code:

import requests
from lxml import html

class YellowPageScraper:
    base_url = "https://www.yellowpages.com/search?search_terms=pizza&geo_location_terms=Los+Angeles%2C+CA"
    url = "https://www.yellowpages.com"

    def __init__(self):
        self.vault = []
        self.session = requests.Session()

    def crawl(self):
        self.get_urls(self.base_url)
        self.process_links(self.base_url)

    def get_urls(self, link):
        """Going to the main page for collecting links leading to the target page."""
        response = self.session.get(link)
        tree = html.fromstring(response.text)

        for item in tree.xpath("//div[@class='info']"):
            item_links = item.xpath(".//a[@class='business-name'][not(@itemprop='name')]/@href")
            for page in item_links:
                self.target_page(self.url + page)

    def process_links(self, link):
        """Creating links leading to the next page."""
        response = self.session.get(link)
        tree = html.fromstring(response.text)

        next_urls = tree.xpath("//div[@class='pagination']//li/a/@href")
        for next_url in next_urls:
            self.get_urls(self.url + next_url)

    def target_page(self, urls):
        """Scraping required data reaching this target page."""
        response = self.session.get(urls)
        tree = html.fromstring(response.text)

        for post in tree.xpath("//*[@id='main-header']"):
            name = post.findtext(".//div[@class='sales-info']/h1")
            phone = post.findtext('.//p[@class="phone"]')

            records = name, phone
            if records not in self.vault:
                self.vault.append(records)

    def __str__(self):
        return self.vault

if __name__ == '__main__':
    crawler = YellowPageScraper()
    crawler.crawl()
    for item in crawler.vault:
        print(item)


Add query string to a URL I have written this code which adds a querystring to a URL:

exports.addQueryString = function(url, queryString) {
  var isQuestionMarkPresent = url && url.indexOf('?') !== -1,
    separator = '';

  if (queryString) {
    separator = isQuestionMarkPresent ? '&' : '?';
    url += separator + queryString;
  }

  return url;
};

Is there any way I can write this better?

Usage:

addQueryString('http://example.com', 'q=1&val=3&user=me');

I think that is ok, only that i see not necesary make url.indexOf('?') if queryString is empty, because you are not going to use it.

exports.addQueryString = function(url, queryString) {
  if (queryString) {
    var isQuestionMarkPresent = url && url.indexOf('?') !== -1,
      separator = isQuestionMarkPresent ? '&' : '?';
    url += separator + queryString;
  }

  return url;
};


Defining application flow in ASP.NET MVC5 Currently I am assigned to a project which has a standard view-flow, yet this flow is determined by whether settings are set to either true or false.

So, for example, let the standard flow be View1 > View2 > View3. When the customer has no need for View2, the application should be adjusting this to View1 > View3.

It is quite important to note that these Views do not require any information from eachother and are in no way bound to eachother.

I've thought of some ways to implement this, but I can't really decide which one to use and which one is better or whether there are better options. Consider this Settings object;

public class Settings
{
    public bool HasView1 { get; set; }
    public bool HasView2 { get; set; }
    public bool HasView3 { get; set; }
}

Implementation 1: Controller controls

This is the first implementation I came up with;

public class ViewController : Controller
{
    public ActionResult View1()
    {
        if(!Settings.HasView1)
            return View2();
        else
            return View("View1");
    }

    public ActionResult View2()
    {
        if(!Settings.HasView2)
            return View3();
        else
            return View("View2");
    }

    public ActionResult View3()
    {
        if(!Settings.HasView3)
            return View4();
        else
            return View("View3");
    }

    //etc.
}

Which works fine as-is. However, if I were to implement another ViewN, I would also have to update ViewN-1, which seems inconsistent.

Implementation 2: Some other object which determines the flow based on settings.

Another implementation I came up with is some kind of ViewManager which returnes a View based on the current View and the settings provided. Something like this;

public class ViewManager
{
    public string GetNextView(string CurrentViewName)
    {
        //Logic to determine the next View
        //I probably have to bind the View names to the settings somehow
        //to achieve this
    }
}

An implementation in the Controller would be as follows, considering View1 is part of the flow;

public class ViewController : Controller
{
    [HttpPost]
    public ActionResult View1(string currentViewName)
    {
        //handle View Post-logic
        var vm = new ViewManager();
        return View(vm.GetNextView(currentViewName));
    }
}

The second implementation seems to be the better one, yet the first one seems easier to implement.

Any suggestions on why you would choose one over another, or perhaps any other ideas on how to implement this?

First off I think its a mistake to call your controller Methods after the view they return. I think this is creating a conceptual link which doesn't really exist.

Secondly, presumably the actions which the user takes at the end of each view are either the same regardless of the next view, or depend on what the next view is. I could have step 1: submit customer address -> 2: submit customer phone OR 2: submit customer name -- but the action from 1 is always 'submit address'

OR

you might have 1: request Phone Detail Form / 1: request name form

If it's the former then the submit address action should contain the workflow logic, and there is no need to decorate the view model, read the settings from config.

If the the later then the view model should include options for the different buttons which may or may not be enabled

If you do put the logic in the action, then yes, I think splitting it into a different workflow class is probably a good idea, even though it's probably going to be tightly coupled, it will give you the option to change it later and make it easy to see where changes just affect the workflow rather than the individual actions.


Bash script to simplify Git deployment workflow In my company I follow the following workflow. (I think it can be defined a kind of 'Continuous Integration'.)

Workflow:

We have 3 fixed branches (staging/master/production)
Every push to staging or production automatically deploy to staging/production server.

We use git in the following way:
(suppose I'm working on the new functionality 'A')

I create a new branch from Master (ex. branch 'A')
If I want to show my changes to the client I merge A to staging and push it.
once finish my work on A I merge back A on Master and push master
to send changes live I merge Master in Production branch and push production

I find quite boring/time consuming to git checkout ... git pull .. bla bla every time I have to deploy live/staging my work.

So I have produced the following bash script, the aim is to simplify the process of deploying branch A into a single command deploy.sh live A

#!/bin/bash
function merge() {
    SOURCE=$1
    DEST=$2

    echo ''
    echo "--- Merging $SOURCE with $DEST ---"
    echo "--> Checkout $DEST ..."
    git checkout $DEST

    echo "--> Pull $DEST ..."
    git pull --ff-only origin $DEST
    if [ $? -ne 0 ]
    then
      echo "ERROR: Could not PULL"
     return 1
    fi

    echo "--> Merging $SOURCE with $DEST ..."
    git merge --ff-only $SOURCE --no-edit  # --ff-only trigger errors if merge/pull is not possible
    if [ $? -ne 0 ]
    then
      echo "ERROR: Could not MERGE"
      return 1
    fi

    echo "--> Push $DEST ..."
    git push origin $DEST
    return 0
}

function deploy() {
    MODE=$1
    SOURCE_BRANCH=$2

    echo ''
    echo "### START ###"
    echo ''
    echo "--- Pull changes from Master ---"
    git checkout $SOURCE_BRANCH
    git pull --progress --no-edit --no-stat -v --progress origin master

    merge $SOURCE_BRANCH 'staging'
    status=$?
    if [ $status -ne 0 ]
    then
      echo "ERROR: STEP 1" >&2
      exit 1
    fi

    if [ $MODE = "live" ]
    then

        merge $SOURCE_BRANCH 'master'
        status=$?
        if [ $status -ne 0 ]
        then
          echo "ERROR: STEP 2"
          return 1
        fi

        merge 'master' 'production'
        status=$?
        if [ $status -ne 0 ]
        then
          echo "ERROR: STEP 3"
          return 1
        fi
    fi

    echo ''
    echo "### END ###"
    echo ''
}

MODE=$1;
SOURCE_BRANCH=$2;

if [ -z "$MODE"  -o -z "$SOURCE_BRANCH" ]
    then
        echo "Usage:"
        echo ""
        echo  "MODE BRANCH_NAME (MODE: live|staging)"
    else
        if git show-ref --verify --quiet "refs/heads/$SOURCE_BRANCH";
        # if [ `git branch --list $SOURCE_BRANCH ` ]
            then
                deploy $MODE $SOURCE_BRANCH
        else
            echo ''
            echo "Error: Branch $SOURCE_BRANCH not found"
        fi
fi

The question:

I'm quite newby both on GIT and BASH scripting.
So I would like to know if the above workflow/script is ok ? Any recommendation is welcome.
(I'm happy to share this as resource.)

Use exit codes directly

Instead of this:

merge $SOURCE_BRANCH 'staging'
status=$?
if [ $status -ne 0 ]
then
  echo "ERROR: STEP 1" >&2
  exit 1
fi

You can use exit codes directly in conditions, like this:

if ! merge $SOURCE_BRANCH 'staging'
then
  echo "ERROR: STEP 1" >&2
  exit 1
fi

Use it this way everywhere.

Handle errors consistently

The error handling is inconsistent throughout the script:

Sometimes you exit 1, sometimes you return 1 in functions. It would be better to do consistently
Sometimes you print the error message on stdout, sometimes on stderr. It would be better to print it consistently on stderr

Style

To print a blank line you can simply echo without a parameter.

You can drop return 0 as the last statement of a function. The exit code of the last  statement will be used as the exit code of the function.

A semicolon is unnecessary at the end of the line.

Long comments at the end of a line are generally hard to read. Move these to the previous line.

The indentation is not consistent. The code inside the functions looks nice, do the same way for the code outside.

Ex. ( edited by @WonderLand)

#!/bin/bash

function merge() {
    SOURCE=$1
    DEST=$2

    echo
    echo "---> Merging $SOURCE with $DEST"

    echo
    echo "---> ---> Checkout $DEST ..."
    git checkout $DEST

    echo
    echo "---> ---> Pull $DEST ..."
    if ! git pull --ff-only origin $DEST
    then
     exit 1
    fi

    echo
    echo "---> ---> Merging $SOURCE with $DEST ..."
    # --ff-only trigger errors if merge/pull is not possible
    if ! git merge --ff-only $SOURCE --no-edit
    then
      exit 1
    fi

    echo
    echo "---> ---> Push $DEST ..."
    git push origin $DEST
}

function deploy() {
    MODE=$1
    SOURCE_BRANCH=$2

    echo
    echo "---> Pull changes from Master ..."

    if ! git checkout $SOURCE_BRANCH
    then
        exit 1
    fi

    git pull --progress --no-edit --no-stat -v --progress origin master

    if ! merge $SOURCE_BRANCH 'staging'
    then
      exit 1
    fi

    if [ $MODE = "live" ]
    then
        if ! merge $SOURCE_BRANCH 'master'
        then
          exit 1
        fi

        if ! merge 'master' 'production'
        then
          exit 1
        fi
    fi
}

MODE=$1
SOURCE_BRANCH=$2

if [ -z "$MODE"  -o -z "$SOURCE_BRANCH" ]
then
    echo "Usage:"
    echo ""
    echo  "MODE BRANCH_NAME (MODE: live|staging)"
else
    if git show-ref --verify --quiet "refs/heads/$SOURCE_BRANCH"
    then
        echo
        echo "### START ###"
        echo
        deploy $MODE $SOURCE_BRANCH
        echo
        echo "### END ###"
        echo
    else
        echo
        echo "Error: Branch $SOURCE_BRANCH not found"
    fi
fi


Find the minimum value that data could have had before it was rounded I have this function in R:

Minimum <- function(data) {
    answer <- numeric(length(data))

    diference <- c(0, diff(data, lag = 1, differences = 1)) #Padded initially =0

    answer[1]=data[1]
    for (i in 2:length(diference)) {
        if (diference[i]==0) {
            answer[i]=answer[i-1]
        } else {
            answer[i]=data[i]-diference[i]/2
        }
    }
    return(answer)
}

Its purpose is to find the minimum value which "data" could had before it was rounded.

The minimum possible value is the average of the values which "data" had at the last change of value in "data"

This code works, but since for loops are inefficient in R, it is advised to vectorize the function.

The problem is that the "answer" vector depends on the former values in "answer", so I cannot use a lambda function.

You can firstly change by difference != 0 and then use na.locf to replace NAs by last available value recursively.

minimum_new <- function(data) {
  answer <- rep(NA, length(data))
  difference <- c(0, diff(data, lag = 1, differences = 1)) / 2
  answer[1] <- data[1]
  answer[difference != 0] <- data[difference != 0] - difference[difference != 0]
  answer <- zoo::na.locf(answer, na.rm = FALSE)
  answer
}

This version is faster for me by at least 2 times.

> data <- sample(10, 10000, replace = TRUE)
> check <- function(values) all(sapply(values[-1], function(x) identical(values[[1]], x)))
> bench <- microbenchmark::microbenchmark(loop = Minimum(data), vectorised = minimum_new(data), check=check)
Unit: microseconds
       expr      min       lq     mean   median       uq      max neval cld
       loop 1401.959 1415.552 1665.816 1457.274 1586.407 4620.835   100   b
 vectorised  742.325  758.183 1111.202  796.507 1383.268 2587.940   100  a

With check it's also checks the equality of output.


Countdown number game solver I haven't a clue whether this works for every case, but it has in the ones I tried.
If there are any optimizations that could happen anywhere please do comment on it. Regarding my code style, I'm sure there's something to fix there.
In this game you are given a set of numbers and a target. You need to reach the target using only the given numbers and the operators (\$ + \$, \$ - \$, \$ / \$, \$ \times \$). You cannot use any number more than once, but every operator can be used as many times as necessary. You do not need to use every number/operator.
import itertools

target = 225
numbers = [25, 1, 9, 9, 4, 2]
math_functions = "+-/*"
solution = None

def generate_number_permutations(numbers, length):
    return list(itertools.permutations(numbers, length))

def generate_function_permutations(length):
    return list(itertools.product(math_functions, repeat=length))

for x in range(len(numbers)):
    number_permutations = generate_number_permutations(numbers, x+1)
    function_permutations = generate_function_permutations(x)

    if x == 0:
        for y in number_permutations:
            if y[0] == target:
                solution = y[0]
                break
        else:
            continue

    for permutation in number_permutations:
        for functions in function_permutations:
            value = permutation[0]
            for function in enumerate(functions):
                if function[1] == "+":
                    value += permutation[function[0]+1]
                elif function[1] == "-":
                    value -= permutation[function[0]+1]
                elif function[1] == "/":
                    value /= permutation[function[0]+1]
                else:
                    value *= permutation[function[0]+1]

            if value == target:
                solution = permutation, functions
                break
        else:
            continue
        break

    if solution is not None:
        break

print(solution)

Globals
Since target, numbers and math_functions are global constants, they should be all-caps - e.g. TARGET. That said, you should refactor this so that the target and numbers are parametric, so that they can (eventually) be randomly generated.
Solution does not belong as a global, since it is mutable - its presence prevents your program from being re-entrant.
Likewise, the globally-scoped code starting with for x should be moved into a function.
Use of operators
This falls into the category of "code as data". This block:
            if function[1] == "+":
                value += permutation[function[0]+1]
            elif function[1] == "-":
                value -= permutation[function[0]+1]
            elif function[1] == "/":
                value /= permutation[function[0]+1]
            else:
                value *= permutation[function[0]+1]

can be greatly simplified if you change your math_functions to be a tuple (or maybe dictionary) of character-operator pairs, where you import your operators from https://docs.python.org/3/library/operator.html .


Encryption/decryption of a string with a salt I have a new job coming up soon and want to increase my PHP knowledge.

This is my first attempt at creating and using a class. I just want to know if it's the correct way to do things.

It's very basic: a simple encrypt / decrypt of a string with a salt.

/**
* Encryption Class
*
* Use for encrypting / decrypting a string securely
*
*/

class stringEncryption
{

  // generate random string if the variable in a encrypt / decrypt is set as false
  public function generateRandomString($length = 10) {
    return substr(str_shuffle("0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"), 0, $length);
  }

  // encrypts the string
  public function encryptString($string, $salt)
  {
    //if there is no salt, generate one
    if (!$salt)
    {
      $salt = self::generateRandomString();
    }
    return trim(base64_encode(mcrypt_encrypt(MCRYPT_RIJNDAEL_256, $salt, $string, MCRYPT_MODE_ECB, mcrypt_create_iv(mcrypt_get_iv_size(MCRYPT_RIJNDAEL_256, MCRYPT_MODE_ECB), MCRYPT_RAND))));
  }

  // decrypt the string
  public function decryptString($string, $salt)
  {
    return trim(mcrypt_decrypt(MCRYPT_RIJNDAEL_256, $salt, base64_decode($string), MCRYPT_MODE_ECB, mcrypt_create_iv(mcrypt_get_iv_size(MCRYPT_RIJNDAEL_256, MCRYPT_MODE_ECB), MCRYPT_RAND)));
  }

}

This is how I have created 2 objects and encrypted / decrypted

include 'libs/PHP_Classes/class.encryption.php';

// Password BEFORE encryption
$passwordBefore = "TestPassword";
$salt = "8i3bf92f";

echo "Current Password: $passwordBefore
";

// object to handle the new password to be encrypted
$safePassword = new stringEncryption;
// object to handle the decryption of a password
$decrypt = new stringEncryption;

// create the encrypted password
$encryptedPassword = $safePassword->encryptString($passwordBefore, $salt);
// decrypt the password
$decryptedPassword = $decrypt->decryptString($encryptedPassword, $salt);

// show the password with encryption
echo "Encrypted: $encryptedPassword
";
// show the password decrypted
echo "Decrypted: $decryptedPassword";

Salt

Your salt isn't a salt, it's a key.

And you are generating a random key if none is given, and the user has no acces to this key. This means that if I do this:

$encryptedPassword = $safePassword->encryptString($passwordBefore);
$decryptedPassword = $decrypt->decryptString($encryptedPassword);

My output will be:

Current Password: TestPassword
Encrypted: ZWZNn1FC8F0NCO7Ac7YZojlUITPupOXsDV3BLvS3W2M=
Decrypted: æÁSLkz-HGÐyV]l¥S3]‹*õA"VÚX'

This shouldn't happen.

I would remove the generation code in encryptString and rename salt to key.

Hashing vs Encryption

Generally, passwords are not encrypted, they are hashed. Is there a reason you are encrypting your passwords?

Padding oracle attacks

You are vulnerable to padding oracle attacks. I linked to some resources about this in this post.

Object creation

Why are you creating two stringEncryption objects? One would be enough.

Style

be consistent: either curly brackets go on the same line or the next
follow conventions: class names start with an uppercase letter

If you want to learn more about OOP in PHP, I would suggest you write something where your objects have a state (for example Conway's Game of Life).


Generic message in Elm for simple model updates In the Elm examples I have seen for form handling, they attach each input element with an update handler that sends out a very specific message to update the model according to the new input state.

So you end up with a lot of message definition in your Msg union type.

Alternatives I have seen for "dynamic" or complex forms use generic handlers that can work with Dict of data, keyed by form input id, at the expense of some type-safety.

I was thinking I can have (at least for simple handlers that only update the model in straightforward ways and do not need to send Commands to the runtime engine) a middle-ground with a generic SimpleModelUpdate message that takes a mutator function as its payload to describe what it is actually supposed to do.

Something like

 type Msg =  ....
   | SimpleModelUpdate (Model -> Model)

 update model msg = ...
   case SimpleModelUpdate mutation ->
     (mutation model, Cmd.none)

 Html.button
  [onClick = SimpleModelUpdate (\m -> {m | counter = counter + 1})]
  [text "Increase"]

That seems to work. And I like it because I do not have to spread the code for trivial updates all over the place (I can reserve dedicated message types for the really "important" stuff).

Is this bad design? Are there other issues that I am not seeing?

I think the general consensus is to avoid functions in Msg constructors when possible.

The one certain thing that will break is the Msg history import/export of the Elm debugger, since functions cannot be serialized.

Evan included this comment in his Sortable Tables example:

  One of the core rules of The Elm Architecture is never put functions in your Model or Msg types. It may cost a little bit of extra code to model everything as data, but the architecture and debugging benefits are worth it.

Here's some commentary by Zoul on Discourse:

  Generally speaking, in Elm you model an app with explicit state modified by a pure function based on messages with clear semantics. This makes it very easy to reason about the state changes: it’s quite obvious what state the app is in and what state transitions lead to this state.

  Once you start using functions in messages, this model gets much more powerful, but much less transparent. You lose the clear distinction between data and code – the messages no longer describe what you want done, they implement a part of it. You cannot see the behaviour just by looking at the update function anymore. And comparing or serializing the messages gets more complicated.

  In the end, it’s probably not worth it.

Besides breaking the debugger for yourself (and for others, in case you publish a package violating this principle), I don't know of any kind of language problems you may run into if you go down this path, and the above comments are the norm for this question: vague warnings about never putting functions in messages but not much specificity into the why not of the question besides the debugger issue.

But in the end, if the language designer says to never put functions in your Model or Msg types, in bold text nonetheless, I'd defer to their judgment on the matter.


Recursively find all objects of type System.Web.UI.Pair in an object array I have an object array that contains various types of objects (primitives, strings, other object arrays) at arbitrary levels of nesting.  I need to pull all objects of type System.Web.UI.Pair from this array and am having a heck of a time writing the method.  This is especially difficult because the Pair class itself can contain other Pairs or enumerable of Pairs.

A picture is worth a thousand words:

(Original URL: http://postimage.org/image/i7fuad3b9/)

Here's the method I came up with, but it feels inelegant and incomplete.  Any improvements, especially ones that can leverage LINQ are welcome.

   public static IEnumerable<Pair> Flatten(this object root, List<Pair> list)
    {
        if (root == null)
        {
            return list;
        }
        if (root is Pair)
        {
            var pair = root as Pair;
            list.Add(pair);
            if (pair.First is IEnumerable)
            {
                Flatten((root as Pair).First, list);
            }
            else if (pair.First is Pair)
            {
                list.Add(pair.First as Pair);
            }
            if (pair.Second is IEnumerable)
            {
                Flatten((root as Pair).Second, list);
            }
            else if (pair.Second is Pair)
            {
                list.Add(pair.Second as Pair);
            }
        }
        if (root.GetType().IsArray || root is IEnumerable)
        {
            foreach (object o in (IEnumerable) root)
            {
                Flatten(o, list);
            }
        }
        return list;
    }

You might consider making this an extension method on Pair instead of object.  You could simplify it to this:

public static IEnumerable<Pair> Flatten(this Pair p, List<Pair> toBuild = null)
{
   if (toBuild == null)
      toBuild = new List<Pair>();

   if (p.First is Pair)
   {
      (p.First as Pair).Flatten(toBuild);
   }
   else if (p.First is IEnumerable)
   {
      foreach (object o in (p.First as IEnumerable).OfType<object>().Where(ob => ob is Pair))
      {
         (o as Pair).Flatten(toBuild);
      }
   }

   //repeat for p.Second

  toBuild.Add(p);
  return toBuild;
}

You can use it like this:

var result = myPair.Flatten();

Or on an IEnumerable<Pair>:

var result = myList.SelectMany(p => p.Flatten());

Or on a regular IEnumerable:

var result = myList.OfType<object>().Where(o => o is Pair)
  .SelectMany(p => (p as Pair).Flatten());


Group and send all files to filetype folder I have around ~2M of files in one folder, which I need to manage. So, I want to separate them in groups by their filetypes.

Example:

filenames

1.A, 2.A, 3.A, 2.B, 3.B, 4.B, 5.C, 6.C

My code:

import os
all_files = os.listdir(".")
filetypes = list(set([i.split(".")[-1] for i in all_files]))
for i in filetypes:
    os.system("mkdir -p %s"%i)
    os.system("mv *.%s %s"%(i, i))

How can I improve this?

Avoid os.system() by all means. It wastes resources by creating 2 processes per invocation, and is vulnerable to various attacks (at least spell out full paths to the mkdir and mv utilities).

os.mkdir() provide all necessary functionality (don't forget to catch OSError). A combination of os.link() and os.unlink() emulates mv (they are only available on Unix - but you are calling mv anyway).


Factors sieve optimization I've written a more powerful version of the Sieve of Eratosthenes, this Sieve returns the number of factors of all the numbers below the limit.Sadly the running time is \$O(n^2)\$, so it is much slower than the ordinary sieve. Can you suggest a way to improve and optimize it?

import doctest

def factors_sieve(limit):
    """
    Returns the number of factors of all the numbers below the limit.
    For better convenience use list(enumerate(factors_sieve(limit)))

    >>> list(enumerate(factors_sieve(9)))
    [(0, 0), (1, 1), (2, 2), (3, 2), (4, 3), (5, 2), (6, 4), (7, 2), (8, 4)]
    >>> factors_sieve(100000).count(2) # How many prime numbers below 100000?
    9592
    """
    sieve = [2]*limit
    sieve[0],sieve[1] = 0,1
    for number,number_of_divisors in enumerate(sieve):
        if number >= 2:
            for multiple in range(number*2, len(sieve), number):
                sieve[multiple] += 1

    return sieve

if __name__ == "__main__":
    doctest.testmod()

The running time is not \$ O(n^2) \$. It is \$ O(n \log n) \$. Why?
$$ T(n) = O \bigg( { n - 2 · 2 \over 2 } + { n - 2 · 3 \over 3 } + \dotsb + { n - 2 · { n \over 2 } \over { n \over 2 }} \bigg) = O(n \log n). $$ This is very similar to the sum of a harmonic series. So it is pretty good in terms of time complexity.

It is possible to optimize the code a little bit:

divisors_count = [2] * limit
divisors_count[0], divisors_count[1] = 0, 1
min_divisor = 2
max_divisor = len(divisors_count) // 2
for divisor in range(min_divisor, max_divisor + 1):
    for multiple in range(divisor * 2, len(divisors_count), divisor):
        divisors_count[multiple] += 1
return divisors_count

I changed the loop over the enumerated list to a loop over a range of divisors and removed the if statement inside the loop's body. In my opinion, it also makes the logic more clear because we do not need a list itself in the outer loop. It is also sufficient to iterate only up to len(sieve) // 2, because otherwise range(number * 2, len(sieve), number) is empty.

I also gave the variables more meaningful names: divisor instead of number and so on to make the code easier to understand (but the initial version is rather clear, too, in my opinion).

But either way, this code shows a good scalability: it runs for about 1.8–1.9 seconds with my optimizations, and 2.1–2.2 seconds without them, for limit = 10 ** 6.


Angular 4 Application I have created an application with Angular 4. I set up the Angular application with Angular CLI. Can anybody check this and suggest some improvements?

Following is the folder structure I used:

In AppModule I have the following code:

import { NgModule, CUSTOM_ELEMENTS_SCHEMA, Injector, APP_INITIALIZER } from '@angular/core';
import { HttpModule } from '@angular/http';
import { FormsModule, ReactiveFormsModule } from '@angular/forms';
import { BrowserModule } from '@angular/platform-browser';
import { RouterModule } from '@angular/router';
import { BrowserAnimationsModule } from '@angular/platform-browser/animations';

import { NgBootstrapFormValidationModule } from 'ng-bootstrap-form-validation';
import { AppRoutingModule } from './app.routing';
import { SharedModule } from './shared/shared.module';
import { AuthenticationService, DashboardService, NotificationService, AlertService, KnowledgeBaseService, AssetService } from './shared/services/index';
import { AuthGuard } from './shared/guards/auth.guard';

import { AppComponent } from './app.component';
import { LoginComponent } from './login/login.component';
import { MainComponent } from './main/main.component';

import { AppPreBootstrap } from '../AppPreBootstrap';

export function appInitializerFactory(injector: Injector) {
    return () => {
        return new Promise<boolean>((resolve, reject) => {
            resolve(AppPreBootstrap.getApplicationConfig());
        });
    }
}

@NgModule({
    declarations: [
        AppComponent

    ],
    imports: [
        BrowserModule,
        BrowserAnimationsModule,
        HttpModule,
        FormsModule,
        ReactiveFormsModule,
        NgBootstrapFormValidationModule.forRoot(),
        SharedModule,
        RouterModule,
        AppRoutingModule
    ],
    providers: [
        AuthenticationService,
        DashboardService,
        NotificationService,
        AlertService,
        KnowledgeBaseService,
        AssetService,
        AuthGuard,
         {
            provide: APP_INITIALIZER,
            useFactory: appInitializerFactory,
            deps: [Injector],
            multi: true
        }
    ],
    bootstrap: [AppComponent],
    schemas: [CUSTOM_ELEMENTS_SCHEMA]
})
export class AppModule { }

Only a couple of beautifying changes i would make. I hate giant app.modules so i like to reorginize all core imports into core.module and import that into app.module, like described here.

under "core feature module"

I do now know what the recent angular-cli generates but i like to think ahead about which modules what my application will include. If it will be many components and you can comfortably group some of them, think about creating more modules(lazy-loading)

Also, consider  where you will put all your constructs (datepicker, timepicker, alert) components and export them. Read more about the shared module on the link above, you have a good setup  for that already. BUT shared module should then not be included into the app.module, only in the functionality modules(if you decide to go that way)


Drawing blocks for a 2D game background This draws blocks on the screen from a grid to make a background for my game. I am wondering if anyone has any suggestions on optimizing it for speed.

int blockwidth=blocksize-2;
//Draw coloured blocks
for (int x=0;x<sizex;x++){
   int screenx=-(int)camerax+(x*blocksize);
      if (screenx>-blocksize && screenx<gamewidth){
        for (int y=0;y<sizey;y++){
          int screeny=-(int)cameray+(y*blocksize);
          if (screeny>-blocksize && screeny<gameheight){
            if (tiles[x][y][0]>0){
                g.setColor(new Color( tiles[x][y][1]));
                //g.fillRect(screenx,screeny,blockwidth,blockwidth);
                g.drawImage(Iloader.Imagelist.get(0), screenx,screeny, screenx+blockwidth,screeny+blockwidth, graphicsize,0,graphicsize*2,graphicsize, null);
            } else {
                //g.setColor(new Color( tiles[x][y][1]  | 0xFFFF0000));
                g.setColor(new Color( tiles[x][y][1]));
                g.fillRect(screenx,screeny,blockwidth,blockwidth);

            }
          }
        }
    }
 }

There are a few optimizations I can see in your code.

creating a new Color every time is a little severe. You can do a few things here, for example, if your color palette is limited, then cache the individual Color instances. I know it sounds petty right now, but, when you add it up there are a lot of new Color instances created.

What you should at minimum do, is track your last Color used, and only create a new one if it is different.
Pull the Iloader.Imagelist.get(0) outside the loop, and have Image image = Iloader.Imagelist.get(0)
Pull calculations outside the loops where you can... and continue/break when you can too.

Image image = Iloader.Imagelist.get(0);
int screenx=-(int)camerax - blocksize;
for (int x = 0; x < sizex; x++){
    screenx += blocksize;
    if (screenx <= -blocksize) {
        continue;
    }
    if (screenx >= gamewidth) {
        break;
    }
    int screeny= -(int)cameray - blocksize;
    for (int y = 0; y < sizey; y++){
        screeny += blocksize;
        if (screeny <= -blocksize)
            continue;
        }
        if (screeny >= gameheight) {
            break;
        }
        if (tiles[x][y][0] > 0) {
            // need to set the color here? g.setColor(new Color( tiles[x][y][1]));
            g.drawImage(image, screenx, screeny, screenx + blockwidth,
                        screeny + blockwidth, graphicsize, 0, graphicsize * 2,
                        graphicsize, null);
        } else {
                //g.setColor(new Color( tiles[x][y][1]  | 0xFFFF0000));
            g.setColor(new Color( tiles[x][y][1]));
            g.fillRect(screenx,screeny,blockwidth,blockwidth);

        }
    }
}

The above code does not have the mechanism for caching the color. You should figure one out.


JavaScript function for to create HTML-elements I have written this function for creating complete HTML-elements including arbitrary attributes and inner HTML.

I want to keep and reuse it. Therefore I have tried to document the usage well.

I would be interested in hints concerning:

Could the code be improved? So that it is becomes more succinct and efficient?
Can one expect the function to work as intended even when it isn't used correctly?
Is the documentation understandable? Or does it have to be improved?
Is there a way to check if a valid HTML node name has be assigned as first parameter? Currently I'm only checking if the parameter has been assigned and is of type string.

/**
 * Returns an HTML-element of the specified node type,
 *  with the specified attributes and text.
 *
 * @param { string } nodeName - E.g. 'div', 'li', 'p'
 * @param { string } attrs - The attributes of the
 *  HTML-element. Each attribute has to be defined
 *  as <name>=<value>. Multiple attributes have to
 *  be separated with a blank between each pair.
 *  Example:
 *  'value=12 class=option-item data-test=123Demo'
 * @param { string } innerHtml - String for the
 *  innerHTML of the HTML element.
 *
 * @return { Object } - The created HTML element.
 * @throws { Object } - In case of error.
 *
 * Usage example:
 * wrap.appendChild( getHtmlElement(
     'div',
            'class=spec data-shipping=express id=123',
            'Your package will be shipped soon.' ));
 */

function getHtmlElement(nodeName, attrs, innerHtml) {
  var ret = null;

  if (!nodeName || typeof nodeName !== 'string') {
    throw { message: 'No nodeName-parameter assigned.' }
  }

  attrs = attrs || '';
  innerHtml = innerHtml || '';

  ret = document.createElement(nodeName);

  attrs = attrs.split(/\s/g);

  if (attrs[0]) {
    attrs.forEach(function(attr) {
      attr = attr.split(/=/);

      if (attr[1]) {
        ret.setAttribute(attr[0], attr[1]);
      }
    });
  }

  ret.innerHTML = innerHtml;

  return ret;
}

// Usage-example:
document.body
        .appendChild(getHtmlElement(
                     'p',
                    'class=inserted-paragraph data-info=paragraph-number-one',
                     'Lorem ipsum dolor sit amet, consectetuer ' +
                       'adipiscing elit. Aenean commodo ligula eget dolor.'));

Some thoughts below:

Consider passing attrs as an object or object literal representing key:value pairs.  This would eliminate need for more fragile string parsing approach you currently have.

You could replace all of this:

  attrs = attrs || '';
  innerHtml = innerHtml || '';

  ret = document.createElement(nodeName);

  attrs = attrs.split(/\s/g);

  if (attrs[0]) {
    attrs.forEach(function(attr) {
      attr = attr.split(/=/);

      if (attr[1]) {
        ret.setAttribute(attr[0], attr[1]);
      }
    });
  }

with this:

  attrs = attrs || {};
  innerHtml = innerHtml || '';

  ret = document.createElement(nodeName);

  for (attr in attrs) {
        ret.setAttribute(attr, attrs[attr]);
  }

Consider different name.  "get", in typical programming thought often refers to retrieval of an existing item. I would think createHtmlElement or similar would be more appropriate.
I would strongly consider adding some additional validation around the input node name, to at a minimum make sure you have a non-zero length string.
After element creation, you could determine whether ret contains HTMLUnknownElement object, which would indicate that the browser did not recognize the node name value.  This could be useful if you wanted to fail out at this point to not allow arbitrary tag creation.  Perhaps whether or not to allow arbitrary (custom) tag creation could even be a flag passed as optional parameter.
Consider throwing known Error object rather than vanilla object literal of your own creation. You get a lot more utility from this Error object than from generic object.  Check out the documentation at link below:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error

Example:

throw new Error('error message');


Convert JavaScript to CSS media query I'm running into page performace issues and I believe this block is causing the biggest slowdown:

  // Section menu on mouseover, hide menu on mouseout
  // Can be more concise using .toggleClass?
  $('section').live('mouseenter', function() {
    if ($(window).width() > 1278) {
      $(this).find('menu').removeClass('hidden')
      $(this).find('div.section-wrapper').addClass('active')
    }
  }).live('mouseleave', function() {
    if ($(window).width() > 1278) {
      $(this).find('menu').addClass('hidden')
      $(this).find('div.section-wrapper').removeClass('active')
    }
  })

For the record, I'm aware .live() is deprecated and will eventually switch to .on(). I'm under the impression this only runs when mouseenter and mouseleave occur, but with it, the page performance is poor - especially for mobile.

I'm trying to convert this to a css media query so it runs faster but the following block of SASS isn't cutting it:

@media(min-width: 1279px) {
  section:hover {
    menu.hidden {
      display:block;
    }
    section .section-wrapper {
      border: 1px #ddd solid;
      padding: 14px;
    }
  }
}

.active just adds a border and changes the padding to accommodate. section:hover appears to do nothing. I added section {background-color: #E1C2E2;} within the media query for troubleshooting and it's working fine. Ideas? Could it be nested SASS with media queries causing the slowdown?

Just a simple case of css hierarchy and not covering my bases with undoing twitter bootstrap's .hidden class. This does the trick:

@media(min-width: 1279px) {
  section:hover {
    menu.hidden {
      display: block;
      visibility: visible;
    }
    .section-wrapper {
      border: 1px #ddd solid;
      padding: 14px;
    }
  }
}

As for performance, it was this block that was causing a much bigger slow down:

  $(window).bind('load resize orientationchange', function(){
    $('#main_menu').trigger('testfit')
    if ($(window).width() < 1278) {
      $('section').find('menu').removeClass('hidden')
      $('section').find('div.section-wrapper').addClass('active')
    } else {
      $('section').find('menu').addClass('hidden')
      $('section').find('div.section-wrapper').removeClass('active')
    }
  })

Using a timeout will speed things up since it doesn't need to run everytime the browser size changes by 1 pixel: https://stackoverflow.com/questions/599288/cross-browser-window-resize-event-javascript-jquery


More readable approach? This is a fairly short code snippet, but it made me a little headache, because it is hard to understand, isn't it?

// You have to be in both 'group' and 'board' to get access (or be a superuser)
if ( ( !$is_in_group || !$is_in_board ) && !$is_superuser) {
    die('No access');
}

This code is from a download page, where registered users can download files. Only users within a certain board and group are allowed to download files. The three variables are just for this demo. The values are coming from a CMS function.

Do you have a more readable or better way to verifying access here?

You can rewrite it like this:

// you have to be in both 'group' and 'board' to get access
$has_access = $is_in_group && $is_in_board;
// you must have access or be a superuser
if (!($has_access || $is_superuser)) die('No access');

I don't think an explanation is needed.

Alternatively you could even write:

// you have to be in both 'group' and 'board' to get access
$has_access = $is_in_group && $is_in_board;
// you must have access or be a superuser
if ($has_access || $is_superuser) {
  // ....
  // do whatever requires access
  // ....
} else die('No access');


PHP database class - Managing two connections with different privileges I'm just trying to master the art of using classes in PHP and have come across a concern.

For security reasons, I sometimes use two database connections in my application; one with read-only privileges and one with full read/write. Unfortunately, I wasn't really thinking about this when I started to build a few classes.

So, I have a database class, which is essentially a pointless PDO wrapper (pointless because PDO is a wrapper), but thought it'd be good practice to write one anyway and I may extend PDO later too. This is what I did:

<?php
class Database {

    private $dbh;

    public function __construct($accessLevel = NULL, $credentials = NULL) {
        if (gettype($credentials) === 'array') {
            $dsn = 'mysql:host='.$credentials['dbHost'].';dbname='.$credentials['dbName'];
            $options = array (
                PDO::ATTR_PERSISTENT => false,
                PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION
            );
            if ($accessLevel == "public") {
                $this->dbh = new PDO($dsn, $credentials['publicUser'], $credentials['publicPassword'], $options);
            } else if ($accessLevel == "private") {
                $this->dbh = new PDO($dsn, $credentials['privateUser'], $credentials['privatePassword'], $options);
            }
        }
    }

    // other database functions

}
?>

For a public connection (read/write), I simply used this:

$db = new Database('public', config['dbConfig']);

... or for a private connection (read-only), I used this:

$db = new Database('private', config['dbConfig']);

... and when I wanted to use the connection in another class, I simply injected the connection, like so:

$user = new User($db);

Works fine, but then I realised that I may need two connections inside that class, one for reading only and one for all. So I got a little confused, but had a go regardless. This is what I did:

Instead of calling the class with either of the connections, I called the class twice with both, like so:

$publicDB = new Database('public', $config['db']);
$privateDB = new Database('private', $config['db']);

and then injected both of those to my other class:

$user = new User($publicDB, $privateDB);

Inside this User class, this how I used the two connections:

<?php
class User {

    private $dbh;
    private $dbhp;

    public function __construct($publicDatabase = NULL, $privateDatabase = NULL) {
        $this->dbh = $publicDatabase;
        $this->dbhp = $privateDatabase;
    }

    public function doSomething() {
        $this->dbh->query("INSERT INTO......");
        $this->dbh->bind(':userInput', $userInput);
        $this->dbh->execute();
    }

    public function doSomethingSafely() {
        $this->dbhp->query("SELECT * FROM......");
        $this->dbhp->bind(':userInput', $userInput);
        $results = $this->dbhp->resultset();
    }
}
?>

Right, this actually works fine but I'm worried it's not the acceptable method or it may cause problems later down the road. I have a few questions:

Is using two connections with different privileges still considered good practice? Even though I'm properly escaping and validating my data and binding the values using PDO?
If yes for above, is there a better way to manage the two connections for using in many classes or is what I have done acceptable?

You want two classes. They extends Database. PublicDatabase and PrivateDatabase, you instantiate whichever you need, and inject whichever you need. That way you can even have the same signature in the constructor in both types.

If you don't have a preference, you just need a database connection in a function, just ask for Database, and either will fit.

Also

You don't need to use gettype, you can hint in the function's signature for the type, it works for all Objects and array. So for example:

public function __construct(array $accessLevel, array $credentials) {

Don't give defaults if you can't handle defaults. Your object won't work if $credentials is empty, don't let it be.
What do your Database object solve? What kind of abstraction it provides? Usually such 'utility' classes are pointless.

Better approach

Your User shouldn't be responsible to save/fetch itself. In fact, if you have a User which doesn't care where it came from, you have a perfectly reusable object, a User that can come from a database, a file, a session, a form, whatever, and work the same.

You should have two objects: the User and the UserMapper, the UserMapper is responsible for fetching information about users and fill them into User objects, as well as saving information from the User object back into the database.

The idea of OOP is separation of responsibilities.


Function Composition: $ versus `.` Learn You a Haskell offers the findKey function:

Here's the book's implementation:

findKey :: (Eq k) => k -> [(k,v)] -> v
findKey key xs = snd . head . filter (\(k,v) -> key == k) $ xs

I implemented it with $:

findKey' :: (Eq k) => k -> [(k,v)] -> v
findKey' k xs = snd $ head $ filter ((== k) . fst) xs

As far as I can tell, it's a stylistic choice here to select . over $?

Lastly, is there a way to write findKey such that the [(k,v)] is curried & thus not required in the signature of findKey key xs?

Both are fine, and it's a matter of choice.

The first definition, being a composition of functions, treats snd . head . filter (\(k, v) -> key == k) as one big function.  It can therefore be transformed more easily into point-free style, if you like that kind of thing.

findKey'' :: (Eq k) => k -> ([(k, v)] -> v)
findKey'' key = snd . head . filter (\(k,v) -> key == k)