n-th Fibonacci number with memoization I am learning Rust and decided to create a simple program to calculate n-th Fibonacci number. It uses a vector for memoization. The program works, but what would you suggest to improve here? Maybe store memo inside fib function as a static variable?
I tried to use u32 everywhere, but 48-th fib number doesn't fit into u32, so I decided to use u64.
use std::io;
use std::io::Write;
use std::str::FromStr;

fn fib(n: u32, memo: &mut Vec<u64>) -> u64 {
    if n < 2 {
        return n as u64;
    }
    else if memo[(n-1) as usize] != 0 {
        return memo[(n-1) as usize];
    }

    memo[(n-1) as usize] = fib(n - 1, memo) + fib(n - 2, memo);
    memo[(n-1) as usize]
}

fn main() {
    loop {
        print!("n = ");
        io::stdout().flush().unwrap();

        let mut str: String = String::new();
        io::stdin().read_line(&mut str)
            .expect("Error getting number");

        str.pop();
        match u32::from_str(&str) {
            Ok(n) => {
                let mut memo: Vec<u64> = vec![0; n as usize];
                println!("fib({})={}", n, fib(n, &mut memo));
                break;
            }
            Err(_e) => {
                println!("Error parsing number.");
                println!("Try again");
            }
        };
    }
}

I like this. Your code is clean and pretty readable. I recommend using rustfmt, if only because it will stop annoying people on the internet from recommending you use rustfmt. But the minor code formatting differences are not something to be overly concerned about, if you have a style and stick with it.
I have two major suggestions:

Don't populate the cache with a sentinel value (zero) and then test whether each value is zero or not. Instead, make the vector start small and grow as you populate it. To avoid reallocation, you can initialize it with a large capacity and small length. You can further simplify the logic by (a) using push to add a value to the cache when you know the length is n; and (b) initializing the cache to [0, 1], which will play into the next point:

Split fib into two functions: one that takes a cache as argument, and one that creates the cache inside itself (and calls the first one to do the heavy lifting). If you expose both of these in an API, then the user can decide whether reusing the cache is appropriate or not.

Here's the main code with both suggestions implemented:
fn fib_cached(n: u32, memo: &mut Vec<u64>) -> u64 {
    if (n as usize) < memo.len() {
        memo[n as usize]
    } else {
        let ret = fib_cached(n - 2, memo) + fib_cached(n - 1, memo);
        // At this point we know that memo.len() is at least n, because
        // fib_cached(n - 1, memo) will have populated memo[n-1]. Furthermore, if
        // memo.len() had been greater than n, we would have taken the other
        // branch of the if. There are no other functions capable of modifying
        // memo because we hold a &mut reference to it, so memo.len() is exactly n
        // and push(ret) will make memo[n] = ret.
        memo.push(ret);
        ret
    }
}

fn fib(n: u32) -> u64 {
    let mut cache = Vec::with_capacity(n as usize);
    cache.extend(&[0, 1]);
    fib_cached(n, &mut cache)
}

Here are some more observations, in no particular order:

Even this modified version is somewhat fragile, since it's easy to call fib_cached with a too-small cache, which may panic. If you were writing a library for others to use, I would suggest making a wrapper type around Vec<u64> so you can insist on correctly-initialized caches. This would be a start:
struct FibCache {
    data: Vec<u64>,
}

impl FibCache {
    fn with_capacity(n: usize) -> Self {
        let mut data = Vec::with_capacity(n);
        data.extend(&[0, 1]);
        FibCache {data}
    }
}

fn fib_cached(n: u32, memo: &mut FibCache) -> u64 {
    if (n as usize) < memo.data.len() {
        memo.data[n as usize]
    } else {
        let ret = fib_cached(n - 2, memo) + fib_cached(n - 1, memo);
        memo.data.push(ret);
        ret
    }
}

fn fib(n: u32) -> u64 {
    let mut cache = FibCache::with_capacity(n as usize);
    fib_cached(n, &mut cache)
}

This means nobody can call fib_cached without creating a FibCache object with a constructor you define. It also lets you make backwards-compatible changes to the type of cache -- you could use a HashMap, as 6005 suggests, or another data structure, without changing the outward-facing API of the library.

n as usize is another source of fragility. Casting between integer types can have surprising results, so you're usually well-advised to use usize::try_from(n).unwrap() instead, which will panic if the conversion can't be done. In this code, I don't regard this as a major issue because u32 is losslessly convertible to usize on virtually all Rust targets and because any n that would overflow a usize, on any target, is already too large for this algorithm. Just be aware you should normally use the conversion traits instead of casting.

Changing fib to use internal static state is pretty simple, using lazy_static or once_cell. I'll use lazy_static here, and assume the FibCache API above:
fn fib(n: u32) -> u64 {
    use lazy_static::lazy_static; // 1.4.0
    use std::sync::Mutex;
    lazy_static! {
        static ref CACHE: Mutex<FibCache> = Mutex::new(FibCache::with_capacity(10));
    }
    fib_cached(n, &mut *CACHE.lock().expect("Could not lock static cache"))
}

Whether this is a good idea or not is rather up for debate. Ultimately it's replacing contention for one resource (the heap) with contention for another resource (the lock). Neither is necessarily the best choice, which is why it's important to expose fib_cached as well so your users aren't stuck with your choice of implementation for fib. Making access to FibCache lock-free could make static memory much more appealing by removing the contention aspect.

Don't use str as the name of a variable, since it's also the name of a built-in type. It will mess with syntax highlighting (as it does on this very site).

Obviously, this is a slow and wasteful algorithm for calculating Fibonacci numbers. So I assume what you're looking for is feedback on the general shape and style of the solution, not the choice of algorithm, and I've written this review based on that assumption. Peter's review deals with algorithmic improvements.


Framed external link redirect - vBulletin I'm just looking for some review for my code.  This is a script that checks for outbound links in posts in vBulletin forums, and if it is an external link it will redirect to a template which will load the external site in an iFrame that I also coded, inspired by themeforest and stumbleupon's way of showing external links.

This is my first PHP script on my own that is a little more advanced.  I'm only use to the basics, so any critique is good critique, it's the only way I can learn.

I don't even know if I went about doing this correctly, but it seems to work how I'm wanting currently.  Let me know what I can do differently.

To get an idea of this you can see it in action - Here

HTTPS links don't work, however, I still want them redirected with this so all outbound links on my site will derive from the same page, so I included a button for if a certain page won't load you can click that button and it will take you to the page.

$curdir = getcwd ();
chdir('./');
define('NO_REGISTER_GLOBALS', 1);
define('THIS_SCRIPT', 'drc-redirect');
$globaltemplates = array('drc_redirect');
require_once('./global.php');

if ($_GET) {
    foreach ($_GET as $key => $value) {
        if ($key=="l") {
            $url1 = $value;
        }
        else {
            $url2 = "&". $key. "=". $value;
        }
    }

    $out = strip_tags($url1. $url2);
}

$site = parse_url($_SERVER['HTTP_HOST']);
$site = $site[path];
$url = parse_url($out);

if ($url[scheme] == "http" OR "https" && $url[host]) {
    if ($site == $url[host]) {
        Header( "Location: ". $out );
    }
    else {
        eval('print_output("' . fetch_template('drc_redirect') . '");');
    }
}

I would like to add a few things to it still but first wanted to get some suggestions on my code.

Here's how I optimized your code:

$curdir = getcwd();
chdir('./');
define('NO_REGISTER_GLOBALS', 1);
define('THIS_SCRIPT', 'drc-redirect');
$globaltemplates = array('drc_redirect');
require_once('./global.php');

if (isset($_GET['l'])) {
    //format passed url
    $passed_url = $_GET['l'];
    unset($_GET['l']);
    if(count($_GET))
        $passed_url = $passed_url . '&' . http_build_query($_GET);

    //deals w/ the display if $passed_url is a valid URL
    if (filter_var($passed_url, FILTER_VALIDATE_URL)) {
        $site = parse_url($_SERVER['HTTP_HOST']);
        $site = $site['path'];
        $url = parse_url($passed_url);

        //the passed url is not an external link
        if ($site == $url['host']) {
            Header( "Location: ". $_SERVER['QUERY_STRING'] );
        }
        //the passed url is an external link
        else {
            eval('print_output("' . fetch_template('drc_redirect') . '");');
        }
    }
}

Below are the modifications that I did:

Using isset() for checking $_GET.
Using http_build_query() instead of looping through each element in $_GET.
Using filter_var() to check if the passed URL is valid.

Explanations in detail:

isset() allows for proper checking of the intended variable from $_GET. It also returns a boolean for the if condition, as opposed to just placing $_GET which is an array.
The looping of the $_GET variable will backfire if there are at least 2 elements in it. The else condition on the foreach loop overwrites the previous values for $url2. Now this can be corrected to having every $key/$value be concatenated everytime. We can utilize http_build_query() instead because it can get the job done, and there's no need to loop the $_GET array.
Finally, parse_url() sometimes is not able to return the scheme of a given url string. filter_var() along with the FILTER_VALIDATE_URL filter can validate URLs regardless whether the scheme was specified or not.


Java function to rotate an image by 180 degrees I want to 180 rotate an Image. Here's what I wrote but it seems way too complicated. Is there any way to do this in a simpler way?

public void rotateImage180Deg() {
        for (int x = 0; x < width / 2; x++) {

            int y = width - 1 - x;
            for (int i = x; i < y; i++) {
                int offset = i - x;
                RGBColor top = imageMatrix[x][i];
                imageMatrix[x][i] = imageMatrix[y][y - offset];
                imageMatrix[y][y - offset] = top;
                RGBColor leftBottom = imageMatrix[y - offset][x];
                imageMatrix[y - offset][x] = imageMatrix[i][y];
                imageMatrix[i][y] = leftBottom;
            }
        }
    }

Your code is complicated and hard to read, say the least. So I'll start discussing dimensions of arrays in java, and ease into a re-implementation of your code. Some lessons to be learned is added to the end.

If you create a two-dimensional array in java, int[2][3] twoDim, and use Arrays.deepToString(twoDim) (from java.util.Arrays) to print it. It'll print something ala: [[0, 1, 2], [3, 4, 5]]. And twoDim[1][2] has the value 5. This I interpret that the first dimension is zero-based rows, and the second dimension is zero-based columns.

In your code you intermix x and y and use the width when calculating the y (which I read as height or rows). In addition you add an i and offset to the mix which makes everything very hard to read.

So I plain and simply gave up trying to understanding what your algorithm tries to do. The last two variables further add to the confusion with one top and the other leftBottom. Is the top to the right or left, and what happens when you start iterating?

Helper methods

In order to try figuring out what is really happening I made these three helper methods and variables:

final int height = 3;
final int width = 3;
int[][] imageMatrix = new int[height][width];

public void swap(int x1, int y1, int x2, int y2) {
  int tmp;
  tmp = imageMatrix[y1][x1];
  imageMatrix[y1][x1] = imageMatrix[y2][x2];
  imageMatrix[y2][x2] = tmp;
}

