Palindromic partitions solution Task

Find the smallest number of cuts required to break a string into a set of valid palindromes.
For example:

ababbbabbababa becomes a|babbbab|b|ababa (cuts = 3)
partytrapb becomes partytrap|b (cuts = 1)

Note: all strings of length 1 are palindromes as a single character reads the same forward and backward

import re
def find_paritions(lst, word, running=[]):
    for (index,item) in enumerate(lst):
        if index > 0:
            running.pop()
        running = running + [item]
        if item[1] == len(word):
            complete_paths.append(running)
            return
        to_explore = []
        end = item[1]
        for item_next in list_parition_index:
            if item_next[0] == end:
                to_explore.append(item_next)
        if len(to_explore) == 0:
            return
        find_paritions(to_explore,word,running)

    return complete_paths

complete_paths=[]
word = "ababbbabbababa"
start = 0
end = 1
parition_count =0
list_parition_index = []
for start in range (len(word)):
    end=start+1
    while end < len(word) + 1:
        if word[start:end] == word[start:end][::-1]:
            list_parition_index.append([start,end])
            end +=1
        else:
            end +=1
list_zeroes = [x for x in list_parition_index if x[0] == 0]

x = find_paritions (list_zeroes,word)
print(f"The total number of divisions required is {len(min(x,key=len))-1}")
print(min(x,key=len))

You definitely want to take a dynamic programming approach to this problem, as explained by Justin, if only to make the code more readable. I've included a solution based on this approach at the end.

Structure

I found the code fairly difficult to read due to the way it is structured. In its present form it wont be very re-usable, due to the use of global variables and the lack of any overall encapsulation.

For example, the following section, which finds all palindromic substrings, should be encapsulated in a function. And, it would be best for find_partitions to somehow make its own call to this function, rather than to rely on a result stored in a global variable.

for start in range (len(word)):
    end=start+1
    while end < len(word) + 1:
        if word[start:end] == word[start:end][::-1]:
            list_parition_index.append([start,end])
            end +=1
        else:
            end +=1

Correctness

I wasn't able to determine the correctness of the program from inspection, but it appears to produce correct answers, except for inputs that are already palindromes (i.e. require zero cuts), in which case it throws an error.

Also, there's a subtle problem with way you've specified the default value for running.

Efficiency

Again, I wasn't able to fully analyse the efficiency by inspection. However it seems to perform about as well as my solution.

Style

Redundancy:

You import re but don't use it
Initialising start and end isn't necessary
This section

if ...:
    ...
    end +=1
else:
    end +=1

could be replaced with

if ...:
    ...
end +=1

On naming variables:

I think paritions is a spelling mistake. Did you mean partitions?
Some of the names could be made more descriptive, such as lst, item, item_next, and running.

The use of whitespace is a little bit inconsistent. I would make the following changes based on the python style guide:

(index,item) -> (index, item)
find_paritions(to_explore,word,running) -> find_paritions(to_explore, word, running)
complete_paths=[] -> complete_paths = []
parition_count =0 -> parition_count = 0
range (len(word)) -> range(len(word))
[start,end] -> [start, end]
end=start+1 -> end = start + 1
end +=1 -> end += 1
find_paritions (list_zeroes,word) -> find_paritions(list_zeroes, word)
min(x,key=len) -> min(x, key=len)

It wasn't clear to me that running = running + [item] served the role of duplicating running before appending item, so I ended up correcting it to running.append(item) which of course broke the code. Maybe a comment here would be worthwhile, or this statement could be written in such a way that it draws attention to the duplication (e.g. new_running = running + [item], then you wouldn't have to use pop on each successive iteration).

For comparison, here is a solution that uses dynamic programming:

def least_palin_cuts(string, memo=None):
    """Return the minimum number of cuts required to break string into
    palindromic substrings.

    >>> least_palin_cuts('ababbbabbababa') # a|babbbab|b|ababa
    3
    >>> least_palin_cuts('partytrapb') # partytrap|b
    1
    >>> least_palin_cuts('kayak') # kayak (no cuts needed)
    0
    """
    if memo is None:
        memo = dict()
    if string not in memo:
        if is_palindrome(string):
            memo[string] = 0
        else:
            memo[string] = min(
                least_palin_cuts(left, memo) + 1 + least_palin_cuts(right, memo)
                for (left, right) in splits(string)
            )
    return memo[string]

def is_palindrome(string):
    """Return True if string is a palindrome, else False."""
    return all(c1 == c2 for (c1, c2) in zip(string, reversed(string)))

def splits(string):
    """Generate each way of splitting string into two non-empty substrings.

    >>> list(splits('abc'))
    [('a', 'bc'), ('ab', 'c')]
    """
    for i in range(1, len(string)):
        yield (string[:i], string[i:])


Finding the minimum number of required deletions to have a non-repeating string I wrote code for the following problem:

  Given a string, print out the number of deletions required so that the adjacent alphabets are distinct.

Please suggest different methods by which I can increase the speed of execution.

tst = int(input())
for i in range(0,tst):
    str = input()
    length = len(str)
    str = list(str)
    j=0;
    count = 0
    while(j<length):
       if(j+1<length):
            if((str[j] == str[j+1])):
                del str[j+1]
                count+=1
                length-=1
            else:
                j+=1
       else:
            j+=1
    print(count)

Here's your culprit:

del str[j+1]

When you remove one character from a string, all subsequent characters need to be shifted into the hole that you create.  That changes your algorithm from O(Length) to a worst-case scenario of O(Length2), if every character is the same.

Additionally, your inner loop looks a lot like C code.  Here is one way to reformulate it.

def count_consecutive_deletions(s):
    deletions = 0
    for i in range(1, len(s)):
        if s[i] == s[i - 1]:
            deletions += 1
    return deletions

def testcases():
    for _ in range(int(input())):
        yield input()

for case in testcases():
    print(count_consecutive_deletions(case))


Performing several integer division operations on an array Program:

  Given an array A of size N. Given Q operations, each operation
  contains an integer D. In each operation you have to divide all the
  elements of the array by D.

  For example, for each operation with a given D, the new array A would
  be: Finally, after processing all the operations you have to print the
  final array after Q operations. Note: The result of the each division
  will be an integer, for example 5 / 2 = 2

  Input:

  First line of input contains a single integer N denoting number of
  elements in the array A. Next line of input contains N space separated
  integers denoting the elements of array A. Next line contains Q
  denoting number of operations. Next Q lines contains a single integer
  D by which divide the elements of the array.

  Output:

  Print single line containing N space separated integers after
  processing Q operations.

  Sample Input

5
50 20 18 27 19
3
2
3
2

  Sample Output

4 1 1 2 1

  Explanation

  In operation 1 after dividing the whole array by D=2, the resultant
  array will be: [25, 10, 9, 13, 9] In operation 2 after dividing the
  array from operation 1 by 3, the resultant array will be: [8, 3, 3, 4,
  3] In operation 3 after dividing the array from operation 2 by 2, the
  resultant array will be: [4, 1, 1, 2, 1] So, the resultant array will
  be [4, 1, 1, 2, 1]

Solution:

import java.io.BufferedReader;
import java.io.InputStreamReader;
import java.util.Arrays;

public class TestClass1 {

    public static void main(String[] args) {
    try{
         BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
         System.out.println("Enter No of element in Array:-");
         String line = br.readLine();
         int N = Integer.parseInt(line);
         int a[]=new int[N];
         System.out.println("Now Enter Array elemnt seperated by space :-");
           String line2= br.readLine();
           String s[]=line2.split(" ");
           for(int p=0;p<s.length ;p++){
               a[p]=Integer.parseInt(s[p]);
           }
           System.out.println("Now Enter No of operations :-");
           int d=Integer.parseInt(br.readLine());
           System.out.println("Now enter all the operationis elemnt one by one:- ");
           int t[]=new int[d];
            for(int i=0;i<d;i++){
                t[i]=Integer.parseInt(br.readLine());
            }
            System.out.println(Arrays.toString(t));
            for(int j=0;j<d;j++){
                for(int k=0;k<N;k++){
                    a[k]=a[k]/t[j];
                }
            }
           System.out.print(Arrays.toString(a));
    }catch(Exception ex){
        ex.printStackTrace();
    }
    }

}

Correctness. Technically the program is incorrect. The assignment requires the result to be printed once. Your solution prints out the state of array after each division.
Optimization. Since you only need to print the result once, you don't have to preform an actual division on each query. Accumulate the product of divisors, and perform the division once at the last step. Certain care to avoid overflow is necessary.

Most likely the program statement included restrictions on the input data. It would be nice to see them.


Is this the best way to reverse a string in C (not in place)? Here is my code. Is there anyway to make it more efficient?

/*
IN: string to have characters reversed
OUT: string containing characters in reversed order
*/
char* reverStr(const char *str)
{
        char* revStr = (char*)malloc(strlen(str));
        int i;
        for(i = strlen(str)-1; i >= 0; i--)
            revStr[strlen(str)-1-i] = str[i];
        return revStr;
}

What about this:

/*
IN: string to have characters reversed
OUT: string containing characters in reversed order
*/
char* reverStr(const char *str)
{
        int index=strlen(str);
        char* revStr = (char*)malloc(index--);
        int destIndex=0;
        while (0<=index)
            revStr[destIndex++] = str[index--];
        revStr[destIndex]=0;
        return revStr;
}


Optimise Division function using divide and conquer, (and multiplication only) I have the following code I am working on, but I think it can be optimised further probably by improving the min/max variable selection, (see below).

The idea is to not use actual division, ('/'), to get the quotient and remainder.

I think there could be a way to improve performance by further minimising the number of loops.

NB The idea is not to have a function faster than Num/Den but rather to have the function running as efficiently as possible.

void DivisionDivideAndConquer(const unsigned int Num, const unsigned int Den, unsigned int& Quo, unsigned int& Rem)
{
  //  short cuts
  if (Num < Den)
  {
    Quo = 0;
    Rem = Den;
    return;
  }

  // R = N - D * Q where 0 <= R < |D|

  // getting us started
  unsigned int min = 0;
  unsigned int max = Num;

  for (;;)
  {
    unsigned int number = static_cast<unsigned int>((min + max) * 0.5);
    int possibleRemainder = (Num - (Den * number));

    // if too small then we have a new max
    if (possibleRemainder < 0) {
      if (number == max) {
        max = max - 1;
      }
      else {
        max = number;
      }
    }
    // too big we have a new min
    else if(possibleRemainder >= static_cast<int>(Den) )
    {
      if (number == max) {
        min = min + 1;
      }
      else {
        min = number;
      }
    }
    else
    {
      //  got it!
      Quo = number;
      Rem = static_cast<unsigned int>(possibleRemainder);
      break;
    }
  }
}

int main(int argc, char** argv)
{
  unsigned int Quo = 0;
  unsigned int Rem = 0;
  DivisionDivideAndConquer(12, 4, Quo, Rem); // Q=3, R=0

  // ...

  DivisionDivideAndConquer(12, 9, Quo, Rem); // Q=1, R=3

}

Before optimising your code, it is probably a good idea to write a few tests to ensure you don't break anything as you go. There is something quite good in your situation : your code is supposedly easy to test as you know the mathematical properties that your result should have : a = b * q + r with 0 <= r < b.

Writing something like :

