Angular Checkbox Filtering I'm very new to Angular, and trying to get my head around whether or not I'm doing it the Right Way (tm).

I want to filter a set of results by their properties, where if none of the filters are checked, all the results are displayed, but if one or more filters are checked, all results which match the checked properties are displayed.

I've setup a simple demo with the colours of fruit. The JSFiddle is available at http://jsfiddle.net/mattdwen/u5eBH/2/

The HTML has a series of checkboxs and a repeating list for the set results:

        <input type="checkbox" ng-click="includeColour('Red')"/> Red

        <input type="checkbox" ng-click="includeColour('Orange')"/> Orange

        <input type="checkbox" ng-click="includeColour('Yellow')"/> Yellow

                {{f.name}}

        Filter dump: {{colourIncludes}}

And the JS adds the checked properties to a scope array, and the filter checks if the fruit colour is in that array:

'use strict'

angular.module('fruit', []);

function FruitCtrl($scope) {
    $scope.fruit = [
        {'name': 'Apple', 'colour': 'Red'},
        {'name': 'Orange', 'colour': 'Orange'},
        {'name': 'Banana', 'colour': 'Yellow'}];

    $scope.colourIncludes = [];

    $scope.includeColour = function(colour) {
        var i = $.inArray(colour, $scope.colourIncludes);
        if (i > -1) {
            $scope.colourIncludes.splice(i, 1);
        } else {
            $scope.colourIncludes.push(colour);
        }
    }

    $scope.colourFilter = function(fruit) {
        if ($scope.colourIncludes.length > 0) {
            if ($.inArray(fruit.colour, $scope.colourIncludes) < 0)
                return;
        }

        return fruit;
    }
}

It doesn't really feel 'Angular' enough to be the correct way to achieve this?

You're attempting to handle the data binding yourself by creating arrays to store objects based on the click event. Angular does the two way binding for you.

Basically, I assign a new property named "included" by assigning it as the model to the input checkbox. Then I use that property on the filter of the fruit array to display only the one's whose colour is selected.

            <input type='checkbox' ng-model='f.included'></input>{{f.colour}}

{{f.name}}

angular.module('fruit', []);

function FruitCtrl($scope) {
    $scope.fruit = [
        {'name': 'Apple', 'colour': 'Red'},
        {'name': 'Orange', 'colour': 'Orange'},
        {'name': 'Banana', 'colour': 'Yellow'}
    ];
}


strstr() implementation In this strstr implementation, I am basically skipping the already matched and checked characters with an if else condition. Is this a right way of implementing it?

char *strstrImp(char *input, char *find) {
    if(*find=='\0')
        return input;
    if(*input=='\0')
        return NULL;

    char *start=input;
    char *p;
    while(*start!='\0')
    {
        p=start;
        char *q=find;
        while(*p!='\0' && *q!='\0' && *p==*q)
        {
            p++;
            q++;
        }

        if(*q=='\0')
        {
            return start;
        }
        //update the starting position based on whether any character matched
        if(p == start) //no match
            start++;
        else
            start = p; //skip characters and start from the previously left place
    }
    return NULL;
}

int main()
{
    char * res;
    res = strstrImp("helloworld","low");
    printf("%s",res);
}

Const-correctness

Since you tagged this question as c++, you must take care of const-correctness:

cr54045.cpp:36:18: warning: conversion from string literal to 'char *' is deprecated [-Wdeprecated-writable-strings]
 res = strstrImp("helloworld","low");
                 ^
cr54045.cpp:36:31: warning: conversion from string literal to 'char *' is deprecated [-Wdeprecated-writable-strings]
 res = strstrImp("helloworld","low");
                              ^

There should be a

const char strstrImp(const char *input, const char *find)
{
    return strstrImp(const_cast<char *>(input), find);
}

Incorrect optimization

Try this test case, which compares your implementation against the standard strstr().

int main()
{
    const char *input = "alfalfalfa romeo", *find = "alfalfa ";
    printf("%s\n", strstrImp(input, find));  // (null)
    printf("%s\n", strstr(input, find));     // alfalfa romeo
}

Logic simplification

Since the start = p optimization is bogus, we will only end the loop with start++.  The function body would then look like:

if (*find == '\0')
    return input;
if (*input == '\0')
    return NULL;

char *start = input;
while (*start != '\0')
{
    …
    start++;
}
return NULL;

Next, notice that the if (*input == '\0') return NULL special case is redundant.  Also notice that you never refer to input again, so you might as well think of start and input as the same thing.  The outline simplifies to:

if (*find == '\0')
    return input;
while (*input != '\0')
{
    …
    input++;
}
return NULL;

Next, let's turn our attention inside the outer loop.

Since p is only ever used inside the loop, you should pull its declaration inside the loop.  p and q should both be const char *.

while (*input != '\0')
{
    const char *p = input, *q = find;
    while (*p != 0 && *q != '\0' && *p == *q)
    {
        p++;
        q++;
    }
    if (*q=='\0')
    {
        return input;
    }
    input++;
}

It turns out that the inner-loop condition can be simplified, because the *p == '\0' test is redundant:

$$\begin{array}{ccc|c|c}
\text{*p == *q} & \text{*q != '\0'} & \text{*p != '\0'} & {\begin{array}{l}\text{*p!='\0' &&}\\\text{*q!='\0' &&}\\\text{*p==*q}\end{array}} & \begin{array}{l}\text{*q != '\0' &&}\\\text{*p == *q}\end{array} \\
\hline \\
\begin{array}{c}T\\T\end{array} & \begin{array}{c}T\\T\end{array} & \begin{array}{c}T\\F\end{array} & \begin{array}{c}T\\F\ (impossible)\end{array} &  T \\
\hline \\
\begin{array}{c}T\\T\end{array} & \begin{array}{c}F\\F\end{array} & \begin{array}{c}T\\F\end{array} & \begin{array}{c}F\ (impossible)\\F\end{array} &  F \\
\hline \\
\begin{array}{c}F\\F\end{array} & \begin{array}{c}T\\T\end{array} & \begin{array}{c}T\\F\end{array} & \begin{array}{c}F\\F\end{array} &  F \\
\hline \\
\begin{array}{c}F\\F\end{array} & \begin{array}{c}F\\F\end{array} & \begin{array}{c}T\\F\end{array} & \begin{array}{c}F\\F\end{array} &  F \\
\end{array}$$

The function can now look like:

if (*find=='\0')
    return input;

while (*input != '\0')
{
    const char *p, *q;
    for (p = input, q = find; *q != '\0' && *p == *q; p++, q++) {}
    if (*q == '\0')
    {
        return input;
    }
    input++;
}
return NULL;

A well designed function shouldn't have surprising behaviour, and therefore shouldn't have special cases.  Can we get rid of if (*find == '\0') return input;?

The behaviour of strstr() is, if find is "", then the function returns input.  In order to reach the return input; statement when find == "" and input == "", we have to get rid of both the if (*find == '\0') special case and the while (*input != '\0') guard condition.

char *strstrImp(char *input, const char *find)
{
    do
    {
        const char *p, *q;
        for (p = input, q = find; *q != '\0' && *p == *q; p++, q++) {}
        if (*q == '\0')
        {
            return input;
        }
    } while (*(input++) != '\0');
    return NULL;
}

const char *strstrImp(const char *input, const char *find)
{
    return strstrImp(const_cast<char *>(input), find);
}

Suppose we make that change — then what about the case where input == "" and find is non-empty?  It turns out, that works just fine: it will flow through all of the conditions and return NULL!  There you go — a compact implementation of strstr() with reasonably simple logic and without special cases.


Smallest prime factor of a large number (using BigInteger) I'm working on a Project Euler problem and am trying to get the smallest prime factor of a BigInteger:

private static BigInteger smallestPrimeFactor( BigInteger n ){
    for( BigInteger i = new BigInteger("2"); n.compareTo(i) >= 0 ; i = i.add(BigInteger.ONE) ){
        while( n.mod(i) == BigInteger.ZERO )
            return i;
        }
    }
    return BigInteger.ZERO;
}

I have basically just made adjustments to a code snippet I found online. They had something with n = n.divide(i), which I believe was to make things prime, but I wanted to save time just getting the smallest. Is there a better/faster way to go about doing this?

Factors to consider

If number 2 is not a factor, then all large even numbers will not be factors. Therefore, checking only the odd numbers is sufficient.
For integers i x j = n, if i is larger than the square root of n, then j has to be smaller than the square root of n (Therefore, already checked). Hence, numbers higher than the square root of n do not have to be checked as candidates for prime factors.

private static BigInteger smallestPrimeFactor(BigInteger n) {
    BigInteger two = new BigInteger("2");
    if (n.mod(two).compareTo(BigInteger.ZERO) == 0) {
        return two;
    }

    for (BigInteger i = new BigInteger("3"); n.compareTo(i.multiply(i)) >= 0; i = i.add(two)) {
        if (n.mod(i).compareTo(BigInteger.ZERO) == 0) {
            return i;
        }
    }
    return n;
}


Shortest way to form a string out of a subsequence of a string The task is taken from LeetCode (subscription required) -

From any string, we can form a subsequence of that string by deleting
some number of characters (possibly no deletions).
Given two strings source and target, return the minimum number of
subsequences of source such that their concatenation equals target. If
the task is impossible, return -1.
Example 1:
Input: source = "abc", target = "abcbc"
Output: 2
// Explanation: The target "abcbc" can be formed by "abc" and "bc", which are subsequences of source "abc".

Example 2:
Input: source = "abc", target = "acdbc"
Output: -1
// Explanation: The target string cannot be constructed from the subsequences of source string due to the character "d" in target
// string.

Example 3:
Input: source = "xyz", target = "xzyxz"
Output: 3
// Explanation: The target string can be constructed as follows "xz" + "y" + "xz".

Note:
Both the source and target strings consist of only lowercase English
letters from "a"-"z". The lengths of source and target string are
between 1 and 1000.

My solution has space complexity of \$O(n)\$ and time complexity of \$O(n^2)\$ (I think...)
/**
 * @param {string} source
 * @param {string} target
 * @return {number}
 */
var shortestWay = function(source, target) {
    const map = new Map();
    for (let i = 0; i < source.length; i++) {
        if (map.get(source[i])) {
            const arr = map.get(source[i]);
            arr.push(i);
            map.set(source[i], arr);
        } else {
            map.set(source[i], [i]);
        }
    }
    let occurrences = 0;
    for (let i = 0; i < target.length; i++) {
        const indexes = map.get(target[i]);
        if (indexes === void 0) return -1;
        occurrences++;
        let max = 0;
        indexes.forEach(index => {
            let j = 0;
            let ignore = 0;
            while(source[index + j + ignore] !== void 0) {
                if(target[i + j] !== source[index + j + ignore]) {
                    ignore++;
                } else {
                    max = Math.max(max, j++);
                }
            }
        });
        i += max;
    }
    return occurrences;
};

