Truthy/falsy in Python def truthy(v):
    if issubclass(v.__class__, numbers.Number):
        return bool(int(v))
    if isinstance(v, basestring):
        vl = v.lower()[0]
        if vl == 'y' or vl == 't':
            return True
    return False

Is there some type/class I missed that can be evaluated by Python as True/False that I missed here?
Drawbacks? Better ways?

How about:

accepted_truthy_characters = {'y', 't'}

def truthy(value):
    if isinstance(value, basestring):
        return value and value[0].lower() in accepted_truthy_characters
    return bool(value)

In Python 3.x, basestring won't work and you'll need isinstance(value, str) instead.

One thing you missed is that value might be a string, but it might be empty, which would leave you with an IndexError.

Python's bool() will check an int for you, so I don't think you need to check if it's a Number, convert it with int(), and then bool(), unless I'm missing something about your use case.

Out of curiosity, why do you need this? Are you checking user input? Won't that always be a string anyway? And in that case, wouldn't it be better to just check a few allowed "positive" strings?


Function to get the most frequently appearing pixel What i have so far is a nice and rapid algorithm for finding the pixel dominant from bmp image.
I am not sure how to make it to work.. i though it works, but the program freezes.

RGB
bitfox_get_primecolor_direct
(char *FILE_NAME)
{
    RGB primecolor;
    BYTE rgb[3];

    dword *counts = calloc(pow(256, 3), sizeof(*counts));
    dword max_count = 0;

    FILE* fp = fopen(convert(FILE_NAME), "rb");

    fseek(fp, 54, SEEK_SET);
    while (fread (rgb, sizeof(BYTE), 3, fp) != EOF)
    {
        dword idx = (((dword)rgb[0]) << 16) | (((dword)rgb[1]) << 8) | (dword)rgb[2];
        if (++counts[idx] > max_count) max_count = idx;
    }

    primecolor.R = (max_count >> 16) & 0xFF;
    primecolor.G = (max_count >> 8) & 0xFF;
    primecolor.B = max_count & 0xFF;

    free(counts);
    fclose(fp);
    return primecolor;
}

BYTE is unsigned char

RGB is struct containg BYTE R, G and B.

There are numerous problems with the code. First of all, fread never returns EOF (which is -1). The loop should end when it reads less than 3 bytes.

Second, max_count is index, not a color value. I presume you meant rgb[max_count].

Test a file pointer returned by fopen for not being NULL.

Test a return value of fseek.

Avoid magic numbers. I suppose 54 is a size of certain header. Create a constant with a meaningful name. Better yet, define a header structure and take its sizeof.


A Trie data structure in Java this is the first time I' implementing a Trie. My goal was to make it as simple as possible. So here is the result:
Can I simplify anything else? Are there some things I can change to increase performance ? Also any feedback on code quality/design is welcome :)
Node.java

package trie;

public class Node {
    final Node[] children = new Node[26];
    boolean isEnd = false;
}

Trie.java

package trie;

import java.util.ArrayList;
import java.util.List;

public class Trie {
    static final int ALPHABET_SIZE = 26;
    final Node root = new Node();

    void add(String str) {
        Node curr = root;
        for (int i = 0; i < str.length(); i++) {
            int idx = str.charAt(i) - 'a';
            if (curr.children[idx] == null) {
                curr.children[idx] = new Node();
            }
            curr = curr.children[idx];
        }
        curr.isEnd = true;
    }

    Node getNode(String str) {
        Node node = root;
        for (int i = 0; i < str.length(); i++) {
            Node child = node.children[str.charAt(i) - 'a'];
            if (child == null) {
                return null;
            }
            node = child;
        }
        return node;
    }

    boolean contains(String str) {
        Node node = getNode(str);
        return node != null && node.isEnd;
    }

    List<String> prefixedWords(String str) {
        Node curr = getNode(str);
        List<String> prefixedWords = new ArrayList<>();
        DFS(curr, str, prefixedWords);
        return prefixedWords;
    }

    void DFS(Node root, String prefix, List<String> list) {
        if (root != null) {
            if (root.isEnd) {
                list.add(prefix);
            }
            for (int i = 0; i < ALPHABET_SIZE; i++) {
                if (root.children[i] != null) {
                    DFS(root.children[i], prefix + (char) (i + 'a'), list);
                }
            }
        }
    }
}

Magic numbers

    final Node[] children = new Node[26];

Consider changing this to

    final Node[] children = new Node[Trie.ALPHABET_SIZE];

Now you don't have to manually change the array size if you change the alphabet size.  It may not matter here (we are unlikely to add a letter to the alphabet and you may not need to change alphabets), but it is good practice to set things up so that the system makes them the same.