public void resetMatrix() {
  int k = 0;
  for (int row = 0; row < height; row++) {
    for (int col = 0; col < width; col++) {
      imageMatrix[row][col] = k++;
    }
  }
}

public void printMatrix(String title) {
  System.out.println(title);
  for (int row = 0; row < height; row++) {
    for (int col = 0; col < width; col++) {
      System.out.printf("%2d ", imageMatrix[row][col]);
    }
    System.out.println();
  }
  System.out.println();
}

This allowed me to have a slightly simpler structure to work with, and I can display the matrix whenever I feel like it.

On a square image of 3x3 I got the following output:

 Original
  0  1  2
  3  4  5
  6  7  8

 rotateImage180Deg
  8  7  6
  5  4  3
  2  1  0

I then proceeded to changed your algorithm using the swap helper method (and added some space to align swap coordinates), this gave this code:

public void rotateImage180DegII() {
     for (int x = 0; x < width / 2; x++) {

         int y = width - 1 - x;
         for (int i = x; i < y; i++) {
             swap(i, x,            y - i + x, y);
             swap(x, y - i + x,    y, i);
         }
     }
 }

Output was the same, as expected, and it does fail with the same (but more on that later). In the code above, you clearly see how strange
the usage of x and y becomes, as you intermix them freely.

Re-implementation

Finally I proceeded to re-implement the algorithm. My thought was to simply start at the upper left and swap with bottom right, and move through the entire row. Repeat for next row(s). After doing this I discovered that I had to cover a special case when there was an odd number of rows, so I added a second loop to handle that case. This resulted in the following code:

public void rotateImage180DegIII() {
  for (int x = 0; x < width; x++) {
    for (int y = 0; y < (height / 2); y++) {
      swap(x, y,         width - 1 - x, height - 1 - y);
    }
  }

  if (height % 2 == 1) {
    int mid_y = height / 2;
    for (int x = 0; x < (width / 2); x++) {
      swap(x, mid_y,      width - 1 - x, mid_y);
    }
  }
}

Even without any comments, I think this code reads a lot clearer. The first double for loop reads as swap an element at x, y with an element at width - 1 - x, height - 1 - y. Repeat this for the entire x-column range, and half the height range.

With top left as 0, 0, this reads as swap an element from top left with an element at bottom right. Move to the right for the first element, and to the left for the second element. Repeat until row is swapped, and then repeat for the second (and second to last row).

Finally, if there is an odd number of rows, swap the far left element of the middle row with the far right element on the same row. Increase x and repeat until row is swapped.

Changing dimensions

This code actually handles various dimensions, whereas your original code gets ArrayIndexOutOfBoundsException whenever the second dimension is larger than the first. And it produces strange examples if the second dimension is less than the first.

Here is the output from a 3x3 run (the first line is the Arrays.deepToString(imageMatrix) output):

[[0, 1, 2], [3, 4, 5], [6, 7, 8]]

Original
 0  1  2
 3  4  5
 6  7  8

rotateImage180DegIII
 8  7  6
 5  4  3
 2  1  0

rotateImage180DegII
 8  7  6
 5  4  3
 2  1  0

Nicely rotated by both alternatives. Let's continue with an 5x3 run:

[[0, 1, 2], [3, 4, 5], [6, 7, 8], [9, 10, 11], [12, 13, 14]]

Original
 0  1  2
 3  4  5
 6  7  8
 9 10 11
12 13 14

rotateImage180DegIII
14 13 12
11 10  9
 8  7  6
 5  4  3
 2  1  0

rotateImage180DegII
 8  7  6
 5  4  3
 2  1  0
 9 10 11
12 13 14

I think my output make sense, but not yours. I'm not sure what is happening, or why. But it does look strange. Finally let us do a 3x5 run:

[[0, 1, 2, 3, 4], [5, 6, 7, 8, 9], [10, 11, 12, 13, 14]]

Original
 0  1  2  3  4
 5  6  7  8  9
10 11 12 13 14

rotateImage180DegIII
14 13 12 11 10
 9  8  7  6  5
 4  3  2  1  0

Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: 4
  at Main.swap(Main.java:13)
  at Main.rotateImage180DegII(Main.java:64)
  at Main.main(Main.java:95)
exited with non-zero status

Lessons to be learned

Work on the variable naming – Using x and y which have a very strong co-nation to coordinates, and intermixing them is bad. You loose control over what they indicate.
Perform wider tests – I don't think you've tested with alternating dimensions, as you then would have discovered the issues I point out in the Changing dimensions tests
Add helper methods – Using helper methods can both help in visualizing the process, and reduce the clutter so you can focus on the algorithm. Compare your original code with the code using swap. Just that is a nice improvement. Adding something like a printMatrix and resetMatrix can also be very helpful to understand what is happening.

Translating my alternate implementation to yours with your definition of imageMagic and RGBColor I trust you'll manage. Note that within the swap method the y coordinates needs to be in the first dimension, whilst the x coordinates are in the second dimension. In other words, keep your head straight on when handling dimension indexes.

Update: Changed to non-static methods, which purely was a result of laziness in my test setup based upon the static main() method, and not proper object initialization.


Note which colour button was clicked I have a script that changes the colour, it selects the colour by the id, then it listens what colour was clicked, and if the colour choose was black or whatever other colours, it will do the job.

However, there is a lot of repetition with the if statement. Is there a way to refactor this?

How about if color choosen ${colorClicked} then style bg with = the ${colorAssosiated} or something like that?

Can we re-factor this? But with ES5 best.

var black = document.getElementById("black");
var blue = document.getElementById("blue");
var red = document.getElementById("red");

//01. Listen for button click
var buttonClick = this.addEventListener ("click", function(e) {
    console.log(e.target.id + " button was clicked");
    var colorChosen = e.target.id;

    //02. Change background colour
      if (colorChosen == "black"){
        document.body.style.background = "black";
        console.log('You chose Black!');
      } else if (colorChosen == "blue") {
        document.body.style.background = "blue";
        console.log('You chose Blue!');
      } else if (colorChosen == "red") {
        document.body.style.background = "red";
        console.log('You chose Red!');

      }

});

I think in your case you can just do this:

document.body.style.background = colorChosen;

Avoiding all the ugly if else part.

A more robust solution will be using a lookup table to decouple the DOM ids from the proper colors string:

const colorFor = {
    "somethingRed": "red",
    "somethingBlue": "blue",
    "somethingBlack": "black"
};

...

document.body.style.background = colorFor[colorChosen];

With somethingBlaBla I mean a meaninful name that not contains the color.
The purpose is to contain the changes in case you want a different color in the future or if you want that more options have the same color.

Another improvement is to have a guard condition to avoid errors or weird situations:

var buttonClick = this.addEventListener ("click", function(e) {
    console.log(e.target.id + " button was clicked");
    var colorChosen = e.target.id;

    if (!colorFor[colorChosen])
        return false;

    document.body.style.background = colorFor[colorChosen];

});

Instead of just return from the function you can log an error or pup up a dialog with a warning to inform the user.
Or you can reset to a default color the background.

In the same way you can use a style instead of setting the color on the DOM element.

This could be better in some situation, depends on your app code convention.


Statement checker c# using forms and frontend I'm just here for suggestions and code optimisations, what i could've done better, are there any better coding practices...
There's github repo link if you're interested in the code with a form too https://github.com/throtz/Operators
You basically have 2 textboxes in which you enter numbers and choose with a combobox what you want to do with those two numbers.
using System;
using System.Windows.Forms;

namespace Operators
{
    public partial class Operators : Form
    {
        public Operators()
        {
            InitializeComponent();
        }
        private void Form1_Load_1(object sender, EventArgs e)
        {
            comboBox.Items.AddRange(new string[] { "+", "-", "*", "/", "%", ">", "<", "=", ">=", "<=" });
        }
        private void ButtonCheck_Click_1(object sender, EventArgs e)
        {
            if (sender is null)
            {
                throw new ArgumentNullException(nameof(sender));
            }

            int a = Convert.ToInt32(textBox1.Text);
            int b = Convert.ToInt32(textBox2.Text);

            Nah(a, b);
        }
        private void Nah(int a, int b)
        {
            switch (comboBox.SelectedItem.ToString())
            {
                case "+":
                    MessageBox.Show("Sum of thenumbers entered is: " + (a + b));
                    break;
                case "-":
                    MessageBox.Show("Subtraction of the numbers entered is: " + (a - b));
                    break;
                case "*":
                    MessageBox.Show("Multiplication of the numbers entered is: " + (a * b));
                    break;
                case "/":
                    MessageBox.Show("Division of the numbers entered is: " + (a / b));
                    break;
                case "%":
                    MessageBox.Show("Modulo the two numbers is: " + (a % b));
                    break;
                case ">":
                    MessageBox.Show("First number is bigger than the secound one " + (a > b));
                    break;
                case "<":
                    MessageBox.Show("First number is smaller than the secound one " + (a < b));
                    break;
                case "=":
                    MessageBox.Show("Numbers are equal: " + (a == b));
                    break;
                case ">=":
                    MessageBox.Show("First number is bigger or equal to number two " + (a >= b));
                    break;
                case "<=":
                    MessageBox.Show("First number is smaller or equal to number two " + (a <= b));
                    break;
                default:
                    MessageBox.Show("Illegal operation.");
                    break;
            }
        }
    }
}

Review
It seems that you are trying to make a simple calculator with number comparison in C#. There are some problems in this implementation.
ToInt32:
Do you expect the input of calculation is always in int format? How
about floating numbers?
Nah method:

Naming:
I have no idea how's the name Nah imply the operation in the method. Something like run or perform are better than Nah.

Integer Division:
Notice that the execution of 3 / 2 in this implementation is 1.
It is a kind of integer division. Is the integer division result
the correct output you expected?

MessageBox.Show Part:
In C# 6, you can use string interpolation and make MessageBox.Show("Sum of thenumbers entered is: " + (a + b)); into MessageBox.Show($"Sum of thenumbers entered is: {(a + b)}");. Interpolated strings are useful in string displaying and it's commonly used with Console.WriteLine or MessageBox.Show.

Error Handling

Null or empty string:
You use textBox1.Text and textBox2.Text as a way to read numbers from user input. How about the case of empty strings (user click button Check directly without type any character)? If textBox.Text passes in Convert.ToInt32 directly and the content is empty, System.FormatException will occur. Maybe String.IsNullOrEmpty check can be used here.

DivideByZeroException
DivideByZeroException Class can be used to handle an attempt to divide an number by zero.


Java Generics - Write a generic method to find the maximal element in the range [begin, end] of a list I'm a Java beginner, going through the Generics Questions and Exercises in Oracle's The Java Tutorials.

Here's my solution to #8:

  Write a generic method to find the maximal element in the range [begin, end] of a list.