std::cout << "j" << "*" << "q" << "+" << "r" << " = " << "t" << " (" << "i" << ")" << std::endl;
for (uint i = 2; i < 10; i++)
    for (uint j = 2; j < 10; j++)
    {
        uint q = 0;
        uint r = 0;
        DivisionDivideAndConquer(i, j, q, r);
        uint total = j * q + r;
        if (total != i)
                std::cout << j << "*" << q << "+" << r << " = " << total << " (" << i << ")" << std::endl;
    }

shows that there is something wrong in your result which is the first problem (Hint: the problem is in the shortcut). Trying to perform tests with j starting from 1 shows that there is another issue (Hint: a loop does not finish).

Actual optimisation

Think about the cases you are trying to handle or how you are trying to reduce the search range.

With the following code, you compute less in each iteration while making the search range smaller :

    // if too small then we have a new max
    if (possibleremainder < 0) {
        max = number - 1;
    }
    // too big we have a new min
    else if(possibleremainder >= static_cast<int>(den) )
    {
        min = number + 1;
    }
    else
    {
        //  got it!
        quo = number;
        rem = static_cast<uint>(possibleremainder);
        break;
    }


Largest prime factor function I have written a function which finds the largest prime factor of some number. This function works but the problem is that it is too slow. For instance, when I enter 600851475143 as a parameter, the process of finding largest prime factor lasts too long. How can I modify it so that it works faster?

class test {

static addArray(someArray, member) {
    for (var i = 0; i <= someArray.length; i++) {
        if (i == someArray.length) {
            someArray[i] = member;
            return someArray;
        }
    }
}
static someLength(someArray) {
    var i = 0;
    while (someArray[i] !== undefined) {
        var lastItem = i;
        i++;
    }
    return i;
}
static testPrime(i) {
    for (var k=2; k < i; k++) {
        if (i % k == 0) {
            return false;
        }
    }
    return true;
}
}

var primeArray = [];
function largestPrime(n) {
    for (var i=2; i < n; i++) {
        //var k = n / i;
        if (n % i == 0 && test.testPrime(i) == true) {
            test.addArray(primeArray, i);
            n == n / i;
        }
    }
    return primeArray[test.someLength(primeArray) - 1];
}

document.write(largestPrime(600851475143));

The algorithm can be improved. I took the liberty to use my existing Java, also as Python might be somewhat slower.

On the Python code: X == true should simply be X.

long largestPrimeFactor(long x) {
    long largest = 0;
    long n = x; // Remaining number where factors divided out.
    if (n % 2 == 0) {
        largest = 2;
        do {
            n /= 2;
        } while (n % 2 == 0);
    }
    for (int c = 3; c*c <= n; c += 2) {
        if (n % c == 0) {
            largest = c;
            do {
                n /= c;
            } while (n % c == 0);
        }
    }
    // Wrong: return largest == 0 ? x : largest;
    return n > 1 ? n : largest;
}

The loop searching for factors handles 2 before the loop and then loops from 3 with steps of 2.

    for (int c = 3; c*c <= n; c += 2) {

In every loop step, if c is a factor of n, n is divided by c as often as possible. In fact n has no longer smaller factors than c.

Hence c is a prime, and in fact the largest found prime upto then.

If after the loop no largest prime was found, the parameter itself is a prime.

As n in every step is divided by all smaller factors, you need only to loop while c <= n / c holds: the second c being the smallest co-factor of n for c. Together with += 2 this hugely decreases the number of steps.

Your extra test on the factor being prime is the largest slow-down, as it does not exploit the factor filtering till then.


A simple pocket calculator I was searching the internet for some Python exercises, and I've found a lab assignment from University of Toronto. Here's the major of it:

  Question 1.
  Welcome Message In the if __name__ == "__main__" block, write code that displays the following: Welcome to the calculator program. Current value: 0

  Question 2.
  Displaying the Current Value Write a function whose signature is display_current_value(), and which displays the current value in the calculator. In the if __name__ == "__main__" block, test this function by calling it and observing the output.

  Question 3.
  Addition Write a function whose signature is add(to_add), and which adds to_add to the current value in the calculator, and modifies the current value accordingly. In the if __name__ == "__main__" block, test the function add by calling it, as well as by calling display_current_value().
  Hint: when modifying global variables from within functions, remember to declare them as global.

  Question 4.
  Multiplication Write a function whose signature is mult(to_mult), and which multiplies the current value in the calculator by to_mult, and modifies the current value accordingly. In the if __name__ == "__main__" block, test the function.

  Question 5.
  Division Write a function whose signature is div(to_div), and which divides the current value in the calculator by to_div, and modifies the current value accordingly. In the if __name__ == "__main__" block, test the function. What values of to_div might cause problems? Try them to see what happens.

  Question 6.
  Memory and Recall Pocket calculators usually have a memory and a recall button. The memory button saves the current value and the recall button restores the saved value. Implement this functionality.

  Question 7.
  Undo Implement a function that simulates the Undo button: the function restores the previous value that appeared on the screen before the current one.

Here's my solution:

current_value = 0
memory = current_value
memo = [current_value]

def is_divisible(x, y):
    """Return whether x is divisible by y. """
    return x % y == 0

def update_list(lst, value):
    """Updates a list with an item. """
    lst.append(value)

def display_current_value():
    """Prints the current value of the calculator. """
    print('Current value:', current_value)

def add(to_add):
    """Adds a number to the current value of the calcalutor. """
    global current_value

    current_value += to_add

    update_list(memo, current_value)

def mult(to_mult):
    """Multiplies the current value of the calcalutor by a number. """
    global current_value

    current_value *= to_mult

    update_list(memo, current_value)

def div(to_div):
    """Divides the current value of the calcalutor by a number. """
    global current_value

    if is_divisible(current_value, to_div):
        current_value //= to_div
    else:
        current_value /= to_div

    update_list(memo, current_value)

def store():
    """Stores the current value. """
    global memory

    memory = current_value

def recall():
    """Recalls the saved value. """
    return memory

def undo():
    """Restores the previous value. """
    global memo

    if len(memo) >= 2:
        global current_value

        current_value = memo[-2]

        memo.pop(-2)

def main():
    print('Welcome to the calculator program.' )
    display_current_value()

    add(6)
    display_current_value()

    mult(2)
    display_current_value()

    div(3)
    display_current_value()

    for iteration in range(3):
        undo()
        display_current_value()

if __name__ == '__main__':
    main()

Is it possible to implement undo without dealing with other functions such as add? Is the code well-documented?

Please point out any bad practice or any bug I've made. Please point out how somethings can be done better. And please note that I'm a beginner and a hobbyist.

With your three globals, it's not clear what memo is from looking at it. Try previous_values instead. Additionally, populating the list with memory at the beginning makes me think they're related, which I'm not sure is actually the case.

boardrider suggests passing your globals as arguments to functions. I agree that this would be a good idea. Another approach would to just wrap it all in a Calculator class. Classes are good for managing state.

Your undo function is a bit weird:

def undo():
    """Restores the previous value. """
    global memo

    if len(memo) >= 2:
        global current_value

        current_value = memo[-2]

        memo.pop(-2)

The previous current_value remains in memory, which makes me not trust it. What happens if you repeatedly undo, then operate? I haven't stepped through to verify, but I suspect a bug.

I'd try this instead:

def undo():
    if len(memo) >= 2:
        global current_value
        memo.pop()
        current_value = memo[-1]

This would maintain the invariant that the current value is always the last item in the list.

My final comment I am much less unsure of, and is probably down to a matter of personal taste:

I'm not sure about this part of your division:

if is_divisible(current_value, to_div):
    current_value //= to_div
else:
    current_value /= to_div

Is this just to help prevent floating-point rounding errors? I think this is overcomplicating things without fully addressing the problem. What I mean is that I could contrive to multiply together two floating-point numbers that have the same value of integers, and it would yield another floating-point int-like number, which is what it looks like you're trying to avoid here. I just don't see value in worrying about this in one specific case.

As another idea, you could do something like if current_value == int(current_value): current_value = int(current_value) after each op.


Objects and instance variables in loops Is there anything wrong with doing this:

Public Class Form1
    Private Sub Form1_Load(ByVal sender As Object, ByVal e As System.EventArgs) Handles Me.Load
        For value As Integer = 0 To 1000000
            Dim p As New Person
            p.ID = value
            p.DoSomething()
            p = Nothing
        Next
    End Sub
    Public Class Person
        Dim _ID As Integer
        Public WriteOnly Property ID() As Integer
            Set(ByVal value As Integer)
                _ID = value
            End Set
        End Property

        Public Sub DoSomething()
            _ID = _ID + 1
            'Do more with ID
        End Sub
    End Class
End Class

As appose to this:

Public Class Form1
    Private Sub Form1_Load(ByVal sender As Object, ByVal e As System.EventArgs) Handles Me.Load
        For value As Integer = 0 To 1000000
            Person.DoSomething(value)
        Next
    End Sub
    Public Class Person
        Public Shared Sub DoSomething(ByVal id As Integer)
            id = id + 1
            'Do more with ID
        End Sub
    End Class

End Class

I have written some code similar to that in sample 1. I have read online that code 1 can cause problems when threading is involved, but I do not understand why? Are there any other problems with sample 1? Sample 1 meets my requirements better.

In sample1, as you call it, you have an instance field which you change. Therefore, the Person class you are using is mutable. Mutable classes are prominent to concurrency issues if a single instance is manipulated by multiple threads, unless explicit synchronization mechanism is properly involved.

In sample2, if you pass the value in the method, and you are not modifying the state of the Person class as you do your business logic, then the code is thread safe. No other thread can access the local variable id and modify it concurrently, since it is visible to the current thread only. So there is no need to apply a synchronization mechanism.

However, sample2 may not be appropriate enough for your concrete requirements. If you give more code clues on what happens in the method in question, then a more-accurate advice can be given.


Find the uncommon elements from two sets I am trying to find the uncommon elements from two sets in Java. Here is my way:

private void findUnCommon{

    Set<Integer> a = new HashSet<>(Arrays.asList(1, 2, 3, 4));
    Set<Integer> b = new HashSet<>(Arrays.asList(3, 4, 5, 6));
    // get all elements from set a and set b

    System.out.println("Before..");
    System.out.println("a is : " + a);
    System.out.println("b is : " + b);

    Set<Integer> result = new HashSet<>(a);
    result.removeAll(b);
    System.out.println("result is : " + result);

    Set<Integer> temp = new HashSet<>(b);
    temp.removeAll(a);
    System.out.println("temp is : " + temp);

    result.addAll(temp);
    System.out.println("Uncommon elements of set a and set b is : "
        + result);

    System.out.println("After..");
    System.out.println("a is : " + a);
    System.out.println("b is : " + b);
}

I have declared two extra sets.  Can this be improved?

Is this a method?  You can't just plop code anywhere in Java.

The name of the operation is symmetric difference, so you should probably call it that.

Here's a more compact implementation.

private Set<T> symmetricDifference(Set<T> a, Set<T> b) {
    Set<T> result = new HashSet<T>(a);
    for (T element : b) {
        // .add() returns false if element already exists
        if (!result.add(element)) {
            result.remove(element);
        }
    }
    return result;
}


Java - How to represent the result of an operation Introduction

In my Java application, I need to represent the result of an operation. This can either be OK or ERROR + an error message.

Background

I have a class to verify the integrity of a directory (e.g. to check that a few conditions hold). If it finds out the directory is corrupted, it should return a description of the error, such as:

Unexpected file - [filename].
Unexpected number of files - expected [X], actual [Y].

The callers of the class don't know anything about the directory and the conditions. They are only interested to know if the directory is OK or not, and in that case, to get an appropriate error message.

The Result class was intended as a way to transmit the results of the verification.

I chose not to throw exceptions as (to me) that seemed more convenient for reporting errors in the verification process. It seemed inappropriate to intermix those two situations (e.g. the directory was found invalid vs. an error occurred while verifying the directory).

The current solution

I have so far been able to come up with the following class.

public class Result
{
    private static enum State { OK, ERROR };

    private State state;
    private String errorMessage;

    private Result(State state, String errorMessage)
    {
        this.state = state;
        this.errorMessage = errorMessage;
    }

    public static Result ok()
    {
        return new Result(State.OK, null);
    }

    public static Result error(String errorMessage)
    {
        return new Result(State.ERROR, errorMessage);
    }

    public boolean isOK()
    {
        return (this.state == State.OK);
    }

    public String getErrorMessage()
    {
        return errorMessage;
    }
}

The client code then looks like this:

Result result = Result.ok();
Result result = Result.error("Could not ...");

if (result.isOK())
    ...
else
{
    String errorMessage = result.getErrorMessage();
    ...
}

This has the benefit that when creating a result, the client only has to deal with error messages in case of an error.

On the other hand, the API does not prevent the client code to ask for an error message in case of an OK result. That does not feel right.

I also don't like the way the information about valid states (OK/ERROR) gets dupplicated into:

the internal enumeration,
the public static methods.

What do you think about my implementation? How would you design the class?

If you don't want to use exceptions you should know that in OOP there is a lot of ways to implement the fact that some object belongs to some specific class. It can be either some enum that enumerates all possible domains or in case of OOP languages it can be class.

So in case of Java (again if you don't want exceptions by some reason) you can use:

class Result
{
    public static final Result ok = new Result(); // I'm not sure this is correct in terms of Java
}

class Failure extends Result
{
    public final String errorMessage;
    public Faliure(String message)
    { this.errorMessage = message; }
}

if (result instanceof Failure)
{
    // do something
}


Drag and drop GUI with native JavaScript Currently I'm engaged with implementing a Drag and Drop GUI.
I've discovered that there a not many resources (tutorials, etc.) available. So I wrote this:

function Cursor(cssSelector, rightLimit, bottomLimit) {
    var element = document.querySelector('#square');
    var styles = window.getComputedStyle(square);
    var x = 0;
    var y = 0;
    var fromLeft = 0;
    var fromTop = 0;
    var pushed = false;
    var limits = {
        top: 0,
        right: rightLimit,
        bottom: bottomLimit,
        left: 0
    }

    // Uses the offsetX and the offsetY of the
    //  mousedown event.
    this.setCoordinates = function(left, top) {
      if (!fromLeft && !fromTop) {
        fromLeft = left;
        fromTop = top;
      }
    }

    this.togglePushed = function() {
        pushed ? pushed = false : pushed = true;
    }

    this.getPushed = function() {
        return pushed;
    }

    // Uses the offsetX and the offsetY of the
    //   mousemove event.
    this.moveCursor = function(offsetX, offsetY) {
      // How much have the x and the y coordinate
      //   changed since the mousedown event?
      var tmpX = offsetX - fromLeft;
      var tmpY = offsetY - fromTop;

      if ((x + tmpX) <= limits.right && (x + tmpX) >= limits.left &&
          (y + tmpY) >= limits.top && (y + tmpY) <= limits.bottom) {
        // If the values are valid then store them ...
        x += tmpX;
        y += tmpY;
        // ... and use them to move the element.
        element.style.left = x + 'px';
        element.style.top = y + 'px';
      }
    }
  }

  var cursor = new Cursor('#square', 550, 450);

  square.addEventListener('mousedown', function(ev) {
    cursor.togglePushed();
    cursor.setCoordinates(ev.offsetX, ev.offsetY);
  });

  document.body.addEventListener('mouseup', function(ev) {
    cursor.togglePushed();
  });

  square.addEventListener('mousemove', function(ev) {
    if (cursor.getPushed()) {
      cursor.moveCursor(ev.offsetX, ev.offsetY);
    }
  });
body {
  background-color: #eefafa;
}

#wrap {
  width: 600px;
  margin: 50px auto;
}

#panel {
  height: 500px;
  position: relative;
  background-color: rgba(150, 150, 150, 0.2);
  border-radius: 3px;
}

