Simple Linux upgrade script in Bash revision 2 As I will be deploying this script on multiple machines with the very same system Linux Mint 18 with rather same configuration, I would like to be semi-sure I won't screw things up much.

This little script will run every day from the root's crontab and will be logged into syslog or my own log AFTER this manual script passes some quality checks. Logging is not the question now. Automation is also not the question yet. In the meantime it is being executed as normal user and elevated with sudo internally. No help message needed. And no docs needed either. I ran it through Shellcheck.net. I have also ruled out the use of subjectively nicer apt, because it is not meant to be used in scripts.

Conditions still are:

Code readability
Output readability
Colored headings
Attempt to correct things
Clean-up after update

My current idea is a little more advanced than the first one, having implemented exit code checking was a great idea:

#!/bin/bash

RED="\033[1;31m"
GREEN="\033[1;32m"
YELLOW="\033[1;33m"
NOCOLOR="\033[0m"

function show_success {

  echo -e "${GREEN}Success.${NOCOLOR}\n"

}

function show_error_and_exit {

  echo -e "${RED}An error occured.${NOCOLOR}\n"

  exit "$1"

}

function error_handler {

  if [[ $1 -ne 0 ]];
  then

    show_error_and_exit "$2"

  else

    show_success

  fi

}

echo -e "\n${GREEN}Step 1: configure packages${NOCOLOR}"
echo -e "${YELLOW}dpkg --configure -a${NOCOLOR}"

sudo dpkg --configure -a

error_handler $?  1

echo -e "${GREEN}Step 2: fix broken dependencies${NOCOLOR}"
echo -e "${YELLOW}apt-get install --fix-broken${NOCOLOR}"

sudo apt-get install --fix-broken

error_handler $?  2

echo -e "${GREEN}Step 3: update cache${NOCOLOR}"
echo -e "${YELLOW}apt-get update${NOCOLOR}"

sudo apt-get update

error_handler $?  3

echo -e "${GREEN}Step 4: upgrade packages${NOCOLOR}"
echo -e "${YELLOW}apt-get upgrade${NOCOLOR}"

sudo apt-get upgrade

error_handler $?  4

echo -e "${GREEN}Step 5: upgrade distribution${NOCOLOR}"
echo -e "${YELLOW}apt-get dist-upgrade${NOCOLOR}"

sudo apt-get dist-upgrade

error_handler $?  5

echo -e "${GREEN}Step 6: remove unused packages${NOCOLOR}"
echo -e "${YELLOW}apt-get --purge autoremove${NOCOLOR}"

sudo apt-get --purge autoremove

error_handler $?  6

echo -e "${GREEN}Step 7: clean up${NOCOLOR}"
echo -e "${YELLOW}apt-get autoclean${NOCOLOR}"

sudo apt-get autoclean

error_handler $?  7

Don't use non-portable terminal escape codes
The tput program exists for exactly this purpose:
red=$(tput setaf 1)
green=$(tput setaf 2)
yellow=$(tput setaf 3)
nocolor=$(tput sgr0)