All suggestions for improvement are welcome. I'd especially like feedback on the following:

Java library usage
Algorithm performance
Exceptions

(imports omitted for brevity)

@SuppressWarnings("serial")
class MaximalElementList<E extends Comparable<E>> extends ArrayList<E> {

    /**
     * @param begin - list index to start iterating from (inclusive)
     * @param end - list index to iterate up to (exclusive)
     * @return max elem in list as determined by the compareTo of the element type.
     */
    public E getMaxInRange(int begin, int end) {
        E max;
        if (isEmpty())
            throw new IllegalStateException("Can't get a max element from an empty list.");
        else {
            max = this.get(begin);
            for (E elem : this.subList(begin, end)) {
                if (elem.compareTo(max) > 0) {
                    max = elem;
                }
            }
        }
        return max;
    }
}

So, here's the thing, what you have is not a generic method. What you have is a generic class.

Now, it's a clearly defined generic class, but, it is not what the question asked for. (as a generic class it has a number of issues too, but let's get the method/class issue resolved first).

Generic methods

A generic method is just that, a method, except the parameters (or return value) are of a generic type. A generic method always has a <...> structure before the return-type declaration. A normal method is:

public RType methodName(P1Type p1, P2Type p2, ....) {...}

(where RType is the return type, and P1Type is parameter1 type, etc.).

A generic method has the <...> before the return type, and that construct is used somewhere in the method signature... for example:

public <T> RType methodName(T t, SomeType sparm) {....}

The above is a generic method that has the generic type T as a parameter.

Your Class

So, having stated that your solution is a generic class, not a generic method, let's assume the exercise goal was to produce a generic class. What then?

Unless you have exceptional reasons, don't extend ArrayList, "compose" it instead. Have a class that does not extend ArrayList, and have a class field instead, like:

class MaximalElementList<E extends Comparable<E>> {

    List<E> data = ......

Your generics here are OK, no problem with <E extends Comparable<E>>
Your code should use a guard-condition instead of an else... let me explain. Your code has an if/else:

public E getMaxInRange(int begin, int end) {
    E max;
    if (isEmpty())
        throw new IllegalStateException("Can't get a max element from an empty list.");
    else {
        max = this.get(begin);
        for (E elem : this.subList(begin, end)) {
            if (elem.compareTo(max) > 0) {
                max = elem;
            }
        }
    }
    return max;
}

That should instead be:

public E getMaxInRange(int begin, int end) {
    if (isEmpty()) {
        throw new IllegalStateException("Can't get a max element from an empty list.");
    }

    E max;
    max = this.get(begin);
    for (E elem : this.subList(begin, end)) {
        if (elem.compareTo(max) > 0) {
            max = elem;
        }
    }
    return max;
}

Note now, that the max is a messy variable, it can just be:

    E max = this.get(begin);
    for (E elem : this.subList(begin, end)) {
        if (elem.compareTo(max) > 0) {
            max = elem;
        }
    }
    return max;

Your use of a sublist is smart, but I would consider it to be overkill in this case. How about a simpler implementation:

    E max = this.get(begin);
    for (int i = begin + 1; i < end; i++) {
        if (get(i).compareTo(max) > 0) {
            max = get(i);
        }
    }
    return max;

That is now some logic which I think would work well.

Making it a method

Putting that logic in a generic method would "simply" mean:

public <E extends Comparable<E>> E max(List<E> data, int begin, int end) {
    E max = data.get(begin);
    for (int i = begin + 1; i < end; i++) {
        if (data.get(i).compareTo(max) > 0) {
            max = data.get(i);
        }
    }
    return max;

}

With that method, for example, you could do:

List<String> data = Files.readAllLines(Paths.get("some file.txt"));
String maxLine = max(data, 0, 10);


Writing a safe class method in terms of a dangerous one I commonly need to trim characters from the end of a string (when building SQL queries, I have trailing commas, brackets, even words sometimes), so I wrote the following:

class String
  def trim(amount=1)
    self[0..-(amount+1)]
  end

  def trim!(amount=1)
    self[0..-1] = trim(amount)
  end
end

The dangerous method, trim!, calls the safe method, trim. According to the Ruby Style Guide, this is backwards.

How do I rewrite this so the safe method calls the dangerous method, without changing the functionality?

Sample output:

str = "Hello World!"
puts str            #=> "Hello World!"
puts str.trim       #=> "Hello World"
puts str.trim 7     #=> "Hello"
puts str            #=> "Hello World!"
puts str.trim!      #=> "Hello World"
puts str.trim! 6    #=> "Hello"
puts str            #=> "Hello"

As hinted to in the Ruby Style Guide, the Object#dup method returns a duplicate of self to be passed to other methods without danger of altering self. The solution is as simple as moving the trim logic into the trim! function and having trim call dup.trim!

class String
  def trim!(amount=1)
    self[0..-1] = self[0..-(amount+1)]
  end

  def trim(amount=1)
    dup.trim! amount
  end
end

This returns the same sample output as above.


How to make it more Scala compatible? When you write a code in python code people always ask to make it more pythonic. I am not sure if there is a term for scala or not. However, I am pretty sure my following code use one or two scala feature.

Objective

Read a property file, which has location to XML file
Read source, destination and some other properties from property and XML File
Copy Data from one location to another location.

FileSystem.scala

bstract class FileSystem(flagFileURI: String){

  def getRecordCount(properties: Properties): String = {
    properties.getProperty("recordCount")
  }

  def getAuditID: String = {
    val instant: Instant = Instant.now
    val zoneId: ZoneId = ZoneId.of("Canada/Eastern")
    val auditIdFormatter: DateTimeFormatter = DateTimeFormatter.ofPattern("yyyyMMddHHmmssSSS")
    val auditId = ZonedDateTime.ofInstant(instant, zoneId).format(auditIdFormatter)
    auditId
  }

  def getSourcePath(properties: Properties): Path = {
    val dataFileURI: String = properties.getProperty("dataFileURI")
    val srcPath = new Path(dataFileURI)
    srcPath
  }

  def getMetaDataFileURI(properties: Properties): String = {
    properties.getProperty("metadataFileURI")
  }

  def getIDPDataDate(properties: Properties): String = {
    properties.getProperty("idpDataDate")
  }
  def getDestinationPath(properties: Properties, metaDataFileURI: String, IDPDataDate: String, dataFileName: String): Path = {
    val l0DirLocation: String = ConfigFactory.load().getString("sparkFramework.hdfs.l0dirlocation")

    val frameWorkUtils = new FrameworkUtils()
    val sourceSystem: SourceSystem = frameWorkUtils.getSourceSystem(metaDataFileURI)
    val schemaName: String = frameWorkUtils.getSchemaName(sourceSystem)
    val tableName: String = frameWorkUtils.getTableName(sourceSystem)

    val destPath = new Path(l0DirLocation + schemaName + "/" + tableName + "/idp_data_date=" + IDPDataDate + "/" + dataFileName)

    destPath
  }

  def getDataFileName(srcPath: Path, auditID: String): String = {
    val dataFileName: String = srcPath.getName + "_" + getAuditID
    dataFileName
  }

}

HDFileSystem.scala

class HDFileSystem(flagFileURI: String) extends FileSystem(flagFileURI) {

  def copyFromLocalFile: (String, String, String, String, String) = {

    val properties: Properties = new Properties()
    val source: InputStreamReader = Source.fromFile(flagFileURI).reader
    properties.load(source)

    val metaDataFileURI = getMetaDataFileURI(properties)
    val srcPath = getSourcePath(properties)
    val auditId = getAuditID
    val dataFileName = getDataFileName(srcPath, auditId)
    val IDPDataDate = getIDPDataDate(properties)
    val destPath = getDestinationPath(properties, metaDataFileURI, IDPDataDate, dataFileName)
    val recordCount = getRecordCount(properties)

    val hadoopConf = new Configuration()
    val hdfs = FileSystem.get(hadoopConf)
    hdfs.copyFromLocalFile(true,false,srcPath, destPath)
    (IDPDataDate,recordCount,auditId,metaDataFileURI,dataFileName)

  }
}

Main.scala

 val hdFileSystem = new HDFileSystem(flagFileURI = args(0))
    val (idpDataDate,count,auditId,metadataFileURI,fileName) = hdFileSystem.copyFromLocalFile

What you have isn't bad, but I feel like you're creating too many variables. For example, in getAuditID and other methods, you assign an object to a variable and immediately return it. You also have some unnecessary type annotations.
def getAuditID: String = {
    val instant: Instant = Instant.now
    val zoneId: ZoneId = ZoneId.of("Canada/Eastern")
    val auditIdFormatter: DateTimeFormatter = DateTimeFormatter.ofPattern("yyyyMMddHHmmssSSS")
    val auditId = ZonedDateTime.ofInstant(instant, zoneId).format(auditIdFormatter)
    auditId
}

After eliminating that, I got this:
abstract class FileSystem(flagFileURI: String) {

  def getRecordCount(properties: Properties): String =
    properties.getProperty("recordCount")

  def getAuditID: String =
    ZonedDateTime
      .ofInstant(Instant.now, ZoneId.of("Canada/Eastern"))
      .format(DateTimeFormatter.ofPattern("yyyyMMddHHmmssSSS"))

  def getSourcePath(properties: Properties): Path =
    new Path(properties.getProperty("dataFileURI"))

  def getMetaDataFileURI(properties: Properties): String =
    properties.getProperty("metadataFileURI")

  def getIDPDataDate(properties: Properties): String =
    properties.getProperty("idpDataDate")

  def getDataFileName(srcPath: Path, auditID: String): String =
    srcPath.getName + "_" + getAuditID

