Multiple regex replacements I receive a string with a lot of characters that I don't need and I am trying to remove them and replace them with characters that I am able to work with. My current structure has me redefining the var multiple times which I feel is not efficient and can probably be done better. Please let me know of a more effective way I can do this.

I define the date then remove the unwanted characters then append the date to the "clean" string with and underscore:

var d = Date.now()
  var article = a.replace(/ |\./g, "_")
  article = article.replace(/\r?\n|\r/g,"")
  article = article.replace(/\$|\#|\[|\]/g, "")
  article = d + "_" + article

This does work but I am curious if there is a better way.

First, since you already did so inside of your two last expressions, with the same replacement:

article = article.replace(/\r?\n|\r/g,"")
article = article.replace(/\$|\#|\[|\]/g, "")

I'm puzzled why you didn't simply put both in a unique regexp:

article = article.replace(/\r?\n|\r|\$|\#|\[|\]/g, "")

Then to integrate with the 1st one, you might choose to:

join the two distincts replacements in a single line:

var article = a.replace(/ |\./g, "_").replace(/\r?\n|\r|\$|\#|\[|\]/g, "")

or use a map approach, either suche the one pointed by @greybeard's link, or like this way (even if it might look a bit too sohpisticated for only two cases):

var replacements = new Map([
    [/ |\./g, '_'],
    [/\r?\n|\r|\$|\#|\[|\]/g, '']
    ]),
    article = a;
replacements.forEach(function(value, key){
      article = article.replace(key, value);
    });

The most interesting aspect in this latter solution is that it may be easily expanded if more replacements are needed.

EDIT following a good suggestion from @Niet the Dark Absol.

As soon as there are several unique characters to look for, with the same replacement, this kind of regexp /(a|b|c)/ can be replaced by /[abc]/, which is both simpler and more efficient!

Any of the above proposed solutions can be improved this way, so the latter one becomes:

    var replacements = new Map([
        [/[ .]/g, '_'],
        [/[\r\n$#[\]]/g, '']
        ]),
        article = a;
    replacements.forEach(function(value, key){
          article = article.replace(key, value);
        });


Comparing dates with SharePoint PnP PowerShell I am wondering if should change my if statement that compares two dates if they are equal. Is there a better way of doing this operation? The code does what it supposed to do. No errors. The Code:

if((Get-date).ToString("yyy-MM-dd") -eq $ListItem["Aviseringsdatum"].ToLocalTime().ToString("yyy-MM-dd")){
Write-Output "True"
}

I think it's okay, but we can do better. Let's see what Get-Date returns. Run this:

> get-date | get-member

It shows that Get-Date returns a DateTime object, and it also lists all the properties that has. I notice that DateTime has a Date property. Looking on the Web, it says that this is the date part of the DateTime but with a zero time component. So let's use that instead of converting to "yyy-MM-dd":

if((Get-date).Date -eq $ListItem["Aviseringsdatum"].Date){
Write-Output "True"
}

Since this is a code review, let's improve the formatting, and indent and add spaces:

if ((Get-Date).Date -eq $ListItem["Aviseringsdatum"].Date) {
    Write-Output "True"
}

I think that's as far as you need to go. If it were my code, however, I would probably go a bit further.

I would use [DateTime]::Today instead (Get-Date).Date because I think it is clearer.

I would also introduce local variables. I think if you are ever using a "today" or "now" value in your code, then it's better to capture them in a variable for the sake of consistency so that all parts of the script agree on what "today" or "now" is. What if for instance your script ran over midnight? Then the (Get-Date).Date value would suddenly change. A local variable might be overkill in this case, but it's a good practice in general to avoid some subtle bugs.

I also like making local variables to capture bigger expressions because they make for self-documenting code. It might be overkill again in this case, but I will add a $notificationDate variable.

$today = [DateTime]::Today
$notificationDate = $ListItem["Aviseringsdatum"].Date
if ($notificationDate -eq $today) {
    Write-Output "True"
}

I think that $notificationDate -eq $today makes the logic very clear to anyone reading the code.


"Longest Substring Without Repeating Characters" in Python I was trying to solve this problem from Leetcode:

  Given a string, find the length of the longest substring without
  repeating characters.

  Examples:

  Given "abcabcbb", the answer is "abc", which the length is 3.

  Given "bbbbb", the answer is "b", with the length of 1.

  Given "pwwkew", the answer is "wke", with the length of 3. Note that
  the answer must be a substring, "pwke" is a subsequence and not a
  substring.

I would like to get some code review for the following solution:

class Solution:
    def lengthOfLongestSubstring(self, s):
      dict = {}
      start, end, curr_length, max_length = 0, 0, 0, 0
      while ( end < len(s)):
        char = s[end]

        if char not in dict:
            curr_length += 1
        else:
            start = max(dict[char], start)

        max_length = max(max_length, end - start + 1)
        dict[char] = end + 1
        end += 1
      return max_length

"""
    my logic is that
    * 1) Initialize a hashmap/dictionary/object that will map characters to their index.
    * 2) Initialize `start`, `end`, and `answer`
    * 3) Loop through the string while `end` is less than `length` of string
    * 4) Check if character at `end` is in hashmap
      * a) If so, repeating character exists between `start` and `end`. Set `start` to be the `max_length` between the value of character in the hashmap, and `start`

"""

And it passes all of the leetcode tests.

Nice solution, please find a few thoughts below:

Do you need to wrap it in class for leetcode? It's unnecessary otherwise.
Function names use snake_case and not camelCase, check PEP8 for more details. Hence it would be length_of_longest_substring.
Do not use dict as a variable name, it is already built-in to construct dictionaries.
I think a for-loop would be preferable over a while-loop in this case, it's more straightforward, less clunky, and easier to understand.
You update curr_length, but never actually use it to determine your max_length.
The docstring should come right after the class Solution and before the first function, and no line should be longer than 79 characters. Moreover, you seem to not have pasted the entire docstrings, it just ends mid-sentence.


Rolling Hash algorithm I would like any ideas or suggestions on how to enhance the code for rolling hash algorithm and make if faster. Even if it is a change in one line. I tried to add a list, because I thought it would be more efficient, but it didn't work properly. So any ideas on how to enhance it. I would appreciate the assist.

import time

class RollingHash:
def __init__(self, text, sizeWord):
    self.text = text
    self.hash = 0
    self.sizeWord = sizeWord

    for i in range(0, sizeWord):
        # ord maps the character to a number
        # subtract out the ASCII value of "a" to start the indexing at zero
        self.hash += (ord(self.text[i]) - ord("a") + 1) * (26 ** (sizeWord - i - 1))  # change the first / with *

    # start index of current window
    self.window_start = 0
    # end of index window
    self.window_end = sizeWord

def move_window(self):
    if self.window_end <= len(self.text) - 1:
        # remove left letter from hash value
        self.hash -= (ord(self.text[self.window_start]) - ord("a") + 1) * 26 ** (self.sizeWord - 1)
        self.hash *= 26
        self.hash += ord(self.text[self.window_end]) - ord("a") + 1
        self.window_start += 1
        self.window_end += 1

def window_text(self):
    return self.text[self.window_start:self.window_end]

def rabin_karp(word, text):
if word == "" or text == "":
    return None
if len(word) > len(text):
    return None

rolling_hash = RollingHash(text, len(word))
word_hash = RollingHash(word, len(word))
# word_hash.move_window()

for i in range(len(text) - len(word) + 1):
    if rolling_hash.hash == word_hash.hash:
        if rolling_hash.window_text() == word:
            print(i)
    rolling_hash.move_window()
return  'Pattern length: ' , len(word)

text = input("Text: ")
word = input("Pattern: ")
t1 = time.clock()
results = rabin_karp(word, text)
print(results)
t2 = time.clock()
print('Run Time:  %0.5f ms' % ((t2 - t1) * 1000.0))

You can do as a decent compiler would: precompute some expressions, and reduce the "strength" of others.
For worse rather than better, most opportunities for this are in the constructor.
Part of the performance of on object depends on its interface - I've included an alternative function that finds the (position of the) next match, if any.
Benchmarking is difficult, micro or not: beware "printing" and use help.

import timeit

class RollingHash:
    '''A rolling hash for a window of constant length into a text,
        both specified at construction.
    '''
    adjust = ord("a") - 1
    alphabet = 26

    def __init__(self, text, size_word):
        '''Set up a rolling hash for a window of size_word into text.'''
        self.text = text
        if len(text) < size_word:
            self.hash = None
            return
        rk = 0
        for c in text[:size_word]:
            rk = rk * self.alphabet + ord(c) - self.adjust
        self.hash = rk
        self.pos = -1
        self.window_start = 0
        self.window_end = size_word
        self.multiplier = RollingHash.alphabet ** (size_word - 1)

    def move_window(self):
        '''Advance window by one position.'''
        if self.window_end < len(self.text):
            # remove left letter from hash value
            self.hash = \
                (self.hash - (ord(self.text[self.window_start])
                              - RollingHash.adjust) * self.multiplier) \
                * RollingHash.alphabet \
                + ord(self.text[self.window_end]) - RollingHash.adjust
            self.window_start += 1
            self.window_end += 1

    def window_text(self):
        '''Return current window text.'''
        return self.text[self.window_start:self.window_end]

    def match(self, other):
        '''Return position of next match, or none.'''
        roll = self.hash
        text = self.text
        # "local copies" may help or hinder readability and performance
        start = self.window_start
        end = self.window_end
        limit = len(self.text)
        result = None
        while end < limit:
            if self.pos < other.hash == roll \
                and other.text == text[start:end] \
                and self.pos < start:
                result = self.pos = start
                break;
            roll = (roll - (ord(text[start])
                            - RollingHash.adjust) * self.multiplier) \
                * RollingHash.alphabet \
                + ord(text[end]) - RollingHash.adjust
            start += 1
            end += 1
        self.window_start = start
        self.window_end = end
        return result

verbose = True

def rabin_karp(word, text):
    '''Print indexes of matches for word in text.'''
    if word == "" or text == "":
        return None
    size_word = len(word)
    if size_word > len(text):
        return None

    rolling_hash = RollingHash(text, size_word)
    word_hash = RollingHash(word, size_word)

    for i in range(len(text) - size_word + 1):
        if rolling_hash.hash == word_hash.hash:
            if rolling_hash.window_text() == word:
                if verbose:
                    print(i)
            else:
                print(rolling_hash.window_text(), '<>', word, "at", i)
        rolling_hash.move_window()
    return 'Pattern length: ', size_word

def karp_rabin(word, text):
    '''Print indexes of matches for word in text.'''
    size_word = len(word)
    if not 0 < size_word <= len(text):
        return None

    rolling_hash = RollingHash(text, size_word)
    word_hash = RollingHash(word, size_word)

    while True:
        position = rolling_hash.match(word_hash)
        if position is None:
            return 'Pattern length: ', size_word
        if verbose:
            print(position)

if __name__ == '__main__':
    text = input("Text: ")
    word = input("Pattern: ")
    print(rabin_karp(word, text))
    print(karp_rabin(word, text))
    verbose = False
    glob = globals()
    # have a look at timeit.Timer.repeat() and autorange(), too
    print(timeit.timeit('results = rabin_karp(word, text)',
                        globals=glob, number=9999))
    print(timeit.timeit('results = karp_rabin(word, text)',
                        globals=glob, number=9999))


Two similar API calls PHP optimisation and structure I have the following script that I'm part way through and I'm not happy with it at all, it feels messy:

<?php
class Process {
    public $search = 'http://api.themoviedb.org/2.1/Person.search/en/json/xxx/';
    public $info = 'http://api.themoviedb.org/2.1/Person.getInfo/en/json/xxx/';

    public function returnId($name){
        return $this->returnJson($this->search . $name);
    }

    public function returnMovies($id){
        return $this->returnJson($this->info . $id);
    }

    public function returnJson($url){
        $raw = file_get_contents($url);
        $json = json_decode($raw, true);
        return $json;
    }
}

$p = new Process();
$first = urlencode($_POST['first']);
$second = urlencode($_POST['second']);

$id = $p->returnId($first);
$movies = $p->returnMovies($id[0]['id']);

Now the idea is that two parameters get sent to this script and each of them run through the first API call ($search) and then the ID field is extracted from that results and passed into the $info URL call. The two arrays that come back from returnMovies will be compared with each other and any matches will be popped into another array.

Now, has anyone got any way for this to be better in structure and cleanliness?

It's not finished yet but its a simple script that I'd like to tidy up before I go on.

I'd start firstly by moving your urlencode() into the class... You should assume that everything passed to a method is unclean and therefore act appropriately.

Secondly, I'd give the class a meaningful name - Process isn't entirely descriptive.

Throw some comments in - follow a set standard such as the PHPDoc format, using @param, @return etc. Always handy to have, even if the code doesn't become public or used by others.

Look at your public/private declarations - or lack of. Personally, I'd look to make your $search and $info URLs private, as well as your returnJson() method.

I'd possibly look to use cURL instead of file_get_contents() - due to some server configs disabling that etc.

Anyway, just a few suggestions!


The final word on contiguous iterators Following the paradigms of SCARY iterators, there should only be a single iterator-type for contiguous iterators per value-type.
Here my code for that iterator, working with C++11+:

#include <type_traits>
#include <iterator>

template<class T, class U>
struct is_more_cv_qualified : std::integral_constant<bool, std::is_same<T, const volatile U>{}
    || std::is_same<T, const U>{} || std::is_same<T, volatile U>{}>;

struct contiguous_iterator_tag : std::random_access_iterator_tag {};
struct mutable_contiguous_iterator_tag : contiguous_iterator_tag, std::output_iterator_tag {};

template<class T>
struct contiguous_iterator_base {
    constexpr bool operator==(contiguous_iterator_base other) const noexcept { return _p == other.p; }
    constexpr bool operator!=(contiguous_iterator_base other) const noexcept { return _p != other.p; }
    constexpr bool operator<(contiguous_iterator_base other) const noexcept { return _p < other._p; }
    constexpr bool operator>(contiguous_iterator_base other) const noexcept { return _p > other._p; }
    constexpr bool operator<=(contiguous_iterator_base other) const noexcept { return _p <= other._p; }
    constexpr bool operator>=(contiguous_iterator_base other) const noexcept { return _p >= other._p; }
    constexpr std::ptrdiff_t operator-(contiguous_iterator_base other) const noexcept { return _p - other._p; }
protected:
    constexpr contiguous_iterator_base& operator=(const contiguous_iterator_base&) noexcept = default;
    T* _p = nullptr;
};

template<class T>
struct contiguous_iterator : contiguous_iterator_base<std::remove_cv<T>::type>
, std::iterator<typename std::conditional<std::is_const<T>{}, contiguous_iterator_tag, mutable_contiguous_iterator_tag>::type, T> {
    constexpr contiguous_iterator() noexcept = default;
    template<class U, class = std::enable_if<is_more_cv_qualified<T, U>{}>::type>
    constexpr contiguous_iterator(contiguous_iterator<U> other) noexcept : contiguous_iterator_base<std::remove_cv<T>::type>(other) {}
    constexpr explicit contiguous_iterator(contiguous_iterator_base<std::remove_cv<T>::type> other) noexcept : decltype(other)(other) {}
    constexpr explicit contiguous_iterator(T* p) noexcept { _p = const_cast<decltype(_p)>(p); }
    template<class U>
    constexpr contiguous_iterator<U> rebind() const noexcept { return contiguous_iterator<U>(_p); }
    constexpr T* operator->() const noexcept { return  _p; }
    constexpr T& operator* () const noexcept { return *_p; }
    constexpr T& operator[](std::ptrdiff_t d) const noexcept { return _p[d]; }
    constexpr contiguous_iterator& operator++() noexcept { ++_p; return *this; }
    constexpr contiguous_iterator operator++(int) noexcept { auto r = *this; ++*this; return r; }
    constexpr contiguous_iterator& operator--() noexcept { --_p; return *this; }
    constexpr contiguous_iterator operator--(int) noexcept { auto r = *this; --*this; return r; }
    constexpr contiguous_iterator& operator+=(std::ptrdiff_t d) noexcept { _p += d; return *this; }
    constexpr contiguous_iterator& operator-=(std::ptrdiff_t d) noexcept { _p -= d; return *this; }
    constexpr contiguous_iterator operator+(std::ptrdiff_t d) const noexcept { return contiguous_iterator(_p) += d; }
    constexpr contiguous_iterator operator-(std::ptrdiff_t d) const noexcept { return contiguous_iterator(_p) -= d; }
protected:
    using contiguous_iterator_base<std::remove_cv<T>::type>::_p;
};

// smoke test

int main() {
    contiguous_iterator_base<int>{};
    contiguous_iterator<int> a;
    contiguous_iterator<const int> b;
    b = a;
    a = b.rebind<int>();
    //a.rebind<char>(); error
}

There really isn't much to say here. This looks really good.

Typos

You have a couple typos in your code on your equality operators. You're comparing against other.p instead of other._p.

Not restrictive enough

We have this constructor:

constexpr explicit contiguous_iterator(T* p);

But that means if we have contiguous_iterator<Base>, we can construct it with a Derived*. We definitely need to disallow this.

On a similar vein, rebind<U> should be restricted to those Us which are the same type as T and are at least as cv-qualified.

Add another alias

Rather than writing this:

constexpr explicit contiguous_iterator(contiguous_iterator_base<std::remove_cv_t<T>> other) noexcept : decltype(other)(other) {}

If you added a using base = contiguous_iterator_base<std::remove_cv_t<T>>;, you could write:

constexpr explicit(base other) noexcept : base(other) {}

which makes way more sense.

Missing alias

You're using the alias templates for the type traits throughout, except you're using typename std::conditional<X>::type instead of std::conditional_t.

Line widths

Some of these lines are really long! Add some line breaks.

Why?

I guess lastly... what does contiguous_iterator<T> accomplish that simple T* doesn't?


Find neighboring PINs on a numeric keypad I am new to coding and to apply for an internship I need to solve several tasks. This is one of them. I wrote working code, but the program says it takes too long to complete. Help me speed up my code please.

Requirements:

Time limit - 1s
Memory Limit - 64mb
Input- standard input
Output- standard output

There is a task:

  Hi, your colleague forgot the pin code from the door to the office.
  The keyboard is:

        ┌───┬───┬───┐
        │ 1 │ 2 │ 3 │
        ├───┼───┼───┤
        │ 4 │ 5 │ 6 │
        ├───┼───┼───┤
        │ 7 │ 8 │ 9 │
        └───┼───┼───┘
            │ 0 │
            └───┘

  A colleague recalls that it seems that the pin code was 1357, but
  perhaps each digit should be shifted horizontally or vertically, but
  not diagonally.

  For example, instead of 1 it can be 2 or 4, and instead of 5 it can be
  2, 4, 6 or 8.

  Help a colleague get into the office.

  Input:
  A string with a pin code. The length of the string is from
  1 to 8 digits, for example, 1357.

  Output:
  The source pin code and possible pin codes with regard to
  shifts, sorted and separated by a comma.

  Do not forget that the pin codes must be strings, as they can begin
  with 0, and must be sorted like strings.

  Examples

  Input: 11
  Output: 11,12,14,21,22,24,41,42,44

  Input: 8
  Output: 0.5,7,8,9

  Input: 46
  Output: 13,15,16,19,43,45,46,49,53,55,56,59,73,75,76,79

Formalization:

var readline = require("readline");
           var rl = readline.createInterface(process.stdin, process.stdout);
           rl.on("line", function(line) {
             console.log("0,5,7,8,9")
             rl.close();
           }).on("close",function(){
             process.exit(0);
           });

MY code

const readline = require('readline');

const rl = readline.createInterface({
 input: process.stdin,
 output: process.stdout
});

function drawMatrix(InArray) {
   let matrix= [];
   for (let z = 0; z < InArray.length; z++) {
       switch (InArray[z]) {
           case '1':
               row = [1, 2, 4];
               break;
           case '2':
               row = [1, 2, 3, 5];
               break;
           case '3':
               row = [2, 3, 6];
               break;
           case '4':
               row = [1, 4, 5, 7];
               break;
           case '5':
               row = [2, 4, 5, 6, 8];
               break;
           case '6':
               row = [3, 5, 6, 9];
               break;
           case '7':
               row = [4, 7, 8];
               break;
           case '8':
               row = [0, 5, 7, 8, 9];
               break;
           case '9':
               row = [6, 8, 9];
               break;
           case '0':
               row = [0, 8];
               break;
       };
      // console.log(row);
       matrix.push(row);
   };
  //console.log(matrix);
  return matrix;
};

function keyVars(matrix) {
   let positions = [];
   let someVar = [];
   let allVars = [];
   let totalCount = matrix.length;

   for (let  i = 0; i < totalCount; i++) {
       positions[i] = 0;
   };

   let next = totalCount - 1;

   while (next >= 0) {
       for (let i=0; i < totalCount; i++) {
           someVar.push(matrix[i][positions[i]]);
       };

       next = totalCount - 1;

       let Var = someVar.join('');
       someVar = [];

       while (next >=0 && (positions[next] + 1) >= matrix[next].length) {
       next--;
       };

       positions[next]++;

       for (let i = next + 1; i < totalCount; i++) {
           positions[i] = 0;
       };

       allVars.push(Var);
   };

   let allVarsStr = allVars.join();
   return allVarsStr;
};

rl.on('line', (input) => {
   console.log(`Входные данные: ${input}`);
   console.log(`Выходные данные: ${keyVars(drawMatrix(input))}`);
   rl.close();
 }).on("close",function(){
   process.exit(0);
 });

```

We can simplify this quite a bit.  At a high level, note that all you are doing is:

Converting each digit of the input to an array of possible neighbors.
Now you have an array of neighbor arrays.
The answer is simply the cartesian cross product of those neighbor arrays.
Sort them.
Turn the into back into strings.

The cartesian cross-product is a simple utility method.

The neighbor possibilities are more compactly expressed as a JS object than by a switch statement, and this representation also makes it easy to map over them.

Putting it all together we get:

function xprod(...arrays) {
  return arrays.reduce((m, x) => m.flatMap(
    results => x.map(elm => results.concat([elm]))
  ), [ [] ])
}

function possibleCodes(code) {
  const neighbors = {
    '1': [1, 2, 4],
    '2': [1, 2, 3, 5],
    '3': [2, 3, 6],
    '4': [1, 4, 5, 7],
    '5': [2, 4, 5, 6, 8],
    '6': [3, 5, 6, 9],
    '7': [4, 7, 8],
    '8': [0, 5, 7, 8, 9],
    '9': [6, 8, 9],
    '0': [0, 8],
  }
  const possibilitiesByDigit = code.split('').map(x => neighbors[x])
  return xprod(...possibilitiesByDigit).map(x => x.join('')).sort()
}


Maximum Sum of Non-Adjacent Elements Purpose

I recently came across an interview question that stumped me for a while

  Given a list of integers, write a function that returns the largest sum of non-adjacent numbers.

I am looking for feedback around both my implementation and the correctness of my proposed solution (I have some test
cases, but I could have missed a case).

Approach

For arrays with 1 or 2 values, the answer is trivial.

For arrays with more than 2 values, keep track of the maximum sum from the preceding non-adjacent elements that
includes the most recent non-adjacent element, and the maximum sum from the preceding non-adjacent elements that
excludes the most recent non-adjacent element.

So for [3, 1, 1, 5, 1] at index 3, the maximum sum from preceding non-adjacent elements that includes the most recent
non-adjacent element is 1, while the maximum sum from preceding non-adjacent elements that excludes the most recent
non-adjacent element is 3.

Taking the max of these two sums and adding them to the current value will produce the maximum sum for non-adjacent
elements up to that index. Continuing this process for each remaining index and then comparing the last two calculated
sums will produce the maximum sum.

Let's walk through this process with [3, 1, 1, 5, 1].

At index 0, the max sum is 3
At index 1, the max sum is 1
At index 2, the max sum from including the most recent non-adjacent element is 3, while the max sum from excluding
the most recent non-adjacent element is 0. Thus, we choose 3 and add that to the current value (1). 4 is now
the max non-adjacent sum for index 2.
At index 3, the max sum from including the most recent non-adjacent element is 1, while the max sum from excluding
the most recent non-adjacent element is 3. Thus, we choose 3 and add that to the current value (5). 8 is now
the max non-adjacent sum for index 3.
At index 4, the max sum from including the most recent non-adjacent element is 4, while the max sum from excluding
the most recent non-adjacent element is 3. Thus, we choose 4 and add that to the current value (1). 5 is now
the max non-adjacent sum for index 4.
The final two sums were 8 and 5 - we take the max (8).

Apologies if this was confusing to follow, it was difficult to describe my thought process.

Implementation

public class MaximumNonAdjacentElementSumIdentifier {
    public static int identify(int[] values) {
        if (values == null || values.length == 0) {
            throw new RuntimeException("Unable to identify sum");
        }

        if (values.length == 1) {
            return values[0];
        }

        int firstSum = 0;
        int secondSum = values[0];
        int previousValueSum = values[1];

        for (int i = 2; i < values.length; i++) {
            int value = values[i];
            int maximumCurrentElementSum = Math.max(firstSum, secondSum) + value;
            firstSum = Math.max(firstSum, secondSum);
            secondSum = previousValueSum;
            previousValueSum = maximumCurrentElementSum;
        }

        return Math.max(secondSum, previousValueSum);
    }
}

Variable names

firstSum and secondSum variable names aren't really descriptive of their purpose. Let's give them names according to the algorithm description:

int sumExcludingMostRecentNonAdjacentElement = 0;
int sumIncludindMostRecentNonAdjacentElement = values[0];
int sumUpToPreviousElement = values[1];

That's too verbose and it makes sense to remove common parts:

int excluding = 0;
int including = values[0];
int upToPrevious = values[1];

for (int i = 2; i < values.length; i++) {
    int value = values[i];
    int upToCurrent = Math.max(excluding, including) + values[i];
    excluding = Math.max(excluding, including);
    including = upToPrevious;
    upToPrevious = upToCurrent;
}

Negative numbers

The algorithm is based on the following assumption:

  Taking the max of these two sums and adding them to the current value will produce the maximum sum for non-adjacent elements up to that index.

which doesn't take negative numbers into account:

if both of these sums are negative they should not be added to the result;
if the current value is negative it should not be added to the result.

To give an example, for the input 0, -1, -1 the method returns -1.

You can try to fix that by carefully keeping sums as positive and handling negative values:

int value = Math.max(values[i], 0);

You will also need to fix the initialization:

int including = Math.max(values[0],0);
int upToPrevious = Math.max(values[1], 0);

, and the trivial case:

if (values.length == 1) {
    return Math.max(values[0], 0);
}

As an alternative, you can replace negative elements with zeroes before processing.

Refactoring/understanding.

As far as I can see, the code should work correctly at this point. But both the algorithm description and the code are hard to follow with all these transitions:

int excluding = 0;
int including = Math.max(values[0], 0);
int upToPrevious = Math.max(values[1], 0);

for (int i = 2; i < values.length; i++) {
    int value = Math.max(values[i], 0);
    int upToCurrent = Math.max(excluding, including) + values[i];
    excluding = Math.max(excluding, including);
    including = upToPrevious;
    upToPrevious = upToCurrent;
}
return Math.max(including, upToPrevious);

Let's try to make code easier to understand step-by-step.

Currently, upToPrevious refers to the upToCurrent from the previous step. upToCurrent values for each iteration can be stored in an array, it saves us an assignment and hopefully helps us to derive a recurrence relationship between values:

int n = values.length;
//upToSums[i] refers to the upToCurrent value at the i-th step
int[] upToSums = new int[n];
upToSums[1] = Math.max(values[1], 0);
for (int i = 2; i < n; i++) {
    int value = Math.max(values[i], 0);
    upToSums[i] = Math.max(excluding, including) + value;
    excluding = Math.max(excluding, including);
    including = upToSums[i-1];
}
return Math.max(including, upToSums[n-1]);

Turns out, including referes upToSums[i-1] from the previous step, we can replace it with upToSums[i-2]:

upToSums[0] = Math.max(values[0], 0);
upToSums[1] = Math.max(values[1], 0);
for (int i = 2; i < n; i++) {
    int value = Math.max(values[i], 0);
    upToSums[i] = Math.max(excluding, upToSums[i-2]) + value;
    excluding = Math.max(excluding, upToSums[i-2]);
}
return Math.max(upToSums[n-2], upToSums[n-1]);

Now it's easier to see what's going on with excluding:

At step 2 excluding is 0.
At every other step excluding is the maximal element of upToSums from 0 to i-3.

Let's define an array for the maximum value of upToSums and calculate excluding based on that:

int[] maxSums = new int[n];
for (int i = 2; i < n; i++) {
    int value = Math.max(values[i], 0);
    int excluding = i==2 ? 0 : maxSums[i-3];
    upToSums[i] = Math.max(excluding, upToSums[i-2]) + value;
    maxSums[i-2] = Math.max(excluding, upToSums[i-2]);
}

This code still vaguely follows your algorithm description as "maximum sum from the preceding non-adjacent elements that excludes the most recent non-adjacent element" is somewhat equivalent to "maximum sum of non-adjacent elements for the sequence with the last three elements removed".

Notice, that maxSums[i-2] is calculated twice. We can reuse the result by changing the order of calculations:

    int value = Math.max(values[i], 0);
    maxSums[i-2] = Math.max(i==2 ? 0 : maxSums[i-3], upToSums[i-2]);
    upToSums[i] = maxSums[i-2]+ value;

It's weird that maxSums[i-2] is calculated at the i-th step. We can define starting values and calculate maxSums[i] right after upToSums[i]:

int[] maxSums = new int[n];
maxSums[0] = upToSums[0];
maxSums[1] = Math.max(upToSums[0], upToSums[1]);
for (int i = 2; i < n; i++) {
    int value = Math.max(values[i], 0);
    upToSums[i] = maxSums[i-2]+ value;
    maxSums[i] = Math.max(maxSums[i-1], upToSums[i]);
}

Also, we can use maxSums to return the result as the maximum is already calculated:

//maximal sum is already calculated
return maxSums[n-1];

Code so far (without trivial cases):

int n = values.length;
int[] upToSums = new int[n];
upToSums[0] = Math.max(values[0], 0);
upToSums[1] = Math.max(values[1], 0);
int[] maxSums = new int[n];
maxSums[0] = upToSums[0];
maxSums[1] = Math.max(upToSums[0], upToSums[1]);
for (int i = 2; i < n; i++) {
    int value = Math.max(values[i], 0);
    upToSums[i] = maxSums[i-2]+ value;
    maxSums[i] = Math.max(maxSums[i-1], upToSums[i]);
}
return maxSums[n-1];

At this point upToSums is never used and can be eliminated:

int n = values.length;
int[] maxSums = new int[n];
maxSums[0] = Math.max(values[0], 0);
maxSums[1] = Math.max(Math.max(values[1], 0), maxSums[0]);
for (int i = 2; i < n; i++) {
    int value = Math.max(values[i], 0);
    maxSums[i] = Math.max(maxSums[i-1], maxSums[i-2]+ value);
}
return maxSums[n-1];

At last we also separate the handling of negative numbers from the main algorithm that will work with an array of non-negative numbers:

//replacing negative values with 0
int n = values.length;
for(int i=0; i<n; i++) {
    if(values[i]<0) {
        values[i] = 0;
    }
}

//trivial case
if(n==0) {
    return values[0];
}

int[] maxSums = new int[n];
maxSums[0] = values[0];
maxSums[1] = Math.max(values[1], values[0]);
for (int i = 2; i < n; i++) {
    maxSums[i] = Math.max(maxSums[i-1], maxSums[i-2]+ values[i]);
}
return maxSums[n-1];

This code is logically equivalent to the previous version, but it's easier to follow. Actually, the main part of the algorithm (recursive formula) is written in the code itself:

  The largest sum of non-adjacent elements (maxSums(i)) for the first i elements of the list of non-negative numbers a is defined using the recurrence
  maxSums(i) = max(maxSums(i-1), maxSums(i-2)+a[i])
  with initial conditions:
  maxSums(0) = a[0]
  maxSums(1) = max(a[0], a[1])

Sum of zero elements

In its current form, the algorithm will return 0 for the list of negative numbers. This may or may not be acceptable, as the question doesn't state that the sum should contain at least one element. You'll need to clarify the particular requirements with your interviewer and, if necessary, write the code to detect this special case.


Safely storing and output data On my application I use this method to store and to output the data.  I would like to know if it is safely and correct.

##store the data###
//sanitize
function clean($testo)
{
    $config = HTMLPurifier_Config::createDefault();
    $purifier = new HTMLPurifier($config);
    $testo = $purifier->purify($testo);

    $testo = mysql_real_escape_string($testo);
    return $testo;
}
$value = clean($_POST[value]);
//the clean function contain mysql_real_escape_string and htmlpurifier class
$sql = mysql_query("insert into table values(null,$value);");

##output the data####
$sql = mysql_query("select * from table");
$val = mysql_fetch_array($sql);
function echoValue($valore){
    $valore = htmlspecialchars(strip_tags(stripslashes($valore)), ENT_QUOTES, "UTF-8");
    return $valore;

}
echo echoValue($val);
// the echoValue function contain strip_tags and htmlentities

To "Safely" store data, please don't use mysql_* functions. Use PDO or mysqli.

Also, use PreparedStatements wherever you use "where" clause in queries. That will protect you against injection.

http://au1.php.net/pdo.prepared-statements


Risks related to overriding attribute access I have written ExtendedConfigParser class that adds ability to dot access config values like ExtendedConfigParser().section_name.option_name:

from ConfigParser import SafeConfigParser

class ExtendedConfigParser(SafeConfigParser):

    def __init__(self, config_file=None, type_conversion={}, *args, **kwargs):
        self.config_file = config_file
        self.type_conversion = type_conversion
        SafeConfigParser.__init__(self, *args, **kwargs)
        self.chain = []
        self.cmap = {}
        if config_file:
            self.read(config_file)

    def section2dict(self, secname):
        opts = self.options(secname)
        def opt_conv_type(optname, value):
            if optname in self.cmap:
                return self.cmap[optname](value)
            return value
        return {o:opt_conv_type(o, self.get(secname, o)) for o in opts}

    def add_type_conversion_map(self, cmap):
        self.cmap.update(cmap)

    def __getattr__(self, attrname):
        if attrname in self.sections():
            self.chain.append(attrname)
            return self
        if len(self.chain) == 1 and attrname in self.section2dict(self.chain[0]):
            return self.section2dict(self.chain[0])[attrname]
        self.chain = []
        return getattr(SafeConfigParser, attrname)(self)

Let's have config file like this:

[globals]

webui_port = 8888

redis_host = 127.0.0.1
redis_port = 5600

Then it works like this:

cmap = {'redis_port':int, 'webui_port':int}
cfg = ExtendedConfigParser(config_file=confile, type_conversion=cmap)
print cfg.globals.webui_port

What I do not like about the code above is that I'm doing sort of manual attribute resolution in __getattr__. Is there a better (cleaner, more general) solution?

And also what will happen if a section name happens to be called like SafeConfigParser attribute? It would override it, correct? How can I avoid that?

I'd start with the caveat that you're buying some syntax sugar at the cost of possible bugs - the need to do the attribute resolution, and as you point out the possibility of name clashes, makes it seem like it may not be worth the effort unless you need to access these class properties in lots of places in your code.

That said you can simplify this by converting your sections - which are dictionaries -- to namedtuples:

from collections import namedtuple

def section2dict(self, secname):
   if secname in self.__dict__:
      raise KeyError, "%s is cannot be used as a property name" % secname
      opts = self.options(secname)
      #nb: I didn't bother recreating the type code, but you would do it here...
      nt = namedtuple(secname, opts.keys())(**opts)
      self.__dict__[secname] = nt

By sticking the result into dict you don't have to override getattr yourself, which limits the intervention to a single place although it does deprive you of the option to do guard checks for bad values, etc.   This example would except on name collisions and get you off the hook for manually redirecting double-dotted attribute queries.


Getting a count of correct and incorrect test attempts I need to get a count of correct and incorrect attempts per testID:

select
    AttempID,
    Test_ID,
    Date,
    SubjectID,
    Date,
    sum(Correct) as Correct,
    sum(InCorrect) as InCorrect
from tbldemo
group by
    AttempID,
    Test_ID,
    Date,
    SubjectID_name,
    Date

Do I need to group by each column other column, and then sum over Correct and Incorrect? AttempID, Test ID, date and subject ID combinations will remain constant.

  AttempID  Test ID   Date      Subject ID  Correct  Incorrect
   1          2      2014-05-12     2          0       1
   1          2      2014-05-12     2          0       1
   1          2      2014-05-12     2          1       0
   1          2      2014-05-12     2          0       1
   1          2      2014-05-12     2          1       0
   1          2      2014-05-12     2          0       1
   1          2      2014-05-12     2          1       0
   1          2      2014-05-12     2          1       0
   1          2      2014-05-12     2          0       1
   1          2      2014-05-12     2          1       0
   1          2      2014-05-12     2          0       1
   1          2      2014-05-12     2          0       1
   1          2      2014-05-12     2          0       1
   1          2      2014-05-12     2          0       1
   3          4      2014-05-12     2          0       1
   3          4      2014-05-12     2          1       0
   3          4      2014-05-12     2          1       0
   3          4      2014-05-12     2          0       1
   3          4      2014-05-12     2          0       1
   3          4      2014-05-12     2          0       1
   3          4      2014-05-12     2          1       0

You don't need the second Date in your select or grouping.  Other than that, the query itself looks fine.

I'm a bit more worried about how your table is built, though.

There appear to be duplicate records.  How would you go about selecting a specific record?
Correct and Incorrect are functionally dependent: if one has 0, the other must have 1.

If you have control over the structure of this table, you may want to look into normalising it.


SSMTP configuration in Ubuntu 18.04 with Bash in my own system (one user) This is how I configure the utility SSMTP that allows me to send mail via smtp protocol through a Google-Gmail proxy instead I'll have to manually configure an "heavier" Postfix bi-directional email server on my environment. I configure it in my own server environment that only I uses.

#!/bin/bash

myHostName="$HOSTNAME"
read -sp "Please paste your Gmail proxy email address (due to pasting, no verfication needed): " gmail_proxy_email_address  && echo
read -sp "Please paste your Gmail proxy email password (due to pasting, no verfication needed):" gmail_proxy_email_password && echo

cat <<-EOF > /etc/ssmtp/ssmtp.conf
    root=${gmail_proxy_email_address}
    AuthUser=${gmail_proxy_email_address}
    AuthPass=${gmail_proxy_email_password}
    hostname=${myHostName}
    mailhub=smtp.gmail.com:587
    rewriteDomain=gmail.com
    FromLineOverride=YES
    UseTLS=YES
    UseSTARTTLS=YES
EOF

Would you do this even otherwise? Something to shorten? Some alternative utility maybe?

First, I recommend set -eu early in the script to abort on errors and to make any use of unset variables be an error.

Second, If HOSTNAME is unset, we probably want to fallback to running the hostname command.  There seems little point in assigning it to a variable we use only once:

# (simplified to show only the relevant part)
cat <<EOF
    hostname=${HOSTNAME:-$(hostname)}
EOF

Then we can delete the line myHostName="$HOSTNAME", as it's no longer used.

Third, the other thing missing is umask 077 to ensure that the file isn't created readable by anyone else (important, as it contains cleartext credentials).

When reading email address, it shouldn't be necessary to hide it using read -s - only the password ought to be a secret.

When reading the password, there's no need to mention that confirmation is not required, as we wouldn't normally expect it here: we use confirmation when creating a password (so the user doesn't accidentally lock herself out of her account by a simple invisible typing error).  Here, we're entering an existing password, and any error can be corrected just by re-running the script.

Edited script

Applying my suggestions, we get:

#!/bin/bash

set -eu

umask 077  # Ensure others can't read the file

read -p "Please paste your Gmail proxy email address: " \
     gmail_proxy_email_address
read -sp "Please paste your Gmail proxy email password:" \
     gmail_proxy_email_password && echo

cat <<-EOF > /etc/ssmtp/ssmtp.conf
    root=${gmail_proxy_email_address}
    AuthUser=${gmail_proxy_email_address}
    AuthPass=${gmail_proxy_email_password}
    hostname=${HOSTNAME:-$(hostname)}
    mailhub=smtp.gmail.com:587
    rewriteDomain=gmail.com
    FromLineOverride=YES
    UseTLS=YES
    UseSTARTTLS=YES
EOF


Find the closest enemy I'm currently creating a 2d game in python. I have to calculate the closest enemy so the player knows which one to target. I have the following function to accomplish this task:

import math

def find_closest(character: tuple, enemies: list) -> (int, int):
    """
    Finds the closest enemy in enemies

    :param character: An (x, y) representing the position of the character\n
    :param enemies: A list of tuples (x, y) that represent enemies

    :return: A tuple (x, y) of the closest enemy
    """
    closest_enemy = None
    smallest_distance = 100_000_000 # Set to large number to ensure values can be less #
    for enemy in enemies:
        distance = math.sqrt(math.pow(character[0] - enemy[0], 2) + (math.pow(character[1] - enemy[1], 2)))
        if distance < smallest_distance:
            closest_enemy = (enemy[0], enemy[1])
            smallest_distance = distance
    return closest_enemy

if __name__ == "__main__":

    # Test Case #

    character = (5, 6)
    enemies = [(1, 2), (3, 4), (7, 6), (11, 4)]
    closest = find_closest(character, enemies)
    print(closest)

I would like to know if there's any better way to go about doing this. While it is a simple function, I'm sure there's some built in python function that can accomplish this even shorter. I use tuples because, in my opinion, they're the best way to represent coordinates. Any and all suggestions and/or improvements are welcome and appreciated.

For small lists of enemies, linearly scanning all of them and computing the distance to the character is sufficient. However, if you have many enemies, a more efficient data structure is needed.

If your list of enemies does not change (or changes less often than you need to find the closest enemy), I would use scipy.spatial.cKDTree. kd-trees take \$\mathcal{O}(n\log n)\$ time to build, but afterwards each query only takes \$\mathcal{O}(\log n)\$.

from scipy.spatial import cKDTree as KDTree

def find_closest_kdtree(character: tuple, enemies: KDTree) -> (int, int):
    """
    Finds the closest enemy in enemies

    :param character: An (x, y) representing the position of the character\n
    :param enemies: A KDTree that represent enemies

    :return: A tuple (x, y) of the closest enemy
    """
    _, i = enemies.query([character], 1)
    return i[0]

if __name__ == "__main__":

    # Test Case #

    character = (5, 6)
    enemies = [(1, 2), (3, 4), (7, 6), (11, 4)]
    enemies_tree = KDTree(enemies)
    closest = enemies[find_closest_kdtree(character, enemies_tree)]
    print(closest)

If you do regularly need to update the list of enemies (because they are spawned, get killed, move off-screen, etc), you might be able to use a R* tree instead:

from rtree.index import Rtree

def find_closest_rtree(character: tuple, enemies) -> (int, int):
    """
    Finds the closest enemy in enemies

    :param character: An (x, y) representing the position of the character\n
    :param enemies: A KDTree that represent enemies

    :return: A tuple (x, y) of the closest enemy
    """
    return next(enemies.nearest(character, 1, objects='raw'))

if __name__ == "__main__":

    # Test Case #

    character = (5, 6)
    enemies = [(1, 2), (3, 4), (7, 6), (11, 4)]
    enemies_tree = Rtree()
    for i, p in enumerate(enemies):
        enemies_tree.insert(i, p+p, p)

    closest = find_closest_rtree(character, enemies_tree)
    print(closest)

Here is how the different methods compare performance wise, including the implementation using min and your original implementation:

Note that building all the R* trees for this took multiple minutes, while building all the KDTrees took only a couple of seconds. So you would probably have to rebuild the KDTree quite often for it to be worth it to switch to the R* tree.

In case you are interested, this is how I generated that graph:

import numpy as np
import pandas as pd
from functools import partial
import timeit
from scipy.spatial import cKDTree as KDTree
from rtree.index import Rtree

def get_time(func, *x):
    timer = timeit.Timer(partial(func, *x))
    t = timer.repeat(repeat=5, number=1)
    return np.min(t), np.std(t) / np.sqrt(len(t))

def get_times(func, inputs):
    return np.array(list(map(partial(get_time, func), inputs))

def find_closest(character: tuple, enemies: list) -> (int, int):
    closest_enemy = None
    smallest_distance = 100_000_000 # Set to large number to ensure values can be less #
    for enemy in enemies:
        distance = math.sqrt(math.pow(character[0] - enemy[0], 2) + (math.pow(character[1] - enemy[1], 2)))
        if distance < smallest_distance:
            closest_enemy = (enemy[0], enemy[1])
            smallest_distance = distance
    return closest_enemy

def find_closest_min(character: tuple, enemies: list) -> (int, int):
    def find_enemy_distance(enemy):
        return math.hypot((character[0] - enemy[0]), (character[1] - enemy[1]))
    return min(enemies, key=find_enemy_distance)

def find_closest_kdtree(character: tuple, enemies: KDTree) -> (int, int):
    _, i = enemies.query([character], 1)
    return i[0]

def find_closest_rtree(character: tuple, enemies) -> (int, int):
    return next(enemies.nearest(character, 1, objects='raw'))

def find_closest_kdtree_with_build(character: tuple, enemies) -> (int, int):
    enemies_tree = KDTree(enemies)
    _, i = enemies_tree.query([character], 1)
    return enemies[i[0]]

if __name__ == "__main__":
    character = 5, 6
    x = [list(map(tuple, np.random.randint(-n, n, (n, 2))))
         for n in np.logspace(1, 6, dtype=int)]
    kdtrees = [KDTree(v) for v in x]
    rtrees = []
    for y in x:
        rtrees.append(Rtree())
        for i, v in enumerate(y):
            rtrees[-1].insert(i, v+v, v)

    df = pd.DataFrame(list(map(len, x)), columns=["x"])
    df["find_closest"], df["find_closest_err"] = get_times(partial(find_closest, character), x).T
    df["find_closest_min"], df["find_closest_min_err"] = get_times(partial(find_closest_min, character), x).T
    df["find_closest_kdtree"], df["find_closest_kdtree_err"] = get_times(partial(find_closest_kdtree, character), kdtrees).T
    df["find_closest_rtree"], df["find_closest_rtree_err"] = get_times(partial(find_closest_rtree, character), rtrees).T
    df["find_closest_kdtree_with_build"], df["find_closest_kdtree_with_build_err"] = get_times(partial(find_closest_kdtree_with_build, character), x).T

    for label in df.columns[1::2]:
        plt.errorbar(df["x"], df[label], yerr=df[label + "_err"], fmt='o-', label=label)
    plt.xlabel("Number of enemies")
    plt.ylabel("Time [s]")
    plt.xscale("log")
    plt.yscale("log")
    plt.legend()
    plt.show()


IRepository Pattern - Interface Segregation Principle I have an IRepository class that I use a lot.  But I noticed that for many of my repositories I do not implement most of the methods.  Also,  I usually don't have a TKey that I want to specify if I'm not using the GetById method but the current design forces me to specify it.  I was thinking of changing the interface or splitting up the interface into multiple smaller interfaces in order to make it cleaner and easier to use. Any ideas?

public interface IRepository<TEntity, in TKey>
{
    TEntity Insert(TEntity entity);

    TEntity Find(Expression<Func<TEntity, bool>> predicate);

    IQueryable<TEntity> FindAll(Expression<Func<TEntity, bool>> predicate);

    TEntity GetById(TKey key);

    TEntity Update(TEntity entity);

    IQueryable<TEntity> GetAll();

    void Delete(TEntity entity);
}

Here is an example implementation. Let me know if you need to see more than one method.  It is somewhat self explanatory.

/// <summary>
/// The invoices repository.
/// </summary>
public class InvoicesRepository : IRepository<Invoice, int>
{
    private readonly string connectionString;

    public InvoicesRepository(string connectionString)
    {
        this.connectionString = connectionString;
    }

    public Invoice Insert(Invoice entity)
    {
        using (SqlConnection connection = new SqlConnection(this.connectionString))
        using (SqlCommand command = new SqlCommand("INSERT INTO INVOICES VALUES (entity.SomeProp)", connection))
        {
            connection.Open();
            entity.Id = (int)command.ExecuteScalar();
            return entity;
        }
    }

    public Invoice Find(Expression<Func<Invoice, bool>> predicate)
    {
        throw new NotImplementedException();
    }

    public IQueryable<Invoice> FindAll(Expression<Func<Invoice, bool>> predicate)
    {
        throw new NotImplementedException();
    }

    public Invoice GetById(int key)
    {
        throw new NotImplementedException();
    }

    public Invoice Update(Invoice entity)
    {
        throw new NotImplementedException();
    }

    public IQueryable<Invoice> GetAll()
    {
        throw new NotImplementedException();
    }

    public void Delete(Invoice entity)
    {
        throw new NotImplementedException();
    }

    public Invoice GetById(int key)
    {
        throw new NotImplementedException();
    }
}

The ISP is indeed taking a beating with all these NotImplementedException. If we boil this interface a little (ok a lot), we get this:

public interface IRepository<TEntity>
{
    TEntity Find(Expression<Func<TEntity, bool>> predicate);
    IQueryable<TEntity> FindAll(Expression<Func<TEntity, bool>> predicate);
}

These two should always be implemented, assuming all entity types can at least be selected. I haven't included GetById because it makes things more complicated, and it's really Find all over again (especially when you have an entity with a composite PK).

That said, TEntity Find(...) feels a bit awkward: one could expect a "Find" method to return more than a single result, like FindAll does. I think it's the naming that's off: Find should be Single, or perhaps Select (consistent with the LINQ method that does the same thing), and then FindAll could become Find.

IQueryable<TEntity> shouldn't be permitted to exist beyond the data access layer - there's no reason FindAll can't materialize the query itself and return an IEnumerable<TEntity> instead, making sure that any further LINQ code on the client side doesn't get to the database.

I'd make Find return an IEnumerable<TEntity>.

Now to keep the ISP happy, we'd have an IInsertable<TEntity>, an IUpdatable<TEntity> and an IDeletable<TEntity>, respectively with Insert, Update and Delete methods; a repository that supports updates will implement IUpdatable<TEntity>, and a repository that doesn't support deletions won't implement IDeletable<TEntity>.

I don't think it's ideal, but it strips the repository interface down quite a bit - in fact it could even be renamed ISelectable<TEntity> at this point. An ApiController would use an IInsertable<Invoice> to insert an invoice in a method, and an IDeletable<Invoice> to delete that invoice in another method - and both methods would actually be dealing with an InvoiceRepository.


Improve a script to generally check status of several git repos I've tried to write a bash script that gives me a general overview of the status of git repos I have cloned/created. It's not meant to replace git status by any means, but give an overview of several repos.

Keep in mind I'm not the most knowledgeable about bash scripting AND git, so I imagine there is a better way to do this:

#!/bin/bash
# Help from: http://www.leancrew.com/all-this/2010/12/batch-comparison-of-git-repositories/

index=0
gitrepos=()

#TODO: There has got to be a better way to add all these folders to the array
#add repos folder
for d in ~/repos/*; do
    gitrepos[(index+=1)]="$d"
done

#add other important folders
gitrepos[(index+=1)]=~/.vim
gitrepos[(index+=1)]=~/dot_files
gitrepos[(index+=1)]=~/bin

for d in "${gitrepos[@]}"; do
    if [ -e $d ]; then
        cd $d
    else
        echo " Did not find repo: $d"
        continue
    fi

    reponame="`basename $d`"

    ok=true
    git fetch --quiet origin 2>/dev/null
    if [ ! -z "`git diff HEAD origin/HEAD 2> /dev/null`" ]; then
        echo " $reponame --> Out of sync with origin/HEAD"
        ok=false
    fi
    if [ ! -z "`git ls-files --other --exclude-standard 2> /dev/null`" ]; then
        echo " $reponame --> Untracked files present"
        ok=false
    fi
    if [ ! -z "`git diff --cached --shortstat 2> /dev/null`" ]; then
        echo " $reponame --> Changes to be committed"
        ok=false
    fi
    if [ ! -z "`git diff --shortstat 2> /dev/null`" ]; then
        echo " $reponame --> Changes to be staged/committed"
        ok=false
    fi
    if $ok; then
        echo " OK --> $reponame"
    fi
done

Yes there is a better way of filling an array. You can use globbing inside of array parens. Also I suggest being explicit about what you do in bash and use declare -a to declare arrays and later on declare for other variables

declare -a repos=(~/repos/* ~/.vim ~/dot_files ~/bin)

In case of adding stuff to an array, after it got created:

repos+=("more stuff" here)

Maybe you want to name you iterating variable more verbose, instead of d repodir or something.

Your first if contains a continue so you can emphasize on this one and just go on with no else. Also don't miss the double quotes in case of directory names with spaces in them. Also you should check for -d since you cd into it.

if [ -d "$d" ]; then
  echo "No repo at $d"
  continue
fi
cd "$d"

[ ! -z "`cmd`" ] is more verbose than it needs to be. You can just do [ "`cmd`" ].

Read the "Arrays" chapter in the bash manpage.

I attach my test file for this answer which also shows functions and some refactoring which allows to discard this ok variable.

#!/bin/bash
main() {
    local -a repos=(* /foo/bar)
    echo -e "repos: ${repos[@]}"
    for path in "${repos[@]}"; do
      check_repo "$path"
    done
}

check_repo() {
    local path="$1"
    local name="`basename "$path"`"
    local report=""
    if [ ! -d "$path" ]; then
      echo -e "$name\n not a directory: $path"
      return
    fi
    cd "$path"
    if [ "`echo foo 2> /dev/null`" ]; then
        report+="\n oh oh"
    fi
    cd - > /dev/null
    if [ -z "$report" ]; then
        report+="\n OK"
    fi
    echo -e "$name$report"
}

main


Validation on a form in JS I've got a form that needs validating before the data in the form is added to a service queue:

self.ee.on('validateForm', function(formData){

            // Validation
            var isValid = false;
            var validTitle = false;
            var validDescription = false;
            var validObjectives = false;
            var validOutcome = false;
            var validHours = false;

            if(formData.title==null){
                console.log("error on title")
                isValid = false;
            }
            else{
                validTitle = true;
                console.log("title is valid");
            }

            if(formData.description==null){
                console.log("error on description")
                isValid = false;
            }
            else{
                validDescription = true;
                console.log("description is valid");
            }

            if(formData.objectives==null){
                console.log("error on objectives")
                isValid = false;
            }
            else{
                validObjectives = true;
                console.log("objectives is valid");
            }

            if(formData.outcome==null){
                console.log("error on outcome")
                isValid = false;
            }
            else{
                validOutcome = true;
                console.log("outcome is valid");
            }

            if(formData.hours== null){
                console.log("Error on hours")
                isValid = false;

            }
            else{
                validHours = true;
                console.log("hours is valid");
            }

            if(validTitle && validDescription && validObjectives && validOutcome && validHours){
                isValid = true;
            }

            if (isValid) {
                self.ee.emit('formIsValid', formData);
            } else {
                self.ee.emit('formIsInvalid', formData);
            }
        });

It works fine, if all the conditions are met it passes onto my formisvalid function, which creates the message and adds and it to the service queue fine.

Is there a nicer/better way of doing the above?

Since you are only checking all of your fields against null, you could add the field names to an array and then loop through the array.

var isValid = true;
var fields  = ['title', 'description', 'objectives', 'outcome', 'hours'];

for ( var i = 0, n = fields.length; i < n; i++ ) {
    key = fields[i];
    if ( formData[key] == null ) {
        console.log("error on " + key);
        isValid = false;
    } else {
        console.log(key + " is valid");
    }
}


Improving quick sort algorithm (Python) I'm learning algorithm and this is my implementation of quick sort, please guide how can i improve it further and what are other more optimal ways to implement quick sort?

def quick_sort(arr):
    length = len(arr)
    if length <= 1:
        return arr

    else:
        pivot = arr.pop()

    items_greater = []
    items_lower = []

    for item in arr:
        if item > pivot:
            items_greater.append(item)
        else:
            items_lower.append(item)

    return quick_sort(items_lower)+[pivot]+quick_sort(items_greater)

Unnecessary else:

    if length <= 1:
        return arr

    else:
        pivot = arr.pop()

    ...

If the first path is taken, the method exits.  If the second path is taken, then execution continues with the remainder of the method.  So you could write the rest of the method inside the else: clause:

    if length <= 1:
        return arr

    else:
        pivot = arr.pop()

        ...  # Now part of the else statement

Or, for more "left leaning" code, remove the else: clause altogether:

    if length <= 1:
        return arr

    pivot = arr.pop()

    ...

Loop like a native

    items_greater = []
    items_lower = []

    for item in arr:
        if item > pivot:
            items_greater.append(item)
        else:
            items_lower.append(item)

Here, you are creating a list, and then calling .append() to repeatedly add items to that list in a loop.  Python has a builtin mechanism to do this, faster.  List comprehension:

    items_greater = [item for item in arr if item > pivot]
    items_lower = [item for item in arr if not(item > pivot)]

Why not(item > pivot), and not simply item <= pivot?  For the most part, the latter will work.  But there are gotchas and special cases.  If there are infinities (+Inf, -Inf) or a not-a-numbers (NaN) in the list, items might be omitted from both lists.  For example, if the list contains a NaN, both NaN > 10 and NaN <= 10 are False, which would result in that item not being added to either the items_greater or the items_lower lists!  Worse, if pivot happened to become NaN, then all arr items would be excluded!  Similarly, if there are two infinities in the list, both +Inf > +Inf and +Inf <= +Inf are False.

PEP-8

The PEP-8 coding standard has many suggestion about how to format code.  Binary operators, for example, should have a single space on either side of the binary operator.

    return quick_sort(items_lower)+[pivot]+quick_sort(items_greater)

should be:

    return quick_sort(items_lower) + [pivot] + quick_sort(items_greater)


Create logic structure with files I wrote code to creating structure with files on hard disc. I used here Composite pattern. Please make code review to this.

using System.Collections.Generic;
using System.Text;

namespace Common.FileSystem
{
    public abstract class FileSystemItem
    {
        public FileSystemItem(string rootPath, string name)
        {
            if (rootPath == null)
                throw new ArgumentNullException("rootPath");

            if (name == null)
                throw new ArgumentNullException("name");

            RootPath = rootPath;
            Name = name;
        }

        public string Name { get; private set; }

        public string RootPath { get; private set; }

        public string Path
        {
            get { return System.IO.Path.Combine(RootPath, Name); }
        }

        public abstract void Create();

        public abstract Directory AddDirectory(string name);

        public abstract void AddFile(string name, IFileBuilder fileBuilder);

        public abstract void Remove(FileSystemItem item);

        public abstract string Print();

        private string _rootPath;
    }

    public class Directory : FileSystemItem
    {
        public Directory(string rootPath, string name) : base(rootPath, name) { }

        public override void Create()
        {
            System.IO.Directory.CreateDirectory(Path);
            foreach (var item in _items)
                item.Create();
        }

        public override Directory AddDirectory(string name)
        {
            var directory = new Directory(Path, name);
            _items.Add(directory);

            return directory;
        }

        public override void AddFile(string name, IFileBuilder fileBuilder)
        {
            var file = new File(Path, name, fileBuilder);
            _items.Add(file);
        }

        public override void Remove(FileSystemItem item)
        {
            _items.Remove(item);
        }

        public override string Print()
        {
            var stringBuilder = new StringBuilder();
            stringBuilder.AppendLine(Path);
            foreach (var item in _items)
                stringBuilder.Append(item.Print());

            return stringBuilder.ToString();
        }

        private List<FileSystemItem> _items = new List<FileSystemItem>();
    }

    public class File : FileSystemItem
    {
        public File(string rootPath, string name, IFileBuilder fileBuilder) : base(rootPath, name)
        {
            if (fileBuilder == null)
                throw new ArgumentNullException("fileBuilder");

            _fileBuilder = fileBuilder;
        }

        public override void Create()
        {
            var fileContent = _fileBuilder.Build();

            System.IO.File.WriteAllText(Path, fileContent);
        }

        public override Directory AddDirectory(string name)
        {
            throw new InvalidOperationException("Operation is unavailable for this object");
        }

        public override void AddFile(string name, IFileBuilder fileBuilder)
        {
            throw new InvalidOperationException("Operation is unavailable for this object");
        }

        public override void Remove(FileSystemItem item)
        {
            throw new InvalidOperationException("Operation is unavailable for this object");
        }

        public override string Print()
        {
            return Path + Environment.NewLine;
        }

        private IFileBuilder _fileBuilder;
    }

    public interface IFileBuilder
    {
        string Build();
    }
}

There is no way to get the items contained in a Directory once they have been added.

Your base class defines:

public abstract Directory AddDirectory(string name);
public abstract void AddFile(string name, IFileBuilder fileBuilder);
public abstract void Remove(FileSystemItem item);

These methods only make sense for a Directory. The fact that one of your two subclasses has to throw an InvalidOperationException for each of these abstract methods is an indication that they should not be part of the base class. Even worse, someone who has a reference to a FileSystemItem has no way of knowing if calling one of these methods is safe without explicitly checking the actual type of the instance.

Note: Iterating over the contents of a directory (mentioned above) is another operation that only makes sense for a Directory. It would be best to let FileSystemItem define the core properties, but not try to force the aspects that don't work for all of the subclasses.

This will likely mean you will need to keep a directories contents in two separate collections. You can still expose a function that allows iterating over all of the items, but someone using that call would not have access the Directory specific methods.

FileSystemItem defines _rootPath as a member variable, but it is not used.

I prefer to have member variables at the top of the class. This way, someone reading the class for the first time, will know what variables are available to methods before seeing a method trying to use one.

I prefer to always use curly brackets for if statements and loops. It makes things less error prone when you have to edit the code later.


Simple javascript code to create an image caption I'm trying to become better at javascript and have written a simple script that just creates some html from getting some details from some elements (e.g. the image alt text) and inserts it onto the page to create a caption. Hover over the top right of the image to see this caption.

I was able to successfully achieve what I wanted through my code. However, my javascript feels a bit 'wet', and I'd like to be able to improve this for brevity and clarity.

Can anyone recommend any improvements to my javascript, and make it either more concise or clear?

I know that jQuery can achieve this in fewer lines of code, but I'm less of a fan of jQuery and believe that ES6/ES7 etc can often write more concise code.

Thanks for any help here. Find a working demo below:

Codepen: https://codepen.io/anon/pen/PrwzBj

/* --- Wide Image Caption --- */
// Define Elements:
const wideImg = document.querySelector('.full-width-image-atf img'),
      captionText = wideImg.getAttribute('alt'),
      captionInnerHTML = '' + captionText + '',
      caption = document.createElement('div');
// Amend caption:
      caption.setAttribute('class', 'image-caption-wrap');
      caption.style.cssText = 'background: none;';
      caption.innerHTML = captionInnerHTML;
// Insert on page:
      wideImg.parentNode.appendChild(caption);
/* --- End of Wide Image Caption --- */
img {width: 100%}
.full-width-image-atf .content-main-image {
  position: relative;
  color: #fff;
}

.full-width-image-atf .image-caption-wrap {
  position: absolute;
  top: 0;
  right: 0;
  display: flex;
  flex-flow: row-reverse;
  align-items: center;
  padding: 15px 20px;
}
.full-width-image-atf .image-caption-wrap:before {
  content: '\f05a';
  font-family: 'Font Awesome 5 Pro';
  font-size: 2em;
}
.full-width-image-atf .image-caption {
  opacity: 0;
  padding-right: 10px;
}
.full-width-image-atf .image-caption-wrap:hover .image-caption {
  opacity: 1;
}
.full-width-image-atf .image-caption-wrap:hover {
  background: black !important;
  cursor: pointer;
}
.full-width-image-atf .content-main-image, .full-width-image-atf .content-main-image * { transition: 0.4s ease all }

Not a comment on functionality, but here:

const wideImg = document.querySelector('.full-width-image-atf img'),
      captionText = wideImg.getAttribute('alt'),
      captionInnerHTML = '' + captionText + '',
      caption = document.createElement('div');

I don't think chaining declarations using , like you are is a good idea for a couple reasons:

It isn't immediately clear that all the variables involved are being declared as const. Yes, you can tell if you check the indentation, or notice the commas at the end, but it isn't as clear as explicit consts all the way down.
It makes refactoring more of a pain. Say in the future you want to break that up and maybe stick a log call in there or some other non-declaration. Now you need to manually replace the commas with semi-colons, and remember to add const to all the now separate declarations.

I'd suggest just writing it out fully from the start, and maybe space out the last one since it has a separate purpose:

const wideImg = document.querySelector('.full-width-image-atf img');
const captionText = wideImg.getAttribute('alt');
const captionInnerHTML = '' + captionText + '';

const caption = document.createElement('div');


Implementing `components(separatedBy:)` method in Swift On looking at the components(separatedBy separator: String) -> [String] method from the Swift Standard Library, I tried to come up with an implementation just for practice. Your comments are welcome to improve the same. Thanks.

Input:

func main() {
    let sampleString = "Do not be sorry. Be better."
    print(sampleString.components(separatedBy: "."))
}

Output:

["Do not be sorry", " Be better", ""]

Implementation:

extension StringProtocol {

    func components<T>(separatedBy separatorString: T) -> [String] where T: StringProtocol {

        var currentIndex = 0; var stringBuffer = ""; var separatedStrings:[String] = []

        forEach { (character) in

            if String(character) == separatorString {
                separatedStrings.append(stringBuffer); stringBuffer = ""
            } else {
                stringBuffer += .init(character)
            }

            if currentIndex == lastIndex { separatedStrings.append(stringBuffer) }
            currentIndex += 1
        }
        return separatedStrings
    }
}

extension Collection {
    var lastIndex:Int {
        get {
            return self.count - 1
        }
    }
}

Coding style

This is of course a matter of personal taste, but would split multiple statements like

var currentIndex = 0; var stringBuffer = ""; var separatedStrings:[String] = []

into separate lines

var currentIndex = 0
var stringBuffer = ""
var separatedStrings:[String] = []

I would also start new lines for nested code blocks, i.e.

if currentIndex == lastIndex { separatedStrings.append(stringBuffer) }

becomes

if currentIndex == lastIndex {
    separatedStrings.append(stringBuffer)
}

Correctness

Your function takes a separator of type string (protocol) as an argument, but actually works only for separators consisting of a single character. Example:

let sampleString = "Do not be sorry. Be better."
print(sampleString.components(separatedBy: ". "))
// ["Do not be sorry. Be better."]

The reason is that here

if String(character) == separatorString

a single character of the source string is compared with the separator.

Your method also behaves differently from the standard library version when called with an empty string,

"".components(separatedBy: ".")

returns an empty array [] instead of a single-element array [""]. The reason is that the check

if currentIndex == lastIndex { separatedStrings.append(stringBuffer) }

is never done for an empty input string. (Checking for the last iteration inside a loop always makes me suspicious.)

Simplifications

Instead of converting a character to a string for appending

stringBuffer += .init(character)

you can append it directly:

stringBuffer.append(character)

Keeping track of the current character position can be done with
enumerated()

for (currentIndex, character) in self.enumerated() {
    // ...
}

instead of incrementing var currentIndex.

Efficiency

The main bottleneck is the

var lastIndex:Int

extension method. For Strings (and other collections which are not random accessible) determining self.count is a O(N) operation (N = number of characters). It requires traversing the entire string.

This method is called for each character in the source string, so that this contributes O(N^2) to the execution time.

It would also be more efficient to locate the next occurrence of the separator and append an entire substring to the result array, instead of appending single characters repeatedly.

Alternative implementation

Here is an alternative implementation, considering the above suggestions:

func components<T>(separatedBy separatorString: T) -> [String]
    where T: StringProtocol, Index == String.Index {
        var separatedStrings: [String] = []
        var pos = startIndex
        while let range = self[pos...].range(of: separatorString) {
            separatedStrings.append(String(self[pos..<range.lowerBound]))
            pos = range.upperBound
        }
        separatedStrings.append(String(self[pos...]))
        return separatedStrings
}


Inheriting needed when only difference is performed in the constructor? I have a class that produces a key based on the two objects passed. The key for the items passed is simply the string of the ID (a GUID) for each object. There is also a subclass that makes sure any to items are only represented once (sets).

The classes looked like the first code block below. But then I ran into a performance problem where creating the key every time was accessed was just to slow. So I changed it such that I create the key in the constructor and store it in a member variable. But now it seems that the inheritance is overkill.

Maybe the inheritance was always overkill but I didn't see it because I overrode an actual behavior so it seemed more OOPy? Or did it become inheritance abuse after the re-factoring? Or is it still fine but I am seeing it wrong?

Note that the ID must be unique and is often, but not always, a GUID.

Public Interface IIdentifiable
    ReadOnly Property ID() As String
End Interface

Public Class Keyed
    Public Sub New(item1 As IIdentifiable, item2 As IIdentifiable)
        PrimaryItem = item1
        SecondaryItem = item2
    End Sub

    Public Property PrimaryItem As IIdentifiable

    Public Property SecondaryItem As IIdentifiable

    Public Overridable Function Key() As String
        Return PrimaryItem.ID & SecondaryItem.ID
    End Function

End Class

Public Class KeyedForSet
    Inherits Keyed

    Public Sub New(item1 As IIdentifiable, item2 As IIdentifiable)
        MyBase.New(item1, item2)
    End Sub

    Public Overrides Function Key() As String
        If PrimaryItem.ID.CompareTo(SecondaryItem.ID) > 0 Then
            Return PrimaryItem.ID & SecondaryItem.ID
        Else
            Return SecondaryItem.ID & PrimaryItem.ID
        End If
    End Function

End Class

Second implementation for performance reason.

Public Class Keyed
    Public Sub New(item1 As IIdentifiable, item2 As IIdentifiable)
        mKey = item1.ID & item2.ID
        PrimaryItem = item1
        SecondaryItem = item2
    End Sub

    Public Property PrimaryItem As IIdentifiable
    Public Property SecondaryItem As IIdentifiable

    Protected mKey as string
    Public Overridable Function Key() As String
        Return mKey
    End Function
End Class

Public Class KeyedForSet
    Inherits Keyed

    Public Sub New(item1 As IIdentifiable, item2 As IIdentifiable)
        MyBase.New(item1, item2)
        If PrimaryItem.ID.CompareTo(SecondaryItem.ID) > 0 Then
            mKey = PrimaryItem.ID & SecondaryItem.ID
        Else
            mKey = SecondaryItem.ID & PrimaryItem.ID
        End If
    End Sub

End Class

Based upon your use case, the code below is what I would recommend.  I'm sorry that it is in C#, on this computer I don't have my vb.net loaded so it was easier to express in c#.

There are 3 parts to this.  The class on the bottom should address your needs (KeySet).  the second class helps to make it more easily usable in your code (KeySetHelpers).  The first class (MyExample) is just showing a few ways that you can utilize it.  I think the direct approach is most natural.

Regarding performance and inheritance.  If you follow the process below, I don't think you need another layer of inheritance.  Performance wise, I think that the collection you choose to utilize will matter more than the KeySet.  Since in the end you are just utilizing strings anyway.

One area I would recommend that you look into is the reason why you are flipping the primary/secondary keys based upon which is greater.  Since you are already injecting which is the primary and which is secondary, they should most likely always have the same order when combined.  Otherwise it could complicate the meaning of the key.  Plus if you keep the order primary-secondary and normalize the key by stripping out anything non-alphanumeric and then adding in a parsing token like "-" between the too,...you wouldn't need to store the Key(composite), primary key, and secondary key.  I hope this helps.

public class MyExample
{
    public void Test()
    {
        // Junk Prep
        var primary = System.Guid.NewGuid();
        var secondary = System.Guid.NewGuid();
        var junkData = "blah blah";

        // Real usage
        var keysList = new List<KeySet>();

        keysList.Add(new KeySet(primary, secondary)); // as guid
        // or
        keysList.Add(new KeySet(primary.ToString(), secondary.ToString())); // as guid strings
        // or
        keysList.Add(primary, secondary);// direct as guid

        var keysDictionary = new Dictionary<KeySet, object>();

        keysDictionary.Add(new KeySet(primary, secondary), junkData); // as guid
        // or
        keysDictionary.Add(new KeySet(primary.ToString(), secondary.ToString()), junkData); // as guid strings
        // or
        keysDictionary.Add(primary, secondary, junkData); // direct as guid

        // etc.
    }
}

public static class KeySetHelpers
{
    #region Lists

    public static void Add(this List<KeySet> keyList, string primary, string secondary)
    {
        keyList.Add(new KeySet(primary, secondary));
    }

    public static void Add(this List<KeySet> keyList, Guid primary, Guid secondary)
    {
        keyList.Add(new KeySet(primary, secondary));
    }

    #endregion

    #region Dictionary

    public static void Add(this Dictionary<KeySet, object> keyDictionary, string primary, string secondary, object value)
    {
        keyDictionary.Add(new KeySet(primary, secondary), value);
    }

    public static void Add(this Dictionary<KeySet, object> keyDictionary, Guid primary, Guid secondary, object value)
    {
        keyDictionary.Add(new KeySet(primary, secondary), value);
    }

    #endregion
}

public class KeySet : IComparable<KeySet>
{
    public KeySet(Guid primary, Guid secondary) { EstablishKey(primary.ToString(), secondary.ToString()); }
    public KeySet(string primary, string secondary) { EstablishKey(primary, secondary); }

    #region Composite Key

    private void EstablishKey(string primary, string secondary)
    {
        if (string.Compare(primary, secondary) > 0)
        {
            Key = primary + secondary;
        }
        else
        {
            Key = secondary + primary;
        }

        Primary = primary;
        Secondary = secondary;
    }

    public string Primary { get; private set; }
    public string Secondary { get; private set; }

    public string Key { get; private set; }

    #endregion

    #region Functional Overrides

    public override bool Equals(object obj)
    {
        return (obj is KeySet) ? Equals((KeySet)obj) : false;
    }

    public bool Equals(KeySet obj)
    {
        return CompareTo(obj) == 0;
    }

    public override int GetHashCode()
    {
        return Key.GetHashCode();
    }

    public override string ToString()
    {
        return Key;
    }

    #endregion

    public int CompareTo(KeySet other)
    {
        return string.Compare(Key, other.Key);
    }

    public static bool operator ==(KeySet obj1, KeySet obj2)
    {
        return obj1.Equals(obj2);
    }

    public static bool operator !=(KeySet obj1, KeySet obj2)
    {
        return !obj1.Equals(obj2);
    }
}


Delete file from server (php function) Is there a better way to write this code.
Suppose I have 10 files to delete and I have to repeat code (public function destroy() ) 10 times.

First remove the database entry.
List item, then remove the file.

OOP - "don't repeat yourself", it works but I repeat the code.

public function image_path() {
        return $this->upload_dir .DS.  $this->filename;
    }

    public function image_path2() {
        return $this->upload_dir .DS.  $this->filename2;
    }

    public function destroy() {
        if($this->delete()) {
            $target_path = PUBLIC_PATH.DS.'public'.DS.$this->image_path();
            return unlink($target_path) ? true : false;
        } else {
            return false;
        }
    }

    public function destroy2() {
        if($this->delete()) {
            $target_path2 = PUBLIC_PATH.DS.'public'.DS.$this->image_path2();
            return unlink($target_path2) ? true : false;
        } else {
            return false;
        }
    }

delete_photo.php

  // Delete photo
  $result = $photo->destroy();
  $result = $photo->destroy2();

photo class

public function delete() {
        global $database;

        $sql = "DELETE FROM " . self::$table_name . " ";
        $sql .= "WHERE id='" . $database->escape_value($this->id) . "' ";
        $sql .= "LIMIT 1";
        $result = $database->query($sql);
        return $result;
    }

protected static $db_fields=array('id', 'filename', 'filename2', 'type', 'type2','size', 'size2', 'caption');
    public $id;
    public $filename;
    public $filename2;
    public $type;
    public $type2;
    public $size;
    public $size2;
    public $caption;

Yes, clearly you should write D.R.Y. code by not writing multiple methods that perform the same task.

You only need one image_path() and one destroy() method, it is the data delivery to those methods that needs to change and then of course the methods need to be altered to iterate the data.

If you expect to be handing multiple files the same way, I recommend a class variable (array of filenames) which can be filled as necessary, then iterated by your methods.

This mean doing away with:

public $filename;
public $filename2;

and instead use:

public $filenames = [];

A final note about reducing syntax:

if($this->delete()) {
    $target_path = PUBLIC_PATH.DS.'public'.DS.$this->image_path();
    return unlink($target_path) ? true : false;
} else {
    return false;
}

Can be rewritten as:

if (!$this->delete()) {
    return false;
}
return unlink(PUBLIC_PATH . DS . 'public' . DS . $this->image_path());

or even shorter if you prefer inline conditions:

return !$this->delete() ? false : unlink(PUBLIC_PATH . DS . 'public' . DS . $this->image_path());

That said, there is nothing wrong with if blocks, but your ? true : false is unnecessary, I try to avoid declaring  single-use variables, and I prefer to write failure / false / early returns before positive outcomes.


Split calculation string into parts Our professor has given us the following task (its part of a bigger document full of tasks):

Write a function in C that handles strings like "233+343" and parse them into the variables "iNum1", "cOp" and "iNum2". (I tried to translate it into english).

We mostly program in Java and my C knowledge consists of mostly provisional half knowledge.

But is my program beautiful enough to show it to a professor?

#include <stdio.h>
#include <string.h>

#define ASCII_POS_OF_0 0x30
#define ASCII_POS_OF_9 0x39

void parse_calc_string(char string[])
{

    // get first number
    int iNum1 = 0;
    int index = 0;
    char current_char = string[index];
    while (current_char >= ASCII_POS_OF_0 && current_char <= ASCII_POS_OF_9)
    {
        iNum1 *= 10;
        iNum1 += (int)current_char - ASCII_POS_OF_0;
        current_char = string[++index];
    }

    // get operation symbol
    char cOp = current_char;
    current_char = string[++index];

    // get second number
    int iNum2 = 0;
    while (current_char >= ASCII_POS_OF_0 && current_char <= ASCII_POS_OF_9)
    {
        iNum2 *= 10;
        iNum2 += (int)current_char - ASCII_POS_OF_0;
        current_char = string[++index];
    }

    printf("iNum1: %i\n", iNum1);
    printf("cOp  : %c\n", cOp);
    printf("iNum2: %i\n", iNum2);
    printf("\n");
}

int main()
{
    parse_calc_string("219+43");
    parse_calc_string("195-143");
    parse_calc_string("15*13");
    parse_calc_string("212/14");
}

You can get the value for ASCII_POS_OF_0 easily by using '0'.

Your code doesn't deal with malformed strings well.

If the string is only "123" it will attempt to read beyond the contents of the string. You can fix this by checking for '\0' at the correct places.

It also doesn't handle any type of whitespace in the string at all.


Efficiently generate large file to create SQL table and insert sample user data Here's a function I use to generate a 2.5 gig SQL dump file for testing. It works but it takes a long time. How can I make it more efficient?

const crypto = require("crypto");
const fs = require('fs');
const os = require('os');
const path = require('path');

(async function main(){
    var dumpfile = await generate_large_dump();
    console.log("\nDump created: ", dumpfile);
})();

function generate_large_dump(){
    return new Promise(async (resolve, reject)=>{
        var total_bytes = 0;
        const target_bytes = 2.5 * 1e+9;
        const target_file = path.join(os.tmpdir(), 'large_dump.sql');

        const writerStream = fs.createWriteStream(target_file, {flags: 'w'});
        writerStream.on('error', console.error);

        const age = ()=>Math.floor(Math.random() * (95 - 18 + 1)) + 18;
        const name = ()=>crypto.randomBytes(16).toString("hex");

        const write = str => new Promise(resolve=>{
            total_bytes += Buffer.byteLength(str, 'utf8');
            writerStream.write(str, resolve);
            var pct = Math.min(100, Math.floor(total_bytes / target_bytes * 10000)/100);
            process.stdout.clearLine();
            process.stdout.cursorTo(0);
            process.stdout.write(pct+"% complete");
        });

        var create_sql = "CREATE TABLE `sample_table` (`id` int(11) NOT NULL AUTO_INCREMENT, `name` varchar(250) NOT NULL, `age` int(11) NOT NULL, `date` datetime NOT NULL DEFAULT CURRENT_TIMESTAMP, PRIMARY KEY (`id`));\n";
        await write(create_sql);

        while(total_bytes < target_bytes) await write("INSERT INTO `sampe_table` (`name`, `age`) VALUES ('"+name()+"', '"+age()+"');\n");

        writerStream.end();
        process.stdout.write("\n");
        resolve(target_file);
    });
}

The main culprit here is your progress indicator.

You're continuously refreshing a line in stdout every time write is called, and write is called a very large number of times. If you reduce the frequency of console writes, you'll make your script a whole lot faster. One option is to write to the console only every 0.5 seconds or so:

// Performance checker:
setTimeout(() => {
    console.log('Process after 10 seconds: ', Math.floor(total_bytes / target_bytes * 10000)/100);
}, 10000);
// Actual code:
let timeoutId;
const write = str => new Promise(resolve=>{
    total_bytes += Buffer.byteLength(str, 'utf8');
    writerStream.write(str, resolve);
    if (!timeoutId) {
        timeoutId = setTimeout(() => {
            process.stdout.clearLine();
            process.stdout.cursorTo(0);
            const pct = Math.min(100, Math.floor(total_bytes / target_bytes * 10000)/100);
            process.stdout.write(pct+"% complete");
            timeoutId = null;
        }, 500);
    }
});

On my machine, this results in a speed improvement from around 0.25% in 10 seconds, to around 2.03% in 10 seconds - an improvement of a whole order of magnitude.

Another thing: if you're going to use ES2015+ syntax - which you are, and should - then always declare variables with const when possible. Never use var, it has too many gotchas to be worth using (such as function scope instead of block scope, the ability to accidentally re-declare it, automatically putting properties on the global object when on the top level in a browser, etc).


Using Templates to add virtual tables to non-virtual classes So I have these classes that are meant to be low level and fast. I don't want them to generally be virtual - rather I want the functions to be inline.

In this case they are mutex implementations:

class MutexOne
{
public:
   void lock        () { /* implementation inline */ }
   bool tryLock     () { /* implementation inline */ }
   void unlock      () { /* implementation inline */ }
   void reset       () { /* implementation inline */ }

   static const char* implementationName() { return "MutexOne"; }
};

class MutexTwo
{
public:
   void lock        () { /* implementation inline */ }
   bool tryLock     () { /* implementation inline */ }
   void unlock      () { /* implementation inline */ }
   void reset       () { /* implementation inline */ }

   static const char* implementationName() { return "MutexTwo"; }
};

... etc ...

All the mmutexes have the same api. Within a testing framework I kind of wish they were virtual so that I can do something like:

testMutexImplementation( MutexVirtual& mutex )
{
    mutex.lock();  // etc... do a lot of complex validating
}

So within the testing framework I wish I had a nice clean virtual base class interface. But in production I don't want to pay for a virtual function on every mutex operation many of which are like 2-3 opcode in the general case.

So I thought of this way to use templates to add a virtual table after the fact, only when I need it.

So I start by creating the desired base class:

class MutexVirtual
{
public:

   virtual void lock        () = 0;
   virtual bool tryLock     () = 0;
   virtual void unlock      () = 0;
   virtual void reset       () = 0;

   virtual const char* implementationName() = 0;
};

And then use template magic to fill in the virtual table:

template < typename T > class MutexPostVirtual : public MutexVirtual, public T
{
public:
   void lock           () { T::lock(); }
   bool tryLock        () { return T::tryLock(); }
   void unlock         () { T::unlock(); }
   void reset          () { T::reset(); }

   const char* implementationName() { return T::implementationName(); }
};

I'm pretty happy and it works great:

MutexPostVirtual<MutexOne> mutex1;
MutexPostVirtual<MutexTwo> mutex2;

testMutexImplementation( mutex1 );
testMutexImplementation( mutex2 );

I can't help thinking that maybe there is a cleaner simpler way? How can this code be better?

I don't know what framework you are using for your tests but gtest has type parametrised tests that you can use to test all the implementations with the same tests.

By using this feature you wouldn't even need the code to begin with, which in my book is the best code: the code you never wrote :)


Ruby code to parse and analyze social media stats in json format What do you think about this code I wrote.
It reads several json files ( I attached them as pastebins)
and the code outputs summary information calculated from the input.

# 1) calculate the
# engagement rate from the first JSON file (the user's Instagram media)
# - Calculate the average number of comments - Calculate the average
# number of likes - Calculate the engagement rate using (Average Likes +
# Average Comments)/# of Followers which is 435
#
# 2) Combine the 2 sets of JSON data (Instagram media objects with their
# Instagram insights), then output a single JSON file where the insights
# are paired with their media objects in a clean format.
#
# Question 1.json: https://pastebin.com/V3Ue87H4
# Question2A.json: https://pastebin.com/YifXzimZ
# Question2B.json: https://pastebin.com/6X0JYXtD

require 'json'
input = JSON.parse( File.read('Question 1.json') )["data"]
avg_likes    = input.map { |x| x["like_count"] }.    inject{ |sum, el| sum + el }.to_f / input.size
avg_comments = input.map { |x| x["comments_count"] }.inject{ |sum, el| sum + el }.to_f / input.size
puts "Answer for Question 1: #{avg_likes + avg_comments / 435}\n\n"

input_a = JSON.parse( File.read('Question2A.json') )["data"]
input_b = JSON.parse( File.read('Question2B.json') ).map { |x| x["data"] }.flatten
output = input_a.map do |post|
  {
    :permalink => post["permalink"],
    :impressions => input_b.find { |x| x["id"] == "#{post['id']}/insights/impressions/lifetime" }["values"][0]["value"],
    :engagement  => input_b.find { |x| x["id"] == "#{post['id']}/insights/engagement/lifetime"  }["values"][0]["value"],
    :reach       => input_b.find { |x| x["id"] == "#{post['id']}/insights/reach/lifetime"       }["values"][0]["value"],
    :saved       => input_b.find { |x| x["id"] == "#{post['id']}/insights/saved/lifetime"       }["values"][0]["value"]
  }
end
puts "Answer for Question 2: " + JSON.pretty_generate(output)

# EXAMPLE OUTPUT - generated by running ruby coding_test.rb
#
# Answer for Question 1: 22.704827586206896
#
# Answer for Question 2: [
#   {
#     "permalink": "https://www.instagram.com/p/B40LKjkAlDV/",
#     "impressions": 236,
#     "engagement": 33,
#     "reach": 187,
#     "saved": 2
#   },
#   {
#     "permalink": "https://www.instagram.com/p/B4z4Vq5Ag5g/",
#     "impressions": 212,
#     "engagement": 22,
#     "reach": 170,
#     "saved": 0
#   },
#   {
#     "permalink": "https://www.instagram.com/p/B4xnmtygmi-/",
#     "impressions": 297,
#     "engagement": 28,
#     "reach": 218,
#     "saved": 1
#   },
#   {
#     "permalink": "https://www.instagram.com/p/B4uyfrLgjVi/",
#     "impressions": 248,
#     "engagement": 21,
#     "reach": 194,
#     "saved": 0
#   },
#   {
#     "permalink": "https://www.instagram.com/p/B4nic12AFps/",
#     "impressions": 292,
#     "engagement": 15,
#     "reach": 219,
#     "saved": 0
#   }
# ]

Good work.

Here are some points where you can improve:

You can use input.sum { |x| x["like_count"] }.to_f instead of input.map { |x| x["like_count"] }.inject{ |sum, el| sum + el }
It is a good idea to do calculations with BigDecimal instead of Float.
435 is a magic number, you can put it into a constant with a good name.
The spaces before the parentheses aren't usual in the Ruby community style guide.
Check the dig method available for Hash instances.
You can use the new syntax for Hash instances when the keys are symbols: { hello: "world" }
Use double-quotes or single-quotes, you're mixing both.