Basic network layer for a JSON API client Just looking for some feedback/ways to improve this basic networking layer written in Swift. I'm still learning, please elaborate with as much detail as possible.

enum HTTPMethod: String {
    case get = "GET"
}

 class HTTPLayer {

     let baseURL = URL(string: "https://example.com")!

     func request(at endpoint: Endpoint, completion: @escaping (Data?, Error?) -> Void) {
        let url = baseURL.appendingPathComponent(endpoint.path)
        let task = URLSession.shared.dataTask(with: url) { (data, response, error) in
            completion(data, error)
        }
        task.resume()
    }

 }

 enum Endpoint {

     case listTransactions
    case transactionDetails(String)

     var method: HTTPMethod {
        return .get
    }

     var path: String {
        switch self {
        case .listTransactions:
            return "/api/transactions/"
        case .transactionDetails(let id):
            return "/api/transactions/id"
        }
    }

 }

 struct Transaction: Decodable {
    let id: String
    let amount: Double
}

 struct TransactionDetails: Decodable {
    let payload: [String: String]
}

 enum Result<T> {
    case success(T)
    case failure(NSError)
}

 class APIClient {

     let httpLayer: HTTPLayer
    init(httpLayer: HTTPLayer) {
        self.httpLayer = httpLayer
    }

     func transactions(completion: @escaping (Result<[Transaction]>) -> Void) {
        self.httpLayer.request(at: .listTransactions) { (data, error) in
            if error != nil {
                completion(.failure(error! as NSError))
            } else {
                do {
                    let transactions = try JSONDecoder().decode([Transaction].self, from: data!)
                    completion(.success(transactions))
                } catch {
                    completion(.failure(self.defaultError))
                }
            }
        }
    }

     func transactionDetails(for transaction: Transaction, completion: @escaping (Result<TransactionDetails>) -> Void) {
        self.httpLayer.request(at: .transactionDetails(transaction.id)) { (data, error) in
            if error != nil {
                completion(.failure(error! as NSError))
            } else {
                do {
                    let transactions = try JSONDecoder().decode(TransactionDetails.self, from: data!)
                    completion(.success(transactions))
                } catch {
                    completion(.failure(self.defaultError))
                }
            }
        }
    }

     var defaultError: NSError = NSError(domain: "", code: 1, userInfo: nil)

 }

There are few things that can be improved, will start with the major ones:

General

Instead of having function for each API call you can generalise it a bit like so:

/* Updated the name, since it's not only for transactions anymore
Since we no longer know the exact endpoint we will have to pass it to the function

I would also add `requestMeta` parameter that will contain any other information that would be needed to perform the api call
`parameters`, `encoding`, `authenticationType`, `additionalHeaders`, `shouldReauthenticate`
but this is in case you need them
*/

func apiReques<T: Codable>(_ dataType: T.Type,
                                    endpoint: Endpoint,
                                  completion: @escaping (Result<[T], ResultError>) -> Void) {

        self.httpLayer.request(at: endpoint) { (data, error) in

// In general an "early return" strategy is preferred, use guard when applicable and avoid nesting `if` statements
            if let error = error as NSError? {
                completion(.failure(error))
                return
            }

// Do not force unwrap, it's better to show empty screen (or error) instead of crashing the app
            guard let data = data else {
                completion(.failure(NSError(domain: "", code: 1, userInfo: [:])))
                return
            }

            do {
                let transactions = try JSONDecoder().decode([T].self, from: data)
                completion(.success(transactions))
            } catch {
                completion(.failure(self.defaultError))
            }
        }
    }

So, since your structures are Decodable (consider using Codable) you can easily use generics.

The _ dataType: T.Type is a bit tricky. Since we are not returning T we have to add it as a parameter so the compiler can infer the actual type (more on the topic can be found here

I would prefer keeping all the urls in one place, so I would move baseURL to endpoints enum. Again do not force unwrap

enum Endpoint {

    static let baseURLString = "https://example.com"

    case listTransactions
    case transactionDetails(String)
}

extension Endpoint {

    var method: HTTPMethod {
        return .get
    }

    var path: String {
        switch self {
        case .listTransactions:
            return Endpoint.baseURLString + "/api/transactions/"
        case .transactionDetails(let id):
            return Endpoint.baseURLString + "/api/transactions/\(id)"
        }
    }
}

This will affect your request func:

func request(at endpoint: Endpoint, completion: @escaping (Data?, Error?) -> Void) {

        guard let url = URL(string: endpoint.path) else { return }
// endpoint.method is available as well if needed

        let task = URLSession.shared.dataTask(with: url) { (data, response, error) in
            completion(data, error)
        }
        task.resume()
    }

It seems that you are instantiating new HTTPLayer for every APIClient. In that case I would just go with private let httpLayer: HTTPLayer = HTTPLayer() and deleting the init. In that case you can mark class HTTPLayer as fileprivate so noone beside APIClient can access it and if any params needs to be passed to it will go through APIClient -> apiReques()

Error handling

If you want to go a bit further you can update the enum Result

enum Result<Value, Error: Swift.Error> {
    case success(Value)
    case failure(Error)
}

and also adding your custom errors:

enum ResultError: Error {
    case general(String?)
    case specific(Int?)
    case `default` // default is a keyword so if you want to use it, you should wrap it in ``, imo don't and choose another name
    case whateverYouWant
}

extension ResultError {
    var errorString: String {
        switch self {
...
        }
    }

    private func specificErrorString(_ errorCode: Int) -> String {
        switch errorCode {
...
        }
    }
}

This will affect the generic apiRequest a bit in the signature @escaping (Result<[T], ResultError>) -> Void) and ofc the completions