#square {
  width: 50px;
  height: 50px;
  background-color: orangered;
  border: 1px solid teal;
  border-radius: 3px;
  position: absolute;
  top: 0px;
  left: 0px;
}

.instruct {
  font-size: 125%;
  font-weight: bold;
  font-family: helvetica;
}

      Click the square. Keep the mouse-button pushed and
        move the pointer slowly.

There's also a demo on CodePen.

Events
Instead of adding 3 (permanent) EventListener, you could just keep the first one: square.addEventListener('mousedown'... and when this is triggered, add the 2 other events mouseup and mousemove on the document (it will also enalbe the user to move quickly)
The mouseup event should then simply remove the 2 event listeners (you will need to keep a trace of the EventListeners).
You can then get rid off the pushed variable.
The square.addEventListener could be moved inside the constructor.
moveCursor
In the method moveCursor instead of moving only if the new position is completely in the area, you could separately check the X and Y coordinates (It will then be easier to move near the edges).
Naming
I don't find the name Cursor to be very expressive... maybe Draggable is a better name?


Return Set with random unique Integer numbers between min and max range I need set of random unique Integer numbers for my unit test. I implemented this method, however I'm not sure if this is best way to implement this.

private Set<Integer> getRandomUniqueNumberSet(Integer min, Integer max)
{
    Set<Integer> added = new LinkedHashSet<Integer>();
    for(int i = min; i <= max; i++)
    {
        Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
        Boolean loop = false;
        if(!added.add(index)) loop = true;
        while(loop)
        {
            index = ThreadLocalRandom.current().nextInt(min, max + 1);
            if(added.add(index)) loop = false;
        }
    }
    return added;
}

Example:

for(Integer index : getRandomUniqueNumberSet(2, 15))
{
    System.out.println("Index: " + index);
}

Results:

Index: 8
Index: 5
Index: 15
Index: 9
Index: 4
Index: 3
Index: 7
Index: 2
Index: 11
Index: 14
Index: 6
Index: 10
Index: 13
Index: 12

Generating unique random numbers within a range

A good way to generate unique random numbers within a range is to create a list with the desired unique numbers, and then shuffle it.

  private Set<Integer> getRandomUniqueNumberSet(int min, int max) {
    List<Integer> numbers = IntStream.rangeClosed(min, max).boxed().collect(Collectors.toList());
    Collections.shuffle(numbers);
    return new LinkedHashSet<>(numbers);
  }

Don't used boxed types when you don't need null values

This method takes Integer parameters, which may be null:

private Set<Integer> getRandomUniqueNumberSet(Integer min, Integer max)

But the implementation doesn't handle the case when these values are null.
And it doesn't make sense to support such ranges.
Change those types to primitive int.

The same goes for the local variables Integer index and Boolean loop.

Why ThreadLocalRandom?

As per the javadoc, "use of ThreadLocalRandom is particularly appropriate when multiple tasks use random numbers in parallel in thread pools". I doubt that's necessary in your use case, in which case I suggest to use an instance of Random instead.


Age Distance Calculator Here is my final build of the Age Distance Calculator - Any feedback on the code would be great - This is my 2nd day of java programming and thought this was a good little project.

package dev.swift.atc;

import java.util.Scanner;

import java.util.concurrent.TimeUnit;

public class AgeCalc {

   private int personAge;
   private int age;
   private int d_age;
   private String name;
   static Scanner s = new Scanner(System.in);

   public static void main(String [] args) throws InterruptedException
   {
       AgeCalc ac = new AgeCalc();
       ac.getInputs();
       boolean result = ac.calculateAge();
       if (result == true) {
          ac.showResults();
          s.close();
       }
       else {
        ac.errorResults();
        TimeUnit.SECONDS.sleep(3);
        System.out.println("Restarting Program");
        TimeUnit.SECONDS.sleep(2);
        ac.ageCalculateTwo();
       }
   }
   //The constructor
   public AgeCalc() throws InterruptedException {
       System.out.println("Welcome To Java Coded Age Differnce Calculator");
       TimeUnit.MILLISECONDS.sleep(100);
   }

   //Handle inputs
   public void getInputs() throws InterruptedException {
        System.out.println("Input Your Name Before We Start");
        this.name = s.nextLine();
        TimeUnit.MILLISECONDS.sleep(100);
        System.out.println("Input Your Age");
        this.age = s.nextInt();
        TimeUnit.MILLISECONDS.sleep(100);
        System.out.println("Your Age Is " + this.age + " Years Old.");
   }

   //Make the calculation
   public boolean calculateAge() throws InterruptedException {
        TimeUnit.MILLISECONDS.sleep(500);
        System.out.println("Now Input The Desired Age To Work Out The Distance To");
        this.d_age = s.nextInt();
        if (this.d_age < this.age){
            System.out.println("You Cannot Set The Target To Lower Then Your Current Age");
            return false;
        }
        else {
         this.personAge = this.d_age - this.age;
         return true;
        }
    }

   //Show the results
   public void showResults() throws InterruptedException {
       System.out.println("Working - Coded By Swift, Hope You Will Use This Program Again " + this.name);
       TimeUnit.SECONDS.sleep(3);
       System.out.println("You Will Be " + this.d_age + " In " + this.personAge +" Years");
       TimeUnit.SECONDS.sleep(3);
       System.out.println("Hope You Enjoyed The Program " + this.name);
       System.exit(0);
       System.out.println("Line Never Reached - Test");
   }
   public void errorResults() throws InterruptedException {
       System.out.println("You Have Entered An Error " + this.name);
   }
   public void ageCalculateTwo() throws InterruptedException {
        TimeUnit.MILLISECONDS.sleep(500);
        System.out.println("Now Input The Desired Age To Work Out The Distance To - Remember Not To Be Lower Than " + this.age);
        this.d_age = s.nextInt();
        if (this.d_age < this.age){
            System.out.println("You Have Already Been Told Not To Do This");
            TimeUnit.SECONDS.sleep(3);
            System.out.println("Program Shutting Down " + this.name + "Due To Multiple Errors You Have Caused ");
            TimeUnit.SECONDS.sleep(2);
            System.out.println("Closed - Coded By Swift, Hope You Will Use This Program Again " + this.name);
            TimeUnit.SECONDS.sleep(2);
            System.exit(0);
            System.out.println("Line Never Reached - Test");
        }
        else {
         this.personAge = this.d_age - this.age;
         System.out.println("Working - Coded By Swift, Hope You Will Use This Program Again " + this.name);
         TimeUnit.SECONDS.sleep(3);
         System.out.println("You Will Be " + this.d_age + " In " + this.personAge +" Years");
         TimeUnit.SECONDS.sleep(3);
         System.out.println("Hope You Enjoyed The Program " + this.name);
         System.exit(0);
         System.out.println("Line Never Reached - Test");
        }
    }
   }

I would use more legible names for variables and classes, they make it easier to read code as plain text (which is useful for other people to understand your code).

For example: naming your class AgeCalculator instead of AgeCalc, or ageCalculator instead of ac when instantiating it.

It is not that important on this example because it's not an extensive code, but if it where, it would be very difficult even for yourself (as the code owner) to remember what all variables mean, for further explanation I would suggest reading the disadvantages paragraph of https://en.wikipedia.org/wiki/Hungarian_notation

Another thing I could suggest you is to have your main method outside AgeCalculator, you should only keep inside methods that represent its behaviour.

I would start practising those, good luck!

EDIT

I found an example at http://www.tutorialspoint.com/java/java_object_classes.htm
I modified it a bit to also apply what I understand are more legible names:

import java.io.*;

public class Employee{

   String name;
   int age;
   String designation;
   double salary;

   // This is the constructor of the class Employee
   public Employee(String name){
      this.name = name;
   }
   // Assign the age of the Employee  to the variable age.
   public void setAge(int newAge){
      age =  newAge;
   }
   /* Assign the designation to the variable designation.*/
   public void setDesignation(String newDesignation){
      designation = newDesignation;
   }
   /* Assign the salary to the variable salary.*/
   public void setSalary(double newSalary){
      salary = newSalary;
   }
   /* Print the Employee details */
   public void printEmployee(){
      System.out.println("Name:"+ name );
      System.out.println("Age:" + age );
      System.out.println("Designation:" + designation );
      System.out.println("Salary:" + salary);
   }
}

As you can see, variable names get a bit longer, but they also state what they are for, just by reading their names, so you don't have to remember "What did 'asdf' mean??" xD get me?

Also take into account that in complex code, you probably would have to be remembering many variables, and sometimes it won't be your code at all! So you won't be familiar with it.

Now, the main method, should be in another class (probably another file):

import java.io.*;

public class TheClassThatHasMainMethod{

   public static void main(String args[]){
      /* Create two objects using constructor */
      Employee firstEmployee = new Employee("James Smith");
      Employee secondEmployee = new Employee("Mary Anne");

      // Invoking methods for each object created
      firstEmployee.setAge(26);
      firstEmployee.setDesignation("Senior Software Engineer");
      firstEmployee.setSalary(1000);
      firstEmployee.printEmployee();

      secondEmployee.setAge(21);
      secondEmployee.setDesignation("Software Engineer");
      secondEmployee.setSalary(500);
      secondEmployee.printEmployee();
   }
}

Because as I said, you should only keep inside a class, methods that represent it's behaviour. At least in Object Oriented Programming, like this case, because each time you initialise a new Object (new Employee), you want it to behave like an Employee and expect it to have Employee methods, like calculating it's salary for example, or getting it's name. But a method called "main" doesn't make sense at all, an employee could give you his salary or name, not his... main?

Also remember that main is a special method that represents the beginning of the execution of your code, it should be separated from the rest of your objects and it must be unique.

Well hope I've helped some, I've also much to learn myself, good luck!


Sieve of Eratosthenes in Python This is yet another take in Python for the infamous Sieve of Eratosthenes. I am a beginner in Python so any criticism is highly appreciated.

# Find primes until this integer.. Modify to your taste..
upper_bound = 1000

# Pass a list of integers and a prime number,
# will return you a list where numbers divisible by the prime you passed are removed.
def remove_non_primes(list_of_ints, prime_to_check_against):
    multiplier = 1
    while prime_to_check_against * multiplier < list_of_ints[-1]:
        multiplier += 1
        # Should be quite fast since list is sorted.
        if prime_to_check_against * multiplier in list_of_ints:
            list_of_ints.remove(prime_to_check_against * multiplier)
    return list_of_ints

ints = list(range(2, upper_bound)) # Generate initial list of integers

# Initial prime for bootstrap
prime = 2
# Keep track of iteration count, so we know the next prime to work with in each iteration.
iteration_count = 0

while True:
    # Use length of list to determine we removed everything we could by
    # checking this length against returning lists length.
    original_list_length = len(ints)
    # Do the magic, remove all non primes found by parameter prime.
    ints = remove_non_primes(ints, prime)
    # List was not modified.. Nothing to remove.
    if original_list_length == len(ints):
        break
    iteration_count += 1
    prime = ints[iteration_count]

print(ints)

Are there any modifications that will dramatically increase the performance?

I question the following comment:

    # Should be quite fast since list is sorted.
    if prime_to_check_against * multiplier in list_of_ints:
        list_of_ints.remove(prime_to_check_against * multiplier)

Why will this be fast, due to the sorted list?  x in list doesn't understand that the list is sorted, nor does the remove(x).  No binary searching will be done; just a straight linear search.  Not fast.

I also question the following comment:

    # List was not modified.. Nothing to remove.
    if original_list_length == len(ints):
        break

Just because no multiples of 29 may remain in the list does not necessarily mean there are no multiples of 31!  The proper stopping condition is when you reach sqrt(upper_bound).

You are attempting to remove numbers you've already removed:

multiplier = 1
while prime_to_check_against * multiplier < list_of_ints[-1]:
    multiplier += 1
    if prime_to_check_against * multiplier in list_of_ints:
        list_of_ints.remove(prime_to_check_against * multiplier)

When you remove multiples of 13, for instance, you are starting by removing 13*2, and 13*3, and 13*4, and so on.

But you have already removed all the multiples of primes lower than 13.  You should start at 13*13,

    multiplier = prime_to_check_against
    while prime_to_check_against * multiplier <= list_of_ints[-1]:
        if prime_to_check_against * multiplier in list_of_ints:
            list_of_ints.remove(prime_to_check_against * multiplier)
        multiplier += 1

Finding and then removing.  You are searching the list twice: once to check if it exists, a second time to find it in order to remove it.

Instead, you could simply removed the item.

    try:
        list_of_ints.remove(prime_to_check_against * multipler)
    except ValueError:
        pass # it wasn't there

Even numbers as a special case.

ints = [2] + list(range(3, upper_bound, 2)) # Generate initial list of odd integers

# Initial prime for bootstrap
prime = 3
iteration_count = 1

And then, instead of increasing the multiplier by 1, you can increase it by 2, so you'd search 13*13, 13*15, 13*17, ...

        multiplier += 2

Skip all of the multiplications, and just increment by 2*prime_to_check_against

    multiple  = prime_to_check_against * prime_to_check_against
    increment = prime_to_check_against * 2
    while multiple <= list_of_ints[-1]:
        try:
            list_of_ints.remove(multiple)
        except ValueError:
            pass
        multiple += increment

Or more pythonically:

    for multiple in range(prime_to_check_against * prime_to_check_against,
                          list_of_ints[-1]+1, 2*prime_to_check_against):
        try:
            list_of_ints.remove(multiple)
        except ValueError:
            pass

Instead of populating a list with all of the candidate numbers, and then removing multiples of prime numbers from the list, which involves expensive list manipulations, you could:

create a bytearray(upper_bound) to store prime/not-prime flags
flag all multiples of prime numbers, as you find them, as not-prime
use list-comprehension to extract the indexes which are flagged as prime

Using a bytearray is very efficient memory-wise, and indexing into the bytearray to set or test a flag is a very fast \$O(1)\$ operation.

I'm trying to decide whether to post my 10-lines of code for the sieve here, or let you attempt it based on the above hints.  Let me know if you need it.


Linux HDD/CPU Temperature Logging Bash Script to run every minute in crontab I wrote a simple script to log HDD and CPU temps to a log file.
If those temperatures reach a threshold, it will shutdown the server. I also made the script make sure the log files don't get over a certain file size. Any ways to improve the code would be greatly appreciated, as I am a noob to programming.

#!/bin/bash
# Check HDD Temps, CPU temp.. shut down server if any exceed acceptable limit
# Using hddtemp to check HDD temps, sensors command to check CPU temp.
date=$(TZ='America/Los_Angeles' date)
HDD1=$(sudo hddtemp /dev/sdb | cut -d":" -f1,2)
HDD2=$(sudo hddtemp /dev/sdc | cut -d":" -f1,2)
T1=$(sudo hddtemp /dev/sdb | cut -d":" -f3 | cut -d" " -f2 | cut -c1-2)
T2=$(sudo hddtemp /dev/sdc | cut -d":" -f3 | cut -d" " -f2 | cut -c1-2)
CPU1=$(sensors | grep "Core" | head -1 | cut -d"+" -f2 | cut -d"." -f1)
CPU2=$(sensors | grep "Core" | tail -1 | cut -d"+" -f2 | cut -d"." -f1)
base_file='/home/brandon/logs/temp1.log'
file='/home/brandon/logs/temp'
log='.log'
error='.error'

find_lowest_file_num() {
        local lowest_file_num=$(ls /home/brandon/logs/ | grep temp | cut -d"p" -f2 | cut -d"." -f1 | sort -rn | tail -1)

        if [ -z "$lowest_file_num" ]
        then
                touch $base_file
                echo 1
        else
                echo $lowest_file_num
        fi
}

find_highest_file_num() {
        local highest_file_num=$(ls /home/brandon/logs/ | grep temp | cut -d"p" -f2 | cut -d"." -f1 | sort -rn | head -1)

        # if highest_file_num is empty, that means there is no temp files, so create one
        if [ -z "$highest_file_num" ]
        then
                touch $base_file
                echo 1
        #else return highest_file_num
        else
               echo $highest_file_num
        fi
}

highest_file_num=$(find_highest_file_num)
lowest_file_num=$(find_lowest_file_num)

get_curr_highest_file() {
        local h=${file}${highest_file_num}${log}
        echo $h
}

get_curr_lowest_file() {
        local l=${file}${lowest_file_num}${log}
        echo $l
}

curr_file=$(get_curr_highest_file)

echo "HDD1: ${HDD1}  Temp:${T1}C     ${date} " >> $curr_file
echo "HDD2: ${HDD2}  Temp:${T2}C     ${date} " >> $curr_file
echo "CPU Core 1 Temp:${CPU1}C     ${date} " >> $curr_file
echo "CPU Core 2 Temp:${CPU2}C     ${date} " >> $curr_file

#if files get bigger than 1000k, make a new one
if [ $(du -k $curr_file | cut -f1) -gt 1000 ]
then
        let "highest_file_num++"
        touch $(get_curr_highest_file)
fi

if [ $(du -k /home/brandon/logs | cut -f1) -gt 100000 ]
then
        rm -f $(get_curr_lowest_file)
fi

if [ $T1 -gt 50 ] || [ $T2 -gt 50 ] || [ $CPU1 -gt 80 ] || [ $CPU2 -gt 80 ]
then
        echo "HIGH TEMPS DETECTED!!! HDD1 Temp: ${T1}C    HDD2 Temp: ${T2}C     ${date}" >> ${curr_file}${error}
        echo "CPU1: ${CPU1}C, CPU2: ${CPU2}           ${date}" >> ${curr_file}${error}
        echo "Shutting down..." >> ${curr_file}${error}
        sudo shutdown now
fi

Here is my revision. I changed it that instead of hardcoding the user's home directory the script can just get that from env. Also, I would run this directly as a root account chron task rather than configuring sudo to be able to run shutdown and hddtemp. Finally, instead of using if -z to check for a file's existance, I would use test -f or something. Anyway, not bad at all for a 'noob', I'd give yourself more credit! Here is my version:

#!/bin/bash
# Check HDD Temps, CPU temp.. shut down server if any exceed acceptable limit
# Using hddtemp to check HDD temps, sensors command to check CPU temp.
date=$(TZ='America/Los_Angeles' date)
# Consider running as root directly instead of requiring sudo (unless you want \
# to configure a nonroot user to be able to use `hddtemp` and `shutdown`) \
# which would work, but since you're running as chron, I would just run \
# it as a root cron job
test $(id -u) == 0 || {echo 'This script must be run as root'; exit 1}
HDD1=$(hddtemp /dev/sdb | cut -d":" -f1,2)
HDD2=$(hddtemp /dev/sdc | cut -d":" -f1,2)
T1=$(sudo hddtemp /dev/sdb | cut -d":" -f3 | cut -d" " -f2 | cut -c1-2)
T2=$(sudo hddtemp /dev/sdc | cut -d":" -f3 | cut -d" " -f2 | cut -c1-2)
CPU1=$(sensors | grep "Core" | head -1 | cut -d"+" -f2 | cut -d"." -f1)
CPU2=$(sensors | grep "Core" | tail -1 | cut -d"+" -f2 | cut -d"." -f1)
logdir="/home/$USER/templogs"
base_file="${logdir}/temp1.log"
file="${logdir}/temp"
log='.log'
error='.error'
test -d $logdir||mkdir $logdir 2>/dev/null

find_lowest_file_num() {
        local lowest_file_num=$(ls ${logdir}/ | grep temp | cut -d"p" -f2 | cut -d"." -f1 | sort -rn | tail -1)

        if [ -z "$lowest_file_num" ]
        then
                touch $base_file
                echo 1
        else
                echo $lowest_file_num
        fi
}

find_highest_file_num() {
        local highest_file_num=$(ls ${logdir}/ | grep temp | cut -d"p" -f2 | cut -d"." -f1 | sort -rn | head -1)

        # How about using `test` instead of ` if -z`?

        test -f "$highest_file_num" || { >"$highest_file_num" ; echo 'Created temp file' ;} && echo "highest_file_num is present"

}

highest_file_num=$(find_highest_file_num)
lowest_file_num=$(find_lowest_file_num)

get_curr_highest_file() {
        local h=${file}${highest_file_num}${log}
        echo $h
}

get_curr_lowest_file() {
        local l=${file}${lowest_file_num}${log}
        echo $l
}

curr_file=get_curr_highest_file

echo "HDD1: ${HDD1}  Temp:${T1}C     ${date} " >> $curr_file
echo "HDD2: ${HDD2}  Temp:${T2}C     ${date} " >> $curr_file
echo "CPU Core 1 Temp:${CPU1}C     ${date} " >> $curr_file
echo "CPU Core 2 Temp:${CPU2}C     ${date} " >> $curr_file

#if files get bigger than 1000k, make a new one
if [ $(du -k $curr_file | cut -f1) -gt 1000 ]
then
        let "highest_file_num++"
        touch $(get_curr_highest_file)
fi

if [ $(du -k $logdir | cut -f1) -gt 100000 ]
then
        rm -f $(get_curr_lowest_file)
fi

if [ $T1 -gt 50 ] || [ $T2 -gt 50 ] || [ $CPU1 -gt 80 ] || [ $CPU2 -gt 80 ]
then
        echo "HIGH TEMPS DETECTED!!! HDD1 Temp: ${T1}C    HDD2 Temp: ${T2}C     ${date}" >> ${curr_file}${error}
        echo "CPU1: ${CPU1}C, CPU2: ${CPU2}           ${date}" >> ${curr_file}${error}
        echo "Shutting down..." >> ${curr_file}${error}
        shutdown now # I would run this as root directly
else
    echo "Temps ok: HDD1: ${T1}, HDD2: ${T2}, CPU1: ${CPU1}, CPU2: ${CPU2}"
fi


Is there a better/cleaner way to assign values based on the result of radio buttons? I have 3 radio buttons all grouped inside a group box in a WinForms project. Currently, I am checking which button is checked and assigning values using the following if/else statement, similar to the below example

        if (rb1.Checked)
        {
            selectedButton = 1;
            selectedButtonText = rb1.Text;
        }
        else if (rb2.Checked)
        {
            selectedButton = 2;
            selectedButtonText = rb2.Text;
        }
        else
        {
            selectedButton = 3;
            selectedButtonText = rb3.Text;
        }

        Console.WriteLine(selectedButtonText);

This doesn't look very efficient or scalable. Is there a better way to do this?

Just a side note for you.  It would probably be more efficient to use the Checked_Change handler.  The main caveat is that both the checked and the unchecked will trigger this event.  but since the unchecked is handled first, it's a simple matter of returning if the sender isn't checked.

Since the radiobuttons are actually numbered you don't need the switch block once you have the checked radiobutton identified.

Something like this would work:

private void radioButton_CheckedChanged(object sender, EventArgs e)
{
    RadioButton rb = (RadioButton)sender;
    if(!rb.Checked)
    {
        return;
    }
    selectedButton = rb.Name.Last() - '0';
    selectedButtonText = rb.Text;
}


Get the environment variable's value I've written a function which can get the environment variable's value. The input parameter would one of the following:

${machine}/bin
${machine/bin}
${machine}/bin${path${OS}}/path

I would need to check the values which start with ${}

I've written a function which is recursive in nature, and I would appreciate it if anyone can review the code for me and tell me what can be done better:

private String replaceWithEnvironmentVariables(String specialChars) {
    String result="";
    boolean flag=true;
    try{
        result = specialChars;
        String temp = "";
        char loopBreaker = '}';
        char dollarCheck = '$';
        char slashCheck = '/';
        int index = result.indexOf("{");
        if (index == -1){
            result = result.replaceAll("[{},$]", "");
            return result;
        }
        for (int i = index; i < result.length(); i++) {
            if(slashCheck==result.charAt(i))
                break;
            temp += result.charAt(i);
            if(dollarCheck==result.charAt(i) || loopBreaker==result.charAt(i)){
                break;
            }
        }
        String sub = temp.replaceAll("[{},$/]", "");
        if(sub==null || sub.length()==0){
            sub="";
            flag=false;
            result = result.replace(temp, sub);
        }
        if(flag){
            String returnSub = getEnvironmentVariableValue(sub);
            result = result.replace(temp, returnSub);
        }
    }catch(NullPointerException nullPointer){
        nullPointer.printStackTrace();
    }catch(Exception except){
        except.printStackTrace();
    }
    return replaceWithEnvironmentVariables(result);
}

private String replaceWithEnvironmentVariables(String specialChars) {

Naming. specialChars are no special chars, it's String in which variables get substituted. Maybe pattern or alike.

String result="";

Spacing. Needless assignment as it gets overwritten 3 lines below. Should be

String result = specialChars;

boolean flag=true;

Naming... flag means actually nothing.

String temp = "";

And neither does temp. A single meaningless name is sometimes tolerable (especially in short methods), but with more, it's getting harder and harder to understand.

char dollarCheck = '$';

Dollar cheque? Check? ✔? It should be a constant and called e.g.

private static static DOLLAR_CHAR = '$';

or better "variable start char" or alike. Prefer to express what it means to what it is, as the latter is obvious.

    if (index == -1){
        result = result.replaceAll("[{},$]", "");
        return result;
    }

I'd personally throw an exception for malformed inputs like foo}bar.

if(slashCheck==result.charAt(i))

That's Yoda style, the form var == expr is more common.

Now I've got lost... you parse the string somehow... OK, let's ignore it.

String sub = temp.replaceAll("[{},$/]", "");
if(sub==null || sub.length()==0){

Know your libraries. sub can't be null. Instead of sub.length()==0, you can use sub.isEmpty().

sub="";

This line is useless now.

... catch ...

Just leave it out. Let the user deal with the problem. You may know, that e.g., using an empty String is acceptable in case of an error, but you surely don't know it here. Just document the exception and let the poor user deal with it. Here, "user" means you in another piece of the program.

NullPointerException

I see a single place where it can be thrown:

String returnSub = getEnvironmentVariableValue(sub);
result = result.replace(temp, returnSub);

Don't let this happen. Maybe throw a new MissingEnvironmentVariableException(sub), maybe allow the caller to specify a replacement, e.g., like this

private String replaceWithEnvironmentVariables(
        String pattern,
        Map<String, String> specificReplacements,
        String defaultReplacement) {...}

where specificReplacements contains the default values for missing variables and defaultReplacement gets used as a last resort. Allow defaultReplacement to be null, but then throw an exception if it should be needed.


mo algorithm for summing the subarray Below is the implementation of MO algorithm for summing the sub array given the range as input.
As MO algorithm is a offline query it is given a list of ranges as input.

Below code calculates the result using MO algorithm and compares the result with expected output by directly summing the sub array.

MO algorithm first divides the whole array in sqrt(len of array) blocks and then divides the queries based on blocks. It sorts the input queries based on blocks and then based on the right parameters if the block number is same. It uses two pointers which moves left and right based on input query. The main intention of the algorithm is to reduce the movements of the pointers.

import collections
import math

def cmp(a, b):
    if a > b:
        return 1
    elif a < b:
        return -1
    return 0

def compare(a, b):
    def bin(x):
        return x[0]/r
    result = cmp(a, b)
    if result:
        return result
    return cmp(x[1], y[1])

a   = [1, 1, 2, 1, 3, 4, 5, 2, 8];
r = math.sqrt(len(a))
d_query = {0:[0, 5], 1:[5, 8], 2:[2, 4], 3:[5, 6]}
od = collections.OrderedDict(sorted(d_query.items(), compare, key= lambda x: x[1]))
print(od)

l = r = 0
cur_sum = 0
cur_l = 0
cur_r = -1
result = {}
for key, value in od.items():
    l = value[0]
    r = value[1]
    while cur_r < r:
        cur_r += 1
        cur_sum += a[cur_r]
    while cur_r > r:
        cur_sum -= a[cur_r]
        cur_r -= 1
    while cur_l < l:
        cur_sum -= a[cur_l]
        cur_l += 1
    while cur_l > l:
        cur_l -= 1
        cur_sum += a[cur_l]
    result[key] = cur_sum

for (k1,v1), (k2, v2) in zip(d_query.items(),result.items()):
    if k1 != k2 or v1 == v2:
        print("failed")
        break
print(result)

I would use functools.partial() to pass the block size into the comparator
choose more explicit and readable variable names - left, right and block_size instead of l, r and r
there is a simpler way to create an expected result dictionary:

expected_result = {key: sum(a[left:right+1]) for key, (left, right) in queries.items()}

we can unpack left and right range borders instead of getting them by index
note that there is no cmp in Python 3 anymore - you would have to use the cmp_to_key() helper to make it work

Improved code:

from functools import partial
import math

def compare(block_size, x, y):
    """The heart of the Mo's algorithm - determines if a range preceeds another."""
    _, (x_left, x_right) = x
    _, (y_left, y_right) = y

    if x_left / block_size != y_left / block_size:
        return x_left / block_size < y_left / block_size

    return x_right < y_right

def get_expected_result(a, queries):
    """Calculates given queries as sums of given ranges."""
    return {key: sum(a[left:right + 1]) for key, (left, right) in queries.items()}

def mos_algorithm(a, queries):
    """Mo's algorithm implementation."""
    block_size = math.sqrt(len(a))

    cur_sum, cur_left, cur_right = 0, 0, -1
    result = {}
    for key, (left, right) in sorted(queries.items(), cmp=partial(compare, block_size)):
        while cur_right < right:
            cur_right += 1
            cur_sum += a[cur_right]

        while cur_right > right:
            cur_sum -= a[cur_right]
            cur_right -= 1

        while cur_left < left:
            cur_sum -= a[cur_left]
            cur_left += 1

        while cur_left > left:
            cur_left -= 1
            cur_sum += a[cur_left]

        result[key] = cur_sum

    return result

if __name__ == '__main__':
    a = [1, 1, 2, 1, 3, 4, 5, 2, 8]
    queries = {0: [0, 5], 1: [5, 8], 2: [2, 4], 3: [5, 6]}

    result = mos_algorithm(a, queries)
    expected_result = get_expected_result(a, queries)

    print(result)
    print(expected_result)

Prints:

{0: 12, 1: 19, 2: 6, 3: 9}
{0: 12, 1: 19, 2: 6, 3: 9}


Calculating the average of all nodes in a Binary Search Tree using visitors I'm required to calculate the average value of all nodes in a BST using a Visitor, I'm not sure if I did this correctly or if i could just calculate the average with inOrder() and therefore I'm not really utilizing the visitor.

import java.util.*;

public class Node{

    int val;
    Node left;
    Node right;
    private static int sum;
    private static int count;
    private static double average;

    public Node(int val){
        this.val = val;
    }

    public int getVal(){
        return val;
    }

    public interface Visitor {
        int visit(Node node);
    }

    static class NodeVisitorImpl implements Visitor{
        @Override
        public int visit(Node node) {
            return node.getVal();
        }
    }

    public void insert(int toInsert) {
        if (toInsert < this.val) {
            if (this.left == null) {
                this.left = new Node(toInsert);
            } else {
                this.left.insert(toInsert);
            }
        } else if (toInsert > this.val) {
            if (this.right == null) {
                this.right = new Node(toInsert);
            } else {
                this.right.insert(toInsert);
            }
        } else {
            System.out.println("Shouldn't have the same value");
        }

    }

    public static void inOrder(Node root){
        if (root == null) {
            return;
        }
        Visitor visitor = new NodeVisitorImpl();
        sum += visitor.visit(root);
        count++;
        average = sum/count;
        inOrder(root.left);
        inOrder(root.right);
    }

    public static void main(String[] args){
        Node root = new Node(4);
        root.insert(2);
        root.insert(1);
        root.insert(3);
        root.insert(6);
        root.insert(5);
        root.insert(7);

        inOrder(root);
        System.out.println(average);
    }

}

You are doing it somewhat right, somewhat wrong.

Your visitor is more of a Mapping transformer: it is converting Node into an int.  How it gets that int value from the Node is left up to the implementation.

Unfortunately, all of your average calculation is still inside inOrder().  Worse, your sum, count and average values are static members of Node, so you cannot do two parallel average calculations.  There is no resetting of the sum or count so a subsequent average calculation is going to compute the average of the aggregate of all data, not just the most recent tree.

Your Visitor visit() method should return nothing:

public interface Visitor {
    void visit(Node node);
}

Then, you can create an AveragingVisitor which averages the data it extracts from Node objects:

static class AveragingVisitor implements Visitor {
    int sum = 0;
    int count = 0;

    @Override
    public void visit(Node node) {
        sum += node.getVal();
        count++;
    }

    public double average() {
        return (double) sum / count;
    }
}

Each time you create a new AveragingVisitor, it will start new count & running total at zero.

To compute the average, you would create this averaging visitor, and perform some kind of traversal (inOrder is fine) of the tree:

AveragingVisitor visitor = new AveragingVisitor();

root.inOrder(visitor);

System.out.println(visitor.average());

Of course, Node::inOrder(Visitor vistor) will need to be rewritten as non-static, and call the visitor.visit(node) method on each node:

public void inOrder(Visitor visitor) {
    if (left != null)
        left.inOrder(visitor);
    visitor.visit(this);
    if (right != null)
        right.inOrder(visitor);
}

With this visitor, you could easily make a PrintingVisitor to print all of the nodes.  No change would need to be made to inOrder().


Creating an inventory using classes The next assignment in my class is to create a program that allows the user to input an inventory, and then display the total value of the inventory.  We are required to create a class with both private and public member functions.  I tried to keep it as streamlined as possible but I know there is always room for improvement.  Let me know what you think.

I defined the member functions outside of the class by direction of the prof. I personally like seeing it all in one place, but this is an OOP class, so I guess it makes sense to define them outside.  The only other direction was that all values have to be positive, hence the do while loops.

#include <iostream>

using namespace std;

class inventory
{
    private:
        int productNum[10];
        int productCount[10];
        double productPrice[10];

    public:
        int locator;
        void retrieve();
        void init();
        void store();

}inv1;

void setItemNumber(int &temp)
{
    do
    {
        cout << "\nPlease enter the item number (if finished please enter '0'): ";
        cout << "\nNumber must be positive: " << endl;
        cin >> temp;
    } while (temp < 0);
}
void setQuantity(int &temp)
{
    do
    {
        cout << "\nPlease enter the Quantity: ";
        cout << "\nNumber must be positive: " << endl;
        cin >> temp;
    } while (temp < 0);
}
void setPrice(double &price)
{
    do
    {
        cout << "\nPlease enter the Price: ";
        cout << "\nNumber must be positive: " << endl;
        cin >> price;
    } while (price < 0);
}

void inventory :: init()
{
    int counter = 0;
    while (counter < 10)
    {
        productCount[counter] = 0;
        productNum[counter] = 0;
        productPrice[counter]= 0;
        counter++;
        locator = counter;
    }
}

void inventory :: store()
{
    int counter = 0, temp = 0;
    double price = 0;
    cout << "\nWelcome to the inventory control system" << endl;
    cout << "When finished please enter '0' for the item number" << endl;
    while (counter < locator)
    {
        setItemNumber(temp);
        if(temp == 0)
        {
            locator = counter;
            return;
        }
        productNum[counter]=temp;
        setQuantity(temp);
        productCount[counter]=temp;
        setPrice(price);
        productPrice[counter]=price;
        counter++;
    }
}

void inventory :: retrieve()
{
    int counter = 0;
    double total = 0;
    cout << "Product Number \t\t Product Count \t\t Price" << endl;
    cout << "_______________________________________________________" << endl;
    while (counter < locator)
    {
        cout << productNum[counter];
        cout << "\t\t\t" << productCount[counter];
        cout << "\t\t\t" << productPrice[counter] << endl;
        total = productCount[counter] * productPrice[counter] + total;
        counter++;
    }
    cout << "Your total inventory is valued at: $" << total << endl;
}

int main()
{

    inv1.init();
    inv1.store();
    inv1.retrieve();

    return 0;
}

int productNum[10];
int productCount[10];
double productPrice[10];

That's not exactly OOP what you did here. A single product is apparently defined by the combination of productNum, productCount and productPrice. So these 3 attributes should have been grouped together in a product class:

class product {
    int productNum;
    int productCount;
    double productPrice;
}

I'll let it to your exercise to decide on the visibility of these attributes, and to add a constructor.

And the inventory class then correspondingly holds a collection of products instead:

product products[10];

Well, except that this isn't exactly a clean way to manage a collection of products in C++ either, better use a dynamically growing data type with checked bounds:

std::vector<product> products;

This also brings us to the next point, your use of these two:

int locator;
int counter = 0;

I assume your locator variable was actually supposed to indicate the fill level of the original inventory tracking mechanism. Please try to name it properly the next time. There is also a second problem with locator, you made it public even though any external modification to this variable would break the integrity of your implementation.

void inventory::init() {
    ...
}

This method should have been called void inventory::inventory(), which makes it the constructor for the inventory class and is called automatically every time a new inventory is instantiated. Have a read on the RAII principle.

void inventory::store();
void inventory::retrieve();

These two have a rather strange signature for what they are called. Your task was to create an inventory class which manages an inventory. No less, no more. What you actually did, was cramming all the input and output logic into that class as well. That's a direct violation of the Separation of Concerns principle.

What that part of the API should look like is simple:

void inventory::store(product p);
double inventory::getTotal();

These methods shouldn't contain any input/output logic, but only the algorithms relevant to storing products, respectively to query all stored products for the grand total.

The input/output belongs into the scope working with the inventory instance, not into that class.

class inventory {...} inv1;

That's a global variable you created here. Unless you have a very good reason to do so (and trust me when I say you almost never have), don't. You should have instantiated inventory inside the main() function instead.

  I defined the member functions outside of the class by direction of the prof. I personally like seeing it all in one place, but this is an OOP class, so I guess it makes sense to define them outside.

Not quite. You should probably be familiar by now with the concept of splitting your source code into header and source files. With headers containing structural information, and source files containing implementations.

In this case, they class definition should have gone into a file called inventory.h and the implementation of the class would have belonged into inventory.cpp.

If take a look back at the section about the global variable inv1, it should strike why that was a mistake.

  We are required to create a class with both private and public member functions.

You don't have any private methods yet. Just food for thought, in regard to the Separation of Concerns, you could actually structure your API like this:

class inventory {
    private:
        // Actually stores the product
        void storeUnchecked(product p);
        ...
    public:
        // Validates that the product fulfills the requirements
        // If yes, passes down to storeUnchecked()
        bool store(product p);
        ...
}

Since adding a product can obviously fail if validation fails, you want to account for that by allowing your API to signal the success or failure back to the calling location.


Which will be better declaration go to with TypeScript? This is a simple question, but during my migration to TypeScript, I'm seeing that in many cases my code could take these two directions... which one could be a better direction to go with: class.ts or interface.ts?

class.ts

export class ActionSet {
  constructor(private name: string) {}

  get ACTION(): string {
    return this.name;
  }

  get PENDING(): string {
    return `${this.name}_PENDING`;
  }

  get FULFILLED(): string {
    return `${this.name}_FULFILLED`;
  }

  get REJECTED(): string {
    return `${this.name}_REJECTED`;
  }
}

export function createActionSet(name: string): ActionSet {
  return new ActionSet(name);
}

interface.ts

export interface IActionSet {
  ACTION: string;
  PENDING: string;
  FULFILLED: string;
  REJECTED: string;
}

export function createActionSet(name: string): IActionSet {
  return {
    ACTION: name,
    PENDING: `${name}_PENDING`,
    FULFILLED: `${name}_FULFILLED`,
    REJECTED: `${name}_REJECTED`
  };
}

Due to ActionSet is not being used in many places, will it be worth to create a class for it? or is this implementation an overkill?

Your question is a bit too abstract. Both ways are okay in different scenarios. You may even have a mixture of two patterns in the same code base... Sorry, but yes, my answer is "it depends".

I think, the key here is the intent and the way it's conveyed via code. If ActionSet's behavior is rigid, and "known in advance", you may want to declare it as a class (as in first example). This will make sure that even when an instance is created NOT by using createActionSet() it still obeys the same behavior rules.

If however, your entity is not rigid and you don't know all its possible usages in advance, it's probably better to start with an interface and a set of factory functions (as in second example).

Since the context of the question is not really known, I hesitate to go beyond this explanation and recommend a particular one.

In my practice interfaces have always been "handier", more refactorable, and easier to deal with. I do pick them as defaults today and rarely go to classes. This, however, is mostly true to DTO-alike classes ("dummy" classes with little to no behavior whatsoever). I find it helpful to separate the data transfer entities from the data processing entities, but it (again) could be due to the client-server-comm saturated nature of my applications.

Use your own judgement! :)


Batch script to make folders hidden I just wrote a simple script cmd to hide private folders by combining several scripts from the Internet.  Can anyone give me suggestions for improvement?

This batch file uses a password hidden by powershell script. Then I create a shortcut and run attrib -s -h, so it is not easy to find.
It works just gave attrib -s -h on the desired folder. Location folder can only be changed by editing the file. Also passwords can only be changed by editing the batch file. The last two things are quite cumbersome.

@echo off
title Privat Locker
set _folder="E:\Private"

dir /a:h %_folder%>nul 2>nul
if %errorlevel%==0 goto PASS1

dir /a:s %_folder%>nul 2>nul
if %errorlevel%==0 goto PASS1

:CONFIRM
echo Do you want to lock the Private Folder?! [Y/N]
set/p "cho=>"
if %cho%==Y goto LOCK
if %cho%==y goto LOCK
if %cho%==n goto end
if %cho%==N goto end
echo Error! Wrong answer, please type [Y/N]
goto CONFIRM

:LOCK
attrib +h +s %_folder%
echo The folder sucsessfully locked...
pause
goto end

:PASS1
set "psCommand=powershell -Command "$pword = read-host 'Masukkan PIN!' -AsSecureString ; ^
     $BSTR=[System.Runtime.InteropServices.Marshal]::SecureStringToBSTR($pword); ^
           [System.Runtime.InteropServices.Marshal]::PtrToStringAuto($BSTR)""
for /f "usebackq delims=" %%p in (`%psCommand%`) do set password=%%p
if NOT "%password%"=="YOURPINHERE" goto FAIL1
attrib -s -h %_folder%
goto EXP

:PASS2
set "psCommand=powershell -Command "$pword = read-host 'Masukkan PIN!' -AsSecureString ; ^
     $BSTR=[System.Runtime.InteropServices.Marshal]::SecureStringToBSTR($pword); ^
           [System.Runtime.InteropServices.Marshal]::PtrToStringAuto($BSTR)""
for /f "usebackq delims=" %%p in (`%psCommand%`) do set password=%%p
if NOT "%password%"=="YOURPINHERE" goto FAIL2
attrib -s -h %_folder%
goto EXP

:PASS3
set "psCommand=powershell -Command "$pword = read-host 'Masukkan PIN!' -AsSecureString ; ^
     $BSTR=[System.Runtime.InteropServices.Marshal]::SecureStringToBSTR($pword); ^
           [System.Runtime.InteropServices.Marshal]::PtrToStringAuto($BSTR)""
for /f "usebackq delims=" %%p in (`%psCommand%`) do set password=%%p
if NOT "%password%"=="YOURPINHERE" goto FAIL3
attrib -s -h %_folder%

:EXP
echo Folder successfully unlocked, do you want to look it? [Y/N]
set/p "tuju=>"
if %tuju%==Y goto BUKA
if %tuju%==y goto BUKA
if %tuju%==n goto end
if %tuju%==N goto end
echo Error! Pilihan salah, ketik [Y/N]
goto EXP

:BUKA
echo Go to folder...
pause
start explorer %_folder%
goto end

:FAIL1
echo Wrong PIN!
echo Try again, 2 remaining
goto PASS2

:FAIL2
echo Wrong PIN!
echo Try again! 1 remaining
goto PASS3

:FAIL3
echo Wrong PIN!
echo Device will reboot!

:PIL
set/p "rst=Automatic Restart? [Y/N]"
if %rst%==Y goto RST
if %rst%==y goto RST
if %rst%==N goto MNL
if %rst%==n goto MNL
echo Error! wrong choice, pleas type [Y/N]
goto PIL

:RST
shutdown -r -t 15 -c "Computer will reboot on 15 second's"
pause
goto end

:MNL
echo If device not rebooting on 15 minutes, all data[s] in the folder
echo will be erased...
pause

:end

I am glad you asked your question here. I'm hoping to explain why what you are trying to do is a bad idea. I cannot speak much for improving your batch as that is not my strong point but that should not matter as the focus of the review is ....

Security Through Obscurity

If you are unfamiliar with that phrase as a concept Wikipedia has a good primer on it.

If I understand the script correctly it is designed to mark a folder hidden. If it is already hidden then using a password will unlock it. You have a very flawed approach to security as it would only keep the most naive of users in the dark. The following is a combination of points, in no particular order (since the really refer to each other), that illustrate why you should not bother doing this.

Hiding a folder does not prevent access to it. A simple control panel change would expose those folders and since the security of the folder has not changed a user can just walk right in. Or if you knew the full path then you can just type it in Windows Explorer or the Run menu.

  The last two things are quite cumbersome.

You have the "password" hard-coded in the script. It is a very trivial action to make changes. Again this is only keeping a certain breed of user from accessing your document location. Since neither the script or folder are technically secure then it is moot.

PowerShell code is doing nothing but masking the view. You take in a secure password but immediately unsecured it. The only thing that approach would prevent is someone directly behind you watching what you are doing and not looking directly at the keyboard.

Consider PowerShell

For reasons stated above I cannot condone this code but there are some concepts you should be aware of that PowerShell can do better. While you code is mostly batch you have a splash of PowerShell there so I think you have at least a vague familiarity with it.

Use functions

As I am sure you know there are several instances of repeated code. Those could be easily replaced with some simple functions that you could call in a loop that runs 3 times for example.

A much improved choice menu

PowerShell has a great way of creating menus guiding user input and acting on the results. It is not too hard to get a grasp at first glance so I am going to include the code snippet from the from the front of that article.

$title = "Delete Files"
$message = "Do you want to delete the remaining files in the folder?"

$yes = New-Object System.Management.Automation.Host.ChoiceDescription "&Yes", `
    "Deletes all the files in the folder."

$no = New-Object System.Management.Automation.Host.ChoiceDescription "&No", `
    "Retains all the files in the folder."

$options = [System.Management.Automation.Host.ChoiceDescription[]]($yes, $no)

$result = $host.ui.PromptForChoice($title, $message, $options, 0)

switch ($result)
{
    0 {"You selected Yes."}
    1 {"You selected No."}
}

Consider Encryption

If you were still concerned about the safety and security of your documents where multiple users would have access consider an encryption solution. It's too much to cover here but there are several simple approaches involving encryption software (I cannot recommend any specific one.).


Git pre-commit hook to check that changes to certain files only happen on the master branch As a beginner in Ruby, I am trying to optimize this script that is a client side pre-commit hook for git.

Was hoping I could get a review to ensure I am following all ruby idioms, and to ensure I have the most elegant solution.

#!/usr/bin/env ruby
#
# Pre-commit hook created to help ensure dns changes are being
# committed to the correct branch.
#

# Hash of zone files to look for when a commit is attempted.
zone_files = {
  development: ‘some_zone_file1’,
  prod: ‘some_zone_file2’
}

# Execute git diff to determine files includes as part of commit.
out_data = %x{git diff --cached --name-only}
pri_branch = "master"

# Iterate through the zone files, and check if any are found in the output.
# Only one needs to be found. No need to find all.
zone_files.keys.each do |key|
  if out_data.include?zone_files[key]
    current_branch = %x{git branch}.match(/\* (.+?)\n/m)
    if current_branch[1].include?pri_branch
      break
    else
      puts "Warning: You have triggered a git pre-commit hook."
      puts "Warning: Your current branch is: #{current_branch[1]}."
      puts "Warning: For dns, ensure you commit on branch: #{pri_branch}"
      puts "Warning: Include '-n' to bypass this check."
      puts "Exiting..."
      exit 1
    end
  end
end

Use fewer abbreviations. It's certainly not bad in this case, but small stuff like production instead of prod (unless perhaps it's a naming convention for your production environment/server), and primary_branch (I guess?) instead of pri_branch. Or, at any rate, be consistent: You have development spelled out, but you shorten production?
Use parenthenses or at the very least spaces in method calls - and include? is just a method call. So this expression out_data.include?zone_files[key] looks weird right now.
When iterating a hash, the block will be passed the elements as [key, value] arrays. If you add a bit of array destructuring, you get key and value separately.

zone_files.each do |environment, file|
  # ...
end

But zone_files doesn't actually need to be a hash. Of course you can use one, and it may be worth it to label to two zone files. But it could just be an array. And you might want to make it constant while you're at it.
Get the current branch once. Right now it's being parsed for each of the zone files, since it's inside the loop.
And check the branch once - if it's the pri_branch then you can skip everything else.
You can use a heredoc for the warning

Here's my take

# Array of zone files to look for when a commit is attempted.
ZONE_FILES = %w(some_zone_file1 some_zone_file2).freeze

# The branch we're allowed to commit zone files on
PRIMARY_BRANCH = "master".freeze

# get the current branch
current_branch = %x{git branch}.match(/\* (.+?)\n/m)[1]

# stop here if we're committing to the primary branch
exit(0) if current_branch == PRIMARY_BRANCH

# execute git diff to determine files includes as part of commit.
commit_files = %x{git diff --cached --name-only}

# find out if any of the zone files are among the files
# being committed
committing_zone_file = ZONE_FILES.detect { |zone_file| commit_files.include?(zone_file) }

# and if we're committing a zone file, complain to the user
if committing_zone_file
  puts <<-EOT
Error: You have triggered a git pre-commit hook.
Your current branch is: #{current_branch}.
For DNS, ensure you commit on branch: #{PRIMARY_BRANCH}
Include '-n' to bypass this check.
Exiting...
  EOT

  exit(1)
end


Multiple joins for fields that need the same data. SQL Server 2008 R2 I'm hoping I can find a better way to do this. I've tried this with a join with multiple 'OR' statements (I know it's terrible), and with multiple joins as shown below.

SELECT  ICD_CODE1, ICD_CODE2, ICD_CODE3, icd1.DESCRIPTION AS ICD_DESCRIPTION1,
        icd2.DESCRIPTION AS ICD_DESCRIPTION2, icd3.DESCRIPTION AS ICD_DESCRIPTION3
FROM ICD_CODES
LEFT OUTER JOIN ICD_DECRIPTIONS AS icd1
    ON ICD_CODES.ICD_CODE1 = icd1.ICD_CODE
LEFT OUTER JOIN ICD_DECRIPTIONS AS icd2
    ON ICD_CODES.ICD_CODE2 = icd2.ICD_CODE
LEFT OUTER JOIN ICD_DECRIPTIONS AS icd3
    ON ICD_CODES.ICD_CODE3 = icd3.ICD_CODE

This takes place in a much larger dataset and is terribly slow. There are other fields that need a similar style join so I'm hoping someone can help me optimize.

Classic de-normalized data. I've seen this often in healthcare. You can create a function to do this for you. It's unlikely to be faster but will save your some typing. You'd have to test each to see which is quicker. But the bottom line is there isn't a "great" improvement method.

create function dbo.get_icd_code (@icd varchar(16))
returns varchar(256)
as
begin
    declare @return varchar(256)
    select @return = DESCRIPTION
    from ICD_DESCRIPTIONS
    where ICD_CODE = @icd
    return @return
end

Then call it...

SELECT
    ICD_CODE1,
    dbo.get_icd_code(ICD_CODE1)
    ICD_CODE2,
    dbo.get_icd_code(ICD_CODE2)
    ICD_CODE3,
    dbo.get_icd_code(ICD_CODE3)
FROM ICD_CODES

Another way to speed it up is to stage your data first since there are so many ICD10 codes, but you'll need to provide the entire query for me to determine if this will work--primarily what other tables you are joining to and limiting from.

Lastly, you could try to un-pivot your data, join to the description field once, and then either keep it in this vertical fashion or pivot it back up. Again, hard to benchmark this against the multiple joins without being in your environment.


Scheduled file sorting with Ruby I have a script running as an hourly cron job. It finds all screenshots on the Desktop and moves them to directories sorted by year/month/day.

On MacOS, screenshots are automatically saved to the desktop with the following filename structure:

"/Users/<USER>/Desktop/Screen Shot 2019-01-02 at 11.56.42 AM.png"

require 'fileutils'

class Screenshot
  attr_reader :filepath

  def initialize(filepath)
    @filepath = filepath
  end

  def dir_exists?
    File.exists?(destination) || FileUtils.mkdir_p(destination)
  end

  def date
    @date ||= filepath.match(screenshot_regex)
  end

  def move
    FileUtils.mv(filepath, destination) if dir_exists?
  end

  def base_path
    "/Users/home/Pictures/Screenshots"
  end

  def destination
    "#{base_path}/#{date[:year]}/#{date[:month]}/#{date[:day]}"
  end

  def screenshot_regex
    /Shot (?<year>(.*))-(?<month>(.*))-(?<day>(.*)) at/
  end
end

class Screenshots
  attr_reader :directory
  def initialize(directory)
    @directory = directory
  end

  def filepaths
    Dir.glob("#{directory}/Screen Shot*.png")
  end

  def files
    filepaths.map{|i| Screenshot.new(i)}
  end

  def move_all
    files.each(&:move)
  end
end

Screenshots.new("/Users/home/Desktop").move_all

I'm sure there's a less messy approach here and I'd love to hear any criticism.

Your program is over-engineered in a way that makes it hard to decipher.  Because every method is one line long, it's like reading a poem whose lines have been scrambled.  You would be much better off writing a simple function, like this:

require 'fileutils'

def move_screenshots(src_dir, dest_tree)
  re = /^Screen Shot (?<year>\d{4})-(?<month>\d{2})-(?<day>\d{2}) at/
  Dir.foreach(src_dir) do |filename|
    if date = re.match(filename)
      dest_dir = "#{dest_tree}/#{date[:year]}/#{date[:month]}/#{date[:day]}"
      FileUtils.mkdir_p(dest_dir)
      FileUtils.mv("#{src_dir}/#{filename}", dest_dir)
    end
  end
end

move_screenshots("/Users/home/Desktop", "/Users/home/Pictures/Screenshots")

I wouldn't bother with Dir#glob, since it's a bit redundant with the regex.  Note that screenshots aren't necessarily in PNG format: the image format can be configured using defaults write com.apple.screencapture type ….  I also wouldn't bother testing File#exists? before calling FileUtils#mkdir_p, since mkdir_p implicitly performs that check anyway.

Instead of an hourly cron job, consider creating a Folder Action Script that is triggered instantly when a file is added to the folder.


Golfed Inverse Ackermann Function The following was posted on PPCG:

  Your job is to create the slowest growing function you can in no more than 100 bytes.

I did some Googling, and I was interested in the Inverse Ackermann Function:

$$
\alpha(m,n) = \min\{i \ge 1 : A(i, \lfloor \frac{m}{n} \rfloor \ge \log_2n\}
$$

Where \$A\$ is the Ackermann Function:

$$
A(m,n) =
\begin{cases}
n + 1, & \text{if} \space m = 0 \\
A(m - 1, 1) & \text{if} \space m > 0 \space \text{and} \space n = 0 \\
A(m - 1, A(m, n - 1)) & \text{if} \space m > 0 \space \text{and} \space n \gt 0
\end{cases}
$$

The Ackermann function itself is not too difficult to implement. Here is my implementation:

A=(m,n)=>m?A(m-1,n?A(m,n-1):1):n+1

However, the mathematical stuff in the Inverse Ackermann stumped me. I came up with the following, also changing it from two variables to one:

a=(m,n=m,i=1)=>{while(A(i,m/n|0)<=Math.log2(n))i++;return i}

However, that was yesterday. Today, I'm pretty sure I screwed some stuff up. i will always be one more than it should be, since the loop increments and then checks. So, the code should be this:

a=(m,n=m,i=1)=>{while(A(i,m/n|0)<=Math.log2(n))i++;return i-1}

Now, of course this will StackOverflow very quickly. However, theoretically, it works.

Readable:

function Ackermann(m, n) {
  return m ? Ackermann(m - 1, n ? Ackermann(m, n - 1) : 1) : n + 1;
}

function inverseAckermann(m) {
  var n = m;
  var i = 1;

  while (Ackermann(i, Math.floor(m / n)) <= Math.log2(n)) {
    i++;
  }

  return i - 1;
}

Any improvements to the implementation of the inverse Ackermann would be appreciated. I really wanted to use Math.min but I could not figure how to.

Note: I know this can be golfed more, and I know I can slow it down more. I'm more interested in my algorithm than scoring well in the challenge. Don't worry about keeping your suggestions/improvements golfed either :)

I know that you're supposed to create the slowest-growing function possible, but that doesn't mean you necessarily need to make your function the slowest possible (LoL!).

As far as I can determine, your code for the Ackermann is mathematically correct - according to both Wikipedia and my (admittedly poor) sense of mathematical intuition.

<BoringMaths>

However, the inverse Ackermann as defined by Wikipedia is supposed to be a two-parameter function, and as such you cannot imitate it using a one-parameter function.

Also, you want a do-while loop here, so that the loop runs at least once even if the constraining condition is false at the start, as well be for my suggestion below. I don't think the return should be i - 1 instead of i either, as you can potentially return 0 if it remains as-is, which is not a legal value.

What you could try, as the ending paragraph of the relevant section of the Wikipedia article mentions, is setting the parameter m to a constant - not n, and especially not making \$n=m\$ - although that works too.

Here, I'd suggest choosing \$m=0\$. Such a choice helps with golf-ability - you can get rid of the division and the Math.floor in one fell swoop - \$\lfloor\frac{m}{n}\rfloor\$ is always \$0\$, regardless of \$n\$ (assuming that NaN, when converted to integral type, is 0, as it is in Java). So the loop condition basically becomes \$Ack(i, 0) < log_2(n)\$. Note the less-than (\$\lt\$), not less-than-or-equal-to (\$\le\$): this is the proper negation of the condition greater-than-or-equal-to (\$\ge\$).

Mathematically, how about a constant function, which is in fact the slowest growing function as it doesn't grow at all? Why not just return a constant and be done with it?

I think the PPCG question's rules forbid submitting a constant function, so let's not dwell on that any more (but it does make for a cheap trick, doesn't it?)

</BoringMaths>

<NonCritical>

A few small improvements for your implementation in the readable version

Extract out Math.log2(n) into a variable.
I think ES6+ gets you let and const - use them instead of var. As far as I remember, the former gets you proper scoping and the latter allows you to declare a named constant. I think my point (1) should have a const declaration.

Type Coercion is not readable!

Don't use implicit Boolean coercion of numbers in code which you claim to be "readable" - it isn't. Use proper comparisons in readable code, and leave the cool weak-typing hackery for the golfed version. How much does writing out the actual condition improve the readability of the code?

Everything else seems to be fine - naming, indentation, and all.

</NonCritical>

Suggested Code:

function Ackermann(m, n) {
  if (m < 0 || n < 0) {
     throw new TypeError("Arguments to Ackermann must be non-negative");
  }
  if (m == 0) {
     return n + 1;
  }
  else if (n == 0) {
     return Ackermann(m - 1, 1);
  } else {
     return Ackermann(m - 1, Ackermann(m, n - 1));
  }
}

function inverseAckermann(n) {
  const log2_n = Math.log2(n);
  let i = 0;
  // Taking `m` to be the constant 0 in the definition of the inverse Ackermann function
  do {
      ++i; // Pre-increment implies less side-effects
  } while (Ackermann(i, 0) < log2_n);

  return i;
}


Searching for a pattern in a list of tuples I have written some code to search a for a pattern in a list of tuples.

It works, but I am sure that it can be improved. Any feedback or ideas would be greatly appreciated.

The list of tuples I am searching in looks like this:

  x = [('A', True), ('B', True), ('C', False), ('D', False),
       ('X', False), ('E', True), ('F', True), ('G', True)]

I want to find X and then also grab all the values in the tuple adjacent to X that have a value of true.

So the expected output would be:

  X E F G

For:

  y = [('X', True), ('1', True), ('2', True), ('3', True), ('4', False), ('5', True)]

The expected output would be:

  X 1 2 3

For:

  z = [('Dog', True), ('Cow', False), ('Shark', True), ('Cat', True), ('X', False)]

The expected output would be:

  Shark Cat X

For:

  z = [('C', True), ('X', False), ('D', True), ('C', False), ('S', True)]

The expected output would be:

  C X D

This is my code:

test = [('A', False), ('B', False), ('C', True), ('D', True),
     ('E', True), ('X', False), ('G', True), ('H', False)]

def find_x(x):
    for item in x:
        if item[0] == 'X':
            index = x.index(item)
            return(index)
    return(None)

def look_around(x, index):
    left = 0
    right = len(x)-1
    found_left = False
    found_right = False

    #look left:
    if index != 0:
        for i in range(index, -1, -1):
            if x[i][1] == True:
                left = i
                found_left = True
            if x[i][1] == False and i != index:
                break

   #look right
    if index != len(x)-1:
        for i in range(index, len(x), +1):
            if x[i][1] == True:
                right = i
                found_right = True
            if x[i][1] == False and i != index:
                break

    if found_left and found_right:
        return(left, right)
    elif found_left and not found_right:
        return(left, index)
    elif not found_left and found_right:
        return(index, right)
    else:
        return (index, index)

index_of_x = find_x(test)
if index_of_x != None:
    left, right = look_around(test, index_of_x)
    print "match:"
    for i in range (left, right+1, +1):
        print test[i][0], #comma prints everything on same line

If I understand correctly, your problem can be defined as:

take the longest uninterrupted chain of True elements ending in X
take X regardless of value
take all True elements after X until the first False one

So the actual position of X is not relevant, we only need to know if we encountered it or not. Based on this, we can generate the result with only one pass through the input list. This works if at most one X element is present.

Bellow you can see my quick and dirty version of the implementation:

from itertools import count, izip
from collections import namedtuple
Elem = namedtuple('Elem', ['name', 'value'])

def valid_elements(elem_lst):
    valid_elem = []
    found_x = False

    for elem in elem_lst:
        if elem.name == 'X':
            found_x = True
            valid_elem.append(elem)
        elif elem.value:
            valid_elem.append(elem)
        elif found_x:
            break
        else:
            valid_elem = []

    if found_x:
        return valid_elem
    return []

z = [Elem('C', True), Elem('X', False), Elem('D', True), Elem('C', False), Elem('S', True)]
print [elem.name for elem in valid_elements(z)]