Find missing web-pages You are writing your web-page and relentlessly adding links, as you think that links are the best thing a web-page can offer.

The fact is that you write links even to pages that do not exist just yet, because you will write them later.

But later becomes a lot later and now you have your website in your directory with a lot of links pointing to nothing.

You decide to systematically write a page for each link, as to eliminate all the pending links, and using this script to list all such pending links:

An example output looks like:

['addition.html', 'definition.html', 'division.html', 'infinity.html', 'multiplication.html', 'primitive_concept.html', 'recursion.html', 'set.html', 'subtraction.html']

The code is very straightforward, but there is always potential for improvement:

"""
Given a folder of html pages, gives a list of all the pages that are linked to,
but do not exist.
"""
import doctest
import itertools
import os
import re

PATH = os.path.dirname(os.path.realpath(__file__))
flatten = itertools.chain.from_iterable

def destinations(html):
    """
    >>> destinations('''
The natural numbers are an infinitesetdefined recursivelyas follows:''')
    ['infinity.html', 'set.html', 'recursion.html']
    """
    return re.findall('[a-z_]+\.html', html)

def read(f):
    with open(f) as c:
        return c.read()

def missing_pages(directory=PATH):
    """
    Lists all the pending links of the html pages if the `directory`.
    """
    all_pages = sorted(set((flatten(destinations(read(i)) for i in os.listdir(directory)))))
    return list(i for i in all_pages if i not in os.listdir(directory))

if __name__ == "__main__":
    doctest.testmod()
    print(missing_pages())

If the script works fine for you the points about robustness below won't
matter too much.

The way PATH is defined is a bit unusual since in most cases I want
to be able to invoke the script from any directory, but this default
forces it to the directory of the script.  I'd almost say that the
normal default argument of os.listdir, namely "." is way better.
os.listdir is called too often, the result can just be reused.
Don't use list when you can have literal list syntax instead.
The output is fine I guess, except for passing it to other scripts
you'd usually want a more "standard" format, i.e. a single file per
line without quotes.

For the record missing_pages should look more like this (filtering
filenames and directories is left as an exercise though):

def missing_pages(directory="."):
    """
    Lists all the pending links of the html pages of the `directory`.
    """
    files = os.listdir(directory)
    all_pages = sorted(set(flatten(destinations(read(i)) for i in files)))
    return [i for i in all_pages if i not in files]

Otherwise looks good, test does what it says, docstrings where needed
and the script even has a top-level description.

Robustness

I ran the script on a random directory just to see what happens.  With
Python 3 I get a UnicodeDecodeError; with Python 2 it errors out on a
directory - it wouldn't hurt to skip them.  It probably should only read
files with the correct ending as well.

Next is that the regular expression to match is quite limited; it also
will not really deal with the situation where a file is mentioned, but
not linked to.  The docstring to destinations implies that the
function does more than what it can - so you could just leave out all
the HTML fluff and just mention the filenames separated by spaces; or,
actually document the corner cases, meaning I'd like to have a docstring
saying something like "Finds mentions of HTML files in a string.  Only lowercase filenames with underscores are returned." or something similar.


Compression unit test data "easy case" In another post I created a piece of data, which make compressor annoyed.

Create 'worst case test' data for compression test

Now I am trying to make it happy.  I generate another data set, with a lot of duplicated binarys.  Of couse a data set composed of only one value is the best, but that is too easy.

This time I use a random number generator.  Inside a loop, each time either add a new byte or duplicate some bytes until reach a predefined length.

#include <iostream>
#include <fstream>
#include <random>
#include <vector>

typedef std::vector<unsigned char> Data;

int main(int argc, char* argv[])
{
    std::random_device rd;
    std::mt19937 mt(rd());
    std::uniform_real_distribution<double> rand(0.0, std::nextafter(1.0, DBL_MAX));

    const unsigned int length = 0x10000; // file minimal size

    Data data;
    data.reserve(length * 2);
    while (data.size() < length)
    {
        if (data.size() <= 1 || rand(mt) < 0.5)
        {
            // add a new byte
            data.push_back((unsigned char)(rand(mt) * 256));
        }
        else
        {
            // duplicate some bytes
            unsigned int count = (unsigned int)(rand(mt) * (data.size() / 2) + 1);
            data.insert(data.end(), data.end() - count, data.end());
        }
    }

    const char* filename = "c:\\_Test\\easy.dat";
    std::ofstream ofs(filename, std::ios::binary);
    if (ofs.bad() || !ofs.is_open())
    {
        std::cerr << "fail to open file\n";
        return -1;
    }
    ofs.write((char*)data.data(), data.size());

    std::cout << "job done\n";
    return 0;
}

The 7-zip is extremely happy with it.  With 7z format, 90kb data compressed to 900 bytes, and with zip format, it compressed to less than 2kb.

Again any suggestions and help are appreciated.

A few thoughts:

1. Don't use C-style casts

Here

 data.push_back((unsigned char)(rand(mt) * 256));
             // ^^^^^^^^^^^^^^^

and here

 unsigned int count = (unsigned int)(rand(mt) * (data.size() / 2) + 1);
                   // ^^^^^^^^^^^^^^

you use (unchecked) C-style cast operations. I C++ all casts should be either checked, or explicitly a reinterpret_cast (which is merely the same, but clearly shows intention).

In your cases a static_cast<unsigned char> and a static_cast<unsigned int> seems to be more appropriate for me.

2. If you're dealing with plain integer numbers prefer using std::uniform_int_distribution

You are using std::uniform_real_distribution<double>

std::uniform_real_distribution<double> rand(0.0, std::nextafter(1.0, DBL_MAX));

to finally generate integer types (done through implicit conversion).

It would seem more appropriate to use

 std::uniform_int_distribution<unsigned int> rand(0, std::nextafter(1, UINT_MAX) /* intentional ?? */ );

or even better simply

std::uniform_int_distribution<unsigned int> rand();

and