(I've downcased your variable names, to avoid surprising conflicts with environment variables)
Inline the single use functions
show_success and show_error_and_exit are laudable, and do help readability, but they are only used once each, and may be clearer if written directly into error_handler.  Either way, their output really ought to go to the error stream: >&2.
Reduce duplication
Look at this pattern:
echo -e "\n${GREEN}Step 1: configure packages${NOCOLOR}"
echo -e "${YELLOW}dpkg --configure -a${NOCOLOR}"

sudo dpkg --configure -a

error_handler $?  1

This structure is repeated throughout the program, but it can be refactored.  Let's write the variable parts using variables:
echo "${green}Step ${step}: ${description}${nocolor}"
echo "${yellow}${command}${nocolor}"

$command

error_handler $?  $step

There's three things we need: a step number, description, and the command to run.  The command needs to be an array; that's easy to arrange by passing it as all the arguments after description.  We can also inline the error_handler:
do_action()
{
    step=$1; shift
    description=$1; shift
    printf "${green}Step %s: %s\n" "$step" "$description"
    printf "${yellow}"
    printf '%q ' "$@"
    printf "${nocolor}\n"

    if "$@"
    then
        printf "${green}Step %s: SUCCESS${nocolor}\n" "$step"
    else
        err=$?
        printf "${red}Step %s: FAILED${nocolor}\n" "$step" >&2
        exit $err
    fi
}

This function is then used as:
i=0
do_action $((++i)) "configure packages" dpkg --configure -a
do_action $((++i)) "fix broken dependencies" apt-get install --fix-broken
do_action $((++i)) "update cache" apt-get update
do_action $((++i)) "upgrade packages" apt-get upgrade
do_action $((++i)) "upgrade distribution" apt-get dist-upgrade
do_action $((++i)) "remove unused packages" apt-get --purge autoremove
do_action $((++i)) "clean up" apt-get autoclean

You might choose to make the step number a global variable and increment it inside the function; I haven't done that, because global variables tend to proliferate and make the program hard to reason about.
Don't sudo in a script
Many calls to sudo is a sign that the entire script needs superuser capabilities, so it's better if the user runs the script as root.  You could automate this (though I would personally just error out):
test $EUID = 0 || exec sudo $0 "$@"

Full program
#!/bin/bash
set -e -u

red=$(tput setaf 1)
green=$(tput setaf 2)
yellow=$(tput setaf 3)
nocolor=$(tput sgr0)

do_action()
{
    step=$1; shift
    description=$1; shift
    printf "${green}Step %s: %s\n" "$step" "$description"
    printf "${yellow}"
    printf '%q ' "$@"
    printf "${nocolor}\n"

    if "$@"
    then
        printf "${green}Step %s: SUCCESS${nocolor}\n" "$step"
    else
        err=$?
        printf "${red}Step %s: FAILED${nocolor}\n" "$step" >&2
        exit $err
    fi
}

i=0
do_action $((++i)) "configure packages" dpkg --configure -a
do_action $((++i)) "fix broken dependencies" apt-get install --fix-broken
do_action $((++i)) "update cache" apt-get update
do_action $((++i)) "upgrade packages" apt-get upgrade
do_action $((++i)) "upgrade distribution" apt-get dist-upgrade
do_action $((++i)) "remove unused packages" apt-get --purge autoremove
do_action $((++i)) "clean up" apt-get autoclean


Porting ProcMonDebugOutput from C# to VBA This code has one public method. ProcMonDebugOutput(). It will sent a string of text to Process Monitor that process monitor will display.

I'm basically ported this file.

I don't have a 64 bit version of office to test this on, but I think I used all the LongPtr right.

My two big concerns are:
* Are the Win32 API calls "correct". I know they're passing the right data, but should I fiddle with them to make more sense?
* Is my error handling appropiate?

Option Explicit

' Win32 API Constants
Private Const GENERIC_WRITE As Long = &H40000000
Private Const OPEN_EXISTING As Long = &O3&
Private Const FILE_WRITE_ACCESS As Long = &H2
Private Const FILE_SHARE_WRITE As Long = &H2
Private Const FILE_ATTRIBUTE_NORMAL As Long = &H80
Private Const METHOD_BUFFERED As Long = 0
Private Const FORMAT_MESSAGE_FROM_SYSTEM = &H1000
Private Const LANG_NEUTRAL = &H0

' Process Monitor Constants
Private Const FILE_DEVICE_PROCMON_LOG As Long = &H9535
Private Const PROCMON_DEBUGGER_HANDLER As String = "\\.\Global\ProcmonDebugLogger"
Private Const IOCTL_EXTERNAL_LOG_DEBUGOUT As Long = -1791655420

Dim hProcMon As LongPtr

Private Declare Function CreateFile Lib "kernel32" Alias "CreateFileW" _
    (ByVal lpFileName As LongPtr, _
     Optional ByVal dwDesiredAccess As Long = GENERIC_WRITE, _
     Optional ByVal dwShareMode As Long = FILE_SHARE_WRITE, _
     Optional lpSecurityAttributes As LongPtr = 0, _
     Optional ByVal dwCreationDisposition As Long = OPEN_EXISTING, _
     Optional ByVal dwFlagsAndAttributes As Long = FILE_ATTRIBUTE_NORMAL, _
     Optional ByVal hTemplateFile As LongPtr = 0) As LongPtr

Private Declare Function DeviceIoControl Lib "kernel32" _
    (ByVal hDevice As LongPtr, _
     ByVal dwIoControlCode As Long, _
     ByVal lpInBuffer As LongPtr, _
     ByVal nInBufferSize As Long, _
     Optional lpOutBuffer As LongPtr, _
     Optional ByVal nOutBufferSize As Long, _
     Optional lpBytesReturned As Long, _
     Optional ByVal lpOverlapped As LongPtr) As Long

Private Declare Function GetLastError Lib "kernel32" () As LongPtr
Private Declare Sub SetLastError Lib "kernel32" (ByVal dwErrCode As LongPtr)
Private Declare Function FormatMessage Lib "kernel32" Alias "FormatMessageA" _
    (ByVal dwFlags As Long, lpSource As Any, ByVal dwMessageId As Long, _
     ByVal dwLanguageId As Long, ByVal lpBuffer As String, ByVal nSize As Long, _
     Arguments As Long) As Long

Public Function ProcMonDebugOutput(message As String) As LongPtr
    ProcMonDebugOutput = False
    Dim outLen As Long
    outLen = 0

    If hProcMon = 0 Then
        hProcMon = CreateFile(StrPtr(PROCMON_DEBUGGER_HANDLER))
    End If
    If hProcMon = -1 Then
        Dim Buffer As String
        'Create a string buffer
        Buffer = Space(200)
        FormatMessage FORMAT_MESSAGE_FROM_SYSTEM, ByVal 0&, Err.LastDllError, LANG_NEUTRAL, Buffer, 200, ByVal 0&
        Err.Raise Buffer
    End If

    ProcMonDebugOutput = DeviceIoControl _
        (hProcMon, IOCTL_EXTERNAL_LOG_DEBUGOUT, _
        StrPtr(message), Len(message) * 2)
End Function

UPDATE

Based on the feedback I've changed one API Signature and my public method:

Private Declare Function DeviceIoControl Lib "kernel32" _
    (ByVal hDevice As LongPtr, _
     ByVal dwIoControlCode As Long, _
     ByVal lpInBuffer As LongPtr, _
     ByVal nInBufferSize As Long, _
     Optional lpOutBuffer As LongPtr, _
     Optional ByVal nOutBufferSize As Long, _
     Optional lpBytesReturned As Long, _
     Optional ByVal lpOverlapped As LongPtr) As Boolean

Public Function ProcMonDebugOutput(message As String) As Boolean
    If hProcMon = 0 Or hProcMon = 0 Then
        hProcMon = CreateFile(StrPtr(PROCMON_DEBUGGER_HANDLER))
    End If
    If hProcMon = -1 Then
        Err.Raise Err.LastDllError
    End If

    ProcMonDebugOutput = DeviceIoControl _
        (hProcMon, IOCTL_EXTERNAL_LOG_DEBUGOUT, _
        StrPtr(message), Len(message) * 2)
End Function

That's some pretty intense Win32 API stuff you got here, it's nice to see pointers and API calls in a VBA question, it's quite a change from the typical "speed up my Excel macro" question -- kudos!

One thing worries me though:

Dim Buffer As String
'Create a string buffer
Buffer = Space(200)
FormatMessage FORMAT_MESSAGE_FROM_SYSTEM, ByVal 0&, Err.LastDllError, LANG_NEUTRAL, Buffer, 200, ByVal 0&
Err.Raise Buffer

The first argument of Err.Raise should be an error number. If FormatMessage fills up buffer with a string that starts with an error number, then that's the error you're raising, and the rest of the message goes to oblivion. If the string does not start with a number, by VBA's implicit coversion rules you're raising error 0, which is a non-error... and that can't be right.

This instruction is redundant at first sight:

ProcMonDebugOutput = False

...but then, the function doesn't return a Boolean, but a LongPtr - why the Boolean assignment then? False would be implicitly converted to 0, which is the same value that would be returned if you didn't make that confusing assignment. I'd just remove it.

Same kind of issue here:

Dim outLen As Long
outLen = 0

outLen is already initialized with a value of 0, the assignment is redundant... and unless I missed something, the variable is assigned, but isn't used at all. I'd remove these two lines too.

I'll admit I didn't know you could specify ByVal at the call site to force a parameter to be passed by value (I wonder if it breaks the rubberduck parser... but that's my problem ;-) - I like it more than the parentheses-wrapping of the parameter to force ByVal for: it's more readable and less confusing - well done!

I can't seem to get a call-site ByVal to compile... will confirm in a few days when I'm on my laptop. Phone doesn't run VBA ;-)


Insert a table row with text in cells when a button is clicked I made a function to dynamically create this table and a button with a function to display texts inside the table shown as a message.

<!DOCTYPE html>

Click the button to add a new row at the first position of the table and then add cells and content.

<button onclick="myFunction()">Try it</button>

So you want for example 6 inputs and create a table from it with the input when you click on a button.

(plain javascript):

This way it doesn't matter how many input fields you have. It will get all of them. I added some comments so you can understand what's going on.

<!DOCTYPE html>

Click the button to add a new row at the first position of the table and then add cells and content.

<input type="text">

<input type="text">

<input type="text">

<input type="text">

<input type="text">

<input type="text">

<button onclick="Display_inputs()">Try it</button>


Manipulating an ES6 static class property that is an array I've been trying to find a way to create static properties in ES6 classes and have found a solution with the help of various Stack Overflow answers (namely this one and this one).

However, I also wanted to take this a step further and make the static property an array that can be updated/added to using a special static method. I've managed to come up with something that works. Here is a complete demonstration using the class Apple and static property possibleColors:

    class Apple
    {
      constructor(args = {})
      {
        this.color = args.color || 'red';
        this.description = args.description || 'An apple.';
      }

      static get possibleColors()
      {
        if (!this._possibleColors)
          this._possibleColors = ['red', 'green', 'yellow'];
        return this._possibleColors;
      }

      static set possibleColors(arr)
      {
        if (Array.isArray(arr))
          this._possibleColors = arr;
        else
          console.log("Possible colors must be an array.")
      }

      static addPossibleColor(c)
      {
        if (!this._possibleColors)
          this._possibleColors = this.possibleColors;
        this._possibleColors.push(c);
      }

      get color()
      {
        if (!this._color)
          this._color = '';
        return this._color;
      }

      set color(c)
      {
        if (Apple.possibleColors.includes(c))
          this._color = c;
        else
        {
          console.log(`Color '${c}' is not a valid apple color.`);
          if (!this._color)
            this._color = 'red';
        }
      }
    }

The possibleColors property belongs to the class only and will not be a part of new instances. I've also taken measures to ensure that its value is always an array. It defaults to red, green, & yellow, but you can view/change possible colors using the regular getter/setter:

Apple.possibleColors = ['color', 'othercolor']

You can also add new possible colors like this:

Apple.addPossibleColor('newcolor')

In this example, the whole point of having the possibleColors static property is to have a definitive list of valid colors stored in the main class and prevent any Apple instances from being set to a color not in the array. Of course, I can think of other use cases for this kind of technique.

For example, I'm currently trying to write my own basic component system that uses templates to draw/render each component as HTML on the page. The main class (Component or similar) would have a static property for storing the templates in an array, and you'd choose a template from this array when instantiating a component (or it would choose a default if none specified).

What I'd like to know:

Could this approach cause any problems I haven't foreseen (either in this specific scenario or a more complex one)?
Are there better ways to do anything I've demonstrated here?
Is the way I'm handling static properties overall considered good practice?

Protected states require enforcement..
Why would you use the possible colors and complicated apple color vetting process when there is no requirement in your code to do so?
You are using a static list (array) in an effort to protect the state of the Apple, or associated states that rely on apples being specific colors.
Maintaining trust in object states is important and allows you to make assumptions and reduce the code complexity. It is also one of the most important methods coders have of controlling our arch nemeses.. The Bug!
But a chain is only as strong as its weakest link. Even if you put a big sign above it saying "Don't yank this pitifully weak link!" you can't trust coders, not even yourself, for one day the convenience is too tempting, and while in a rush to meet a dead line you yank that chain.
const myApple = new Apple();  // Needs to be "Reddish" not "red"
myApple._color = "Reddish";   // Done, so much easier than wrangling the
                              // possibleColors static and its dificult siblings

And just like that all the work put into protecting the state of your apple is out the window.
Coders are lazy they will not put up with complications if there is an easy way. Coders want performant code and they are not going to use getters and setters when there is direct access. Putting a sign up "Do not use" (_ as the metaphoric sign "private") does not stop use.
If you want to control the color of the apple you must make it impossible to change it without the vetting process, or it is pointless, and pointless code is bad code.


Multiplayer Peer-To-Peer position interpolation I have coded this movement interpolation for Unity3d using Photon.

OnPhotonSerializeView gets called at the tickrate.

Any kind of criticism/improvement suggestions are welcome. I tried to make the code as plain as possible because I don't see this as something that should be over-engineered because it is rather algorithmic in nature.

using System.Collections;
using System.Collections.Generic;
using UnityEngine;

public class PlayerInterpolation : MonoBehaviour {
    Queue<Vector3> positions = new Queue<Vector3>();
    Queue<Quaternion> rotations = new Queue<Quaternion>();
    Queue<int> frameNumbers = new Queue<int>();
    static int amountBuffered = 1;
    static float oneTickTime = 1.0f / Settings.tickRate;

    PhotonView photonView;
    int localSentFrameNumber = 0;
    int lastReceivedFrameNumber = -1;
    int lastFrameNumberMovedTowards = 0;

    int expectedExecuteFrameNumber = 0; //An estimation of what frame number should be executing at this time.
    float expectedExecuteFrameNumberLastExecutedTime = 0;

    Vector3 moveTowards;
    float moveTowardsSpeed = 1.0f;
    bool moveInProgress = false;

    void Start() {
        photonView = gameObject.GetComponent<PhotonView>();
        moveTowards = transform.position;
    }

    void Update() {
        if (photonView.isMine) return;

        transform.position = Vector3.MoveTowards(transform.position, moveTowards, moveTowardsSpeed * Time.deltaTime);
        if (Mathf.Abs(Vector3.Distance(transform.position, moveTowards)) < 0.001f) {
            moveInProgress = false;
        } else {
            moveInProgress = true;
        }

        if (expectedExecuteFrameNumberLastExecutedTime + oneTickTime < Time.realtimeSinceStartup) {
            expectedExecuteFrameNumber++;
            expectedExecuteFrameNumberLastExecutedTime = Time.realtimeSinceStartup;
        }

        if (frameNumbers.Count > 0 && ((frameNumbers.Peek() + amountBuffered <= lastReceivedFrameNumber) || frameNumbers.Peek() <= expectedExecuteFrameNumber - amountBuffered)) {
            if (lastReceivedFrameNumber - frameNumbers.Peek() > amountBuffered) {
                int trimUntil = lastReceivedFrameNumber - amountBuffered;
                while (frameNumbers.Peek() < trimUntil) {
                    positions.Dequeue();
                    rotations.Dequeue();
                    frameNumbers.Dequeue();
                }
            }

            Vector3 position = positions.Dequeue();
            Quaternion rotation = rotations.Dequeue();
            int frameNumber = frameNumbers.Dequeue();

            if (moveInProgress == true) {
                transform.position = moveTowards;
            }

            float distance = Vector3.Distance(transform.position, position);
            moveTowards = position;
            moveTowardsSpeed = distance * (frameNumber - lastFrameNumberMovedTowards) / oneTickTime;

            lastFrameNumberMovedTowards = frameNumber;

            expectedExecuteFrameNumber = frameNumber;
            expectedExecuteFrameNumberLastExecutedTime = Time.realtimeSinceStartup;
        }
    }

    void OnPhotonSerializeView(PhotonStream stream, PhotonMessageInfo info) {
        if (stream.isWriting) {
            stream.SendNext(transform.position);
            stream.SendNext(transform.rotation);
            stream.SendNext(localSentFrameNumber++);
        } else {
            positions.Enqueue((Vector3)stream.ReceiveNext());
            rotations.Enqueue((Quaternion)stream.ReceiveNext());
            int frameNumber = (int)stream.ReceiveNext();
            frameNumbers.Enqueue(frameNumber);
            lastReceivedFrameNumber = frameNumber;
        }
    }
}

I recommend you always write access modifiers for class members and for classes itself. It will make code more clear and understandable. So instead of

Queue<Vector3> positions = new Queue<Vector3>();
Queue<Quaternion> rotations = new Queue<Quaternion>();
...
void Start() { ... }

write

private Queue<Vector3> positions = new Queue<Vector3>();
private Queue<Quaternion> rotations = new Queue<Quaternion>();
...

private void Start() { ... }

Also these queues are not supposed to be changed so mark them with readonly keyword:

private readonly Queue<Vector3> positions = new Queue<Vector3>();
...

It is a matter of taste but it is more common for C# to place opening curly braces on the next line:

private void Start()
{
    photonView = gameObject.GetComponent<PhotonView>();
    moveTowards = transform.position;
}

This code

if (Mathf.Abs(Vector3.Distance(transform.position, moveTowards)) < 0.001f) {
    moveInProgress = false;
} else {
    moveInProgress = true;
}

can be simplified to

moveInProgress = Mathf.Abs(Vector3.Distance(transform.position, moveTowards)) >= 0.001f;

Also it would be better to declare 0.001f as named constant like

private const float DistanceEpsilon = 0.001f;

You should definitely rewrite these conditions:

if (frameNumbers.Count > 0 && ((frameNumbers.Peek() + amountBuffered <= lastReceivedFrameNumber) || frameNumbers.Peek() <= expectedExecuteFrameNumber - amountBuffered)) {
    if (lastReceivedFrameNumber - frameNumbers.Peek() > amountBuffered)

First of all, you use frameNumbers.Peek() multiple times here so store the result of it to variable:

var firstFrameNumber = frameNumbers.Peek();

I recommend to use Any method for checking a collection on emptiness:

frameNumbers.Any()

instead of

frameNumbers.Count > 0

Thanks @Nikita B for pointing out that frameNumbers.Any() should be done before frameNumbers.Peek(). In your case you can just return from the method if there are no elements in the frameNumbers:

if (!frameNumbers.Any())
    return;

These conditions

frameNumbers.Peek() + amountBuffered <= lastReceivedFrameNumber
lastReceivedFrameNumber - frameNumbers.Peek() > amountBuffered

can be written in one form:

lastReceivedFrameNumber - frameNumbers.Peek() > amountBuffered
lastReceivedFrameNumber - frameNumbers.Peek() >= amountBuffered

Now we see that we can store lastReceivedFrameNumber - frameNumbers.Peek() to variable since this expression is used multiple times. So the final form of conditions above will be

if (!frameNumbers.Any())
    return;

var firstFrameNumber = frameNumbers.Peek();
var frameNumbersDifference = lastReceivedFrameNumber - firstFrameNumber;

if (frameNumbersDifference >= amountBuffered ||
    firstFrameNumber <= expectedExecuteFrameNumber - amountBuffered)
{
    if (frameNumbersDifference > amountBuffered)

Also I recommend to extract all subconditions to methods with appropriate names. It will make your code much more easy to read and understand.

Small note: you don't need to write == true or == false when checking value of bool variable. This

if (moveInProgress == true) {

should be rewritten as

if (moveInProgress)
{


VB.NET | SQL Code Cleanup I am trying to teach myself a new language in three weeks. Can you please give me some advice on how to clean my code and make it faster or more reliable? This works fine. I don't "need" any 'free' code, I need the knowledge of what I am doing wrong, and what I am doing right so I can learn.

   Using sqlCon = New SqlConnection("Data Source=(LocalDB)\v11.0;AttachDbFilename=|DataDirectory|\ITCSDatabase.mdf;Integrated Security=True")
        Dim resultVar As String
        sqlCon.Open()
        Dim sqlText = "SELECT appLink FROM appTable WHERE Id = @name"
        Dim cmd = New SqlCommand(sqlText, sqlCon)
        cmd.Parameters.AddWithValue("@name", x)
        resultVar = cmd.ExecuteScalar()
        cmd.ToString()
        If y = 1 Then
            System.Diagnostics.Process.Start(resultVar)
        ElseIf y = 0 Then
            System.Diagnostics.Process.Start("iexplore.exe", resultVar)
        Else
            MsgBox("When calling the appCall Function, an invalid parameter is being sent in.")
        End If
        Dim sqlText2 = "UPDATE appTable SET appClickCount = appClickCount + 1 " & _
        "WHERE Id = @x"
        Dim cmd2 = New SqlCommand(sqlText2, sqlCon)
        cmd2.Parameters.AddWithValue("@x", x)
        cmd2.ExecuteNonQuery()
End Using

As you're beginner, your code can be considered ok.

However you may want to consider following points which help you to write reliable (maintainable) code and improve performance.

You want to store connectionstring to some common place like web.config. You don't have to find and change it all over the place you used it, just change at one place and it will be reflected.
You may want to use stored procedure instead of inline query. With stored procedure, your sql code will be more maintainable and you can go through execution plan of sql code to find out any performance bottleneck. To change sql code you don't have to modify and compile your .net code.

I hope it helps!


(Y.E.S.E.J.) Yet Another Singleton with Enum in Java I have a Java class which has to be generated only once for all the objects that I have - it is a small program. This singleton class holds a mapping of characters. I Googled stack overflow and found that using enum is the most safest way to create a singleton class as Josh Bloch explained in his Java book.

I also read some reviews that singletons are not good. But for my scope I think I need one. So my code is:

public enum  SingletonClassA {
    INSTANCE;
    private static Map<Character, Character> instance;
    static {
        Map<Character, Character> aCharMap = new HashMap();
        aCharMap.put('a', 'e');
        aCharMap.put('o', 'u');
        // in order to keep short I erased other puts.
        instance = Collections.unmodifiableMap(aCharMap);
    }

    public static Map<Character, Character> getInstance() {
        return instance;
    }
}

And in order to reach the elements of this HashMap I use

SingletonClassA.INSTANCE.getInstance().containsKey(aLetter);

Am I creating this singleton class correctly? Do I also need to create a private constructor?

If you have any suggestions about using another data structure for this case please recommend it, write the reasons also.

Despite some other comments, I believe there are times when a singleton is very appropriate, and a powerful tool. The best practice, in Java, since the introduction of enums, is to use them for singletons.

But, your implementation is broken.

When using an enum as a singleton, you should treat the enum as a regular-ish class, and do everything in instance methods, and not have any static content. Then, by declaring just one member in the enum, you get just one instantiation.... always.

So, your code would be better as:

public enum SingletonClassA {
    INSTANCE;

    private final Map<Character, Character> characters;

    // private constructor
    SingletonClassA() {
        Map<Character, Character> aCharMap = new HashMap();
        aCharMap.put('a', 'e');
        aCharMap.put('o', 'u');
        // in order to keep short I erased other puts.
        characters = aCharMap;
        counter++;
    }

    public boolean containsKey(char letter) {
        return characters.containsKey(letter);
    }
}

Your use case then becomes:

SingletonClassA.INSTANCE.containsKey(aLetter);


Summing integers in stream using Fenwick tree In short, the input is an ordered list of numbers, and we have to sum the numbers from index a to b quickly. For example, assume array[] = {1, 2, 3}. Please sum from array[0] to array[2] (1 + 2 + 3).

Because the input array can have as much as 200,000 elements, I used a Fenwick tree to save time, but the time limit is still exceeded on UVa.

Here is the problem on UVa.

Which part can I improve?

I built:

lowbit() return low bit for fenwick tree
create() create the fenwick tree
update() update the fenwick tree
sum() return the sum by fenwick tree

update :

add Arrays.fill(FT, 0); to initialize Fenwick tree
create() use update()
add array[x] = y; to update the actual array

import java.util.*;

public class Main {

    static int[] FT = new int[200001];    /* store fenwick tree */
    static int[] array = new int[200001]; /* store input data */
    static int N; /* size of input data  */

    public static void main(String[] args) {

        Scanner in = new Scanner(System.in);

        int testCase = 0; /* not important, just for UVa output format*/
        int x, y;

        while ((N = in.nextInt()) > 0) {

            Arrays.fill(FT, 0); /* Initialize Fenwick tree */

            for (int i = 1; i <= N; i++) { /* Initialize input array */
                array[i] = in.nextInt();
            }

            create();

            System.out.printf("Case %d:\n", ++testCase);

            String act;
            while (!(act = in.next()).equals("END")) { /* receive action: print sum, update data or END */
                if (act.equals("M")) { /* to print sum from x to y*/
                    x = in.nextInt();
                    y = in.nextInt();
                    System.out.println(sum(y) - sum(x - 1));
                } else { /* to update the array[x] to y ,also fenwick tree*/
                    x = in.nextInt();
                    y = in.nextInt();

                    update(x, y - array[x]);
                    array[x] = y;
                }
            }
        }
    }

    public static void create() {
        for (int i = 1; i <= N; i++) {
            update(i, array[i]);
        }
    }

    public static void update(int i, int delta) {
        for (int j = i; j <= N; j += lowbit(j)) {
            FT[j] += delta;
        }
    }

    public static int sum(int k) {
        int ans = 0;
        for (int i = k; i > 0; i -= lowbit(i)) {
            ans += FT[i];
        }
        return ans;
    }

    public static int lowbit(int k) {
        return -k & k;
    }
}

Incorrect creation of fenwick tree

Your function create() is both incorrect and slow.  You had the correct function update() but you never used it.  Instead you tried to create the fenwick tree yourself but used an \$O(n^2)\$ algorithm to do so.  The correct way would have been just:

public static void create() {
    for (int i = 1; i <= N; i++) {
        update(i, array[i]);
    }
}

This would have created the fenwick tree in \$O(n \log n)\$ time.

Bug

Another problem you have is when you update the array.  You do correctly update the fenwick tree, but you forgot to update the actual array.  In other words, after this line:

               update(x, y - array[x]);

you need this line:

                array[x] = y;


Finding an object in a nested object/array structure I have an app with a bunch of questions defined in a yaml file. For various reasons I would like the data to be divided into sections (section_0, section_1, section_2 etc.). When I load this file into my js app I get an object that looks like this:

const questionData = {
  "section_0": {
    "items": [
      {
        "key": "0.0",
        "title": "Question one",
        "type": "integer"
      },
      {
        "key": "0.1",
        "title": "Question 0.1",
        "type": "integer"
      }
    ]
  },
  "section_1": {
    "items": [
      {
        "key": "1.0",
        "title": "Some title",
        "type": "integer",
        "options": {
          "1": "one",
          "2": "two",
          "3": "other option"
        }
      },
      {
        "key": "1.1",
        "title": "Question 1.1",
        "description": "Longer description text",
        "type": "string"
      },
      {
        "key": "1.2",
        "title": "Question 1.2",
        "type": "integer",
        "options": {
          "0": "option zero",
          "1": "option one",
          "2": "option two"
        }
      },
      {
        "key": "1.3",
        "title": "Q 1.2",
        "type": "boolean",
        "options": {
          "true": "Yes",
          "false": "No"
        }
      },
      {
        "key": "1.4",
        "title": "Question 1.4",
        "type": "string"
      },
      {
        "key": "1.5",
        "title": "Question 1.5",
        "type": "integer"
      },
      {
        "key": "1.6",
        "title": "Question 1.6",
        "type": "integer"
      },
      {
        "key": "1.7",
        "title": "Q1.7",
        "type": "integer"
      }
    ]
  }
}

(a lot more questions and sections in the actual app)

Now I would like to be able to query for a question by the key. I found/figured out two ways to do this. One is sort of home made and the other flattens the array using lodash.

Traversing the object:

function findByKey(data, key){
    if (data.hasOwnProperty('key') && data['key'] === key) {
      return data;
    }

    for (let i = 0; i < Object.keys(data).length; i++) {
      if (typeof data[Object.keys(data)[i]] === 'object') {
        let obj = findByKey(data[Object.keys(data)[i]], key);
        if (obj != null) {
          return obj;
        }
      }
    }

    return null;
}

findByKey(questionData, "0.1")
// => returns the object with the key "0.1"

Using lodash to create a flat array of objects:

const questionsArray = _.flatten(_.map(_.keys(questionData), function(val) {
  return questionData[val].items;
}));

_.find(questionsArray, function(obj) { return obj.key === "0.1" })
// => returns the object with the key "0.1"

Do I need to transform the questionData object like this just to be able to query for an object somewhere in the tree or am I missing some simpler solution (other than not creating the nested structure in the first place)?

I usually have methods to normalize data in the format I want it so you don't have to iterate again.
If you want you can have Schema that provides you methods or access to the data using custom methods.

var questions = new QuestionsSchema(data); // Normalizes to preferred format    questions.getOriginal(); // => Access to the original object
questions.getQuestion('0.1');    // =>      {"key": "0.0","title": "Question one","type": "integer"}
questions.getSection('section_1'); // => whole section

I like more the composed functions you build, is a more like functional way.


Testing after_save hooks in Rails 4.2 with MiniTest I've recently started working on a new application in Rails 4.2.0-beta2. I've been using this as an opportunity to learn more about MiniTest, with a stretch goal of being as strict with myself with testing as possible.

My general problem: How do I test the after_save action for an ActiveRecord model in such a way that I can be confident that an ActiveRecord callback will not cause my code to break?

Here's what I've got so far (gist or inline code below).

How would you (re)write these files to detect if the NotificationMailer called in the after_save hook prevented a TeamMembership from being persisted?

Note that not included here are the relevant test fixtures. You may safely assume that fixtures users(:carol) and teams(:alpha) are not associated with each other.

app/models/team_membership.rb

class TeamMembership < ActiveRecord::Base
  # A proc that will enqueue `NotificationMailer.team_invitation`
  DEFAULT_NOTIFIER = proc do |user, team|
    NotificationMailer.team_invitation(team, user).deliver_later
  end

  class << self
    # This is a class level attribute that is mainly used for testing.
    # Defaults to {TeamMembership::DEFAULT_NOTIFIER}
    attr_accessor :notifier
  end
  self.notifier = DEFAULT_NOTIFIER

  belongs_to :team
  belongs_to :user

  after_create :invite_user_to_team!

  private

  # ActiveRecord callback used to equeue team invitation emails
  # @return void
  def invite_user_to_team!
    self.class.notifier.call(team, user)
    nil
  end
end

tests/models/team_membership_test.rb

require 'test_helper'

class TeamMemberTest < ActiveSupport::TestCase
  test 'callbacks' do
    # setup
    test_notifier = Minitest::Mock.new
    test_notifier.expect(:call, nil, [teams(:alpha), users(:carol)])
    TeamMembership.notifier = test_notifier

    # test
    TeamMembership.create(user_id: users(:carol).id, team_id: teams(:alpha).id)
    assert test_notifier.verify

    # teardown
    TeamMembership.notifier = TeamMembership::DEFAULT_NOTIFIER
  end
end

A valid approach would be check if NotificationMailer.team_invitation was called. In order to get this job done, you'll need first of all change this NotificationMailer.team_invitation hard-coded call to something injected. Something like this:

class TeamMembership < ActiveRecord::Base
  # other methods

  after_create :invite_user_to_team!

  def notification_method(notification_service = NotificationMailer)
    @notification = notification_service
  end

  private
  def invite_user_to_team!
    @notification.team_invitation(team, user)
    nil
  end
end

Now, you can create an expectation over NotificationMailer. Inside your test, you can define your mock and define its behavior:

class TeamMemberTest < ActiveSupport::TestCase
  def test_after_create_new_team_member_it_should_be_notified
    notification = Minitest::Mock.new
    notification.expects(:team_invitation).with(team, user).once
    # your TeamMember class working to create new
  end
end

I hope it helps :)


String reverse and pairing reversed words These are questions from an interview:

Reverse a string
Find matching anagrams in a word list

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

namespace ConsoleApplication2
{
public class ReverseString
{
    public ReverseString()
    {
        string str = "Gilad";
        string res = Reverse(str);

        List<string> wordsList = new List<string>();
        wordsList.Add("batel");
        wordsList.Add("Gilad");
        wordsList.Add("daliG");
        wordsList.Add("enon");
        wordsList.Add("none");
        wordsList.Add("letab");
        var pairs = PairAnagramWords(wordsList);
    }

    public string Reverse(string str)
    {
        StringBuilder s = new StringBuilder();
        for (int i = str.Length - 1; i >= 0; i--)
        {
            s.Append(str[i]);
        }
        return s.ToString();
    }

    Dictionary<string, string> PairAnagramWords(List<string> wordsList)
    {
        wordsList.Sort();
        Dictionary<string, string> res = new Dictionary<string, string>();
        foreach (var item in wordsList)
        {
            if (res.ContainsValue(item))
            {
                continue;
            }

            foreach (var item2 in wordsList)
            {
                if (item == item2 || item.Length != item2.Length)
                {
                    continue;
                }
                var reversed = Reverse(item2);
                int i;
                for (i = 0; i < item.Length; i++)
                {
                    if (item[i] != reversed[i])
                    {
                        break;
                    }
                }
                if (i == item.Length)
                {
                    res.Add(item, item2);
                }
            }
        }
        return res;
    }
}

}

I don't like the fact that I'm doing it in \$ O(n^2) \$, although I am checking for no repetitions.

Please review my code's complexity and algorithm. I did the testing inside the constructor just for convenience, although I usually will use unit tests.

Reverse

Your Reverse function is functional, but there are some improvements I could suggest:

if you use the StringBuilder approach (which is OK), then you should at least initialize the size of the internal capacity so that resizes are not needed later (for performance reasons). Use StringBuilder s = new StringBuilder(str.Length);
While looking at that, why have the variable name s ... that's a poor choice.
Consider using a char[] array for the reverse as it reduces some more overheads (again, performance). StringBuilders are useful for many things, but their biggest benefit comes from their dynamic nature. In this case, that nature is not needed.

If it was me, I would have:

    public string Reverse(string forward)
    {
        char[] chars = forward.ToCharArray();
        var last = forward.Length - 1;
        for (int i = last / 2; i >= 0; i--)
        {
            char temp = chars[i];
            chars[i] = chars[last - i];
            chars[last - i] = temp;
        }
        return new String(chars);
    }

Matching Anagrams

Your code is not matching anagrams. Your code matches words with the reverse of the word. An anagram is any rearranged form of the word. So, you have batel and lateb in your list, but it should also match table. Your code will not do that.

The trick to solving this problem would be to reduce each word to its sorted array of characters. For example, to have a function like:

    public string SortedString(string word)
    {
        char[] chars = word.ToCharArray();
        Array.Sort(chars);
        return new String(chars);
    }

Or, if Linq is your thing:

        return String.Concat(word.OrderBy(c => c));

With the char-sorted words, you can use the sorted version as the key of a dictionary, something like:

Dictionary<string,List<string>> anagrams = ....

And you can then populate that dictionary with lists of words that have the same sorted-char anagram key.

Once you have that, you can return all words that are anagrams of each other.


Procedural generation of an infinite map made up of 1's and 0's This is my function to procedural generate an infinite random map of 1's and 0's it only calculates what you can see at the time and when the view moves it calculates the next rows.

Complexity is the size of the view that is on the screen e.g. if complexity = 7 then the view on the screen is a grid 7x7.

It works fine but how can I tidy this up, simplify the code and make it more elegant?

void CreateMap(){
    if(complexity%2==0) {
        complexity--;
    }

    if (randomseed) {
        levelseed=Time.time.ToString();
        Debug.Log ("Random");
    }

    psuedoRandom = new System.Random(levelseed.GetHashCode());
    offsetx=psuedoRandom.Next();
    levellocation=psuedoRandom.Next();
    offsety=psuedoRandom.Next();
    level = new int[complexity,complexity];
    for (int x=0;x<complexity;x++){
        for (int y=0;y<complexity;y++){
            level[x,y]=RandomReturn(x+offsetx,y+offsety);
        }
    }
}

int RandomReturn(int x, int y){
    psuedoRandom = new System.Random(x*y*levellocation.GetHashCode());
    return (psuedoRandom.Next (0,2));
}

An example of moving right

void MoveRight(){
    offsetx++;
    for (int x=0;x<complexity-1;x++){
        for (int y=0;y<complexity;y++){
            level[x,y]=level[x+1,y];
        }
    }

    for (int y=0;y<complexity;y++){
        level[complexity-1,y]=RandomReturn(complexity-1+offsetx,y+offsety);
    }
}

I know that this is minor, but you should not stuff all your code together without spaces like:

for (int y=0;y<complexity;y++){
    level[complexity-1,y]=RandomReturn(complexity-1+offsetx,y+offsety);

Because spaces help differentiate tokens at a first glance, I suggest one space on each side of each operator.


Measuring Execution Times I was wondering which time.time() of from datetime import timedelta was the quickest and best way to find how long a programme had been running for example.

import time
start = time.time()

#do stuff

print(start - time.time())

or (although longer)

from datetime import datetime
from datetime import timedelta

start_time = datetime.now()

def millis():
   dt = datetime.now() - start_time
   ms = (dt.days * 24 * 60 * 60 + dt.seconds) * 1000 + dt.microseconds / 1000.0
   return ms
def tickscheck(start):
    x = 0
    count = 0
    while millis() - start < 1000:
        x = 4+5
        #counting up
        count = count + 1
        print("It Took " + str(count) + " Counts\nOver " + str(millis()- start) + "ticks")

running = True
while(running == True):
    tickscheck(millis())

In Python 3.3 and later you can use one of

  time.perf_counter()
  Return the value (in fractional seconds) of a
  performance counter, i.e. a clock with the highest available
  resolution to measure a short duration. It does include time elapsed
  during sleep and is system-wide. The reference point of the returned
  value is undefined, so that only the difference between the results of
  consecutive calls is valid.

  time.process_time() Return the value (in fractional seconds) of the
  sum of the system and user CPU time of the current process. It does
  not include time elapsed during sleep. It is process-wide by
  definition. The reference point of the returned value is undefined, so
  that only the difference between the results of consecutive calls is
  valid.

Older Python versions offer timeit.default_timer(), which is now an alias of time.perf_counter().

The timeit module offers tools to get more reliable timings by running the code multiple times.


Idiomatic F# unit test using xunit and unquote I'm writing some unit tests for testing C# code but I'm using F# and Unquote for the first time for the unit tests and would like some input as to how I might make the tests more 'f-sharpier' to take advantage of and learn more about the functional approach in F#. Here's an example of one of my tests - please make recommendations:

[<Fact>]
let ``Two courses, one with one time and one with two times that all differ, returns two unique schedules with two entries each``() =
    // arrange
    let course1 = getCourse1()
    let course2 = getCourse2()
    let time1 = getTime1()
    let time2 = getTime2()
    let time3 = getTime3()
    time1 |> course1.AddTime
    time2 |> course2.AddTime
    time3 |> course2.AddTime

    let courses = List<Course>()
    [|course1; course2 |] |> courses.AddRange

    let scheduleEntry1 = ScheduleEntry(course1, time1)
    let scheduleEntry2 = ScheduleEntry(course2, time2)
    let scheduleEntry3 = ScheduleEntry(course2, time3)

    // act
    let schedules = courses |> ScheduleGenerator.Schedulize

    // assert
    test <@ schedules.Count() = 2 @>

    let allSchedulesHave2Entries = schedules.All(fun schedule -> schedule.Count() = 2)
    test <@ allSchedulesHave2Entries @>

    let hasSchedule entries = schedules.Any(fun schedule -> schedule.ToArray() = entries)
    test <@ hasSchedule [| scheduleEntry1; scheduleEntry2 |] @>
    test <@ hasSchedule [| scheduleEntry1; scheduleEntry3 |] @>

ScheduleGenerator.Schedulize returns an IEnumerable<IEnumerable<ScheduleEntry>> which is the type of schedules. So the type of schedule is just IEnumerable<ScheduleEntry>.

What ScheduleGenerator.Schedulize does is take a set of courses IEnumerable<Course>, where each course contains all available times for the course, and computes the Cartesian product for all the times across all the courses and then filters out any schedule which contains conflicting times.

I guess I mostly needed to figure out how to replace LINQ with F# equivalents and then refactor a bit. Now I have this:

let ToArrays schedules =
    schedules
    |> Seq.map (fun schedule -> schedule |> Seq.toArray)
    |> Seq.toArray

[<Fact>]
let ``Two courses, one with one time and one with two times that all differ, returns two unique schedules with two entries each``() =
    // arrange
    let course1 = getCourse1()
    let course2 = getCourse2()
    let time1 = getTime1()
    let time2 = getTime2()
    let time2a = getTime2a()
    time1 |> course1.AddTime
    time2 |> course2.AddTime
    time2a |> course2.AddTime

    let schedulesExpected =
        [|
            [| ScheduleEntry(course1, time1); ScheduleEntry(course2, time2) |]
            [| ScheduleEntry(course1, time1); ScheduleEntry(course2, time2a) |]
        |]

    // act
    let schedulesActual = [| course1; course2 |] |> ScheduleGenerator.Schedulyze

    // assert
    test <@ schedulesActual |> ToArrays = schedulesExpected @>


Migrating HTML elements I have this code which migrates HTML elements to the word chunks by bracketing each element. Here, chunks is the list of word chunks to be processed and dom is to access the given HTML source. This returns a list of processed word chunks.

Could you suggest any improvements to this :

def _migrate_html(self, chunks, dom):
    elements = self._get_elements_list(dom)
    for element in elements:
      result = []
      index = 0
      concat_chunks = []
      for chunk in chunks:
        if (index + len(chunk.word) <= element.index or
            element.index + len(element.text) <= index):
          result.append(chunk)
        elif (index <= element.index and
              element.index + len(element.text) <= index + len(chunk.word)):
          result.append(Chunk(
              chunk.word.replace(element.text, element.source),
              HTML_POS, HTML_POS, True))
        elif (index < element.index + len(element.text) and
              element.index + len(element.text) <= index + len(chunk.word)):
          concat_chunks.append(chunk)
          new_word = u''.join([c_chunk.word for c_chunk in concat_chunks])
          new_word = new_word.replace(element.text, element.source)
          result.append(Chunk(new_word, HTML_POS, HTML_POS, True))
          concat_chunks = []
        else:
          concat_chunks.append(chunk)
        index += len(chunk.word)
      chunks = result
    return chunks

result points to the variable being the actual result being returned, but it's actually the current state of processing. Even new_chunks would be a better name.
Some expressions like index + len(chunk.word) or element.index + len(element.text) repeat themselves, and would be better as named variables describing their meaning. This also makes advancing the index inside the for more readable.

index_after_chunk = index + len(chunk.word)
index_after_element = element.index + len(element.text)
Advancing the index: index = index_after_chunk

You should be using Python's chained comparisons for the last if:

if index < index_after_element <= index_after_chunk:

EDIT:

On further examination it seems you're actually checking for intersection of index ranges. Let's say we have this:

from collections import namedtuple
class IndexRange(namedtuple('IndexRange', ['start', 'length'])):
  @property
  def end(self): return self.start + self.length

  # more helper methods here

chunk_range = IndexRange(start=index, length=len(chunk.word))
element_range = IndexRange(start=element.index, length=len(element.text))

You can now cleanly express the logic as (I'm inventing methods here but you get the point):

if chunk_range.disjoint_from(element_range):

first case, ranges are disjoint

elif chunk_range.fully_contains(element_range):

second case, element_range is fully contained in chunk_range

elif chunk_range.contains_point(element_range.end):

third case, element_range starts before chunk_range and ends inside_chunk_range

else:

final case, element_range starts inside chunk_range and ends after chunk_range OR element_range fully contains chunk_range

If I got the cases right, then

You should at least add them as comments explaining what each branch means
Possibly justify the final case in the comment
Possibly reorder the cases to better express your meaning. You don't have to rely on elif, it's not a big deal if you re-test something you've tested before as long as it's readable and encapsulated in a helper method or function.


Simple string reverse program in C++ I want to learn how to write good code. Starting off with simple programs for now. So any suggestions regarding style, readability or more efficient ways of implementation, etc. would really be appreciated!

#include <algorithm>
#include <iostream>
using namespace std;

int main(int argc, char** argv)
{
  bool running = true;

  while(running == true)
  {
    string input;
    cout << "[Enter 'exit' to exit the program]\n";
    cout << "Enter the text you want to reverse: ";
    getline(cin, input);

    if (input.compare("exit") == 0)
    {
      running = false;
      break;
    }

    reverse(input.begin(), input.end());
    cout << "The reversed version of your text is: " << input << endl << endl;
  }
  return 0;
}

Don't using namespace std;

It is considered bad practice because of possible name collisions, ... Although nobody will hurt you, as this is such a small program :)

Don't compare booleans to booleans

Things like running == true are completely unnecessary, as running is already a condition in itself. Just use running.

You don't need running

The variable running is unnecessary. You need one for nested loops (breaking out of both), but for single loops, a single break and an infinite loop is preferred.

Why are you using compare?

Seeing input.compare("exit") == 0 would send me right to the docs, as I don't know what the return value of compare is.

if (input == "exit");

is more clear.

Don't flush 2 times in a row (or at all)

std::endl prints a new line, and then flushes stdout. This results in a performance hit, so it is better to output an actual newline. In your case:

std::cout << something << "\n\n";

Note that on some platforms (or if you need to), you have to flush the steam to see the output. In this case it is better to be explicit and use std::flush.

You're not using the command line parameters

So why name them? You can just omit the names, or define a main which takes no parameters:

int main(int, char**) {} //1)
int main() {} //2)

I prefer option 2.

Your code uses implementation specific behavior

std::string is defined in the string header. Not every compiler includes string with iostream or algorithm (VS doesn't), so you have to include it explicitly. Don't rely on automatic includes.

Technically, you don't need return 0;

Only for main, if you omit return 0;, the compiler will add it for you, so technically, you don't need to specify it.


Sorting multiple vectors based on a reference vector I am trying to sort vectors based on a reference vector. First I am getting the indexes for the reference vector and then using that I am doing inplace sorting for the rest of the vectors.

#include <vector>
#include <cassert>
#include <iostream>
#include <algorithm>

using iter = std::vector<int>::iterator;
using order_type = std::vector<std::pair<size_t, iter>>;

void create_permutation(order_type &order)
{
        struct ordering {
                bool operator ()(std::pair<size_t, iter> const& a, std::pair<size_t, iter> const& b) {
                        return *(a.second) < *(b.second);
                }
        };

        std::sort(order.begin(), order.end(), ordering());
}

void reorder(std::vector<int> &vect, order_type index)
{
        for (int i=0; i< vect.size(); i++)
        {
                while (index[i].first != i)
                {
                        int  old_target_idx  = index[index[i].first].first;
                        int old_target_v  = vect[index[i].first];

                        vect[index[i].first] = vect[i];
                        index[index[i].first].first = index[i].first;

                        index[i].first = old_target_idx;
                        vect[i]   = old_target_v;
                }
        }
}

template<typename... argtype>
void sort_from_ref(argtype... args);

template<typename T, typename A, typename... argtype>
void sort_from_ref(order_type order, std::vector<T,A> &vect, argtype &&...args)
{
        reorder(vect, order);
        sort_from_ref(order, std::forward<argtype>(args)...);
}

template<>
void sort_from_ref(order_type order, std::vector<int> &vect) {
        reorder(vect, order);
}

int main()
{
        size_t n = 0;
        std::vector<int> vect_1{ 1, 0, 2, 3 };
        std::vector<int> vect_2{ 100, 200, 300, 400 };
        std::vector<int> vect_3{ 100, 200, 300, 400 };
        std::vector<int> vect_4{ 400, 200, 3000, 4000 };
        std::vector<int> vect_5{ 500, 200, 360, 400 };
        order_type order(vect_1.size());

        for (iter it = vect_1.begin(); it != vect_1.end(); ++it) {
                order[n] = std::make_pair(n, it);
                n++;
        }
        create_permutation(order);
        sort_from_ref(order, vect_2, vect_3, vect_4, vect_5);

        {
                std::vector<int> test{200, 100, 300, 400};
                assert(vect_2 == test);
        }
        {
                std::vector<int> test{200, 100, 300, 400};
                assert(vect_3 == test);
        }
        {
                std::vector<int> test{200, 400, 3000, 4000};
                assert(vect_4 == test);
        }
        {
                std::vector<int> test{200, 500, 360, 400};
                assert(vect_5 == test);
        }

        return 0;
}

For another take on the same problem (sorting parallel arrays), see Quicksort template (for sorting corresponding arrays).

Your code is strangely organized. I would expect it to have a single entry point, something like this:

template<class Vector, class... Vectors>
void parallel_sort(Vector& keyvector, Vectors&... vectors) {
    std::vector<size_t> order(keyvector.size());
    std::iota(order.begin(), order.end(), 0);
    std::sort(order.begin(), order.end(), [&](size_t a, size_t b) {
        return keyvector[a] < keyvector[b];
    });
    (reorder(keyvector, order) , ... , reorder(vectors, order));
}

(Hey, look at that! It's a complete implementation, except for the reorder function you already wrote!)

But instead you require the calling code to set up their own order vector manually, then call create_permutation, then sort_from_ref, in that order. That's a lot of steps. At the very least, you could provide a function create_order_vector to simplify that first step.

The one advantage I can see to requiring the caller to provide the order vector is that it allows your library code to do no heap allocations. Vice versa, a problem with my code above is that it requires O(n) memory to sort n elements. If I have a million-element vector, I need to heap-allocate another eight million bytes to sort it! That could be annoying or prohibitive for some callers — they might not expect a sorting function to throw std::bad_alloc. At least by making the caller allocate the eight million bytes themselves, you're putting the issue front and center where they can't miss it.

I think it's strange that you define the alias order_type to be std::vector<std::pair<size_t, iter>>. I would rather see

using order_type = std::pair<size_t, iter>;

[...] std::vector<order_type> order;

Again, I guess it comes down to my wanting to see std::vector-ness explicitly in the code. I have the same complaint about typedefs such as using Foo = foo*; that hide a type's pointer-ness.

template<typename T, typename A, typename... argtype>
void sort_from_ref(order_type order, std::vector<T,A> &vect, argtype &&...args)
{
        reorder(vect, order);
        sort_from_ref(order, std::forward<argtype>(args)...);
}

In C++17, you can use a comma-operator fold-expression to do this, as I did above. But even in C++11, you can use a pack expansion to do this without all the "recursive template" stuff. (See "Iteration is better than recursion.") You'd just do something like this:

template<class... Args>
void sort_from_ref(order_type order, Args&... args)
{
    int dummy[] = {
        [&]() { reorder(args, order); return 0; } ...
    };
}

Notice that I also changed argtype to Args. It's a pack of multiple argtypes, not just one; and the C++ convention for template parameters is to CamelCase them.

I also eliminated your perfect forwarding. We know that all our args are going to be non-const lvalue references; so we should just say so.

We also know that all our args are going to be std::vector<T> for some T, because that's the only kind of argument that will be accepted by our reorder template. Therefore, it is incorrect (but largely harmless) that you wrote

template<typename T, typename A, [...]>
void sort_from_ref([...] std::vector<T,A> &vect, [...]

because there is only one possible A that can go there and have the code still compile. You should just have said

template<typename T, typename... argtype>
void sort_from_ref(order_type order, std::vector<T>& vect, argtype&&... args)

(except that, as we've seen, you don't need this "recursive" template at all).

No comment on your reorder function; that part is all math. (Does it work? Did you test it exhaustively?)

Okay, two comments...

                    index[i].first = old_target_idx;
                    vect[i]   = old_target_v;

Your whitespace here (and other places) is wonky. It looks like maybe you were trying to align the = signs, but failed? I strongly recommend not trying to align anything, ever. It's not robust against refactoring. (For example, if you change the name of a variable, now you have to realign everything it touched.)

for (int i=0; i< vect.size(); i++)

You should be getting a stupid warning from your compiler about "comparing signed and unsigned" here. If you're not, then turn up your warning levels — you should be using -W -Wall (and maybe -Wextra) on non-MSVC compilers, and I'd say -W4 on MSVC. The traditional workaround would be to make i a size_t instead of an int.


Calculate the closest point to many hyperbolic paraboloids In this question I asked for a way to compute the closest projected point to a hyperbolic paraboloid using python.

Using the iterative approximation answer, I'm able to use the code below to calculate the closest point to multiple paraboloids.

import numpy as np
np.set_printoptions(suppress=True)

# This function calculate the closest projection on a hyperbolic paraboloid
# As Answered by @Jaime https://stackoverflow.com/questions/18858448/speeding-up-a-closest-point-on-a-hyperbolic-paraboloid-algorithm

def solve_uv(p0, p1, p2, p3, p, tol=1e-6, niter=100):
    a = p1 - p0
    b = p3 - p0
    c = p2 - p3 - p1 + p0
    p_ = p - p0
    u = 0.5
    v = 0.5
    error = None
    while niter and (error is None or error > tol):
        niter -= 1

        u_ = np.dot(p_ - v*b, a + v*c) / np.dot(a + v*c, a + v*c)
        v_ = np.dot(p_ - u*a, b + u*c) / np.dot(b + u*c, b + u*c)
        error = np.linalg.norm([u - u_, v - v_])
        u, v = u_, v_
    return np.array([u, v])

# Generate random hyperbolic paraboloids
COUNT = 1000
p0 = np.random.random_sample((COUNT,3))
p1 = np.random.random_sample((COUNT,3))
p2 = np.random.random_sample((COUNT,3))
p3 = np.random.random_sample((COUNT,3))
p = np.random.random_sample(3)

iterated = np.empty((COUNT,2))
for i in xrange(COUNT):
    iterated[i,:] = solve_uv(p0[i], p1[i], p2[i], p3[i], p)

This works fine, but obviously doesn't scale well and becomes very slow when dealing with large data sets.

To remedy this I made some changes to vectorize the function. It will stop iterating until all the given items pass the tolerance test or reach the maximum iteration count. I also use inner1d from numpy.core.umath_tests instead of np.dot and np.linalg.norm because in my tests inner1d performs better with large data sets.

import numpy as np
from numpy.core.umath_tests import inner1d

def solver_vectorized(p0, p1, p2, p3, p, tol=1e-6, niter=100):
    a = p1 - p0
    b = p3 - p0
    c = p2 - p3 - p1 + p0
    p_ = p - p0
    u  = 0.5 * np.ones((p0.shape[0],1))
    v  = 0.5 * np.ones((p0.shape[0],1))

    # Init index list of elements to update
    mask = np.arange(p0.shape[0])

    while niter and mask.shape[0]:
        niter -= 1

        avc = a[mask] + v[mask]*c[mask]
        buc = b[mask] + u[mask]*c[mask]
        u_ = (inner1d(p_[mask] - v[mask]*b[mask], avc) / inner1d(avc,avc))[:,np.newaxis]
        v_ = (inner1d(p_[mask] - u[mask]*a[mask], buc) / inner1d(buc,buc))[:,np.newaxis]

        # Calculate delta and update u,v
        delta = np.hstack([u[mask]-u_, v[mask]-v_])
        delta = inner1d(delta,delta)**0.5
        u[mask], v[mask] = u_, v_

        # Update the index list only with items which fail the tolerance test
        mask = mask[np.where(delta > tol)]

    # Return uv's
    return np.hstack([u, v])

vectorized = solver_vectorized(p0, p1, p2, p3, p)
print np.allclose(iterated,vectorized) #Returns: True

Question

On very large data sets (over 100,000 items) the vectorized version will solve in ~1.5 seconds. But I am sure there are some inefficiencies that could be improved. For example the square root could probably be dropped for a small performance increase. Or by using masks differently. How would you change the function to improve its performance?

How would you change the function to improve its performance?

I agree that using masks differently could prove fruitful. Here is my suggestion. Currently the tolerance is a constant. Suppose we substitute it with cur_tol which starts out as e.g. 100 * tol, and change the loop exit criterion to (niter and mask.shape[0] AND cur_tol <= tol)? Then perform cur_tol /= 2 in each iteration. The idea is to have mask reference fewer elements during early updates, and for each iteration to simultaneously get closer to the answer AND include a potentially larger number of input values that will drive us closer to that answer. So we are lazy about evaluating a smaller number of points when we clearly are far from the answer, and cast a wider net as we approach closer to it.


Downloads historical data from a yahoo finance csv and returns a pandas DataFrame Not too long ago, yahoo-finance shut down its public API for downloading historical data. There are however works around and the below code is on of them. I started looking for solutions of how to grab the correct cookie and crumb pair and then implemented that into it's own function.

I am new to Pandas and don't know the best practices for accessing data and creating DataFrames. Are there any ways that I can make this project more efficient?

This project can be downloaded here https://github.com/AndrewRPorter/yahoo-historical

import datetime as dt
import pandas as pd
import requests
import re
import csv
import time

class Fetcher:
    def __init__(self, ticker, start, *args):
        self.ticker = ticker.upper()
        self.cookie, self.crumb = self.init()

        self.start = int(time.mktime(dt.datetime(start[0],start[1],start[2]).timetuple()))

        if args:
            end = args[0]
            self.end = int(time.mktime(dt.datetime(end[0],end[1],end[2]).timetuple()))
        else:
            self.end = int(time.time())

    def init(self):
        """Returns a tuple pair of cookie and crumb used in the request"""
        url = 'https://finance.yahoo.com/quote/%s/history' % (self.ticker)
        r = requests.get(url)
        txt = r.content
        cookie = r.cookies['B']
        pattern = re.compile('.*"CrumbStore":\{"crumb":"(?P<crumb>[^"]+)"\}')

        for line in txt.splitlines():
            m = pattern.match(line.decode("utf-8"))
            if m is not None:
                crumb = m.groupdict()['crumb']
        return cookie, crumb  # return a tuple of crumb and cookie

    def getHistorical(self):
        """Returns a list of historical data from Yahoo Finance"""
        url = "https://query1.finance.yahoo.com/v7/finance/download/%s?period1=%s&period2=%s&interval=1d&events=history&crumb=%s" % (self.ticker, self.start, self.end, self.crumb)
        data = requests.get(url, cookies={'B':self.cookie})
        content = data.content.decode("utf-8")
        csv_content = csv.reader(content.splitlines(), delimiter=',')
        return pd.DataFrame(list(csv_content))

    def getDatePrice(self):
        """Returns a DataFrame for Date and Price from getHistorical()"""
        return self.getHistorical().ix[0:,5]

    def getDateVolume(self):
        """Returns a DataFrame for Date and Volume from getHistorical()"""
        return self.getHistorical().ix[0:,6]

You could be using pandas DataFrame.read_csv method, instead of manually going through csv.reader first (you need to go through StringIO, though...):

try:
    from io import StringIO
except ImportError:
    # python 2.x
    from StringIO import StringIO
...

class Fetcher:
    ...

    def getHistorical(self):
        """Returns a list of historical data from Yahoo Finance"""
        data = requests.get(self.url, cookies={'B':self.cookie})
        content = StringIO(data.content.decode("utf-8"))
        return pd.DataFrame.read_csv(content, sep=',')

If you didn't need the cookie, you could just do the following, alas you can't get around that part...

    def getHistorical(self):
        """Returns a list of historical data from Yahoo Finance"""
        return pd.DataFrame.read_csv(self.url, sep=',')

I would also expect a class called Fetcher to accept an url parameter to determine where it is fetching from (hence I already used self.url above). Since it is some arcane API url here, I would make it at least a class variable that can be modified if needed in a sub-class or on-the-fly:

class Fetcher:
    api_url = "https://query1.finance.yahoo.com/v7/finance/download/%s?period1=%s&period2=%s&interval=1d&events=history&crumb=%s"

    def __init__(self, ticker, start, *args):
        self.ticker = ticker.upper()
        self.cookie, self.crumb = self.init()

        self.start = int(time.mktime(dt.datetime(start[0],start[1],start[2]).timetuple()))

        if args:
            end = args[0]
            self.end = int(time.mktime(dt.datetime(end[0],end[1],end[2]).timetuple()))
        else:
            self.end = int(time.time())
        self.url = self.api_url % (self.ticker, self.start, self.end, self.crumb)

    ...

if __name__ == "__main__":
    fetcher = Fetcher(...)

I also added a section for you to actually call the code, guarded by a if __name__ == "__main__": guard.


Google Code Jam - Store Credit Below is my answer to the Google Code Jam's answer to the Store Credit. As usual the code run's perfectly well, however, my credit_pos function gives a warning when returning the temp array..

  You receive a credit C at a local store and would like to buy two
  items. You first walk through the store and create a list L of all
  available items. From this list you would like to buy two items that
  add up to the entire value of the credit. The solution you provide
  will consist of the two integers indicating the positions of the items
  in your list (smaller number first).

  Input

  The first line of input gives the number of cases, N. N test cases
  follow. For each test case there will be:

  One line containing the value C, the amount of credit you have at the store.
  One line containing the value I, the number of items in the store.
  One line containing a space separated list of I integers. Each integer P indicates the price of an item in the store.

  Each test case will have exactly one solution.

Link to the question

#include <iostream>
#include <fstream>
#include <cstdlib>
#include <vector>

int * credeit_pos(std::vector<int> v, int n, int credit);

int main(int argc, char ** argv) {

    std::ifstream infile(argv[1]);
    if(!infile) {
        std::cout << "could not open " << infile <<std::endl;
        exit(EXIT_FAILURE);
    }
    std::ofstream outfile (argv[2]);
    if(!outfile) {
    std::cout << "could not open " <<outfile << std::endl;
        exit(EXIT_FAILURE);
    }

    int num_cases, i=0, n_items, credit;
    infile >> num_cases;
    while(i < num_cases){
        infile >> credit;
        infile >> n_items;
        int price;
        std::vector<int> prices;
        for(int j=0; j<n_items; j++){
            infile >> price;
            prices.push_back(price);
        }
        int * loc = credeit_pos(prices, n_items, credit);
        outfile <<"Case #"<< i+1 <<": " << loc[0] <<" "<< loc[1] << std::endl;
        i++;
    }
}

int * credeit_pos(std::vector<int> prices, int n_items, int credit){
    int  temp[2];
    for(int i=0; i<n_items-1; i++){
        for(int j=i+1; j<n_items; j++){
            if(!(prices[i] + prices[j] == credit))
                ;
            else {
                temp[0] = i + 1, temp[1] = j + 1;
            }
        }
    }
    return temp;
}

Leaving alone an UB (which is already addressed), let's look at an actual solution, the credeit_pos function.

  From this list you would like to buy two items that add up to the entire value of the credit.

In a programming terms: given a range, find two values which add up to the given value.

The proposed solution is

for each elements in a range,
    try to find a peer adding up to a target

It is correct but an implementation has quadratic complexity. It feels wrong. Every time you work with a linear range and end up with a quadratic algorithm, ask yourself if it can be improved if the range was sorted (sort is cheap comparing to quadratic). In this case it can be indeed: finding (or failing to find) a peer in a sorted range is a binary search (\$\log(n)\$), which drives an overall complexity to \$n \log(n)\$.

Summing up, the template is

std::sort(std::begin(prices), std::end(prices));
for (auto it = std::begin(prices); it != std::end(prices); ++it) {
    auto peer = std::upper_bound(it, std::end(prices), target - *it);
    if (*it + * peer == credit) return std::make_pair(it, peer);
 }

PS: since the problem statement guarantees that the solution exists, I didn't bother to check that peer != end(prices).

PPS: the second phase could be completed in a linear time, but due to presence of sort it doesn't matter here.


Kohana newbie: setting a layout view and variables for layout in before() The same view (a layout) used for all pages in Kohana3.3 project. That's why it could be placed in before() method instead of every action. Right?

Is there any better way to optimize the code below so you shouldn't use so many $this->? Are there any other improvements of this code so it looks really good?

public function before()
{   //shows layout for all pages
    $this->view = View::factory('layout');
    $this->view->left_column='pass this string to layout';
}

public function action_index()
{   //index section on the page
    $this->view->content = View::factory('index');
    $this->view->content->text = 'some index text';
    $this->response->body($this->view);
}

public function action_about()
{
    //about section on the page
    $this->view->content = View::factory('about');
    $this->view->content->text = 'some about text';
    $this->response->body($this->view);
}

Your code is violating the "Don't Repeat Yourself" (DRY) Principle, which is why it seems like you have "too many" $this->* lines. As the name implies, your code should not repeat. Usually, in an instance such as this, I would say to use the constructor and inject those values you need; However, I don't know for sure if the class you provided is also a factory, in which case you would use a wrapper method.

//non-factory method
public function __construct( $view, $text ) {
    $this->view->content = View::factory( $view );
    $this->view->content->text = $text;
    $this->response->body( $this->view );
}

//no other methods need, simply inject while instantiating

//factory method
public function render( $view, $text ) {
    $this->view->content = View::factory( $view );
    $this->view->content->text = $text;
    $this->response->body( $this->view );
}

public function action_index() {
    $this->render( 'index', 'some index text' );
}


Finding and deleting a certain word I have a working macro that finds a specific word and deletes the whole text. It finds the word "JOBSPECIALTYCODE" in the sheet, selects and deletes it, then moves to the next sheet. I just want to ask if there is any alternative way to make my macro work fast. I only use a record macro.

Sub aHaveProgess()
'
'
'
'

'Deleting Trash Text to make it clear

Selection.Find.ClearFormatting
Selection.Find.Replacement.ClearFormatting
With Selection.Find
    .Text = "JOBSPECIALTYCODE"
    .Replacement.Text = ""
    .Forward = True
    .Wrap = wdFindContinue
    .Format = False
    .MatchCase = False
    .MatchWholeWord = False
    .MatchByte = False
    .CorrectHangulEndings = False
    .HanjaPhoneticHangul = False
    .MatchAllWordForms = False
    .MatchSoundsLike = False
    .MatchWildcards = False
    .MatchFuzzy = False
End With

Do While Selection.Find.Execute
    Selection.SelectCell
    Selection.Delete Unit:=wdCharacter, Count:=1
    Selection.Find.ClearFormatting
    Selection.Find.Replacement.ClearFormatting
Loop
End Sub

ScreenUpdating

In order for you to interact with a spreadsheet (e.g. moving the mouse, selecting a cell) , the display on your monitor is re-drawn something like 30+ times per second. This takes a lot of processing power.

If you set Application.ScreenUpdating = False then, while it is false, the screen will not be updated, and your code will run much faster.

Crucially though, make sure it is set back to True before your code ends. Else your code will finish running, but your user will think that Excel has frozen because the screen won't respond to their mouse.

Make sure that before any End/Exit statements, or if errors are thrown, that you change the setting back.

Like so:

Public Sub DoThing()

    Application.ScreenUpdating = False

    On Error Goto CleanFail

    ...

    ...

    Code

    ...

    ...

CleanExit:

    Application.ScreenUpdating = True

    Exit Sub

CleanFail:

    On Error Goto 0

    '/ Handle errors here

    Resume CleanExit

End Sub

And now, assuming you put some kind of error handling in, your sub will always exit through CleanExit, which will always reset ScreenUpdating back to normal.


cartesian product of two vector spaces A few days back I found an interesting problem which reads the following:
Given two vector spaces generate the resulting set of its cartesian product.
\begin{gather}
\text{Let: } \mathcal{V}, \mathcal{W} \text{ be vector spaces}\\
\mathcal{V} \times \mathcal{W} = \{ (v,w) \mid v \in \mathcal{V} \land w \in \mathcal{W} \}
\end{gather}

Hint 1: A vector space is a set of elements called vectors which accomplishes some properties
Hint 2: Design the solution for finite vector spaces
Tip 1: It is recommended to use structures
Constraint: You are forbidden to use any stl class

I solved this problem with the next approach:
struct vector_pair
{
    double *vector_a;
    double *vector_b;
    size_t a_dimension;
    size_t b_dimension;
};

struct cartesian_product_set
{
    vector_pair *pairs;
    size_t pairs_number;
};

cartesian_product_set vector_spaces_cartesian_product(double **space_v, size_t v_vectors,
    size_t v_dimension, double **space_w, size_t w_vectors, size_t w_dimension)
{
    cartesian_product_set product_set{new vector_pair[v_vectors * w_vectors], v_vectors * w_vectors};

    for (size_t i = 0, j, k = 0; i < v_vectors; i++)
        for (j = 0; j < w_vectors; j++)
            product_set.pairs[k++] = vector_pair{space_v[i], space_w[j], v_dimension, w_dimension};

    return product_set;
}

How could I improve this code if possible?
Thank you.

const-correctness
use references in favor of pointers where possible
The fact that you leave the obligation to free the memory that you allocate to the caller is generally not a good practice
a common pattern in your code is that you have pointers to arrays and their length - why not make a structure to bundle them up?
try to make use of iterators and range-based-for-loops when you don't really need the index (which you don't in your example)
since we don't really care about the type of the elements in a vector space you could use templates to generalize your algorithm

And just to see if it would be possible, I tried to come up with a compile-time version of the algorithm:
template<typename T>
struct pair
{
    T first;
    T second;
};

template<std::size_t N, typename T>
struct cvp
{
    pair<T> pairs[N];
};

template <typename T, size_t NV, size_t NW>
auto get_cvp(const T (&vs)[NV], const T (&ws)[NW])
{
    cvp<NV*NW, T> result;
    auto it_pairs = std::begin(result.pairs);
    for (const auto v : vs) {
        for (const auto w : ws) {
            *(it_pairs++) = {v, w};
        }
    }
    return result;
}

you can try the code here: https://godbolt.org/z/e8GvEf


1-D intersection of lines given begin and end points I have a fairly basic question here: I want to find if two lines on a 1-D plane intersect. I know of two simple ways to solve this, but I wanted to know if Python has a more elegant way to solve this.

x = [1, 10] # 1 = begin, 10 = end
y = [15, 20]
z = [5, 12]

#Method 1: Works. Is quick. Lots of typing.
def is_intersect_1(a, b):
    bool_check = False
    if a[0] <= b[0] <= a[1] or \
    a[0] <= b[1] <= a[1] or \
    b[0] <= a[0] <= b[1] or \
    b[0] <= a[1] <= b[1]:
        bool_check = True
    return bool_check

is_intersect_1(x,y) # False
is_intersect_1(x,z) # True

#Method 2: Quicker to write. Simpler to read. Uses more memory and is slower.

def is_intersect_2(a, b):
    bool_check = False
    if set(range(a[0], a[1]+1)).intersection(set(range(b[0], b[1])):
        bool_check = True
    return bool_check

is_intersect_2(x,y) # False
is_intersect_2(x,z) # True

Using explicit booleans is a common antipattern:

check = False
if condition:
    check = True
return check

is equivalent to much more transparent (and preferable)

return condition

Segments do not intersect if a lies completely to the left, or completely to the right of b. Assuming that segments are themselves sorted:

def is_intersect_3(a, b):
    return not (a[1] < b[0] or a[0] > b[1])

is_intersect_2 fails badly on floats


TamperMonkey script to add a button to Jira The below script detects whether we are in Jira, and if it is run in Jira it will add a button. Clicking that button will use the REST API of Jira to create an issue, and navigate to the recently created issue.

// ==UserScript==
// @name         Tell Jira I did a thing
// @namespace    http://demuyt.net/
// @version      0.1
// @description  try to take over the world!
// @author       Tom J Demuyt
// @match        https://jira*
// @match        https://jira.it.aenetworks.com/*
// @grant        none
// ==/UserScript==

//Documentation
//https://docs.atlassian.com/software/jira/docs/api/REST/7.12.0/
//https://developer.atlassian.com/server/jira/platform/jira-rest-api-examples/

(function() {
    'use strict';

    let userName = "";
    let projectCode = "CF";
    let fixVersion = "15484";

    //The id of the create button is `create_link`, dont show up if that button is not there
    let createButton = document.getElementById('create_link');
    //Leave quietly if we cannot find that button
    if(!createButton){
        return;
    }

    //Get the parent (
) of the
 that holds the
    let createButtonParent = createButton.parentNode.parentNode;

    //Add a button for the user
    var node = document.createElement("LI");
    node.innerHTML = '

Log activity

';
    createButtonParent.appendChild(node);

    //Get a reference to the button
    let idat = document.getElementById("idat");

    //Make it listen to clicks;
    idat.addEventListener("click", recordTheThing);
    //Try and build a ticket
    function recordTheThing(){
        //Prep a call to Jira
        let xhr = new XMLHttpRequest(),
            url = '/rest/api/2/issue/';
        //Ask what the user did
        var thing = window.prompt("Describe the activity performed?","");
        //Get out if the user got cold feet
        if(!thing){
            return;
        }
        //Start prepping the http request
        xhr.open('POST', url, true);
        //Use JSON since we will not fill in a form
        xhr.setRequestHeader("Content-Type", "application/json");
        //Prep the navigation to the new issue
        xhr.onreadystatechange = function () {
            debugger;
            //readyState:3
            //response: {"id":"125529","key":"CF-10282","self":"https://jira.it.aenetworks.com/rest/api/2/issue/125529"}
            //HTTP/REST return code 201 means created
            if (xhr.readyState === 4 && xhr.status === 201) {
                var json = JSON.parse(xhr.responseText);
                window.location.href = 'https://jira.it.aenetworks.com/browse/' + json.key;
            }
        };
        //The heart of the thing
        var data = JSON.stringify({
            "fields": {
                "project":
                {
                    "key": projectCode
                },
                "summary": thing,
                "description": thing,
                "issuetype": {
                   "name": "Task"
                },
                "assignee": {
                   "name": userName
                },
                //15484 -> Busines As Usual
                "fixVersions": [{ "id": fixVersion}],
            }
        });
        //Finalize the REST call
        xhr.send(data);

    }//Record the thing

    //Find from the load/start the user id
    let xhr = new XMLHttpRequest(),
    url = '/rest/api/2/mypermissions/';
    //Prep to extract the username from the call header
    //200 -> OK
    xhr.onreadystatechange = function () {
      if(xhr.readyState == 4 && xhr.status == 200){
        userName = xhr.getResponseHeader("X-AUSERNAME");
      }
    }
    //Shoot and pray
    xhr.open('GET', url, true);
    xhr.send();
})();

Please review for cleanliness, readability, standards, the usual..

The code is well commented. The indentation in most places seems to be four spaces but the last few lines have indentation of two spaces so that could be made more consistent. The last comment is a bit humorous...

It seems that if the button isn't found by the id attribute, it just bails. There could be alternative ways to find that button. For example, my company recently upgraded to version 7.11.2 and it appears that the create button still has an id attribute of create_link but the list item above it also has an id attribute of create-menu so that element could be found instead. And the unordered list also has a class name of aui-nav so if the create button can't be found the list could be found instead.  I also see in the latest online version 1001.0.0 (e.g. *.atlassian.net) that the navigation menu has been moved to the left side of the screen and no longer uses list items so if your Jira gets updated you'll need to update your approach to finding that element.

I know you are familiar with const given your answers like this one so why not use it for things like projectCode, idat?

Also, a list item element is created and then the innerHTML contains another list item element:

var node = document.createElement("LI");
node.innerHTML = '

Log activity

';

This yields HTML like below:

Log activity

MDN's documentation for
 claims "It must be contained in a parent element: an ordered list (
), an unordered list (
), or a menu (<menu>)"1 and that the permitted content is Flow Content, which does not include list item elements.  But obviously our browsers render it as expected. Ideally that list item would not contain a child list item...

You could consider using the fetch API instead of classical XHR requests unless browser compatibilty is an issue.

And since you are using ecmascript-2015 features like let, you could consider using more arrow functions...

1https://developer.mozilla.org/en-US/docs/Web/HTML/Element/li


Unix tail command using stack I'm working on a version of the Unix tail command that works by iterating over a file, pushing line by line into a stack. Finally, the number of desired lines will be popped and printed to the screen. I have used void * (void pointers) as the data field of my stack nodes (the stack is basically a linked list with limitations) and due to that I had to do a bit of work with pointer casting and successive allocations with malloc() (one chunk per line). I wonder if there isn't a better (as in simpler, more readable and less memory intensive) way of accomplishing the same goal.

Main tail.c file

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "../utils/type_definitions.h"
#include "../stack/stack_list/stack_list.h"
#define LINE_SIZE 256

int main(int argc, char *argv[])
{

  int number_of_lines;
  if (argc == 2) {
    number_of_lines = atoi(argv[1]);
  } else {
    printf("%s\n", "Error!");
    exit(1);
  }

  FILE *f;
  if (!(f = fopen("file.txt", "r"))) {
    printf("%s\n", "Error opening file!\n");
    exit(1);
  };

  // pushing lines into stack

  int i = 0;
  char *line, *temp = (char*)malloc(LINE_SIZE);
  Node *stack = stack_new();

  while (fgets(temp, sizeof(line), f) != NULL) {
    line = (char*)malloc(LINE_SIZE);
    strcpy(line, temp);
    stack_push((void*)line, &stack);
  }

  // poping and printing n lines to screen

  for (int i = 0; i < number_of_lines; i++) {
    printf("%s\n",(char*)stack_top(stack));
    stack_pop(&stack);
  }

  return 0;
}

Stack header file

#ifndef __STACK_H__
#define __STACK_H__

#include <stdbool.h>
#include "../../utils/type_definitions.h"

Node *stack_new();
void *stack_top(Node *head);
void stack_pop(Node **head);
bool stack_is_empty(Node *head);
void stack_push(void *x, Node **head);

#endif

Implementation of stack functions

Node *stack_new() {
    return NULL;
}

bool stack_is_empty(Node *head) {
    return head == NULL;
}

void *stack_top(Node *head) {
    return head->data;
}

void stack_pop(Node **head)
{
    if (*head == NULL) return;
    Node *temp = *head;
    *head = (*head)->next;
    free(temp);
}

void stack_push(void *x, Node **head)
{
    Node *new_node = (Node*)malloc(sizeof(Node));
    new_node->data = x;
    new_node->next = *head;
    *head = new_node;
}

Edit - following are the includes missing from the file "implementations of stack functions".

#include "stack_list.h"
#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>

The stack approach is dubious. It uses too much memory (the entire file is eventually collected, while you only care about its tail). Consider a ring buffer instead.
I don't see the point of #include <stdbool.h>
Casting is unnecessary, as in (char*)stack_top(stack) - and even dangerous, as in (Node*)malloc(sizeof(Node)). Why you shouldn't cast malloc.
Always test that malloc doesn't return NULL.
A declaration

int i = 0;

is unnecessary.
Nitpicking. #define LINE_SIZE 256 is a very optimistic estimate. Try to manage a file with really long lines.


GridItem and GridContainer This library provides methods for storing items into an inventory. the container is based on a grid as well as the items going into that container.

can you please review the code in any way?

public interface GridContainer {
    public Collection<GeoPoint> getContainer();
    public Collection<GridShape> getContent();
    public boolean fitsInside(GridShape container, GeoPoint location);
    public void add(GridShape shape, GeoPoint location);
    public GridShape remove(GeoPoint location);
}

public interface GridShape {
    public boolean fitsInside(GridContainer container, GeoPoint pointer);
    public Collection<GeoPoint> getShape();
    public GeoPoint getPointer();
    public Collection<GeoPoint> getRelativeShape(GeoPoint point);
}

so far we have an implementation for rectangle shaped containers and items

public class RectangleContainer implements GridContainer {

    private final List<GeoPoint> shape;
    private final Map<GeoPoint, GridShape> content;

    public RectangleContainer(int width, int height){
        shape = Collections.unmodifiableList(RectangleUtility.createShape(width, height));
        content = new HashMap<>();
    }

    @Override
    public Collection<GeoPoint> getContainer() {
        return shape;
    }

    @Override
    public Collection<GridShape> getContent() {
        return content.values();
    }

    @Override
    public boolean fitsInside(GridShape shape, GeoPoint location) {
        Collection<GeoPoint>relativeShape = shape.getRelativeShape(location);
        boolean hasIntersections = hasIntersection(relativeShape);
        boolean isInBounds = isInBounds(relativeShape);
        return !hasIntersections && isInBounds;
    }

    private boolean isInBounds(Collection<GeoPoint>relativeShape) {
        for (GeoPoint point: relativeShape){
            if (!shape.contains(point)){
                return false;
            }
        }
        return true;
    }

    private boolean hasIntersection(Collection<GeoPoint>relativeShape) {
        for (Map.Entry<GeoPoint, GridShape> entry: content.entrySet()){
            for (GeoPoint pointOfContent: entry.getValue().getRelativeShape(entry.getKey())){
                if (relativeShape.contains(pointOfContent)){
                    return true;
                }
            }
        }
        return false;
    }

    @Override
    public void add(GridShape shape, GeoPoint location) {
        content.put(location, shape);
    }

    @Override
    public GridShape remove(GeoPoint location) {
        return content.remove(location);
    }
}

public class RectangleItem<I> implements GridShape, ItemHolder<I> {

    private final List<GeoPoint> shape;
    private I item;

    public RectangleItem(int width, int height){
        shape = Collections.unmodifiableList(RectangleUtility.createShape(width, height));
    }

    @Override
    public boolean fitsInside(GridContainer container, GeoPoint pointer) {
        return container.fitsInside(this, pointer);
    }

    @Override
    public Collection<GeoPoint> getShape() {
        return shape;
    }

    @Override
    public GeoPoint getPointer() {
        return shape.get(0);
    }

    @Override
    public Collection<GeoPoint> getRelativeShape(GeoPoint point) {
        return shape.stream().map(p -> new GeoPoint(p.getX()+point.getX(), p.getY() + point.getY())).collect(Collectors.toList());
    }

    @Override
    public I getItem() {
        return item;
    }

    @Override
    public void setItem(I item) {
        this.item = item;
    }
}

Note: i've skipped the test classes and the RectangleUtility class, the geoLib project  and the ItemHolder since they are not scope of the review. If they are required please feel free to add a comment asking for them, then i'll add them. (i think this question is already bloated, that's the reason for deciding so)

General

Task

In my opinion you lost the focus of your task description:

   [...]storing items into an inventory. The container is based on a grid as well as the items going into that container.

The word container can easily be replaced by inventory:

   [...] storing items into an inventory. The inventory is based on a grid as well as the items going into that inventory.

The goal is to store items in an Inventory/Container but currently it stores Shapes instead of Items:

  public class RectangleContainer implements GridContainer {

  /* ... */
  private final Map<GeoPoint, GridShape> content;

  /* ... */

  @Override
  public boolean fitsInside(GridShape shape, GeoPoint location) { /* ... */ }

  /* ... */

  @Override
  public void add(GridShape shape, GeoPoint location) { /* ... */ }

}

What is an Item

 What an Item currently is

  public class RectangleItem<I> implements GridShape, ItemHolder<I> { /* ... */}

To make it simpler to discuss:

  public class Item<I> implements Shape, ItemHolder<I> { /* ... */}

Is Item an Item? When I see only the class name I would say yes, but after looking on the signature I would say: "It could be".
From the signature it is an item that is a shape that holds an item.
Is it an item that holds an other item?

What an Item should be

A Item is something that has a Shape.

  public class Item<I> {

    private I value;
    private Shape shape;

    public Item(I value, Shape shape) { /* ... */ }

}

and an ìtem should be packed into an inventory/container:

public class RectangleContainer implements GridContainer {
    /* ... */
    @Override    public void add(Item item, GeoPoint location) {/* ... */}
}

Code Smell

Feature Envy

Inside RectangleContainer#fitsInside(GridShape shape, GeoPoint location) is the following line:

Collection<GeoPoint> relativeShape = shape.getRelativeShape(location);

The relativeShape gets passed into hasIntersection and isInBounds of RectangleContainer:

  @Override
public boolean fitsInside(GridShape shape, GeoPoint location) {
  Collection<GeoPoint> relativeShape = shape.getRelativeShape(location);
  boolean hasIntersections = hasIntersection(relativeShape);
  boolean isInBounds = isInBounds(relativeShape);
  return !hasIntersections && isInBounds;
}

Because RectangleContainer works with the internals of shape (relativeShape) it is a feature envy.

You could return a custom object PositionedItem that has the methods intersectsNotInside(inventory) and isInBoundOf(inventory):

@Override
public boolean fitsInside(Item item, GeoPoint location) {
   PositionedItem positionedItem = shape.at(location);
   return !positionedItem.intersectsNotInside(this) && positionedItem.isInBoundOf(this);
}

The benefit will be that you will not have duplicate logic for hasIntersections and isInBounds in the different subtypes of GridContainer.

Redundant Public

In both interfaces GridContainer and GridShape you have used the key word public. Per default are fields in an interface are public.

Also valid java:

public interface GridShape {
    boolean fitsInside(GridContainer container, GeoPoint pointer);
    Collection<GeoPoint> getShape();
    GeoPoint getPointer();
    Collection<GeoPoint> getRelativeShape(GeoPoint point);
}

Everything is addable

  @Override
public void add(GridShape shape, GeoPoint location) {
   content.put(location, shape);
}

I could add a shape, that is 1000 times bigger as a RectangleContainer..

The adding of a shape should be checked so that it is not possible to at wrong shapes