I believe your code is O(n^2*m), where n is the target and m is the source. Overall, though, I think there are a lot of readability improvements that could be made here by separating functions and renaming variables (j is a particularly bad contender).
// indexCharacters('hello')
// = {'h':[0],'e':[1],'l':[2,3],'o':[4]}
const indexCharacters = source => {
    const map = new Map();
    for (let i = 0; i < source.length; i++) {
        const char = source[i];
        if (!map.get(char)) map.set(char,[]);
        map.get(char).push(i);
    }
    return map;
}

// matchLength('abc','abeee') = 2
const matchLength = (source,target) => {
    let count = 0;
    for (let i=0;i<source.length;++i)
        if (target[count] === source[i])
            ++count;
    return count;
}

const shortestWay = (source, target) => {
    const all_character_indices = indexCharacters(source);
    let occurrences = 0;
    let targetIndex = 0;
    while (targetIndex < target.length) {
        const starting_indices = all_character_indices.get(target[targetIndex]);
        if (!starting_indices) return -1;

        const match_lengths = starting_indices.map(
            sourceIndex=>matchLength(
                source.substr(sourceIndex),
                target.substr(targetIndex)
            )
        );

        targetIndex += Math.max(...match_lengths);
        ++occurrences;
    }
    return occurrences;
};

console.log(shortestWay('abc','abcbc'))
console.log(shortestWay('abc','acdbc'))
console.log(shortestWay('xyz','xzyxz'))


Showing a tooltip on hover based on a tooltip attribute I'm showing a tooltip when you hover an element with the tooltip attribute. E.g.
Hover me.

I also have to set the position of the tooltip based on its width, and because I use width: max-content for the element, I have to do something like shown in the code example.

Is there any way I can make my code more efficient? I feel like I'm making it unnecessarily complicated.

(function() {
    var tooltip = document.querySelectorAll("[tooltip]");

    callback = (element, index, array) => {
        tooltip[index].onmouseenter = () => {
            let data = element.getAttribute("tooltip");
            let child = document.createElement("div");

            child.className = "tooltip";
            child.innerHTML = data;

            element.appendChild(child);

            child.style.left = "50%";
            child.style.marginLeft = - (child.offsetWidth / 2) + "px";

        }

        tooltip[index].onmouseleave = () => {
            if(element.getElementsByClassName("tooltip")[0] != undefined) {
                element.removeChild(element.getElementsByClassName("tooltip")[0]);
            }
        }
    }

    tooltip.forEach(callback);
})();
/* Don't review this CSS; it was added just for the demo. */
div[tooltip] { width: max-content; margin: auto; }

Hover me.

Don't do anything in JavaScript that can be done with just CSS!  The :hover pseudo-class is perfect for tooltips.  Use ::after to append a pseudo-element with the tooltip content, where the content is specified by the content property.

div[tooltip]:hover::after {
    content: attr(tooltip);
    display: block;
    position: absolute;
    transform: translateX(-50%);
}

/* Don't review this CSS; it was added just for the demo. */
div[tooltip] { width: max-content; margin: auto; }

Hover me.


Adjusting a landscape screen I got the solution for the question asked here, but I don't like how the code is written.

This part of the code rotates the screen if it's in landscape and sets the size to match the screen. I don't like the if and the 7x else if. Can you suggest something more readable? Something like (appOrientation - fromInterfaceOrientation) == 2 than angle (M_PI / 2.0)?