Range-based for loops

        for (int i = 0; i < str.length(); i++) {
            int idx = str.charAt(i) - 'a';

This could be

        for (char c : str.toCharArray()) {
            int idx = c - 'a';

Then you don't have to maintain i manually.

You also may want to check that idx is in the range 0 to 25.  You currently just trust that that will be so.

Early exit

    void DFS(Node root, String prefix, List<String> list) {
        if (root != null) {

This could be

    static void DFS(Node root, String prefix, List<String> list) {
        if (root == null) {
            return;
        }

This way you don't have to enclose the entire method inside an if.  You can just bail out immediately.

The method should be static because it doesn't use any of the state of the trie.  In fact, one of its parameters has the same name as an object field.

Performance

I don't see anything that leaps out at me performance wise.  Tries are best at prefix searches and relatively slow and memory intensive to build.  For many uses, a HashSet (for full word searches) or even an ArrayList is faster.  We need a long run to amortize out the build time enough to give faster prefix lookups.

Anyway, time your actual usage to see if this solution is faster than other solutions.


OpenMP loop parallel for loop with function calls and STL vector I have a function initialize_path_statistics(). I have used openMP to make it parallel. I am not sure where certain lines such as

float length_enclosed = Nodes::get_length_enclosed(i);

need additional pragma like #pragma omp atomic.

void Path_Statistics :: initialize_Path_statistics()
{
    path_match_score.resize(no_of_valid_nodes);
    combinatorial_structures.resize(no_of_valid_nodes);
    node_path_matches.resize(no_of_valid_nodes);
    path_loop_indexes.resize(no_of_valid_nodes);
    node_path_energy.resize(no_of_valid_nodes);

    #pragma omp parallel for
    for(unsigned int i = 0;i < no_of_valid_nodes;i++)
    {
        int matches = node_matches[i];
        float length_enclosed = Nodes::get_length_enclosed(i);
        float match_density = float(matches)/length_enclosed;
        path_match_score[i] = match_density;

        int child_count = (adjacency_table_vector[i].second).size();
        if((adjacency_table_vector[i].second).empty())
        {
            combinatorial_structures[i] = 1;
        }
        else
        {
            combinatorial_structures[i] = child_count+1;
        }
        node_path_matches[i] = matches;

        int loop_start_position = node_start[i] + node_matches[i];
        int loop_end_position = node_end[i] - node_matches[i];

        path_loop_indexes[i].first = loop_start_position;
        path_loop_indexes[i].second = loop_end_position;

        //the size of the loop
        int loop_size = node_end[i] - node_start[i] - 2*node_matches[i] + 1;

        //if the node can form a valid loop node
        if(loop_size <= MAXIMUM_SIZE_OF_LOOP)
        {
            //the loop path energy will also have the energy of the node + loop formed
            float energy = get_loop_energy(i) + node_energy[i];
            node_path_energy[i] = energy;
        }
        else
        {
            node_path_energy[i] = float(INFINITE);

        }

    }

The code for nodes::get_length_enclosed(i) is:

//calculates the length enclosed by the given node
inline int Nodes :: get_length_enclosed(int node_id)
{
    int length = node_end[node_id] - node_start[node_id] +1;
    return(length);
}

Can I make it more parallel using sections somehow?  I tried #pragma omp parallel for sections, which doesn't work. Also, is this code thread safe?

The code seems thread-safe to me. All operations are either done with local variables or with data addressed via the loop index, so data races are seemingly absent. Thus, no additional protection like #pragma omp atomic is necessary.

I do not think you can make it "more parallel". Though technically parts of the loop iteration are independent and can be computed in parallel, the overall amount of computation seems rather small, so exploiting that "inner" parallelism is unlikely to bring more performance. If you want to try it though, I would recommend you to use #pragma omp task, not sections.


How to make this code of shortest Path Finder more maintainable,extendable and get better static code analysis result? I have the below code for finding the shortest path between a source node and destination node in a BiDirectional graph.It is working alright.
However, when I am running Static Code Analysis (VS2017), it is giving poor score (42) on Maintainability Index.
My questions are:

How to improve the overall code quality?
How can I make it more extendable or maintainable?
Will employing some specific Design Pattern here would have mad more sense?If yes, which one and how, can you please guide?

Here is the code below:

interface IRouter
  {
    void AddNode(string name, Dictionary<string, int> node);
    List<string> GetShortestPath(string source, string destination);
  }
public  class BaggageRouter: IRouter
  {
    Dictionary<string, Dictionary<string, int>> vertices = new Dictionary<string, Dictionary<string, int>>();

    public void AddNode(string name, Dictionary<string, int> node)
    {
      vertices[name] = node;
    }

    public List<string> GetShortestPath(string start, string finish)
    {
      #region SettingUp
      var previous = new Dictionary<string, string>();
      var distances = new Dictionary<string, int>();
      var queue = new List<string>();// the set of all nodes in Graph   - all nodes in the graph are unoptimized - thus are in Q

      List<string> path = null;

      foreach (var vertex in vertices)
      {
        if (vertex.Key == start)
        {
          distances[vertex.Key] = 0;//setting Source Node Distance=0
        }
        else
        {
          distances[vertex.Key] = int.MaxValue;//remaining all nodes are set at max distance
        }
        queue.Add(vertex.Key);
      }
      #endregion

      while (queue.Count != 0)
      {
        queue.Sort((x, y) => distances[x] - distances[y]);

        var smallest = queue[0];
        queue.Remove(smallest);//Removing the optimized Node at every Iteration

        if (smallest == finish)//Meaning we have calculated the shortest path upto destination node, so no need to calculate shortest path for remaining nodes like in original Dijkstra
        {
          path = new List<string>();
          while (previous.ContainsKey(smallest))
          {
            path.Add(smallest);
            smallest = previous[smallest];
          }
          path.Add(start);
          path.Reverse();
          path.Add(": " + distances[finish].ToString());//Adding the shortest path distance as last element
          break;
        }

        if (distances[smallest] == int.MaxValue)
        {
          break;//This corner-case scenario can happen if the remaining nodes are Un-reachable.
        }

        foreach (var neighbor in vertices[smallest])
        {
          var alt = distances[smallest] + neighbor.Value;
          if (distances.ContainsKey(neighbor.Key))
          {
            if (alt < distances[neighbor.Key])
            {
              distances[neighbor.Key] = alt;
              previous[neighbor.Key] = smallest;
            }
          }
        }
      }

      return path;
    }
  }

How to improve the overall code quality?

I don't know much about C#, but I guess your analyzer is complaining mainly about the length of your method and its cyclomatic complexity. Try to split it in multiple well named methods and use guard conditions to reduce the nesting level of your ifs.

Maybe you could also use the ternary operator instead of the if in your first foreach to extract the common part of the if branches. But this is an highly discussed topic.

How can I make it more extendable or maintainable?

There is no real use in making something extensible, that you never plan to extend. So if you don't know possible (useful) extension points, you shouldn't desperately try to find some. You can refactor it later when you need it.

Maintainable is in general achieved by readable code and a good test base. If you are not afraid of breaking something, you can do everything with your code easily.

Will employing some specific Design Pattern here would have mad more sense?

Actually I would say, you applied the Strategy pattern here already. This is more or less the default use case, having an interface and change the path finding algorithm. (Maybe building the graph should be splitt from traversing)


Converting Pandoc Markdown images from captioned to inline After writing a rather long document in Markdown and using pandoc to convert it to a PDF, I found, to my dismay, that many of the images were out of place, and that they all had their alternate text proudly displayed underneath them as captions. My document is rather instructional, so this rearrangement was harmful to its readability.

I eventually found a way to display the images as inline. I still wanted to write the document in standard Markdown, though, so I wrote a Python script to convert all the standalone images in a document to this inline form.

pandoc_images.py:

import sys

# Convert standalone images in standard Markdown
# to inline images in Pandoc's Markdown
# (see http://pandoc.org/README.html#images)
with open(sys.argv[1], 'r') as markdown:
    lines = markdown.read().splitlines()
    for index, line in enumerate(lines):
        is_first_line = index == 0
        preceding_blank = True if is_first_line else not lines[index - 1]

        is_last_line = index == len(lines) - 1
        following_blank = True if is_last_line else not lines[index + 1]

        is_standalone = preceding_blank and following_blank
        is_image = line.startswith('![') and '](' in line and line.endswith(')')
        print(line + ('\\\n' if is_standalone and is_image else ''))

Example (text.md):

This is some text.

![This is an image.](image.png)

### This is a header.

Running python3 pandoc_images.py text.md would produce:

This is some text.

![This is an image.](image.png)\

### This is a header.

It seems like a lot of mess (enumerate, bounds checking, etc.) for such a simple job, though. Is there any way I can improve any of this code?

How about a regular expression?

def convert(s):
  return re.sub(r"((?:\A|^ *\n)!\[.*\]\(.*\))\n(^ *\n|\Z)", r"\1\\\2", s, 0, re.M)

def test1():
  print convert("""![foo](bar)\n\nthis is a test\n""")

def test2():
  print convert("""line 1\n\n![foo](asd)\n\nanother test\n""")

def test3():
  print convert("""line 1\n\n![foo](asd)\n""")

def test4():
  print convert("""line 1\n\n![foo](asd)\nNot blank\n""")

Note: I am using ^\s*\n to match a blank line - i.e. it can also contain spaces.


FizzBuzz - officially now in CLISP I had to jump on the bandwagon for this one. The task does a great job of helping you learn a language you're unfamiliar with.

So today I present you with my version of the infamous FizzBuzz game in Common Lisp. This is really the first "program" I've made in CLISP, and even though it's pretty small, I'm quite proud!!

I'd really love to know if there's an even easier way to make the same functionality. I notice a lot of repetition in terms of is-multiple, so if anyone has any ideas how I could DRY up all that, I'd appreciate it. Also, is there a formatting standard for symbol names and such in Lisp?

fizzbuzz.lisp

(defun is-multiple (i against)
  (= (mod i against) 0))
(defun num-action (i)
  (cond ((and (is-multiple i 3) (is-multiple i 5)) (print "FizzBuzz"))
    ((is-multiple i 3) (print "Fizz"))
    ((is-multiple i 5) (print "Buzz"))
    (T (print i))))
(dotimes (i 101) (num-action i))

Can it be cleaned up any further?

Do not recompute is-multiple repeatedly by either binding the value:

(defun num-action (i)
  (let ((i3 (is-multiple i 3))
        (i5 (is-multiple i 5)))
    (cond ((and i3 i5) (print "FizzBuzz"))
          (i3 (print "Fizz"))
          (i5 (print "Buzz"))
          (T (print i)))))

or by using if:

(defun num-action (i)
  (if (is-multiple i 3)
      (if (is-multiple i 5)
          (print "FizzBuzz")
          (print "Fizz"))
      (if (is-multiple i 5)
          (print "Buzz")
          (print i))))

PS. please fix indentation


Angular pipe to suppress sensitive information I have created an Angular pipe to suppress the sensitive information like credit cards, bank account, ABA numbers etc.

This is working fine but I would like to know if this is the best possible way to implement the logic.

Here is the Typescript code for pipe logic.

export class SuppressInfoPipe implements PipeTransform {

  transform(valueToSupress: string, unSuppressedCount?: number): string {

    let suppressedOutput = '';

    const valueToRemainUnsuppressed =
    valueToSupress.substring(valueToSupress.length - unSuppressedCount, valueToSupress.length);

    let astariskLength = valueToSupress.length - unSuppressedCount;

    for ( let i = 0; i < astariskLength; i++) {
     suppressedOutput = suppressedOutput.concat('*');
    }
    suppressedOutput = suppressedOutput.concat(valueToRemainUnsuppressed);

    return suppressedOutput;
  }

}

it takes the string input and the number of character they will no be hidden and then return the suppressed output.

Comments and suggestions are welcomed.

I would avoid the for loop to generate the "suppressed string".

My approach would be:

export class SuppressInfoPipe implements PipeTransform {

  transform(valueToSupress: string, unSuppressedCount = 0): string {
    const suppressedCount = valueToSupress.length - unSuppressedCount;
    const valueToRemainUnsuppressed =
    valueToSupress.substring(suppressedCount, valueToSupress.length);

    return Array(suppressedCount + 1).join('*') + valueToRemainUnsuppressed; // suppressedCount + 1: since join will a string of length "suppressedCount"
  }
}

In this case:

Array(n) will return an array of length n.

.join("*") will join the list and return a string equivalent of length n-1.


Core location background update and upload to server I want to upload a user's location to a server to send them a location-sensitive alert, so the location needs to be fairly accurate:

import Foundation
import CoreLocation

protocol LocationServiceDelegate {
    func tracingLocation(currentLocation: CLLocation)
    func tracingLocationDidFailWithError(error: NSError)
}

class LocationService: NSObject, CLLocationManagerDelegate {

class var sharedInstance: LocationService {
    struct Static {
        static var onceToken: dispatch_once_t = 0

        static var instance: LocationService? = nil
    }
    dispatch_once(&Static.onceToken) {
        Static.instance = LocationService()
    }
    return Static.instance!
}

var locationManager: CLLocationManager?
var lastLocation: CLLocation?
var delegate: LocationServiceDelegate?

override init() {
    super.init()

    self.locationManager = CLLocationManager()
    guard let locationManager = self.locationManager else {
        return
    }

    if #available(iOS 9.0, *) {
        locationManager.allowsBackgroundLocationUpdates = true
    } else {
        // Fallback on earlier versions
    };

    if CLLocationManager.authorizationStatus() == .NotDetermined {
        // you have 2 choice
        // 1. requestAlwaysAuthorization
        // 2. requestWhenInUseAuthorization
        locationManager.requestAlwaysAuthorization()
    }
    locationManager.desiredAccuracy = kCLLocationAccuracyBest // The accuracy of the location data
    locationManager.distanceFilter = 2000 // The minimum distance (measured in meters) a device must move horizontally before an update event is generated.

    locationManager.delegate = self
}

func startUpdatingLocation() {
    print("Starting Location Updates")
    self.locationManager?.startUpdatingLocation()
}

func stopUpdatingLocation() {
    print("Stop Location Updates")
    self.locationManager?.stopUpdatingLocation()
}

// CLLocationManagerDelegate
func locationManager(manager: CLLocationManager, didUpdateLocations locations: [CLLocation]) {

    guard let location = locations.last else {
        return
    }

    // Check if Application is active
        // Check Current View Controller
            // Refresh Weather

    // singleton for get last location
    self.lastLocation = location

    // use for real time update location
    updateLocation(location)
}

func locationManager(manager: CLLocationManager, didFailWithError error: NSError) {

    // do on error
    updateLocationDidFailWithError(error)
}

// Private function
private func updateLocation(currentLocation: CLLocation){
    let todaysDate:NSDate = NSDate()
    let dateFormatter:NSDateFormatter = NSDateFormatter()
    dateFormatter.dateFormat = "MM-dd-yyyy HH:mm"
    let DateInFormat:String = dateFormatter.stringFromDate(todaysDate)

    print(DateInFormat)
    print(currentLocation.coordinate.latitude.description + ", " + currentLocation.coordinate.longitude.description)
    guard let delegate = self.delegate else {
        return
    }

    delegate.tracingLocation(currentLocation)
}

private func updateLocationDidFailWithError(error: NSError) {

    guard let delegate = self.delegate else {
        return
    }

    delegate.tracingLocationDidFailWithError(error)
}
}

This code does work, although I'm not sure how well the location accuracy and update works.  Please review this code.

LocationManager Singleton

It is not necessary to use dispatch_once to make LocationService a Singleton.

This pattern can be achieved by doing the following:

Exposing a static type property that creates a new instance of LocationService
Marking the initializer of the class as private

For example:

class LocationService: NSObject, CLLocationManagerDelegate {
    static let shared = LocationSerivce()

    // This class cannot be instantiated outside of this class
    override private init() {
        // Initialization code
    }
}

shared is lazily instantiated. A performance benefit that you inherit by implementing the Singleton this way.

From Apple’s documentation on Type Properties:

  "Stored type properties are lazily initialized on their first access. They are guaranteed to be initialized only once, even when accessed by multiple threads simultaneously, and they do not need to be marked with the lazy modifier.”

The private initializer is key to the implementation of the Singleton pattern because it prevents anyone from outside of the class from creating another instance. That said, under other circumstances you may still want instances to be created to give your consumer more choices/functionality.

Ultimately these changes make your code easier to read, ensure you have Singleton and are inline with Apple’s documented approach to Singletons using Swift.

Private CLLocationManager

Your LocationSerivce appears to be a wrapper around Apple’s CLLocationManager class. It would probably be best to mark this constant as private. This prevents outside code from changing the reference or state of your locationManager property.

private let locationManager = CLLocationManager()

Self

In the following functions, the use of self is not required. The reference to locationManager is not ambiguous and self is not required to explicitly state otherwise.

func startUpdatingLocation() {
    print(“Starting Location Updates”)
    self.locationManager.startUpdatingLocation()
}

func stopUpdatingLocation() {
    print(“Stop Location Updates”)
    self.locationManager.stopUpdatingLocation()
}

CLLocationManager Desired Accuracy

You can remove the following code from your initializer:

locationManager.desiredAccuracy = kCLLocationAccuracyBest

On iOS, kCLLocationAccuracyBest is the default value of the desiredAccuracy property.

Accuracy Constants

In terms of accuracy, you have six options:

kCLLocationAccuracyBestForNavigation
kCLLocationAccuracyBest
kCLLocationAccuracyNearestTenMeters
kCLLocationAccuracyHundredMeters
kCLLocationAccuracyAccuracyKilometer
kCLLocationAccuracyThreeKilometers

At first glance one may think to use kCLLocationAccuracyBestForNavigation, however because increasing the accuracy requires more device resources (like GPS or Cellular Radio), it also means that more device power will be required. This can have a negative impact on the user experience if expectations are not set. There are also some other caveats that should be understood when setting the  desiredAccuracy property.

Apple’s desiredAccuracy documentation: https://developer.apple.com/reference/corelocation/cllocationmanager/1423836-desiredaccuracy

When deciding which constant to use, careful consideration will be required based on the app’s intended use and the user’s expectations (and delivering a great mobile experience). I recommend reading the CLLocationManager documentation to better understand its usage and how to strike a good balance between your app’s functionality and device performance.

Apple’s CLLocationManager documentation: https://developer.apple.com/reference/corelocation/cllocationmanager


Validating email without Regex I'm trying to improve my JavaScript (I'm usually a copy/paste guy but can do basic DOM stuff with jQuery too), so I decided to try and make a function to validate an email address without using Regex.

The code seems to work, but I'm interested in hearing how it could be improved.

I'll include the code below, but you can run/fork it on Codepen.

HTML

<input id="email" type="text">
<input id="submit" type="submit" value="Validate">

JS

// Function: valid email address without regex
function isvalidemail(email) {

    // Get email parts
    var emailParts = email.split('@');

    // There must be exactly 2 parts
    if(emailParts.length !== 2) {
        alert("Wrong number of @ signs");
        return false;
    }

    // Name the parts
    var emailName = emailParts[0];
    var emailDomain = emailParts[1];

    // === Validate the parts === \\

    // Must be at least one char before @ and 3 chars after
    if(emailName.length < 1 || emailDomain.length < 3) {
        alert("Wrong number of characters before or after @ sign");
        return false;
    }

    // Define valid chars
    var validChars = ['a','b','c','d','e','f','g','h','i','j','k','l','m','n','o','p','q','r','s','t','u','v','w','x','y','z','.','0','1','2','3','4','5','6','7','8','9','_','-'];

    // emailName must only include valid chars
    for(var i = 0; i < emailName.length; i += 1) {
        if(validChars.indexOf(emailName.charAt(i)) < 0 ) {
            alert("Invalid character in name section");
            return false;
        }
    }
    // emailDomain must only include valid chars
    for(var j = 0; j < emailDomain.length; j += 1) {
        if(validChars.indexOf(emailDomain.charAt(j)) < 0) {
            alert("Invalid character in domain section");
            return false;
        }
    }

    // Domain must include but not start with .
    if(emailDomain.indexOf('.') <= 0) {
        alert("Domain must include but not start with .");
        return false;
    }

    // Domain's last . should be 2 chars or more from the end
    var emailDomainParts = emailDomain.split('.');
    if(emailDomainParts[emailDomainParts.length - 1].length < 2) {
        alert("Domain's last . should be 2 chars or more from the end");
        return false;
    }

    alert("Email address seems valid");
    return true;
}

document.getElementById('submit').onclick = function() {
    var email = document.getElementById('email').value;
    isvalidemail(email);
};

One thing I know could be improved is where I repeat basically the same code twice to validate the characters in each section. I am more interested in hearing if the way I've structured the function and sections could be done better, as the exercise for me was more about writing well-structured JavaScript and getting familiar with some of the built-in functions than about writing a perfect validator (which is also the reason I didn't use Regex).

Read the (email address: syntax) article on Wikipedia  for what is allowed. Your current validation will produce a lot of fails for valid email addresses.


Reading Map from csv with Singleton There are 2 csv:
1 - areaMap.csv
2 - currencyMap.csv

Enum AreaMapFromCsv is the Singleton to store the Map<String, Enum> read from 1 csv.
Enum CurrencyMapFromCsv is the Singleton to store the Map<String, String> read from 2 csv.
Interface IMapFromCsv provides default method for reading Map from csv.

AreaMapFromCsv and CurrencyMapFromCsv looks almost identical. They differs only in FILE_TO_LOAD, argument passing to the interface default method and second type of Map parameters. Any ideas how to DRY?

import java.io.File;
import java.io.FileNotFoundException;
import java.util.HashMap;
import java.util.Map;
import java.util.Scanner;
import java.util.function.Function;

public interface IMapFromCsv{

    public default <T> Map<String, T> getMap(String FILE_TO_LOAD, Function<String, T> convertValue){
        Map<String, T> mapResult = new HashMap<>();
        File f = new File(FILE_TO_LOAD);
        try (Scanner sc = new Scanner(f)) {
            while (sc.hasNextLine()) {
                String tmp = sc.nextLine();
                String[] tmp2 = tmp.split(";");
                mapResult.put(tmp2[0], convertValue.apply(tmp2[1]));
            }
        } catch (FileNotFoundException fntf) {
            System.err.println("File " + f.getPath() + "was not found.");
        } catch (Exception e) {
            System.err.println("Something went wrong with reading " + f.getPath());
        }
        return mapResult;
    }
}

.

import java.util.Map;
import java.util.function.Function;

public enum AreaMapFromCsv implements IMapFromCsv {

    INSTANCE;
    final String FILE_TO_LOAD = "resources/areaMap.csv";
    private Map<String, String> map;

    private AreaMapFromCsv() {
        init();
    }

    private void init() {
        this.map = getMap(FILE_TO_LOAD, Function.identity());
    }

    public void reload() {
        init();
    }

    public Map<String, String> get() {
        return map;
    }

}

.

import java.util.Map;

public enum CurrencyMapFromCsv implements IMapFromCsv {

    INSTANCE;
    final String FILE_TO_LOAD = "resources/currencyMap.csv";
    private Map<String, Double> map;

    private CurrencyMapFromCsv() {
        init();
    }

    private void init() {
        this.map = getMap(FILE_TO_LOAD, Double::parseDouble);
    }

    public void reload() {
        init();
    }

    public Map<String, Double> get() {
        return map;
    }

}

soo... going from top to bottom:

public interface IMapFromCsv{

superfluous I before name, missing space before opening curly brace

public default <T> Map<String, T> getMap(String FILE_TO_LOAD, Function<String, T> convertValue){

FILE_TO_LOAD should be named according to naming convention: fileToLoad. Next step is making it a Path because a String is not a File and File is superseeded by Path, because it sucked.

Also missing a space before the opening curly again.

    Map<String, T> mapResult = new HashMap<>();
    File f = new File(FILE_TO_LOAD);

so far so good. did I mention that File is crap?

    try (Scanner sc = new Scanner(f)) {
        while (sc.hasNextLine()) {
            String tmp = sc.nextLine();
            String[] tmp2 = tmp.split(";");
            mapResult.put(tmp2[0], convertValue.apply(tmp2[1]));
        }
    }

Bonus points for using try-with-resources, deductions for using tmp and tmp2 as variable names. How about we simplify this a bit?

try (Stream<String> lines = Files.lines(fileToLoad)) {
    return lines.map(s -> s.split(";"))
        .collect(Collectors.toMap(line -> line[0]
            , line -> convertValue.apply(line[1])));
}

Note that this only returns either the full map, or currently nothing. If something goes wrong while reading the map, I'd suggest using Collections.emptyMap() as a sensible default result.

    } catch (FileNotFoundException fntf) {

This exception is one of the reasons File is crap. The good part is: You get a unified Exception API from the java.nio.Path and related, which is: IOException.

At this point I'm asking myself the question why this is a default method on an interface. Methods in an interface are meant to be overwritten to modify them. The method instead takes parameters to configure it. IMO this would be much nicer as a static method in some IO module or something.

Last but not least: Why are AreaMapFromCsv named like they'd be Methods? Let's reconsider this for a second. You're trying to implement something that will hold information on Maps of your choosing, namely where they are saved, and what value you're currently assigning to them.

I'm not clear on why that needs to be two separate classes (or actually singleton enums), or where the singleton comes into this.

From my PoV it seems like you heard that Singletons are an acceptable solution when you try to handle global state. Tip: they aren't.

The correct solution for global state is a properly encapsulated statically accessible instance of a proper class. Consider this instead:

public class MapHolder<T> {

    // consider making this final
    private Map<String, T> map;
    private final String mapPersistenceLocation; // this is a Path...
    private final Function<String, T> converter;

    public MapHolder(String parameterName, Function<String, T> converter) {
        this.mapPersistenceLocation = parameterName;
        this.converter = converter;
        reload();
    }

    public void reload() {
        // if map is final, replace by map.clear(); map.putAll(...);
        map = IOHelper.readMap(mapPersistenceLocation, converter);
    }

    public Map<String, T> get() {
        // consider returning a readonly copy, if modifications should be disallowed
        return map;
    }
}

Which can now be used as follows:

public final class GlobalContext {
    public static final MapHolder<Double> CURRENCY = new MapHolder<>("resources/currencyMap.csv", Double::parseDouble);
    public static final MapHolder<String> AREA = new MapHolder<>("resources/areaMap.csv", Function.identity());

    // prevent instantiation
    private GlobalContext() {}
}


Counting results of die rolls The algorithm bellow is meant to allow a user to roll a die as many times as he would like, then print how many times each side was rolled and how many rolls there were total.

    import java.util.Random;
import java.util.Scanner;

public class DieRoll {

    public static void main(String[] args) {
        // TODO Auto-generated method stub

        int roll;
        int totalRolls = 0;
        int roll1 = 0;
        int roll2 = 0;
        int roll3 = 0;
        int roll4 = 0;
        int roll5 = 0;
        int roll6 = 0;
        int numberOfRolls;

        Scanner scan = new Scanner(System.in);
        Random generator = new Random();

        System.out.println("How many times would you like to roll? ");
        numberOfRolls = scan.nextInt();

    for (int numberOfLoops = numberOfRolls; numberOfLoops>0; numberOfLoops--) {

        roll = generator.nextInt(6) + 1;
        System.out.println("your roll was: " + roll);

        switch (roll) {
            case 1:
                roll1 = roll1 + 1;
                break;
            case 2:
                roll2 = roll2 + 1;
                break;
            case 3:
                roll3 = roll3 + 1;
                break;
            case 4:
                roll4 = roll4 + 1;
                break;
            case 5:
                roll5 = roll5 + 1;
                break;
            case 6:
                roll6 = roll6 + 1;
                break;
                default:
                    break;
            }
        }
        System.out.println("\nyou rolled a 1: " + roll1);
        System.out.println("you rolled a 2: " + roll2);
        System.out.println("you rolled a 3: " + roll3);
        System.out.println("you rolled a 4: " + roll4);
        System.out.println("you rolled a 5: " + roll5);
        System.out.println("you rolled a 6: " + roll6);

        System.out.println("\nYou spun a total of : " + numberOfRolls);
    }
}

The algorithm can be simplified quite a bit. No need for switch-case. Just create an array (faceCount) with six slots and increment the appropriate slot. In other words:

roll = generator.nextInt(6);
faceCount[roll]++;

Then to print, iterate over faceCount and display the appropriate values (increment by one to adjust for array indexing).


Is the Number Less than Can my code be reviewed by common C++ formatting standards, and can it be optimized? My code determines if the number (x) you input is greater then the other number (y) you input, the code then prints out the results.

#include <iostream>
#include <stdio.h>

using namespace std;

int oper();

int main() {
    oper(); //function for the operation.
}
int oper() {
    int x = 0;
    int y = 0;
    int c;

    std::cin >> x;
    std::cin >> y;

    if (x > y)
        cout << "\nx = " << x <<
        ", y must be < " << x << "\n";

}

using namespace std;

You import using namespace std;, but you actually only use it in your call to cout.  As a general rule, you don't want to import this in its entirety, but to call the components directly, like in your calls to std::cin.  See Why is using namespace std considered bad practice for more information.

Function Declaration

As a general rule, you order C++ functions from last used to first, unlike in Java.  That means your oper() method would typically be written above your main() method.  Typically, you only explicitly declare functions in header (.h) files, and when two functions call each other and you do not have a .h file to go with your .cpp file.

Comments

Please do not use comments when it is obvious what is happening.  Your only comment here does not help understand either the how or why of the program.

Style

Please use braces around your if statements.  At first, I thought you had a bug here because of the two lines, but then I realized it was just a line break in a single statement.  Braces would help clear this up, as would writing the statement either on one line, or with better indentation as:

cout << "\nx = " << x
     << ", y must be < " << x << "\n";

Error Handling

Your program has absolutely no error handling, and that is perfectly understandable at this point.  Eventually, however, you will need to use a try block to ensure your program will not crash if the user enters invalid input and a loop to ensure a valid value is eventually entered.  Bjarne Stroustrup's "Programming: Principles and Practice Using C++" dedicates the majority of several chapters to proper error handling, and I strongly recommend you read this book; it will help you with your Java programming as well as your C++ programming.

Improvements

You only output the result if the x value entered is greater than the y value entered.  Your program should output the result when y is greater than x, and when x is the same value as y as well.


Iterating through two objects with different structures in parallel I have two objects, one input = Array<Type> and one stored = Array<{..., Type}>

What's the best (cleanest) way to put the elements in the first array into the sub-elements of the second? We can assume they're of the same size.

My current code below works, but looks bad due to the double iterator usage.

var count = 0;
stored.forEach((obj)=> {
    obj.value = input[count];
    count++;
});

Is there some sort of prototyping that I can use to shorten the code up?

You don't need the count variable to increment the index, Array.forEach accepts a second ( optional ) parameter which is the current index that you can use :

stored.forEach((obj, index) => {
   obj.value = input[index];
});

OR

stored = stored.map((obj, index) => ({...obj, value: input[index]}))

example :

let stored = [{ name : 'a', value: 2 }, {name : 'b',  value: 4 }, { name : 'c',  value: 6 }];
let input = [10, 11, 12];

stored.forEach((obj, index) => { obj.value = input[index]; });

// OR
// stored = stored.map((obj, index) => ({...obj, value: input[index]}))

console.log(stored)


Prime factoring function using recursion As a programming exercise, I decided to make a prime factoring function using recursion:

    static bool isPrime(int n)
    {
        if (n < 2)
            return false;
        if (n % 2 == 0)
            return (n == 2);
        int root = (int)Math.Sqrt((double)n);
        for (int i = 3; i <= root; i += 2)
            if (n % i == 0) return false;
        return true;
    }

    static bool iseven(int n)
    {
        return n % 2 == 0;
    }

    static int[] prfact(int n, int curr, List<int> nums)
    {
        if (nums.All(x => isPrime(x)) && isPrime(curr))
        {
            nums.Add(curr);
            return nums.ToArray();
        }
        else if (iseven(curr))
        {
            nums.Add(2);
            return prfact(n, curr / 2, nums);
        }
        else if (!iseven(curr))
        {
            int div = 3;
            while (curr % div != 0 || !isPrime(div))
                div += 2;

            nums.Add(div);
            return prfact(n, curr / div, nums);
        }
        else
            return null;
    }

    static int[] factor(int n)
    {
        return prfact(n, n, new List<int>());
    }

How efficient is it and how can it be improved?

+1 for stopping your isPrime search at the SqRt(n). That's a nice little optimization.
+1 for incrementing by 2. It's nice to see you realized that since it wasn't possibly even by this point, that you could optimize.
Next, consider a sieve or memento implementation for further performance improvements on isPrime.
Consider renaming root to limit.
+1 for extracting an isEven method.
-1 for neglecting to use it in your isPrime method.
Instead of having prfact and factor, Rename prfact to factor, overloading the latter. Make it private scoped to hide the implementation details from the outside world.

Sorry for the mostly non-algo review. Not my strong suit & short on time.


Spreading the load more evenly from a background thread I have a background thread which runs every 5 minutes. I am using ScheduledExecutorService for this. Every five minutes, I am making a call to one of my service and parsing the response coming back from that service.

public class ThreadScheduler {

private final ScheduledExecutorService scheduler = Executors.newScheduledThreadPool(1);

    public void startScheduleTask() {

        scheduler.scheduleAtFixedRate(new Runnable() {
            public void run() {
                try {
                    callService();
                } catch (Exception ex) {
                    // log error
                }
            }
        }, 0, 5L, TimeUnit.MINUTES);
    }

    public void callService() throws Exception {
        // execute the service and parse the response
    }
}

Now the above code is running in 1000 machines and they all are started at the same time, so that means it will make all the 1000 calls every 5 minutes simultaneously to my service.  I'm trying to spread the load more evenly over a 5 minute window. Is this possible to do by any chance with the above code?

I am just trying to spread the load more evenly on my service over the 5 minute window so that it doesn't hit my service at the same time which will put more load once all the systems are started up  simultaneously.

I came up with the below code trying to spread the load more evenly over the five minute window:

public class ThreadScheduler {

private final ScheduledExecutorService scheduler = Executors.newScheduledThreadPool(1);

    public void startScheduleTask() {

        scheduler.scheduleAtFixedRate(new Runnable() {
            public void run() {
                try {
                    callService();
                } catch (Exception ex) {
                    // log error
                }
            }
        }, (int)(Math.random()*5*60), 5*60L, TimeUnit.SECONDS);
    }

    public void callService() throws Exception {
        // execute the service and parse the response
    }
}

I am opting for code review to see whether this way load will get evenly spread out over the 5 minute window or not.

If you use a random delay and fixed period, all machines will make roughly the same number of calls during their lifetime. With this approach you can be sure that every machine makes a call within the period.

If you use a fixed delay and random period, then some machines may end up making more calls than average, and others less than average.

If you want more balanced calls, the first approach makes more sense.

To measure the actual load, add some logging on the server. To give you an example, given this simplified sample from an apache log:

66.249.67.190 - - [30/Aug/2014:04:28:23 -0700] "GET
66.249.67.203 - - [30/Aug/2014:04:28:34 -0700] "GET
66.249.65.126 - - [30/Aug/2014:05:32:54 -0700] "GET
66.249.65.126 - - [30/Aug/2014:05:32:54 -0700] "GET
66.249.65.118 - - [30/Aug/2014:08:11:20 -0700] "GET
66.249.65.118 - - [30/Aug/2014:08:11:20 -0700] "GET
66.249.65.126 - - [30/Aug/2014:08:11:21 -0700] "GET
180.76.5.168 - - [30/Aug/2014:14:30:57 -0700] "GET

I would extract the timestamp part using cut or awk commands, for example:

# extract the timestamp part, until minutes, without seconds
cat access.log | cut -d' ' -f4 | cut -d: -f-2
[30/Aug/2014:04
[30/Aug/2014:04
[30/Aug/2014:05
[30/Aug/2014:05
[30/Aug/2014:08
[30/Aug/2014:08
[30/Aug/2014:08
[30/Aug/2014:14

And pipe this to uniq -c to get a count per-minute:

cat access.log | cut -d' ' -f4 | cut -d: -f-2 | uniq -c
  2 [30/Aug/2014:04
  2 [30/Aug/2014:05
  3 [30/Aug/2014:08
  1 [30/Aug/2014:14

Hopefully you should see a balanced count, and a repeating pattern every 5 minutes.

This is just an example (and not the best way, quick and dirty way) of measuring the load, use whatever method that suits you best.


RSpec test for handling known IP addresses I want to replace the array of ips with a method (like known_ips or just ips) but I don't know how.

I'll use this known_ips somewhere else.

describe WelcomeController, type: :controller do
  [
    {
      ip: '73.53.61.23',
      location_name: 'Seattle, Washington, US'
    }, {
      ip: '8.26.157.16',
      location_name: 'San Francisco, California, US'
    }, {
      ip: '174.112.13.21',
      location_name: 'Mississauga, Ontario, CA'
    }
  ].each do |params|
    it 'geolocalizes the visitor based on the ip' do
      allow_any_instance_of(ActionDispatch::Request).to receive(:remote_ip).and_return params[:ip]
      get :index
      expect(controller.current_location.name).to eq params[:location_name]
    end
  end
end

I'll use this known_ips somewhere else.

I'm guessing that "somewhere" will still be in a test file. If so you can put this in support file (which is a common patern in RSpec).

# spec/support/ip_list.rb
IP_LIST = [
  # easily to recognize that IP address is first and the location name the second, but you can use your code that uses Hash here
  ['73.53.61.23',   'Seattle, Washington, US'],
  ['8.26.157.16',   'San Francisco, California, US'],
  ['174.112.13.21', 'Mississauga, Ontario, CA']
]

Then on spec_helper.rb (see this example)

# spec/spec_helper.rb
# after the last require
Dir[Rails.root.join("spec/support/**/*.rb")].each { |f| require f }

And in your actual test:

describe WelcomeController, type: :controller do
  it 'geolocalizes the visitor based on the ip' do
    IP_LIST.each do |(ip, location_name)|
      expect(controller.request).to receive(:remote_ip).and_return ip
      get :index
      expect(controller.current_location.name).to eq location_name
    end
  end
end

Note that I nested IP_LIST.each inside the it block, I don't think you need to create one it for each test. This is a personal opinion, I'm not sure what is the best practices here.


Clock that changes document background color This is a timer based clock (using setInterval()) that changes the background color of the body based on the clock time hexadecimal values. I suspect that this code is messy. Is there any way the following JS code be made more efficient?

var clock = document.getElementById('clock');
var color = document.getElementById('hex-color');

function hexclock(){

  var time = new Date();
  var hours = (time.getHours() % 12).toString();
  var minutes = time.getMinutes().toString();
  var seconds = time.getSeconds().toString();

  if(hours.length < 2) {

    hours = '0' + hours;
  }

  if(minutes.length < 2) {

    minutes = '0' + minutes;
  }

    if(seconds.length < 2) {

    seconds = '0' + seconds;
  }

  var clockString = hours + ' : ' + minutes + ' : ' + seconds;
  var hexColorString = "#" + hours + minutes + seconds;

  clock.textContent = clockString;
  color.textContent = hexColorString;

  document.body.style.backgroundColor = hexColorString;
}

setInterval(hexclock, 1000);
body{
  marin-top:20px;
  text-align:center;
  color:white;
}

#clock{
  font-weight:300;
  font-size:60px;
}

00 : 00 : 00

#000000

Questions

I suspect that this code is messy.

It doesn't really look very messy... One great aspect is that the DOM references are cached.

Is there a way the following JS code be made efficient?

I'm not sure it needs to be much more efficient. The code looks similar to many examples one can find searching the internet for a clock in JavaScript. If memory was an issue and you really wanted, perhaps you could create only one date object and merely update that date object with the updated hour, minute and seconds values but you would need to do the bookkeeping manually, which might end up being less efficient.
Suggestions
global variables
The variables created using var are technically global variables, since they are not contained in a function. For a small script like this it likely won't make a difference, but in a larger application that can lead to namespace collisions. It would be wise to wrap the code in a function (perhaps in an IIFE) and use const/let instead of var to keep the scope limited.
Use Date method to get formatted time
It could be simplified by using the Date method toLocaleTimeString() (or alternatively Intl.DateTimeFormat()). See the example below for a demonstration of this.
Edit: I realize after posting it that the 24-hour format was still the default for the hours so I had to add hour12 to the options and an extra call to .replace() to remove the am/pm. Also that doesn't apparently support the 2-digit format well so it also has to additionally add the leading 0 if necessary. Apparently this code suffers from this issue.

(function() { //IIFE
  const clock = document.getElementById('clockElement');
  const color = document.getElementById('hex-color');
  const options = {
    hour: "2-digit",
    minute: "2-digit",
    second: "2-digit",
    hour12: true
  };

  function hexclock() {
    const time = new Date();
    //use preferred locale accordingly
    let clockString = time.toLocaleTimeString('en-GB', options).replace(/[a-z\s]/g, '');
    //add missing 0 - hour12 doesn't work ideally for this scenario
    clockString.length < 8 && (clockString = '0' + clockString);
    const hexColorString = '#' + clockString.replace(/:/g, "");

    clock.textContent = clockString;
    color.textContent = hexColorString;

    document.body.style.backgroundColor = hexColorString;
  }

  setInterval(hexclock, 1000);
  setTimeout(function() {
    console.log('window.clock (global variable)? :', window.clock !== undefined);
  }, 1100);
})(); //end IIFE
body {
  marin-top: 20px;
  text-align: center;
  color: white;
}

#clock {
  font-weight: 300;
  font-size: 60px;
}

00 : 00 : 00

#000000


Sending duplicate to heaven with Java 8 Follow up to Duplicate like a weapon, arrays like heaven.

@rolfl suggested in chat that I try to complete this challenge again in a Java 8 friendly way. I took the opportunity to also employ the algorithm suggested in this brilliant answer.

I'm not familiar with Java 8 features, but am definitely interested in using it more and finding the best ways to implement it.

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.util.Arrays;

public class ArrayAbsurdityJava8 {
    public static void main(String[] args) throws IOException {
        Files.lines(new File(args[0]).toPath())
                .filter(s -> !s.isEmpty())
                .forEach(ArrayAbsurdityJava8::printDuplicate);
    }

    private static void printDuplicate(String line) {
        int length = Integer.parseInt(line.split(";")[0]);
        int total = Arrays.stream(line.split(";")[1].split(","))
                .mapToInt(Integer::parseInt)
                .sum();

        System.out.println(total - (length - 1) * (length - 2) / 2);
    }
}

This ended up shorter than I expected. Is this best? I was thinking of doing it my way as well, but the boolean comparison requires a for loop (am I wrong?).

line.split(";") is being called twice, so you can assign it to a variable to remove one of the calls.

Since we know there's only one ; per line, we can use indexOf to find it, and substring to extract the relevant parts of the string. This may be a little bit faster than calling split(";").


Leetcode: number of islands I am on my second iteration of the famous "Number Of Islands" graph problem on Leetcode.
https://leetcode.com/problems/number-of-islands/
Given an m x n 2D binary grid grid which represents a map of '1's (land) and '0's (water), return the number of islands.

An island is surrounded by water and is formed by connecting adjacent lands horizontally or vertically. You may assume all four edges of the grid are all surrounded by water.

Example 1:

Input: grid = [
  ["1","1","1","1","0"],
  ["1","1","0","1","0"],
  ["1","1","0","0","0"],
  ["0","0","0","0","0"]
]
Output: 1

Example 2:

Input: grid = [
  ["1","1","0","0","0"],
  ["1","1","0","0","0"],
  ["0","0","1","0","0"],
  ["0","0","0","1","1"]
]
Output: 3

My approach is to iterate through each cell in the grid, incrementing a counter when the value == 1. Then, do a breadth-first search of the cell's neighbors, adding them to the queue if their value == 1. Once the queue is empty, keep iterating through all the cells.
To avoid infinite loops, I first used a set() to keep track of a list of tuple coordinates. This seemed to jack up my memory usage, so I then improved the algorithm by removing the set and simply setting the value of visited cells to 0.
I passed all the test cases but still can't figure out how to get a better runtime. I'm currently slower than ~90% of python submissions.
I would appreciate if anyone can help speed this thing up!

class Solution:
    def numIslands(self, grid: List[List[str]]) -> int:
        """
        Gets the neighbors of a coordinate in a grid. Returns a list of tuples.
        """
        m = len(grid)
        n = len(grid[0])

        def get_neighbors(coord: Tuple[int, int]) -> List[Tuple]:
            delta_row    = [0, 1, 0, -1]
            delta_column = [1, 0, -1, 0]

            neighbors = []
            for i in range(len(delta_row)):
                new_x = coord[0] + delta_row[i]
                new_y = coord[1] + delta_column[i]

                if -1 < new_x < m and -1 < new_y < n:
                    neighbors.append((new_x, new_y))

            return neighbors

        # loop through the entire grid and set coords to 0 as we visit them
        # if val == 1, do a breadth-first search for neighbors and increment islands
        # if val == 0, continue to next coordinate.

        islands = 0

        for i in range(len(grid)):
            for j in range(len(grid[0])):
                coord = (i, j)
                if grid[coord[0]][coord[1]] == "1":
                    islands += 1

                    # breadth-first search
                    q = deque([coord])
                    while len(q) > 0:
                        c = q.popleft()
                        grid[c[0]][c[1]] = "0"
                        for neighbor in get_neighbors(c):
                            if grid[neighbor[0]][neighbor[1]] == "1":
                                q.append(neighbor)
                                grid[neighbor[0]][neighbor[1]] = "0"

        return islands

My suggestions:

The variables m and n are initialized but never used

The function get_neighbors:
def get_neighbors(coord: Tuple[int, int]) -> List[Tuple]:
    delta_row    = [0, 1, 0, -1]
    delta_column = [1, 0, -1, 0]

    neighbors = []
    for i in range(len(delta_row)):
        new_x = coord[0] + delta_row[i]
        new_y = coord[1] + delta_column[i]

        if -1 < new_x < m and -1 < new_y < n:
            neighbors.append((new_x, new_y))

    return neighbors

Can be simplified to:
def get_neighbors(r: int, c: int) -> List[Tuple]:
    neighbors = (r + 1, c), (r - 1, c), (r, c + 1), (r, c - 1)
    return [(r, c) for r, c in neighbors if -1 < r < m and -1 < c < n]

Instead of unpacking coord you can pass the row r and column c directly. This also improves the runtime by roughly 30%.

Alternative approach
An alternative and faster approach is to use recursion:
def recursive(grid: List[List[str]]) -> int:
    islands = 0
    m, n = len(grid), len(grid[0])

    def sink_neighbors(row, col):
        grid[row][col] = '0'
        if row > 0 and grid[row - 1][col] == '1':
            sink_neighbors(row - 1, col)
        if row < m - 1 and grid[row + 1][col] == '1':
            sink_neighbors(row + 1, col)
        if col > 0 and grid[row][col - 1] == '1':
            sink_neighbors(row, col - 1)
        if col < n - 1 and grid[row][col + 1] == '1':
            sink_neighbors(row, col + 1)

    for r in range(m):
        for c in range(n):
            if grid[r][c] == '1':
                islands += 1
                sink_neighbors(r, c)
    return islands

Running the recursive solution on LeetCode:
Runtime: 120 ms, faster than 98.12% of Python3 online submissions for Number of Islands

This is the running time of the original, improved, and recursive solutions on a random grid of 1 million elements:
Runtime original: 2.345 s
Runtime improved: 1.544 s
Runtime improved2: 1.097 s
Runtime recursive: 0.696 s

Full code
from collections import deque
from typing import List, Tuple
import random
from time import perf_counter as pc

def original(grid: List[List[str]]) -> int:
    m = len(grid)
    n = len(grid[0])

    def get_neighbors(coord: Tuple[int, int]) -> List[Tuple]:
        delta_row = [0, 1, 0, -1]
        delta_column = [1, 0, -1, 0]

        neighbors = []
        for i in range(len(delta_row)):
            new_x = coord[0] + delta_row[i]
            new_y = coord[1] + delta_column[i]

            if -1 < new_x < m and -1 < new_y < n:
                neighbors.append((new_x, new_y))

        return neighbors

    islands = 0

    for i in range(len(grid)):
        for j in range(len(grid[0])):
            coord = (i, j)
            if grid[coord[0]][coord[1]] == "1":
                islands += 1
                q = deque([coord])
                while len(q) > 0:
                    c = q.popleft()
                    grid[c[0]][c[1]] = "0"
                    for neighbor in get_neighbors(c):
                        if grid[neighbor[0]][neighbor[1]] == "1":
                            q.append(neighbor)
                            grid[neighbor[0]][neighbor[1]] = "0"

    return islands

def improved(grid: List[List[str]]) -> int:
    m, n = len(grid), len(grid[0])

    def get_neighbors(r: int, c: int) -> List[Tuple]:
        neighbors = (r + 1, c), (r - 1, c), (r, c + 1), (r, c - 1)
        return [(r, c) for r, c in neighbors if -1 < r < m and -1 < c < n]

    islands = 0

    for row in range(m):
        for col in range(n):
            if grid[row][col] == "1":
                islands += 1
                q = deque([(row, col)])
                while len(q) > 0:
                    r, c = q.popleft()
                    grid[r][c] = "0"
                    for r, c in get_neighbors(r, c):
                        if grid[r][c] == "1":
                            q.append((r, c))
                            grid[r][c] = "0"

    return islands

def improved2(grid: List[List[str]]) -> int:
    m, n = len(grid), len(grid[0])
    islands = 0
    for row in range(m):
        for col in range(n):
            if grid[row][col] == '1':
                islands += 1
                grid[row][col] = '0'
                q = [(row, col)]
                while q:
                    r, c = q.pop(0)
                    for r, c in (r + 1, c), (r - 1, c), (r, c + 1), (r, c - 1):
                        if 0 <= r < m and 0 <= c < n and grid[r][c] == '1':
                            grid[r][c] = '0'
                            q.append((r, c))
    return islands

def recursive(grid: List[List[str]]) -> int:
    islands = 0
    m, n = len(grid), len(grid[0])

    def sink_neighbors(row, col):
        grid[row][col] = '0'
        if row > 0 and grid[row - 1][col] == '1':
            sink_neighbors(row - 1, col)
        if row < m - 1 and grid[row + 1][col] == '1':
            sink_neighbors(row + 1, col)
        if col > 0 and grid[row][col - 1] == '1':
            sink_neighbors(row, col - 1)
        if col < n - 1 and grid[row][col + 1] == '1':
            sink_neighbors(row, col + 1)

    for r in range(m):
        for c in range(n):
            if grid[r][c] == '1':
                islands += 1
                sink_neighbors(r, c)
    return islands

if __name__ == "__main__":
    N = 1000
    random_grid = [[str(random.randint(0, 1)) for _ in range(N)] for _ in range(N)]
    res = []

    # Benchmark
    for f in original, improved, improved2, recursive:
        random_grid_copy = [row[:] for row in random_grid]
        t0 = pc()
        res.append(f(random_grid_copy))
        t1 = pc()
        print(f'Runtime {f.__name__}: {round(t1 - t0, 3)} s')

    # Correctness
    assert all(x == res[0] for x in res)


Simplifying similar functions using a hover state with different div content I'm trying to hover over over the img then use Greensock Tweenmax on hover to control the speed and opacity of the text within the span.  The opacity will change on mouseover and mouseout.

The problem is that I'm having to repeat the same exact same effect on several div's and it's becoming repetitive.  Each one will be unique to the image being hovered.  I don't want them all to appear at the same time when I hover over one div.  I could give them all the same class then use .hover but then that would activate all of them instead of individually.

They are only to appear respective to that particular div being hovered.  It works just fine, but I would like to simplify it since I will be doing this to about 20 different divs.

I need some help simplifying the code below.  It works just fine, but maybe there is an easier way.

    Here is some text

    Here is some other text

/* tweenmax hover animations */
 $('.test img').hover(
    function(){
     /*mouseover function*/
     TweenLite.to(".test span", 1.5, {opacity:1});
    },
    function(){
    /*mouseout function*/
     TweenLite.to(".test span", 1.5, {opacity:0});
 });

 $('.testx img').hover(
    function(){
     /*mouseover function*/
     TweenLite.to(".testx span", 1.5, {opacity:1});
    },
    function(){
    /*mouseout function*/
     TweenLite.to(".testx span", 1.5, {opacity:0});
 });

 $('.testxy img').hover(
    function(){
     /*mouseover function*/
     TweenLite.to(".testxy span", 1.5, {opacity:1});
    },
    function(){
    /*mouseout function*/
     TweenLite.to(".testxy span", 1.5, {opacity:0});
 });

 $('.testxyz img').hover(
    function(){
     /*mouseover function*/
     TweenLite.to(".testxyz span", 1.5, {opacity:1});
    },
    function(){
    /*mouseout function*/
     TweenLite.to(".testxyz span", 1.5, {opacity:0});
 });

The recommended way is to use event delegation:

$('.test, .testx, .testxy, .testxyz')
  .on('mouseover', 'img', function (e) {
    TweenLite.to($(e.delegateTarget).find('span'), 1.5, {opacity:1});
  }).on('mouseout', 'img', function (e) {
    TweenLite.to($(e.delegateTarget).find('span'), 1.5, {opacity:0});
  });

This will add the mouseover and mouseout events to all img tags within your parent elements. When the event is fired e.delegateTarget will be the parent element, so we can easily search for the span.


Two Fibonacci functions These are two of tasks, which I had to do for the recruitment process to be started in some company. Unfortunately they didn't like it so I've decided to share it here for discussion.

Add up all the even elements of the Fibonacci sequence below 1000. In my solution it is a method fibSumEven.
Write a recursive function that calculates the Fibonacci sequence up to 1000. In my solution it is a method fibR.

Fibonacci.h

#ifndef FIBONACCI_H_
#define FIBONACCI_H_

#include <iostream>
#include <string>
#include <sstream>

class CFibonacci
{

private:

    // Recursive function that calculates the Fibonacci sequence up to given LIMIT
    // limit - The limit, up to which sequence numbers will be calculated
    static int fibHelper(int limit, int x1, int x2)
    {
        int x = x1 + x2;
        if(x <= limit)
        {
            return fibHelper(limit, x2, x);
        }
        return x2;
    }

public:

    // A caller to recursive Fibonacci sequence up to given limit
    static int fibR(int limit = 1000)
    {
        if(limit <= 0)
        {
            return 0;
        }
        return fibHelper(limit, 0, 1);
    }

    //Adds up all the even elements of the Fibonacci sequence below limit
    static int fibSumEven(int limit=1000)
    {
        int x1 = 0, x2 = 1, x = 0, sum = 0;
        while(x <= limit)
        {
            x = x1 + x2;
            if(x % 2 == 0)
            {
                sum += x;
            }
            x1 = x2;
            x2 = x;
        }
        return sum;
    }
};

#endif /* FIBONACCI_H_ */

There was a disclaimer:

  Considering the ith element of the Fibonacci sequence Fib(i) = x, the element x is a natural number and is itself bounded by the value 1000 and not all the elements to Fib(1000).

Why could the solution have been disliked by the recruiters?

I don't like how you have all your logic in the header file.  You should declare your methods and fields in the header and use the .cpp file for logic.

On the other hand, C++ is not strictly object oriented, the way C# and Java are.  You didn't need a class for this solution, you could just have called fibHelper() from main().

Third point is your naming.  fibHelper() actually isn't a helper function, it is the function that actually calculates the Fibonacci sequence.  fibR() actually isn't recursive, like your comment says, it calls the recursive function.  int x1 doesn't say what it actually does, although it is easy to guess because the program is only a couple lines long - in a large project, this could (and probably would) cause many weeks of time wasted catching bugs.

Fourth is consistency:

static int fibR(int limit = 1000)
static int fibSumEven(int limit=1000)

For the most part, you are consistent, and I like how you use spaces around your operators and braces on your one-line ifs.


Filter a list based on some ComboBoxes So I have a grid with data and above that grid 2 comboboxes, the grid will be filled with data based on the selected values from the comboboxes. This works but can you do it way easier like I did?

if (_InitialDataGridPersonList != null)
{
    if (GetSelectedComboBoxPersonStateText() == "-" && GetSelectedComboBoxPersonLanguageText() != "-")
    {
        var dataGridPersonQuery = _InitialDataGridPersonList
            .Where(w => w.Language.Equals(GetSelectedComboBoxPersonLanguageText()));

        foreach (var person in dataGridPersonQuery)
        {
            _DataGridPersonList.Add(person);
        }
    }
    else if (GetSelectedComboBoxPersonStateText() != "-" && GetSelectedComboBoxPersonLanguageText() == "-")
    {
        var dataGridPersonQuery = _InitialDataGridPersonList
            .Where(w => (w.PersonState.Equals(GetSelectedComboBoxPersonStateText())));

        foreach (var person in dataGridPersonQuery)
        {
            _DataGridPersonList.Add(person);
        }
    }
    else if (GetSelectedComboBoxPersonStateText() == "-" && GetSelectedComboBoxPersonLanguageText() == "-")
    {
        foreach (var person in _InitialDataGridPersonList)
        {
            _DataGridPersonList.Add(person);
        }
    }
    else if (GetSelectedComboBoxPersonStateText() != "-" && GetSelectedComboBoxPersonLanguageText() != "-")
    {
        var dataGridPersonQuery = _InitialDataGridPersonList
            .Where(w => (w.PersonState.Equals(GetSelectedComboBoxPersonStateText()))
                    && (w.Language.Equals(GetSelectedComboBoxPersonLanguageText())));

        foreach (var person in dataGridPersonQuery)
        {
            _DataGridPersonList.Add(person);
        }
    }

Imagine there are like 10 comboboxes. So what I'm asking is, if there is a easier way with less code?

PS. I'm using MVVM so no code behind.

You have a lot of code duplication. As a first step, your code should be rewritten somehow like that:

if (_InitialDataGridPersonList != null)
{
    var personState = GetSelectedComboBoxPersonStateText();
    var personLanguage = GetSelectedComboBoxPersonLanguageText();

    var personStateSpecified = personState != "-";
    var personLanguageSpecified = personLanguage != "-";

    if (!personStateSpecified && personLanguageSpecified)
    {
        AddPersons(_InitialDataGridPersonList
                   .Where(w => w.Language.Equals(personLanguage)));
    }
    else if (personStateSpecified && !personLanguageSpecified)
    {
        AddPersons(_InitialDataGridPersonList
                   .Where(w => w.PersonState.Equals(personState)));
    }
    else if (!personStateSpecified && !personLanguageSpecified)
    {
        AddPersons(_InitialDataGridPersonList);
    }
    else if (personStateSpecified && personLanguageSpecified)
    {
        AddPersons(_InitialDataGridPersonList
                   .Where(w => w.PersonState.Equals(personState) &&
                               w.Language.Equals(personLanguage)));
    }
}

private void AddPersons(IEnumerable<Person> persons)
{
    foreach (var person in persons)
    {
        _DataGridPersonList.Add(person);
    }
}

As a second step, you can place all comboboxes in a map where you'll link each combobox with property selector:

public enum PropertyType
{
    State,
    Language
}

private static readonly Dictionary<PropertyType, Func<Person, object>> _propertySelectors =
    new Dictionary<PropertyType, Func<Person, object>>
    {
        [PropertyType.State] = p => p.PersonState,
        [PropertyType.Language] = p => p.Language
    }

private void PopulatePersons()
{
    if (_InitialDataGridPersonList != null)
    {
        var propertyValues = Enum.GetValues(typeof(PropertyType))
                                 .Cast<PropertyType>()
                                 .ToDictionary(t => t,
                                               t => GetComboBoxText(t));

        AddPersons(_InitialDataGridPersonList.Where(p =>
        {
            return propertyValues.All(v => v.Value == "-" ||
                                           _propertySelectors[v.Key](p).Equals(v.Value)));
        });
    }
}

private void AddPersons(IEnumerable<Person> persons)
{
    foreach (var person in persons)
    {
        _DataGridPersonList.Add(person);
    }
}

private string GetComboBoxText(PropertyType propertyType)
{
    switch (propertyType)
    {
        case PropertyType.State:
            return GetSelectedComboBoxPersonStateText();
        case PropertyType.Language:
            return GetSelectedComboBoxPersonLanguageText();
        ...
    }

    return "-";
}

This approach allows you to add another comboboxes without a pain.


Macro to format a specific worksheet I am still new to VBA. I recorded several actions that I need to perform to ensure that a specific worksheet is formatted properly.

Row 1 contains headers. Other than the headers, there is no data in the sheet when this should be run. I know that the raw code from the recording can be shrunk down, but I would appreciate some assistance in figuring that out.

Sub MasterSheetFormatTest()
'
' MasterSheetFormatTest Macro
'

'
    Columns("A:A").ColumnWidth = 7
    Columns("B:B").ColumnWidth = 6
    Columns("C:C").ColumnWidth = 17.14
    Columns("D:D").ColumnWidth = 13.57
    Columns("E:E").ColumnWidth = 2.71
    Columns("F:F").ColumnWidth = 21.43
    Columns("G:G").ColumnWidth = 16.43
    Columns("H:H").ColumnWidth = 7.86
    Columns("I:I").ColumnWidth = 13.43
    Columns("J:J").ColumnWidth = 25.14
    Columns("K:K").ColumnWidth = 39.29
    Columns("L:L").ColumnWidth = 34.14
    Columns("M:M").ColumnWidth = 23.14
    Columns("N:N").ColumnWidth = 5.57
    Columns("O:O").ColumnWidth = 17.14
    Columns("P:P").ColumnWidth = 17.14
    Columns("Q:Q").ColumnWidth = 8.14
    Columns("R:R").ColumnWidth = 17.71
    Columns("S:S").ColumnWidth = 22.57
    Columns("T:T").ColumnWidth = 20.43
    Columns("U:U").ColumnWidth = 15.57
    Columns("V:V").ColumnWidth = 13.43
    Columns("W:W").ColumnWidth = 13.43
    Columns("X:X").ColumnWidth = 10.86
    Columns("Y:Y").ColumnWidth = 8.57
    Columns("Z:Z").ColumnWidth = 7.57
    Columns("AA:AA").ColumnWidth = 7.57
    Columns("AB:AB").ColumnWidth = 15
    Columns("AC:AC").ColumnWidth = 9.29
    Columns("AD:AD").ColumnWidth = 15.86
    Columns("AE:AE").ColumnWidth = 67.29
    Range("A1:AE1").Select
    Range("AE1").Activate
    With Selection
        .HorizontalAlignment = xlCenter
        .VerticalAlignment = xlBottom
        .WrapText = False
        .Orientation = 0
        .AddIndent = False
        .IndentLevel = 0
        .ShrinkToFit = False
        .ReadingOrder = xlContext
        .MergeCells = False
    End With
    With Selection
        .HorizontalAlignment = xlCenter
        .VerticalAlignment = xlCenter
        .WrapText = False
        .Orientation = 0
        .AddIndent = False
        .IndentLevel = 0
        .ShrinkToFit = False
        .ReadingOrder = xlContext
        .MergeCells = False
    End With
    ActiveWindow.Zoom = 90
    ActiveWindow.Zoom = 80
    ActiveWindow.Zoom = 70
    Range("A2").Select
    With ActiveWindow
        .SplitColumn = 0
        .SplitRow = 1
    End With
    ActiveWindow.FreezePanes = True
End Sub

This is repeated for each column's width:

Columns("A:A").ColumnWidth

It could just be:

Columns("A").ColumnWidth

The code repeats this block of code almost verbatim twice in a row, with no change in the Selection:

With Selection
    .HorizontalAlignment = xlCenter
    .VerticalAlignment = xlCenter
    .WrapText = False
    .Orientation = 0
    .AddIndent = False
    .IndentLevel = 0
    .ShrinkToFit = False
    .ReadingOrder = xlContext
    .MergeCells = False
End With

With the only difference being .VerticalAlignment = xlBottom (1st one) and .VerticalAlignment = xlCenter (2nd one).

You should be able to remove one of the two altogether and get identical results, as the 2nd one should overwrite changes made by the 1st one.

This repeats the same command with varying zoom levels:

ActiveWindow.Zoom = 90
ActiveWindow.Zoom = 80
ActiveWindow.Zoom = 70

I would say unless you need to see that is actually zooms progressively, just do ActiveWindow.Zoom = 70 by itself.


Ability to forget the memoized supplier value Guava has a feature request which is asking to provide the ability to forget memoized value on a given supplier.

On top on that I needed another functionality where if during the calculation of expensive value another thread would invalidate the data, the memoisation should retry (guava LoadableCache doesn't provide this feature: invalidation during calculation is ignored).

I couldn't wait for this feature so I wrote this myself. First, an interface for forgettable supplier:

public interface ForgettableSupplier<T> extends Supplier<T> {
    public void forget();
}

Then, a base class, mostly taken from here:

public abstract class DoubleCheck<T> extends ReentrantReadWriteLock {

    protected abstract T create();

    protected abstract T retrieve();

    protected abstract boolean invalid(T value);

    public T get() {
        try {
            readLock().lock();
            T value = retrieve();
            while (invalid(value)) { // this spins until no more invalidations are made
                readLock().unlock();
                writeLock().lock();
                value = retrieve();
                try {
                    if (invalid(value)) {
                        value = create();
                    }
                } finally {
                    readLock().lock();
                    writeLock().unlock();
                }
            }
            return value;
        } finally {
            readLock().unlock();
        }
    }
}

And finally, the static utility that combines everything together:

public class Suppliers {

    private Suppliers() {
    }

    public static <T> ForgettableSupplier<T> memoize(Supplier<T> delegate) {
        return new ForgettableMemoizingSupplier<>(delegate);
    }

    private static final class ForgettableMemoizingSupplier<T> implements ForgettableSupplier<T> {

        private final Supplier<T> delegate;

        private volatile T t;

        private final AtomicBoolean needsToBeForgotten = new AtomicBoolean(false);

        private final DoubleCheck<T> doubleCheck = new DoubleCheck<T>() {
            @Override
            protected T create() {
                needsToBeForgotten.set(false);
                return ForgettableMemoizingSupplier.this.t = delegate.get();
            }

            @Override
            protected T retrieve() {
                return ForgettableMemoizingSupplier.this.t;
            }

            @Override
            protected boolean invalid(final T t) {
                return (ForgettableMemoizingSupplier.this.t == null) || needsToBeForgotten.get();
            }
        };

        private ForgettableMemoizingSupplier(final Supplier<T> delegate) {
            this.delegate = delegate;
        }

        @Override
        public void forget() {
            needsToBeForgotten.set(true);
        }

        @Override
        public T get() {
            return doubleCheck.get();
        }
    }
}

Class names should be nouns. I'd rename DoubleCheck to DoubleCheckedResource or something similar.
ForgettableMemoizingSupplier should not allow null suppliers. You should check that and throw a NullPointerException.
The name of the invalid method in the DoubleCheck class is ambiguous. Does it invalidate something or does it check that something is valid or not? It seems that the latter is true, so I'd call this isInvalid (or isValid to eliminate the negation).
The T t field could be in the DoubleCheck anonymous subclass because this is the only class which uses it.
Is DoubleCheck a ReentrantReadWriteLock? Currently it is. I think your clients should not be able to call getOwner(), writeLock() or isFair() on a DoubleCheck instance. I'd use composition instead of inheritance here:

public abstract class DoubleCheck<T> {

    private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();

    private final ReadLock readLock = lock.readLock();

    private final WriteLock writeLock = lock.writeLock();

    ...

}

Check Effective Java, Second Edition, Item 16: Favor composition over inheritance.
The get method with the lock() call in the finally block smells a little bit. Are you sure that the object will always be in a consistent state? What happens when retrieve(), create() or invalid() throws an exception? Will  all locks be released properly?

If I'm right the following is the same but use the usual lock(); try { use() } finally { unlock(); } construction:

public T get() {
    readLock.lock();
    try {
        final T value = retrieve();
        if (!isInvalid(value)) {
            return value;
        }
    } finally {
        readLock.unlock();
    }

    writeLock.lock();
    try {
        T value = retrieve();
        while (isInvalid(value)) { // this spins until no more invalidations
                                   // are made
            value = retrieve();
            if (isInvalid(value)) {
                value = create();
            }
        }
        return value;
    } finally {
        writeLock.unlock();
    }
}

(Please note that in the original code between the readLock.unlock() and writeLock.lock() calls another threads could get the read lock, therefore more than one thread could acquire the write lock at the same time.)
If you can ignore the forget calls which are parallel with get calls it could be more simple:

public T get() {
    readLock.lock();
    try {
        final T value = retrieve();
        if (!isInvalid(value)) {
            return value;
        }
    } finally {
        readLock.unlock();
    }

    writeLock.lock();
    try {
        T value = retrieve();
        if (!isInvalid(value)) {
            return value;
        }
        return create();
    } finally {
        writeLock.unlock();
    }
}

(Here if (isValid(value)) would be more natural than the current if (!isInvalid(value) ....)
I'd consider using an AtomicReference and a simple ReentrantLock too:

public final class ForgettableMemoizingSupplier<T>
        implements ForgettableSupplier<T> {

    private final Supplier<T> delegate;

    private final AtomicReference<T> resourceRef = new AtomicReference<T>();

    private final ReentrantLock lock = new ReentrantLock();

    public ForgettableMemoizingSupplier(final Supplier<T> delegate) {
        this.delegate = checkNotNull(delegate, "delegate cannot be null");
    }

    @Override
    public void forget() {
        resourceRef.set(null);
    }

    @Override
    public T get() {
        final T resource = resourceRef.get();
        if (resource != null) {
            return resource;
        }
        return getAndSetResource();
    }

    private T getAndSetResource() {
        lock.lock();
        try {
            final T resource = resourceRef.get();
            if (resource != null) {
                return resource;
            }
            final T newResource = delegate.get();
            resourceRef.set(newResource);
            return newResource;
        } finally {
            lock.unlock();
        }
    }
}

(Untested code.)


Throttling duplicate requests in an HttpModule I'm writing a method to throttle duplicate requests taking place within multiple HttpModule instances within a web application.

Currently, I have the following setup:

// Container for semaphores
private static readonly ConcurrentDictionary<string, SemaphoreSlim>
SemaphoreSlims = new ConcurrentDictionary<string, SemaphoreSlim>();

// Wrapper for getting semaphore
private static SemaphoreSlim GetSemaphoreSlim(string id)
{
    return SemaphoreSlims.GetOrAdd(id, new SemaphoreSlim(1, 1));
}

private async Task ProcessImageAsync(HttpContext context)
{
    // `hash` is the request path hashed.
    SemaphoreSlim semaphore = GetSemaphoreSlim(hash);
    await semaphore.WaitAsync();

    try
    {

    // Do awaitable task

    }
    finally
    {
        semaphore.Release();
    }
}

This will work but I have no way of disposing of the semaphore instances since I need to preserve the ConcurrentDictionary and cannot dispose of a semaphore within the Dispose() method for that HttpModule instance as it could be currently waiting in another instance.

By my calculation it would take up ~156 megabytes of memory for 1 million items stored in the dictionary. It's not a massive amount but I'd really like it to be scalable and somehow find a way to clean up after myself.

Is this a sensible pattern and is there a way I can improve on it?

Semaphore has a bigger overhead than a traditional lock pattern, aside being too complex for what you need. You could use a class such as this:

public class DeDuper : IDisposable
{
    private static readonly ConcurrentDictionary<String, Object> currentRequests = new ConcurrentDictionary();
    private string _hash;

    public DeDuper(string hash)
    {
        _hash = hash;
        var lockable = currentRequests.GetOrAdd(hash, () => new Object());
        Monitor.Enter(lockable);
    }

    public void Dispose()
    {
        object lockable;
        if(currentRequests.TryGetValue(_hash, out lockable) && Monitor.IsEntered(lockable))
        {
            Monitor.Exit(lockable);
        }
    }

    ~DeDuper()
    {
        Dispose();
    }
}

Then your class can simply be written as:

private async Task ProcessImageAsync(HttpContext context)
{
    using(new DeDuper(hash))
    {
        // do awaitable task
    }
}

You will have a class that only allows a single hash to be run at any one time, with the added side effect of not having to release the Semaphore ;)

Not sure if this would work, i'd need to benchmark but, you could reduce the amount of memory further by reducing the commonality before you hash, eg:

http://mydomain.com/path1/part1   becomes   path1/part1
http://mydomain.com/path1/part2   becomes   path1/part2

Update

I thought that you might be able to use Interlocked.CompareExchange to do the job that you want, but it turns out AutoResetEvent works better. Here is a working sample:

Actual locking code

public class LockAcquisition<T> : IDisposable
{
    private static readonly ConcurrentDictionary<T, LockWrapper> currentRequests = new ConcurrentDictionary<T, LockWrapper>();
    private readonly LockWrapper _currentLock;

    public LockAcquisition(T key)
    {
        _currentLock = currentRequests.GetOrAdd(key, new LockWrapper());
        _currentLock.AcquireLock();
    }

    public void Dispose()
    {
        if (_currentLock != null)
        {
            _currentLock.Dispose();
        }
    }

    ~LockAcquisition()
    {
        Dispose();
    }

    private class LockWrapper : IDisposable
    {
        private readonly AutoResetEvent _locker = new AutoResetEvent(true);

        public void AcquireLock()
        {
            _locker.WaitOne();
        }

        public void Dispose()
        {
            _locker.Set();
        }

        ~LockWrapper()
        {
            Dispose();
        }

    }
}

Wrapped to use a string as the key

public class LockAcquisition : LockAcquisition<String>
{
    public LockAcquisition(string key)
        : base(key)
    {
    }
}

Testing code

internal class Program
{
    private static void Main(string[] args)
    {

        Run();
        Console.ReadKey();
    }

    private static async void Run()
    {
        const string hash = "123456";
        const string hash2 = "1234567";

        using (new LockAcquisition(hash))
        {
            using (new LockAcquisition(hash2))
            {
                using (new LockAcquisition(hash)) // This will deadlocked
                {
                }
                await Task.Delay(TimeSpan.FromSeconds(2));
                Console.WriteLine("Done 2");
            }
            await Task.Delay(TimeSpan.FromSeconds(2));
            Console.WriteLine("Done 1");
        }
    }
}


Python simple battleship game I'm relatively new to Python, I decided to make a simple battleship game. Is there anything I should do to make it shorter, or in practice, "better"?

from pprint import pprint as pp
import random

Rows = 0
Columns = 0
turns = 0
Answer = "NaN"

print("Welcome to battleship!")

while (Rows > 10) or (Columns > 10) or (Rows <= 0) or (Columns <= 0):
    Rows = int(input("Please enter the number of rows you want. \n"))
    Columns = int(input("Please enter the number of columns you want. \n"))

def create_grid(Rows, Columns):
    #Creates the 2D Data Grid
    grid = []
    for row in range(Rows):
        row = []
        for col in range(Columns):
            row.append(' ')
        grid.append(row)
    return grid

grid = create_grid(Rows,Columns)

def display_grid(grid, Columns):
    #Prints the labels for the grid
    column_names = 'abcdefghijklmnopqrstuvwxyz'[:Columns]
    print('  | ' + ' | '.join(column_names.upper()) + ' |')
    for number, row in enumerate(grid):
       print(number + 1, '| ' + ' | '.join(row) + ' |')

grid = create_grid(Rows, Columns)
display_grid(grid, Columns)

def random_row(grid):
    #Makes a random row integer
    return random.randint(1,len(grid))

def random_col(grid):
    #Makes a random column integer
    return random.randint(1,len(grid[0]))

def update_gridHit(grid, GuessRow, GuessColumn):
    grid[GuessRow-1][GuessColumn-1] = 'O'

def update_gridMiss(grid, GuessRow, GuessColumn):
    grid[GuessRow-1][GuessColumn-1] = 'X'

ShipRow = random_row(grid)
ShipColumn = random_col(grid)

#Testing purposes only, comment out if needed.
print(ShipRow)
print(ShipColumn)

while (turns != 5):
    GuessRow = int(input("What row do you guess? \n"))
    GuessColumn = int(input("What column do you guess? \n"))

    if (GuessRow == ShipRow) and (GuessColumn == ShipColumn):
        turns += 1
        update_gridHit(grid, GuessRow, GuessColumn)
        display_grid(grid, Columns)
        print("You hit the battleship! Congratulations!")
        break

    else:
        if (GuessRow < 1 or GuessRow > Rows) or (GuessColumn < 1 or GuessColumn > Columns):
            #Warning if the guess is out of the board
            print("Outside the set grid. Please pick a number within it your Rows and Columns.")

        elif (grid[GuessRow-1][GuessColumn-1] == "X"):
            #If "X" is there than print that it missed
            print("You guessed that already.")

        else:
            #Updates the grid with an "X" saying that you missed the ship
            turns += 1
            print("You missed the ship.")
            update_gridMiss(grid, GuessRow, GuessColumn)
            display_grid(grid, Columns)

    if (turns >= 5):
        print("Game over! You ran out of tries")

Variable naming

In general, you should follow PEP8, the Python style guide. In particular, it suggests using lowercase for variable names. That is, rows instead of Rows.

User input conditions

while (Rows > 10) or (Columns > 10) or (Rows <= 0) or (Columns <= 0):
    Rows = int(input("Please enter the number of rows you want. \n"))
    Columns = int(input("Please enter the number of columns you want. \n"))

So, if I enter a correct row, but a wrong column, it'll ask again. Without telling me what I did wrong.

I'd suggest

rows = 0
while not (0 < rows <= 10):
    rows = int(input("Please enter the number of rows you want [1-10]."))

And a similar loop for columns. This has one problem: What if the user does not enter a number but something else? 'Pizza' or 'one'. In that case, the int throws an exception, and that should be handled as well.

rows = 0
while not (0 < rows <= 10):
    try:
        rows = int(input("Please enter the number of rows you want [1-10]."))
    except ValueError:
        pass  # Just re-ask the question.

Mixing statements and definitions

The code as given has the form

define function
run some statements
define function
run some statements

In general, you should use

define function
define function
define function
define function main:
    statements
    statements
if __name__ == "__main__":
   main()

Re-using variables

Notice how in create_grid, the variable row has two purposes: It is both a number (due to the for row in range(Rows)), and a list. Please don't do that. Because the number row is unused, we should indicate that to the readers by using the conventional _ as variable name.

def create_grid(Rows, Columns):
    #Creates the 2D Data Grid
    grid = []
    for _ in range(Rows):
        row = []
        for _ in range(Columns):
            row.append(' ')
        grid.append(row)
    return grid

(Note: I myself prefer using a double underscore __ instead, but convention is _.)

Encapsulation

Not sure if you are at the level of classes yet, but I'd suggest learning about them.

Instead of

def create_grid(Rows, Columns):
    #Creates the 2D Data Grid
    grid = []
    for row in range(Rows):
        row = []
        for col in range(Columns):
            row.append(' ')
        grid.append(row)
    return grid

grid = create_grid(Rows,Columns)

You can write

class Grid(object):
    def __init__(self, columns, rows):
        self.columns = columns
        self.rows = rows
        self._grid = []
        for __ in range(rows):
            row = []
            for __ in range(columns):
                row.append(' ')
            self._grid.append(row)

Now, that last part (assigning self._grid) can be done 'nicer' (and faster!).

self._grid = [ [' ' for __ in range(columns)] for __ in range(rows)]

Useless call to .upper.

In display_grid, you've written

    column_names = 'abcdefghijklmnopqrstuvwxyz'[:Columns]
    print('  | ' + ' | '.join(column_names.upper()) + ' |')

Why not

    column_names = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'[:columns]
    print('  | ' + ' | '.join(column_names) + ' |')

That's a bit nicer, right?

Using enumerate

The builtin enumerate has a second argument: start. You can use it to signal counting should start at 1 instead:

    for number, row in enumerate(grid, 1):
       print(number, '| ' + ' | '.join(row) + ' |')

display_grid as a method on the class

If you follow my suggestion of turning the grid into a class, you can write

def display(self):
    """
    Displays a grid.
    """
    column_names = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'[:self.columns]
    print('  | ' + ' | '.join(column_names) + ' |')
    for number, row in enumerate(self._grid, 1):
       print(number, '| ' + ' | '.join(row) + ' |')

But that's just a suggestion.

grid = create_grid(Rows, Columns)
display_grid(grid, Columns)

Docstrings

In Python, it is the convention to add the documentation of a method/function in a docstring.

Instead of

def random_row(grid):
    #Makes a random row integer
    return random.randint(1,len(grid))

write

def random_row(grid):
    """
    Makes a random row integer
    """

(There are also a lot of conventions on how to phrase a docstring. I won't get into that for now.)

Parenthesis in conditional

The code says

while (turns != 5):

It could (and should) be

while turns != 5:

That is, you don't need the ( and ) there.

Weird order in the guess loop

Now, the following is just preference. But to me, the final loop is unclear. I've written it as pseudo-code below.

The final part looks like this (pseudocode)

while (turns != 5):
    get_guess
    if hit:
        turns += 1
        notify_success
        break
    else:
        if out_of_bounds:
            # Print out-of-bounds message
        elif already_guessed:
            # Print already guessed message
        else:
            # Update miss
if (turns >= 5):
    # Print 'you lost'

What I notice is that it first checks for hit, before checking if the input is valid (in bounds, and not already guessed).

I'd suggest cleaning it up as follows:

while turns != 5:
    while True:
        get_guess()
        if out_of_bounds:
            # Print out-of-bounds message
        elif already_guessed:
            # Print already guessed message
        else:
            break
    turns += 1
    if hit:
        # Update grit hit.
        # Print hit message.
        break
    else:
        # Update grid miss
        # Print miss message.
    display_grid()