data.push_back(static_cast<unsigned char>
    (rand(mt) % std::numeric_limits<unsigned char>::max());
// ...
unsigned int count = rand(mt) * (data.size() / 2) + 1;

Floating point conversions and calculations may cause unnecessary overhead in the emitted assembly instructions.

3. Prefer std::numeric_limits over the C-style macro definitions

In C++ you should prefer using std::numeric_limits<double>::max() over the DBL_MAX C-style macro.

At least with GCC I always get compiler errors using these constants (even though <cstdint> was included).

4. Modularize your code to be resuable

As you tagged this with unit-testing I would consider to create a little helper library API that can be used with the unit tests, without calling a separate program.

Instead of the int main(int argc, char* argv[]) entry point, refactor your code (including the stuff from Create 'worst case test' data for compression test) to a helper function API, that can be used easily with the various unit tests:

 namespace Test7ZipData {
     int create_worst_compressible_data(const char* filename);
     int create_easily_compressible_data(const char* filename);
     // int create_average_compressible_data(const char* filename); ????
 }

Compile these into a library, that will be linked with you unit tests.

5. Prefer using raw string literals instead of escaping special characters

Instead of

const char* filename = "c:\\_Test\\easy.dat";

you can provide a raw string literal

const char* filename = R"(c:\_Test\easy.dat)";

That enhances readability of the literal text.


All subsets of a String in java using recursion I wrote this code for printing all sets of a String. It also prints the empty subset. Is this the right approach

public class AllSubSet {

    public static void allSet(String s, String t) {
        System.out.print(t + " ");
        if (s.length() > 0) {
            for (int i = 0; i < s.length(); i++) {
                allSet(s.substring(i + 1, s.length()), t + s.charAt(i));
            }
        }
    }

    public static void main(String args[]) {
        String a = "ABC";
        String t = "";
        allSet(a, t);
    }
}

If I have understood correctly, you're aiming for all subset of a String.
Through  your code for String = "ABC"; //Result is -- >  A AB ABC AC B BC C

However, for

String = "ABB";
// Result is --> A AB ABB AB B BB B (You see AB twice as well as letter B).

So to make it more clear for unique subsets, added a set implementation.

TreeSet (extra sorted feature over hash), as below : (using same recursion)

public class Test {

 public static void allSet(String s, String t, Set<String> set) {
     // System.out.print(t + " ");   will print later.
      if(!"".equalsIgnoreCase(t))
      set.add(t);
      if (s.length() > 0) {
          for (int i = 0; i < s.length(); i++) {
              allSet(s.substring(i + 1, s.length()), t + s.charAt(i),set);
          }
      }
  }
public static void main(String args[]) {
    String a = "ABB";
    String t = "";
    Set<String> set = new TreeSet<String>();
    allSet(a, t, set);
    System.out.println(set.toString());
   }

// Now, result --> [A, AB, ABB, B, BB]


Google Foobar Level 3 - Lucky Triples (Find the Access Codes) I have been working on this problem for 3 days now. I wrote a piece of java code that works perfectly on my machine testing in Eclipse, but when I put it in Foobar, it either returns "Error(400) Bad Request" or "Took too long to execute" or something else.

To quote it from this other question of the same Foobar question, the prompt of the challenge is essentially:

  Given a list (from 1 to 2000 elements) of random integers (from 1 to 999999) write a function, answer(l) that accepts a list as input and returns the number of "lucky triples" present in the list.

  For this purpose, a lucky triple is defined as a list of three numbers \$(x, y, z)\$ such that \$x\$ divides \$y\$, \$y\$ divides \$z\$, and \$x \le y \le z\$. So, for instance, \$(2, 4, 8)\$ is a lucky triple and so is \$(1, 1, 1)\$.

  Test cases:

  input: [1, 1, 1] ouput: 1

  input: [1, 2, 3, 4, 5, 6] output: 3

public static int answer(int[] l) {
    ArrayList<ArrayList<Integer>> log = new ArrayList();
    for(int i1=0; i1 < l.length; i1++){
        for(int i2=i1+1; i2 < l.length; i2++){
            for(int i3=i2+1; i3 < l.length; i3++){
                if(l[i3]%l[i2]==0 && l[i2]%l[i1]==0 && !log.contains(new ArrayList<>(Arrays.asList(l[i1], l[i2], l[i3])))){
                    log.add(new ArrayList<>(Arrays.asList(l[i1], l[i2], l[i3])));
                }
            }
        }
    }
    return log.size();
}

My idea is to generate all the possible combinations number by number and see if that combination satisfies the divisible situation, and whether it has been already recorded, if not, then record the set of numbers in a "log". Then return the size of the log. Like I said, this piece of code runs perfectly on my own machine. So I wonder if it is a performance issue.

Optimizing this algorithm

                if(l[i3]%l[i2]==0 && l[i2]%l[i1]==0 && !log.contains(new ArrayList<>(Arrays.asList(l[i1], l[i2], l[i3])))){

I have two problems with this section of code.

First, in l[i2]%l[i1]==0, this will return the same result regardless of the value of i3.  So why is it inside the i3 loop?  It could be before that loop and skip the loop if not true.

Second, you use List.contains with an arbitrarily long list.  The longer the list, the slower that method is.  Consider using a HashSet instead.  Then the speed remains constant relative to the size.

With these two changes:

    public static int answer(int[] l) {
        Set<List<Integer>> log = new HashSet<>();
        for (int i1 = 0; i1 < l.length; i1++) {
            for (int i2 = i1 + 1; i2 < l.length; i2++) {
                if (l[i2] % l[i1] != 0) {
                    continue;
                }

                for (int i3 = i2 + 1; i3 < l.length; i3++) {
                    if ((l[i3] % l[i2] == 0)) {
                        log.add(Arrays.asList(l[i1], l[i2], l[i3]));
                    }
                }
            }
        }

        return log.size();
    }

Now you only check the third element if the first two elements are divisible.

And now, instead of using List.contains, you simply rely on the uniqueness of elements of a Set.

I also switched to use the interfaces as the types for log rather than the implementation.  This allows me to skip the new ArrayList operations altogether.

I'm not crazy about the variable names either, but I didn't try to change them.

Assumptions

Both your original and this variant assume that the list is sorted in ascending order.

Custom datatype

This solution probably takes longer for small result sets, because HashSet is often higher overhead than List.  You may be able to reduce the overhead by declaring your own type and providing an optimized hashCode method for it.  Or the compiler may have already optimized it.  You'll have to benchmark to be sure.


Create a program that will take user input, reverse it, and then check if it is a palindrome Assuming there is no punctuation. It should remove spaces and bring all characters down to lowercase.

My aim was to write code that directly represented my idea of how I would have done things.
This is relevant because there were many ways I could have implemented the cleanString() method and what you see below is not the first way. This is the way that I actually found logical and understand the code for. The other way was to use #include <algorithm> and transform which did not make sense to me despite working.

Here is my solution:

#include <iostream>
#include <vector>
#include <locale>

  std::vector <char> stringToVec(std::string inputString) {
    std::vector <char> letters;
    for (int a = 0; a < inputString.size(); a++) {
      letters.push_back(inputString.at(a));
    }
    return letters;
  }

std::string vecToString(std::vector <char> inputVec) {
  std::string reversedString("");
  for (int a = 0; a < inputVec.size(); a++) {
    reversedString.push_back(inputVec.at(a));
  }
  return reversedString;
}

std::string cleanString(std::string inputString) {
  std::locale loc;
  for (std::string::size_type i = 0; i < inputString.length(); i++) {
    inputString[i] = std::tolower(inputString[i], loc);
  }
  for (std::string::size_type i = 0; i < inputString.length(); i++) {
    if (inputString[i] == ' ') {
      inputString.erase(i, 1);
    }
  }
  return inputString;
}

std::string checkPalindrome(std::string reversedString, std::string originalString) {
  if (cleanString(originalString) == cleanString(reversedString)) {
    return "Yes";
  }
  return "No";
}

std::vector <char> reverseVec(std::vector <char> inputVec) {
  std::vector <char> reversedReturn;
  for (int a = inputVec.size() - 1; a >= 0; a--) {
    reversedReturn.push_back(inputVec.at(a));
  }
  return reversedReturn;
}

int main() {
  std::cout << "Enter the string you would like reversed: ";
  std::string userInput("");
  std::getline(std::cin, userInput, '\n');
  std::string reversedInput(vecToString(reverseVec(stringToVec(userInput))));
  std::cout << "Reversed: " << reversedInput << std::endl;
  std::cout << "Palindrome: " << checkPalindrome(reversedInput, userInput) << std::endl;
  return 0;
}

I think you have a classic case of not knowing the standard.
What you are writing is generally already available via either a normal constructor or a standard algorithm. In a few cases I would have just implemented a minor wrapper class that could be used by these facilities.
Lets have a look at a few and see how you could have utilized the standard:
  std::vector <char> stringToVec(std::string inputString) {
    std::vector <char> letters;
    for (int a = 0; a < inputString.size(); a++) {
      letters.push_back(inputString.at(a));
    }
    return letters;
  }

The first thing to point out is that you are passing the parameter by value std::string inputString. This means you are copying the parameter into the function. This can be expensive (especially with strings and vectors). Also you are not mutating them so much easier to pass by const reference.
  std::vector <char> stringToVec(std::string const& inputString) {
                                    //       ^^^^^^^
    std::vector <char> letters;
    for (int a = 0; a < inputString.size(); a++) {
      letters.push_back(inputString.at(a));
    }
    return letters;
  }

This means you can read the original parameter but can not modify it:
The method at() is a bounds checking access operator. If you know that your index is going to be in range then prefer to use operator[]. In the case above you always know that a is going to be in the correct range because you actively check to make sure it is in the correct range a < inputString.size().
  std::vector <char> stringToVec(std::string const& inputString) {
    std::vector <char> letters;
    for (int a = 0; a < inputString.size(); a++) {
      letters.push_back(inputString[a]);  // Don't need to valid range
                                          // When your loop is already
                                          // guaranteeing the range.
    }
    return letters;
  }

Most algorithms we use tend to use iterators, so get used to using them. So I would re-write the above loop in terms of iterators:
  std::vector <char> stringToVec(std::string const& inputString) {
    std::vector <char> letters;
    for (auto loop = std::begin(inputString), loop != std::end(inputString); ++loop) {
      letters.push_back(*loop);
    }
    return letters;
  }

This is such a common pattern that in C++11 they introduced the range based for expression. Basically this is a for() loop that works with an object that can accept std::begin() and std::end() being called on it. It is basically syntactic sugar to make the above loop simpler to write:
  std::vector <char> stringToVec(std::string const& inputString) {
    std::vector <char> letters;
    for (auto letter: inputString) {
      letters.push_back(letter);
    }
    return letters;
  }

But if we look at std::vector<> we see that it also has a constructor that accepts two iterators so you can simply use this to construct the vector:
  std::vector <char> stringToVec(std::string const& inputString) {
    std::vector <char> letters(std::begin(inputString), std::end(inputString));
    return letters;
  }

Or more simply:
  std::vector <char> stringToVec(std::string const& inputString) {
    return std::vector<char>(std::begin(inputString), std::end(inputString));
  }

We can go through the same processes in reverse of the next function vecToString().
std::string vecToString(std::vector<char> const& inputVec) {
  return std::string(std::begin(inputVec), std::end(inputVec));
}

Just as a little style niggle. The seporation of the template type from the class looks strange.
std::vector <char>
         ^^^^^     That space just looks strange.

Lets move to cleaning the string:
std::string cleanString(std::string inputString) {
  std::locale loc;
  for (std::string::size_type i = 0; i < inputString.length(); i++) {
    inputString[i] = std::tolower(inputString[i], loc);
  }
  for (std::string::size_type i = 0; i < inputString.length(); i++) {
    if (inputString[i] == ' ') {
      inputString.erase(i, 1);
    }
  }
  return inputString;
}

Again looking at your loops. I would convert them to using iterators and then range based for. But these are also classic algorithms supported by the standard.
Converting to lower case:
  for (std::string::size_type i = 0; i < inputString.length(); i++) {
    inputString[i] = std::tolower(inputString[i], loc);
  }

This is a standard transform:
 std::transform(std::begin(inputString), std::end(inputString), // Source
                std::begin(inputString),                        // Destination (can be source) as long as it it is already large enough.
                [&loc](unsigned char c){return std::tolower(c, loc)}
               );

Removing a particular character:
  for (std::string::size_type i = 0; i < inputString.length(); i++) {
    if (inputString[i] == ' ') {
      inputString.erase(i, 1);
    }
  }

This is a standard remove/erase
 auto newEnd = std::remove(std::begin(inputString), std::end(inputString), ' ');
 inputString.erase(newEnd, std::end(inputString));

This is so common we usually write it as a single line:
 inputString.erase(std::remove(std::begin(inputString), std::end(inputString), ' '), std::end(inputString));

Commonly referred to as the erase/remove pattern.
Simple tests can sometime be replaced by the trinary operator. BUT be careful if overused or used in bad places this technique can make the code work (so be careful of using this). But in this instance I think it makes the code easier to read:
if (cleanString(originalString) == cleanString(reversedString)) {
  return "Yes";
}
return "No";

You can simplify this to:
return (cleanString(originalString) == cleanString(reversedString))
    ? "Yes"
    : "No";

Reversing a container:
std::vector <char> reverseVec(std::vector <char> inputVec) {
  std::vector <char> reversedReturn;
  for (int a = inputVec.size() - 1; a >= 0; a--) {
    reversedReturn.push_back(inputVec.at(a));
  }
  return reversedReturn;
}

Like above you can improve your code by using iterators.
std::vector <char> reverseVec(std::vector<char> const& inputVec) {
  std::vector <char> reversedReturn;
  for (auto loop = std::rbegin(inputVec), loop != std::rend(inputVec); ++loop) {
    reversedReturn.push_back(*loop);
  }
  return reversedReturn;
}

The rbegin() and rend() provide `reverse iterators. They go in the opposite direction to your standard iterator but are basically the same.
We can expand this (like above to just constructing the container with the iterators).
std::vector <char> reverseVec(std::vector<char> const& inputVec) {
  return std::vector<char>(std::rbegin(inputVec), std::rend(inputVec));
}

There is also standard algorithm std::reverse() that reverse a container in place.
Using the above:
int main()
{
  std::cout << "Enter the string you would like reversed: ";

  std::string userInput;
  std::getline(std::cin, userInput, '\n');

  auto newEnd = std::remove(std::begin(userInput), std::end(userInput), ' ');
  std::erase(newEnd, std::end(userInput));

  std::transform(std::begin(userInput),  std::end(userInput),
                 std::begin(userInput),
                 [](unsigned char* c){std::tolower(c);});

  std::string reversedInput(std::rbegin(userInput), std::rend(userInput));

  std::cout << "Reversed: " << reversedInput << "\n"
            << "Palindrome: "
            << ((userInput == reversedInput) ? "Yes" : "No")
            << "\n";
}


Project Euler - Problem No.4 - Largest palindrome product according to the problem:

  A palindromic number reads the same both ways. The largest palindrome made from the product of two 2-digit numbers is 9009 = 91 × 99.
  Find the largest palindrome made from the product of two 3-digit numbers.

Here is my code:

def largest_palindrome_product(n:int) -> int:
    '''
    Returns largest palindrome whose a product of two n digit(base 10) integer
    :param n: the number of digits in the numbers we compute the product of
    :return: largest palindrome whose a product of two n digit(base 10) integer or -1 if non were found
    '''
    # Dealing with edge cases
    if n == 1:
        return 9
    elif n < 1:
        raise ValueError("Expecting n to be >= 1")

    mul_max = -1
    upper_boundary = (10**n) - 1
    lower_boundary = 10**(n-1)

    # Searching for the largest palindrome between the upper boundary and the lower one.
    for i in range(upper_boundary, lower_boundary, -1):
        for j in range(i, lower_boundary, -1):
            str_prod = str(i*j)
            if i*j > mul_max and str_prod[::-1] == str_prod:
                mul_max = i*j

    return mul_max

Here is a small test case for this code:

from ProjectEuler.problem4 import largest_palindrome_product

if __name__ == "__main__":
    # largest prime product is of 91*99 -> returns 9009
    print(largest_palindrome_product(2))
    # Checking edge cases -> returns 9
    print(largest_palindrome_product(1))
    # largest prime product is of 993*913 -> returns 906609
    print(largest_palindrome_product(3))

Let me know your thoughts on this solution :)

Errors

range(start, end) goes from the start value, inclusive, to the end value, exclusive.  So

for i in range(upper_boundary, lower_boundary, -1):

will not include lower_boundary in the values which will be tested, so you will be ignoring products where i would be 10 (two digit case) and 100 (three digit case).

Similarly, for j in range(i, lower_boundary, -1) will ignore products where j would be 10 and 100.

The solution is to use range(..., lower_boundary - 1, -1).

Special Case

Why is n == 1 special cased, to return 9?  Why don’t you trust the algorithm to return the correct value?  Oh, right, 9*1 wouldn’t be tested, because lower_boundary = 1, and got excluded due to the bug above.

Perhaps you should have examined this special case closer.

Optimizations

You compute i*j up to 3 times each loop.  You should compute it once, and store it in a variable, such as prod.

        prod = i * j
        str_prod = str(prod)
        if prod > mul_max and str_prod[::-1] == str_prod:
            mul_max = prod

You are searching in decreasing ranges for the outer and inner loops.  Why?  True: You’ll find the target value faster.  But you still search all product values where j <= i.  Is there any way of determining there won’t be any larger mul_max value, either from the inner loop, or from the outer loop, or both?  For instance, if i*j > mul_max is not true, would it be true for any smaller value of j?

Turning a integer into a string is an \$O(\log n)\$ operation.  Can you skip doing it for every product?

    for j in range(i, lower_boundary - 1, -1):
        prod = i * j

        if prod <= mul_max:
           break

        str_prod = str(prod)
        if str_prod[::-1] == str_prod:
            mul_max = prod

Can something similar be done with the for i in range(...) loop, to speed things up even further?


High/Low Guessing Game This is my first by-self written code. Looking for advice on what not to or to do so I could deal with any bad habits now. It's a guessing game and works perfectly fine. I just wanted to know if more experienced programmers would see any huge mistakes in the coding style.

Basically it generates a number and then asks for the user input. If the two match you win. I tried playing around with arrays, multiple classes and methods and I was curious if there is anything wrong here.

Class: GuessGame

package base;

import java.util.Scanner;

public class GuessGame {

    public static Scanner scanKeyboard = new Scanner(System.in);

    public static void main(String[] args) {

        Handler gameHandler = new Handler();

        System.out.println("Start guessing! You have " + gameHandler.guessLeft + " guesses left.");
        System.out.println(gameHandler.goal);

        while(!gameHandler.gameOver){
            if (gameHandler.guessLeft == 0) {
                gameHandler.gameOver = true;
                gameHandler.gameOver();
            } else if(gameHandler.isGuessRight()){
                gameHandler.gameWin();
                gameHandler.gameOver = true;
            } else{

                gameHandler.guessCount++;
                System.out.println("Give me your " + gameHandler.guessCount + ". guess!");
                gameHandler.guesses[gameHandler.guessCount] = scanKeyboard.nextInt();
                gameHandler.guessLeft--;
                }
            }

        scanKeyboard.close();

        }

Class: Handler

package base;

import java.util.Random;

public class Handler {

    public int guessLeft = 5;
    public int guessCount = 0;
    public int[] guesses = new int[guessLeft + 1];

    public boolean gameOver = false;

    public Random rand = new Random();
    public int goal = rand.nextInt(100);

    public Handler(){
    }

    public String PrintGuesses(){
        String str = " ";
        for(int i = 1; i < guessCount; i++){
            str += guesses[i] + " ";
        }
        return str;
    }

    public void gameOver(){
        System.out.println("You suck!");
        System.out.println("The answer was: " + goal);
        System.out.println("Your miserable guesses were:" + PrintGuesses()+ guesses[guessCount]);

    }

    public void gameWin(){
        System.out.println("You won!");
        System.out.println("You guessed it on your " + guessCount + ". guess. With the guess " + guesses[guessCount] + ".");
        System.out.println("Your glorious guesses were:" + PrintGuesses() + guesses[guessCount]);
    }

    public boolean isGuessRight(){
        if (guessCount != 0){
            if (guesses[guessCount] == goal){
                return true;
            } else if(guesses[guessCount] < goal){
                System.out.println("Too low.");
            } else if(guesses[guessCount] > goal){
            System.out.println("Too high.");
            }
        }
        return false;

    }

Naming

Handler is too generic and doesn't explain what this class is about. GameHandler would be better. Or maybe GameManager even better.

Methods should be named camelCase, so this violates the convention: PrintGuesses

The gameOver and gameWin methods' main job is to print messages.
So it would be better to name them as such, for example printGameOverMessage, printGameWinMessage.

Visibility

A good general rule of thumb:
you shouldn't manipulate the fields of objects directly like this: gameHandler.gameOver = true

All the fields in Handler should be private:

They should not be writable by other classes
Other classes should not need to see these implementation details

This is related to good encapsulation and information hiding:
other classes should not need to know how the game handler works,
they should be able to interact with it using public interface methods.

General coding style

The formatting is a bit inconsistent. Use your IDE's function to reformat the code nicely and consistently.
The empty public constructor of Handler is unnecessary, you can delete it, the compiler will add it automatically


User authentication using Passport I'm currently building a Node/Express app using Passport for user authentication and Sequelize for database queries. It's a 'To-do list' app which currently has 3 models: User, List and Task. My back-end database calls are done using an API. My front-end uses AJAX to retrieve the data.

This is my first time using Passport for authentication, so I was wondering if there are any security problems in my current code. I am specifically concerned about Users being able to retrieve info about other Users by modifying the requests to my API.

Here is an example of an API call to create a List:

// Create list
router.post('/lists', isLoggedIn, (req, res) => {
    models.List.build( { name: req.body.name, userId: req.user.id } ).save()
    .then(list => {
        res.send(list);
    })
    .catch(err => {
        res.send(err);
    });
});

// ... other API calls

function isLoggedIn(req, res, next) {
    if (req.isAuthenticated())
        return next();
    res.redirect('/');
}

And the front-end function that calls it:

// Create list and open it
function addList(name) {
    if (!name.length) return;
    $.ajax({
           type: "POST",
           url: '/api/lists',
           data: { name: name },
           success: function(data)
           {
               window.location = "/lists/"+data.id;
           }
         });
}

I am 99% sure that this is a secure implementation, since it seems like any data sent in the POST request is contained within req.body, whereas req.user is generated by Express/Passport on the server side. However there is still the 1% of me that thinks it might be possible to modify req.user in the POST request and be able to get information about other Users.

Are my worries for nothing, or is there a better way to implement this?

Given req.user is set at the server then, unless your server is compromised, you can ignore the 1%.

Relying on server-side user data is the correct approach - never trust the client.


Inversion of an array that contains indices I have an array of length n that contains numbers from 0 to n-1 in random order.
I now want to "inverse" this array to one that where index i corresponds to the location of i in the source array.

Example

Source_Array = {2,0,1}

Transform to:

Result_Array = {1,2,0}

This means I find the index 0 at position 1 in my source, 1 is at location 2 and so on.

I did this with the following function:

Dim Sourcelist As List(Of Integer) = Source.ToList
Dim Result(Source.Count - 1) As Integer
For i = 0 To Source.Count - 1
   Result(i) = Sourcelist.IndexOf(i)
Next

Casting the array to List and then slowly using IndexOf to select the indices is not really great. Is there a better method maybe using LINQ?

Both VB.NET and C# answers are very much appreciated.

Unfortunately I don't really know visual basic but I think that you will find it easy to grasp the idea (and implement it in any language) of the following java code:

int[] source = new int[]{2,0,1};
int[] dest = new int[source.length];
for (int i = 0; i < source.length; i += 1) {
    dest[source[i]] = i;
}

The code runs at \$O(n)\$ time whereas if you search for every element you get \$O(n^2)\$ time.


Is this a good example of the strategy pattern? I've been reading The Pragmatic Programmer for a few days and I've come across a reference to the strategy pattern. Looked it up in Design Patterns and figured I could refactor a piece of code I'm currently working on to comply with the strategy pattern.

I need to process text using many string algorithms much like Lucene analyze and tokenize text. This is for an importation routine. Strategy:

public class ChainOfProcessor: IStringProcessor
{
    public List<IStringProcessor> Processors;

    public ChainOfProcessor()
    {
        Processors = new List<IStringProcessor>();
    }

    public ChainOfProcessor Add<TProcessor>()
        where TProcessor: IStringProcessor, new()
    {
        return Add(new TProcessor());
    }

    public ChainOfProcessor Add(IStringProcessor processor)
    {
        Processors.Add(processor);
        return this;
    }

    public string Process(string input)
    {
        return Processors.Aggregate(input, (current, processor) => processor.Process(current));
    }
}

public interface IStringProcessor
{
    string Process(string input);
}

public class HtmlStripper: IStringProcessor
{
    public virtual string Process(string input)
    {
        var htmlDoc = new HtmlDocument();
        htmlDoc.LoadHtml(input);
        return htmlDoc.DocumentNode.InnerText;
    }
}

public class HtmlDecoder: IStringProcessor
{
    public string Process(string input)
    {
        return HttpUtility.HtmlDecode(input);
    }
}

In Design Patterns, the authors use a Composition class and Compositor classes as examples. The client is allowed to chose a single Compositor through Composition to do whatever the Compositor is supposed to do. There is a subtle difference in how the client uses the Composition in my example. Instead of allowing a single IStringProcessor to be used through some Composition class, I allow many, to be executed one after the other through the concrete class ChainOfProcessor. Usage:

var text = new ChainOfProcessor()
    .Add<HtmlStripper>()
    .Add<HtmlDecoder>()
    .Process(html);

It seems like a you should implement Chain of responsibility as you are passing the same string to different kind of processor with a particular sequence

http://www.dofactory.com/Patterns/PatternChain.aspx#_self1

This is a gud pattern in your scenario.

One more thing your StringProcessorChain might not be needed in this case....

Let me know if you need a more highlight on same


Return XML file from Web API I have a HttpGet method which returns XML file:

[HttpGet]
public async Task<IActionResult> Get()
{
    XmlDocument xmlDoc = new XmlDocument();
    await Task.Run(() => xmldoc.Load(_appEnvironment.WebRootPath + @"\\user.config"));
    return Ok(xmlDoc);
}

Colleague said to me that this piece of code has smell and that I interrupted SOLID principles, but he did not explained to me what exact I do wrong.

Guys, please review my code and please explain me what I've done wrong.

Dependency Inversion Priciple

The smell is that your code violates the Dependency Inversion Priciple which reads

  Entities must depend on abstractions not on concretions. It states that the high level module must not depend on the low level module, but they should depend on abstractions.

This meas that your API depends on the Load method of the XmlDocument and not an abstraction such as a service like this one.

interface IXmlLoader
{
    XmlDocument Load(string path);
}

In consequence you cannot replace it for testing or use dependency injection and easily exchange it later if necessary. A correct implementation could look like this where you pass the dependency via the constructor. In some cases it might be also a property:

class XmlController
{
    private readonly IXmlLoader _xmlLoader;
    public XmlControler(IXmlLoader xmlLoader)
    {
        _xmlLoader = xmlLoader;
    }

    // Sometimes a property.
    // public IXmlLoader XmlLoader { get; set; }

    [HttpGet]
    public async Task<IActionResult> Get()
    {
        await Task.Run(() => _xmlLoader.Load(_appEnvironment.WebRootPath + @"\\user.config"));
        return Ok(xmlDoc);
    }

}

This is just an example. In your real code you should add null checks.

Another smaller smell is the hardcoded string user.config that you should at least configure via a property.


Faster way to render XML (10MB) file, loop the content and display output <?php
$xml = simplexml_load_file("http://www.example.com/example.xml");
            //echo "
";
            //print_r($xml);
            $cnt =0;
            $customereview='';
            foreach($xml->comment as $comment){
                    if($comment->ProdNid == $productID){
                        if($comment->PostedOn != '' && $comment->PostedBy != '' && $comment->comment_text != ''){
                            if($cnt < 5) {
                            $cnt++;
                            $customereview.="
".$title."".(string)$comment->comment_text."".(string)$comment->PostedBy.",".(string)$comment->PostedOn ."

";
                                if($cnt==5)
                                {
                                    break;
                                }
                            }

                        }//end of if
                    }
                }

                echo $customereview;
 ?>

There is no error handling.  Loading a remote resource is somewhat likely to fail.  Unfortunately, simplexml doesn't make error-handling easy.

Instead of filtering the XML using PHP code, I suggest selecting exactly the nodes you want using an XPath query.  The XPath library is more likely to be optimized than your PHP code.  In any case, it is certainly simpler to maintain one string describing the relevant nodes than a bunch of nested PHP statements.

I've chosen to use PHP templating for rendering, as it is easier to see that it is output.

<?php
$xml = simplexml_load_file('http://www.example.com/example.xml');
foreach ($xml->xpath("//comment[ProdNid = '" . htmlspecialchars($productId) .
                     "'][PostedOn][PostedBy][comment_text][position() <= 5]") as $comment) {
?>

      <?php echo htmlspecialchars($title);
      ?><?php echo htmlspecialchars($comment->comment_text);
      ?><?php echo htmlspecialchars($comment->PostedBy);
      ?>,<?php echo htmlspecialchars($comment->PostedOn); ?>

<?
}

All of those output strings probably need to be escaped to prevent, for example, cross-site scripting.  I've used htmlspecialchars() for that.


Ownership and deletion responsibility I have a class that delegates some responsibilities to another helper class which is abstract. When instantiating the class the user needs to provide a pointer to a concrete instance of the abstract helper class. I'm wondering who should be responsible for deleting the helper class instance, the user, or the class itself?

It seems to me that it is better for the class to delete the helper since it ensures that the lifetime of the helper class instance is the same as the lifetime of the class thats using it.

P.S. I know shared pointers can help but I'd rather avoid them here.

Specifically which is better, this:

classifier.h

class Classifier {
 public:
  Classifier(const Trainer* trainer);
  Train(const RecordList& training_records);
  ~Classifier();
 private:
  const Trainer* trainer_;
  Vector parameters_;
}

classifier.cpp

Classifier::Classifier(const Trainer* trainer)
    : trainer_(trainer);

Classifier::Train(const RecordList& training_records) {
  parameters = trainer_->UpdateParameters(training_records);
}

Classifier::~Classifier() {
  delete trainer_;
}

main.cpp

int main() {
  Classifier classifier(new SgdTrainer());
}

Or is this better?

classifier.h

class Classifier {
 public:
  Classifier(const Trainer* trainer);
  Train(const RecordList& training_records);
 private:
  const Trainer* trainer_;
  Vector parameters_;
}

classifier.cpp

Classifier::Classifier(const Trainer* trainer)
    : trainer_(trainer);

Classifier::Train(const RecordList& training_records) {
  parameters = trainer_->UpdateParameters(training_records);
}

main.cpp

int main() {
  SgdTrainer trainer();
  Classifier classifier(&trainer);
}

It depends.  (By the way, as things are your Classifier::Train function is ill-formed due to missing a return type; I'll assume void.)

If you expect the value to need dynamic storage duration, use the first method, but use a smart pointer (std::unique_ptr, boost::scoped_ptr or similar).  That may remove the need for an explicitly-defined destructor; if not, remember the rule of three.

If you do not expect the value to need dynamic storage duration, use the second form.  I recommend passing trainer by const reference, but it really doesn't matter all that much.

If you can't decide, use the latter unless you have a compelling reason to use the former.


Speeding up Excel distance calculation using Bing API calls I am writing VBA in Excel to calculate the distance between an employee's home address and work address using Bing Maps API calls. The process follows this general flow:

1) Convert the employee's address to Lat-Long values using the GetLatLong function

2) Convert the employee's work address to Lat-Long values using the GetLatLong function

3) Calculate the distance between these two points using the GetDistance function

4) Calculate the drive time between these two points using the GetTime function

The spreadsheet looks like this:

The process is working, but it is excruciatingly slow. The employee population is approximately 2300, and it takes almost an hour to execute.

I am not a coder, but I can functionally modify found code to my purposes. This is an amalgamation of multiple different processes found through Google searches. The code pieces in use are:

Public Function GetDistance(start As String, dest As String)
    Dim firstVal As String, secondVal As String, lastVal As String
    firstVal = "https://dev.virtualearth.net/REST/v1/Routes/DistanceMatrix?origins="
    secondVal = "&destinations="
    lastVal = "&travelMode=driving&o=xml&key=<My Key>&distanceUnit=mi"
    Set objHTTP = CreateObject("MSXML2.ServerXMLHTTP")
    Url = firstVal & start & secondVal & dest & lastVal
    objHTTP.Open "GET", Url, False
    objHTTP.setRequestHeader "User-Agent", "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)"
    objHTTP.send ("")
    GetDistance = Round(WorksheetFunction.FilterXML(objHTTP.responseText, "//TravelDistance"), 0) & " miles"
End Function

Public Function GetTime(start As String, dest As String)
    Dim firstVal As String, secondVal As String, lastVal As String
    firstVal = "https://dev.virtualearth.net/REST/v1/Routes/DistanceMatrix?origins="
    secondVal = "&destinations="
    lastVal = "&travelMode=driving&o=xml&key=<My Key>&distanceUnit=mi"
    Set objHTTP = CreateObject("MSXML2.ServerXMLHTTP")
    Url = firstVal & start & secondVal & dest & lastVal
    objHTTP.Open "GET", Url, False
    objHTTP.setRequestHeader "User-Agent", "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)"
    objHTTP.send ("")
    GetTime = Round(WorksheetFunction.FilterXML(objHTTP.responseText, "//TravelDuration"), 0) & " minutes"
End Function

Public Function GetLatLong(address As String, city As String, state As String, zip As String)
    Dim firstVal As String, secondVal As String, thirdVal As String, fourthVal As String, lastVal As String
    firstVal = "https://dev.virtualearth.net/REST/v1/Locations?countryRegion=United States of America&adminDistrict="
    secondVal = "&locality="
    thirdVal = "&postalCode="
    fourthVal = "&addressLine="
    lastVal = "&maxResults=1&o=xml&key=<My Key>"
    Url = firstVal & state & secondVal & city & thirdVal & zip & fourthVal & address & lastVal
    Set objHTTP = CreateObject("MSXML2.ServerXMLHTTP")
    objHTTP.Open "GET", Url, False
    objHTTP.setRequestHeader "User-Agent", "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)"
    objHTTP.send ("")
    GetLatLong = WorksheetFunction.FilterXML(objHTTP.responseText, "//Point//Latitude") & "," & WorksheetFunction.FilterXML(objHTTP.responseText, "//Point//Longitude")
End Function

To be clear, the process works well, just extremely slowly. Any thoughts on speeding this up?

Depending on what you want to return you can actually leverage just one API method from Bing to retrieve the data you are after, the Driving Route API. Also, as others have pointed out what's killing performance is the synchronous code.

What I've done below is limited the pull to a single request, and made the code asynchronous.

I've also changed this code to a sub, as I'd need to iterate over some sort of collection. For ease of use, I used a range.

I've structured my data as the from address being in the first column of the range, and the destination address being in the column immediately after that. Travel Distance and Travel Duration will be output offset relative to the from address (2 and 3 columns offset respectively).

I did a quick benchmark, this is taking just over 4 seconds for 250 requests. Hope it helps!

Option Explicit

Const BaseURL As String = "http://dev.virtualearth.net/REST/V1/Routes/Driving?wp.0="
Const APIKey  As String = "YOUR_KEY"
Private Const READYSTATE_COMPLETE  As Long = 4

Public Sub GetDistances(Addresses As Range)
    Dim Server           As Object
    Dim ServerItem       As Variant
    Dim Servers          As Object
    Dim Cell             As Range
    Dim URL              As String

    Set Servers = CreateObject("Scripting.Dictionary")

    'Send all the requests up front, but don't wait for them to complete
    For Each Cell In Addresses
        'See here: https://docs.microsoft.com/en-us/bingmaps/rest-services/examples/driving-route-example for more details on this api
        URL = BaseURL & Cell & "&wp.1=" & Cell.Offset(0, 1) & "&key=" & APIKey & "&DistanceUnit=mi&DurationUnit=min&output=xml"
        Set Server = CreateObject("MSXML2.ServerXMLHTTP")
        Server.Open "GET", URL, True 'Last param will make request async
        Server.setRequestHeader "User-Agent", "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)"
        Server.send ("")
        Servers.Add Cell.Address, Server
    Next

    'Iterate each XML request sent to see if done
    For Each ServerItem In Servers.Keys()
        Set Server = Servers(ServerItem)

        While Server.readyState <> READYSTATE_COMPLETE
            DoEvents
        Wend

        'Parse result
        If Server.Status = 200 Then
            'Add result to the sheet to an offsetting column
            Addresses.Parent.Range(ServerItem).Offset(0, 2) = WorksheetFunction.FilterXML(Server.ResponseText, "/Response/ResourceSets/ResourceSet/Resources/Route/TravelDistance")
            Addresses.Parent.Range(ServerItem).Offset(0, 3) = WorksheetFunction.FilterXML(Server.ResponseText, "/Response/ResourceSets/ResourceSet/Resources/Route/TravelDuration") / 60
            'You can also return the lat/long from this request, see --> https://docs.microsoft.com/en-us/bingmaps/rest-services/examples/driving-route-example
        End If
    Next

End Sub

Sub ProcessData()
    Dim myRng As Range
    Dim t As Double
    t = Timer
    Set myRng = ThisWorkbook.Sheets(1).Range("A1:a250")
    GetDistances myRng
    Debug.Print Timer - t
End Sub


Optimizing Python loop with Numba for live load analysis I have written some code that computes flexural moments imposed by different trucks for a bridge with 300 ft length. Truck data are contained in two lists: ax_list and sp_list, which are the axle weights and axle spacings, respectively.

There is nothing much to the code, however, this needs to be repeated for millions of different truck types, and I am trying to optimize my code, which takes real long time when the actual data size set is concerned.

I tried using Numba to see if I can get any speed gains, but it did not change the execution time, whether I add Numba @jit decorators for each function or not. What am I doing wrong here? Any help would be welcome! I also included code to generate representative pseudo data for 1000 records below:

import random
from numba import jit
import numpy as np
from __future__ import division

#Generate Random Data Set

ax_list=[]
sp_list=[]

for i in xrange(1000):
    n = random.randint(3,10)
    ax = []
    sp = [0]
    for i in xrange(n):
        a = round(random.uniform(8,32),1)
        ax.append(a)
    for i in xrange(n-1):
        s = round(random.uniform(4,30), 1)
        sp.append(s)
    ax_list.append(ax)
    sp_list.append(sp)

#Input Parameters
L=300
step_size=4
cstep_size=4
moment_list=[]

@jit
#Simple moment function
def Moment(x):
    if x<L/2.0:
        return 0.5*x
    else:
        return 0.5*(L-x)

#Attempt to vectorize the Moment function, hoping for speed gains
vectMoment = np.vectorize(Moment,otypes=[np.float],cache=False)

@jit
#Truck movement function that uses the vectorized Moment function above
def SimpleSpanMoment(axles, spacings, step_size):
    travel = L + sum(spacings)
    spacings=list(spacings)
    maxmoment = 0
    axle_coords =(0-np.cumsum(spacings))
    while np.min(axle_coords) < L:
        axle_coords = axle_coords + step_size
        moment_inf = np.where((axle_coords >= 0) & (axle_coords <=L), vectMoment(axle_coords), 0)
        moment = sum(moment_inf * axles)
        if maxmoment < moment:
            maxmoment = moment
    return maxmoment

Then to run the loop for 1000 times:

%%timeit
for i in xrange(len(ax_list)):
    moment_list.append(np.around(SimpleSpanMoment(ax_list[i], sp_list[i], step_size),1))

yields:

1 loop, best of 3: 2 s per loop

Just a few quick points on your code:

any future imports should be at the top of any code. Are you using this intentionally (2to3?) or this is leftover code?
your loop to generate test data overrides i in two secondary loops - this is bad coding practice.
you're using xrange, which is pretty much deprecated since Python 2.3. Are you really using Python 2.3 or you just have stuck with xrange?
no entry point aka if __name__ == "__main__": to demonstrate the start of your code, you have pieces of code scattered around your listing.
Your code as-is with the random function cannot prove Numba JIT nor caching will have any effect. My results for your functions show:

CacheInfo(hits=0, misses=665297, maxsize=0, currsize=0)
CacheInfo(hits=0, misses=1000, maxsize=0, currsize=0)

Which is to be expected with a random dataset. It's recommended to put a real dataset somewhere on the 'net for people to use.
SimpleSpanMoment uses a global variable L, you should rewrite the function to inject all variables (or create them inside the function) and not use global variables.
cstep_size is unused.

Hope this helps you move towards a better implementation, to solve the performance issue. Good Luck!


Implement service with explicit interface implementation I would like to implement service with explicit interface implementation.
I don't want to have public methods in service, only possible way to create instance is trough interface.

I am not sure what is best way to call method of same class if method is explicit implementation of interface method.

You can see in my code that I am using "AsIArchiveService" property.

I have public interface in class library:

using Common.Entities;
using System.Threading.Tasks;

namespace Common.Services
{
    public interface IArchiveService
    {
        Task<ArchiveConfiguration> GetConfiguration();

        Task<bool> Update(ArchiveConfiguration machine);
    }
}

Serivice class in main project:

using Common.Configurations;
using Common.Entities;
using Common.Services;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using RemoteManager.EntityFramework;
using System.IO;
using System.Linq;
using System.Threading.Tasks;

namespace Manager.Services
{
    internal class ArchiveService : IArchiveService
    {
        private readonly IOptions<ApplicationConfiguration> applicationConfiguration;
        private readonly ILogger<ArchiveService> logger;
        private readonly ManagerContext context;
        IArchiveService AsIArchiveService
        {
            get { return this; }
        }

        internal ArchiveService(ManagerContext context, IOptions<ApplicationConfiguration> applicationConfiguration, ILogger<ArchiveService> logger)
        {
            this.applicationConfiguration = applicationConfiguration;
            this.logger = logger;
            this.context = context;
        }

        async Task<ArchiveConfiguration> IArchiveService.GetConfiguration()
        {
            try
            {
                logger.LogInformation($"Start  {nameof(ArchiveService)}:{nameof(AsIArchiveService.GetConfiguration)}");
                if (this.context.ArchiveConfiguration.Count() == 0)
                {
                    var res = this.context.ArchiveConfiguration.Add(new ArchiveConfiguration() { isArchive = true, maxDataSetCount = 20, path = Path.Combine("C:/", "TestArchive") });
                    this.context.SaveChanges();
                }
                var result = this.context.ArchiveConfiguration.First();
                return await Task.FromResult(result);
            }
            catch
            {
                logger.LogError($"Error {nameof(ArchiveService)}:{nameof(AsIArchiveService.GetConfiguration)}");
                throw;
            }
        }

        async Task<bool> IArchiveService.Update(ArchiveConfiguration archive)
        {
            try
            {
                logger.LogInformation($"Start {nameof(ArchiveService)}:{nameof(AsIArchiveService.Update)}");
                this.context.Entry(archive).State = Microsoft.EntityFrameworkCore.EntityState.Modified;
                this.context.SaveChanges();
                return await Task.FromResult(true);
            }
            catch
            {
                logger.LogError($"Error {nameof(ArchiveService)}:{nameof(AsIArchiveService.Update)}");
                throw;
            }
        }
    }
}

You're only using AsIArchiveService to statically get the name of the current method.  Instead, you could go off the interface directly, nameof(IArchiveService.GetConfiguration), which obviously returns the same value as going through AsIArchiveService.GetConfiguration.

logger.LogInformation($"Start  {nameof(ArchiveService)}:{nameof(IArchiveService.GetConfiguration)}");

If you changed your four usages in your log lines to go this route, you could dispense with the peculiar property you created altogether.


Is this a good approach for a BayeuxClient class? Is this a good approach for a BayeuxClient class?

Reading cometd reference book, Chapter 6 Java Libraries, I created this BayeuxClient class.

As a newbie to cometd, I would like to know if this class has any flaws:

import java.util.*;
import java.net.URL;
import org.cometd.client.BayeuxClient;
import org.eclipse.jetty.client.HttpClient;

public class CometD_Client extends BayeuxClient
{
    private URL BayeuxServer_URL;

    private HttpClient JettyHttpClient;

    private Map<String, Object> TransportOptions;

    private ClientTransport C_Transport;

    private ClientSession C_Session;

    private boolean HandIsShaken£ = false;

    private final ClientSessionChannel.MessageListener MyChannelListener;

    public CometD_Client(URL BayeuxServer_URL)
    {
        this.BayeuxServer_URL = BayeuxServer_URL;

        // Create (and eventually set up) Jetty's HttpClient;
        JettyHttpClient = new HttpClient();
        JettyHttpClient.start();

        // Prepare the transport;
        TransportOptions = new HashMap<String, Object>();

        ClientTransport C_Transport =
            LongPollingTransport.create(TransportOptions,
                                        JettyHttpClient  );

        C_Session = new BayeuxClient(URL, C_Transport);

        C_Session.handshake();

        HandIsShaken£ =
            C_Session.waitFor(1000, BayeuxClient.State.CONNECTED);

        if(HandIsShaken£)
        {
            // TO DO: Output that the handshake had succeed;
        }
        else
        {
            // TO DO: Output that the handshake had failed;
        }

        MyChannelListener = new AChannelListener();

        C_Session.getChannel(Channel.META_HANDSHAKE).addListener
        (
         new ClientSessionChannel.MessageListener()
         {
            @Override
            public void onMessage(ClientSessionChannel ThisMetaChanel,
                                  Message msg)
            {
                // TO DO: Output msg to check it out;

                C_Session.getChannel("/myBroadcast/myChannel").subscribe
                (
                 new ClientSessionChannel.MessageListener()
                 {
                    @Override
                    public void onMessage(ClientSessionChannel ThisMetaChanel,
                                          Message msg)
                    {
                        // TO DO: Handle the received messages to this chaned;
                    }
                 }
                );
            }
         }
        );
    }

    public boolean GetIfHandIsShaken£()
    {
        return HandIsShaken£;
    }
}

I have no idea how to type the last character on my keyboard which makes modification harder:

public boolean GetIfHandIsShaken£()

It's better to use only ASCII letters.
See Effective Java, 2nd edition, Item 56: Adhere to generally accepted naming conventions and The Java Language Specification, Java SE 7 Edition, 6.1 Declarations:

  Class and Interface Type Names

  Names of class types should be descriptive nouns or noun phrases, not overly long, in mixed
  case with the first letter of each word capitalized.

  [...]

  Names of fields that are not final should be in mixed
  case with a lowercase first letter and the first letters of
  subsequent words capitalized.

CometD_Client could have a more descriptive classname too. Every CometD client could have been called CometdClient. Why is it special? Try to put that into the name.

TransportOptions = new HashMap<String, Object>();

This could be in the same line as the declaration:

private Map<String, Object> transportOptions = new HashMap<String, Object>();

HttpClient has a stop() method. I suppose you should call it somewhere.


An example of transferring the C ++ class hierarchy to Golang. Is it done correctly? I am trying to transfer my c ++ experience to the Go area. I'm new to Go, so not sure if the implementation is correct. In any case, it looks very ugly compared to C ++ code. Is there a nicer solution? Both solutions are executable in playground and give same result.
c++ playground: https://code.sololearn.com/c67xbW2AEWW2/#cpp
#include <iostream>
using namespace std;

// Abstract class
class AbstractGenerator
{
public:
    AbstractGenerator() {}
    std::string generate(std::string some_internal_data)
    {
        _some_internal_data = some_internal_data;
        return step1() + step2();
    }

protected:
    // some shared fuctions
    std::string generationHelper1(std::string s) { return s + _some_internal_data + "123"; }
    std::string generationHelper2(std::string s) { return s + _some_internal_data + "456"; }
    // abstract fucntions
    virtual std::string step1() = 0;
    virtual std::string step2() = 0;

private:
    std::string _some_internal_data;
};
// Implementation
class MyGenerator : public AbstractGenerator
{
public:
    MyGenerator() {}

protected:
    std::string step1() override { return generationHelper1("my step1") + generationHelper2("my step1"); }
    std::string step2() override { return "my step2"; }
};

int main() {
    MyGenerator my_gen;
    AbstractGenerator* abstract_gen = &my_gen;
    cout << abstract_gen->generate("data");
    return 0;
}

Go playground: https://play.golang.org/p/e3iXfYar4YJ
package main

import "fmt"

// some shared fuctions - interface
type ImplementationHelper interface {
    generationHelper1(s string) string
    generationHelper2(s string) string
}

// abstract fucntions - interface
type Implementation interface {
    step1() string
    step2() string
}

// Public main interface
type Generator interface {
    Generate(s string) string
}

type AbstractGenerator struct {
    someInternalData string
    implementation   Implementation
}

func NewAbstractGenerator(i Implementation) *AbstractGenerator {
    g := new(AbstractGenerator)
    g.implementation = i
    return g
}
func (g *AbstractGenerator) Generate(s string) string {
    g.someInternalData = s
    return g.implementation.step1() + g.implementation.step2()
}
func (g *AbstractGenerator) generationHelper1(s string) string { return s + g.someInternalData + "123" }
func (g *AbstractGenerator) generationHelper2(s string) string { return s + g.someInternalData + "456" }

// Implementation of AbstractGenerator
type MyGenerator struct {
    basic *AbstractGenerator
}

// MyGenerator constructor
func NewMyGenerator() *MyGenerator {
    myGen := new(MyGenerator)
    myGen.basic = NewAbstractGenerator(myGen)
    return myGen
}

// Interface Generator
func (i *MyGenerator) Generate(s string) string {
    return i.basic.Generate(s)
}

// Interface Implementation
func (i *MyGenerator) step1() string {
    return i.basic.generationHelper1("my step1") + i.basic.generationHelper2("my step1")
}
func (i *MyGenerator) step2() string {
    return "my step2"
}

func main() {
    myGen := NewMyGenerator()

    var abstractGen Generator
    abstractGen = myGen

    fmt.Println(abstractGen.Generate("data"))
}

This:
g := new(AbstractGenerator)
g.implementation = i
return g

Can be more simply written as:
return &AbstractGenerator{implementation: i}


Manually formatting numbers in Python I was working on a little Python challenge that was a bit more difficult than expected: format any non-negative number so 1000 turns into "1,000" , 10000000 into "10,000,000" and so forth. I managed to do it, but I find my solution not very readable:

def formatting(n):
    m = str(n)  # Turn it into string
    m2 = []  # Empty list that will have the split version of the final number
    for i in range(0,len(m),3):  # Iterate every three elements

        m2.append(m[::-1][i:i+3][::-1])  # Append last three numbers, second to last three numbers, etc.
        m2.append(',')  # Add a comma between loops
    m3 = ''.join(m2[::-1])[1:] # Join and drop the first element which is always a comma.
    return m3

This works but it isn't as nice as it could be. What can be done?

A few points:

Reversing the string m multiple times is unnecessary. Slicing can be done on the original string using correct indices. m[::-1][i:i+3][::-1] is equivalent to m[-i-3:len(m)-i]. Another approach is to traverse the indices in the reverse order for i in range(len(m)-1, -1, -3) and then perform slicing accordingly.
The logic can be simplified using str.join. m2.append(',') can be omitted. And you can just do

m3 = ','.join(m2[::-1])

Further improvements can be achieved by using list comprehensions and traversing the string from the beginning rather than in the reverse order. Some calculation for the indices are needed:

def formatting(n):
    m = str(n)  # Turn it into string
    num_digits = len(m)
    seg1_len = (num_digits % 3) or 3  # Calculate length of the first segment
    segments = [m[:seg1_len], *(m[i:i+3] for i in range(seg1_len, num_digits, 3))]
    return ','.join(segments)

Python 3's built-in formatting specification actually supports the thousands separator. So you can just do this to achieve the same functionality:

formatted_n = f"{n:,}"

Or equivalently:

formatted_n = "{:,}".format(n)


Use and understanding of async/await in .NET 4.5 + I am just about to embark upon a massive job of multi-threading a cost-engine.  I could use TPL of which I am very familiar, but would like to leverage the async/await keywords of .NET 4.5 to make life that little bit simpler.

Is my understanding of what is going on correct?  How can I improve this design?

CancellationTokenSource cancelSource;

// Mark the event handler with async so you can use await in it.
private async void StartButton_Click(object sender, RoutedEventArgs e)
{
    cancelSource = new CancellationTokenSource();
    CancellationToken token = cancelSource.Token;
    TaskScheduler uiScheduler = TaskScheduler.FromCurrentSynchronizationContext();
    IProgress<ProgressInfo> progressInfo = new Progress<ProgressInfo>(ReportProgress);
    await ProcessScript(progressInfo, token, uiScheduler);

    // await sets up the following as an Contnuation to
    // continue once returned from await.
    this.resultsTextBox.AppendText("\nReturn to caller...\n\n");
}

// Class to report progress...
private void ReportProgress(ProgressInfo progressInfo)
{
    this.progressBar.Value = progressInfo.PrecentComplete;
    this.progressLabel.Content = progressInfo.Message;
}

private async Task ProcessScript(IProgress<ProgressInfo> progressInfo,
                                 CancellationToken token,
                                 TaskScheduler uiScheduler)
{
    ProgressInfo pi = new ProgressInfo();
    pi.PrecentComplete = 0;
    pi.Message = "In script processor...";
    progressInfo.Report(pi);

    Thread.Sleep(5000); // This is UI Blocking...
    string str = this.resultsTextBox.Text;
    str = "We have added this => going into await...";
    this.resultsTextBox.AppendText(str);
    Task task = Task.Factory.StartNew(() => LongRunning(uiScheduler));

    pi.Message = "Task set";
    pi.PrecentComplete = 50;
    progressInfo.Report(pi);

    // awaits the long runniing task - non-UI blocking.
    await task;

    // The await above sets this up as a continuation on the UI thread.
    pi.Message = "Task awaited";
    pi.PrecentComplete = 95;
    progressInfo.Report(pi);

    this.resultsTextBox.AppendText("Where are we now??");
    return;
}

private bool LongRunning(TaskScheduler uiScheduler)
{
    // Allow access to the UI from this background thread from
    // the ThreadPool.
    Task.Factory.StartNew(() =>
        {
            this.resultsTextBox.AppendText("\n\nNow in 'LongRunning'!! Waiting 5s simulating HARD WORK!!");
        }, CancellationToken.None,
           TaskCreationOptions.None,
           uiScheduler);

    // Simulate hard work.
    Thread.Sleep(7000);
    Task.Factory.StartNew(() =>
    {
        this.resultsTextBox.AppendText("\n\n\nHARD WORK COMPLETE!!");
    }, CancellationToken.None,
       TaskCreationOptions.None,
       uiScheduler);
    return true;
}

Yes, your understanding is correct. Note that you're not using CancellationToken provided to ProcessScript. If your operations are cancellable and/or you're using other asynchronous API that accepts CancellationTokens it's highly recommended to pass it through the code, otherwise just don't create the CancellationTokenSource.

Following your comments:

Thread.Sleep(5000); // This is UI Blocking...

This is correct since async methods return control to the caller only when something is  awaited.

// awaits the long runniing task - non-UI blocking.
await task;

Here we release the UI thread and running a (computing) task in parallel. The reason I've added "computing" is that you're manually spanning a new task using Task.Factory. In case of I/O-related task returned by .NET framework it won't actually represent a new thread as it can wait for external data on the same thread.

// The await above sets this up as a continuation on the UI thread.

Correct. See good article Await, SynchronizationContext, and Console Apps that describes in details the behavior of await.

// Allow access to the UI from this background thread from
// the ThreadPool.
Task.Factory.StartNew(() =>
    {
        this.resultsTextBox.AppendText("\n\nNow in 'LongRunning'!! Waiting 5s simulating HARD WORK!!");
    }, CancellationToken.None,
       TaskCreationOptions.None,
       uiScheduler);

You're passing TaskScheduler corresponding to UI thread so naturally the code will be safe to work with UI. I would recommend using IProgress<T> to update/inform UI about changes in long-running task though, since you may want to separate computation logic from UI-related code.


Generating a string with only one space between the words I wrote a program that inputs a string with spaces between them and should allocate a new string with only 1 space between the words. I was wondering if there's a better way to write it (I rewrote it a couple of times and I'm trying to find the most effective way to write this code).

#define _CRT_SECURE_NO_WARNINGS
#define SIZE 80
#include <stdio.h>
#include <string.h>
#include <stdlib.h>

char* UpgradeString(char* oldText);
void inputStr(char* str);
int main()
{
    char str[SIZE], *ans;
    printf("Enter string\n");
    inputStr(str);

    ans = UpgradeString(str);
    puts(ans);

}
void inputStr(char* str)
{
    int i = 0;
    fgets(str, SIZE - 1, stdin);
    while (str[i] != '\n')i++;
    str[i] = 0;

}
char* UpgradeString(char* oldText)
{
    char temp[SIZE], *ans;
    int i=0, k = 0;

    while (1)
    {
        if (oldText[i] == 0)break;
        if (oldText[i] != ' ')
        {
            temp[k] = oldText[i];
            k++;
        }
        if (oldText[i] == ' '&&(oldText[i + 1] != ' '&&oldText[i + 1] != 0))
        {
            temp[k] = oldText[i];
            k++;
        }
        if (oldText[i] == ' ')i++;
        else i++;
    }
    temp[k] = 0;
    ans = (char*)malloc(sizeof(char)*(strlen(temp) + 1));
    strcpy(ans, temp);
    return ans;

}

Always specify length of array pointers.

In C, it is not always guaranteed that an array of char will be null terminated and therefore, the size can be determined by calling strlen. Always pass along the length of the array when passing a pointer to a function. This is not just for strings, but also for other pointer types which point to a dynamically allocated and static arrays.

Naming

Choose a consistent naming plan. The most commonly found style in most C code is snake_case. But whatever you choose, stick to it and don't deviate.

Const-correctness

If a method is not going to modify it's parameters, make that intention clear by accepting the arguments as const. This form of programming is what makes code self-documenting because you don't need to tell anyone that the function does not modify it's parameter, the const specifier tells the whole story.

Malloc + free

Always remember, for every malloc, there should be an equal and opposite free. Not freeing memory leads to unexpected memory leakages. This may not apply to you now, but just know this, many man hours have gone into developing tools for detecting memory leaks (valgrind, leakSanitizer, etc) when the best programmers are making extra effort not to leak memory, but still fail. Memory leaks are no joke. Free the memory when you are finished with it.

With all that being said, here is my rendition of your code

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <ctype.h> /* isspace */
#include <stddef.h> /* size_t */

#define SIZE (size_t)80

void inputStr(char*, size_t);
char* upgradeString(const char const*, size_t);

int main() {
    char string[SIZE] = {'\0'}, *result;
    printf("Enter string: ");
    inputStr(string, SIZE);

    size_t strLength = strlen(string);

    if (strLength > 0 && isspace(string[strLength - 1])) {
        string[--strLength] = '\0';
    }

    result = upgradeString(string, strLength);
    puts(result);
    free(result);
}

void inputStr(char* str, size_t length) {
    fgets(str, length, stdin);
}

char* upgradeString(const char const* oldText, size_t length) {

    size_t i, newSize = 0, foundSpace = 0;
    for (i = 0; i < length; i++) {
        if (isspace(oldText[i])) {
            if (foundSpace) {
                continue;
            }
            foundSpace = 1;
        } else {
            foundSpace = 0;
        }
        newSize++;
    }

    char *result = malloc((sizeof *result) * (newSize + 1));

    if (result) {
        char *temp = result;
        foundSpace = 0;
        for (i = 0; i < length; i++) {
            if (isspace(oldText[i])) {
                if (foundSpace) {
                    continue;
                }
                foundSpace = 1;
            } else {
                foundSpace = 0;
            }
            *temp++ = oldText[i];
        }
    }

    return result;
}

The changes made are based on the following additional considerations:

What happens if the user enters nothing (standard input is empty)?
What happens if the user just presses enter?

Conclusion

In conclusion be precise, be constant with naming, be explicit with intent. This will make your code robust and potentially bug free.

P.S.
It is not necessary to cast the result of malloc


Machine gun with multiple firing modes I have a class 'M249' it is initiated with 'ammo'
The class has one responsibility(I hope) to expend ammo.
Does there exist a way to increment without accessor or writer?

I am shaky on when I should be using mixins, or inheritance. I think module is best here?

module MG_tools
  Rof = {:sustained => rand(3..9), :rapid => rand(5..9), :cyclic =>
rand(1..200), :cease => 0}
end

class M249
include MG_tools
  attr_accessor :ammo
  def initialize( ammo )
    @ammo = ammo
  end

  def fire(command)
    self.ammo -= Rof[command]
    raise ArgumentError.new("Out of Ammo!") if self.ammo < 1
  end
end
command = :sustained
gunner = M249.new(200)

loop do
case gunner::ammo
  when (200..1000)
   command = :cyclic
  when (100..199)
   command = :rapid
  when (6..99)
   command = :sustained
  when (1..5)
   command = :cease
end
    puts "#{command.upcase} FIRE"
    break if command == :cease
    gunner.fire(command)
    puts "*MACHINE GUN NOISES*"
    puts "SAW: #{gunner::ammo} rounds left!"

end

Without more code / context it is hard to give much of an answer, but a few comments:

You can make ammo a read only attribute by using attr_reader :ammo then inside the class or module you can access it using @ammo just like you do in your initializer.
Don't over-complicate your code. Given what you have now there is no reason to do any of what you want. Wait till your code gets complicated enough that you need to break it up then you won't just be guessing what the best option is.
Consider using delegation rather than inheritance or modules. So your gun could have an ammo_clip attribute referencing an AmmoClip instance.
This code:

Rof = {:sustained => rand(3..9), :rapid => rand(5..9),
       :cyclic => rand(1..200), :cease => 0}

probably doesn't do what you expect. Those random numbers will be calculated once at program startup, not each time a gun is fired.


Is this `img` creator decently secure from XSS? This is being used now, seems decent to me but I'm curious.

    if (m=s.match(/^\s*(https?:\/\/[^\s<>"]+)\.(bmp|png|webp|gif|jpg|jpeg|svg)\s*$/)) {
         // example s: http://mustachify.me/?src=http://www.someImage.jpg
        return '';
    }
    if (m=s.match(/^\s*(https?:\/\/[^\s<>?"]+)\.(bmp|png|webp|gif|jpg|jpeg|svg)(\?[^\s<>?"]*)?\s*$/)) {
        // example s: http://place.com/photo/something.jpg?height=600&modified_at=1384796271&ratio_x=03&ratio_y=02&width=900
        return '';
    }

I've been trying to get around it but haven't yet managed it. So I'm thinking of just using this.

The part of your code shown here does not look like it's vulnerable to XSS, but it could be cleaner. You seem to be doing 3 separate things:

Ensuring that a URI looks like a path to an image.
Ensuring that a URI doesn't contain certain characters like quotes and angle brackets.
Trimming whitespace from the ends of a URI.

Doing those things in discreet steps would be cleaner. If you trim the string first, you won't need to look at the match results, only check whether there was a match. That means you can avoid the poor style used in the construct if (m=foo) without having to rewrite that part, and you can eliminate redundancy.

You could use encodeURI instead of throwing out URIs with certain characters; this way the user doesn't have to encode those characters before passing you the string.

s = s.trim();
if (s.match(/^https?:\/\/.*?\.(bmp|png|webp|gif|jpg|jpeg|svg)(\?.*)?$/)) {
    // looks like a path to an image
    return '<img src="' + encodeURI(s) + '">';
}

If you are doing this on the client, consider creating an image element and returning that instead of returning a string.

s = s.trim();
if (s.match(/^https?:\/\/.*?\.(bmp|png|webp|gif|jpg|jpeg|svg)(\?.*)?$/)) {
    var img = new Image();
    img.src = encodeURI(s);
    return img;
}

Also, the regular expressions you are using and the one in this example will match things like http://lol.jpg; you may want to consider making it more strict.


Calling Stored Procedure "insert query" using EF 6, UOW, and MVC 5 I'm a beginner in MVC programming, repository, and UOW using Entity framework. I tried to calling my existing SP in my MVC project using UOW and EF.

Here is my code to mapping my SP in myDbContext file:

modelBuilder.Entity<Applicant>().MapToStoredProcedures(
            s => s.Insert(
                i => i.HasName("InsertApplicant")
                .Parameter(fn => fn.First_Name,"FirstName")
                .Parameter(ln => ln.Last_Name,"LastName")
                .Parameter(em => em.Email,"Email")
                .Parameter(tn => tn.Tel_No,"TelNo")
                .Parameter(mn => mn.Mobile_No,"MobileNo")
                .Parameter(vt => vt.Visa_Type,"VisaType")
                .Parameter(lu => lu.LinkedIn_URL,"LinkedInURL")
                .Parameter(ob => ob.Objective, "Objective")
                .Parameter(ac => ac.Active, "Active")));

And, below is the code I used to call the SP on my controller (i don't know if it's right or not):

using (var context = new MyExperienceDbContext())
            {
                var fn = new SqlParameter("@FirstName", entity.First_Name);
                var ln = new SqlParameter("@LastName", entity.Last_Name);
                var em = new SqlParameter("@Email", entity.Email);
                var pn = new SqlParameter("@TelNo", entity.Tel_No);
                var mn = new SqlParameter("@MobileNo", entity.Mobile_No);
                var vt = new SqlParameter("@VisaType", entity.Visa_Type);
                var lu = new SqlParameter("@LinkedInURL", entity.LinkedIn_URL);
                var ob = new SqlParameter("@Objective", entity.Objective);
                var ac = new SqlParameter("@Active", entity.Active);

                context.Database.ExecuteSqlCommand("InsertApplicant @FirstName,@LastName,@Email,@TelNo,@MobileNo,@VisaType,@LinkedInURL,@Objective,@Active ",
                    fn, ln, em, pn, mn, vt, lu, ob, ac);
                return RedirectToAction("Index");
            }

It works fine for me, but I noticed if I delete the mapping's code in myDbContext file everything still works fine. I believe i'm failed to implemented the UOW and repository concept in EF when calling my existing SP, right?
Do you have any idea with my question above or do you have tutorial suggestion to calls SP using UOW and EF in MVC project?

Your mapping is designed to call a stored procedure when you call context.Applicants.Add() and context.SaveChanges(). Your second code manually calls a stored procedure, that is not what you want to do. Simply keep your mapping:

using(var context = new MyExperienceDbContext())
{
    context.Applicants.Add(entity);
    context.SaveContext();
}

Also this code does not belong in your controller, it belongs in your repository.

public interface IApplicantRepository
{
    void AddApplicant(Applicant applicant);
}

For a more advanced look at Repositories please read the first part of my article: http://www.lind-i.com/Docs/DataSelectorsWithMVVM.pdf


Argument passing to dumb tasks I'm becoming acquainted with ruby by taking some of my legacy shell scripts and rewriting them as .rb.

The pattern that is emerging is that I have a number of different dumb tasks, that do what they are told, and a coordinator that is responsible for ordering the tasks, and providing to the tasks the data that will be needed.

For instance, at the end of one of my jobs, I will need to convert some data to a human readable format, perform a two phase upload of that data to a remote server, and then send a notification to subscribers announcing the successful delivery of the data.

class PublishReportProcess
  def initialize(convertor, uploader, email)
    @convertor = convertor
    @uploader = uploader
    @email = email
  end

  def run(task = {})
    convert_context = {}
    convert_context[:type] = 'geo'
    convert_context[:output] = "#{task[:workspace]}/#{task[:report_id]}.json"

    @convertor.convert(convert_context)

    upload_context = {}
    upload_context[:local] = convert_context[:output]
    upload_context[:staging] = "#{task['reports']['workspace']['uri']}/#{task['report_id']}"
    upload_context[:remote] = "#{task['reports']['repository']['uri']}/#{task['report_id']}"

    @uploader.upload(upload_context)

    email_context = {}
    email_context[:message] = <<MESSAGE
There is a new report available at #{task['reports']['repository']['uri']}/#{task['report_id']}.

Share and enjoy
MESSAGE

    @email.email(email_context)
  end
end

The implementation of the tasks, if it matters, tend to be using the arguments of the map passed to them to create a command line string that can be passed to Open3.popen2e.

The standard American English spelling is "converter", not "convertor". If it's different where you live, use the appropriate regional spelling, but if you live in America, use the American spelling. Since I do, I did.

You have @converter and @uploader, but @email. I suggest changing it to @emailer, for consistency's sake.

You mix symbols and strings as Hash keys. I suggest using all symbols, since it's much more standard. This means that

task['reports']['workspace']['uri']

becomes

task[:reports][:workspace][:uri]

Is the workspace at [:reports][:workspace] or [:workspace]? Based on the very similar [:reports][:repository] that you use, I'm assuming it's the former. Pick one and stick with it.

You use #{task[:reports][:repository][:uri]}/#{task[:report_id]} a couple times in your strings, and it's long. Why not declare a variable?

repository_uri = "#{task[:reports][:repository][:uri]}/#{task[:report_id]}"

repository_uri might not be the best name; I'd recommend picking whatever works best.

Your heredocs are weird. What I normally see is something like this:

      puts <<-MESSAGE.gsub(/^[ \t]*> /, '')
        > Stuff!
        > More stuff!
        > #{pound_sign}Octothorpe
      MESSAGE

Note the two differences between that syntax and yours:

Instead of << at the beginning, I have <<-. This lets you have whitespace (i.e. indentation) immediately before the closing tag, so it's in the same indent level as the rest of the code.
Immediately after the beginning tag, I have .gsub(/\s*> /, ''). This operates exactly like a normal .gsub call on the contents of the heredoc, after string interpolation, and you can feel free to change the regex in front to whatever suits you. It's there to let you specify where the line starts; in effect, it allows you to indent the contents and have indenting in the string itself. Note that this specific regex requires a space after the > or the entire line is there, unchanged. Also, it uses [ \t] rather than \s because \s will also match newlines, and it's probably better to allow multiple lines.

The benefit of this approach is that you don't have to get rid of all indents for however many lines; the drawback is that it's ever so slightly slower, because it has to perform a gsub on the end result.

Now, you build your hashes oddly. Rather than creating an empty one, then putting values in it, why not build it with the values already in it? For example, this:

convert_context = {}
convert_context[:type] = 'geo'
convert_context[:output] = "#{task[:workspace]}/#{task[:report_id]}.json"

becomes this:

convert_context = {
  type:   'geo'
  output: "#{task[:workspace]}/#{task[:report_id]}.json"
}

If you store "#{task[:workspace]}/#{task[:report_id]}.json" as a variable (let's call it workspace_url; you'll want a better name) then you can use this super cool syntax for calling methods with hashes:

@converter.convert(
    type: 'geo',
    output: workspace_url,
)

You could, of course, put the string in directly in both places where you reference it, if you want to. It's your choice.

Note that with heredocs, you have to put the comma on the first line for it to register; otherwise, it doesn't see the end of the doc . This is due to a mildly annoying quirk in how heredocs work.

If I'm using this class and see that the parameter is optional, and I don't know what it does, I'm gonna leave it out, then when it starts putting nothing in the URLs (or throwing NoMethodError: Undefined method `[]' for nil:NilClass) I'm gonna be really confused, since it should work with the default argument, instead of throwing errors!

Of course, someone could easily supply a blank Hash themselves. It's much safer to create a basic ReportableTask (because Task is really generic and a bad name for a class) class as a data wrapper; something like this is all you need:

class ReportableTask
  def initialize(workspace, reports, report_id)
    @workspace = workspace
    @report_id = report_id
    @reports = reports
  end

  attr_accessor :workspace, :report_id, :reports
end

You could even put in some convenience methods, like repository_uri that returns @reports[:repository][:uri].

Then, if you insist on being able to use your code totally unchanged, you can add a method:

def [](name)
    instance_variable_get("@#{name}")
end

but I'd recommend just changing your code.

Then you could make similar objects for reports -- maybe a Report class? -- and maybe lots of other things. Without seeing the structures you have in mind I can't tell you for sure what'd be best or even give advice on providing that, but in general if you're making lots of hashes with all the same keys it's good to create a class, especially if the usage of those hashes depends on them always having those keys.

Anyway, aside from those tips, it's pretty hard to give a more complete/accurate review without seeing examples of the data you're passing in.

With the changes I suggested (except the one about adding a new class), here's what your code looks like:

class PublishReportProcess
  def initialize(converter, uploader, emailer)
    @converter = converter
    @uploader = uploader
    @emailer = emailer
  end

  def run(task)
    repository_uri = "#{task[:reports][:repository][:uri]}/#{task[:report_id]}"
    workspace_uri = "#{task[:workspace]}/#{task[:report_id]}.json"

    @converter.convert(
        type:   'geo',
        output: "#{task[:workspace]}/#{task[:report_id]}.json",
    )

    @uploader.upload(
        local:   workspace_uri,
        remote:  repository_uri,
        staging: "#{task[:reports][:workspace][:uri]}/#{task[:report_id]}",
    )

    @emailer.emailer(
        message: <<-MESSAGE.gsub(/\s*> /, '')
          > There is a new report available at #{repository_uri}.
          >
          > Share and enjoy
        MESSAGE
    )
  end
end

Note: I didn't add ReportableTask because I don't know how you want to implement it, or if your code structure makes it better suited for another place. If you'd like, I can provide a sample implementation of that as well.


Cross-site request forgery defense for code to count Likes I have the following code which is supposed to insert a row into a DB table "clicks" (consisting of 1 Primary AI column "id" and another column "user" which contains the user's session ID) upon clicking the Like button. For each user assuming they have a session ID set from a login I would like to return to them their most recently inserted ID from the table. So the first time the button is clicked it will return 1 etc. I would like this to be accessible to multiple users through a login system.

Is this vulnerable to Cross-site request forgery and if so, how can I alter the code to defend against it?

index.php:

<?php
include 'init.php';
include 'connect.php';
?>
<!doctype html>

    <?php
    $userid = $_SESSION['user_id'];
   echo 'Like';
    ?>

connect.php

<?php
$servername = "localhost";
$username = "root";
$password = "";
$dbname = "DB";
$conn = new mysqli($servername, $username, $password, $dbname);

if ($conn->connect_error) {
die("Connection failed: " . $conn->connect_error);
}
?>

init.php

<?php
session_start();
$_SESSION['user_id']='1';
$userid = $_SESSION['user_id'];
include 'connect.php';
include 'like.php';
?>

like.js

function like_add(userid) {
$.post('like_add.php', {userid:userid}, function(data) {
   if (data == 'success'){
    add_like($userid);
   } else{
       alert(data);
   }
});
}

like.php

<?php
function add_like($userid){
include 'connect.php';

$stmt = $conn->prepare("INSERT INTO clicks (user) VALUES (?)");
$stmt->bind_param("s", $userid);

$stmt->execute();
$stmt = $conn->prepare("SELECT max(id) FROM clicks WHERE user=?");
$stmt->bind_param("s", $userid);
$stmt->execute();
$stmt->bind_result($click);
$stmt->fetch();
echo $click;
$stmt->close();
}
?>

like_add.php

<?php
include 'init.php';
if (isset($userid)) {
$userid = $userid;
add_like($userid);
}
?>

Defending agains CSRF offens happens by creating a token that can only be used once by the client that requested it. For proper protection, that token should ofcourse be linked to a form.

A simple one could be to add a md5 hash or similar to all your forms, store that hash somewhere. And then when the user submits, check or the hash is valid. Depending on the requirements, you can also add a Time to live to the hash (e.g. did it take more then 30minutes to submit the form?)

In your case, it is not a form, but a request being send to the server. Simply adding a token to that request (and validating it) would help you protect agains CSRF atacks.


Is there a way to refactor this code? I'm new to ruby on rails, on this project I'm using Ruby 2.0 and Ruby on Rails 3.0.

I would like to know if this piece of code can be refactored, as it is

unless params["ot_code"].nil?
  ots = params["ot_code"].gsub(/\r\n?/, "").gsub(";","','").upcase
  ots[ots.length,1] = "'"
  ots =  ots.rjust(ots.length+1,"'")
end

unless params["circuit_id_multiple"].nil?
   multiple_circuit = params["circuit_id_multiple"].gsub(/\r\n?/, "").gsub(";","','")
   multiple_circuit[multiple_circuit.length,1] = "'"
   multiple_circuit = multiple_circuit.rjust(multiple_circuit.length+1,"'")
end

unless params["multiple_element_code"].nil?
   multiple_element_code = params["multiple_element_code"].gsub(/\r\n?/, "").gsub(";","','")
   multiple_element_code[multiple_element_code.length,1] = "'"
   multiple_element_code = multiple_element_code.rjust(multiple_element_code.length+1,"'")
end

I'm a little worried about your gsubs, but assuming this is what you want to do:

def replace_and_wrap(str)
  return nil if str.nil?
  %Q{'#{str.gsub(/\r\n?/, "").gsub(";","','")}'}
end

ots = replace_and_wrap(params["ot_code"])
multiple_circuit = replace_and_wrap(params["circuit_id_multiple"])
multiple_element_code = replace_and_wrap(params["multiple_element_code"])

Note that this changes the behavior of your code slightly: variables (like ots) are set to nil if the param is nil, rather than remaining undefined. Depending on how you are using these variables, this behavior is probably better anyway.


Lockless Queue Multiple-Reader Singler-Writer in C++ I wrote a lockless queue for sending small objects from a single thread to a random worker thread.

Assumptions:

x86/x86_64 compiled with GCC
one thread may Write(), multiple threads may Read()
notifying a thread that data is available is done elsewhere
T is copy assignable and sizeof(T) is small
N is a power of two

Any suggestions welcome.

//RnW1FifoFixed.h
#pragma once
#include <memory>

//multiple reader single writer first in first out fixed length ring buffer queue
//compatible with x86/x86_64 GCC
template<typename T,uint32_t N> class RnW1FifoFixed
{
private:
  const uint32_t MASK = 2*N-1;
public:
  RnW1FifoFixed()
    :m_array(new T[N]),m_read(0),m_write(0)
  {
    static_assert(std::is_default_constructible<T>::value,"T does not have a default constructor.");
    static_assert(std::is_copy_assignable<T>::value,"T does not support copy assignment.");
    static_assert(N!=0,"N is too small.");
    static_assert(N!=0x80000000,"N is too large.");
    static_assert((N&(N-1))==0,"N is not a power of two.");
  }
  //one thread
  bool Write(T t)
  {
    //full
    if(m_write-m_read==N)
      return false;
    m_array[m_write&MASK] = t;
    //CPU does not reorder writes
    //prevent compiler from reordering writes
    asm volatile("":::"memory");
    m_write++;
    return true;
  }
  //multiple threads
  bool Read(T& t)
  {
    while(true)
    {
      //use a constant m_read each loop
      uint32_t read = m_read;
      //empty
      if(read==m_write)
        return false;
      t = m_array[read&MASK];
      if(__sync_bool_compare_and_swap(&m_read,read,read+1))
        return true;
    }
  }
private:
  std::unique_ptr<T[]> m_array;
  uint32_t m_read;
  uint32_t m_write;
};

This line seems really strange to me:

static_assert(N!=0x80000000,"N is too large.");

Technically, it is rather odd to just compare for equality with one big number where there could be numbers even bigger. Didn't you mean:

static_assert(N >= 0x80000000, "N is too large.");

const uint32_t MASK = 2*N-1;

Since all the values in this line are known at compile time and MASK is apparently not meant to be changed, you should consider making it both static and constexpr:

static constexpr uin32_t MASK = 2*N-1;

That's kind of trivial, but you can also use curly braces instead of parenthesis in your constructor initialization list:

RnW1FifoFixed():
    m_array{new T[N]},
    m_read{0},
    m_write{0}
{
    // ...
}