Depth-first Search Non-Reursive Implementation in Clojure I've created a non-recursive dfs implementation in Clojure. I'm using the wikipedia article on dfs as a guide. I'm very new to Clojure and functional programming in general. Any tips to improve readability, make the code more idiomatic, or improve performance would be very helpful. I'm aware there is already question about a recursive dfs implementation in Clojure, but I feel like this code is different enough to warrant a separate post.

(def graph {'a '(b c e) 'b '(a d f) 'c '(a g) 'd '(b) 'e '(a f) 'f '(b e)})

(defn label
  [v discovered]
  (cons v discovered))

(defn get-edges
  [g v]
  (get g v))

(defn labeled?
 [v discovered]
  (cond
    (some #(= v %) discovered) true
    :else false))

(defn build-stack
 [S edges]
  (concat S edges))

(defn dfs
  [g v]
   (loop
    [discovered '() S (cons v ())]
     (cond
      (empty? S) (reverse discovered)
      (labeled? (last S) discovered) (recur discovered (butlast S))
      :else
       (recur
         (label (last S) discovered)
         (build-stack (butlast S) (get-edges g (last S)))))))

From the top ...

The label function is a synonym for cons, as is build-stack for
concat. Drop them.
The get-edges function doesn't get edges. It gets the neighbouring
vertices. Call it neighbours, or some such.

The labeled? function:

It determines whether the first argument is a member of the
collection second argument. So call it something generic like
includes?.
Swap the arguments around, so that it reads more naturally.
Get rid of the explicit falses and trues. You usually can.

We end up with

(defn includes?
  [coll x]
  (some #(= x %) coll))

Now for the dfs function itself.

Your local S is, as its name suggests, a stack. Use the other end
of it - the front of the list. This gets rid of the slow last and
butlast. You can't use Clojure's stack lingo - peek, pop, and
conj - because there's a concat in there. So use first, next,
and cons instead.
If you use a vector instead of a list to accumulate discovered, you
don't need the final reverse.
(cons v ()) is just (list v)

The result is

(defn dfs
  [g v]
  (loop [discovered [], S (list v)]
    (cond
      (empty? S) discovered
      (includes? discovered (first S)) (recur discovered (rest S))
      :else (recur
              (conj discovered (first S))
              (concat (neighbours g (first S)) (rest S))))))

There's still a performance problem. Whenever it deals with a potentially new vertex, it uses includes? to do a linear search of discovered. For a graph of n vertices, this takes of the order of n^2 time.

The answer is to hold the discovered data twice:

as a vector, for returning the answer; and
as a set, for testing for presence.

This roughly doubles the amount of memory required, but reduces time required enormously (roughly to order of n).

I'll leave you to make the necessary changes to dfs.


Checking proper English sentences of user's input with grammar using C++ Write a program that checks if a sentence is correct according to the
  “English” grammar in §6.4.1. Assume that every sentence is terminated
  by a full stop (.) surrounded by whitespace. For example, birds fly
  but the fish swim . is a sentence, but birds fly but the fish swim
  (terminating dot missing) and birds fly but the fish swim. (no space
  before dot) are not. For each sentence entered, the program should
  simply respond “OK” or “not OK.” Hint: Don’t bother with tokens; just
  read into a string using >>.

#include <string>
#include <iostream>
using namespace std;

string str;

void putback(string s) {
    str = s;
}

string verb() {
    if (str == "rules" || str == "fly" || str == "swim") {
        return str + ' ';
    }
    else
        return "";
}

string noun() {
    if (str == "birds" || str == "fish" || str == "C++") {
        return str + ' ';
    }
    else
        return "";
}

string article() {
    if (str == "the") {
        string temp = str;
        cin >> str;
        return temp + ' ';
    }
    else
        return "";
}

string conjunction() {
    if (str == "and" || str == "or" || str == "but") {
        return str + ' ';
    }
    else
        return "";
}

string l;
string sentence() {
    l += article();
    if (noun() == "") {
        cerr << "NOT OK";
        return "";
    }
    else {
        l += noun();
        cin >> str;
        string temp = verb();
        if (temp == "") {
            cerr << "NOT OK";
            return "";
        }
        else {
            l += temp;
            cin >> str;
            if (str == "") {
                cerr << "NOT OK";
                return " ";
            }
            else if (str == ".") {
                l += ".";
                return l;
            }
            else {
                if(conjunction() == "") {
                    cerr << "NOT OK";
                }
                else {
                    l += str + ' ';
                    cin >> str;
                }
                return sentence();
            }
        }
    }
}

int main()
try
{
    string sen = " ";
    while (cin) {
        cin >> str;
        sen = sentence();
        if (str == ".") {
            cout << sen << '\n';
        }
    }
    //keep_window_open();
}
catch (exception& e)
{
    cerr << "error: " << e.what() << '\n';
    //keep_window_open();
    return 1;
}
catch (...)
{
    cerr << "Oops: unknown exception!\n";
    //keep_window_open();
    return 2;
}

The Grammar:
Sentence:
          Noun Verb                                   // e.g., C++ rules
          Article Noun Verb
          Sentence Conjunction Sentence   // e.g., Birds fly but fish swim
Conjunction:
          "and"
          "or"
          "but"
Article:
          "the"
Noun:
          "birds"
          "fish"
          "C++"
Verb:
          "rules"
          "fly"
          "swim"

This is perfectly working code. Is there anything I can improve on for my code? Do you think using global variables in this case is okay? Even for whether I have proper variable names is worth commenting. This question is from this webpage, exercise 6 at the very bottom.

You could learn how to use bison. It designed specifically for this.

Boiler plate for flex/bison project:

Eng.l

%option c++

%{
#define IN_EXPR_LEXER
#include "EngYac.hpp"
%}

SpaceChar     [ \t]
Space         {SpaceChar}+
SpaceTerm     {SpaceChar}|<<EOF>>
FullStop      {SpaceChar}\.{SpaceTerm}

%%

and                     {return yy::parser::token::Conjunction;}
or                      {return yy::parser::token::Conjunction;}
but                     {return yy::parser::token::Conjunction;}

the                     {return yy::parser::token::Article;}

birds                   {return yy::parser::token::Noun;}
fish                    {return yy::parser::token::Noun;}
C\+\+                   {return yy::parser::token::Noun;}

rules                   {return yy::parser::token::Verb;}
fly                     {return yy::parser::token::Verb;}
swim                    {return yy::parser::token::Verb;}

{FullStop}              {return yy::parser::token::FullStop;}

{Space}                 {/*Ignore*/}
.                       {std::cerr << "Error unknown character\n";}

%%

int yyFlexLexer::yywrap(void)
{
    return 1;
}

int yylex(int* /*type*/, yyFlexLexer& lexer)
{
    return lexer.yylex();
}

Eng.y

%{
    #ifndef IN_EXPR_LEXER
    #include <FlexLexer.h>
    #endif
    int yylex(int* type, yyFlexLexer& lexer);
%}
%skeleton "lalr1.cc"
%defines
%parse-param {yyFlexLexer& lexer}
%lex-param   {yyFlexLexer& lexer}

%token      Noun
%token      Verb
%token      Article
%token      Conjunction
%token      FullStop

%%
Input:      Sentence FullStop

Sentence:   Noun Verb
        |   Article Noun Verb
        |   Sentence Conjunction Sentence

%%

void yy::parser::error(yy::location const&, std::string const& msg)
{
    std::cerr << "Error: " << msg << "\n";
}

main.cpp

#include <FlexLexer.h>
#include "EngYac.hpp"

int main()
{
    yyFlexLexer     lexer(&std::cin, &std::cout);
    yy::parser      parser(lexer);

    std::cout << (parser.parse() ? "FAIL" : "OK") << "\n";
}

Makefile

FLEX_SRC    = $(wildcard *.l)
BISON_SRC   = $(wildcard *.y)
FLEX_CPP    = $(patsubst %.l, %Lex.cpp, $(FLEX_SRC))
BISON_CPP   = $(patsubst %.y, %Yac.cpp, $(BISON_SRC))

SRC         = $(filter-out $(BISON_CPP), $(filter-out $(FLEX_CPP), $(wildcard *.cpp))) $(FLEX_CPP) $(BISON_CPP)
OBJ         = $(patsubst %.cpp,%.o,$(SRC))

eng:       $(OBJ)
    g++ -o eng -O3 -Wall -Wextra -Werror $(OBJ)

clean:
    rm EngLex.cpp EngYac.cpp EngYac.hpp location.hh position.hh stack.hh $(OBJ) expr

$(OBJ): $(FLEX_CPP) $(BISON_CPP)

%Lex.cpp:       %.l
    flex -o $*Lex.cpp $^

%Yac.cpp:       %.y
    bison -o $*Yac.cpp $^

Usage:

> make
> echo "birds rules but fish swim." | ./eng

OK
> echo "A test of English." | ./eng
A test of English
Error: syntax error
FAIL


Optimised Python Code gives TLE for PRIME1 PRIME1 is a codechef problem which states:

  Shridhar wants to generate some prime numbers for his cryptosystem.
  Help him! Your task is to generate all prime numbers between two given
  numbers.

  Input

  The first line contains t, the number of test cases (less then or
  equal to 10). Followed by t lines which contain two numbers m and n (1
  <= m <= n <= 1000000000, n-m<=100000) separated by a space.

  Output

  For every test case print all prime numbers p such that m <= p <= n,
  one number per line. Separate the answers for each test case by an
  empty line.

I have optimised my code according to suggestions given here. But, my edits were rolled back when I tried to post my optimised code. So, I am still getting a TLE error and I don't understand why. Help me optimise this code further:

import math
def sieve_basic_list(list_prime):
    max_n=10**9
    m = int(math.sqrt(max_n))
    lim = int(math.sqrt(m))+1
    arr = [True]*(m+1)
    arr[0]=arr[1]=False

    #Calculate primes upto m only so loop runs sqrt(m) times.
    for x in xrange(2,lim):
        if arr[x]==False:
            continue
        for i in xrange(x*x,m+1,x):
            arr[i]=False
    for i,each in enumerate(arr):
        if each:
            list_prime.append(i)

def mnsieve(list_prime):
    #program tc input and mnrange calc
    for _ in xrange(int(raw_input().strip())):
        m,n = map(int,raw_input().split())
        mnrange = [True]*(n-m+1)
        mnrange[0] = False if m==1 else True
        for each in list_prime:
            firstfactor = ((m-1)/each)*each #firstfactor nearest to m i.e firstfactor-m gives index
            for x in xrange(firstfactor,n+1,each):
                try:
                    if x not in list_prime:
                        mnrange[x-m]=False
                except:
                    pass
        for i,each in enumerate(mnrange):
            if each:
                print i+m
        print

list_prime=[]
sieve_basic_list(list_prime)
mnsieve(list_prime)

Finally, I solved the problem. And these suggestions helped. Here is the final solution:

import math
def sieve_basic_list(list_prime):
    max_n=10**9
    m = int(math.sqrt(max_n))
    lim = int(math.sqrt(m))+1
    arr = [True]*(m+1)
    arr[0]=arr[1]=False

    #Calculate primes upto m only so loop runs sqrt(m) times.
    for x in xrange(2,lim):
        if arr[x]==False:
            continue
        for i in xrange(x*x,m+1,x):
            arr[i]=False
    for i,each in enumerate(arr):
        if each:
            list_prime.add(i)

def mnsieve(list_prime):
    #program tc input and mnrange calc
    for _ in xrange(int(raw_input().strip())):
        m,n = map(int,raw_input().split())
        mnrange = [True]*(n-m+1)
        mnrange[0] = False if m==1 else True
        for each in list_prime:
            if each <= int(n**0.5): #important optmisation
                firstfactor = ((m-1)/each)*each #firstfactor nearest to m i.e firstfactor-m gives index
                for x in xrange(firstfactor,n+1,each):
                        if x!=each and x-m >= 0: #x-m can become -ve and that is disastorous in python lists!
                            mnrange[x-m]=False
       for i,each in enumerate(mnrange):
            if each:
                print i+m
       print

list_prime=set([]) #important optimisation
sieve_basic_list(list_prime)
mnsieve(list_prime)

Check it here: on codechef


Change the navigation ul li based on the clicked country I'm trying to create a very simple jQuery plugin in the object-oriented way. Now, I'm not sure whether the code I produced is correct and efficient OO programming. The plugin's aim is to change the navigation ul  li based on the clicked country.

jsFiddle

// Widget container
(function($) {
// Widget container plugin
    $.fn.myWidget = function () {
        this.each (function () {
            // Vars
            var item = $ (this);

            // Set events
            item.click (function (e) {
                if (e) e.preventDefault ();
                combine_all(item);
            });
        });
    };

    //uk navigation
    var uk_navigation = '
'
        +'
UK stuff
'
        +'
UK phones
'
        +'
';

    //global navigation
    var global_navigation = '
'
        +'
EU stuff
'
        +'
EU phones
'
        +'
';

    //italy navigation
    var italy_navigation = '
'
        +'
Italy stuff
'
        +'
Italy phones
'
        +'
';

    //germany
    var germany_navigation = '
'
        +'
Germany stuff
'
        +'
Germany phones
'
        +'
';

    this.get_region = function(item){
        my_region = $ (item).attr ('id');
        return my_region;
    };

    this.make_active = function(item){
        item.each(function(){
            item.siblings().removeClass('flag-active');
        });
        item.addClass('flag-active');
    }

    this.update_navigation = function(my_region){
        var old_navigation = $('#navigation').find('ul');
        var new_navigation;
        if(my_region==='europe_flag'){
            new_navigation = global_navigation;
        }else if(my_region==='uk_flag'){
            new_navigation = uk_navigation;
        }else if(my_region==='germany_flag'){
            new_navigation = germany_navigation;
        }else{
            new_navigation = italy_navigation;
        }

        $(old_navigation).hide().html(new_navigation).fadeIn(800);

    };

    this.combine_all = function(item){
        this.get_region (item);
        this.update_navigation(my_region);
        this.make_active(item);
    };

})(jQuery);

// Main
$(function() {
    $('#flags').find('div').myWidget();
});

From a once over:

Using each seems overkill, this works:

$.fn.myWidget = function () {
    this.click( function (e) {
        if (e) {
            e.preventDefault ();
        }
        combine_all(this);
    });
};

This code assigns the same function to all elements in this, the function has this as the clicked element, so that is why you provide to combine_all
JavaScript uses lowerCamelCase so combine_all -> combineAll, also combineAll is not enlightening as function names go
Your code is repetitive to determine the links, for starters you could do something like this:

var country_links = {
  uk     : [ 'UK stuff', 'UK phones' ]
  europe : [ 'EU stuff', 'EU phones' ]
  italy  : [ 'Italy stuff', 'Italy phones' ]
  germany: [ 'Germany stuff', 'Germany phones' ]
};

 this.updateNavigation = function(my_region){
      var oldNavigation = $('#navigation').find('ul'),
          menuItems = country_links[ my_region.split('_')[0] ];
          newNavigation = '
';
       for( var i = 0 ; i < menuItems.length ; i ++ ){
           newNavigation += '
' + menuItems[i] + '
';
       }
       newNavigation += '
'

      $(old_navigation).hide().html(new_navigation).fadeIn(800);
 };

This is not the only approach, but you split data from style, country_links has all the data. updateNavigation knows how to build a list from that. I did not test this code but you should catch my drift.
To finish on the first point, I would just call all three functions on top instead of combineAll


Rotating contents of a matrix (vector) to the left or right Each vector element is a row and each character is a column.
Assume the matrix is square (N*N) and N is < 10.

I have some code to do it but it doesn't look great and would love some feedback on how to make it better. You can use any function of the C++ standard library. The only constraint is it can't be in-place. I'm looking to improve it in style and any optimisations needed.
enum DIRECTION
{
    DIR_RIGHT = 0,
    DIR_LEFT = 1
};

vector<string> rotate(const vector<string>& original, DIRECTION direction)
{
    vector<string> rotated(original);
    for (vector<string>::const_iterator irow = original.begin(); irow != original.end(); ++irow) {
        int new_col = direction == DIR_RIGHT ? (original.size() - 1) - (irow - original.begin())
                                             : (irow - original.begin());
        for (string::const_iterator icol = (*irow).begin(); icol != (*irow).end(); ++icol) {
            int new_row = direction == DIR_RIGHT ? (icol - (*irow).begin())
                                                 : ((*irow).size() - 1) - (icol - (*irow).begin());
            rotated[new_row][new_col] = *icol;
        }
    }
    return rotated;
}

Example input:
"###"
"oho"
"ooo"

Rotating right should output:
"oo#"
"oh#"
"oo#"

P.s. For convenience I've included using namespace std;, the final code uses the namespace explicitly E.g. std::vector.

I didn't look very closely at the algorithm, so I'll trust that it is correct ;)

Some stylistic changes you could do are:

Use auto to clean up those long iterator names.
Break long lines. Shorter lines are usually easier to digest.
DIRECTION is a bit unusual for the name of an enum type. That notation is normally used for the enum constants, the type usually goes with the same notation used for classes and structs (CamelCase, in most projects).
Use const as much as possible. Making the integer indexes const in this case won't change much, but making things that are not supposed to change const is a good habit, so you should aways try to do it, even for small sections of code. You never know when that tiny function might grow.

Following is a snippet of code with the above changes applied:

enum Direction
{
    DIR_RIGHT = 0,
    DIR_LEFT  = 1
};

std::vector<std::string> rotate(const std::vector<std::string>& original, Direction direction)
{
    std::vector<std::string> rotated(original);

    for (auto irow = original.cbegin(); irow != original.cend(); ++irow) {

        const auto new_col = (direction == DIR_RIGHT) ?
            (original.size() - 1) - (irow - original.cbegin()) :
            (irow - original.cbegin());

        for (auto icol = (*irow).cbegin(); icol != (*irow).cend(); ++icol) {

            const auto new_row = (direction == DIR_RIGHT) ?
                (icol - (*irow).cbegin()) :
                ((*irow).size() - 1) - (icol - (*irow).cbegin());

            rotated[new_row][new_col] = *icol;
        }
    }

    return rotated;
}

Side note: You might also want to check out std::distance() to replace the iterator diferences you are computing with operator -.

Also, by using cbegin()/cend(), you can ensure the returned iterators are of type const_iterator and thus pointing to immutable data.


Prime sieve generator I wrote a simple prime sieve in Python. But it ended up looking... ugly. I included a few of the most basic optimizations. When posting this question SE suggested a bunch of questions on the same topic, which lead me to some improvements. But few were in generator form.

It's the optimizations' fault.
Unfortunately I can't continue an outer loop from an inner loop, like I can in JavaScript. I very much disagree with Guido here, as it makes the inner loop clunky.

from itertools import count

def sieve():
    primes = [2]
    for candidate in count(start=3, step=2):
        cont = False
        n = 0
        while primes[n]**2 <= candidate:  # You only need to check up to the square root of a number.
            if candidate % primes[n] == 0:
                cont = True  # outer
                break
            n = n + 1
        if cont:
            cont = False
            continue
        yield primes[-1]
        primes.append(candidate)

Can we make this more concise without changing the basic logic?
For example this is extremely concise, but it doesn't have some of the optimizations my code does.

For fun, I wrote the same logic in Javascript. This looks cleaner due to being able to continue an outer loop. But the lack of negative indices is a step backwards.

  function* sieve() {
    let primes = [2]
    counter: for (let candidate = 3;; candidate+=2) {
        for (let n = 1; primes[n]**2 <= candidate; n++)
            if (candidate % primes[n] == 0)
                continue counter
        yield primes[primes.length - 1]
        primes.push(candidate)
    }
}

We can move the while loop into another function.

def is_prime(candidate, primes):
    n = 0
    while primes[n]**2 <= candidate:
        if candidate % primes[n] == 0:
            return False
        n += 1
    return True

def sieve():
    primes = [2]
    for candidate in count(start=3, step=2):
        if not is_prime(candidate, primes):
            continue
        yield primes[-1]
        primes.append(candidate)

We can use itertools.takewhile to expose the while loop as an iterable.

def is_prime(candidate, primes):
    for prime in itertools.takewhile(lambda p: p**2 <= candidate, primes):
        if candidate % prime == 0:
            return False
    return True

We can use any to make is_prime easier to read.

If for any of the values, candidate % prime == 0, are true the result is true.
If none of them are then it is false.
Since we want it to be the other way around we can just use not.

def is_prime(candidate, primes):
    return not any(
        candidate % prime == 0
        for prime in itertools.takewhile(lambda p: p**2 <= candidate, primes)
    )

We can move is_prime back into the first function.

def sieve():
    primes = [2]
    for candidate in count(start=3, step=2):
        if not not any(
            candidate % prime == 0
            for prime in itertools.takewhile(lambda p: p**2 <= candidate, primes)
        ):
            continue
        yield primes[-1]
        primes.append(candidate)

We can swap the if to cancel the continue and a not.
We can swap the any and == 0, with not all.
We can cancel the double not.

def sieve():
    primes = [2]
    for candidate in count(start=3, step=2):
        if all(
            candidate % prime
            for prime in itertools.takewhile(lambda p: p**2 <= candidate, primes)
        ):
            yield primes[-1]
            primes.append(candidate)

At the expense of readability you can get the following trade offs.

There is a chance that using int(candidate ** 0.5).__ge__ is faster than the lambda.


Asynchronous task on every element of an array Here is some synchronous code:

let barsClient = [{
    id: 1,
    name: "The Shindig",
    goingTonight: []
  },
  {
    id: 2,
    name: "Steakapolis",
    goingTonight: []
  },
  {
    id: 3,
    name: "Booty Town",
    goingTonight: []
  }
];

let barsDB = [{
  id: 2,
  name: "Steakapolis",
  goingTonight: ["Joe", "Jim", "Joey"]
}];

function findById(bar) {
  return barsDB.find((dbBar) => {
    return dbBar.id === bar.id;
  });
}

// Check to see if bar is present in barsDB
for (let bar of barsClient) {
  let result = findById(bar);
  // If bar is not in barsDB, add it
  if (!result) {
    barsDB.push(bar);
    // If it is present, update its goingTonight property
  } else {
    bar.goingTonight = result.goingTonight;
  }
}

// Both barsClient and barsDB should contain the same data, albeit not necessarily in the same order
console.log("barsClient:", barsClient);
console.log("barsDB:", barsDB);

Now, imagine that the findById function was asynchronous? It took me about six hours of Googling, reading and fiddling to finally figure this one out and I'm pretty happy with myself. I'm just wondering if my solution is in accordance with best practices. Here is the code to review:

let barsClient = [{
    id: 1,
    name: "The Shindig",
    goingTonight: []
  },
  {
    id: 2,
    name: "Steakapolis",
    goingTonight: []
  },
  {
    id: 3,
    name: "Booty Town",
    goingTonight: []
  }
];

let barsDB = [{
  id: 2,
  name: "Steakapolis",
  goingTonight: ["Joe", "Jim", "Joey"]
}];

function findById(bar) {
  return barsDB.find((dbBar) => {
    return dbBar.id === bar.id;
  });
}

function findByIdAsync(bar) {
  var promise = new Promise(function(resolve, reject) {
    setTimeout(function() {
      resolve(findById(bar));
    }, 3000);
  });
  return promise;
}

Promise.all(barsClient.map((bar, i) => {
    return findByIdAsync(bar)
      .then(function(result) {
        if (!result) {
          barsDB.push(bar);
        } else {
          bar.goingTonight = result.goingTonight;
        }
      });
  }))
  .then(() => {
    console.log("barsClient:", barsClient);
    console.log("barsDB", barsDB);
  });

From reading it I think it makes more sense to put:

.then(function(result) {
        if (!result) {
          barsDB.push(bar);
        } else {
          bar.goingTonight = result.goingTonight;
        }
      });

Into the promise that is actually executing that code:

function findByIdAsync(bar) {
  return new Promise(function(resolve, reject) {
    setTimeout(function() {
      const result = findById(bar)
      if (result) {
        bar.goingTonight = result.goingTonight;
        resolve("Successfully updated bar")
      } else {
        barsDB.push(bar);
        resolve("Successfully added new bar")
      }
    }, 3000);
  });
}

Then you can just call the array of promises as so:

Promise.all(barsClient.map(findByIdAsync))
  .then(() => {
    console.log("barsClient:", barsClient);
    console.log("barsDB", barsDB);
  });

It seems that the concerns are more aligned this way.


Using System.Security.Cryptography.ProtectedData, do you see any issue with encryption and encoding? The application needs to keep a secret which is known to logged in windows user. User enters secret in windows form's text box which is read by application. This is in form of System.String. Now I have to persist this secret into a XML file. And later able to decipher it back and use it.

Questions:

Do you see any concern with encoding done below?
Is there any concern with using System.String to begin with? I was
considering to use System.Security.SecureString but at some point
down the line the application needs to have base underlying string
in clear text.

I'm using System.Security.Cryptography.ProtectedData to do the encryption.

The code

using System.Security.Cryptography;

static public string Encrypt(string password, string salt)
{
    byte[] passwordBytes = Encoding.Unicode.GetBytes(password);
    byte[] saltBytes = Encoding.Unicode.GetBytes(salt);

    byte[] cipherBytes = ProtectedData.Protect(passwordBytes, saltBytes, DataProtectionScope.CurrentUser);

    return Convert.ToBase64String(cipherBytes);
}

static public string Decrypt(string cipher, string salt)
{
    byte[] cipherBytes = Convert.FromBase64String(cipher);
    byte[] saltBytes = Encoding.Unicode.GetBytes(salt);

    byte[] passwordBytes = ProtectedData.Unprotect(cipherBytes, saltBytes, DataProtectionScope.CurrentUser);

    return Encoding.Unicode.GetString(passwordBytes);
}

Typical usage

[TestMethod()]
public void EncryptDecryptTest()
{
    string password = "Gussme!";
    string salt = new Random().Next().ToString();

    string cipher = Authenticator.Encrypt(password, salt);
    Assert.IsFalse(cipher.Contains(password), "Unable to encrypt");
    Assert.IsFalse(cipher.Contains(salt), "Unable to encrypt");

    string decipher = Authenticator.Decrypt(cipher, salt);
    Assert.AreEqual(password, decipher);
}

Your code is correct. I usually use Encoding.UTF8 because it is more compact for typical character sets.
SecureString is kind of a useless class. It is meant to never keep a secret unencrypted even in memory. This would only help if an attacker could read your processes memory. If he can do that you have lost already because he can read every character of your password the moment you use it or add it to the SecureString. Anyway, what attack scenario on earth allows an attacker to read your webserver process memory and not do other more dangerous things at the same time? In that case your box is probably compromised entirely. So you are right not using this class.


keep track of friends and number of days not spoken to, dynamic memory location Write a program that lets users keep track of the last time they talked to each of their friends. Users should be able to add new friends (as many as they want!) and store the number of days ago that they last talked to each friend. Let users update this value (but don't let them put in bogus numbers like negative values). Make it possible to display the list sorted by the names of the friends of by how recently it was since they talked to each friend.

This is the code I have written so far. I'm having problems with the array size not growing as it should do. Also I'm unsure about how to go about list the names with numbers of days out.

#include <iostream>
#include <string>

using namespace std;

int *growArray (int * friends, int * size)
{
    *size *= 2;
    int *new_friends = new int[*size];
    for (int i=0; i < *size; i++) {
        new_friends[i] = friends[i];
    }
    delete [] friends;
    return new_friends;
}

string *growArray2 (string * name1, int * size)
{
    *size *= 2;
    string *new_name2 = new string[*size];
    for (int i=0; i < *size; i++) {
        new_name2[i] = name1[i];
    }
    delete [] name1;
    return new_name2;
}

void printArray (string *name1, int *friends,int size, int element_set)
{
    cout << "the total size of the array is: " << size << endl;
    cout << "Values in the array: " << endl;
    for (int i = 0; i < element_set; ++i)
    {
        cout << name1[i] << "[" << i << "] = " << friends[i] << " days since you last spoke to them" << endl;
    }
}

int main()
{
    string exit = "X";
    string name;
    int noDays;
    int next_element = 0;
    int size = 4;
    int *friends = new int[size];
    string *name1 = new string [size];
        cout << "Please enter your friends name" << endl;
        cin >> name;
        cout << "Please enter no of days you last spoke to them" << endl;
        cin >> noDays;
    while(true) {
        if (size == next_element+1) {
            friends = growArray(friends, &size);
            name1 = growArray2(name1, &size);
        }
        friends[next_element] = noDays;
        name1[next_element] = name;
        next_element++;
        printArray(name1, friends, size , next_element);
        cout << "Please enter your friends name or X to exit" << endl;
        cin >> name;
        if (name == exit) {
            break;
        }
        cout << "Please enter no of days you last spoke to them" << endl;
        cin >> noDays;

        }
        delete name1;
        return 0;
}

First of all the "growArray" function will never be called since you put it inside an if block that runs only once. when the if statement is evaluated your size is 10 and your next_element+1 is 1 and you never check for (size == next_element+1) again.


Adapting C# 2.0 event code for C# 5.0 I came across this excellent post on delegates and event and tried to adapt it to C# 5.0L

namespace Bla
{
    public delegate void WorkStarted();
    public delegate void WorkProgressing();
    public delegate Task<int> WorkCompleted();

    public class Worker
    {
        public event WorkStarted Started;
        public event WorkProgressing Progressing;
        public event WorkCompleted Completed;

        public void DoWork()
        {
            Console.WriteLine("Worker: work started");
            if (Started != null)
                Started();

            Console.WriteLine("Worker: work progressing");
            if (Progressing != null)
                Progressing();

            Console.WriteLine("Worker: work completed");
            if (Completed == null) return;
            foreach (var del in Completed.GetInvocationList())
            {
                var wc = (WorkCompleted) del;
                var wc2 = wc;
                wc.BeginInvoke(delegate(IAsyncResult result)
                {
                    var grade = wc2.EndInvoke(result);
                    Console.WriteLine("Worker grade = {0}", grade.Result);
                },
                    null);
            }
        }
    }

    public class Boss
    {
        public async Task<int> WorkCompleted()
        {
            await Task.Delay(TimeSpan.FromSeconds(3));
            Console.WriteLine("Better...");
            return 5; // out of 10
        }
    }

    public class Universe
    {
        public static void WorkerStartedWork()
        {
            Console.WriteLine("Universe notices worker starting work");
        }

        public static async Task<int> WorkerCompletedWork()
        {
            await Task.Delay(TimeSpan.FromSeconds(10));
            Console.WriteLine("Universe pleased with worker's work");
            return 7;
        }

        private static void Main()
        {
            var peter = new Worker();
            var boss = new Boss();
            peter.Completed += boss.WorkCompleted;
            peter.Started += WorkerStartedWork;
            peter.Completed += WorkerCompletedWork;
            peter.DoWork();

            Console.WriteLine("Main: worker completed work");
            Console.ReadLine();
        }
    }
}

Could this be further improved?

There are indeed some things lacking to call this C#5 code: your delegates are not necessary.

The first two are easy: remove the delegate, make the event return EventHandler instead, call the event with Started(this, EventArgs.Empty) and change the method signature accordingly. This results in this:

public event EventHandler Started;

if (Started != null)
{
    Started(this, EventArgs.Empty);
}

public static void WorkerStartedWork(object sender, EventArgs e)
{
    Console.WriteLine("Universe notices worker starting work");
}

The same thing goes for Progressing.

The Completed event is more difficult but only because you made it so. In your Completed event you have 2 consumers at this point. All you have to do is change the signature to EventHandler, change the calling to

if(Completed != null)
{
    Completed(this, EventArgs.Empty);
}

And change all handlers to

public async void WorkCompleted(object sender, EventArgs e) { }

Now it will call every consumer and you don't have to mess around by looking at the invocation list anymore

Don't use the universe statically. Combined with events, that is just bound to cause memory problems. Instead I would make it an instance just like Worker and Boss and then connect its handlers to peter.

By making these changes to the WorkCompleted handlers you'll notice that you can't use Task anymore as a return type. That's true, events are forced to adhere to their void X(object, T) where T : EventArgs definition. Because of this, events are the only place where it is allowed to use async void.

This will cause your events to become fire-and-forget but there are ways around that as well if you don't want this.

Lastly: you now don't have your program spitting out grades. You used to do this in your invocationlist iteration but that was the wrong place for multiple reasons (abuse of events, single responsibility, etc).

I would suggest making a service that allows you to pass in a grade and have it spit out the output.

Combining all these remarks brings me to this:

public class Worker
{
   public event EventHandler Started;
   public event EventHandler Progressing;
   public event EventHandler Completed;

   public void DoWork()
   {
        Console.WriteLine("Worker: work started");
        if (Started != null)
        {
            Started(this, EventArgs.Empty);
        }

        Console.WriteLine("Worker: work progressing");
        if (Progressing != null)
        {
            Progressing(this, EventArgs.Empty);
        }

        Console.WriteLine("Worker: work completed");
        if(Completed != null)
        {
            Completed(this, EventArgs.Empty);
        }
   }
}

public class Boss
{
   public async void WorkCompleted(object sender, EventArgs e)
   {
       await Task.Delay(TimeSpan.FromSeconds(3));
       Console.WriteLine("Better...");
       Grader.Grade(5);
   }
}

public static class Grader
{
    public static void Grade(int grade)
    {
        Console.WriteLine("Worker grade = {0}", grade);
    }
}

public class Universe
{
   public void WorkerStartedWork(object sender, EventArgs e)
   {
       Console.WriteLine("Universe notices worker starting work");
   }

   public async void WorkerCompletedWork(object sender, EventArgs e)
   {
       await Task.Delay(TimeSpan.FromSeconds(10));
       Console.WriteLine("Universe pleased with worker's work");
       Grader.Grade(7);
   }

    private static void Main()
    {
        var universe = new Universe();
        var peter = new Worker();
        var boss = new Boss();
        peter.Completed += boss.WorkCompleted;
        peter.Started += universe.WorkerStartedWork;
        peter.Completed += universe.WorkerCompletedWork;
        peter.DoWork();

        Console.WriteLine("Main: worker completed work");
        Console.ReadLine();
    }
}

I toyed around with events as well some time ago so you might be interested in taking a look at my proof of concept: https://github.com/Vannevelj/VariousSmallProjects/tree/master/KeyConcepts/KeyConcepts/Events


Function with jquery.alphanumeric I want to implement jquery.alphanumeric in HTML pattern, so I used a jQuery plugin from here. Can this be shortened or improved?

<input type="text" char-allow="$#*" class="alpha">
<input type="text" char-allow="+-" class="numeric">
<input type="text" char-allow="&" class="alphanumeric">

<script>
    function setAlphaNumeric(){
        $(".numeric").each(function(){
            var a = $(this).attr("char-allow"); $(this).numeric({allow:a});
        });
        $(".alpha").each(function(){
            var a = $(this).attr("char-allow"); $(this).alpha({allow:a});
        });
        $(".alphanumeric").each(function(){
            var a = $(this).attr("char-allow"); $(this).alphanumeric({allow:a});
        });
    }

$(function(){

setAlphaNumeric();

});
</script>

It could be shortened to:

function setAlphaNumeric() {
  $(".numeric, .alpha, .alphanumeric").each(function() {
    $(this)[this.className]({
      allow: $(this).attr("char-allow")
    });
  });
}

$(setAlphaNumeric);


Find the number closest to 0, breaking ties in favor of positive numbers I'm a Perl beginner and I really love this language, and I was wondering if it was possible to reduce the size of this code, just for fun and so as to learn Perl specificities:

chomp($n=<STDIN>);
if (!$n) {
   print 0;
} else {
   @t=split/ /,<STDIN>;
   foreach $i(0..$n-1) {
      $m=$t[$i]if(!$m)||(abs($t[$i])<abs($m))||(($t[$i]==-$m)&&($t[$i]>0));
   }
}print $m;

The goal of this code is to give the closest number to 0 of a list , and if there are -4 and 4 for exemple, it will always return the positive number.

$n is the number of numbers in the list
@t is the list as an array
$m is the min

This is "Temperatures" game on the website Codingame.com.

This code is working pretty well, and I'm trying to reduce its size so as to discover some Perl secrets.

I have some precise questions:

Is it possible to use a the ternary operator with several instructions in Perl?
Is is possible to write the foreach loop on one line in this case, or even replace the foreach loop with something else?

I don't really want perfect code as an answer, but some indications, and then I can search details and learn by myself.

A bad example

Perl is a language in which the shortest code is usually not the best code.  For example, this short program accomplishes the task:

print((sort {abs($a)<=>abs($b) || $b<=>$a} (split ' ',<>)[0..<>-1])[0] // 0)

Not only is it hard to read, it's also algorithmically inefficient (because sorting is O(n log n) rather than O(n)).  I don't recommend writing code like that.

Critique

You should use more whitespace.  It won't make your code look any less clever, nor will it make your code run any slower.

You can safely dispense with chomp(), as Perl will naturally disregard the trailing newline when interpreting the string as a number.  You could probably write <> instead of <STDIN>, unless you insist on reading only from standard input.

Counting loops are clumsy in Perl.  When possible, opt for higher-level list-processing techniques.  In this case, I recommend List::Util::reduce.

A possible solution

In Perl, there are usually many possible ways to solve a problem.  Here's one short solution that is O(n), but avoids being the shortest possible solution.  I'll let you figure out how it takes advantage of the <=> operator.

use List::Util qw(max reduce);
use strict;

my $n = <>;
my @temps = (split ' ', <>)[0 .. $n - 1];
print((reduce { ($a, max($a, $b), $b)[1 + (abs($a) <=> abs($b))] } @temps)
       // 0);

The special case of $n == 0 can be handled using a // logical defined-or operator.  A || logical or operator would also happen to work, but it wouldn't be as appropriate.

Since the @t is meant to represent temperatures, renaming it to @temps would make the code less cryptic.


Binding data to a TreeView The entire flow depends on parameters. It's all in the same method with an if/else, but I want to make the code a bit more clear. How can I functionally decompose this method into many?

private void BindTreeView(IEnumerable<Opcion> OptionList, TreeNode parentNode)
{
    var nodes = OptionList.Where(x => parentNode == null ? x.ParentID == 0 : x.ParentID == int.Parse(parentNode.Value));

    if (_pagemode != BoPage.PageMode.View)
    {
        foreach (Opcion node in nodes)
        {
            TreeNode newNode = new TreeNode();
            newNode.Text = (node.ParentID == 0) ? string.Format(@"{1}", node.OpcionID.ToString(), node.Descripcion.ToString()) : string.Format(@"{1}", node.ParentID.ToString(), node.Descripcion.ToString());
            newNode.Value = node.OpcionID.ToString();
            newNode.ToolTip = node.Descripcion.ToString();

            if (parentNode == null)
            {
                TreeViewPalabrasClaveOpciones.Nodes.Add(newNode);
            }
            else
            {
                if (node.PalabrasClaveOpciones.Where(c => c.KeywordID == _idKeyWord).Count() > 0)
                {
                    newNode.Checked = true;
                    parentNode.Expand();
                }
                parentNode.ChildNodes.Add(newNode);
            }
            BindTreeView(OptionList, newNode);
        }

    }
    else
    {
        foreach (Opcion node in nodes)
        {
            if (ExistKeyWord(node))
            {
                TreeNode newNode = new TreeNode();
                newNode.Text = (node.ParentID == 0) ? string.Format(@"{1}", node.OpcionID.ToString(), node.Descripcion.ToString()) : string.Format(@"{1}", node.ParentID.ToString(), node.Descripcion.ToString());
                newNode.Value = node.OpcionID.ToString();
                newNode.ToolTip = node.Descripcion.ToString();

                if (parentNode == null )
                {
                    TreeViewPalabrasClaveOpciones.Nodes.Add(newNode);
                }
                else
                {
                    if (node.PalabrasClaveOpciones.Where(c => c.KeywordID == _idKeyWord).Count() > 0)
                    {
                        newNode.Checked = true;
                        parentNode.Expand();
                    }

                    if (node.PalabrasClaveOpciones.Where(c => c.KeywordID == _idKeyWord).Count() > 0)
                    {
                        parentNode.ChildNodes.Add(newNode);
                    }

                }
                BindTreeView(OptionList, newNode);
            }
        }

    }

}

I want to make the code a bit more clear

I would start by removing one of the if branches and customize the loop to do the job according to the parameters because both loop are nearly identical.

You can create two helper variables for the conditions and act appropriately:

var canAddTreeNote = _pagemode != BoPage.PageMode.View || ExistKeyWord(node);

if (!canAddTreeNote)
{
    continue;
}

and for adding child nodes something like this:

var canAddChildNote =
    _pagemode == BoPage.PageMode.View
    && ExistKeyWord(node)
    && node.PalabrasClaveOpciones.Where(c => c.KeywordID == _idKeyWord).Count() > 0;

if (canAddChildNote)
{
    parentNode.ChildNodes.Add(newNode);
}

by their powers combined you'll get:

private void BindTreeView(IEnumerable<Opcion> OptionList, TreeNode parentNode)
{
    var nodes = OptionList.Where(x => parentNode == null ? x.ParentID == 0 : x.ParentID == int.Parse(parentNode.Value));

    foreach (Opcion node in nodes)
    {

        var canAddTreeNote = _pagemode != BoPage.PageMode.View || ExistKeyWord(node);

        if (!canAddTreeNote)
        {
            continue;
        }

        TreeNode newNode = new TreeNode();
        newNode.Text = (node.ParentID == 0) ? string.Format(@"{1}", node.OpcionID.ToString(), node.Descripcion.ToString()) : string.Format(@"{1}", node.ParentID.ToString(), node.Descripcion.ToString());
        newNode.Value = node.OpcionID.ToString();
        newNode.ToolTip = node.Descripcion.ToString();

        if (parentNode == null)
        {
            TreeViewPalabrasClaveOpciones.Nodes.Add(newNode);
        }
        else
        {
            if (node.PalabrasClaveOpciones.Where(c => c.KeywordID == _idKeyWord).Count() > 0)
            {
                newNode.Checked = true;
                parentNode.Expand();
            }

            var canAddChildNote =
                _pagemode == BoPage.PageMode.View
                && ExistKeyWord(node)
                && node.PalabrasClaveOpciones.Where(c => c.KeywordID == _idKeyWord).Count() > 0;

            if (canAddChildNote)
            {
                parentNode.ChildNodes.Add(newNode);
            }
        }
        BindTreeView(OptionList, newNode);
    }
}

  How can I functionally decompose this method into many?

When you apply the above refactoring I think it's no longer necessary to create more methods.


Wordpress single post content class customization with if-else function I have the following code, which displays the content for my custom page template "landingpage.php" and for single posts. The site is based on Bootstrap 4.
<?php if(is_page_template( 'landingpage.php' )): ?>

        <?php elseif (is_single()): ?>

        <?php else: ?>

<?php endif; ?>

Is there a more elegant and/or shorter way to achieve this?

The third
 appears to be the same in all three cases, so that can be abstracted out of the cases.
The outermost
 appears to have class pt-4 applied only if is_page_template( 'landingpage.php' ) returns a value that evaluates to FALSE and is_single() returns a value that evaluates to TRUE.
And the first nested (i.e. second)
 has class container-fluid if either of those functions return a value that evaluates to TRUE.
This logic could be rewritten as below. I know it strays away from the familiar wordpress style of using the Alternative syntax for control structures and uses the shortcut syntax (i.e. <?=) of echo but it does add some separation of the logic from the markup
<?php
// default class names
$contentClass = 'site-content';
$containerClass = 'container-fluid';
if (!is_page_template( 'landingpage.php' )) {
    if (is_single()) {
        $contentClass .= ' pt-4';
    }
    else {
        $containerClass = 'container';
    }
}
?>
    <div id="content" class="<?= $contentClass; ?>">
        <div class="<?= $containerClass; ?>">

It could be simplified using ternary operators but some may argue that would detract from readability.

Is this shorter?
yes, only by two lines

is this more elegant?
Well, that is subjective. The code above should be simpler to read. Consider the scenario where the code is updated by multiple people, including programmers and designers. The programmers can have designers update the markup without affecting the business logic. For more on this topic, refer to Mixing PHP and HTML.


Implement a function to check if a linked list is a palindrome in c++ This is working example, I'm interested in mostly in knowing if there is any c++ related improvements you guys suggest? Also, do you think its better to solve this question iteratively or interviewer would prefer some recursive solution (assuming if that is possible)? Any other suggestions from interview point of view welcome?

#include <iostream>
#include <string>
#include <vector>
#include <cstdlib>

template<typename valT>
struct node
{
  valT val;
  node<valT> *next;
  node() { next = nullptr; }
  node(valT v) : val(v),next(nullptr) { }
  ~node() { next = nullptr; }
};

using myNode = node<int>;
using myNodePtr = myNode *;
myNodePtr head = nullptr;

void showLL(myNodePtr head)
{
 std::cout << "Linked List =>";
 while(head){
   std::cout << head->val << ", ";
   head = head->next;
 }
 std::cout << std::endl;
}

int main(int argc, char **argv)
{
  myNodePtr prev = nullptr;
  for( int i = 1; i < argc ; i++ ) {
    myNodePtr tmp = new myNode(strtol(argv[i], NULL, 10));
    if(!head) {
      head = tmp;
    }
    if(prev){
      prev->next = tmp;
    }
    prev = tmp;
  }

  std::cout << "Input "  << std::endl;
  showLL(head);

  //Find middle
  //Push on stack until you find middle
  //Stop fast runner and pop back and compare and break if not same

  //Palindrome loop
  myNodePtr middleNode = nullptr;
  std::vector<int> stack;
  for(myNodePtr slowRunner = head, fastRunner = head; slowRunner; slowRunner = slowRunner->next) {
    if(!middleNode) {
      stack.push_back(slowRunner->val);

      if(fastRunner->next) {
        if(fastRunner->next->next) { //odd
          fastRunner = fastRunner->next->next;
          if(!fastRunner->next) {
            slowRunner = slowRunner->next;
          }
        } else { //even
          fastRunner = fastRunner->next;
        }
        if(!fastRunner->next) {
          middleNode = slowRunner;
          std::cout << "middle node =" << slowRunner->val << std::endl;
        }
      }
    } else {
      //even odd
      if( stack.back() != slowRunner->val) {
        std::cout << "Not a palindrom!" << std::endl;
        return 1; //delete all linked lists
      }
      stack.pop_back();
    }
  }

  std::cout << "is Palindrome"<< std::endl;
}

Building linked list from command line arguments

The multiple conditionals inside this loop is kind of ugly,
because head and prev will only ever be null once:

  for( int i = 1; i < argc ; i++ ) {
    myNodePtr tmp = new myNode(strtol(argv[i], NULL, 10));
    if(!head) {
      head = tmp;
    }
    if(prev){
      prev->next = tmp;
    }
    prev = tmp;
  }

You could avoid the unnecessary conditional evaluations by lifting the special treatment of the head outside the loop:

  myNodePtr head = nullptr;
  if (argc > 1) {
    head = new myNode(strtol(argv[1], NULL, 10));
  }
  myNodePtr node = head;
  for (int i = 2; i < argc; i++) {
    node->next = new myNode(strtol(argv[i], NULL, 10));
    node = node->next;
  }

Iterating until the middle and verifying the rest

Iterating until the middle could be written simpler:

  std::vector<int> stack;
  myNodePtr slow = head;
  myNodePtr fast = head;
  while (fast && fast->next) {
    stack.push_back(slow->val);
    slow = slow->next;
    fast = fast->next->next;
  }

At this point,
if the length is odd, slow will be pointing to the single middle element,
that should be skipped.
The length is odd if fast is not null, so you can write:

  if (fast) slow = slow->next;

Finally, to check that the rest of the list matches the content of the stack:

  while (!stack.empty()) {
    if (stack.back() != slow->val) {
      std::cout << "Not a palindrom!" << std::endl;
      return 1;
    }
    stack.pop_back();
    slow = slow->next;
  }

This alternative implementation uses much fewer conditions and cleanly separates the interesting steps, so I think it's easier to read.

From an interview point of view

At an interview,
if you write an iterative solution,
the interviewer is likely to ask to also write a recursive solution.
And vice versa.
You need to be able to ready to do both,
and discuss about the trade-offs of each technique.

This solution would get important criticism at an interview for several reasons:

Large chunk of code inside main method, instead of separated to small methods with single simple clear purpose
Knowledge of memory management is not demonstrated (the program simply relies on cleanup on exit)


Disable reboot on control-alt-delete for RedHat/CentOS 5/6/7 As root, run:

#!/bin/sh
if cat /etc/redhat-release | grep -q 'release 5'; then
  cp /etc/inittab /etc/inittab.install
  sed -i 's/ca::ctrlaltdel/#ca::ctrlaltdel/' /etc/inittab
  init q
elif cat /etc/redhat-release | grep -q 'release 6'; then
  cat << EOF > /etc/init/control-alt-delete.conf
exec false
EOF
elif cat /etc/redhat-release | grep -q 'release 7'; then
  systemctl mask ctrl-alt-del.target
  systemctl daemon-reload
else
  cat /etc/redhat-release 2>&1 | mailx -s "ctrl-alt-delete disable failure" some.user@company.com
fi

That's a four calls to cat /etc/redhat-release.  A case statement could help reduce the clutter:

case "`cat /etc/redhat-release`" in
  *"release 5."*)
    cp /etc/inittab /etc/inittab.install
    sed -i 's/ca::ctrlaltdel/#ca::ctrlaltdel/' /etc/inittab
    init q
    ;;
  *"release 6."*)
    echo 'exec false' > /etc/init/control-alt-delete.conf
    ;;
  *"release 7."*)
    systemctl mask ctrl-alt-del.target
    systemctl daemon-release
    ;;
  *)
    cat /etc/redhat-release 2>&1 | mailx -s "ctrl-alt-delete disable failure" some.user@example.com
    ;;
esac


Replace each element in a list Here is how to transform a Zorglub (it is an API that I can not modify):

Zorglub transformedZorglub = zorglub.transform()

I am given a java.util.List<Zorglub> and I must transform them all, then afterwards I will perform more operations on the transformed Zorglubs. After transformation I never use the originals anymore. This question is only about the transformation part. I wrote this:

List<Zorglub> transformedZorglubs = new ArrayList<Zorglub>(zorglubs.size())
zorglubs.each {
    transformedZorglubs.add(
        it.transform()
    )
}
zorglubs = transformedZorglubs

Is there something smarter or more elegant to do?

In particular, something that would avoid creating a new list and using twice as much memory, while avoiding bugs that often arise when modifying a list while iterating over it?

To replace the elements in-place without building a new list,
you would need to reassign the elements at each index, that is,
essentially zorglubs[i] = zorglubs[i].transform() for each index i in the list.

A perhaps elegant way to do this is using the eachWithIndex function,
which gives you access to each element and its index:

zorglubs.eachWithIndex { it, index -> zorglubs[index] = it.transform() }


Meshing algorithm for generating faces in Minecraft To generate faces of a Minecraft-like world from a three dimensional array, I developed the following meshing algorithm. The faces are given vertices and indices. A vertex consists of a position in space, a normal vector and a texture coordinate. Indices point to vertices to consecutively define faces.

Those variables are used:

// dimensions of the world
#define SIZE 32

// voxel grid representing the world, where 0 means air
uint8_t data[SIZE][SIZE][SIZE];

// results are stored in those vectors
vector<float> positions, normals, texcoords;
vector<unsigned int> elements;

This is the algorithm:

int n = 0;
for(int X = 0; X < SIZE; ++X)
for(int Y = 0; Y < SIZE; ++Y)
for(int Z = 0; Z < SIZE; ++Z)
{
    if(data[X][Y][Z])
    {
        uint8_t tile = clamp((int)data[X][Y][Z], 0, TILES_U * TILES_V - 1);
        for(int dim = 0; dim < 3; ++dim) { int dir = -1; do {
            ivec3 neigh = Shift(dim, ivec3(dir, 0, 0)) + ivec3(X, Y, Z);

            if(Inside(neigh, ivec3(0), ivec3(SIZE) - 1))
                if(data[neigh.x][neigh.y][neigh.z])
                    goto skip;

            {
                for(float i = 0; i <= 1; ++i)
                for(float j = 0; j <= 1; ++j)
                {
                    vec3 vertex = vec3(X, Y, Z) + vec3(Shift(dim, ivec3((dir+1)/2, i, j)));
                    positions.push_back(vertex.x); positions.push_back(vertex.y); positions.push_back(vertex.z);
                }

                /*
                 * the normal vector is not important for this question
                 *
                 * vec3 normal = normalize(vec3(Shift(dim, ivec3(dir, 0, 0))));
                 * for(int i = 0; i < 4; ++i)
                 * {
                 *  normals.push_back(normal.x); normals.push_back(normal.y); normals.push_back(normal.z);
                 * }
                 */

                /*
                 * texture coordinates are not important for this question
                 *
                 * vec2 coords(tile % TILES_U, tile / TILES_U);
                 * vec2 position = coords * GRID;
                 * texcoords.push_back(position.x          + GAP); texcoords.push_back(position.y          + GAP);
                 * texcoords.push_back(position.x + GRID.x - GAP); texcoords.push_back(position.y          + GAP);
                 * texcoords.push_back(position.x          + GAP); texcoords.push_back(position.y + GRID.y - GAP);
                 * texcoords.push_back(position.x + GRID.x - GAP); texcoords.push_back(position.y + GRID.y - GAP);
                 */

                if(dir == -1) {
                    elements.push_back(n+0); elements.push_back(n+1); elements.push_back(n+2);
                    elements.push_back(n+1); elements.push_back(n+3); elements.push_back(n+2);
                } else {
                    elements.push_back(n+0); elements.push_back(n+2); elements.push_back(n+1);
                    elements.push_back(n+1); elements.push_back(n+2); elements.push_back(n+3);
                }
                n += 4;
            }

        skip: dir *= -1; } while(dir > 0); }
    }
}

And here are the two helper functions.

bool Inside(ivec3 Position, ivec3 Min, ivec3 Max)
{
    if(Position.x < Min.x || Position.y < Min.y || Position.z < Min.z) return false;
    if(Position.x > Max.x || Position.y > Max.y || Position.z > Max.z) return false;
    return true;
}

ivec3 Shift(int Dimension, ivec3 Vector)
{
    if      (Dimension % 3 == 1) return ivec3(Vector.z, Vector.x, Vector.y);
    else if (Dimension % 3 == 2) return ivec3(Vector.y, Vector.z, Vector.x);
    else                         return Vector;
}

As you can see, the algorithm uses a goto statement to skip faces which adjacent blocks first, are inside the world bounds and second, are not filled with air. Those faces shouldn't be generated since they are not visible to the player.

How can I restructure the statement to not use goto anymore?

First of all, the way you're formatting your code is horribly confusing.

if(Inside(neigh, ivec3(0), ivec3(SIZE) - 1))
    if(data[neigh.x][neigh.y][neigh.z])
        goto skip;

{
    ...
}

The block here looks a lot like it's in the if condition but it's not (it's just a C-like block without any condition attached to it). I really suggest using braces for your ifs to avoid confusion. Another confusing part is for(int dim = 0; dim < 3; ++dim) { int dir = -1; do {: when I see a for loop, I don't expect to see another loop at the end of it.

Now, about your question. Some languages (such as Java) support naming loop, which allows you to break to outer loops. This doesn't exist in C++, and the best option is to use a goto.


How to write a cleaner code for this piece of php snippet? I would like to know if there is a better/shorter/cleaner/resource saving method for this snippet of php code with codeigniter? Thanks.

$code = $this->uri->segment(3);

if (!$code) {
    // validation
    $this->form_validation->set_rules('code', 'PassCode', 'required|trim|max_length[100]|xss_clean');

    // run validation to check if validation rules are satifised
    if ($this->form_validation->run() == true) {
        $code = $this->input->post('code');
        $user_id = $this->user_model->getUserId($code);
        if (empty($user_id)) {
            $data['errorMessage'] = 'Your code is not valid.';
        } else {
            redirect('user/profile');
        }
    }
} else {
    $user_id = $this->user_model->getUserId($code);
    if (empty($user_id)) {
        $data['errorMessage'] = 'Your code is not valid.';
    } else {
        redirect('user/profile');
    }
}

if ($this->form_validation->run() == true)

is redundant, just use...

if ($this->form_validation->run())

Also, you can move the whole if (empty($user_id)) block outside to the end to avoid duplicating it:

$code = $this->uri->segment(3);

if (!$code) {
    // validation
    $this->form_validation->set_rules('code', 'PassCode',
        'required|trim|max_length[100]|xss_clean');

    // run validation to check if validation rules are satifised
    if ($this->form_validation->run()) {
        $code = $this->input->post('code');
        $user_id = $this->user_model->getUserId($code);
    }
} else {
    $user_id = $this->user_model->getUserId($code);
}

if (empty($user_id)) {
    $data['errorMessage'] = 'Your code is not valid.';
} else {
    redirect('user/profile');
}


Palindrome checker program I've been studying Java for some days and I'm just now starting to make some simple programs, I'd like to know if there's any 'noob' mistakes or habits that I could avoid.

In other words: how could my code be more streamlined to Java?

My code:

import java.util.Scanner;

public class Main
{
    private static String reverse_string(String my_string)
    {
        String reversed_string = "";
        for(int j = my_string.length() - 1; j >= 0; j--)
        {
            reversed_string = reversed_string + my_string.charAt(j);
        }
        return reversed_string;
    }

    public static void main(String[] args)
    {
        System.out.print("Insert a 'String': ");

        Scanner input = new Scanner(System.in);
        String user_string = input.nextLine().toLowerCase().replace(" ", "");

        if (user_string.equals(reverse_string(user_string)))
            {
                System.out.println("It is a palindrome.");
            } else
            {
                System.out.println("It is not a palindrome.");
            }
    }
}

I believe there are already other alternatives to my  reverse_string function, hence the 'reinventing the wheel' tag.

To answer your comment, yes there are equivalent of PEP8 in java. I suggest checkstyle, this plugin works in lots of IDE / text editors and can be configured with your code style.

Code review

reverse_string method

In java, we try to use the upper-case version of Snake case only on constants / enums. For the methods and variables, I suggest that you use the Camel case style.

Before

private static String reverse_string(String my_string)
{
   //[..]
}

After

private static String reverseString(String myString)
{
   //[..]
}

When concatenating String in a loop, it's generally recommended to use a java.lang.StringBuilder to gain performance and take fewer operations.

private static String reverseString(String myString)
{
   StringBuilder reversedString = new StringBuilder();

   for (int j = myString.length() - 1; j >= 0; j--)
   {
      reversedString.append(myString.charAt(j));
   }

   return reversedString.toString();
}

If you want not to use it, then I suggest the += operator instead of the +; it will give the same result and make the code shorter.

Other observations

The code was missing a bit of formatting, but nothing too important, I sugest that you pick a formatter for your style (Horstmann style) depending on your IDE / text editor.

Refactored code

private static String reverseString(String myString)
{
   String reversedString = "";

   for (int j = myString.length() - 1; j >= 0; j--)
   {
      reversedString += myString.charAt(j);
   }

   return reversedString;
}

public static void main(String[] args)
{
   System.out.print("Insert a 'String': ");

   Scanner input = new Scanner(System.in);
   String userString = input.nextLine().toLowerCase().replace(" ", "");

   if (userString.equals(reverseString(userString)))
   {
      System.out.println("It is a palindrome.");
   }
   else
   {
      System.out.println("It is not a palindrome.");
   }
}


Using prototype to simulate extending a 'base class' object in JavaScript I'm investigating using prototypes in JavaScript to simulate using 'base classes' which can be 'extended'. I've created an example where dialogBox is my 'base' and welcomeMessage is the 'extension'.

// OBJECT CONSTRUCTORS

var dialogBox = function(xPosition, yPosition, width, height){
    this.x = xPosition;
    this.y = yPosition;
    this.width = width;
    this.height = height;
    this.draw = function(){
        STAGE.fillRect(this.x, this.y, this.width, this.height);
        STAGE.strokeRect(this.x, this.y, this.width, this.height);
    };
};

var welcomeMessage = function(messageString){
    this.message = messageString;
    this.addMessage = function(){
        STAGE.strokeText(this.message, this.x + 10, this.y + 10, this.width-10);
        console.log(this.message);
    };
};
welcomeMessage.prototype = new dialogBox();

This works as intended (or at least gives the appearance of working as intended) - see this fiddle.

However, the way I have to instantiate these objects seems rather inelegant:

var box1 = new dialogBox(20, 20, 300, 100);

var box2 = new welcomeMessage('hello there');
box2.x = 20;
box2.y = 140;
box2.width = 300;
box2.height = 100; // wouldn't it be great if I could pass these properties as args?

I wrote these constructors using the MDN Object.prototype reference and am following their example quite closely, but I've observed from reading around Stack Overflow / Code Review many examples where prototyping is used very differently, often in relation to getters and setters. This is leading me to question if I have quite grasped the 'proper' use of prototype.

Any feedback is welcome, but I am particularly interested in identifying any potential problems my approach could cause when applied to more complex situations.

The way you are approaching the problem is essentially correct. What you want to accomplish is also possible in a very straightforward way:

var welcomeMessage = function(messageString, xPosition, yPosition, width, height){
    this.message = messageString;
    this.x = xPosition;
    this.y = yPosition;
    this.width = width;
    this.height = height;
    this.addMessage = function(){
        STAGE.strokeText(this.message, this.x + 10, this.y + 10, this.width-10);
        console.log(this.message);
    };
};
welcomeMessage.prototype = new dialogBox();

In this way, the properties can be assigned through the constructor of the subclass as well. But we can go further. There is duplication between the constructors. We can eliminate this by calling the super constructor on the subclassed object:

var welcomeMessage = function(messageString, xPosition, yPosition, width, height){
    this.message = messageString;
    dialogBox.call(this, xPosition, yPosition, width, height);
    this.addMessage = function(){
        STAGE.strokeText(this.message, this.x + 10, this.y + 10, this.width-10);
        console.log(this.message);
    };
};
welcomeMessage.prototype = new dialogBox();

Now we have DRYed up our constructor code. This should work fine.

I just want to make a few additional points. First, rather than adding methods in the constructor, you should add them to the constructor's prototype:

dialogBox = function(...) {
    ...
}

dialogBox.prototype.draw = function() {
    ...
}

When you add methods in the constructor, they are recreated every time a new object is made, whereas if they are added to the prototype, they are created only once. Having them in the constructor also means that it has to be called to add them to a subclass. There may be reasons not to do this.

If your environment supports it, you should use Object.create(dialogBox.prototype) to set the prototype rather than new dialogBox(). This avoids calling the constructor just to set up inheritance. This is good in cases where the super class constructor does work or requires parameters be passed in.

That makes the final version:

var dialogBox = function(xPosition, yPosition, width, height){
    console.log('dialogBox Constructor');
    this.x = xPosition;
    this.y = yPosition;
    this.width = width;
    this.height = height;
};

dialogBox.prototype.draw = function() {
    STAGE.fillRect(this.x, this.y, this.width, this.height);
    STAGE.strokeRect(this.x, this.y, this.width, this.height);
};

var welcomeMessage = function(messageString, xPosition, yPosition, width, height){
    this.message = messageString;
    dialogBox.call(this, xPosition, yPosition, width, height);
};
welcomeMessage.prototype = Object.create(dialogBox.prototype);
welcomeMessage.prototype.addMessage = function() {
    STAGE.strokeText(this.message, this.x + 10, this.y + 10, this.width - 10);
    console.log(this.message);
};

JSFiddle

As a side note, the prototype must be set before any new methods are added, so welcomeMessage.prototype = ... cannot come after you add any methods.


Wrapper class for mediainfo I made the following wrapper for the mediainfo command line. I did this mainly because mediainfo gives me more information than os.stat, specifically it gives me the Encoded date for .wma files, while the dates in os.stat don't represent the time the files were made, but rather the date the files were compressed (they were delivered to me in a compressed folder).

But is it actually a good idea to use a class for this? I needed to do this in bulk, for 20.000 files. Also it feels inefficient to let mediainfo format the information first and then read it again, but I'm not sure how to do that otherwise.

from pathlib import Path
from subprocess import Popen, PIPE

class MediaInfo:
    """Wrapper class for the mediainfo command line tool."""

    # Define the path to the `mediainfo` command line tool.
    mediainfo = "/usr/bin/mediainfo"

    def __init__(self, filename):
        self.file = Path(filename).resolve()
        self.info = self.readinfo()

        if not self.file.is_file():
            raise OSError(f"File not found: {file}")

    def __str__(self):
        """String representation (the same as what mediainfo shows)"""
        if hasattr(self, "info") and self.info is not None:
            return self.info.strip()

        return self.readinfo().strip()

    def infofield(self, field, postread=None):
        """Find a given field and return its value.

        If the `field` is not in the mediainfo metadata, return None.
        `postread` can be a callable to, for example, cast the returned type.
        """
        val = None
        for line in self.info.split("\n"):
            if not line.strip() or not ":" in line:
                continue
            key, val = line.strip().split(" : ")
            key = key.strip()
            val = val.strip()
            if key == field:
                break

        if postread is None:
            return val
        return postread(val)

    def readinfo(self):
        # Start the command line process
        proc = Popen([self.mediainfo, str(self.file)], stdout=PIPE, stderr=PIPE)
        # Catch the output
        stdout, stderr = proc.communicate()

        if stderr:
            self.info = stderr.decode()
            raise MediaInfoException("Error in MediaInfo process", self.info)

        self.info = stdout.decode()
        return self.info

class MediaInfoException(Exception):
    """Custom exception for MediaInfo."""
    pass

Using a class might be too much. It all depends of how you want to use your script.

But I think you should take a look at mediainfo help. Issuing:

mediainfo --Inform="Audio;%Encoded_Date%" Filename

outputs only the encoded date value of your audio file, or an empty line if not applicable. Maybe you won't need your script anymore.


Flattening XML and selecting nodes based on input to convert to CSV I have some XML that I want to flatten based on its XPath. The task is to acquire certain nodes that are passed as input. That said, I have to get to parent nodes and look for nodes that might be part of the configuration. I am avoiding lists in a given iteration.

This is my code:

from lxml import etree

tree = etree.parse("sample.xml")
path = "/DATA_DS/G_1"

def getNodesDict(node_element, nodes_to_pick):
    some_dict = {}
    for element in node_element:
        if not isinstance(element, list):
            if element.tag in nodes_to_pick:
                some_dict[element.tag] = element.text

    return some_dict

def fetchNodesListBasedOnXpath(Xpath, tree, selected_nodes):
    """

    :param Xpath:
    :param tree:
    :param selected_nodes:
    :return:
    """
    temp_list = []
    temp_list1 = []
    element_split = Xpath.split('/')
    for i in range(len(element_split), 1, -1):
        path = "/".join(element_split[0:i])
        try:
            element = tree.xpath(path)
            for j, node_element in enumerate(element):
                value = getNodesDict(node_element, selected_nodes)
                if len(temp_list) > 0 and len(temp_list) - 1 > j:
                    for k, item in enumerate(temp_list):
                        dall = {}
                        dall.update(value)
                        dall.update(item)

                        if item:
                            temp_list1.append(dall)
                    temp_list = []
                else:
                    temp_list.append(value)
        except:
            pass

nodes_to_pick = ['SUBSCRIPTION_ID', "PRODUCT_NUMBER", "SUBSCRIPTION_ID1", "SALES_PRODUCT_TYPE1"]

fetchNodesListBasedOnXpath(path, tree, nodes_to_pick)

This is my sample XML:

<DATA_DS>
    <SUBSCRIPTION_ID1>300000031070490</SUBSCRIPTION_ID1>
    <SALES_PRODUCT_TYPE1>SUBSCRIPTION</SALES_PRODUCT_TYPE1>
    <G_1>
        <SUBSCRIPTION_ID>300000031070490</SUBSCRIPTION_ID>
        <PRODUCT_NUMBER>1</PRODUCT_NUMBER>
        <SALES_PRODUCT_TYPE>SUBSCRIPTION</SALES_PRODUCT_TYPE>
        <PRODUCT_NAME>Demo4</PRODUCT_NAME>
        <STATUS>ORA_EXPIRED</STATUS>
        <START_DATE>2019-06-01T00:00:00.000+00:00</START_DATE>
        <END_DATE>2019-06-01T00:00:00.000+00:00</END_DATE>
    </G_1>

    <G_1>
        <SUBSCRIPTION_ID>300000031070491</SUBSCRIPTION_ID>
        <PRODUCT_NUMBER>4</PRODUCT_NUMBER>
        <SALES_PRODUCT_TYPE>SUBSCRIPTION</SALES_PRODUCT_TYPE>
        <PRODUCT_NAME>Demo4</PRODUCT_NAME>
        <STATUS>ORA_ACTIVE</STATUS>
        <START_DATE>2019-06-01T00:00:00.000+00:00</START_DATE>
        <END_DATE>2020-05-31T00:00:00.000+00:00</END_DATE>
    </G_1>
    <G_1>
        <SUBSCRIPTION_ID>300000031070492</SUBSCRIPTION_ID>
        <PRODUCT_NUMBER>2</PRODUCT_NUMBER>
        <SALES_PRODUCT_TYPE>SUBSCRIPTION</SALES_PRODUCT_TYPE>
        <PRODUCT_NAME>Demo4</PRODUCT_NAME>
        <STATUS>ORA_CLOSED</STATUS>
        <START_DATE>2019-06-01T00:00:00.000+00:00</START_DATE>
        <END_DATE>2019-06-30T00:00:00.000+00:00</END_DATE>
    </G_1>
    <G_1>
        <SUBSCRIPTION_ID>300000031070493</SUBSCRIPTION_ID>
        <PRODUCT_NUMBER>3</PRODUCT_NUMBER>
        <SALES_PRODUCT_TYPE>SUBSCRIPTION</SALES_PRODUCT_TYPE>
        <PRODUCT_NAME>Demo4</PRODUCT_NAME>
        <STATUS>ORA_ACTIVE</STATUS>
        <START_DATE>2019-06-01T00:00:00.000+00:00</START_DATE>
        <END_DATE>2019-08-31T00:00:00.000+00:00</END_DATE>
    </G_1>
</DATA_DS>

Docstrings

You should include a docstring at the beginning of every module, class, and function you write. This will allow documentation to identify what your code is supposed to do. Docstrings are great for the understanding the functionality of the larger part of the code, i.e., the general purpose of any class, module or function. Comments are used for code, statement, and expressions which tend to be small.

Function Naming

You have these two function names:

fetchNodesListBasedOnXpath
getNodesDict

According to PEP-8's Guidelines for function names, these function names  should be snake_case.

_ for unused loop variables

When you don't use a variable in a loop, like:

for i in range(5):
    print("Hi!")

You should use a _. This makes it clear that the variable used for the loop is not needed, and should be ignored.

Use of if len(...)

  For sequences, (strings, lists, tuples), use the fact that empty sequences are false.

Yes: if not seq:
     if seq:

No:  if len(seq):
     if not len(seq):

Essentially, if seq will return True if it's not empty, and if not seq will return True if it is empty. Since you are checking if len(temp_list) > 0 ..., this should be changed to if temp_list ....

Constant Variable Naming

According to PEP8s rule on constants:

  Constants are usually defined on a module level and written in all capital letters with underscores separating words. Examples include MAX_OVERFLOW and TOTAL.

Long Version:

In the Python community (as in many other communities) exist conventions about how to write code. This is different from working code: even if you write your constants all lowercase, your code still works.

But there is community consensus (as documented in PEP8) which is "enforced" with tools like pylint. If you program for your own happiness, you may neglect the hints pylint gives you. If you want open exchange with the community, aka »someone besides myself should use my code«, you should prepare your code according to PEP8. [source

Main Guard

Having a main guard clause in a module allows you to both run code in the module directly and also use procedures and classes in the module from other modules. Without the main guard clause, the code to start your script would get run when the module is imported. This can cause a plethora of issues if you want to reuse this code. Wrapping it in a main guard (check updated code) protects against this.

Parameter Naming

Parameters, variables, and default parameters all follow the same PEP-8 Guidelines for naming. They should all be snake_case. You currently have a parameter named Xpath. A simple change to xpath will do the trick.

Catching Errors/Exceptions

Right now, your main function can catch an exception, but doesn't handle it. If your program suddenly fails, a detailed error message would do the trick. You should catch specific errors that you think your code will generate. Since you're dealing with files and paths, catching FileNotFoundError is a good idea. Then, after dealing with all the specific errors, you can use a finally. Since this will always be run, you can put your pass here.

Updated Code

"""
Module Docstring
A description of your program goes here
"""

from lxml import etree

def get_nodes_dict(node_element, nodes_to_pick):
    """
    A description of this method should go here
    """
    some_dict = {}
    for element in node_element:
        if not isinstance(element, list):
            if element.tag in nodes_to_pick:
                some_dict[element.tag] = element.text

    return some_dict

def fetch_nodes_list_based_on_xpath(xpath, tree, selected_nodes):
    """

    A description of this method should go here

    :param xpath:
    :param tree:
    :param selected_nodes:
    :return:
    """
    temp_list = []
    temp_list1 = []
    element_split = xpath.split('/')
    for i in range(len(element_split), 1, -1):
        path = "/".join(element_split[0:i])
        try:
            element = tree.xpath(path)
            for j, node_element in enumerate(element):
                value = get_nodes_dict(node_element, selected_nodes)
                if temp_list and len(temp_list) - 1 > j:
                    for _, item in enumerate(temp_list):
                        dall = {}
                        dall.update(value)
                        dall.update(item)
                        if item:
                            temp_list1.append(dall)
                    temp_list = []
                else:
                    temp_list.append(value)
        except FileNotFoundError as _:
            print(f"The file at [{PATH}] doesn't exist!")
        finally:
            pass

if __name__ == '__main__':

    TREE = etree.parse("code/sample.xml")
    PATH = "/DATA_DS/G_1"

    NODES_TO_PICK = ['SUBSCRIPTION_ID', "PRODUCT_NUMBER", "SUBSCRIPTION_ID1", "SALES_PRODUCT_TYPE1"]

    fetch_nodes_list_based_on_xpath(PATH, TREE, NODES_TO_PICK)


How can I implement the generic repository pattern and improve the performance for the code below? I've used EF-DB first approach and have written a simple app to retrieve the data from the database and show it in the view. The code is simple and ok for a beginner like me, but how can I implement an effective design-pattern like interface-specification pattern or generic repository pattern, and possibly dependency injection to improve the code below ? I'd like to know because when extending the project I might have to get data from different databases, and would like to implement an effective pattern for maintainability.

Please note that I've started looking into topics like interfaces, design patterns, etc only recently, So I'd greatly appreciate it if you could please break down the explanation to a beginner level.

Below are my view-model and the controller. My model contains a stored-procedure and a table in the .edmx file - Thanks

View-Model

    public class OfficeDTO
    {
        public IEnumerable<EmployeeDTO> Employees { get; set; }
        public IEnumerable<DepartmentDTO> Departments { get; set; }
    }

    public class EmployeeDTO
    {
       public int EmpId { get; set; }
       public string EmpName { get; set; }
    }

    public class DepartmentDTO
    {
        public string DeptCode { get; set; }
        public string DeptName { get; set; }
    }

Controller

  public ActionResult Index()
            {
                EF.OfficeEntities ctx = new EF.OfficeEntities();
                Models.OfficeDTO office = new Models.OfficeDTO();

                using (ctx)
                {

                    var empList = ctx.GetEmployeesByYearJoined("2009")
                              .ToList();

                    var empResults = (from q in empList
                                   select new Models.EmployeeDTO
                                   {
                                       EmpId = q.EmpID,
                                       EmpName = q.FirstName + q.LastName
                                   }).ToList();
                    office.Employees = empResults;

                    var depResults = (from q in ctx.Departments
                                      select new Models.DepartmentDTO
                                      {
                                          DeptCode = q.DepartmentCode,
                                          DeptName = q.DepartmentName

                                      }).ToList();
                    office.Departments = depResults;
                }

                return View(office);
            }

The only way DI (IoC) is going to improve this code is by extracting the creation and disposal of your data context. This also means you can mock it for your unit tests.

You could argue the DataContext already is a Repository.

You could do one of:

Leave it as it is. No reason to complicate it just for the sake of it. It's small, pretty easy to follow and easily found.
Extract a Query object (Query Pattern)
Wrap the query and creation of the OfficeDTO in an OfficeDTO factor


Extending ThreadPoolExecutor I have implemented a ThreadPoolExecutor that will run a Consumer<T> only on elements not already consumed. This code uses Java 8.

The background behind this is that I scan a directory every x time units for which files are present, I must maintain a 100% accuracy on finding files and other mechanisms such as JNotify or a simple plain WatcherService do not achieve that.

public class SingleExecutionThreadPoolExecutor<E> extends ThreadPoolExecutor {
    private final Consumer<E> consumer;
    private final List<E> elementsInProcess = new ArrayList<>();

    public SingleExecutionThreadPoolExecutor(int corePoolSize, int maximumPoolSize, long keepAliveTime, TimeUnit unit, BlockingQueue<Runnable> workQueue, Consumer<E> consumer) {
        super(corePoolSize, maximumPoolSize, keepAliveTime, unit, workQueue);
        this.consumer = consumer;
    }

    public SingleExecutionThreadPoolExecutor(int corePoolSize, int maximumPoolSize, long keepAliveTime, TimeUnit unit, BlockingQueue<Runnable> workQueue, Consumer<E> consumer, RejectedExecutionHandler handler) {
        super(corePoolSize, maximumPoolSize, keepAliveTime, unit, workQueue, handler);
        this.consumer = consumer;
    }

    public SingleExecutionThreadPoolExecutor(int corePoolSize, int maximumPoolSize, long keepAliveTime, TimeUnit unit, BlockingQueue<Runnable> workQueue, Consumer<E> consumer, ThreadFactory threadFactory) {
        super(corePoolSize, maximumPoolSize, keepAliveTime, unit, workQueue, threadFactory);
        this.consumer = consumer;
    }

    public SingleExecutionThreadPoolExecutor(int corePoolSize, int maximumPoolSize, long keepAliveTime, TimeUnit unit, BlockingQueue<Runnable> workQueue, Consumer<E> consumer, ThreadFactory threadFactory, RejectedExecutionHandler handler) {
        super(corePoolSize, maximumPoolSize, keepAliveTime, unit, workQueue, threadFactory, handler);
        this.consumer = consumer;
    }

    public void execute(E element) {
        if (!elementsInProcess.contains(element)) {
            super.execute(() -> consumer.accept(element));
            elementsInProcess.add(element);
        }
    }

    @Override
    @Deprecated
    public void execute(Runnable command) { }
}

It is called using the following classes and snippets:

public final class UniqueTimePath {
    private final Path path;
    private final FileTime fileTime;

    public UniqueTimePath(final Path path) {
        this.path = path;
        try {
            this.fileTime = Files.getLastModifiedTime(path);
        } catch (IOException ex) {
            throw new UncheckedIOException(ex);
        }
    }

    public Path getPath() {
        return path;
    }

    public FileTime getFileTime() {
        return fileTime;
    }

    @Override
    public int hashCode() {
        int hash = 3;
        return hash;
    }

    @Override
    public boolean equals(Object obj) {
        if (obj == null) {
            return false;
        }
        if (getClass() != obj.getClass()) {
            return false;
        }
        final UniqueTimePath other = (UniqueTimePath) obj;
        if (!Objects.equals(this.path, other.path)) {
            return false;
        }
        if (!Objects.equals(this.fileTime, other.fileTime)) {
            return false;
        }
        return true;
    }

    @Override
    public String toString() {
        return "{" + path + ", " + fileTime + "}";
    }
}

private SingleExecutionThreadPoolExecutor<UniqueTimePath> executor;
//...
executor = new SingleExecutionThreadPoolExecutor<>(threadCount, threadCount, 0L, TimeUnit.MILLISECONDS, new LinkedBlockingQueue<>(), fileConsumer);
//...
Files.list(directory)
        .map(UniqueTimePath::new)
        .forEach(executor::execute);

.equals

The last part of your .equals implementation can be simplified to:

return Objects.equals(this.path, other.path) && Objects.equals(this.fileTime, other.fileTime);

HashCode

int hash = 3;
return hash;

Do I even have to comment anything about what I think about this implementation? You use Objects.equals above, you can use Objects.hashCode(this.path, this.fileTime); here. It is preferred if hashCode and equals use the same fields for their operations.

Set or List?

Whenever you use a List in a program, ask yourself the question: Does the order of the elements matter?

In this case, they don't. Use the Set interface and create as HashSet for your elementsInProcess variable.

private final Set<E> elementsInProcess = new HashSet<>();

When using a Set, it is extra important that the elements in the set has properly implemented hashCode and equals! (Or are using the default implementations inherited from the Object class)

Cleanup

I suggest you override the shutdown methods of a ThreadPoolExecutor to clear the elementsInProcess collection.

execute(Runnable command)

Please throw a UnsupportedOperationException or similar in this method:

public void execute(Runnable command) { }

Edit: Scratch that. I think palacsint is absolutely right. By doing nothing here, you're breaking the contract of the Executor interface (which you implement, whether you know it or not - by extending an ExecutorService). The Javadoc for the execute(Runnable command) method says:

  Executes the given command at some time in the future.

As it currently stands, you are not doing that and have no real ability of doing so in that method. This fact is an indication that you should not use extends ThreadPoolExecutor and instead use a private ThreadPoolExecutor executor;, and therefore using composition over inheritance.

Concurrency

If two threads would call execute at the same time, there is a concurrency issue on elementsInProcess. You need to synchronize on something there, for minimal blocking I suggest you do something like this:

synchronize (lock) {
    if (elementsInProcess.contains(element)) {
        return;
    }
    elementsInProcess.add(element);
}
super.execute(() -> consumer.accept(element));

Where lock is a private final Object lock = new Object();

Or, you could utilize the fact that the .add method of a set actually returns a boolean indicating if the add was successful and use a Collections.synchronizedSet.

if (!elementsInProcess.add(element)) {
    return;
}
super.execute(() -> consumer.accept(element));

It is important when using a synchronized Set that you only do one method call here, as there could otherwise be a gap between .contains and .add which would again cause concurrency issues between threads.


From object to array with revert and regroup Basically, I need to regroup keys by values from an object to an array

An example worth a thousand words in my case, as I don't know how to clearly explain my problem.

Here is my input object:

{
    "Data 1": 0,
    "Data 2": 1,
    "Data 3": 3,
    "Data 4": 0
}

What I should output:

[
    ["Data 1", "Data 4"], // data 1 and 4 are grouped as they both "worth" 0
    ["Data 2"], // data 2 is alone to "worth" 1
    ["Data 3"] // data 3 is alone to "worth" 3
]

First solution:

var output = [];
for (var data in input) { // my input object
    var value = input[data];
    if (output[value] == undefined) {
        output[value] = [];
    }
    output[value].push(data);
}
console.log(output);

It works well for most of the cases, except the one presented at the beginning: if I skip a value - say 0, 1, 3 - then my output will be

[
    0: ["Data 1", "Data 4"],
    1: ["Data 3"],
    3: ["Data 4"]
]

What I understood so far is that since the key 2 is missing, JavaScript somehow understand the output as an associative array.

Final solution:

From the code below, I used a "trick", making an equivalent of array_values() from PHP:

function array_values(arr) {
    var tmp = [];
    for (key in arr) {
        tmp[tmp.length] = arr[key];
    }
    return tmp;
}

var output = [];
for (var data in input) { // my input object
    var value = input[data];
    if (output[value] == undefined) {
        output[value] = [];
    }
    output[value].push(data);
}
console.log(array_values(output));

This finally works perfectly fine.

Nevertheless, I feel like I could optimize and simplify my code. I fear that a big object could be long to process since I could have to parse it 2 times. Does anyone have a guess?

I assume that the values are no longer needed in the output array and that you only want to construct the final array that you receive after the call to your array_values function.

// I extend the data with another duplicated value for a better test.
var input = { "Data 1": 0, "Data 2": 1, "Data 3": 3, "Data 4": 0, "Data 5": 3 };

// No change, our output array.
var output = [];

// We will use an additional array to create a mapping between actual values
// and the output array's indices they are stored at.
var mapping = [];

for (var data in input) {
    // Check if the value of this input data is already present in the
    // mapping's array, if it is, append the actual data to the already
    // existing array.
    //
    // NOTE: Using `mapping[input[data]] == undefined` is not a secure method
    // to check if an array index exists, since the value might be undefined
    // and that is not what we want. It is like `isset` and `array_key_exists`
    // in PHP, if you are familiar with that language.
    if (input[data] in mapping) {
        // We access the output array via the mappings array and the original
        // input arrays value and push the data.
        output[mapping[input[data]]].push(data);
        // output[0].push("Data 4");
        // output[2].push("Data 5");
    }
    // No entry exists in the mapping's array.
    else {
        // Push will simply append the data, we also inline the creation of the
        // desired array for additional data for this index.
        output.push([ data ]);
        // output[0] = [ "Data 1" ];
        // output[1] = [ "Data 2" ];
        // output[2] = [ "Data 3" ];

        // Now we need to create the actual mapping in case other data has the
        // same value. We simply create a new index and store the output
        // array's current index there (note the `-1` since `length` starts at
        // one in contrast to the indices which start at zero).
        mapping[input[data]] = output.length - 1;
        // mapping[0] = 0;
        // mapping[1] = 1;
        // mapping[3] = 2;
    }
}

console.dir(output);


Removing the extension from a filename Let me know if the given code can be improved or optimized:

public static String getFileNameWithoutExtensiotn(File file) {
    String fileName = null;
    if (file.getName().startsWith(".")) {//if starts with "." like .htaccess
        fileName = file.getName();
    } else {
        try {
            fileName = file.getName().substring(0, file.getName().lastIndexOf("."));
        } catch (Exception e) {
            fileName = file.getName();
        }
    }

    return fileName;
}

Your code can be improved in few lines:

        String fileName = file.getName();
        if (fileName.indexOf(".") > 0)
            fileName = fileName.substring(0, fileName.lastIndexOf("."));
        return fileName;

As far as catching the exception, an advice, always catch the specific exception, not the generic, so you should be catching IOException or IndexOutOfBoundException instead of Exception. Also, it's always a good idea to log exception somehow.


Shortening Script - Add new class/css to an li in setIntervals I am currently working on a site where I have a bunch of tiles "fly in" to the screen.  I need to add a class to each tile on a delay so they come in one after another.  I have the following code that works, but if I want to add more tiles, I will have to edit the jQuery each time.

Is there a way to trim this script down so I don't have to manually add new animations to the new tiles?

function tilesAnimation() {

    setInterval(function () {
        $("ul.homepageTabs li").eq(0).css("display", "inline-block");
        $("ul.homepageTabs li").eq(0).addClass('animated fadeInDown');
        $(".loader").fadeOut();
    }, 1800);
    setInterval(function () {
        $("ul.homepageTabs li").eq(1).css("display", "inline-block");
        $("ul.homepageTabs li").eq(1).addClass('animated fadeInDown');
    }, 2100);
    setTimeout(function () {
        $("ul.homepageTabs li").eq(2).css("display", "inline-block");
        $("ul.homepageTabs li").eq(2).addClass('animated fadeInDown');
    }, 2400);
    setTimeout(function () {
        $("ul.homepageTabs li").eq(3).css("display", "inline-block");
        $("ul.homepageTabs li").eq(3).addClass('animated fadeInDown');
    }, 2700);
    setTimeout(function () {
        $("ul.homepageTabs li").eq(4).css("display", "inline-block");
        $("ul.homepageTabs li").eq(4).addClass('animated fadeInDown');
    }, 3000);
    setTimeout(function () {
        $("ul.homepageTabs li").eq(5).css("display", "inline-block");
        $("ul.homepageTabs li").eq(5).addClass('animated fadeInDown');
    }, 3300);
    setTimeout(function () {
        $("ul.homepageTabs li").eq(6).css("display", "inline-block");
        $("ul.homepageTabs li").eq(6).addClass('animated fadeInDown');
    }, 3600);
    setTimeout(function () {
        $("ul.homepageTabs li").eq(7).css("display", "inline-block");
        $("ul.homepageTabs li").eq(7).addClass('animated fadeInDown');
    }, 3900);
    setTimeout(function () {
        $("ul.homepageTabs li").eq(8).css("display", "inline-block");
        $("ul.homepageTabs li").eq(8).addClass('animated fadeInDown');
    }, 4200);

}

One simple solution would be to extract your common functionality into a method. We'll also cache the selector for speed (don't do this if the elements change).

var tabs = $("ul.homepageTabs li");

function animateAfter(index,timeout){

    setTimeout(function () {
        tabs.eq(index).css("display", "inline-block");
        tabs.eq(index).addClass('animated fadeInDown');
    }, timeout);
}

Then we can call it like so:

for(var i=3;i<8;i++){
   animateAfter(i,2400+300*i);
}

I noticed the first two are intervals, was that a mistake? If it was, you can start the loop at 0 (and a smaller timeout). If it wasn't you can can create a similar function for intervals.

Also, consider using queued animations.

We want all our animations to occur 300 mili-seconds (after each other) right?

(function(){
    var i=3; //the start index

    function myAnimEffect(next){
        $(this).eq(i).css("display", "inline-block").
               eq(i).addClass('animated fadeInDown');
       next();
    }
    var $el = $('ul.homepageTabs li');
    $el.delay(1800);
    for(var j=0;j<5;j++){ //5 is the number of lis
        $el.queue(myAnimEffect).delay(300);
    }
});