completion(.failure(.general("something went wrong")))
completion(.failure(.specific(123))
completion(.failure(.default)

Now you can delete var defaultError: NSError = NSError(domain: "", code: 1, userInfo: nil)

How to use it

APIClient(). apiReques(Transaction.self,endpoint: Endpoint.listTransactions) { data in

    switch data {
    case .success(let response):
        print("Success")
    case .failure(let error):
        print("error")
    }
}

Bonus

You can also split the completion into two completion success and failure

Signature

func apiReques <T: Decodable>(_ dataType: T.Type,
                                    endpoint: Endpoint,
                                    success: @escaping (([T]) -> Void),
                                    failure: @escaping ((ResultError) -> Void))

block calls

failure(.general(""))
failure(.default)
success(data)

Usage

APIClient().transactions(Transaction.self,
                         endpoint: Endpoint.listTransactions,
                         success: { data in
                            print(data)
}, failure: { error in
    print(error)
})


Fixing navbar when scrolling beyond a certain number of pixels I have written a jquery code, which fixes a navbar header on top of page when a user scrolls down to a certain pixels. But I am not sure whether is this the correct way to do this.

Because this is going to be a majorly used function by all users, it would be better if I can improve it in terms of optimizations and speed.

What basically I am doing is removing few classes and adding few classes when that breakpoint reaches.

$(function () {
    $(window).scroll(function (event) {
        var scroll = $(window).scrollTop();

        if(scroll > 130){
            $('#catNavBar').removeClass("navbar-static-top");
            $('#catNavBar').addClass("navbar-fixed-top");
            $('.gbText').addClass('hidden');
            $('.small-logo').removeClass('hidden');
            $('#headerGbCont').css('width', '50px');
        } else{
            $('#catNavBar').addClass("navbar-static-top");
            $('#catNavBar').removeClass("navbar-fixed-top");
            $('.gbText').removeClass('hidden');
            $('.small-logo').addClass('hidden');
            $('#headerGbCont').css('width', '109px');
        }
    });
});

Can you suggest any improvements in this code? Or anything that I am doing wrong that should be avoided?

Cache your jQuery objects as @Abbas mentioned. Also, you could use toggleClass with a state switch:

var $catNavBar = $('#catNavBar'),
    $headerGbCont = $('#headerGbCont'),
    followScroll = $(window).scrollTop() > 130;

$catNavBar.toggleClass('navbar-static-top', !followScroll);
$catNavBar.toggleClass('navbar-fixed-top', followScroll);
// etc..
$headerGbCont.width(followScroll ? 50 : 109);

Regarding magic numbers like 130, I would suggest that you calculate distances between elements and define rules based on the result rather than approximating when you should switch classes.


Image duplicate finder I have a bunch of exact-duplicate pictures that I've acquired over the years. I'd like to create a list of all them so I can eventually delete some. My idea was simple: dump the hash and location of every image file under a path in MongoDB for later analysis. This is what I came up with:

import com.david.mongodocs.ImageEntry;
import com.mongodb.MongoClient;
import org.apache.commons.codec.digest.DigestUtils;
import org.mongodb.morphia.Datastore;
import org.mongodb.morphia.Morphia;

import java.io.FileInputStream;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;

public class MD5Deduplicator {
    private static Datastore datastore;

    public static void main(String[] args) throws Exception {
        long startTime = System.nanoTime();
        Morphia morphia = new Morphia();
        morphia.mapPackage("com.david.mongodocs");
        datastore = morphia.createDatastore(new MongoClient(), "md5Deduplicator");
        datastore.ensureIndexes();
        logDuplicates(Paths.get(args[0]));
        System.out.println("Completed scan in " + (System.nanoTime() - startTime )+ " nanosecs");
    }

    private static void logDuplicates(Path path) throws IOException {
        Files.walk(path).parallel()
                .filter(Files::isReadable)
                .filter(Files::isRegularFile)
                .forEach(filePath -> {
                    try {
                        String contentType = Files.probeContentType(filePath);
                        if (contentType != null && contentType.startsWith("image")) {
                            FileInputStream fis = new FileInputStream(filePath.toFile());
                            String md5 = DigestUtils.md5Hex(fis);
                            fis.close();
                            ImageEntry imageEntry = new ImageEntry(filePath.toAbsolutePath().toString(), md5);
                            datastore.save(imageEntry);

                        }
                    } catch (Exception e) {
                        e.printStackTrace();
                    }

                });
    }
}

ImageEntry class:

package com.david.mongodocs;

import org.mongodb.morphia.annotations.Entity;
import org.mongodb.morphia.annotations.Id;
import org.mongodb.morphia.annotations.Indexed;

@Entity
public class ImageEntry {
    @Id
    public final String filePath;
    @Indexed
    public final String md5;

    public ImageEntry(String filePath, String md5) {
        this.filePath = filePath;
        this.md5 = md5;
    }
}

From the visualVM profiler it looks like the slowest part of this is the md5hex function (I was a little surprised, I was expecting the slowest part to be related to the FileInputStream or save function). Is there perhaps an alternative hashing function or md5 implementation I should use?

I'm also a little worried that Files.walk().parallel() might be putting a little bit too much faith in Java defaults and may not be the most optimized way to parallelise this.

The question and the subject is interesting. Recently I needed to make some similar processing; this question pushed me to proceed.

Review

First of all, some remarks about the original implementation.

The method logDuplicates(Path), according to what I see, does not log duplicates, but saves into mongodb all ImageEntry objects corresponding to all image files found in the path. So, to trace duplicates, it is still necessary to execute a couple of requests in mongodb.

There is some room for improvements in this method. Calling datastore.save(imageEntry) for each item separately looked rather suspicious. Doing it in batch mode for all available items should make the thing quicker. Indeed, Datastore.save is overloaded with save(Iterable<T>) and save(T... entities). A slightly improved version of the method would look like this:

private void digestImages(Path path) throws IOException {
  List<ImageEntry> images = new ArrayList<>(APPROX_IMAGES_COUNT);
  Files.walk(path)
       .parallel()
       .filter(Files::isReadable)
       .filter(Files::isRegularFile)
       .forEach(filePath -> {
         if (isImage(filePath)) {
             ImageEntry img = digestAndBuildImageEntry(filePath);
             if (img != null) {
               images.add(img);
             } else {
               System.out.println(String.format("Failed to digest image: %1$s", filePath));
             }
         }});
  datastore.save(images);
}

private boolean isImage(Path path) {
  try {
    String contentType = Files.probeContentType(path);
    return contentType != null && contentType.startsWith("image");
  } catch (IOException ex) {
    ex.printStackTrace();
    return false;
  }
}

private ImageEntry digestAndBuildImageEntry(Path filePath) {
  try (InputStream is = Files.newInputStream(filePath);
       BufferedInputStream buffered = new BufferedInputStream(is)) {
    String hash = DigestUtils.md5Hex(buffered);
    return new ImageEntry(filePath.toAbsolutePath().toString(), hash);
  } catch (IOException ex) {
    ex.printStackTrace();
    return null;
  }
}

Tests

I've got a folder with about 900 JPG image files in numerous sub-folders that I used for tests. Introducing a try-with-resources + batch saving seemed to improve the overall performance for about 10% (see results below).

Based on this SO post that helped me recall different ways and APIs to produce checksums and hashes, I executed a few tests to compare the perfs with the reviewed implementation, as well as when using Guava. The averages of 5 runs on my old i7 gave the following results:

impl               avg time,ms    %
-------------------------------------
original              6583      100.0
reviewed              5873       89.2
guava/sha1            8267      125.6
guava/md5             5865       89.1
guava/murmur3-128     3819       58.0
guava/murmur3-32      2689       40.9
guava/adler32         2432       36.9
-------------------------------------

For the tests with Guava, I used the following overloading of digestAndBuildImageEntry:

private ImageEntry digestAndBuildImageEntry(Path filePath, HashFunction hashFunc) {
  try {
    String hash = com.google.common.io.Files.hash(filePath.toFile(), hashFunc).toString();
    return new ImageEntry(filePath.toAbsolutePath().toString(), hash);
  } catch (IOException ex) {
    ex.printStackTrace();
    return null;
  }
}

So, we can see that, at least for my test case and without entering into theoretical discussions, Guava's adler32 hash performs almost three times faster than the original implementation.


Ember App - Initial State I'm learning Ember, and I've come across a situation where I haven't been able to find a pattern online.

My application's main route includes a list of "stories" (think scrum).  However, this list also has a StoryController associated with it.  I don't want to assign the stories in the IndexRoute file; it doesn't feel like it belongs there.  I ended up adding a property to the StoriesController and calling the App.Story.fetch() method to the Controller's init() method.  I can't help but feel there is a better, more "Ember" way to do this.

App.IndexRoute

App.IndexRoute = Ember.Route.extend({

    renderTemplate: function(){
        this.render();

        this.render('stories');
    }
});

App.StoriesController

App.StoriesController = Ember.ArrayController.extend({
    init : function(){
        this.stories = App.Story.find();
    }
});

I think you can pass in the controller to the "stories" render function and use the model hook :

App.IndexRoute

App.IndexRoute = Ember.Route.extend({
    renderTemplate: function(){
        this.render();
        this.render('stories', {controller: App.StoriesController});
    }
});

App.StoriesController

App.StoriesController = Ember.ArrayController.extend({
    model : function() {
        return App.Story.find();
    }
});


Calculating pi via collisions This code calculates pi via collisions; it asks for a user input of N which determines the mass of the second block. It is fully working, it just takes forever to run when N >= 2. I want to be able have at least N=5 and have a reasonable runtime. The issue is in the velocity and x1, x2 calculation I believe. There are just so many that need to be appended that it takes forever to run and then just as long to append.

My code shows that I have tried to use Numba to speed up the runtime, but that doesn't seem to be helping. I am currently using the RK-4 method to update position, and I previously tried the Verlet method which did not seem to have an affect on the runtime. Any help on this is greatly appreciated.

import numpy as np
import matplotlib.pyplot as plt
from matplotlib.animation import FuncAnimation
import matplotlib.animation as animation
from numba import jit
import time

start = time.time()

@jit(nopython=True)
def RK4_1(x, vx):
    return vx

@jit(nopython=True)
def RK4_2(x, vx):
    return 0

@jit(nopython=True)
def iterate(x1, x2, vx1, vx2, col):
    k1 = dt*RK4_1(x1, vx1)
    k2 = dt*RK4_1(x1 + k1/2, vx1)
    k3 = dt*RK4_1(x1 + k2/2, vx1)
    k4 = dt*RK4_1(x1 + k3, vx1)
    x1 += (1/6)*(k1 + 2*k2 + 2*k3 + k4)

    k1 = dt*RK4_1(x2, vx2)
    k2 = dt*RK4_1(x2 + k1/2, vx2)
    k3 = dt*RK4_1(x2 + k2/2, vx2)
    k4 = dt*RK4_1(x2 + k3, vx2)
    x2 += (1/6)*(k1 + 2*k2 + 2*k3 + k4)

    k1 = dt*RK4_2(x1, vx1)
    k2 = dt*RK4_2(x1, vx1 + k1/2)
    k3 = dt*RK4_2(x1, vx1 + k2/2)
    k4 = dt*RK4_2(x1, vx1 + k3)
    vx1 += (1/6)*(k1 + 2*k2 + 2*k3 + k4)

    k1 = dt*RK4_2(x2, vx2)
    k2 = dt*RK4_2(x2, vx2 + k1/2)
    k3 = dt*RK4_2(x2, vx2 + k2/2)
    k4 = dt*RK4_2(x2, vx2 + k3)
    vx2 += (1/6)*(k1 + 2*k2 + 2*k3 + k4)

    if x1 < 0:
        x1 = 0
        vx1 = -vx1
        col += 1

    if x2 < x1:
        x2 = x1
        vx1_i = vx1
        vx2_i = vx2
        vx1 = (2*m2*vx2_i + m1*vx1_i - m2*vx1_i)/(m1+m2)
        vx2 = (2*m1*vx1_i + m2*vx2_i - m1*vx2_i)/(m1+m2)
        col += 1

    return x1, x2, vx1, vx2, col

dt = 0.01

m1 = 1
N = int(input("Enter an integer N that will determine the mass of the second block: "))
m2 = 100**N

w1 = 1
w2 = w1*(100**N)**(1/3)
x1 = 1
x2 = 1.15
y1 = 1
y2 = 1
vx1 = 0
vx2 = -1
col = 0

x1arr = np.array([])
x2arr = np.array([])
y1arr = np.array([])
y2arr = np.array([])
vx1arr = np.array([])
vx2arr = np.array([])
colarr = np.array([])

t = 0

while (vx2 < 0) or (abs(vx1) > abs(vx2)):
    x1, x2, vx1, vx2, col = iterate(x1, x2, vx1, vx2, col)
    #print(vx1, vx2)
    t += dt

    x1arr = np.append(x1arr, x1)
    x2arr = np.append(x2arr, x2)
    y1arr = np.append(y1arr, y1)
    y2arr = np.append(y2arr, y2)
    vx1arr = np.append(vx1arr, vx1)
    vx2arr = np.append(vx2arr, vx2)
    colarr = np.append(colarr, col)

print("Number of collisions: %f" % (col))

speed = 1000

def update_plot(i, fig, scat1, scat2, txt):
    last = 0
    if(i>int(len(x1arr)/speed)-2):
        last=1
    s = int(speed*i)
    scat1.set_data(x1arr[s],y1arr[s])
    scat2.set_data(x2arr[s],y2arr[s])
    #scat.set_sizes(lx1=5, lx2=5*N)
    txt.set_text('x1= %.3f   m1=%.0f\nx2= %.3f   m1=%.0f\nCollisions=%.0f\n t=%.3fs' % (x1arr[s],m1,x2arr[s],m2,colarr[s]+last,(s*dt))) #update of legend
    #print("Frame %d Rendered" % (s))
    return scat1, scat2, txt,

size = 5*N
fig =  plt.figure()
ax = fig.add_subplot(111)
ax.set_xlim([0, 3]) #animation scale
ax.set_ylim([0,5])
ax.grid()
txt = ax.text(0.05, 0.8, '', transform=ax.transAxes)
scat1, = ax.plot([], [],'s', c='r', markersize=5)
scat2, = ax.plot([], [],'s', c='r', markersize=5*(N+1))
anim = FuncAnimation(fig, update_plot, fargs = (fig, scat1, scat2, txt), frames = int(len(x1arr)/speed), interval = 1, blit=True, repeat=False)
anim.save("originalpi.mp4", fps=30, bitrate=-1)

end = time.time()
print("Total runtime in seconds:  ", end-start)

plt.show()

I am currently using the RK-4 method to update position

Firstly, the naming is unhelpful here. A name like RK4_1 implies that the method does the quadrature. I think that function is really dx_dt, RK4_2 is really dvx_dt, and there would be room to pull out a function like

def RK4_step(t, x, dx_dt, dt):
    k1 = dt * dx_dt(x, t)
    k2 = dt * dx_dt(x + k1 / 2, t + dt / 2)
    k3 = dt * dx_dt(x + k2 / 2, t + dt / 2)
    k4 = dt * dx_dt(x + k3, t + dt)
    return x + (1/6) * (k1 + 2 * k2 + 2 * k3 + k4)

But secondly, RK4 is completely overkill here. If we inline

@jit(nopython=True)
def RK4_1(x, vx):
    return vx

@jit(nopython=True)
def RK4_2(x, vx):
    return 0

@jit(nopython=True)
def iterate(x1, x2, vx1, vx2, col):
    k1 = dt*RK4_1(x1, vx1)
    k2 = dt*RK4_1(x1 + k1/2, vx1)
    k3 = dt*RK4_1(x1 + k2/2, vx1)
    k4 = dt*RK4_1(x1 + k3, vx1)
    x1 += (1/6)*(k1 + 2*k2 + 2*k3 + k4)

    ...

    k1 = dt*RK4_2(x1, vx1)
    k2 = dt*RK4_2(x1, vx1 + k1/2)
    k3 = dt*RK4_2(x1, vx1 + k2/2)
    k4 = dt*RK4_2(x1, vx1 + k3)
    vx1 += (1/6)*(k1 + 2*k2 + 2*k3 + k4)

we get

def iterate(x1, x2, vx1, vx2, col):
    x1 += dt*vx1

    ...

    vx1 += 0

It seems that it would be much quicker to solve simultaneous linear equations to work out when the collision will happen rather than to use quadrature. In fact, I had a go, and the actual simulation takes less than a second for N = 5.

  My code shows that I have tried to use Numba to speed up the runtime, but that doesn't seem to be helping.

The jit is on the wrong parts. I added some debug printing and observed that it slowed down over time, at which point I very quickly spotted the real problem:

    x1arr = np.append(x1arr, x1)
    x2arr = np.append(x2arr, x2)
    y1arr = np.append(y1arr, y1)
    y2arr = np.append(y2arr, y2)
    vx1arr = np.append(vx1arr, vx1)
    vx2arr = np.append(vx2arr, vx2)
    colarr = np.append(colarr, col)

This is copying the entire array every time, so the length of a step is proportional to the number of steps taken.

Solution: use normal lists rather than numpy arrays. numpy is not a panacea: it's good for parallel processing, but that's not what you want here.

However, the one advantage that numpy arrays give you here is that they don't have length bounds. In testing with N = 3 using normal Python lists I got a memory error after slightly more than 245 million steps and 3140 collisions. It might need a 2D structure to handle the large quantities of data. Of course, that doesn't prevent the extremely likely problem of the plot running into memory problems too. A complete structural rethink may be necessary.

If you do decide to implement RK4, it's more correct to do a single multi-variable quadrature rather than multiple single-variable ones. Here is where there is a good reason to use numpy arrays. What I mean by multi-variable quadrature is something like

k1_x, k1_vx = dt * RK4(x, vx), dt * RK4(vx, ax)
k2_x, k2_vx = dt * RK4(x + k1_x / 2, vx + k1_vx / 2), dt * RK4(vx + k1_vx / 2, ax)

etc. (NB For clarity I separated out the variables. For speed you'd put x and vx in a single numpy array).

It makes no sense to start timing before taking the input: the time I take to type 3 and press Enter is not really runtime.

In terms of general readability, the inline mixing of functions and top-level code is not helpful. It's good to put constants like dt at the top, so that when the reader comes to the usage of dt in iterate they've already seen it. The main loop should probably be a function which returns the arrays (or perhaps a single 2D array) with intermediate values; the plot creation should be a function, and then Python best practice is for the "main" functionality to go at the end with a guard:

if __name__ == "__main__":
    N = int(input("Enter an integer N that will determine the mass of the second block: "))
    start = time.time()
    data = generate_collisions(N)
    plot = graph_collisions(data)
    end = time.time()
    print("Total runtime in seconds:  ", end - start)
    plot.show()


Rotating an NxN matrix I came up with the following solution for rotating an NxN matrix 90 degrees clockwise, to solve this CodeEval challenge:

  Input

  The first argument is a file that contains 2D N×N matrices (where 1 <= N <= 10), presented in a serialized form (starting from the upper-left element), one matrix per line. The elements of a matrix are separated by spaces.

  For example:

a b c d e f g h i j k l m n o p

  Output

  Print to stdout matrices rotated 90° clockwise in a serialized form (same as in the input sample).

  For example:

m i e a n j f b o k g c p l h d

It looks elegant to me, but can someone explain how its performance compares to the more common solutions mentioned here? Does it have \$O(n^2)\$ time complexity?

import sys, math

for line in open(sys.argv[1]):
    original = line.rstrip().replace(' ', '')
    nSquared = len(original)
    n = int(math.sqrt(nSquared))
    output = ''

    for i in range(nSquared):
        index = n * (n - 1 - i % n) + int(i / n)
        output += original[index] + ' '

    print(output.rstrip())

The expression n * (n - 1 - i % n) + int(i / n) is something I found while observing common patterns among rotated matrices.

The formula is elegant, and the approach is correct.
You have to be careful with complexities though. Specifically you have to be very clear about what \$n\$ is. Typically complexity is a function of the size of input (which is, provided that \$n\$ is a matrix dimension, \$n^2\$ itself), so I'd qualify your solution as linear. And since each element should be accounted for, no better solution is possible.
The asymptotic constant could be better. The problem doesn't ask to rotate the matrix; it is only asks to print the matrix as if it was rotated. In other words, building output is technically a waste of time. Use your formula to print elements as you enumerate them.


Finds all the prime numbers up to n using a strange pattern Here's an odd pattern I found for sieving out composite numbers to find primes. I wrote it myself, originally thinking I made an indexing error, but it turned out to work after removing squares.

import bitarray
def sieve(n):
    """bitarray sieve"""
    b=bitarray.bitarray([1]*(n+1))
    x=int(n**.5)+1
    for k in range(2,x):
        if b[k]:
            f=k+1 #this line is indexing error
            e=n//k +1
            for g in range(f,e):
                if b[g]:
                    b[g*k]=0
    return sorted(list(to_numbs(b)-squares_set(b)))

def squares_set(ba):
    """this returns a set of all primes**(2n).
note 0**2==0, 1**2==1"""
    return {i**2 for i in range(int(len(ba)**.5)+1) if ba[i]}

def to_numbs(b_ar):
    """converts the bit array to a set of numbers"""
    return {i for i in range(len(b_ar)) if b_ar[i]}
if __name__=="__main__":
    print(sieve(100))

I'm not entirely sure what causes this pattern to work so I figured it would be fun to post here.

The Sieve of Eratosthenes works by going through and marking all numbers that can be divided into, essentially.

Take your input "n", for which you want to find all the prime numbers up to it. The sieve starts at 2, and multiplies 2 by increasing amounts until the multiplication is about to exceed the value "n" - in your program, this is accomplished through the integer division and then iteration up to the calculated result. So, as it multiplies the values together, it marks the result as non-prime. Meaning that 2, 4, 6, 8, 10... etc are all set in the bitarray as 0 (meaning non-prime in this context). It moves on to the next value, so it starts at 3. The process repeats as it marks all multiples as non-prime. Time to move on to the next number - 4. As 4 has been marked as non-prime, it skips over 4 and moves on to 5. This continues until it finally reaches the number which is the value of its square root, so when you are using the sieve to get primes below 100, that means the algorithm checks the multiples of values from 2 to sqrt(100), so 2 to 10. It doesn't need to go past this value as all prime values above that share multiples which values that have already been checked. So, for example, if we moved past 10 to check multiples of 11, we'll get 22 (which is divisible by 2, so has already been marked non-prime) and 33 (divisible by 3). The values which remain with a value of 1 are prime.

tl;dr
The algorithm goes through all prime values below or equal to sqrt(n) and marks their multiples up to n as non-prime. The remaining numbers must be prime.

EDIT: The code above adds 1 to the div operation, which I didn't mention in my explanation.

EDIT 2: This specific modification of the sieve is essentially the same as the original sieve, however rather than removing square values during the normal removal process it removes them separately at the end.


OOP php method - pass parameters in or get from inside object I am new to OOP and have written a products class. All is working fine but I am unsure which of the below version of a method within this class is best?

The first gets the variables from within the object and the second passes the variables into the class. Both work. I originally had it as the first version but things seems to be running slow and then changed it to the second.

public function getProductURLstart(){
    $select = "SELECT l.URL, p.id FROM logins AS l
        INNER JOIN Pages AS p ON l.id = p.clientID
        WHERE l.id = '$this->skID' AND p.productPage = 1";

    $res = mssql_query($select);
    $r = mssql_fetch_row($res);

    $url = trim($r[0]);
    $page_id = $r[1];

    return  $url .'/index.aspx?pageID='. $page_id . '&prodID=$this->prodID';
}

OR

static function getProductURLstart($skID, $prodId){
    $select = "SELECT l.URL, p.id FROM logins AS l
        INNER JOIN Pages AS p ON l.id = p.clientID
        WHERE l.id = '$skID' AND p.productPage = 1";

    $res = mssql_query($select);
    $r = mssql_fetch_row($res);

    $url = trim($r[0]);
    $page_id = $r[1];

    return  $url .'/index.aspx?pageID='. $page_id . '&prodID=$prodId';
}

Which is the better should not depend on mainly speed. I think you should post more code (at least the name of class and its responsibilities or the list of its functions).

Some other notes:

It's really hard to read code which contains one character long variable names:

$r = mssql_fetch_row($res);

You have to decode them. I'd rename it to $row. The same true for the SQL queries: logins AS l. I would let it be logins.
In the second version there are three variable naming convention: $skID, $prodId, $page_id. Try to be consistent and use just one: they should be $skId, $prodId, $pageId or $skID, $prodID, $pageID or $sk_id, $prod_id, $page_id. (I think the first, the camelCase version is the most readable.)
There is no error handling in the code. What happens when the query returns empty results?
Probably the code is vulnerable to SQL injections: http://en.wikipedia.org/wiki/SQL_injection
Try accessing database attributes with keys not numeric indexes.

$url = trim($r[0]);
$page_id = $r[1];

The following would be more readable:

$url = trim($r["url"]);
$page_id = $r["id"];


Kotlin program to summarize git commits by regex match I am trying to learn Kotlin. I am coming from some background in Java. As a learning exercise, I wrote this simple program to summarize occurrences of a string by author email in a list of git repositories.

I am curious if I am approaching things in an idiomatic Kotlin way. Extension functions are awesome but I suspect it's something that I should not overuse.

I plan to make this a command line application and maybe use recursion to get the git repositories from the source directory without explicitly providing them. Before I get too far though I would like to see how I can improve what I have.

Any feedback is appreciated!

import java.io.File
import java.time.OffsetDateTime
import java.time.format.DateTimeFormatter
import java.util.concurrent.TimeUnit
import java.util.regex.Pattern
import kotlin.concurrent.thread

data class CommitSummary(
        var author: String,
        var regexString: String,
        var count: Int)

data class Commit(
        var commit: String = "",
        var author: String = "",
        var date: OffsetDateTime? = null,
        var message: String = "")

fun main(args: Array<String>) {

    val cmd = "git log --all --branches=* --remotes=*"
    val matchStr = "d'{0,1}oh" //case-insensitive
    val gitDir = "C:\\dev\\git\\"

    arrayListOf("repo-1", "repo-2")
            .forEach { repo ->
                thread {
                    val log = cmd.run(File(gitDir + repo), createTempFile())
                    val commits = log.parseGitLog()
                    val summaries = commits.summarizeGitMessages(matchStr)
                    summaries.forEach { author, summary ->
                        println(String.format("repo: %s, author: %s, %s count: %s", repo, author, summary.regexString, summary.count))
                    }
                }
            }
}

fun File.parseGitLog(): ArrayList<Commit> {
    return this.readLines().fold(arrayListOf()) { accumulated, current ->

        val startingWord = current.split("\\s".toRegex())[0]

        when (startingWord) {
            "commit" -> accumulated.add(Commit(current.split("\\s".toRegex())[1]))
            "Author:" -> accumulated.last().author = current.substring(current.indexOf("<") + 1, current.indexOf(">"))
            "Date:" -> accumulated.last().date = current.split("Date:")[1].trim().parseGitDateString()
            else -> accumulated.last().message += current.trim()
        }

        return@fold accumulated
    }
}

fun ArrayList<Commit>.summarizeGitMessages(regexString: String): MutableMap<String, CommitSummary> {
    val pattern = Pattern.compile(regexString, Pattern.MULTILINE or Pattern.CASE_INSENSITIVE)

    return this.fold(mutableMapOf()) { acc, commit ->
        val summary: CommitSummary = acc.getOrPut(commit.author) { CommitSummary(commit.author, regexString, 0) }
        val match = pattern.matcher(commit.message)
        while (match.find()) {
            summary.count = summary.count.plus(1)
        }
        return@fold acc
    }
}

// string extensions
fun String.run(workingDir: File, targetFile: File = File("C:\\dev\\tmpFile")): File {

    if (targetFile.exists()) {
        targetFile.delete()
    }

    ProcessBuilder(*split(" ").toTypedArray())
            .directory(workingDir)
            .redirectOutput(ProcessBuilder.Redirect.appendTo(targetFile))
            .redirectError(ProcessBuilder.Redirect.INHERIT)
            .start()
            .waitFor(60, TimeUnit.MINUTES)

    return targetFile
}

fun String.parseGitDateString(format: DateTimeFormatter = DateTimeFormatter.ofPattern("EEE MMM d HH:mm:ss yyyy Z")): OffsetDateTime {
    return OffsetDateTime.parse(this, format)
}

I have never used Kotlin, so I can't give a comprehensive review or comment particularly on specific idioms.

I'm slightly bothered by this line, because null objects seems very much like a Java habit which Kotlin's paradigm generally tries to avoid.

var date: OffsetDateTime? = null,

It seems to me that Commit is the sort of thing whose values should be known at construction, so consider using the data class constructor.

I'm also a bit nervous about this line:

val gitDir = "C:\\dev\\git\\"

File(gitDir + repo)

I know you'll parameterise the list of repositories you want to use. When you do, it's worth thinking about things like cross platform usability. I can't give a specific recommendation, but it's generally better to use some sort of filesystem library rather than stitching together strings like that.

// string extensions
fun String.run(workingDir: File, targetFile: File = File("C:\\dev\\tmpFile")): File

Class extensions is a really cool capability, but it also feels like a dangerous use of that capabiltiy. As a rule of thumb, you wouldn't have a class method which isn't meaningful and legal to run for whatever instance of that class you have, and similarly be wary of adding extensions that don't make sense of most versions of a class. Since matchStr.run(...) wouldn't make any sense I'd be suspicious of extending String like this. I'd say either have a specific class for runnable strings, or just have a function that takes the string to run as a parameter.


Replacing div if there is any validation error connected with the div value I have an input group:

        @Html.DisplayNameFor(m => m.StorageId)

    @if(ViewData.ModelState["StorageError"] !=null && ViewData.ModelState["StorageError"].Errors.Count>0)
    {

            @Html.DropDownList("storageId", null, new { @class = "form-control",@style="border-color:red" })

    }
    else
    {

            @Html.DropDownList("storageId", null, new { @class = "form-control" })

    }

As you can see, if there are no errors, the site should render div without a red border and tooltip.

Do you think it's efficient and good-looking enough to leave it as it is, or does it need to be rewritten?

Basically, 95% of your code is the same between the two sections, so I'd prefer to use just a single instance with a bool on the page.

@{
    var error = ViewModel.ModelState["StorateError"];
    var hasErrors = error != null && error.Errors.Any();
}

<div class="a" title="@( hasErrors ? error.Errors[0].ErrorMessage : string.Empty" )
    data-toggle="tooltip" data-placement="left" id="storage">
        @Html.DropDownList("storageId", null, new { @class="form-control",
            @style=" @( hasErrors ? "border-color:red", string.Empty ) })
</div>

You should also get into the habit of using .Any() instead of Count() > 0 as Any will stop iterating after it finds the first item that matches the filtering (usually not a big deal, but the collection could get rather large, and it's a minor change to coding that could give big performance enhancements).

The only other thing I might recommend would be to have a CSS class for border-color:red instead of inline styling.


Java Matrix multiplication using Multithreading I am trying to achieve matrix multiplication using the concept of concurrency and cyclic barrier.

Here is the code snippet:-

import Jama.Matrix;
public class Application {
public final CyclicBarrier cyclicBarrier;
public Matrix A = new Matrix(200,120);
public Matrix B = new Matrix(120,100);
public Matrix C = new Matrix(200,100);

public Application() {
    initializeMatrix(A,A.getRowDimension(),A.getColumnDimension());
    initializeMatrix(B,B.getRowDimension(),B.getColumnDimension());
    cyclicBarrier = new CyclicBarrier(4, new Runnable() {
        @Override
        public void run() {
            display(C);
        }
    });

    new Thread(new Task(50,25)).start();
    new Thread(new Task(100,50)).start();
    new Thread(new Task(150,75)).start();
    new Thread(new Task(200,100)).start();
}

public void initializeMatrix(Matrix X,int row,int col){
    for (int i=0;i<row;i++)
        for (int j=0;j<col;j++)
            X.set(i,j,Math.random());
}

public void Multiply(int rowend, int col){

    for (int i=0;i<rowend;i++){
        for (int j=0;j<col;j++){
            double sum = 0;
            for (int k=0;k<B.getRowDimension();k++){
                sum += A.get(i,k)*B.get(k,j);
            }
            C.set(i,j,sum);
        }
    }
    display(C);
}

public void display(Matrix X){
    System.out.print("C = [");
    for (int i=0;i<X.getRowDimension();i++){
        for (int j=0;j<X.getColumnDimension();j++){
            System.out.print(X.get(i,j) + ",");
        }
        System.out.println();
    }
    System.out.print(" ]\n\n");
}

private class Task implements Runnable{
    int row = 0;
    int col = 0;

    public Task(int row, int col){
        this.row = row;
        this.col = col;
    }

    @Override
    public void run() {
        Multiply(row, col);
        try {
            cyclicBarrier.await();
        } catch (InterruptedException iex) {
            System.out.println(iex.getMessage());
        } catch (BrokenBarrierException bbex) {
            System.out.println(bbex.getMessage());
        }
    }
}

public static void main(String args[]) {
    new Application();
}
}

The need of concurrency in this is that the Matrix are huge and I need to do the computation fast. Is this the correct way of doing it? and Can I optimize it further?

Your basic idea is right: you can multiply matrices with heavy multithreading because each new value can be computed as a scalar product of vectors independently from the others. But I heavily doubt you need three nested loops for a scalar multiplication. Here's my approach (it is not tested and you should assure that B.getRowDimension() == A.getColumnDimension() as well as the dimensions of C before):

public void multiply(int row, int col) {
    int sum = 0;
    for (int r = 0; r < B.getRowDimension(); r++)
        sum += A.get(row, r) * B.get(r, col);
    C.set(row, col, sum);
}

This should speed up things up a lot already. If you look at this implementation, this method is the inner loop of the multiplication. The outer ones can be done concurrently and normally you use an ExecutorService and Runnable to do this.

    ExecutorService executor = Executors.newCachedThreadPool();
    for (int x = 0; x < C.getRowDimension(); x++)
        for (int y = 0; y < C.getColumnDimension(); y++) {
            final int row = x, col = y;
            executor.submit(() -> multiply(row, col));
        }
    executor.shutdown();
    executor.awaitTermination(Long.MAX_VALUE, TimeUnit.DAYS);
    display(C);

You already did the right thing to use a runnable and pass the position of the cell you want to calculate, but your Runnable shouldn't wait at the end. In my case, the Runnable is created implicitly through lambdas. After having submitted all tasks, the program waits for them to terminate and then prints the result.

I don't know your implementation of the Matrix, but please make sure it can be edited from multiple places concurrently without messing up. Modifying does not have to be completely thread safe because each cell is only changed from one thread. If you are using an array internally to store the data and the set(r, c, v) method does not modify any states apart from values in the array, it should be fine.


Parzen Window Density Estimation in C# Is my implementation correct (it is matching this result)? How can I improve this code?

public class Parzen
{
    public static double Variance(IEnumerable<double> source)
    {
        double avg = source.Average();

        double d = source.Aggregate(0.0, (total, next) => total += Math.Pow(next - avg, 2));

        return d / (source.Count() - 1);
    }

    private static double GausianFunction(double x1, double x, double sigma2)
    {
        double up = Math.Pow(x1 - x, 2);

        double dw = 2 * sigma2;

        double firstPart = 1 / Math.Sqrt(2 * Math.PI * sigma2);

        double ret = firstPart * Math.Exp((-1) * (up / dw));

        return ret;
    }

    public static double PrazenWindowDensity(double [][] Xn, double x, double sigma2)
    {
        double gauss = 0;

        foreach(double [] arr in Xn)
        {
            foreach (double item in arr)
            {
                double xx = GausianFunction(item, x, sigma2);
                gauss += xx;
            }
        }

        return gauss / Xn.Sum(xy => xy.Length); ;
    }
}

public class Program
{
    static void Main(string[] args)
    {
        double[][] mat =
        {
            new double[]{2, 2.5},
            new double[]{3, 1},
            new double[]{6}
        };

        //double sigma2 = Parzen.Variance(mat.Cast<double>().ToList());

        double den = Parzen.PrazenWindowDensity(mat, 3, 1);

        string str = string.Empty;
    }
}

I assume your implementation is correct then I go directly to (2).

Even assuming you do not worry too much about speed you should replace foreach with for in PrazenWindowDensity(), relative speed-up is consistent:

for (int m=0; m < xn.Length; ++m)
{
    for (int n=0; n < xn[m].Length; ++n)
    {
        gauss += GausianFunction(xn[m][n], x, sigma2);
    }
}

Few notes: I renamed Xn to xn because parameters should be camelCase. I dropped local variable xx because it's not helpful here.

In the same function you're using LINQ to sum lengths of each array, it's deadly slow and it may be replaced with a simple ad-hoc function:

static int SumLengths(double[][] data)
{
    int sum = 0;
    for (int i=0; i < data.Length; ++i)
        sum += data[i].Length;
}

To have this:

return gauss / SumLengths(xn);

Optimization of GausianFunction() might be more controversial. I observed that often (but not always) C# compiler + JIT compiler will emit better assembly code when an expression is computed without intermediate results (if possible all values stay in 80 bit FPU registers, better accuracy and no need to convert back/to 64 bit double values). I can't say if this is your case but you may give it a chance (let me also use using static Math; I think it helps clarity when I have to work with math formulas):

private static double GausianFunction(double x1, double x, double sigma2)
    => (1 / Sqrt(2 * PI * sigma2)) * Exp(Pow(x1 - x, 2) / (-2 * sigma2));

Variance() function is actually unused then I'll go quickly through it but: if possible use double[] instead of IEnumerable<double> and avoid LINQ when you can write an ad-hoc function, relative speed-up is huge in this case (of course unless actual performance are enough for your task.)


Merge two sorted arrays in NASM I have the following function which I wrote in NASM to merge two sorted arrays. Its not a standalone, but its supposed to be compiled into a .o file and then linked to a .c file from which the function can be called with the following signature:

void merge(int* arr1, int size1, int* arr2, int size2, int* merged_array)

I have tested it and my code works as intended. However, I am somewhat new to assembly and I don't know if I'm following best practices with this code. Also, I timed it against a function that I wrote in C to do the same thing, and the C function consistently outperforms it for large arrays. So I'm wondering if there's any obvious sections of it I can rewrite to improve performance.

global merge

merge: ; entry point for function merge
    push ebp
    mov ebp, esp

    mov eax,[esp+8] ; pointer to array 1
    mov esi,[esp+12] ; length of array 1

    mov ebx,[esp+16] ; pointer to array 2
    mov edi,[esp+20] ; length of array 2

    mov edx,[esp+24] ; pointer to destination array (pre-allocated)

begin_merge:
    test esi,esi ; if we've run out of array 1
    jz copy2 ; just copy over array 2

    test edi,edi ; if we've run out of array 2
    jz copy1 ; just copy over array 1

    ;otherwise, compare the first elements
    mov ecx,[eax]
    cmp ecx,[ebx]
    jl merge_from_1 ; if eax > ebx, merge from array 1
    jmp merge_from_2; otherwise keep going for the merge from array 2

; merge a single element from an array
merge_from_2:
    mov ecx,[ebx]
    mov [edx],ecx
    add edx,4
    add ebx,4
    dec edi
    jmp begin_merge
merge_from_1:
    mov ecx,[eax]
    mov [edx],ecx
    add edx,4
    add eax,4
    dec esi
    jmp begin_merge

; copy over an entire array
copy1:
    test esi,esi ; if we've run out of array 1
    jz finish ; end
    ; otherwise, copy an element from array 1
    jmp merge_from_1 ; this will return to begin_merge, but thats okay
    ; because begin_merge will just forward it on to here again
copy2:
    test edi,edi ; if we've run out of array 2
    jz finish ; end
    ; otherwise, copy an element from array 2
    jmp merge_from_2 ; this will return to begin_merge, but thats okay
    ; because begin_merge will just forward it on to here again

finish:
    leave
    ret

While OP seemed to find the comments useful, we're supposed to put this stuff in answers.  Seems about as sensible as Jeopardy's "put your response in the form of a question."  Still, that's the rule, so:

Why are you doing jmp merge_from_2? Where do you think the execution is going to go if you don't do that jmp?
And in copy2, why have 2 jumps, instead of just jnz merge_from_2?
Which brings us to jz copy1. In copy1, you are going to check if esi is zero. But you just checked that before the jz.  If there no way to get to copy1 with esi zero, then checking for zero there is redundant. And if you remove the test/jz, that just leaves jmp merge_from_1. Why have a jump to a jump?

And one more:

When you do dec edi or dec esi, the dec sets the flags.  You can do the conditional jump right there.  Since you are only decrementing one or the other, jumping back to a place where you test both again seems redundant.  If you check the results of the decrement, you don't need to do this.  Maybe move the begin_merge label down so you only do the 2 tests on entry.


Error handling techniques for nested methods that can throw exceptions I have 2 different approaches I'm using to handle error state or Exceptions from within nested functions.  Take the following code for example:

public void Func1()
{
    try
    {
        Func2();
        //... continued logic if no exception was encountered
    }
    catch (Exception ex)
    {
        log(ex.Message);
        return;
    }

}

public void Func2()
{
    try
    {
        Func3();
    }
    catch (Exception ex)
    {
        log(ex.Message);
        throw;
    }
}

In the above sample, Func1 needs to know if Func2 (and other methods Func2 calls) have thrown an exception, so that it can continue with execution.  This appears to me to be an obvious code-smell and it's something I'd like to avoid doing.  Here's the next option I've tried:

public void Func1()
{
    try
    {
        bool success = Func2();
        if (!success)
        {
            //... continued logic if no exception was encountered
        }
    }
    catch (Exception ex)
    {
        log(ex.Message);
        return;
    }
}

public bool Func2()
{
    try
    {
        bool success = Func3();
        if (!success) return false;
        return true;
    }
    catch (Exception ex)
    {
        log(ex.Message);
        return false;
    }
}

Using this second method, the nested functions return a boolean to determine state and whether or not the calling function should continue.  This also doesn't feel right to me.  I'd really like suggestions on ways to handle situations like this where I'm calling nested functions (particularly when they return void because they do unrelated processing to the class).  Thanks!

EDIT: updated from pseudo-code to valid C#.

If shortly, using exceptions is not a code-smell.

In my opinion using bool return values is appropriate for methods that are purposed for checking some conditions. Examples:

bool CanFunc2BeCalled() {  ... }
bool TryParse(string s, out ... value) { ... }

Such methods specifically designed to return bool. It is the main job they do.

If a method should perform some actions and during its execution an exception can be occured I would not return bool to indicate a failure. An exception is the right choice for these cases.

Exceptions is a native conception of .NET so don't be afraid of them :) Also with good XML comment on methods you always know what exceptions the method can throw:

///<summary>
///...
///</summary>
///<exception cref="InvalidOperationException">Description of failure.</exception>
void Func2() { ... }

So if you are able to check that call of a method can cause an exception before its execution, create the method for checking some conditions and call it before:

private bool CanFunc2BeCalled() {  ... }

public void Func1()
{
    if (!CanFunc2BeCalled())
        return;

    Func2();
}

But if you can't check if the Func2 can be safely called before its execution then dealing with exceptions is a good way and I would stick with it.


Validation form JavaScript I am a beginner to JavaScript and I know about jQuery Validations however I do not want to use a foundation like jQuery just yet.

I am wondering if what I have done its good or if there is another way to do it other then jQuery?

I won't post the html just the JS. It does work by the way! Just wondering if there is a better way!

function validateForm() {
  var userName = document.forms["register"]["username"].value;
  var passWord = document.forms["register"]["password"].value;
  var c_passWord = document.forms["register"]["c_password"].value;

  if (userName == null || userName == "") {
    alert("Your username can not be empty!");
    return false;
  } else if (userName.length < 3) {
    alert("Your username must be atleast 3 characters long!");
    return false;
  }

  if (passWord == null || passWord == "") {
    alert("Your password can not be empty");
    return false;
  } else if (passWord.length < 5) {
    alert("Your password must be more the 5 characters");
    return false;
  }
}

It mostly looks fine. Some things I would change:

var userName = document.forms["register"]["username"].value;
var passWord = document.forms["register"]["password"].value;

I would extract the repetition here to get the form. This makes the code cleaner and makes it simpler if you change the name of the form.

var form     = document.forms["register"];
var userName = form["username"].value;
var passWord = form["password"].value;

Here

if (userName == null || userName == "") {

You could just use

if (!userName == null) {

One last thing is that you probably want to check and remove leading and trailing spaces in the user name (and possibly in the password). There is a built in trim method you can use if you're not targeting IE8.


How to take the some elements of a list of random numbers and sort them? I want to create a file consisting take rows of distinct numbers in ascending order. They are randomly taken from the first total integer. The file will be used to make an excerpt as discussed here.

I realize that my naming convention for identifiers below is not good but please ignore it for the sake of simplicity.

using System;
using System.IO;
using System.Linq;

namespace Karls.Students.Excerpting
{
    class Program
    {
        static void Main(string[] args)
        {
            int total = int.Parse(args[0]);
            int take = int.Parse(args[1]);
            int seeder = int.Parse(args[2]);
            string filename = args[3];

            int[] array = Enumerable.Range(1, total).ToArray();

            Random random = new Random(seeder);
            for (int i = total - 1; i > 0; i--)
            {
                int j = random.Next(i+1);
                int temp = array[i];
                array[i] = array[j];
                array[j] = temp;
            }

            File.WriteAllLines(filename, array.Take(take).OrderBy(x => x).Select(x => x.ToString()));
        }
    }
}

With best practice in mind, is there anything "bad" in my code?

Instead of shuffling items yourselves it's easier to apply random sort. So your logic can be rewritten in a single statement:

var random = new Random();

var result = Enumerable.Range(1, total)
    .OrderBy(i => random.Next())
    .Take(take)
    .OrderBy(i => i);


Simple Binary Search Tree At the moment I am learning algorithms and here I am trying to implement a simple binary search tree. I would like to know your suggestions on whether I am on the right track or not, and how this can be improved.

BinaryTree.cs

class BinaryTree
{
    private Node root;
    private int count;

    public BinaryTree()
    {
        root = null;
        count = 0;
    }
    public bool isEmpty()
    {
        return root == null;
    }

    public void insert(int d)
    {
        if (isEmpty())
        {
            root = new Node(d);
        }
        else
        {
            root.insertData(ref root, d);
        }

        count++;
    }

    public bool search(int s)
    {
        return root.search(root, s);
    }

    public bool isLeaf()
    {
        if(!isEmpty())
        return root.isLeaf(ref root);

        return true;
    }

    public void display()
    {
        if(!isEmpty())
        root.display(root);
    }

    public int Count()
    {
        return count;
    }
}

Node.cs

class Node
{
    private int number;
    public Node rightLeaf;
    public Node leftLeaf;

    public Node(int value)
    {
        number = value;
        rightLeaf = null;
        leftLeaf = null;
    }

    public bool isLeaf(ref Node node)
    {
        return (node.rightLeaf == null && node.leftLeaf == null);

    }

    public void insertData(ref Node node, int data)
    {
        if (node == null)
        {
            node = new Node(data);

        }
        else if (node.number < data)
        {
            insertData(ref node.rightLeaf, data);
        }

        else if (node.number > data)
        {
            insertData(ref node.leftLeaf, data);
        }
    }

    public bool search(Node node, int s)
    {
        if (node == null)
            return false;

        if (node.number == s)
        {
            return true;
        }
        else if (node.number < s)
        {
           return search(node.rightLeaf, s);
        }
        else if (node.number > s)
        {
            return search(node.leftLeaf, s);
        }

        return false;
    }

    public void display(Node n)
    {
        if (n == null)
            return;

        display(n.leftLeaf);
        Console.Write(" " + n.number);
        display(n.rightLeaf);
    }

}

Program.cs

class Program
{
    static void Main(string[] args)
    {
        BinaryTree b = new BinaryTree();

        b.insert(1);
        b.insert(6);
        b.insert(2);
        b.insert(4);
        b.insert(5);
        b.insert(3);

        b.display();

        Console.ReadLine();
    }
}

Output

  1 2 3 4 5 6

I don't quite understand why you'd use a ref here.

public void insertData(ref Node node, int data)
{
    if (node == null)
    {
        node = new Node(data);

    }
    else if (node.number < data)
    {
        insertData(ref node.rightLeaf, data);
    }

    else if (node.number > data)
    {
        insertData(ref node.leftLeaf, data);
    }
}

Why not just return the node?

public Node insertData(Node node, int data)
{
    if (node == null)
    {
        return new Node(data);

    }

    if (node.number < data)
    {
        return insertData(node.rightLeaf, data);
    }

    if (node.number > data)
    {
        return insertData(node.leftLeaf, data);
    }
}

Your capitalization is consistent, but slightly confusing and non-standard. For example, both your private fields and public methods are camelCase.

   public void insert(int d)
    {
        if (isEmpty())
        {
            root = new Node(d);
        }

This would be perfectly fine if you distinguished the field by using the this keyword or an underscore to make it look different from your methods.

    public void insert(int d)
    {
        if (isEmpty())
        {
            _root = new Node(d);
        }

Or

    public void insert(int d)
    {
        if (isEmpty())
        {
            this.root = new Node(d);
        }

But like I said, it's non-standard. The standard is to use PascalCase for methods, camelCase for local vars, and _underscoredCamelCase for private fields.

    public void Insert(int d)
    {
        if (IsEmpty())
        {
            _root = new Node(d);
        }


Ping Pong Pi - A Ping Pong score and serving manager I have spent a few hours on this code, but think it could be improved a bit. It requires eSpeak to be installed to run the speech synthesis, but the voice can be toggled on or off using the togglevoice command. This was created for Python 2.7.3.

My major concerns are the fluctuations of the use of global to define global variables, the re-use of the variable n in functions and readability of the d array. It currently works fine on a Raspberry Pi running the latest version of Raspbian.

import os
def talk(n):
    if voice_mode == 1:
        os.system("espeak -ven+m3 '" + n + "' 2>/dev/null")
    else:
        print n

def print_score(n):
    if n == 0:
        other_n = 1
    else:
        other_n = 0
    if d[n][1] == 0:
        score1 = "love"
    else:
        score1 = str(d[n][1])
    if d[other_n][1] == 0:
        score2 = "love"
    else:
        score2 = str(d[other_n][1])
    talk(score1 + " " + score2)

def win(n):
    global first_server
    talk(d[n][0] + " wins the game")
    d[n][2] += 1
    d[0][1] = 0
    d[1][1] = 0
    deuce_mode = 0
    serve_turns = old_serve_turns
    if first_server == 0:
        first_server = 1
        current_server = 1
    else:
        first_server = 0
        current_server = 0
    if d[n][2] == match_points:
        talk(d[n][0] + " wins the match")
        d[0][2] = 0
        d[1][2] = 0

def add_score(n):
    global deuce_mode
    global current_server
    global serve_turns
    d[n][1] += 1
    if n == 0:
        other_n = 1
    else:
        other_n = 0
    if deuce_mode == 0:
        if d[0][1] == d[1][1] and d[0][1] == game_points - 1:
            deuce_mode = 1
            old_serve_turns = serve_turns
            serve_turns = 1
        if current_server == 0:
            print_score(0)
        else:
            print_score(1)
        if d[n][1] == game_points:
            win(n)
    else:
        if d[n][1] == d[other_n][1] + 2:
            if current_server == 0:
                print_score(0)
            else:
                print_score(1)
            win(n)
        else:
            if current_server == 0:
                print_score(0)
            else:
                print_score(1)
    if (d[0][1] + d[1][1]) % serve_turns == 0:
        if current_server == 0:
            current_server = 1
            talk(d[1][0] + " is serving next")
        else:
            current_server = 0
            talk(d[0][0] + " is serving next")

d = []
for i in xrange(2):
    d.append(["", 0, 0])
d[0][0] = "Player 1"
d[1][0] = "Player 2"
game_points = 11
match_points = 2
deuce_mode = 0
voice_mode = 1
serve_turns = 2
old_serve_turns = 2
current_server = 2
first_server = 0

talk("Ready to play")
while True:
    c = raw_input("Command: ").lower()
    if c == "z":
        if current_server != 2:
            add_score(0)
        else:
            talk(d[0][0] + " is serving first")
            current_server = 0
            first_server = 0
    elif c == "x":
        if current_server != 2:
            add_score(1)
        else:
            talk(d[1][0] + " is serving first")
            current_server = 1
            first_server = 1
    elif c == "1name":
        d[0][0] = raw_input("Player 1 name: ")
        talk("Welcome, " + d[0][0])
    elif c == "2name":
        d[1][0] = raw_input("Player 2 name: ")
        talk("Welcome, " + d[1][0])
    elif c == "gm":
        game_points = int(raw_input("Game points: "))
        match_points = int(raw_input("Match points: "))
    elif c == "s":
        serve_turns = int(raw_input("Serve turns: "))
    elif c == "togglevoice":
        if voice_mode == 0:
            voice_mode = 1
        else:
            voice_mode = 0
    elif c == "quit":
        quit()
    else:
        talk("Command not recognised")

Line by line analysis

import os

You need 2 blank lines before each global function definition.

def talk(n):

n is not descriptive and associated with a number. Don't use one letter variables unless it's really obvious (for example i loop counter, k and v as dict key and a value, or d as Twisted's deferred object. Something like phrase would be better.

    if voice_mode == 1:

Where does the voice_mode come from? Global variables are bad, pas it as a parameter to a talk method. Also, canonical way of checking if boolean is True is simply if voice_mode.

        os.system("espeak -ven+m3 '" + n + "' 2>/dev/null")
    else:
        print n

def print_score(n):

Again, n does not tell reader a lot.

    if n == 0:
        other_n = 1

What is other_n? If there's no better name for it - add a comment.

    else:
        other_n = 0
    if d[n][1] == 0:

Where did d come from? Pass it to a function. Also, better name.

        score1 = "love"
    else:
        score1 = str(d[n][1])
    if d[other_n][1] == 0:
        score2 = "love"
    else:
        score2 = str(d[other_n][1])
    talk(score1 + " " + score2)

def win(n):

Same thing with the n variable. Also, docstring might be helpful for this method.

    global first_server

Global variables are bad, really bad. Make your method take and return this if you need the variable.

    talk(d[n][0] + " wins the game")

This d is really quite obscure.

    d[n][2] += 1
    d[0][1] = 0
    d[1][1] = 0
    deuce_mode = 0
    serve_turns = old_serve_turns
    if first_server == 0:
        first_server = 1
        current_server = 1
    else:
        first_server = 0
        current_server = 0
    if d[n][2] == match_points:
        talk(d[n][0] + " wins the match")
        d[0][2] = 0
        d[1][2] = 0

def add_score(n):

Same thing: add docstring, replace n with better name and pass all the global variables as arguments to the function.

    global deuce_mode
    global current_server
    global serve_turns
    d[n][1] += 1
    if n == 0:
        other_n = 1
    else:
        other_n = 0
    if deuce_mode == 0:
        if d[0][1] == d[1][1] and d[0][1] == game_points - 1:
            deuce_mode = 1
            old_serve_turns = serve_turns

I don't see old_serve_turns used anywhere.

            serve_turns = 1
        if current_server == 0:
            print_score(0)
        else:
            print_score(1)
        if d[n][1] == game_points:
            win(n)
    else:
        if d[n][1] == d[other_n][1] + 2:
            if current_server == 0:
                print_score(0)
            else:
                print_score(1)
            win(n)
        else:
            if current_server == 0:
                print_score(0)
            else:
                print_score(1)
    if (d[0][1] + d[1][1]) % serve_turns == 0:
        if current_server == 0:
            current_server = 1
            talk(d[1][0] + " is serving next")
        else:
            current_server = 0
            talk(d[0][0] + " is serving next")

d = []

You need either a better data structure, or at least a good comment explained what the d is. Also, d needs a better name.

for i in xrange(2):
    d.append(["", 0, 0])
d[0][0] = "Player 1"
d[1][0] = "Player 2"
game_points = 11
match_points = 2
deuce_mode = 0
voice_mode = 1
serve_turns = 2
old_serve_turns = 2
current_server = 2

You only play with server 0 and 1, correct? Why do you need to obscure and add server 2 which is only there to start the game? Better have a flag of some sort.

first_server = 0

talk("Ready to play")
while True:
    c = raw_input("Command: ").lower()

Better name: command.

    if c == "z":
        if current_server != 2:
            add_score(0)
        else:
            talk(d[0][0] + " is serving first")
            current_server = 0
            first_server = 0
    elif c == "x":
        if current_server != 2:
            add_score(1)
        else:
            talk(d[1][0] + " is serving first")
            current_server = 1
            first_server = 1
    elif c == "1name":
        d[0][0] = raw_input("Player 1 name: ")
        talk("Welcome, " + d[0][0])
    elif c == "2name":
        d[1][0] = raw_input("Player 2 name: ")
        talk("Welcome, " + d[1][0])
    elif c == "gm":
        game_points = int(raw_input("Game points: "))
        match_points = int(raw_input("Match points: "))
    elif c == "s":
        serve_turns = int(raw_input("Serve turns: "))
    elif c == "togglevoice":
        if voice_mode == 0:
            voice_mode = 1
        else:
            voice_mode = 0
    elif c == "quit":
        quit()
    else:
        talk("Command not recognised")

Even though this if/else tree is acceptable in your scope, you might want to look into Strategy pattern. This is further reading though.

Summary

Overall, your variable naming needs to be more readable. Don't use global variables, just pass the ones you need as arguments. For example, your talk function could be changed like this:

def talk(phrase, voice_mode=True):
    """Pronounce the phrase if the voice mode is on, otherwise print."""
    if voice_mode:
        os.system("espeak -ven+m3 '%s' 2>/dev/null" % phrase)
    else:
        print phrase

Don't forget to specify docstrings for non-trivial methods. Make them long if you need to. Use comments extensively to elaborate if variable name or a data structure is not descriptive enough.


Manipulate original DOM in React.js As an introduction, I would like to have my inputs validated with the built-in validation HTML5 attributes, e.g. use pattern for text input. So this is my sample code that implements that.

Is it considered best practice while handling event, to access event.target, which is the original DOM, and do something with it, rather than dealing with React virtual DOM?

Please let me know if there are more improvements can be made. I'm using React.js 0.14.7. Thanks.

NodeList.prototype.forEach = Array.prototype.forEach;

class InputSet extends React.Component {
    render() {
        return (
            <div>
                <input type="text" defaultValue={this.props.firstName} data-field="firstName" required/>
                <input type="text" defaultValue={this.props.lastName} data-field="lastName" required/>
            </div>
        );
    }
}

class FormWithValidation extends React.Component {
    handleSubmit(event) {
        event.preventDefault();
        console.log(event.target);

        let childNodes = Array.prototype.slice.call(event.target.childNodes);
        childNodes = childNodes.filter((elm) => elm.tagName === 'DIV');
        const values = childNodes.map((div) => {
            const value = {};
            div.childNodes.forEach((input) => {
                const field = input.getAttribute("data-field");
                value[field] = input.value
            })
            return value;
        });

        // Do something with values.
        console.log(values);
    }

    render() {
        const inputSets = [
            {firstName: "Donald", lastName: "Duck"},
            {firstName: "Bill", lastName: "Williams"}
        ]

        return (
            <form action="" onSubmit={this.handleSubmit} >
                {inputSets.map((obj, index) => {
                    return <InputSet {...obj} key={index} />
                })}
                <input type="submit" value="Save"/>
            </form>
        );
  }
}

ReactDOM.render(<FormWithValidation/>, document.getElementById('container'))

It is fine to read event.target in react. In many cases (e.g. finding out which item was clicked) it's the only way to get the data your code needs.

It is generally considered best practice to not change DOM directly, but let react handle have monopoly on DOM changes -> You're good here, you do not change DOM, you only read from it.

Two things that could be improved:

You store the field name in data-field, and later on read it to find out which div is which field. This is not so clean react code.
You read the entire form from event.target, filtering out all divs.

It may work, and it is efficient: react is only triggered after submit is clicked.
But the code is very dependent on the structure of your form in the real DOM. If there is one div to many in your real DOM, then your code breaks down. And to debug, one also needs to understand the tricky real DOM traversal methods (e.g. that a DOM node is not the same as a DOM element).

I would advise to refactor to a react-only solution, making the following changes:

Give your FormWithValidation component state, which stores an array of inputset values (firstname and lastname)
Pass down a this.valueUpdate method, which individual fields can invoke to update state. The method would receive an index (of the input set), a fieldname, and a new value.
Make a single component for a single input field, which can receive the fieldname as a prop.
Give the input field an onBlur handler. Which calls this.props.valueUpdate.

This refactoring has two disadvantages:

it is a lot more code for these simple tasks
react will do a lot more cycles: every time an input field has changed, react will update state and re-render the form (efficiently and only diffs of course)

The big advantage is that all your code and all your logic lives inside the react structure and lifecycle. Which will make your code easier to maintain.


Extremely basic authentication class I'm in the process of building a small application and obviously I need some sort of way to authenticate users. I'm not sure if I am way off or if this is close to what I should be thinking. Please let me know what you think...

<?php
class Authentication_controller {
    private __constructor() {
        include(Authentication_model.php);

        $auth_model = new Authentication_Model();
        $auth_model->database_connect();
    }

    public login($username, $password) {
        if($auth_model->username($username)) {
            //Username exists, now check password
            if($auth_model->password($username, $password)) {
                //User is OK to login, send to designated page
                include(Page_controller.php);

                $page_controller = new Page_controller();
                $page_controller->go('home');

                //Create session
                session_start();

                //Set session variables
                $_SESSION['logged_in'] = true;
                $_SESSION['id'] = $auth_model->id($username);
                $_SESSION['access_level'] = $auth_model->access_level($username);
            } else {
                //Password didn't match
                return $error = "Username or password incorrect."
            }
        } else {
            //Username didn't match
            return $error = "Username or password incorrect."
        }
    }

    public permission($task_access_level) {
        $user_access_level = $_SESSION['access_level'];

        if($user_access_level >= $task_access_level) {
            //User has access level that is high enough for task
            return true;
        } else {
            //User does not have access level high enough for task
            return false;
        }
    }

    public logout() {
        session_destroy();
    }
}

Is this typically how a class like this should work? I'm still very new to this stuff.

jsanc623 had some good points, but I think he missed a lot too.  In particular, I think you have a fairly major design problem.  Without seeing all of the related classes, my comments are a bit limited, but there's a few things below.

There's a few different 'concerns' your code is handling here:

Authenticating (is the username/password pair x, y valid?)
Persisting an identity (remembering that the user is logged in under username x)
Checking permissions

These concerns should be separated.

The logic of "is [x, y] a valid authentication pair?" and the logic of "can this person do z?" have no need to be coupled.

Additionally, there's no reason to hard code all of the session stuff.  (The class should definitely not be responsible for starting the session.)

Based on the API seen, I have a suspicion that your controller and model classes have convoluted functionality and APIs.

You might want to consider looking into how a few different frameworks have handled authentication.  Zend Framework is the only one I have experience with the authentication components.  It has a few quirks, but overall, it has a good API.


Wrapping a CSV file for access and testing I have a simple class that represents and accesses the data from a CSV file (using the CsvHelper library). I've tried restructuring the code to allow for better unit testing, but I'm sure the structure of the class could be better; both for testing and for using it (I'm not sure if there is any standard recommendations for these types of classes).

Here is the class code:

public class CsvCountFile
{
    public string AbsolutePath { get; }
    public string Delimiter { get; }

    public LocationDefinition LocationData { get; private set; }
    public List<CountDefinition> CountData { get; private set; }

    public CsvCountFile(string absolutePath, string delimiter = ",")
    {
        AbsolutePath = absolutePath;
        Delimiter = delimiter;
    }

    public void ReadCountData()
    {
        using (var fileReader = File.OpenText(AbsolutePath))
        {
            ReadCountData(fileReader);
        }
    }

    public void ReadCountData(TextReader fileReader)
    {
        CountData = new List<CountDefinition>();

        using (var csvReader = new CsvReader(fileReader))
        {
            csvReader.Configuration.HasHeaderRecord = false;
            csvReader.Configuration.RegisterClassMap<LocationMap>();
            csvReader.Configuration.RegisterClassMap<CountMap>();

            csvReader.Read(); // get header
            csvReader.Read(); // get first record
            LocationData = csvReader.GetRecord<LocationDefinition>();

            csvReader.Read(); // skip blank line
            csvReader.Read(); // skip second header section

            while (csvReader.Read())
            {
                var count = csvReader.GetRecord<CountDefinition>();
                CountData.Add(count);
            }
        }

    }
}

The ReadCountData was split into two so that the reading logic could be tested by forming a TextReader in memory, instead of needing an actual csv file. While this code works, I don't foresee any usage scenarios that would typically require reading the csv by passing in a TextReader.

Also, due to this structure, when CsvCountFile is constructed, the csv absolutePath is passed in, but testing using a TextReader doesn't require this, and it doesn't feel right instantiating it with an empty string.

Just passing by. There are few comments I want to make here.

ReadCountData() is definitely out of place because the first method just exposes you to a StreamReader object reference. This method can not be unit-tested. I created an interface called IFile that wraps the static OpenText() method

public  interface IFile
{
    StreamReader OpenText(string path);
}

  I created a FileClass that uses this interface

public  class FileClass
{
    private readonly IFile file;

    public FileClass(IFile file)
    {
        this.file = file;
    }

    // I renamed your ReadCountData as OpenFile
    public void OpenFile(string path)
    {
        try
        {
            var fileReader = file.OpenText(path);
        }
        catch (Exception e) {
            // do whatever;
        }
    }
}

Using this approach, you can unit-test the OpenText() using the below code

[TestClass]
public class UnitTest1
{
    [TestMethod]
    public void OpenFile_PathAsAParameter_CSVFileOpened()
    {

        //Arrange
        string path = @"C:\myfilerocks.txt";
        var fileMock = new Moq.Mock<IFile>();
        var fileClass = new FileClass(fileMock.Object);

        //Act
        fileClass.OpenFile(path);

        //Assert
        fileMock.Verify(x=> x.OpenText(path));
    }
}

Notice there is still a dependency on StreamReader and TextReader, it's best to create a wrapper for those classes. When you have some many static class in a testable method I see it more as a smell . When a wrapper can't be created for them keep them as dependency e.g  public void ReadCountData(TextReader fileReader) seems alright to me. If you are looking for wrappers for the file system, visit System.IO.Abstractions by Jonathan Channon. His blog also explains a great deal Abstracting the File System

So the above test that the OpenText() was called.
Rather than using csvReader, I will use TextFieldParser from Microsoft.VisualBasic.FileIO.TextFieldParser . It works for both C# and VB. Note this example was extracted from Reading CSV Files using C#

using (TextFieldParser parser = new TextFieldParser(@"c:\temp\test.csv"))
{
    parser.TextFieldType = FieldType.Delimited;
    parser.SetDelimiters(",");
    while (!parser.EndOfData)
    {
        //Processing row
        string[] fields = parser.ReadFields();
        foreach (string field in fields)
        {
           //TODO: Process field
        }
    }
}

Note: AbsolutePath, Delimiter is redundant here.
Refrain from names like ReadCountData, they don't provide information about their functionality. I would've thought ReadDataCount sounds better.


Generating permutation of indices from seed ID In my OpenCL kernel, I have the following logic:

size_t id = get_global_id(0);
uchar index0 = (id / 32760) % 16;
uchar index1 = (id / 2184) % 15;
uchar index2 = (id / 156) % 14;
uchar index3 = (id / 12) % 13;
uchar index4 = (id / 1) % 12;

if (index1 >= index0)
    ++index1;

if(index2 >= index0 ||
   index2 >= index1)
    ++index2;
if(index2 >= index1 && index2 >= index0)
    ++index2;

if(index3 >= index0 ||
   index3 >= index1 ||
   index3 >= index2)
    ++index3;
if(index3 >= index0 && index3 >= index1 ||
   index3 >= index1 && index3 >= index2 ||
   index3 >= index2 && index3 >= index0)
    ++index3;
if(index3 >= index0 && index3 >= index1 && index3 >= index2)
    ++index3;

if(index4 >= index0 ||
   index4 >= index1 ||
   index4 >= index2 ||
   index4 >= index3)
    ++index4;
if(index4 >= index0 && index4 >= index1 ||
   index4 >= index0 && index4 >= index2 ||
   index4 >= index0 && index4 >= index3 ||
   index4 >= index1 && index4 >= index2 ||
   index4 >= index1 && index4 >= index3 ||
   index4 >= index2 && index4 >= index3)
    ++index4;
if(index4 >= index0 && index4 >= index1 && index4 >= index2 ||
   index4 >= index1 && index4 >= index2 && index4 >= index3 ||
   index4 >= index2 && index4 >= index3 && index4 >= index0 ||
   index4 >= index3 && index4 >= index0 && index4 >= index1)
    ++index4;
if(index4 >= index0 && index4 >= index1 && index4 >= index2 && index4 >= index3)
    ++index4;

Now, in my testing, I've verified that this logic is correct and applies correct behavior to my variables. However, it is extremely cumbersome to read and maintain, and I'm looking for a way to optimize it such that it's much easier to read and understand. How can I rewrite this logic so that it's much easier to read, and possibly faster to execute?

The intent of this code is to take an input integer in the range [0, 16*15*14*13*12), and convert it into a permutation of 5 unique indices ranged [0,16). Like I said, this code works, but it's very confusing to read.

There are a few restrictions:

No Recursion. I'm aware that most OpenCL compilers will allow limited degrees of recursion by in-lining away recursive function calls, but I want this code to be as portable as possible, and that means adhering to the specifications set by the Official OpenCL spec.
Readability is more important than speed. Obviously, getting the algorithm faster would be ideal, but what I really care about is making this code less cumbersome to maintain, particularly if I expand it to add more variables to this algorithm.
I have limited access to OOP related functionality. This is OpenCL, so anything object-oriented needs to be compatible with OpenCL 2.0.

Use your words

Let's start with what you're actually accomplishing in all those if statements. It looks like what you're doing is incrementing each index by the number of smaller indices, for each possible such count.

That translates itself naturally to using arrays:

uchar index[5] = {(id / 32760) % 16,
                  (id / 2184) % 15,
                  (id / 156) % 14,
                  (id / 12) % 13,
                  (id / 1) % 12};

And writing that idea as a loop:

// for each index
for (int i=1; i<5; ++i) {
    // for each possible count of smaller indices
    for (int cnt=1; cnt<=i; ++cnt) {
       // count the number of smaller indices
       int k=0;
       for (int j=0; j<i; ++j) {
           k += (index[i] >= index[j]);
       }

       // and increment if necessary
       if (k >= cnt) {
           ++index[i];
       }
    }
}

This can be translated into the sort-of-more/sort-of-less readable (YMMV) C++11 version:

for (int& i : index) {
    for (int cnt=1; cnt <= std::distance(index, &i); ++cnt) {
        i += std::count_if(index, &i, [i](int j){return i >= j; }) >= cnt;
    }
}


nginx serve static - images, js, css and proxy node.js server I'm building a server for serving static content, and uploading images there.

Everything works, but I'm not expert in nginx and server configuration, so maybe someone sees some improvements for this.

Here is my nginx config:

server {
    listen 80;

    server_name localhost;

    location / {
        proxy_pass http://localhost:8080;
        proxy_http_version 1.1;
        proxy_set_header Upgrade $http_upgrade;
        proxy_set_header Connection 'upgrade';
        proxy_set_header Host $host;
        proxy_cache_bypass $http_upgrade;
    }

    location ~ \.(gif|jpg|png)$ {
        gzip_static off;
        root /var/www/assets;
    }

    location ~ \.(css|js)$ {
        gzip_static on;
        root /var/www/assets;
    }
}

Users will upload and save images on this server, and I added node.js server for that with 1 view: app.get('/upload', function (req, res) {...});

That's why I'm proxying server here:

location / { proxy_pass http://localhost:8080; ....

location ~ \.(gif|jpg|png)$ {`

I will serve images on this server
location ~ \.(css|js)$ {

I will serve js & css already gzipped on this server

This makes sense to me. You can add a few more directives, though.

In-memory caching of file metadata

Nginx has an in-memory cache for file metadata, which can improve performance. Here's a sample config:

open_file_cache max=10000 inactive=120s;
open_file_cache_valid 60s;
open_file_cache_min_uses 2;
open_file_cache_errors on;

Sendfile

Nginx supports the syscall sendfile, which, as I understand, can directly spit bytes on disk into the network, without intermediary reading into ram, which is obviously faster. Here's an article about sendfile and co, and a basic config:

sendfile on;
tcp_nodelay on;
tcp_nopush on;

events

Outside of the server {} block, there are a few settings you can configure.

epoll on linux; if you're on a different OS consult the manual
multi_accept is generally recommended, and causes the worker process to accept as many connections as possible.

Here's a config:

worker_processes auto;

events {
  use epoll;
  multi_accept on;
}

I'd recommend looking further into things, for example by searching "nginx static performance" or similar, there are many articles. These are what show up from the top of my head. I don't see anything wrong with your config as a base, though.


Mutualising multi-threaded calls to EJB from Servlet I have a Servlet that makes an EJB call to a backing server which takes about a second to return some data that changes reasonably regularly.

I can't cache the data on the servlet-side, so I have decided to put the call to the EJB into a separate thread and use a Future to get the data as late as possible. There is no reason why an invocation of a servlet concurrent to an existing invocation can't use the data returned from the first request to the EJB.  For instance, if there is already a thread waiting for the data from the EJB, don't make another request; use the data from the first request.

public MyServlet extends HttpServlet {

  //this lock protects all access to currentRosterRequest
  private static final ReentrantReadWriteLock LOCK = new ReentrantReadWriteLock();

  private RosterEJB rosterEJB=...;

  //the executor service that will process the roster request
  private static final ExecutorService ROSTER_FETCH_EXECUTOR_SERVICE = Executors.newSingleThreadExecutor(new ThreadFactory() {
    @Override
    public Thread newThread(Runnable r) {
      return new Thread(r, "roster-fetch");
    }
  });

  //the roster request task - this never changes, it never needs parameters
  private static final Callable<ArrayList<String>> ROSTER_FETCH_TASK = new Callable<ArrayList<String>>() {

      @Override
      public ArrayList<String> call() throws Exception {
          try {
              return rosterEJB.getRoster();
          } finally {
              //one way or another the roster request has terminated
              LOCK.writeLock().lock();
              try {
                  //clear out the static variable
                  MyServlet.currentRosterRequest = null;
              } finally {
                  LOCK.writeLock().unlock();
              }
          }
      }
  };

  //the currently active request
  private static Future<ArrayList<String>> currentRosterRequest = null;

  public void doGet(...) {

    //obtain or create the current roster request
    final Future<ArrayList<String>> localRosterRequest = getLocalRosterRequest();

    //do other things

    //wait up to two seconds for data from the roster request
    final Boolean hasAccess = localRosterRequest.get(2, TimeUnit.SECONDS).contains(user.toLowerCase());

    //etc
  }

  private Future<ArrayList<String>> getLocalRosterRequest() {
    Future<ArrayList<String>> localRosterRequest;
    LOCK.readLock().lock();
    try {
        localRosterRequest = currentRosterRequest; //get a reference to the current request, if there is one
    } finally {
        LOCK.readLock().unlock();
    }
    if (localRosterRequest == null) { //if there wasn't a current request
        LOCK.writeLock().lock();
        try {
            if (currentRosterRequest == null) { //if there still isn't a current request, create a new one
                currentRosterRequest = ROSTER_FETCH_EXECUTOR_SERVICE.submit(ROSTER_FETCH_TASK);
            }
            localRosterRequest = currentRosterRequest; //get a reference to the current request
        } finally {
            LOCK.writeLock().unlock();
        }
    }
    return localRosterRequest; //return the reference to the current request
  }
}

(I have left the majority of the servlet out - the main thing I want reviewing is the method getLocalRosterRequest().)

My aims:

If there is a request already underway, don't create a new request, use the existing one.
If a new GET comes in when there is no request underway, create a new request, and make it available to subsequent callers.
Avoid deadlocks!

My questions:

Is this threadsafe? I can't see a way that threads could block.
Does it put the servlet container at risk? I'm using Tomcat 6
Is there a better way?

The concurrency looks complete to me in the sense that I cannot see any places it can fail, or block significantly. The static LOCK and Future make sense. The getLocalRosterRequest itself is technically correct in the sense that it returns the right results, without logic problems. There is no need for the double-check locking though. It saves nothing, and solves nothing, other than creating two separate lock points.

Additionally, methods should not return concrete-types when interface-types would be available. Your method (and Future should be of type List<String> and not ArrayList<String>

Your code pulls the writeLock() from the LOCK, and locks on that. There are only two places where you use LOCK (one in the call, the other in the get). Neither of them need a sub-lock of the LOCK, they can both operate on the LOCK itself.

Your code would be simpler, and slightly faster, as simply:

  private Future<List<String>> getLocalRosterRequest() {
        LOCK.lock();
        try {
            if (currentRosterRequest == null) { //if there isn't a current request, create a new one
                currentRosterRequest = ROSTER_FETCH_EXECUTOR_SERVICE.submit(ROSTER_FETCH_TASK);
            }
            return currentRosterRequest; //return a reference to the current request
        } finally {
            LOCK.unlock();
        }
  }

and then, in the call method, you would have:

      try {
          return rosterEJB.getRoster();
      } finally {
          //one way or another the roster request has terminated
          LOCK.lock();
          try {
              //clear out the static variable
              MyServlet.currentRosterRequest = null;
          } finally {
              LOCK.unlock();
          }
      }


Minimizing CSS media queries for the page title? I am working on page titles to be responsive. The code I have works and gets the job done, but I know that this is verbose. I decided upon the widths by trial and error based on how the words were stacking on each other

Desktop

Tablet

Mobile

This is the what I currently have as code

HTML

BruxZir&reg; Solid Zirconia Crowns &amp; Bridges

CSS

#page-title, #page-title-video {
  background: url(../img/top-banner.jpg) #273344 no-repeat right top;
  padding: 0.2em 0 0.2em 1em;
}

#page-title { margin: 12px 15px 24px 15px; }
#page-title-video { margin: 12px 0 24px 0; }

#page-title h1, #page-title-video h1 {
  color: #FFFFFF;
  letter-spacing: 0.08em;
}

@media screen and (min-width: 1px) and (max-width: 321px) {
  #page-title h1, #page-title-video h1 {
    font-size: 18px;
  }
}

@media screen and (min-width: 322px) and (max-width: 569px) {
  #page-title h1, #page-title-video h1 {
    font-size: 20px;
  }
}

@media screen and (min-width: 570px) and (max-width: 749px) {
  #page-title h1, #page-title-video h1 {
    font-size: 22px;
  }
}

@media screen and (min-width: 750px) and (max-width: 950px) {
  #page-title h1, #page-title-video h1 {
    font-size: 24px;
  }
}

@media screen and (min-width: 951px) {
  #page-title h1, #page-title-video h1 {
    font-size: 36px;
  }
}

If you have control over your html, give both headers the same class, for example .header-stretch.
For media queries - you actually can get rid of min-width. Yep, your code will be overriden for several times. But browser won't care rewriting the same parameter(font-size in this case - in any other you can do so) so it should be like this:

@media screen and (min-width: 951px) {
  .header-stretch {
    font-size: 36px;
  }
}
@media screen and (max-width: 950px) {
  .header-stretch {
    font-size: 24px;
  }
}
@media screen and (max-width: 749px) {
  .header-stretch {
    font-size: 22px;
  }
}
@media screen and (max-width: 569px) {
  .header-stretch {
    font-size: 20px;
  }
}
    @media screen and  (max-width: 321px) {
  .header-stretch {
    font-size: 18px;
  }
}