  def getDestinationPath(
      properties: Properties,
      metaDataFileURI: String,
      IDPDataDate: String,
      dataFileName: String
  ): Path = {
    val l0DirLocation =
      ConfigFactory.load().getString("sparkFramework.hdfs.l0dirlocation")
    val frameWorkUtils = new FrameworkUtils()
    val sourceSystem =
      frameWorkUtils.getSourceSystem(metaDataFileURI)
    val schemaName = frameWorkUtils.getSchemaName(sourceSystem)
    val tableName = frameWorkUtils.getTableName(sourceSystem)

    new Path(
      l0DirLocation + schemaName + "/" + tableName + "/idp_data_date=" + IDPDataDate + "/" + dataFileName
    )
  }
}

However, your other method, copyFromLocalFile, could also be improved. The return type is a tuple of 5 strings, which to me seems much too complex. Instead of that, I'd suggest making a case class that makes it obvious what each of those strings mean.
case class FileInfo(
    IDPDataDate: String,
    recordCount: String,
    auditId: String,
    metaDataFileURI: String,
    dataFileName: String
)

Then you can define this method in FileSystem to get all the information at once
def getFileInfo(properties: Properties): FileInfo = {
    val auditId = getAuditID
    FileInfo(
      getIPDDataDate(properties),
      getRecordCount(properties),
      auditId,
      getMetaDataFileURI(properties),
      getDataFileName(getSourcePath(properties), auditID)
    )
}

After that, you can turn your method into something like this:
def copyFromLocalFile: FileInfo = {

    val properties: Properties = new Properties()

    properties.load(Source.fromFile(flagFileURI).reader)

    val fileInfo = getFileInfo(properties)

    FileSystem
      .get(new Configuration())
      .copyFromLocalFile(
        true,
        false,
        fileInfo.srcPath,
        getDestinationPath(
          properties,
          fileInfo.metaDataFileURI,
          fileInfo.IDPDataDate,
          fileInfo.dataFileName
        )
      )

    fileInfo
}

You can still destructure the result:
val FileInfo(idpDataDate, count, auditId, metadataFileURI, fileName) =
  hdFileSystem.copyFromLocalFile

Of course, I don't know about the whole structure of your program, so this might not work for you.
Link to Scastie


Bitcoin wallet address and private key generator I wanted to learn how to create a Bitcoin wallet in code. I used as reference this guide which has code examples in JavaScript.

I wrote my implementation in Python. Here is the resulting code:

#!/usr/bin/env python

'''
Creates BitCoin Wallet complaint credentials:
- Public Key
- Private Key
- Private Key (Wallet Import Format)
'''

import hashlib
import base58
import ecdsa

from ecdsa.keys import SigningKey
from utilitybelt import dev_random_entropy
from binascii import hexlify, unhexlify

def random_secret_exponent(curve_order):
    while True:
        random_hex = hexlify(dev_random_entropy(32))
        random_int = int(random_hex, 16)
        if random_int >= 1 and random_int < curve_order:
            return random_int

def generate_private_key():
    curve = ecdsa.curves.SECP256k1
    se = random_secret_exponent(curve.order)
    from_secret_exponent = ecdsa.keys.SigningKey.from_secret_exponent
    return hexlify(from_secret_exponent(se, curve, hashlib.sha256).to_string())

def generate_public_key(private_key_hex):
    hash160 = ripe_hash(private_key_hex)
    public_key_and_version_hex = b"04" + hash160
    checksum = double_hash(public_key_and_version_hex)[:4]
    return base58.b58encode(public_key_and_version_hex + checksum)

def ripe_hash(key):
    ret = hashlib.new('ripemd160')
    ret.update(hashlib.sha256(key).digest())
    return ret.digest()

def double_hash(key):
    return hashlib.sha256(hashlib.sha256(key).digest()).digest()

def generate_private_key_wif(private_key_hex):
    private_key_and_version = b"80" + private_key_hex
    checksum = double_hash(private_key_and_version)[:8]
    hashed = private_key_and_version + checksum
    return base58.b58encode(private_key_and_version + checksum)

def main():
    private_key_hex = generate_private_key()
    public_key_hex = generate_public_key(private_key_hex)
    private_key_wif_hex = generate_private_key_wif(private_key_hex)
    print("Private Key: " + private_key_hex)
    print("Public Key: " + public_key_hex)
    print("Private Key (WIF Format): " + private_key_wif_hex)

if __name__ == '__main__':
    main()

How to run:

Private Key: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
Public Key: 2Uc973e5adf5867512dd7a21638810bd53422
Private Key (WIF Format): xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx

I have my concerns about the correctness of the generated keys, as well as for the private key in WIF format. In theory it should be a short string.

IMPORTANT!!!
Not a Python tip, but rather a MUST when it comes to sensitive information (quote from @nzall's comment):

Those private keys should NEVER, EVER be posted in a location even
as remotely public as a code review site. More, replacing them
is not enough. For all purposes, you should now view the original keys as compromised and must NEVER EVER EVER use them again for
bitcoin storage. Doing so runs high risk of losing the bitcoins stored
in them

I just made this tip a bit more seeable for everybody.

Another three small observations, in addition to what @Graipher said.
In Python you can "chain" comparison operations which just means they are "and"ed together.
if random_int >= 1 and random_int < curve_order:
    return random_int

In your case, it'd be like this:
if 1 <= random_int < curve_order:
    return random_int

Read more about it here

More, I'd recommend you use .format() when it comes to printing:
print("Private Key: " + private_key_hex)
print("Public Key: " + public_key_hex)
print("Private Key (WIF Format): " + private_key_wif_hex)

Could be:
print("Private Key: {}".format(private_key_hex))
print("Public Key: {}".format(public_key_hex))
print("Private Key (WIF Format): {}".format(private_key_wif_hex))

You should also have two blank lines between your functions (instead of a single one)
This:
# ...
from binascii import hexlify, unhexlify

def random_secret_exponent(curve_order):
    while True:
        random_hex = hexlify(dev_random_entropy(32))
        random_int = int(random_hex, 16)
        if random_int >= 1 and random_int < curve_order:
            return random_int

def generate_private_key():
    curve = ecdsa.curves.SECP256k1
    se = random_secret_exponent(curve.order)
    from_secret_exponent = ecdsa.keys.SigningKey.from_secret_exponent
    return hexlify(from_secret_exponent(se, curve, hashlib.sha256).to_string())

# ...

Should be:
# ...
from binascii import hexlify, unhexlify

def random_secret_exponent(curve_order):
    while True:
        random_hex = hexlify(dev_random_entropy(32))
        random_int = int(random_hex, 16)
        if random_int >= 1 and random_int < curve_order:
            return random_int

def generate_private_key():
    curve = ecdsa.curves.SECP256k1
    se = random_secret_exponent(curve.order)
    from_secret_exponent = ecdsa.keys.SigningKey.from_secret_exponent
    return hexlify(from_secret_exponent(se, curve, hashlib.sha256).to_string())

# ...

For more details / suggestions / improvements regarding your code layout, you could take a look at PEP8, Python's official style guide.


LESS CSS, including support for shadowboxes I'm learning about using LESS and wanted to get anyone's input on if I'm using the concepts, syntax, etc. correctly.I know this might seem subjective and not the correct place to post, so please let me know if there is a more appropriate place to do this.

LESS code

.centered(@position: inline, @width: 100%){
    margin-left: auto;
    margin-right: auto;
    width: @width;
    display: @position;
}

.table-center(@width: 100%){
    width:@width;
    display:table!important;
    text-align:center;
    &:nth-child(1) {
        display:table-cell;
    }
}

.clear-border-radius{
     border-radius: initial;
    -webkit-border-radius: initial;
    -moz-border-radius: initial;
}

.shadowbox-format(@caption-font-size, @button-font-size, @shadowbox-margin) {
    display: block;
    position: absolute;
    min-height: 85px;
    width: 60%;
    margin: @shadowbox-margin;
    left: 20%;

    .shadowbox-caption {
        font-size: @caption-font-size;
        .table-center;
    }

    .shadowbox-button-wrapper {
        width: 35%;
        margin: 26px auto 0;

        & > a {
            .clear-border-radius;
            font-size: @button-font-size;
        }
    }
}

.home-slide-container{
    img{
        .centered(block);
        height:auto;
    }
}
.callout-header{
    text-align:center;
    width: auto;
    margin: 40px auto 0;
    border-bottom: 1px solid #9e9e9e;
    padding: 0 0 30px;
     @media (max-width: @screen-xs-max){
         width: 90%;
     }
          @media (min-width: @screen-sm-min){
         width: 60%;
     }
     span{
         font-size: 24px;
     }

}
 .carousel-shadowbox
 {
         text-transform: uppercase;
         color:#000;
         background-color:rgba(255,255,255,0.4);

     @media (max-width: @screen-xs-max)
     {
         background-color: white;
         opacity:1.0;

          .shadowbox-caption{
            .table-center;
             span{
                font-size: 16px;
                font-weight: 100;
                }
         }
     }
     @media (min-width: @screen-sm-min)
     {
        .shadowbox-format(20px, 16px, -91px auto 0);
     }
     @media (min-width: @screen-md-min)
     {
         .shadowbox-format(22px, 18px, -96px auto 0);
     }
     @media (min-width: @screen-lg-min)
     {
         .shadowbox-format(24px, 20px, -102px auto 0);
     }
}

 .callout{
     position:relative;
     margin-top: 5%;
     img{
         @media (max-width: @screen-xs-max){
             margin: 0 auto;
         }
     }
     .callout-text-container{
         padding:3px;
      @media (min-width: @screen-sm-min) and (max-width: @screen-sm-max)
         {
            bottom:22px;
         }

         min-height: 200px;
         .callout-title{
             padding-top: 5px;
             text-transform: capitalize;
             font-size: 22px;
             display:table;
             width:100%;
             text-align:center;
             margin-bottom: 25px;
             span{
                 display: table-cell;
             }
         }
         .callout-body{
             font-size: 15px;
             padding:8px;
             text-align:justify;
         }
         .button-wrapper{
             padding: 10px;
             a{
                 text-transform: capitalize;
                 border-bottom: 4px solid #cb2b06;
                 background-color: #e6431e;
                .clear-border-radius;
                 font-size: 16px;
             }
         }
     }
 }

Syntax is correct. When I was learning it was helpful to me to view the major LESS libraries out there and see what they were doing.

There are some mixin techniques that you will stumble upon as you advance your proficiency with the language. Also if you have JavaScript skills you can easily add additional logic to the mixins that make them even more powerful.

Once I got to a specific level with my proficiency I wrote my own library and I still use this to date for all of my projects. You can view it here - feel free to pick it apart and see if you find anything useful to use.

Edits suggested by @Dannnno

A few good techniques you will stumble upon are looping through the variables passed into a mixin. This is very powerful because as you create your mixins you will no longer have to create large pass ins that require comma separated notation like your '.shadowbox-format' mixin.

Also using @media inside of each mixin will result in larger then normal CSS output.

You might want to look into some of the Javascript features of LESS and build yourself a way to pass in array values to a holding variable so that when the compiler hits the @media mixin all values are itteraed in one @media statement rather then scattered throughout the document.


Finding the smallest number whose digits sum to N I tried my hand at a grade XII Board exam program:

  Given two positive numbers M and N, such that M is between 100 and 10000 and N is less than 100, find the smallest integer that is greater than M and whose digits add up to N. For example, if M = 100 and N = 11, the minimum number is 119 whose digits add up to N. Write a program to accept the numbers M and N from the user and print the smallest required number whose sum of all its digits is equal to N. Also, print the total number of digits present in the required number. The program should check for the validity of the inputs and display an appropriate message for an invalid input.

I solved the program, but as a curious person, I would love to know how to increase it's efficiency, compiler time delays, and if the program could be even shorter in length. So the primary question is, what modifications need be made in my code so that it becomes more compiler-friendly and efficient?

import java.util.Scanner;
public class ISC {
static Scanner sc =new Scanner(System.in);
static int m,n; static int ndigit;
void input(){
System.out.println("Enter the value of M: ");
m = sc.nextInt();
if (m>=100&&m<=10000) {
}
else {
    System.out.println("INVALID, Enter again: ");
    while (!(m>=100&&m<=10000)){
        m = sc.nextInt();
    }
}

System.out.println("Enter the value of N: ");
n = sc.nextInt();
if (n>=1&&n<=100){
    //do nothing
}
else {
    System.out.println("INVALID INPUT, Enter again");
    while (!(n>=1&&n<=100)){
        n = sc.nextInt();
    }
 }
}
 static int sumOfDigits(int n){
int sum = 0;
String a = Integer.toString(n); int digit;
for (int i = 0; i<a.length();i++){
    digit = Integer.parseInt(Character.toString(a.charAt(i)));
    sum += digit;
  }
return sum;
}
 static int getNo(){
ndigit = 0;
for (int i = m; i<=10000;i++){
    if (sumOfDigits(i)==n){
        ndigit = (Integer.toString(i)).length();
        return i;
    }
 }
return 0;
}
 public static void main(String[] args){
ISC a = new ISC();
a.input();
if (getNo()==0) System.out.println("NO NUMBER FOUND");
else {
  System.out.println("Minimum number is: " + getNo());
  System.out.println("Total number of digits: " + ndigit);
}
}
}

The program should look more like this:
public static void main(String[] args) {
    int m = askInt("Enter the value of M: ", 100, 10000),
        n = askInt("Enter the value of N: ", 1, 100);
    long solution = findSolution(m, n);
    System.out.println("Minimum number is: " + solution);
    System.out.println("Total number of digits: " + numberOfDigits(solution));
}

Notable differences:

Your program isn't object-oriented anyway, so there's no point in instantiating a and calling input() as an instance method.  (Furthermore, it's weird that input(), which is an instance method, stores its results in static variables instead of instance variables.)
There is a lot of repetition within input().  A general-purpose integer-prompting routine, used for both M and N, would be better.
m, n, and ndigit should not be static variables, as that makes them essentially global variables.  Any function in the class can alter their values as a side-effect, making your code harder to analyze — you have to read all of the code to understand any of it.
The solution needs to be a long, in case N is large. As @OleTange pointed out, if N is 99, then the solution must be at least 99999999999, which won't fit in an int. Note that brute-force enumeration is a very poor strategy for such large numbers.

Here's how I would write askInt():
static int askInt(String prompt, int min, int max) {
    int result;
    System.out.println(prompt);
    while ((result = sc.nextInt()) < min || result > max) {
        System.out.println("Invalid input, enter again: ");
    }
    return result;
}

Note that it returns a value to the caller, rather than setting some variable as a side-effect.

Assuming that we stick with your brute-force algorithm (and indeed there is a much faster way), these functions could be improved:
static int sumOfDigits(long num) {
    int sum = 0;
    while (num > 0) {
        sum += num % 10;
        num /= 10;
    }
    return sum;
}

static int numberOfDigits(long num) {
    return (int)Math.log10(num) + 1;
}

Arithmetic is preferable to converting numbers to strings and back.  Keep in mind that the stringification process itself involves similar arithmetic operations, and therefore must be slower than arithmetic.  Dealing with strings also incurs a cost for allocating memory for them.
I've chosen num as the parameter names to avoid confusion with the N in the problem statement.

As for the solution search itself, there are two bugs:

The problem calls for the solution to be strictly greater than M.
The problem does not state that the solution should be capped at 10000.  (For example, given M = 10000 and N = 2, the solution should be 10001.)

public static long findSolution(int m, int n) {
    for (long i = m + 1; ; i++) {
        if (sumOfDigits(i) == n) {
            return i;
        }
    }
}


Cleaning multiple build directories I'm cleaning build directories produced by GNOME build tool, JHBuild. This tool either downloads tarballs or clones git repositories, depending on set-up. After that, it proceeds to compilation (and then installation). Once in a while, something gets screwed up, and I need to clean the build directories so that I can start from scratch (because I don't know how to fix some of these problems).

Here's how I did it, and I'd like you to tell me if it can be improved:

import os
import subprocess

top_level = os.path.expanduser("~/src/gnome")
for filename in os.listdir(top_level):
    full_path = "{}/{}".format(top_level, filename)
    if os.path.isdir(full_path):
        cmd = "cd ~/src/gnome/{} && git clean -dfx".format(filename)
        if subprocess.call(cmd, shell=True) != 0:
            cmd = "cd ~/src/gnome/{} && make distclean".format(filename)
            if subprocess.call(cmd, shell=True) != 0:
                cmd = "cd ~/src/gnome/{} && make clean".format(filename)
                subprocess.call(cmd, shell=True)

full_path = "{}/{}".format(top_level, filename)

You can use os.path.join(top_level, filename) for that. That way it will also work on any system which does not use / as a directory separator (that's not really a realistic concern in this case, but using os.path.join doesn't cost you anything and is a good practice to get used to).

cmd = "cd ~/src/gnome/{} && git clean -dfx".format(filename)

First of all spelling out ~/src/gnome again is bad practice. This way if you want to change it to a different directory you have to change it in all 4 places. You already have it in the top_level variable, so you should use that variable everywhere.

On second thought you should actually not use top_level here, because what you're doing here is you're joining top_level and filename. However you already did that with full_path. So you should just use full_path here.

You should also consider using os.chdir instead of cd in the shell. This way you only have to change the directory once per iteration instead of on every call. I.e. you can just do:

if os.path.isdir(full_path):
    os.chdir(full_path)
    if subprocess.call("git clean -dfx", shell=True) != 0:
        if subprocess.call("make distclean", shell=True) != 0:
            subprocess.call("make clean", shell=True)

(Note that since full_path is an absolute path, it doesn't matter that you don't chdir back at the end of each iteration.)

Another good habit to get into is to not use shell=True, but instead pass in a list with command and the arguments, e.g. subprocess.call(["make", "clean"]). It doesn't make a difference in this case, but in cases where your parameters may contain spaces, it saves you the trouble of escaping them.


Jasmine unit tests for simple toggle class Using JavaScript/jQuery I created a simple toggle and started writing unit tests for it using Jasmine. I am not sure, however, if I should be testing for things like e.g. if "click" listener is registered when object is instantiated. Should this be done as part of unit test or rather E2E test using WebDriver or something? At the moment, I'm mostly checking if e.g. toggle function changes content of HTML as expected but am a bit lost in how far I should take my unit testing.

MySwitch = function(options) {

    this.options = options;
    this._$el = $(options.element);
    this._status = 0;
    this._$el.data("value-on") ? this._valueOn = this._$el.data("value-on") : this._valueOn = "ON";
    this._$el.data("value-off") ? this._valueOff = this._$el.data("value-off") : this._valueOff = "OFF";
    if (!this._$el.text()) this._$el.text(this._valueOff);

    this._$el.on("click", $.proxy(this.toggle, this));
};

MySwitch.prototype.toggle = function(el) {

    this._status ? (this._$el.text(this._valueOff), this._status = 0) : (this._status = 1, this._$el.text(this._valueOn));

    if (typeof this.options.onToggle === "function")
        this.options.onToggle.call(this, this._status);
};

MySwitch.prototype.getStatus = function(el) {

    return this._status;
};

And tests:

describe("My switch", function() {

    var options, element, myToggle;

    beforeEach(function() {

    element = document.createElement("div");

    element.innerHTML = '"';

    $("body")[0].appendChild(element);

    function onToggleCallback() {};

    options = {

        element: $(element).find("span")[0],
        onToggle: onToggleCallback
    };

    myToggle = new MySwitch(options);
});

afterEach(function() {

    $("body > div")[0].remove();

});

it("Should set ON/OFF as default values", function() {

    $("body > div")[0].remove();

    element = document.createElement("div");

    element.innerHTML = '';

    $("body")[0].appendChild(element);

    options = {

        element: $(element).find("span")[0],
        onToggle: function(status) {}
    };

    myToggle = new MySwitch(options);
    $(element).removeAttr("data-value-on").removeAttr("data-value-off");

    expect($(element).text()).toMatch("OFF");

    myToggle.toggle();

    expect($(element).text()).toMatch("ON");

});

it("Should have AM as off value", function() {

    expect($(element).text()).toMatch("AM");

});

it("Should have PM as on value", function() {

    expect($(element).text()).toMatch("AM");

});

it("Should toggle AM->PM->AM", function() {

    myToggle.toggle();

    expect($(element).text()).toMatch("PM");

    myToggle.toggle();

    expect($(element).text()).toMatch("AM");
});

it("Should call onToggle callback", function() {

    spyOn(options, "onToggle");

    myToggle.toggle();

    expect(options.onToggle).toHaveBeenCalled();

    });

});

To begin with, don't interact with the DOM. The reason for that is imagine that you have hundreds of unit tests. Do you want to clean up the DOM after them? Are you going to forget sometimes? If you forget will you be able to find why unit tests are failing? When you will be setting up a Continuous Integration process which will run the unit tests will you use a JavaScript engine tool that is aware of a DOM? What I'm getting at is only test the logic.

In order to make your class unit testable it needs to be refactored. To begin with, you need to make the constructor not do anything except set the this.options and this._status and move all the other code to an init method:

MySwitch = function(options) {
    this.options = options;
    this._status = 0;
};

MySwitch.prototype.init = function() {
    this._$el = $(options.element);
    this._$el.data("value-on") ? this._valueOn = this._$el.data("value-on") : this._valueOn = "ON";
    this._$el.data("value-off") ? this._valueOff = this._$el.data("value-off") : this._valueOff = "OFF";
    if (!this._$el.text()) this._$el.text(this._valueOff);

    this._$el.on("click", $.proxy(this.toggle, this));
};

Now refactor the toggle method:

MySwitch.prototype.toggle = function() {
    this.toggleStatus();
    this.setText();
    this.runToggleFunction();
};

MySwitch.prototype.runToggleFunction = function() {
    if (typeof this.options.onToggle === "function")
        this.options.onToggle.call(this, this._status);
};

MySwitch.prototype.toggleStatus = function() {
    this._status = this._status === 0 ? 1 : 0;
};

MySwitch.prototype.setText = function() {
    this.getEl.text( this._status ? this._valueOn : this._valueOff );
};

MySwitch.prototype.getEl = function() {
    return this._$el;
};

Now you can test the code:

describe("MySwitch", function() {

    it("toggle()", function() {
        var mySwitch = new MySwitch();

        spyOn(mySwitch, 'toggleStatus');
        spyOn(mySwitch, 'setText');
        spyOn(mySwitch, 'runToggleFunction');

        mySwith.toggle();

        expect(mySwitch.toggleStatus).toHaveBeenCalled();
        expect(mySwitch.setText).toHaveBeenCalled();
        expect(mySwitch.runToggleFunction).toHaveBeenCalled();
    });

    describe('runToggleFunction()', function() {
        it('callback provided', function() {
            var func = jasmine.createSpy('func');
            var mySwitch = new MySwitch({onToggle: func});
            var status = 1;

            mySwitch._status = status;
            mySwitch.runToggleFunction();

            expect(func).toHaveBeenCalledWith(status);
        });

        it('no callback', function() {
            var mySwitch = new MySwitch();

            try {
                mySwitch.runToggleFunction();
                expect(true).toBe(true);
            } catch (e) {
                expect(true).toBe(false);
            }
        });
    });

    it('toggleStatus()', function() {
        var mySwitch = new MySwitch();

        mySwitch._status = 1;
        mySwitch.toggleStatus();

        expect(mySwitch._status).toBe(0);
    });

    describe('setText()', function() {

        var valueOn = "on";
        var valueOff = "off";
        var mySwitch;
        var $el;

        beforeEach(function() {
            mySwitch = new MySwitch();

            $el = {
                text: function(){}
            };

            spyOn($el, 'text');
            spyOn(mySwitch, 'getEl').and.returnValue($el);
        });

        it('status 0', function() {
            mySwitch._status = 0;
            mySwitch.setText();
            expect($el.text).toBe(valueOff);
        });

        it('status 1', function() {
            mySwitch._status = 1;
            mySwitch.setText();
            expect($el.text).toBe(valueOn);
        });
    });

});


Generate Letter Combinations of a Phone Number The task: Given a string containing digits from 2-9 inclusive, return all possible letter combinations that the number could represent.

and my solution:

import itertools
letters_stack = list('abcdefghijklmnopqrstuvwxyz')
keypad_dict = {}
for num in range(2, 10):
    size = 3
    if num in [7, 9]:
        size = 4
    keypad_dict[str(num)] = letters_stack[:size]
    letters_stack = letters_stack[size:]

def aux(numbers):
    if len(numbers) == 1:
        return keypad_dict[numbers[0]]
    return itertools.product(keypad_dict[numbers[0]],  iletters(numbers[1:]))

def iletters(numbers):
    assert len(numbers) > 0
    return [''.join(x) for x in aux(numbers)]

print list(iletters('234'))
# ['adg', 'adh', 'adi', 'aeg', 'aeh', 'aei', 'afg', 'afh', 'afi', 'bdg', 'bdh', 'bdi', 'beg', 'beh', 'bei', 'bfg', 'bfh', 'bfi', 'cdg', 'cdh', 'cdi', 'ceg', 'ceh', 'cei', 'cfg', 'cfh', 'cfi']

You can set the keypad lookup as a constant instead of computing it dynamically. Setting it up as constant makes it clearer that what your intent is.

KEYPAD = {
    '2': 'abc',
    '3': 'def',
    '4': 'ghi',
    '5': 'jkl',
    '6': 'mno',
    '7': 'pqrs',
    '8': 'tuv',
    '9': 'wxyz'}

itertools.product can accept multiple lists. And in python, you can use * to unpack an iterable into positional arguments, and ** to unpack as keyword arguments.

To sum up:

def t9_mode(numbers):
    if len(numbers) == 1:
        return KEYPAD[numbers[0]]
    maps = map(KEYPAD.get, numbers)
    return [''.join(_) for _ in itertools.product(*maps)]

does the job.


Regex to validate font names /((([\w -]+)|("[\w -]+"))( *, *)?)+/

http://refiddle.com/18ql

I'm trying to use a PHP regex to sanitize a user input for a list of fonts. The above one seems to work nicely, but also feels a bit long and redundant, having two [\w -]*s, and allowing a trailing comma, and possibly stuff I can't see. Can this be more efficient (smaller, less redundant, more secure)?

Examples

The regex should match these:

Roboto
"Roboto"
"Roboto Condensed"
Roboto Condensed
Roboto Condensed, Roboto
"Roboto Condensed", Roboto
Roboto Condensed, "Roboto"
"Roboto Condensed", "Roboto", sans-serif

or any string that matches the W3C specification for a CSS font-family property

It must be able to match these, but not any preceding (e.g. /*) or succeeding (e.g. ; DROP TABLE foo) string that might cause errors and open up exploits. So, it should match the font-family list (bold) in the following example, but not the surrounding potentially malicious characters.

  /*"Roboto Slab", "Helvetica Neue", "Arial", sans-serif; DROP TABLE foo --

Similarly, I want it to also remove strings that someone might not know would produce broken code:

  font-family:"Roboto Slab", "Helvetica Neue", "Arial", sans-serif; /* My fonts */

My input-handling function:

$userFontName = sanitizeFontName($_GET['font'])

function sanitizeFontName($fontName)
{
    $match = null;
    preg_match('/((([\w -]+)|("[\w -]+"))( *, *)?)+/', $fontName, $match); // Only get a W3C font list. Proven here: http://refiddle.com/18ql
    return $match[0]; // only one
}

Demo

Your approach is completely wrong.

There is a difference between escaping and validation

Validation is the act of making sure a piece of input conforms to certain rules (be it logical, or business rules). It should not be used for security. You can't catch all possible edge cases with a blacklist, don't try.

Escaping (or Sanitization) is the act of escaping characters with special meaning and replacing them with another character or a string that will have the meaning of the literal character in the target context (for example, replacing < with &lt; in HTML). Escaping can and should be used for making sure nothing with special meaning enters the target context (HTML, MySQL query, JavaScript, etc).

In your case, the context is a MySQL query, you're trying to prevent SQL injection. The solution to SQL injection is not validation. It's prepared statements. So:

$pdo = //Instantiate PDO connection.
$stmt = $pdo->prepare("INSERT INTO fonts(id) VALUES (:id)");
$stmt->bindValue(":id", "1; DROP TABLE fonts -- "); //Unsafe input
$stmt->execute(); //Will not drop the table. No results will be returned.

With an insert, the entire string 1; DROP TABLE fonts -- will be inserted.

Sometime in the future (when you try to fetch the font from the database), and before you output it into CSS, you'll want to escape the string for this new context. CSS escaping is something that hasn't a native PHP API, but there are probably libraries around.


Is my multithreaded form code correct? The basic idea is this:

There are two buttons: a DoWork button and a Cancel button.

The DoWork button should launch a thread to do some work, unless that thread was already launched and not canceled.

The Cancel button should cancel the worker thread, if it's running.

Please ignore the fact that it calls Thread.Abort() for the moment, that is a separate issue that I will address later.

Also, how does one generally show the correctness of one's own multithreaded code?  It seems much more difficult to debug.

Here's the code:

    private Thread m_WorkingThread;
    private bool m_finishedWorking;

    public Form()
    {
        InitializeComponent(); // initialize all the form controls.
        m_WorkingThread = null;
        m_finishedWorking = false;
    }

    private void bDoWork_Click(object sender, EventArgs e)
    {
        if (m_WorkingThread != null)
            if(m_WorkingThread.IsAlive  || m_finishedWorking)
                return;

        ThreadStart ts = new ThreadStart(DoWork);
        Thread t = new Thread(ts);
        t.Start();
        m_WorkingThread = t;

    }
    private void bCancel_Click(object sender, EventArgs e)
    {
        AbortThread(m_WorkingThread);
        m_finishedWorking = false;
    }
    private void AbortThread(Thread t)
    {
        if (t != null)
            if (t.IsAlive)
                t.Abort();
    }
    private void DoWork()
    {
        // do some work here, maybe using Invokes / BeginInvokes to update any controls.

        m_finishedWorking = true;
    }

Have you considered using BackgroundWorker? It might save you some trouble.

However, it does use ThreadPool and you are better to create your own threads if you have any of the following requirements:

You require a foreground thread.
You require a thread to have a particular priority.
You have tasks that cause the thread to block for long periods of time. The thread pool has a maximum number of threads, so a large number of blocked thread pool threads might prevent tasks from starting.
You need to place threads into a single-threaded apartment. All ThreadPool threads are in the multithreaded apartment.
You need to have a stable identity associated with the thread, or to dedicate a thread to a task.

I add my comment to the answer of "Peter K." here as it seemed to please you.

  Mutex is a bit overkill as it works
  across multiple processes. One should
  use locks (i.e. Monitor) when there is
  no need to synchronize across
  processes. Anyway, I don't see here
  any need for the locks assuming that
  the bDoWork_Click() is always called
  from the UI thread. It only creates a
  new thread if the existing thread is
  not alive. However, the resources used
  by DoWork() must be protected with
  locks if they are shared with other
  threads.


Number Guessing Game in C++ without global varibles Recently, I have gotten into C++. My past experience is some batch programming and beginner JavaScript/HTML. This language, unlike the others, has made me really think about the efficiency of my code. I am trying to do a beginner exercise from a C++ forum (Bracketing Searching) and make it as good as I possibly can. Can someone explain to me anything I am doing right or wrong? I also feel like this code is the most efficient, however it is really confusing to read, even for me.

#include <iostream> //include the input, output stream
#include <Windows.h> //include windows command library
#include <ctime> //include time library

int ask() { //ask for input and return it
    int guess;
    std::cout << "Input your guess: \n";
    std::cin >> guess;
    return guess;
}

bool calc(int guess,int num) { //takes the guess and  the number then returns true or false accordingly
    bool hasWon = false;
    if (guess > num) {
        std::cout << "Too high!\n";
    }
    else if (guess < num) {
        std::cout << "Too low!\n";
    }
    else if (guess == num) {
        std::cout << "Congrats, you are correct!\n";
        Sleep(3000);
        hasWon = true;
    }
    return hasWon;
}

void startMenu() { //Display welcome menu
    std::cout << "Welcome to my game.\n";
    std::cout << "I will think of a number from 1 to 100.\n";
    std::cout << "Guess it!\n";
}

int makeNum() {
    srand(time(0)); //Truly random number
    int num = rand() % 100; //Generate a random Number
    return num; //return the number to the main loop
}

int main() {
    startMenu(); //Display the welcome screen
    int ranNum = makeNum(); //generate the main random number
    bool hasWon = false; //set default state of win varible

    while (hasWon != true) { //while the player has NOT won keep asking
        hasWon = calc(ask(), ranNum); //Main loop of code for asking and calculating
        }
    return 0; //Terminate program
}

rand() is bad; don't use it.

Instead, use the <random> library for generating random numbers:

static std::mt19937_64 engine(std::random_device{}());
std::uniform_int_distribution<> dist(1, 100);
std::cout << dist(engine);

You'll then have:

int makeNum() {
    static std::mt19937_64 engine(std::random_device{}());
    std::uniform_int_distribution<> dist(1, 100);
    return dist(engine);
}

The name calc() doesn't make sense with the expected return and it doesn't even calculate anything. You can rename it to hasWon() and rename the variable to won.

Moreover, I would move the output to main() since it shouldn't really be responsible for printing to the console. Just have it do the comparison and then return the value.
Do you really need the program to sleep? It may not be worth that call, plus it prevents it from being portable to other systems. You can always use std::cin.get() to "pause" the program by asking the user to enter a character before proceeding.


Linked list abstraction using python arrays Building linked list abstraction using class 'list' data model.

##################
#   Linked List  #
##################

#Representation - start
empty = 'empty'
def is_link(s):
    """ s is a linked list if it is empty or a (first, rest) pair."""
    return s == empty or (isinstance(s, list) and len(s) == 2 and is_link(s[1]))
#Constructor
def link(first, rest):
    """Construct a linked list from its first element and the rest."""
    assert is_link(rest), "rest must be a linked list"
    return [first, rest]
#Selector 1
def first(s):
    """Return a first element of a linked list s"""
    assert is_link(s), "first only applies to linked lists."
    assert s != empty, "empty linked list has no first element."
    return s[0]
#Selector 2
def rest(s):
    """Return the rest of the elements of a linked list s."""
    assert is_link(s), "rest only applies to linked lists."
    assert s != empty, "empty linked list has no rest."
    return s[1]

def isempty(s):
    return s == 'empty'

#Representation - end

###  +++ === Abstraction barrier === +++ ###

#Use(interface) -  start
def create_linked_list(first, rest):
    return link(first, rest)

def len_rlist(s):
    """Compute the length of the recursive list s"""
    def compute_length(s, length):
        if isempty(s):
            return length
        else:
            return compute_length(rest(s), length + 1)
    return compute_length(s, 0)

def getitem_rlist(s, i):
    """Return the element at index i of recursive list s"""
    if i == 1:
        return first(s)
    else:
        return getitem_rlist(rest(s), i-1)

def count(s, value):
    """Count the occurence of value in the list s """
    def count_occurence(s, value, count):
        if isempty(s):
           return count
        else:
           if first(s) == value:
              return count_occurence(rest(s), value, count + 1)
           else:
              return count_occurence(rest(s), value, count)
    return count_occurence(s, value, 0)

#Use - end

#Constructor and Selector constitutes Abstract Data Type that supports below invariants:
#If a recursive list s is constructed from a first element f and a recursive list r, then
#   • first(s) returns f, and
#   • rest(s) returns r, which must be a list or 'empty'.

#Code using this abstraction
Lst = empty
Lst = create_linked_list(4, Lst)   #  Representation [4, 'empty']
Lst = create_linked_list(3, Lst)   #  Representation [3, [4, 'empty']]
Lst = create_linked_list(1, Lst)   #  Representation [1, [3, [4, 'empty']]]
Lst = create_linked_list(1, Lst)   #  Representation [1, [1, [3, [4, 'empty']]]]
print(count(Lst, 1))

Intention is to build a single linked list abstraction that has no correctness issues.

Are there any limitations in building this abstraction?

asserts are not run if you use Python's optimized bytecode mode. And while nobody uses the -O switch, because the benefits are really meager, it still is not a good practice to you assert to validate user input. It is typically used to document invariants or assumptions, which roughly translates to it validating input that other parts of your code have generated, not users. Instead, you should raise an adequate exception, e.g.:

def first(s):
    """Return a first element of a linked list s"""
    if not is_link(s):
        raise TypeError("first only applies to linked lists.")
    if s == empty:
        raise IndexError("empty linked list has no first element.")
    return s[0]

Your is_link function, which is called by most of your other functions, has to scan the full list to validate it. That means that, especially with functions like count, which already need to scan the whole list, performance is going to end up being \$O(n^2)\$. That is not a good thing! You should probably go for a lazier is_link, that defers erroring until possible non-compliant items are actually used, e.g.:

def is_link(s)
    return s == empty or (isinstance(s, list) and len(s) == 2 and
                          (s[1] == empty or isinstance(s[1], list)))

You can simplify your count code a lot, by letting the recursion stack keep track of the number of items, rather than doing it yourself explicitly:

def count(s, value):
    if isempty(s):
        return 0
    return count(rest(s), value) + (1 if first(s) == value else 0)

While obviously doable, this reusing a list to make a linked list doesn't seem like a very good choice. It is very typical to use a dict of lists to represent a graph, rather than creating an explicit new class. And heaps also fit very well into a bare list with some helper functions, that's e.g. how the heapq module is implemented. But in your case, while it would be ok for a quick and dirty implementation where you directly accessed s[0] and s[1], if you are going to go through the trouble of writing all those helper functions, it is infinitely clearer to actually create a new class that embodies the abstraction.


Java Pub Sub implementation using broker I have developed a simple pubsub event subscription in java. Something similar to this thread

Simple Pub-Sub Event Emitter

These are the goals.

Class should be able to "listen" to events happening in the module.
It should be disconnected from the event emitter. That is, the listeners shouldn't have to know which classes may be emitting events.
All the subscribers should be notified parallelly.

My Broker.java

import java.util.*;

public final class  Broker {
    private Object mutex = new Object();
    static class CommonTopics{
        private CommonTopics(){}
        public static final String ON_CACHE_RESET = "onCacheReset";
    }
    private static Broker brokerInstance;
    private Broker(){

    }
    public static Broker getInstance(){
        if(null== brokerInstance){
            brokerInstance = new Broker();
        }
        return brokerInstance;
    }

    private Map<String, Set<Subscriber>> subscribers = new HashMap<>();

    public boolean deregister(String topic, Subscriber subscriber) {
        synchronized (mutex) {
            final Set<Subscriber> subs = this.subscribers.get(topic);
            return subs.remove(subscriber);
        }
    }

    public boolean register(String topic, Subscriber subscriber) {
        boolean returnVal;
        synchronized (mutex) {
            if (subscribers.containsKey(topic)) {
                returnVal = subscribers.get(topic).add(subscriber);
            } else {
                Set<Subscriber> sub = new HashSet<>();
                returnVal = sub.add(subscriber);
                subscribers.put(topic, sub);
            }
        }
        return returnVal;
    }

    public void sendMessage(String topic,Map map){
        synchronized (mutex) {
            final Set<Subscriber> sub = this.subscribers.get(topic);
            sub.parallelStream().forEach(subscriber -> subscriber.update(map));
        }
    }

}

My Subscriber interface

import java.util.Map;

public interface Subscriber {
    public void update(Map map);
}

And an example subscriber

import java.util.Map;

public class ExampleSubscriber implements Subscriber {
    @Override
    public void update(Map map) {
        System.out.println(map.get("oldCachedValue"));
        System.out.println(map.get("newCachedValue"));
    }

    public static void main(String[] args) {
        ExampleSubscriber subscriber = new ExampleSubscriber();
        Broker.getInstance().register(Broker.CommonTopics.ON_CACHE_RESET,subscriber);
    }
}

Example event emmitter

Map m = new HashMap();
        m.put("oldCachedValue","Yes");
        m.put("newCachedValue","No");
        Broker.getInstance().sendMessage(Broker.CommonTopics.ON_CACHE_RESET,m);

First of all, mutex should be final, since you are synchronizing on it. In the current state of your code, it wouldn't make much difference, since it is private anyway and never exposed outside the class, but still, if you make it final, then the field mutex is protected against modification by the compiler, reducing the risk of bugs.

If you don't ever intend to change brokerInstance, i.e. if Broker is intended to be a singleton, you can also make brokerInstance final and initialize it immediately with its declaration. This would eliminate the need for the if construct in the method getInstance(). After googling about this, I found out that the purpose of what you are doing is probably lazy initialization. However, in this question on stackoverflow, some points are made in favor of the approach with the final variable (especially the point about thread safety might be interesting to you).

You might consider taking advantage of the class ConcurrentHashMap for your synchronization purposes. A ConcurrentHashMap is optimized for synchronized access in that it is not locked entirely during an operation, so that operations that do not conflict with each other can be performed simultaneously (you might want to take a look at this).
For example, you could make each Set<Subscriber> in Broker.subscribers a ConcurrentHashMap.KeySetView (which serves as the Set equivalent of a ConcurrentHashMap, because there is no class ConcurrentHashSet) instead of a regular HashMap. This would at least make the synchronized block in Broker.deregister(String, Subscriber) unnecessary. Making Broker.subscribers itself a ConcurrentHashMap might also help, but you still have to be careful, for example in the method Broker.register(String, Subscriber), because, without any other synchronization, the first call to subscribers.containsKey(topic) might yield false, so the else block is entered, but before the call subscribers.put(topic, sub) is reached, subscribers might have been changed and could now already contain a mapping for topic which would then be overwritten. There seem to be special methods in the class ConcurrentHashMap for such cases (like the compute methods), but since this is merely a general suggestion, I won't go into detail about it.

Your code lacks handling of exceptional cases, for instance, if the methods deregister(String, Subscriber) or sendMessage(String, Map) are called with a topic that is not contained in the key set of subscribers.

As I already hinted at in a comment, using raw types is a dangerous undertaking. It is not wrong per se, but the risk of bugs is higher because generics provide compile-time type-safety which you are not taking advantage of if you use raw types. I don't know what your code might do with the map, so maybe it won't be a problem, but you might still take a look at this or this.

Also note that the map that is being passed to Broker.sendMessage(String, Map) will be accessed concurrently in sub.parallelStream().forEach(subscriber -> subscriber.update(map)), which might not be desirable if the map can be modified by the subscribers.


Is this a proper callback implementation in Java? I have a code which I believe is callback implementation. Can you please provide your more experienced opinions on whether this is a good way to callback?

public interface Callback {
    void run();
}

Test of callback with anonymous function

public class Test {
    static void method (Callback callback){
        callback.run();
    }

    public static void main(String[] args) {
        method(new Callback() {
            @Override
            public void run() {
                System.out.println("Printed!");
            }
        });
    }
}

Yes, that is an implementation of a callback. There are some things to note though:

Callbacks in Java are seldom called Callbacks, but rather 'events' or 'listeners' (which is why you probably found it hard to search for the concepts in Java). The most common Listeners are likely in the AWT/Swing library, like ActionListener
having a run() method on the callback is a poor choice of name, because it confuses it with the Runnable interface (which, in a sense, is a callback of sorts too).
Java8 introduces functional interfaces which allows you to extend your callback in more concise ways. You should consider it.

Using a functional interface:

If you were to use your interface, but declare it as a functional interface to make it obvious (it is not required to declare it as functional, the compiler can automatically detect it...):

@FunctionalInterface
public interface Callback {
    void run();
}

Then, in your code, you can have:

static void method (Callback callback){
    callback.run();
}

public static void main(String[] args) {
    method(() -> System.out.println("Printed!"));
}

(you can also use the anonymous class, but the functional-interface implementation is.... neater.


Finding an equilibrium index in an int array Here is a programming challenge from codility

  A zero-indexed array A consisting of N integers is given. An
  equilibrium index of this array is any integer P such that 0 ≤ P < N
  and the sum of elements of lower indices is equal to the sum of
  elements of higher indices, i.e.

  A[0] + A[1] + ... + A[P−1] = A[P+1] + ... + A[N−2] + A[N−1].

  Sum of zero elements is assumed to be equal to 0. This can happen if P
  = 0 or if P = N−1.

  For example, consider the following array A consisting of N = 8
  elements:

  A[0] = -1   A[1] =  3   A[2] = -4   A[3] =  5   A[4] =  1   A[5] =
  -6   A[6] =  2   A[7] =  1

  P = 1 is an equilibrium index of this array, because:

  • A[0] = −1 = A[2] + A[3] + A[4] + A[5] + A[6] + A[7]

  P = 3 is an equilibrium index of this array, because:

  • A[0] + A[1] + A[2] = −2 = A[4] + A[5] + A[6] + A[7]

  P = 7 is also an equilibrium index, because:

  • A[0] + A[1] + A[2] + A[3] + A[4] + A[5] + A[6] = 0

  and there are no elements with indices greater than 7.

  P = 8 is not an equilibrium index, because it does not fulfill the
  condition 0 ≤ P < N.

  Write a function:

class Solution { public int solution(int[] A); }

  that, given a zero-indexed array A consisting of N integers, returns
  any of its equilibrium indices. The function should return −1 if no
  equilibrium index exists.

  For example, given array A shown above, the function may return 1, 3
  or 7, as explained above.

  Assume that:

  • N is an integer within the range [0..100,000];
  • each element of
  array A is an integer within the range
  [−2,147,483,648..2,147,483,647].

  Complexity:

  • expected worst-case time complexity is \$O(N)\$;
  • expected worst-case
  space complexity is \$O(N)\$, beyond input storage (not counting the
  storage required for input arguments).

  Elements of input arrays can be modified.

I think my whole solution is \$O(N)\$ because both my methods are \$O(N)\$. Is that right?
I believe I used some space to generate the sums and it Is still \$O(N)\$. Is it right?

Please review my code:

class Equilibrium {

    public int getEquilibrium(int[] A) {
        long[] sums = generateSums(A);
        long lowSum = 0L;
        int res = -1;
        for (int i = 0; i < A.length; i++) {
            if (lowSum == sums[i+1]) {
                res = i;
                break;
            }
            lowSum += A[i];
        }
        return res;
    }

    // I used long to handle sums greater than 32 bits
    public long[] generateSums(int[] A) {
        // I added another index (default value is 0) to handle the last value (E.G. the sum of previous elements is 0, so the equilibrium index should be the last value because there are no other items after it. I used the newly added index to handle it.)
        long[] res = new long[A.length+1];
        for (int i = A.length-1; i >= 0; i--) {
            res[i] = i+1 == A.length ? A[i] : res[i+1]+A[i];
        }
        return res;
    }
}

Did I follow all the requirements in the problem? How can I improve this and make it faster?

I think my whole solution is \$O(N)\$ because both my methods are O(N). Is that right?

Correct. Both of your methods are \$O(N)\$: they are linear in terms of the number of elements in the input array. More specifically, both generateSums and getEquilibrium loop over the array once.

  I believe I used some space to generate the sums and it Is still \$O(N)\$. Is it right?

Also correct. The intermediate array containing the sum contains as many elements as there was in the input array; doubling the input array in size would double the size of the intermediate array. This makes in linear in terms of space complexity.

The method generateSums doesn't have a good name. Its purpose is to calculate the cumulative sum of the input array, going from the right to the left. Consider naming it cumulativeSums instead. The rest of the algorithm is correct. You won't be able to make this faster than \$O(N)\$, in terms of time complexity.

A small comment: instead of using a break explicitly inside the for loop, consider returning directly the found index. You currenly have:

int res = -1;
for (int i = 0; i < A.length; i++) {
    if (lowSum == sums[i+1]) {
        res = i;
        break;
    }
    lowSum += A[i];
}
return res;

but it would be shorter to have:

for (int i = 0; i < A.length; i++) {
    if (lowSum == sums[i+1]) {
        return i;
    }
    lowSum += A[i];
}
return -1;

It avoids the use of a temporary local variable holding the result, and makes it clearer that -1 is the default value returned.

In terms of space complexity however, you can do better: this is possible in \$O(1)\$. Instead of generating an array containing the cumulative sums of the array, just calculate the total sum, let's call it right sum. Then, you keep a running sum of the array by traversing it from left to right, decreasing this right sum as you go: if the running sum is equal to the right sum, you found an equilibrium.

public int getEquilibrium(int[] array) {
    long totalSum = sum(array);
    long lowSum = 0L;
    for (int i = 0; i < array.length; i++) {
        totalSum -= array[i];
        if (lowSum == totalSum) {
            return i;
        }
        lowSum += array[i];
    }
    return -1;
}

public long sum(int[] array) {
    return Arrays.stream(array).asLongStream().sum();
}


SKAction on SKLabel with array of Strings I have an SKLabel attached to a parent SKNode and an array of strings:

let configText:[String] = [
                            "Configuration",
                             "Do stuff",
                             "Do more stuff",
                             "Nil",
                             "It is the void"]

The array is looped through with the following:

parentNode_Label.run(
  SKAction.sequence([
    SKAction.run{
      if self.counter == self.configText.count - 1
      { self.counter = 0 } },
    SKAction.wait(forDuration: 3.0),
    SKAction.run { self.sprite_Label.text = self.configText[self.counter + 1] },
    SKAction.fadeIn(withDuration: 0.5),
    SKAction.wait(forDuration: 3.0),
    SKAction.fadeOut(withDuration: 0.5),
    SKAction.run{ self.counter += 1 }
    ]).forever()
)

extension SKAction
public func forever() -> SKAction { return SKAction.repeatForever( self ) }
}

It works but seems kind of clunky/hacky. Is there a simpler, more efficient yet readable way to do this?

The first array element configText[0] is never used, so you can
remove it from the array (and modify the index calculations accordingly).
Incrementing the counter with wrap-around can be simplified using the
remainder operator:

self.counter = (self.counter + 1) % self.configText.count

The three "run" actions can be combined into one.
Inside the actions array you can refer to the SKAction members
without specifying the type explicitly, e.g. .wait instead
of SKAction.wait.

Putting it all together:

parentNode_Label.run(
    SKAction.sequence([
        .wait(forDuration: 3.0),
        .run {
            self.sprite_Label.text = self.configText[self.counter]
            self.counter = (self.counter + 1) % self.configText.count
        },
        .fadeIn(withDuration: 0.5),
        .wait(forDuration: 3.0),
        .fadeOut(withDuration: 0.5),
        ]).forever()
)

Also have a look at the Swift naming conventions:

  Names of types and protocols are UpperCamelCase. Everything else is lowerCamelCase.

For example: parentNode, textLabel, without underscores.


C++11 Blocking Queue learning exercise As part of my own C++11 learning exercise I have implemented this Blocking Queue using the new C++11 thread API. One of the aspects I would like to improve is efficiency i.e. in the take member function while returning the Job template type to be returned by reference instead of by value.

/*
 * blockingqueue.h
 *
 *  Created on: May 20, 2016
 *      Author: bravegag
 */

#ifndef BLOCKINGQUEUE_H_
#define BLOCKINGQUEUE_H_

#include <condition_variable>
#include <mutex>
#include <thread>
#include <iostream>
#include <queue>
#include <chrono>
#include <cassert>

using namespace std;

template<class Job>
class blocking_queue {
private:
    queue<Job> queue_;
    mutex mutex_;
    condition_variable not_empty_cond_;
    condition_variable not_full_cond_;
    int capacity_ = 0;

public:
    blocking_queue(int capacity) : capacity_(capacity) {
        // empty
    }

    void put(const Job& job) {
        unique_lock<mutex> lock(mutex_);
        while (queue_.size() >= capacity_) {
            cout << "queue full, waiting for jobs to be taken" << endl;
            not_full_cond_.wait(lock, [&]() { return (queue_.size() < capacity_); } );
        }
        queue_.push(job);
        not_empty_cond_.notify_one();
        assert(!queue_.empty());
    }

    Job take() {
        unique_lock<mutex> lock(mutex_);
        while (queue_.empty()) {
            cout << "queue empty, waiting for jobs to be put" << endl;
            not_empty_cond_.wait(lock, [&]() { return !queue_.empty(); });
        }
        Job job = queue_.front();
        queue_.pop();
        not_full_cond_.notify_one();
        assert(queue_.size() < capacity_);
        return job;
    }

    int size() const {
        return queue_.size();
    }

    virtual ~blocking_queue() {
        // empty
    }
};

#endif /* BLOCKINGQUEUE_H_ */

One of the aspects I would like to improve is efficiency i.e. in the take member function while returning the Job template type to be returned by reference instead of by value.

So what you would like is:

Job& take()   ?

the problem is that you would return a reference to local variable which is Undefined Behaviour.

Just Job take() should be fine, compiler will use NRVO optimization so that no copy will be done, copy constructor call would be elided. You could return by moving: return std::move(job);, but that can prevent elision.

One place where you will have a copy constructor called is here:

Job job = queue_.front();

To prevent it you can implement move semantics into your Job class. Then replace above with:

Job job = std::move(queue_.front());

if you will not add any of the user defined constructor/destructor/assignment operator to your Job class, then compiler will add move constructor/assignment operator on  its own - depending whether it is possible.


Small C++ thread pool I've made a small thread pool that i'll be using to communicate with a Redis server:

#include "redox.hpp"

redox::Redox rdx;

void my_cool_function()
{
    // do stuff with rdx
}

int main()
{
    rdx.connect("localhost");
    int max_threads = std::thread::hardware_concurrency();
    std::vector<std::thread> pool;

    for (int i = 0; i < max_threads; i++) {
        pool.push_back(std::thread(my_cool_function));
    }

    for (std::thread &thread : pool) {
        thread.join();
    }

    pool.empty();
    rdx.disconnect();

    return 0;
}

Do I need to call delete rdx on the end of my main function? What about the pool var?

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

Make sure to #include all required headers

This program refers to std::vector and std::thread but does not include the corresponding headers.  Fix that by adding these lines:

#include <thread>
#include <vector>

Check return values

The function rdx.connect() can fail.  Your program should check the return value and handle the case in which it does.

Understand your data structures

The std::vector is used appropriately except for the peculiar call to pool.empty() at the end of the code.  The purpose for empty() is to check to see if the vector is empty, not to empty the vector.  In any case, it's not necessary to empty it since it will automatically be destroyed when it goes out of scope at the end of main().

Understand the thread pool pattern

The question describes this as a "thread pool" but that's not what I see here.  It's not a finite set of threads doing some other number of tasks; rather it's one function being called once for each thread.  While this is multithreading, it's not a thread pool.  To keep from confusing yourself or others, it's useful to use common terms in the usual way.

Avoid concurrency problems

Any time multiple tasks run in parallel, we have to take extra care to assure that things work correctly.  In particular, my_cool_function() claims to "do stuff with rdx", but rdx is a single global variable.  The code for that function isn't shown, but it's highly likely that some form of locking will be required to avoid problems with concurrent access to the single global object.

Omit return 0 at the end of main

The compiler will automatically generate a return 0; at the end of main so it is not necessary to supply your own.