- (void)didRotateFromInterfaceOrientation:(UIInterfaceOrientation)fromInterfaceOrientation
{

    UIInterfaceOrientation appOrientation = [UIApplication sharedApplication].statusBarOrientation;
    float width = self.view.bounds.size.width;
    float height = self.view.bounds.size.height;
    //NSLog(@"width %3.0f, height: %3.0f", width, height);

//    UIInterfaceOrientationPortrait           = UIDeviceOrientationPortrait,
//    UIInterfaceOrientationPortraitUpsideDown = UIDeviceOrientationPortraitUpsideDown,
//    UIInterfaceOrientationLandscapeLeft      = UIDeviceOrientationLandscapeRight,
//    UIInterfaceOrientationLandscapeRight     = UIDeviceOrientationLandscapeLeft

    CGFloat angle = 0;

    // rotate from Portait
    if((fromInterfaceOrientation == UIInterfaceOrientationPortrait) && (appOrientation == UIInterfaceOrientationLandscapeLeft)){
        angle = -(M_PI / 2.0);
    }
    else if((fromInterfaceOrientation == UIInterfaceOrientationPortrait) && (appOrientation == UIInterfaceOrientationLandscapeRight)){
        angle = (M_PI / 2.0);
    }
    // rotate from PortraitUpsideDown
    else if((fromInterfaceOrientation == UIInterfaceOrientationPortraitUpsideDown) && (appOrientation == UIInterfaceOrientationLandscapeLeft)){
        angle = (M_PI / 2.0);
    }
    else if((fromInterfaceOrientation == UIInterfaceOrientationPortraitUpsideDown) && (appOrientation == UIInterfaceOrientationLandscapeRight)){
        angle = -(M_PI / 2.0);
    }

    //rotate from Landscape:
    else if((fromInterfaceOrientation == UIInterfaceOrientationLandscapeLeft) && (appOrientation == UIInterfaceOrientationPortrait)){
        // search for exactly the inverse...
    }
    // must be something more elgant code

    CGAffineTransform transform = self.view.transform;
    transform = CGAffineTransformRotate(transform, angle);
    self.view.transform = transform;

    [self.view setBounds:CGRectMake(0, 0, height, width)];

I think that you can rewrite the code in a more OO way, with the help of an NSDictionary and setting up a sort of command pattern.

You can store a sum of the two enumerator value as a key of the Dictionary in this way :

NSDictionary *dicAngles = [[NSDictionary alloc] initWithObjectsAndKeys:
        (UIInterfaceOrientationPortrait + UIInterfaceOrientationLandscapeLeft), -(M_PI / 2.0),     (UIInterfaceOrientationPortrait + UIInterfaceOrientationLandscapeRight), (M_PI / 2.0),
(UIInterfaceOrientationPortraitUpsideDown + UIInterfaceOrientationLandscapeLeft), (M_PI / 2.0),
(UIInterfaceOrientationPortraitUpsideDown + UIInterfaceOrientationLandscapeRight), -(M_PI / 2.0),
 nil];

Once you have the dictionary you can avoid the if - else control flow in this way :

 CGFloat angle = 0;
 //Retrieve the angle from the dictionary
 angle = [dicAngles objectForKey: ( fromInterfaceOrientation + appOrientation )];

It is only an example and not tested, but could be a more elegant solution i think ...


All permutations with known letter on one place Following code is doing what I want but I am looking for a way how to make it nicer and more effective

for i in c:
  for j in c:
    for k in c:
        for l in c:
            for m in c:
             if (i != j and i != k and i != l and j != k and j != l and k != l and i != m and j != m and k != m and l !=m):
                print i + j + 'u' + k + l + m

odusdi
odusdt
odusit
odusti
oduids
oduidt
oduist
oduits
odutds
odutdi
odutsi
...

I tried filters, lambdas and generators but that makes everything just worse.

I am new to python so I also have problems with copying and sending python code any hint with comment would be nice.

Usually, when you find yourself with multiple nested loops doing functionally the same thing, that's a good time to look for a solution using recursion, which are functions (you could indeed use generators) calling themselves.

So, you've got a list of characters c.
For the first letter, you pick from c.
For the second letter, you pick from c except that you can't choose the first letter.
For the third letter, likewise but letters 1 and 2 are banned.
And so on.

It should be clear that each of those is doing the same thing, it's just using a different set of allowed letters. That means you can use the same function for the job, and have it take as a parameter the letters that it's allowed to use.

There are two advantages to such a function over your implementation (although you could patch your version easily enough, it would involve writing even more duplicate code)

Firstly, at each step in my description, I rule out impossible options straight away. In your version, i and j will both be the first element of c straight away which is clearly a waste of time, but you don't check until you have looped through k, l, and m too. It would be better to move on to the next j as soon as it becomes clear that you can't use it. (And likewise with the others)

Secondly, your version is very specific to your input c. It's fine so long as c is exactly 5 long and only contains each letter once. If c contains any double letters you won't find any permutations at all. They'd all get swallowed up because python wouldn't know that they're meant to be different 'e's or whatever. Likewise if c was not 5 characters long, your code would not give a permutation. The recursive functional approach avoids both of those problems, and so is much more flexible.

Alternatively, as Peilonrayz suggests, look for the same code that someone else has written. itertools has permutations as well as combinations. The known letter thing is easy enough: you loop through permutations of the remaining characters, and for each one you print out the prefix, the 'u', and the suffix.

for perm in itertools.permutations(c):
    print (perm[0] + perm[1] + 'u' + perm[2] ...)

If you're a professional programmer, "use the library someone else wrote" is usually the best answer. It's just worth knowing the above so you can understand what's going on, including under the hood of the library function.


"Safe" memory management In order to concisify the handling of a failed call to malloc, realloc, or calloc, I have made a few files to dictate "safe" memory management. This is part of my project stac, so the prefix will appear in the following code snippets. Further, I have made the decision to use snake_case as opposed to standard camelCase.

What I hope to get out of this, and questions I have: are these functions in bad practice? Is there a different way I should be handling failed calls to memory allocators? And is there anything particularly wrong with the code I have written?

safe_mem.c

#include "safe_mem.h"

void* safe_malloc(size_t size){
    void* memory = malloc(size);

    if(memory == NULL){
        free(memory);
        runtime_error("failed to allocate memory.");
    }

    return memory;
}

void* safe_calloc(size_t num, size_t size){
    void* memory = calloc(num, size);

    if(memory == NULL){
        free(memory);
        runtime_error("failed to allocate memory.");
    }

    return memory;
}

void* safe_realloc(void* ptr, size_t size){
    void* memory = realloc(ptr, size);

    if(memory == NULL){
        free(memory);
        runtime_error("failed to reallocate memory.");
    }

    return memory;
}

safe_mem.h

#ifndef STAC_SAFE_MEM
#define STAC_SAFE_MEM
#include <stdio.h>
#include <stdlib.h>
#include "error.h"

void* safe_malloc(size_t);
void* safe_calloc(size_t, size_t);
void* safe_realloc(void*, size_t);
#endif

error.c

#include "error.h"

void make_error(char* type, char* message){
    eprintf("%s error: %s\n", type, message);
}

void fatal_error(char* type, char* message, int exit_code){
    make_error(type, message);
    exit(exit_code);
}

void runtime_error(char* message){
    fatal_error("Runtime", message, STATUS_RUNTIME_ERROR);
}

void generic_error(char* message){
    fatal_error("Generic", message, STATUS_GENERIC_ERROR);
}

error.h

#ifndef STAC_ERROR
#define STAC_ERROR
#include <stdio.h>
#include <stdlib.h>

#define eprintf(...) fprintf(stderr, __VA_ARGS__)
#define STATUS_OKAY (0)
#define STATUS_RUNTIME_ERROR (1)
#define STATUS_GENERIC_ERROR (-1)

void make_error(char*, char*);
void fatal_error(char*, char*, int);

void runtime_error(char*);
void generic_error(char*);

#endif

are these functions in bad practice?

I would say not, as long as the original functions they are wrapping have the same function signature to allow their replacement.

  is there anything particularly wrong with the code I have written?

Allocating 0 bytes may return NULL, which does not indicate Out-of-memory.
Same for calloc(), realloc().

void* memory = malloc(size);
// if(memory == NULL){
if(memory == NULL && size > 0){

No point in calling free(memory) after if(memory == NULL)

if(memory == NULL){
  //  free(memory);

With eprintf(...), consider passing __FUNC__, __LINE__ of the error source to aid in debugging.  Or at least create unique error messages.

void* safe_malloc(size_t size){
   runtime_error("failed to malloc memory.");

void* safe_calloc(size_t num, size_t size){
    runtime_error("failed to calloc memory.");

In a couple places, use const to allow wider application of the functions that are not changing the referenced data.

// void make_error(char* type, char* message){
void make_error(const char* type, const char* message){

I am not a fan of your name-space choices for error.h which includes eprintf,
 STATUS_OKAY, STATUS_GENERIC_ERROR, make_error, ...  If I say these used in other ..c files how would I be guided back to error.h/error.c as their origin?  Although a bit verbose, I prefer error_printf, ERROR_STATUS_GENERIC,...

Minor: #include <stdio.h> need not be in safe_mem.h nor #include <stdlib.h> in error.h.  error.c should have #include <stdlib.h> as it calls exit().  error.c should not rely on error.h including that.  Similar issues for safe_mem.c

From a design standpoint, I would also include void safe_free(void *) as the functionality of these safe functions may expand and require that even if it present does nothing.  It makes safe_...() symmetric.


Python Hash Table Implementation I'm attempting to implement a basic hash table in Python using only lists. Any tips would be appreciated (including better hash functions).
I intend this to handle collisions with separate chaining.

Are there any features I didn't implement that are standard in hash tables?

Are there any things I handled incorrectly or could have implemented in a better way?

My implementation:
class HashTable(object):

    table = [None] * 256

    def get_value(self, key):
        total = 0
        for i in range(len(key)):
            total += ord(key[i]) * (7**i)
        return (len(key) * total) % 256

    def insert(self, key):
        val = self.get_value(key)
        if self.table[val] == None:
            self.table[val] = key
        else:
            if type(self.table[val]) == list:
                self.table[val].append(key)
            else:
                self.table[val] = [self.table[val], key]

    def delete(self, key):
        val = self.get_value(key)
        if self.table[val] != None:
            if type(self.table[val]) == list:
                i = self.table[val].index(key)
                self.table[val][i] = None
            else:
                self.table[val] = None
        else:
            KeyError()

    def lookup(self, key):
        found = False
        val = self.get_value(key)
        if type(self.table[val]) == list:
            found = key in self.table[val]
        else:
            found = self.table[val] == key
        return found

Note: This works on both Python 2 and should work in Python 3

I'm pretty sure you implemented the necessary features in a standard hash table. If you want the hash table to populate only unique values then you need to change your insert method by looking up the value before inserting it.

Here are some things you did incorrectly or can do better in my opinion:

table = [None] * 256

table is static right now, which means that any instance of the class will have the same table variable. You need to initiate it in __init__.

def get_value(self, key):

get_value is a method that should not be called by the user of the class. I recommend making is private by changing its name to _get_value.

def insert(self, key):
    val = self.get_value(key)
    if self.table[val] == None:
        self.table[val] = key
    else:
        if type(self.table[val]) == list:
            self.table[val].append(key)
        else:
            self.table[val] = [self.table[val], key]

Why starting with a single value and only after that changing to a list? I recommend starting with a list right from the start. As said by python's module this:

  Special cases aren't special enough to break the rules.

That way, you can start with a table of empty lists right from the start, this will simplify your insert and lookup methods.

def delete(self, key):
    val = self.get_value(key)
    if self.table[val] != None:
        if type(self.table[val]) == list:
            i = self.table[val].index(key)
            self.table[val][i] = None

Making the value None can be dangerous - what if the user inserts lot's of values and then removes all of them? the lookup method will then take much more time than required. Although it takes more time, I still think list.remove is the right thing to do here.

    ...
    else:
        KeyError()

You need to raise the KeyError. Also - it's better to put in the error the right message. something like:
raise KeyError("key {key} can't be found.".format(key=key)


Split a file into multiple files if it reaches a particular limit? For each clientId, I have a Collection<Task> object. I am making file for each clientId and for each clientId file, I have all their tasks in it separated by new line in it.

Below is my original code:-

public class FileProcessor {
  private final String clientId;
  private final Collection<Task> tasks;

  public FileProcessor(String clientId, Collection<Task> tasks) {
    this.clientId = clientId;
    this.tasks = tasks;
  }

  public void generateFile() {
    String fileName = "tasks_info_" + clientId + ".txt";
    StringBuilder sb = new StringBuilder();
    for (Task task : tasks) {
      sb.append(task).append(System.getProperty("line.separator"));
    }
    try (BufferedWriter writer = new BufferedWriter(new FileWriter(fileName))) {
      writer.write(sb.toString());
    } catch (IOException ex) {
      ex.printStackTrace();
    }
  }
}

Let' say if clientId is 123, then it will create a file called "tasks_info_123.txt" with all the tasks in it. Now I have a requirement that file size of each clientId file cannot go more than 50000 bytes. So now what I have to do is:

I have to split each clientId file into multiple files like this: "tasks_info_123_0.txt", "tasks_info_123_1.txt", "tasks_info_123_2.txt"
As you can see, I have added "_0", "_1", "_2" in it.
So I will keep creating multiple files for each clientId until I can fit all the data in it and also making sure each file size is not more than 50000 bytes.
If we can fit all the data in one file, then I will have only one clientId file like this: "tasks_info_123_0.txt"
Also we should not create a file if collection of task object is empty.

So I got below code but it is not efficient as well seems like so opting for code review to see if there is any better and efficient way?

  public void generateFile() {
    int size = 0;
    int index = 0;
    StringBuilder sb = new StringBuilder();
    for (Task task : tasks) {
      sb.append(task).append(System.getProperty("line.separator"));
      size += sb.toString().getBytes(StandardCharsets.UTF_8).length;
      if (size > 50000) {
        String fileName = "tasks_info_" + clientId + "_" + index + ".txt";
        try (BufferedWriter writer = new BufferedWriter(new FileWriter(fileName))) {
          writer.write(sb.toString());
        } catch (IOException ex) {
          ex.printStackTrace();
        }
        index++;
        sb = new StringBuilder();
        size = 0;
      }
    }
    // for cases where we don't reach the limit
    String fileName = "tasks_info_" + clientId + "_" + index + ".txt";
    try (BufferedWriter writer = new BufferedWriter(new FileWriter(fileName))) {
      writer.write(sb.toString());
    } catch (IOException ex) {
      ex.printStackTrace();
    }
  }

Note: I don't want to use logger for this, just that I want to try it out on my own.

You want to limit file size, measured in bytes.
You write records, one for each task, terminated by line.separator.
You are concerned about performance (with a limit of just 50 kB on file size - oh well).

do not encode chars more than once (with 2n (guesstimated: 50) records per file, your average char gets encoded n+1 (26) times with the second generateFile() presented)
using "old Java IO", have a FilterOutputStream tally bytes
do not use a buffered Writer: it would obscure record boundaries

giving it a try:

/** Writes a limited amount of bytes to each of
 *  a succession of files numbered from 0 where necessary.
 *  Buffers liberally. */
static class SequentialFileOutputStream extends java.io.FilterOutputStream {
    final static int MAXBUF = 1<<20;
    final byte[]buffer;
    int pos, index;
    final long limit;
    long left;
    File file;
    String path, name, suffix;
    /** Creates an OutputStream that starts writing
     *  to file, creating numbered files as necessary. */
    public
    SequentialFileOutputStream(File file, boolean append, long limit)
        throws IOException
    {
        super(null);
        File parent = (this.file = file).getParentFile();
        if (null == parent)
            parent = new File(".");
        // checks path, too
        if (parent.getFreeSpace() < limit)
            throw(new java.io.IOException("free < limit"));
        this.limit = left = limit;
        buffer = new byte[(int) Math.min(limit, MAXBUF)];
    }
    /** Closes the current part, if any. */
    void closePart() throws IOException {
        if (null != out) try {
            flush();
        } finally { try {
            out.close();
        } finally {
            out = null;
        }}
    }
    /** Finishes the current part, if any.
     * Prepares a new one as far as necessary. */
    void nextPart() throws IOException {
        closePart();
    }
    /** Ensures out is open and buffer reset. */
    void ensureOpen() throws FileNotFoundException {
        if (null != out)
            return;
        left = limit;
        pos = 0;
        if (null == suffix) {
            path = file.getPath();
            name = file.getName();
            int dot = name.lastIndexOf('.');
            if (0 <= dot) {
                suffix = name.substring(dot);
                path = path.substring(0,
                    path.length() - suffix.length()) + '_';
            } else
                suffix = "";
            out = new java.io.FileOutputStream(file);
        } else {
            if (null != file) {
                File zero = new File(path + '0' + suffix);
                zero.delete();
                file.renameTo(zero);
                file = null;
            }
            out = new java.io.FileOutputStream(path + ++index + suffix);
        }
    }
    @Override
    public void write(int b) throws IOException {
        byte[]a = { (byte) b };
        write(a, 0, a.length);
    }
    @Override
    public void write(byte[] b, int off, int len)
        throws IOException {
        if (len <= 0 || null == b)
            return;
        ensureOpen();
        if (left < len && 0 < pos) {
            flush();
            nextPart();
        }
        if (limit < len)
            out.write(b, off, len);
        else {
            System.arraycopy(b, off, buffer, pos, len);
            pos += len;
            left -= len;
        }
    }
    @Override // closePart()?
    public void flush() throws IOException {
        if (0 < pos) {
            ensureOpen();
            out.write(buffer, 0, pos);
            pos = 0;
        }
        out.flush();
    }
    @Override
    public void close() throws IOException { closePart(); }
}
private final String clientId;
private final Collection<Task> tasks;

public FileSplitter(String clientId, Collection<Task> tasks) {
    this.clientId = clientId;
    this.tasks = tasks;
}
/** Kludge to flush an OutputStreamWriter's encoder's buffer */
static class FlushWriter extends java.io.OutputStreamWriter {
    final static Class[]NO_CLASSES = {};
    static java.lang.reflect.Method flushEncoder;
    // further constructors left as an exercise
    public FlushWriter(OutputStream out) { super(out); }
    /** Flushes the encoder's buffer */
    java.io.Writer flushEncoder() {
        try {
            if (null == flushEncoder) {
                final Class<?> osWriter = FlushWriter.class.getSuperclass();
                flushEncoder = osWriter
                    .getDeclaredMethod("flushBuffer", NO_CLASSES);
                flushEncoder.setAccessible(true);
            }
            flushEncoder.invoke(this, (Object[])null);
        } catch (ReflectiveOperationException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
        return this;
    }
}

public void generateFile() {
    String fileName = "tasks_info_" + clientId + ".txt",
        lineSeparator = System.getProperty("line.separator");
    try (FlushWriter writer = new FlushWriter(
            new SequentialFileOutputStream(
                new File(new File("."), fileName), false, 9000))) {
        for (Task task : tasks) {
            writer.append(task.toString()).append(lineSeparator);
            writer.flushEncoder();
        }
    } catch (IOException ex) {
        ex.printStackTrace();
    }
}

public static void main(String[] args) {
    Task t = new Task() {@Override public String toString() {
                return "I am the most useless task ever";
        }};
    Collection<Task>tasks = new java.util.ArrayList<>(999);
    for (int i = 999 ; 0 < --i ; )
        tasks.add(t);
    new FileSplitter("broke", tasks).generateFile();
}

The non-buffered OutputStreamWriter didn't suffice, in the end:
Sunsoft's encoder buffers. Trying to keep that from interfering messed things up.

The generateFile() from the question seems to have a correctness issue with the way it guesstimates byte count: it extends sb and accumulates "the length of byte[]s gotten from String representations of sb". It should rather

set the size to said length - or -
accumulate the "byte length" of the String for each Task
which, in combination with instantiating sb with an appropriate length, should lead to run time linear in the number of bytes in each file rather than quadratic.

But once you "have the byte[]", you might as well collect those.
This seems to call for java.nio.ByteBuffer &co:

public void generateFile() {
    ByteBuffer bb = ByteBuffer.allocate(limit);
    for (Task task : tasks) {
        byte[]taskBytes = task.toString().getBytes(
            java.nio.charset.StandardCharsets.UTF_8);
        int taskLen = taskBytes.length+LS_LENGTH;
        if (bb.remaining() < taskLen)
            buffer2file(bb, index++);
        if (limit < taskBytes.length+2*LS_LENGTH) {
            ByteBuffer big = ByteBuffer.allocate(taskLen);
            big.put(taskBytes).put(LINE_SEPARATOR_BYTES);
            buffer2file(big, index++);
        } else
            bb.put(taskBytes).put(LINE_SEPARATOR_BYTES);
    }
    buffer2file(bb, index++);
}

/** Write a numbered client file if bb not empty. */
private void buffer2file(ByteBuffer bb, int i) {
    bb.flip();
    if (bb.hasRemaining()) {
        String fileName = "tasks_info_" + clientId + "_" + i + ".txt";
        try (java.nio.channels.FileChannel
            fc = java.nio.channels.FileChannel.open(
                java.nio.file.Paths.get(fileName, NO_STRINGS),
                java.nio.file.StandardOpenOption.CREATE,
                java.nio.file.StandardOpenOption.WRITE)) {
            fc.write(bb);
        } catch (IOException ex) {
            ex.printStackTrace();
        }
    }
    bb.flip();
}
static final String NO_STRINGS[] = {},
    LINE_SEPARATOR = System.getProperty("line.separator");
static final byte[]
    LINE_SEPARATOR_BYTES = LINE_SEPARATOR.getBytes();
static final int LS_LENGTH = LINE_SEPARATOR_BYTES.length;


Prim algorithm implementation for adjacency list represented graph Please review the implementation of Prim algorithm. Points on which I have doubt:

My Graph doesn't have any ID for nodes. Nodes are accessed based on their data. Is there any general approach in that? Should nodes be referenced by an ID as that will also make these algo somewhat easier to implement by using id as index into arrays.
With current graph representation, are there any issue in the algo implementation?
Are there any issues in method of accessing vertex and edges shared_ptr? Should weak_ptr be used at some places? Any shared_ptr validity issue?
Anything that you can suggest to improve will be helpful.

Vertex:

class Vertex {
public:
    Vertex() {
        bVisited = false;
    }
    int data = 0;
    EdgeList edgeList;
    bool bVisited = false;
};

Edge:

struct Edge {
    int cost;
    boost::weak_ptr<Vertex> srcVertex;
    boost::weak_ptr<Vertex> dstVertex;
};

Graph:

typedef std::list<boost::shared_ptr<Vertex>> VerticesList;
typedef std::map<int, VerticesList::const_iterator> DataVertexMap;
class Graph {

    VerticesList _verticesList;
    DataVertexMap _dataVertexMap;

    //TODO: Need some design to have this function internal only and not exposed to client
    boost::shared_ptr<Vertex> addAndGetVertex(int data);
public:
    Graph();
    ~Graph();

    bool isEmpty() const;
    //We don't check for duplicate vertex
    void addVertex(int data);
    void addEdge(int srcData, int dstData, int cost);
    int getCostForEdge(int srcData, int dstData);
    void displayGraph();
    void dfsTraversal();
    void bfsTraversal();

    void findShortestPath(int srcData, int dstData);
    void kruskalMST();
    void primMST();
};

Prim's implementation:

 //return min cost edge
 boost::shared_ptr<Edge> getMinEdge(const EdgeList& edgeList) {
    int min = INFINITY;
    boost::shared_ptr<Edge> minEdge;
    for(boost::shared_ptr<Edge> edge : edgeList) {
        if(edge->cost < min) {
            min = edge->cost;
            minEdge = edge;
        }
    }

    return minEdge;
}

void Graph::primMST() {
    EdgeList mstEdgeList;
    EdgeList potentialEdgeList;

    //insert edges for first vertex
    boost::shared_ptr<Vertex> firstVertex = _verticesList.front();
    potentialEdgeList.insert(potentialEdgeList.begin(), firstVertex->edgeList.begin(), firstVertex->edgeList.end());

    while(mstEdgeList.size() < _verticesList.size() - 1) {
        boost::shared_ptr<Edge> minEdge = getMinEdge(potentialEdgeList);
        mstEdgeList.push_front(minEdge);
        potentialEdgeList.remove(minEdge);
        //Mark the vertices visited so that edge having them as destination are not again included
        (minEdge->srcVertex).lock()->bVisited = true;
        (minEdge->dstVertex).lock()->bVisited = true;
        EdgeList& dstVertexEdgeList = (minEdge->dstVertex).lock()->edgeList;
        for(boost::shared_ptr<Edge> edge : dstVertexEdgeList) {
            if(!(edge->dstVertex).lock()->bVisited)
                potentialEdgeList.push_front(edge);
        }
    }

    //TODO: set all bVisited false

    // print the contents of result[] to display the built MST
    printf("Following are the edges in the constructed MST\n");
    for (boost::shared_ptr<Edge> edge : mstEdgeList)
        printf("%d -- %d == %d\n", (edge->srcVertex).lock()->data, (edge->dstVertex).lock()->data,
               edge->cost);
    return;
}

I try some review, although I don't know the Prim algorithm until now, so there are mostly general things here. Hope it is helpful nevertheless. The following points are in random order:

Regarding the language:

In your Vertex() {} class, you can omit the bVisited = false; as it is already initialized in the declaration. Otherwise, it's better to use an initializer list, Vertex() : bVisited(false) {}.
In C++11 (which you tagged) I would rather use the standard-library smart pointers and not the boost stuff. So, I would add an #include<memory> and replace all boost:: namespaces by std::.
Your function getMinEdge can be replaced by an single-line STL algorithm:

auto getMinEdge(const EdgeList& edgeList) -> boost::shared_ptr<Edge>
{
    return *std::min_element(edgeList.begin(),edgeList.end()
                           , [](boost::shared_ptr<Edge> const& a
                              , boost::shared_ptr<Edge> const& b)
                               { return a->cost < b->cost; });
}

When using range-based for loops

 for(boost::shared_ptr<Edge> edge : dstVertexEdgeList)

I would rather use auto instead of explicitly stating the type. Moreover, as copying a shared_ptr is not an unexpensive operation (as the memory manager must increase its reference count), here I would use auto& or better auto const& and pass the shared_ptr by reference.

The same holds for statements like boost::shared_ptr<Edge> minEdge = getMinEdge(potentialEdgeList); or EdgeList& dstVertexEdgeList = (minEdge->dstVertex).lock()->edgeList;. Better use auto or auto&, resp.

Regarding the design:

Regarding the storage scheme: The std::list<std::shared_ptr<Vertex> > you chose is probably the safest option, but probably also not the most performant. As an alternative you could for instance use std::shared_ptr<std::list<Vertex> >. It's rather fast and advantageous imo if vertices are more or less constant, such that you do not make changes in copies of the class -- because these would affect the original list. Further I would also consider whether a simple std::list<Vertex> is sufficient for you, as that is surely the easiest choice (--then you could then forget about all the smart-pointer stuff). Much here depends on the size of the pointed-to objects. The smaller they are, probably the less appropriate is the overhead of a shared_ptr.

The same holds for EdgeList. Here, you seem to make some copies and change these, such that std::shared_ptr<std::list<Edge> > doesn't make much sence (as you would change the original list). However, as edge is a rather small object, again a simple std::list could be more efficient.

But these are fundamental design questions which affect "only" the performance. I leave them up to you and will go on with the implementation as it is.
Rather than a std::list, you can also consider using a std::set or a sorted std::vector (as you already using boost, the latter corresponds to a flat_set). This gives you better performance for the search (logarithmic vs. linear for a list) but an increased effort for insertion and deletion (logarithmic vs. constant for a list).
The weak_ptr in the Edge class is ok, though it might not exactly be what you want. At the moment it works like this: an Edge connects two vertices. If now one vertex is removed from the graph, the Edge remains, but when you want to go along it the shared_ptr from weak_ptr initialization will fail and implicitly tell you that it is a dead end. I don't know whether this is desired.

Instead, the graph object should know that it has to automatically remove this Edge once the Vertex is destroyed. This obviously would require some corresponding logic in the Graph class. As an alternative, you could also create a closer coupling between the two classes (e.g. maybe hold an EdgeList for each vertex).

Further, when you go for a weak_ptr and .lock() it, you must keep track that it still points to something which is alive. Otherwise, access to variables of the pointed-to class fails. If you not check for existence and simply rely on it due to your code, a raw pointer would be sufficient.

So, that's it for now, hope it helps. When I get to know the Prim algorithm itself, I'll maybe make an update.


Caesar cipher in Ruby I must implement a simple Caesar cipher class. Initializer takes two parameters - number of positions to shift the alphabet (integer) and the alphabet used (as string, not necessarily the standard English alphabet). The class must have two instance methods encrypt and decrypt, which take a string and do the respective action based on the parameters of the initializer. These are my rspec tests:

describe "Caesar" do

  latin_encrypter = Caesar.new 4
  dna_encrypter = Caesar.new 3, 'ACTG'

  it 'encrypts correctly' do
    expect(latin_encrypter.encrypt 'hello').to eq "lipps"
  end

  it 'encrypts correctly using DNA alphabet' do
    expect(dna_encrypter.encrypt 'ACCTGA').to eq "GAACTG"
  end

  it 'decrypts correctly' do
    expect(latin_encrypter.decrypt 'lipps').to eq 'hello'
  end

  it 'decrypts correctly using DNA alphabet' do
    expect(dna_encrypter.decrypt 'GAACTG').to eq 'ACCTGA'
  end
end

and this is my implementation:

class Caesar
  def initialize(shift, alphabet = 'abcdefghijklmnopqrstuvwxyz')
    @shift = shift % alphabet.size
    @alphabet = alphabet
  end

  def encrypt(string)
    string.chars.map { |char| @alphabet[@alphabet.index(char) + @shift - @alphabet.size] }.join
  end

  def decrypt(string)
    string.chars.map { |char| @alphabet[@alphabet.index(char) - @shift] }.join
  end
end

Is this an optimal algorithm for encryption/decryption or is there a better one (perhaps using gsub)?

The only (conceptual) problem I see in your code is that both encrypt/decrypt perform a O(n) operation (detect) for every character (so at then end it's a O(n*m) algorithm), that's unnecessarily inefficient. Build a hash object to use as an indexer:

class Caesar
  def initialize(shift, alphabet = ('a'..'z').to_a.join)
    @shift = shift
    @alphabet = alphabet
    @indexes = alphabet.chars.map.with_index.to_h
  end

  def encrypt(string)
    string.chars.map { |c| @alphabet[(@indexes[c] + @shift) % @alphabet.size] }.join
  end

  def decrypt(string)
    string.chars.map { |c| @alphabet[(@indexes[c] - @shift) % @alphabet.size] }.join
  end
end

You can also build encrypter/decrypter hash tables at initialization:

class Caesar
  def initialize(shift, alphabet = ('a'..'z').to_a.join)
    chars = alphabet.chars.to_a
    @encrypter = chars.zip(chars.rotate(shift)).to_h
    @decrypter = @encrypter.invert
  end

  def encrypt(string)
    @encrypter.values_at(*string.chars).join
  end

  def decrypt(string)
    @decrypter.values_at(*string.chars).join
  end
end


REST Server for Users using Restify and Mongoose I am creating a REST API to access a database of users. I am new to Node.js and developing REST APIs in general. I am also trying to use best security practices.

main.js

var fs = require('fs');
var restify = require('restify');
var bodyParser = require('body-parser');
var mongoose = require('mongoose');
var bcrypt = require('bcrypt');

mongoose.connect('localhost', 'nodejs-sample');

var User = require('./models/user')

// SSL options
var options = {
  certificate: fs.readFileSync('./ssl/cert.pem'),
  key: fs.readFileSync('./ssl/key.pem')
}

var server = restify.createServer(options);

server.use(bodyParser.urlencoded({extended: true}));
server.use(bodyParser.json());

server.get('/users', function(req, res) {
  User.find(function(err, users) {
    if (err) {
      res.send(err);
    }

    res.json(users);
  });
});

server.post('/users', function(req, res) {
  bcrypt.hash(req.body.password, 10, function(err, hash) {
    if (err) {
      res.send(err);
    }

    User.create({
      username: req.body.username,
      first_name: req.body.first_name,
      last_name: req.body.last_name,
      email: req.body.email,
      hash: hash
    }, function(err) {
      if (err) {
        res.send(err)
      }

      res.json({message: 'User added'});
    });
  });
});

server.get('/users/:user_id', function(req, res) {
  User.findById(req.params.user_id, function(err, user) {
    if (err) {
      res.send(err);
    }

    res.json(user);
  });
});

server.put('/users/:user_id', function(req, res) {
  var body = req.body;
  if (body.password) {
    bcrypt.hash(body.password, 10, function(err, hash) {
      if (err) {
        res.send(err);
      }

      delete body.password;
      body.hash = hash;
      User.update({_id: req.params.user_id}, body, function() {
        res.json({message: 'User updated'});
      });
    });
  } else {
    User.update({_id: req.params.user_id}, body, function() {
      res.json({message: 'User updated'});
    });
  }
});

server.del('/users/:user_id', function(req, res) {
  User.findByIdAndRemove(req.params.user_id, function(err) {
    if (err) {
      res.send(err);
    }

    res.json({message: 'User deleted'});
  });
});

server.listen(process.env.PORT || 8080, function() {
  console.log('%s listening at %s', server.name, server.url);
});

models/user.js

var mongoose = require('mongoose');
var Schema = mongoose.Schema;

var UserSchema = new Schema({
  username: {
    type: String,
    required: true,
    unique: true
  },
  first_name: {
    type: String,
    required: true
  },
  last_name: {
    type: String,
    required: true
  },
  email: {
    type: String,
    required: true
  },
  hash: {
    type: String,
    required: true
  }
});

// remove hash from JSON output
UserSchema.methods.toJSON = function() {
  var obj = this.toObject();
  delete obj.hash;
  return obj;
};

UserSchema.methods.verifyPassword = function(password, cb) {
  bcrypt.compare(password, this.hash, function(err, res) {
    if (err) {
      return cb(err);
    }

    cb(null, res);
  });
};

module.exports = mongoose.model('User', UserSchema);

Few points to consider:
- Extract users CRUD logic to UsersService - this service will interact with mongoose and will be responsible for create, get, update and delete operations.
- Consider using Json Web Tokens instead of passing the password for every put request - it is an alternative for using sessions.

Overall for a newbie I consider this as a very good code.


Binary search in Java I have started learning Java recently and was looking into some easy algorithms. I found the Binary Search Algorithm here

I am trying to get better at writing good code for my solutions. I have used MergeSort for sorting the unsorted array first.Please give me your suggestions.

    import java.util.Scanner;

    public class binarysearch {

        public static void main(String[] args) {

            Scanner keyboard = new Scanner(System.in);
            int size = keyboard.nextInt();
            int[] array = new int[size];
            for (int i = 0; i < array.length; i++) {
                array[i] = keyboard.nextInt();
            }
            // sort the array using mergesort
            mergeSort (array);
            System.out.println("Enter the element to search");
            int searchkey = keyboard.nextInt();
            // call the function to do the binary search
            binarySearch (array, searchkey);

        }

        private static void binarySearch(int[] array, int searchkey) {

            int low = 0; // lowest index
            int high = array.length - 1; // highest index
            int guess = (low + high) / 2;

            while (low <= high) {
                if (array[guess] == searchkey) {
                    System.out.println("The element " + searchkey
                            + " is found at the index " + guess);
                    break;
                } else if (array[guess] < searchkey) {
                    low = guess + 1;
                } else
                    high = guess - 1;
                guess = (low + high) / 2;

            }
            if (low > high) {
                System.out.println("The element " + searchkey + " is not found");
            }

        }

        private static int[] mergeSort(int[] a) {

            if (a.length <= 1) {
                return a;
            }

            // split the array into two halves

            int[] first = new int[a.length / 2];
            int[] second = new int[a.length - first.length];

            System.arraycopy(a, 0, first, 0, first.length);
            System.arraycopy(a, first.length, second, 0, second.length);

            mergeSort (first);
            mergeSort (second);

            merge (first, second, a);

            return a;

        }

        private static void merge(int[] first, int[] second, int[] a) {
            int i = 0;
            int j = 0;
            int k = 0;

            while (i < first.length && j < second.length) {
                if (first[i] <= second[j]) {
                    a[k] = first[i];
                    i++;
                    k++;
                } else {
                    a[k] = second[j];
                    j++;
                    k++;
                }

            }

            while (i < first.length) {
                a[k++] = first[i++];
            }
            while (j < second.length) {
                a[k++] = second[j++];
            }

        }

    }

I'll only be reviewing the binary search portion of the code since that's what it seems like you're asking for.

Separation of Concerns

You should separate I/O from processing and let the user control what kind of I/O they'd like to have. So I would remove the output statements from your binarySearch function and find a better way to indicate the element was not found. The solution I'll go with is that we'll return the index of the element if it is found and -1 otherwise (since -1 is never a valid array index).

Better Naming

You call the middle element guess even though it is not a guess. Someone unfamiliar with binary search would read this code and assume that you guessed the element was in the middle. This is not true so I would rename that variable to pivot or middle. Moreover, you can move the calculation of it to be inside the while loop.

Miscellaneous

Use consistent bracing style (e.g. always use them. They never hurt).
The if statement at the end of the binary search is unnecessary since when we exit the while loop, we know that low > high.
Since middle won't be changing in one while loop cycle, we can mark it final.

Here is a possible implementation taking into account all of the above.

    private static int binarySearch(int[] array, int searchkey) {

        int low = 0; // lowest index
        int high = array.length - 1; // highest index

        while (low <= high) {
            final int middle = low + (high - low) / 2;
            if (array[middle] == searchkey) {
                return middle;
            } else if (array[middle] < searchkey) {
                low = middle + 1;
            } else {
                high = middle - 1;
            }
        }
        return -1;
    }


Random C++ STL-only password generator for education This is some code I wrote five years ago and I'm now using it partially as instructional material. I think there are some things that are 'off' about it, namely the top of the main function being the definition of a few variables instead of the argument parsing, and the actual generation not being split into its own function.

#include <chrono>
#include <functional>
#include <iostream>
#include <random>
#include <string>

static void show_usage(std::string name)
{
    std::cerr << "Usage: " << name << " <option(s)>\n"
              << "Options:\n"
              << "\t-h,--help\t\tShow this help message\n"
              << "\t-l,--length LENGTH\tSpecify the password length\n"
              << "\t-s,--seed SEED\t\tUse a unique seed for the random engine\n"
              << "\t-v,--version\t\tOutput the program version\n";
}

int main (int argc, char *argv[])
{
    std::string charset ("abcdefghijklmnopqrstuvwxyz"
                         "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
                         "1234567890!@#$%^&*()");
    int length = 16;
    std::default_random_engine generator;
    std::uniform_int_distribution<int> distribution (0, (int) charset.size() - 1);
    generator.seed (std::chrono::system_clock::now().time_since_epoch().count());

    if (argc >= 2) {
        for (int i = 1; i < argc; i++) {
            std::string arg = argv[i];
            if ((arg ==  "-l") || (arg == "--length")) {
                if (i + 1 < argc) {
                    length = atoi(argv[++i]);
                } else {
                    std::cerr << "-l,--length takes one argument" << std::endl;
                    return 1;
                }
            } else if ((arg == "-s") || (arg == "--seed")) {
                if (i + 1 < argc) {
                    std::string str (argv[++i]);
                    std::seed_seq seed (str.begin(), str.end());
                    generator.seed (seed);
                } else {
                    std::cerr << "-s,--seed takes one argument" << std::endl;
                    return 1;
                }
            } else if ((arg == "-h") || (arg == "--help")) {
                show_usage(argv[0]);
                return 0;
            } else if ((arg == "-v") || (arg == "--version")) {
                std::cout << "Version 0.0.1" << std::endl;
                return 0;
            }
        }
    }

    auto randchar = std::bind (distribution, generator);
    std::string password;
    password.resize(length);

    for (int i = 0; i < length; i++) {
        password.at(i) = charset.at(randchar());
    }

    std::cout << password << std::endl;
    return 0;
}

The code looks good. May be argument parsing could be done better, but in general this is solid simple password generator. There are things to mention though, but they mostly apply on reusability and expandability

Separate concerns

At the moment the main() function does quite a lot of things:

Setup random number generator
Parse program arguments
Generate password
Output it to the user

Encapsulate

I'm not saying reduce usability, but the password generator is very much like random number generator, it just gives out strings with random elements instead of bit blocks.

So, in my opinion, the following code would have sound design:

class random_password_generator
{
    //random engine and distribution
public:
    random_password_generator()
    {
        //initialize
    }

    //random engine will decide rule of 5

    std::string operator()()
    {
        //create password
    }
}

Parsing arguments can be just a plain function. For complex cases may be using Boost.program_options would be better.

Code Review

atoi(argv[++i]);

atoi is a C standard library function. stoi is a C++ function standard library. In general, C++ library is more compatible with concepts of C++ itself, whereas C library usually needs some wrappers to be usable with the rest of C++.

int length = 16;

My personal preference is to use unsigned int or std::size_t for things that cannot have negative values.

Do not use bind unless necessary

std::bind() might have problems that lambda can't solve, but I'm yet to encounter such problems. In general: use lambda for function objects unless you can't. The reason is that std::bind does heap allocation, type erasure, and possibly other stuff, whereas lambda is just anonymous object with operator().

Range loops

for (int i = 0; i < length; i++)

The above also has a chance to fail in case you'll change the resizing part of the string. It is better to use range for loop:

for (char& c: password)
{
    //assign
}

It will also remove the need to use at(), which does bounds checking, and in general frowned upon in release code.

Even better

Use standard algorithms. Since the functor generates random characters, code could just use std::generate(password.begin(), password.end(), generator).

Do not use std::endl

std::endl is not only a platform independent way to print newline. Printing newline is achieved with streaming '\n'. Even on windows ostream will deal with it. std::endl prints newline and flushes stream, which might cause detrimental performance hit in tight loops.

Don't use C style casts

(int) charset.size()

C style casts are much more dangerous, and compiler will not warn you if it does something dangerous. On the other hand, using one of C++ style casts will ensure compilation error in case something fishy is going on, except reinterpret_cast<>, which is fishy by definition.


Performance analysis of 3 digits sum I have a method that finds 3 numbers in an array that add up to a desired number.

public static void threeSum(int[] arr, int sum) {
    quicksort(arr, 0, arr.length - 1);
    for (int i = 0; i < arr.length - 2; i++) {
        for (int j = 1; j < arr.length - 1; j++) {
            for (int k = arr.length - 1; k > j; k--) {
                if ((arr[i] + arr[j] + arr[k]) == sum) {
                    System.out.println(Integer.toString(i) + "+" + Integer.toString(j) + "+" + Integer.toString(k) + "="  + sum);
                }
            }
        }
    }
}

I'm not sure about the big O of this method. I have a hard time wrapping my head around this right now. My guess is O(n2) or O(n2logn). But these are complete guesses. I can't prove this. Could someone help me wrap my head around this?

The running time is O(n3).  You have three nested loops:

Approximately n iterations of i
For each i, approximately n iterations of j
For each (i, j) pair, up to about n iterations of k.  On average, it take just 0.5 * n iterations of k, but the constant factor doesn't matter.

Quicksort is generally O(n log n).  So the total is O(n log n + 0.5 n × n × n), which is O(n3).

Since you took the trouble to sort the array, the innermost loop could be replaced by a binary search, which is O(log n).  Therefore, an improved algorithm could work in O(n2 log n).


Recursive LAMBDA() function to create a formula that adds internal separators to a string in Excel I have created a named function with signature PadInternal(base, width, paddingStr) where:

base is a string you want to add padding to
width is the length of the individual chunks
paddingStr is the string to pad with

called from a cell like:

PadInternal("Hello", 1, " ") = "H e l l o"
PadInternal("World", 3, "**") = "Wor**ld"

And here's the function:

Tag
Description

Name
PadInternal

Scope
Workbook

Comment
base : a string you want to add padding to | width : the length of the individual chunks | paddingStr : the string to pad with

Refers To
=LAMBDA(base,width,paddingStr, IF(LEN(base)<=width, base, LET(LHS, LEFT(base, width), RHS, RIGHT(base, LEN(base) - width), LHS & paddingStr & PadInternal(RHS,width,paddingStr))))

=LAMBDA(
    base,
    width,
    paddingStr,
    IF(
        LEN(
            base
        ) <= width,
        base,
        LET(
            LHS,
            LEFT(
                base,
                width
            ),
            RHS,
            RIGHT(
                base,
                LEN(
                    base
                ) - width
            ),
            LHS & paddingStr &
            PadInternal(
                RHS,
                width,
                paddingStr
            )
        )
    )
)

Questions
As this is my first time using recursive Lambda functions in Excel, I'd like some feedback. In particular:

Is my algorithm efficient - I was thinking something with TEXTJOIN may be faster?
How could this be improved to take a dynamic array as "base"?
Can I have default values for the arguments?
What about meta data (formatting of the tooltip, scope etc), are there other ways to make my function more accessible?

I thought about this for a while and came up with some (what I think are) improvements:
=LAMBDA(base,width,paddingStr,TEXTJOIN(paddingStr,,MID(base,SEQUENCE((LEN(base)/width)+1,,1,width),width)))

This would:

No longer require a recursive LAMBDA() which should prove to be faster (untested)
Uses TEXTJOIN() which can handle errors and empty values internally so there is no longer any worry about that.
The use of MID() negates the use of LEFT(), RIGHT() etc.
No longer be using LET() which may also be using internal memory.


Adjacency matrix on undirected graph Please review my Adjacency Matrix class based on this code.

Since I use std::vector to create the matrix, I would like to store as many nodes as possible. Is it OK to use std::size_t all over the code?
Are using 1 for connected and 0 for not connected a bad idea?

class AdjMatrix {
public:
    AdjMatrix(const std::size_t& vertsize);
    std::size_t Vertices() const;
    std::size_t Edges() const;
    void addEdge(const std::size_t& a, const std::size_t& b);
    std::size_t degree(const std::size_t& v) const;
    std::size_t maxDegree() const;
    std::size_t minDegree() const;
private:
    std::size_t vertices;
    std::size_t edges;
    std::vector<std::vector<std::size_t>> matrix;
};

AdjMatrix::AdjMatrix(const std::size_t& vertsize) :vertices{vertsize < 0? throw std::out_of_range{"necgative values"} : vertsize}, edges{0}, matrix{vertsize} {
    for (auto& v : matrix) {
        v.resize(vertsize);
    }
}

std::size_t AdjMatrix::Vertices() const {
    return vertices;
}

std::size_t AdjMatrix::Edges() const {
    return edges;
}

void AdjMatrix::addEdge(const std::size_t& a, const std::size_t& b) {
    if (a < 0 || b < 0) {
        throw std::out_of_range{"negative values"};
    }
    if (matrix[a][b] == 0) {
        edges++;
    }
    matrix[a][b] = 1;
    matrix[b][a] = 1;
}

std::size_t AdjMatrix::degree(const std::size_t& v) const {
    std::size_t d = 0;
    for (std::size_t i = 0; i < matrix[v].size(); ++i) {
        if (matrix[v][i] == 1) {
            d++;
        }
    }
    return d;
}

std::size_t AdjMatrix::maxDegree() const {
    std::size_t max = 0;
    for (std::size_t i = 0; i < matrix.size(); i++) {
        std::size_t d = degree(i);
        max = d > max ? d : max;
    }
    return max;
}

std::size_t AdjMatrix::minDegree() const {
    std::size_t min;
    for (std::size_t i = 0; i < matrix.size(); i++) {
        std::size_t d = degree(i);
        if (i == 0) {
            min = d;
        } else {
            min = min < d ? min : d;
        }
    }
    return min;
}

How can I improve this code?

Using std::size_t. You asked:

  Is it ok to use std::size_t all over the code?

I think that's perfectly alright.
Use of const std::size_t

Using const std::size_t for the type of an argument has no advantage over using just std::size_t. It adds unnecessary verbosity. I would change them all to use std::size_t.
Inconsistent member function names.

Function names are better that start with a verb, such as addEdge. However, all other member functions don't follow that naming convention.

I would recommend the following changes:

  Vertices  -> getNumVertices
  Edges     -> getNumEdges
  degree    -> getDegree
  maxDegree -> getMaxDegree
  minDegree -> getMinDegree

Variable names

I think it's more appropriate to use numVertice and  numEdges instead of vertices and edges.
Checking for negative inputs

An object of std::size_t will never have a negative value. It's pointless to check whether an input is negative when it is of type std::size_t. I would recommend removing those checks altogether.
Range checking

In addEdge, you have:

if (a < 0 || b < 0) {
    throw std::out_of_range{"negative values"};
}

The check for negative values is not necessary. However, it is necessary to check whether those are within the upper bound. I would replace that check with:

if (a >= numVertices || b >= numVertices ) {

Better initialization of matrix.

I would change the constructor to:

AdjMatrix::AdjMatrix(std::size_t& vertsize) : numVertices{vertsize},
                                              edges{0},
                                              matrix{vertsize, std::vector<std::size_t>{vertsize, 0}}
{
}

Using std namespace in member function implementations.

If the member functions are implemented in a .cpp file, I would add

using std::size_t;
using std::vector;

in the .cpp file to reduce the amount of typing.
Remove if/else from minDegree().

Initialize min to std::numeric_limits<std::size_t>::max. Then, you don't need the if/else check. You can replace the lines:

if (i == 0) {
    min = d;
} else {
    min = min < d ? min : d;
}

with

min = min < d ? min : d;


Iterator pattern/iterator class I have implemented a simple iterator. Just want to check if I have missed anything.

interface iterator<E>
    {
        boolean hasNext();

        E next();
    }

    class Iterator<E> implements iterator
    {
        E[] arr;

        int curPos = 0;

        @Override
        public boolean hasNext()
        {
            if (curPos >= arr.length || arr[curPos] == null)
            {
                return false;
            }
            else
            {
                return true;
            }
        }

        @Override
        public E next()
        {
            if(hasNext())
            {
                E res = arr[curPos];
                curPos++;
                return arr[curPos+1];
            }
            else
            {
                throw new runtimeException("No next element available !");
            }
        }

    }

Questions:

Why should I not use Object instead of E/template everywhere ?
How do I make an iterator for a binary tree.

I assume apart from the above, I would have to implement a traversal algorithm and try to find if the next "traversal" successor exist?

I would write an iterator for arrays that way:

import java.util.Iterator;
import java.util.NoSuchElementException;

public class ArrayIterator<E> implements Iterator<E> {

    private final E[] arr;
    private int currentIndex = 0;

    public ArrayIterator(E... arr) {
        if(arr == null) {
            throw new IllegalArgumentException("Array is null");
        }
        this.arr = arr;
    }

    public boolean hasNext() {
        return currentIndex < arr.length;
    }

    public E next() {
        if (! hasNext()) {
            throw new NoSuchElementException();
        }
        return arr[currentIndex++];
    }

    public void remove() {
        throw new UnsupportedOperationException("Not supported.");
    }

}

To your second question: I assume you want to traverse the tree in-order, and that the nodes have no link to the parent, only to the children. Then you need a node list from the root node to the current node, which can store if a node was already visited or not, too.

Then you go left as deep as possible, storing the path in the node list. This is your first current node. When you need the next one, take its parent (from the node list).  Next would be the right children of that node (if there are any) or its own parent. The best way to understand this is to take a sheet of paper, draw different trees, and look how the traversal must work.


Refactor specs for service class methods Service Class

# services/search.rb

class Search
  include Rails.application.routes.url_helpers
  include ActionView::Helpers::TagHelper

  def initialize(term, app_ids)
    @term = term
    @app_ids = app_ids
  end

  def results
    term = @term
    app_ids = @app_ids

    multi_search = Tire.multi_search do
      no_app_indices = [Event, PastEvent, Review].map {|c| c.tire.index_name}
      search :without_app, indices: no_app_indices do
        query { string term }
      end

      app_indices = [Question, Answer, Link, ServiceProvider].map {|c| c.tire.index_name}
      search :with_app, indices: indices do
        query do
          filtered do
            query { string term }
            filter :terms, app_id: app_ids
          end
        end
      end
    end
    multi_search.results[:with_app].to_a + multi_search.results[:without_app].to_a
  end

  def autocomplete
    results.map do |result|
      title, caption, route = set_item_based_on_type(result)
      {title: strip_tags(title), label: set_label(strip_tags(title), caption), value: route}
    end
  end

  def strip_tags(title)
    title.gsub(%r{</?[^>]+?>}, '').strip
  end

Specs

describe Search do
  before(:all) do
    Question.index.delete
    Question.tire.create_elasticsearch_index

    @question1 = create :question, content: "Some super question?"
    @question2 = create :question, content: "Some extra question?"

    app = create :app, name: "Marketing", id: 76
    @question1 = create :question, content: "Some super question?", app: app
    @question2 = create :question, content: "Some extra question?", app: app

    # wait for indexing to finish
    sleep 1
  end

  describe "#results" do
    it "returns matched results" do
      Search.new("Some", [76]).results.map(&:content).should == ["Some super question?", "Some extra question?"]
    end

    it "excludes not matching results" do
      Search.new("extra", [76]).results.map(&:content).should == ["Some extra question?"]
    end

    it "returns no matches" do
      Search.new("hiper", [76]).results.map(&:content).should == []
    end

    it "should filter results by app" do
      Search.new("some", [77]).results.map(&:content).should == []
    end
  end

  describe "#autocomplete" do
    it "returns array with matching items" do
      Search.new("super", [76]).autocomplete.should ==
        [{
          :title=>@question1.content,
          :label=>"Some super question? Question",
          :value=>"/Marketing/q/#{@question1.id}"
        }]
    end
  end

  describe "#strip_tags" do
    it "should strip title" do
      Search.new("super", [76]).strip_tags("
Some text
 ").should == "Some text"
    end
  end

Is there a better way to test that methods in service class ?

Use let (or let!) to set up your fixtures instead of the instance variables.

let!(:app)        { create :app, name: "Marketing", id: 76 }
let!(:question1)  { create :question, content: "Some super question?" }
let!(:question2)  { create :question, content: "Some extra question?" }
let!(:with_app1)  { create :question, content: "Some super question?", app: app }
let!(:with_app2)  { create :question, content: "Some extra question?", app: app }

Use subject (in combination with let). Here are some examples:

describe "#results" do
  subject { Search.new(keyword, [app_id]).results.map(&:content) }

  context "existing app_id" do
    let(:app_id) { 76 }

    context "multiple matches" do
      let(:keyword) { "Some" }

      its(:size) { should == 2 }
      it { should include(with_app1.content) }
      it { should include(with_app2.content) }
    end

    context "single matches" do
      let(:keyword) { "extra" }

      its(:size) { should == 1 }
      it { should include(with_app2.content) }
    end

    context "no matches" do
      let(:keyword) { "hiper" }

      it { should be_empty }
    end
  end

  context "non-exisiting app_id" do
    let(:keyword) { "some" }
    let(:app_id)  { 123 }

    it { should be_empty }
  end
end

You will end up with more code if you follow this through, but also with more and finer grained examples (each it or its block is a single example).


You hate h̶a̶t̶s̶ reputation? Click here to get rid of it Soo... I recently read a meta-post about the inherent problems of reputation. It listed some things like:

Confirmation bias.
Demotivating for low-rep users.
Hubris and elitism of high-rep users over low-rep users.
Incorrect representation of competence.
...

So I thought to myself: "How would the network be, just without reputation?"

I grabbed the inspector, checked questions, my profile page and the active page and how reputation is displayed.

Then I quickly hacked together the following small user-script (ECMA6) for Greasemonkey that would hide all reputation from sight:

// ==UserScript==
// @name         Reputation Hider
// @namespace    http://gihub.com/Vogel612/ReputationHider
// @version      0.1
// @description  Hides all reputation displays from StackExchange sites
// @author       Vogel612
// @grant        none
// @include      /https?:\/\/(meta\.)?.*\.stackexchange\.com\/.*/
// @include      /https?:\/\/(stackoverflow|serverfault|superuser|askubuntu)\.com\/.*/
// ==/UserScript==
/* jshint -W097 */
'use strict';

let slice = function(collection) {
  return [].slice.call(collection);
}

let repClasses = [ "rep", "reputation", "reputation-score" ];

let hideRepz = function() {
  repClasses.forEach(clazz =>
    slice(document.getElementsByClassName(clazz))
      .forEach(el => {
        // this results in minor inconsistencies in CSS, but alas
        el.style.visibility = 'hidden';
      })
    );
}

hideRepz();
// add event listener for live-updates
document.addEventListener("DOMNodeInserted", hideRepz);

This has been running for a while now for me and I like to say it's a refreshing change. That aside:

It hides all reputation numbers on the page. If you're curious you can still find them of course. Also it looks funny on /questions when the active timers are outdented differently, but that's just my OCD going mad there.
It also checks for new questions you load over the notification.

What can I do better in that userscript?

It would be better to do this with a user style, e.g. using the Stylus extension, like this:

.rep, .reputation, .reputation-score {
    visibility: hidden;
}

One advantage of user styles over scripts is that they're applied as soon as the page starts to load, so you won't see the rep counts flicker briefly while the page is loading.  Also, user styles will automatically apply also to new dynamic content loaded e.g. via Ajax (such as new answers posted while you're viewing the page).

If you insist on using a user script, it's possibly to emulate user styles by using @run-at document-start and injecting a custom style element into the page early, like this:

// ==UserScript==
// @name        Hide Stack Exchange reputation (demo)
// @namespace   http://vyznev.net/
// @description A demonstration of using style injection to emulate user styles
// @author      Ilmari Karonen
// @version     1.0
// @license     Public domain
// @match       *://*.stackexchange.com/*
// @match       *://*.stackoverflow.com/*
// @match       *://*.superuser.com/*
// @match       *://*.serverfault.com/*
// @match       *://*.stackapps.com/*
// @match       *://*.mathoverflow.net/*
// @match       *://*.askubuntu.com/*
// @homepageURL http://codereview.stackexchange.com/a/120100
// @grant       none
// @run-at      document-start
// ==/UserScript==

var css =
    ".rep, .reputation, .reputation-score {\n" +
    "    visibility: hidden;\n" +
    "}\n";

var style = document.createElement('style');
style.textContent = css;
(document.head || document.documentElement).appendChild(style);

I use this method in my Stack Overflow Unofficial Patch (SOUP) user script (which combines a bunch of more or less minor interface bugfixes and improvements together) to let me efficiently apply both CSS-only and JS-based tweaks from a single user script.

One detail worth noting is that styles injected this way will generally end up being included before any actual site style sheets on the page, which means that they will be overridden by site CSS selectors of equal specificity.  In some cases, you may need to use !important and/or various specificity hacks to make sure that your custom styles will take precedence.


Java reflection: Inject data generically and safely I have become interested in reflection and I wrote a class to wrap an Object so that I can access its private instance fields. The code works great. I do not need to wory about the exceptions in the get() and set() methods because I cast the values appropriately through my wrappers accessor/mutators, but I was wondering if there is a better way of dealing with different types?

Example

public class Driver {
    public static void main(String... args) {
        Point p = new Point();

        System.out.printf("Point initialized: %s\n", p);

        PointDecorator mp = new PointDecorator(p);

        mp.setX(2);
        mp.setY(4);

        p = mp.getPoint();

        System.out.printf("Point updated:     %s\n", p);
    }
}

Output

Point initialized: (0, 0)
Point updated:     (2, 4)

As you can see, I have modified the Point Objects fields, although they are inaccessible through traditional method calls. I was able to bypass Java's typechecking which I find both neat and dangerous. Does this mean that anyone can just wrap another persons class, unless it is immutable, and inject data?

Point.java

public class Point {
    private int x, y;

    public Point() {
        this(0, 0);
    }

    public Point(int x, int y) {
        this.x = x;
        this.y = y;
    }

    public String toString() {
        return String.format("(%d, %d)", x, y);
    }
}

PointDecorator.java

import java.lang.reflect.Field;

public class PointDecorator extends Point {

     protected Point point;

    public PointDecorator(Point point) {
        this.point = point;
    }

    public void setX(int x) {
        set("x", x);
    }

    public int getX() {
        return (Integer) get("x");
    }

    public void setY(int y) {
        set("y", y);
    }

    public int getY() {
        return (Integer) get("y");
    }

    public Point getPoint() {
        return point;
    }

    protected Object get(String fieldName) {
        try {
            return field(fieldName).get(point);
        } catch (Exception e) { }

        return null;
    }

    protected void set(String fieldName, Object value) {
        try {
            field(fieldName).set(point, value);
        } catch (Exception e) { }
    }

    private Field field(String fieldName) {
        try {
            Field f = point.getClass().getDeclaredField(fieldName);
            f.setAccessible(true);
            return f;
        } catch (Exception e) { }

        return null;
    }
}

Essentially you are after the answer to:

  Does this mean that anyone can just wrap another persons class, unless it is immutable, and inject data?

And you are not really interested in a review of your code... right? (By the way, your code looks neat, and covers the bases of your 'academic exercise' quite well).

Java reflection is a dangerous process. On the up-side, reflection is regulated by the Java security system, and it can be enabled or disabled depending on the site, situation, and whatever else people are worried about.

So, if someone has code in your JVM, then they have access to the internals of your program (assuming Java has a permissive setting for it's security - which is the 'default'). On the other hand, other languages (C, cough *cough*) have even more of a problem if someone has been able to 'inject' malicious code in to their program.

It is all about relative-ness. Relative to many languages, Java is quite safe, and it has ways of being even safer.

On the other hand, if you let someone untrusted run code on your JVM then you have bigger problems to worry about.... ;-)

For what it's worth, there are easier ways to produce problematic code, like deserializing 'malicious' instances of values, or simply manipulating the JVM classpath before the program launches.

It's all about perspective....


Image pre-processing and segmentation I'm new learning python and image processing with python. For this reason, I took a project called "Classification of breast cancer images with deep learning".

I applied the following techniques: 1)Threshold, 2)K-Means

import cv2
import numpy as np
import matplotlib.pyplot as plt

img = cv2.imread('n001.tif', 0)
_, blackAndWhite = cv2.threshold(img, 100, 255, cv2.THRESH_BINARY_INV)

nlabels, labels, stats, centroids = cv2.connectedComponentsWithStats(blackAndWhite, None, None, None, 8, cv2.CV_32S)
sizes = stats[1:, -1] #get CC_STAT_AREA component
img2 = np.zeros((labels.shape), np.uint8)

for i in range(0, nlabels - 1):
    if sizes[i] >= 100:   #filter small dotted regions
        img2[labels == i + 1] = 255

res = cv2.bitwise_not(img2)

cv2.imwrite('n001New.tif', res)
########################################################################

img = cv2.cvtColor(res, cv2.COLOR_BGR2RGB)

Z = img.reshape((-1,3))
Z = np.float32(Z)

criteria = (cv2.TERM_CRITERIA_EPS + cv2.TERM_CRITERIA_MAX_ITER, 250 , 1.0)

K=2
ret, label, center= cv2.kmeans(Z, K, None, criteria, 250,
                                  cv2.KMEANS_RANDOM_CENTERS)
center = np.uint8(center)
res = center[label.flatten()]
output = res.reshape((img.shape))

plt.imshow(output)

plt.show()

Data set consists of tif extension image: benign, inSitu, invasive and normal

Output images:

I have a few questions:

Are my methods correct?
Are the outputs correct? Do I find the right areas?
Did I use the K-Means algorithm correctly?

I would appreciate it if you could answer the questions above.

Are my methods correct?

That depends on what you want to accomplish. Since you are "learning python and image processing with python", it seems you picked some related methods to explore, which is good. But since your project is called "Classification of breast cancer images with deep learning", and you're not using deep learning, maybe you didn't pick the right methods...

See below for some more concepts.

  Are the outputs correct? Do I find the right areas?

Again, that depends on what your goal is. Do you know what these images represent? If no, then you need to start there. Understand what the image represents, what is relevant in it, and understand what the output of your algorithms will be used for. Then you will be able to answer your question yourself.

It looks like you found mostly the nuclei, the results are quite OK if that is what you're after.

  Did I use the K-Means algorithm correctly?

You've copy-pasted this from the OpenCV tutorial, so it's correct. But it also is a bit redundant, since the values of center are not useful to you in this case. The output label is an image with values 0 and 1, representing background and foreground. You should be able to directly display that (maybe multiply by 255 first).

Also the line where you convert BGR to RGB is redundant, the k-means result will be the same regardless, and you don't need the colors after that.

When dealing with brightfield microscopy, as you are, you might want to consider separating the stains instead of directly using the RGB values. These images are all H&E stain (Hematoxylin and Eosin). These are purple and pink, respectively. Because we're dealing with transmitted light, and the dyes absorb light of specific wavelengths, we see darker pixels of specific colors. The absorption is characterized by the Beer-Lambert law. This law states that the amount of light transmitted (and thus seen by the camera) is given by \$10^{-A}\$, with \$A\$ proportional to the amount of dye. Thus, given a pixel value \$v\$ and the illumination intensity \$v_\text{max}\$ (the whitest area in the image), you can compute \$-log(v/v_\text{max})\$, which is proportional to the amount of dye. You can do this separately for each of the channels R, G and B. Each dye has a different absorption coefficient for each channel. If you know these (you can compute them from the image data), you can now do a linear unmixing (solve a linear set of equations) to derive, for each pixel, the relative amount of Hematoxylin and the relative amount of Eosin at that location on the slide. These two values is what you should be working with.


Check smsc type of batch and submits it to the proper queue Here, the for loop loops over a arraylist and checks smsc type of batch and submits it to the proper queue.

Is there a way to rewrite this piece of code in a better way?  Is there any way to eliminate the if/else?

for (Batch batch : input.getBatches()){
ArrayList<Messages> arrSentsms = batch.getlist();
String smsc =batch.getSmsc();
    if (smsc.equals("v")) {
            SubmitToQueueV.sendJMSMessage(arrSentsms, priority);
        } else if (smsc.equals("s")) {

            SubmitToQueueS.sendJMSMessage(arrSentsms, priority);
        } else if (smsc.equals("fake1")) {

            SubmitToQueueFake.sendJMSMessage(arrSentsms, priority);
        } else if (smsc.equals("con")) {

            SubmitToQueueCon.sendJMSMessage(arrSentsms, priority);

        } else if (smsc.equals("ce")) {

            SubmitToQueueCEcns.sendJMSMessage(arrSentsms, priority);

        } else if (smsc.equals("cli")) {

            SubmitToxQueueCli.sendJMSMessage(arrSentsms, priority);

        } else if (smsc.equals("pel")) {
            SubmitTQueuePel.sendJMSMessage(arrSentsms, priority);

        } else {
            LOG.debug("smsc" + " smsc not found");
        }
    }
}

I assume you have (or can easily extract) an interface JMSQueue for this answer.
That's not really an unreasonable assumption I'd say, if you need help with that just drop me a note.

You're using the wrong tool here. What you have is a "mapping" between a string and a certain JMS-Queue.
Let's put that into code:

private Map<String, JMSQueue> queues = new HashMap<>();

// [...]
for (Batch batch : input.getBatches()) {
    JMSQueue responsibleQueue = queues.get(batch.getSmsc());
    if (responsibleQueue == null) {
        // queue not found. Log or whatever and then do the next batch
        continue;
    }
    responsibleQueue.sendJMSMessage(batch.getList(), priority);
}

Note that I've done some small changes on your code:

Respect indentation and whitespace around operators. Your IDE can help you with that.
Avoid unnecessary variables. They only make it harder to understand the code. Don't have everything inlined, but exercise care when creating variables.
Respect the naming conventions (talking about getlist)


Initialization of Extended class without too much overhead? I am getting object of a class AAA from somewhere and I want to add more information in that object. So, I am creating a new class BBB which is derived from AAA. The class BBB has additional field dictionary. I am populating this dictionary in derived class constructor which is taking the Class AAA object and array of item which I want to use as keys of dictionary and values of this dictionary are elements of a field of object of class AAA. I tried to create similar scenario in blow example code:

void Main(){
     A obj = new A () ;
     obj.prop1 =  new int [] {5 ,10, 15}   ;
     obj.prop2 =  "Hello" ;
     obj.prop3 = "World" ;
//   obj.Dump () ;
     B obj2 = new B (new int [] {1,2,3}, obj)  ;
//   obj2.Dump () ;

}

// Define other methods and classes here
public class A {
    public int [] prop1 ;
    public string prop2 ;
    public string prop3 ;
}

public class B : A {
    public Dictionary <int, int> prop4 ;
    public B (int [] keys, A a) {
    prop4 = new Dictionary <int, int> () ;
        if (keys.Length == a.prop1.Length ) {
            for (int i = 0 ; i < keys.Length ; i++ ) {
                prop4.Add (keys[i], a.prop1[i]) ;
            }
            // is there a way to obsolete below lines of code???
            this.prop1 = a.prop1 ;
            this.prop2 = a.prop2 ;
            this.prop3 = a.prop3 ;
        }
        else {
            throw new Exception ("something wrong") ;
        }
    }
}

In derived class constructor, I am filling the properties manually and I do not want to do it. Is there another way to do it. I have more than 20 properties in my actual class.

Those lines are managing the properties of A, so they belong in A.  I've added a protected copy constructor and used constructor chaining to solve your issue:

public class A
{
  public int [] prop1 ;
  public string prop2 ;
  public string prop3 ;

  public A() {}

  protected A(A a)  //copy constructor
  {
    this.prop1 = a.prop1 ;  //review: should this reference be copied, or a new copy made?
    this.prop2 = a.prop2 ;
    this.prop3 = a.prop3 ;
  }
}

public class B : A
{
  public Dictionary <int, int> prop4 ;

  public B (int [] keys, A a) : base(a)  //constructor chaining
  {
    prop4 = new Dictionary <int, int> () ;
    if (keys.Length == a.prop1.Length )
    {
      for (int i = 0 ; i < keys.Length ; i++ )
      {
        prop4.Add (keys[i], a.prop1[i]) ;
      }
    }
    else
    {
      throw new Exception ("something wrong") ;
    }
  }
}