Preventing email injection - Part 2 A day ago I have asked a question on here about Preventing email injection. I had some feedback and worked on it, and below is the latest update.

Could anyone please share their opinion?  Is it secure enough?  Is there a way to short-cut the code?

<?php
   session_start();

   if ($_SERVER['REQUEST_METHOD'] == 'POST'){
      ob_start();

      if(isset(
         $_REQUEST['name'],
         $_REQUEST['email'],
         $_REQUEST['message'],
         $_REQUEST['number'],
         $_REQUEST['date'],
         $_REQUEST['select'],
         $_REQUEST['radio'],
         $_REQUEST['checkbox'],
         $_REQUEST['token']
      )){
      if($_SESSION['token'] != $_POST['token']){
         $response = "0";
      }else{
         $_SESSION['token'] = "";
         $name = $_REQUEST['name'];
         $email = $_REQUEST['email'];
         $message = $_REQUEST['message'];
         $number = $_REQUEST['number'];
         $date = $_REQUEST['date'];
         $select = $_REQUEST['select'];
         $radio = $_REQUEST['radio'];
         $checkbox = $_REQUEST['checkbox'];

         $spam_pattern = "/[\r\n]|Content-Type:|Bcc:|Cc:/i";
            switch (true){
            case !filter_var($email, FILTER_VALIDATE_EMAIL):
                  $response = "Invalid Email Address!";
            break;
                case !preg_match($spam_pattern, $name):
                case !preg_match($spam_pattern, $number):
                case !preg_match($spam_pattern, $date):
                case !preg_match($spam_pattern, $select):
                case !preg_match($spam_pattern, $radio):
                case !preg_match($spam_pattern, $checkbox):
                case !preg_match($spam_pattern, $message):
                  $response = "Invalid Request!";
            break;
                default:
                    $to = "";
                    $subject = "New Message From: $name";
                    $message = "Name: $name

                                   number: $number

                                   date: $date

                                   select: $select

                                   radio: $radio

                                   checkbox: $checkbox

                                   Email: $email

                                   Message: $message";

                    $headers  = 'MIME-Version: 1.0' . "\r\n";
                    $headers .= 'Content-type: text/html; charset=utf-8' . "\r\n";
                    $headers .= 'From: '.$email . "\r\n";

                    if(mail($to, $subject, $message, $headers)){
                        $response = "
Success! Your submission has been sent.
";
                    }else{
                        $response = "
Error! There was a problem with sending.
";
                    }
            break;
               }
            }
         }
         else {
            $response = "Error";
         }
      ob_flush();
   }
?>

Due to the nested and long conditions the code becomes hard to follow
at some points. Early returns prevent nesting of conditions.
Some code is duplicate (step 9)
Possibly some values could be overwritten by url-forging (depending on the server-configuration). (step 5)

I'll do a step-by-step refactoring here with some explanations.

Move everything in a function. This eases early-return.

function sendMail() {
    // your code here.
}

While not necessary, it's I like to invert the conditions so the static expression stands on the left side. This avoids accidental assignment in conditions. Avoids tricky to detect bugs (though most IDEs and codesniffer emit a warning).
Using the === operator for comparison avoids bugs to unforeseen casting

function sendMail() {
    if ('POST' !== $_SEVER['REQUEST_METHOD']) {
        return '';
    }

    if ($_SESSION['token'] !== $_POST['token']) {
        return '0';
    }
    // remaining code
}

What's the purpose of your output-cachting? I can't see any outputs being made? Trying to catch any errors? Hiding them is a bad idea, catch and handle them. If simply removed them.
You are using $_REQUEST instead of $_POST. Sure you do check if the request type. But this doesn't prevent anyone from submitting both, GET and POST values. And here lies a dangerous problem: If I send you a link to form.php?message=something this does get send to the server as well when you submit the form. Now GET and POST both having the key name collide. Depending on the variables_order the GET value I set overwrites your message. If you use $_POST to access your variables instead this issue can be prevented.
I have extracted a simple accessor function to check for existence of a $_POST variable and return else:

function getPostValue($key) {
    return isset($_POST[$key]) ? $_POST[$key] : null;
}

function sendMail() {
    if ('POST' !== $_SEVER['REQUEST_METHOD']) {
        return '';
    }

    if ($_SESSION['token'] !== $_POST['token']) {
        return '0';
    }

    $_SESSION['token'] = '';

    $name = getPostValue('name');
    $email = getPostValue('email');
    $message = getPostValue('message');
    $number = getPostValue('number');
    $date = getPostValue('date');
    $select = getPostValue('select');
    $radio = getPostValue('radio');
    $checkbox = getPostValue('checkbox');

    if (!isset($name, $email, $message, $number, $date, $select, $radio, $checkbox)) {
        $response = "Error";
    }
}

All those input validations go with early return again (snippet only):

    $spam_pattern = "/[\r\n]|Content-Type:|Bcc:|Cc:/i";
    if (!preg_match($spam_pattern, $name)
        || !preg_match($spam_pattern, $number)
        || !preg_match($spam_pattern, $date)
        || !preg_match($spam_pattern, $radio)
        || !preg_match($spam_pattern, $checkbox)
        || !preg_match($spam_pattern, $select)
        || !preg_match($spam_pattern, $message)) {
        return 'Invalid Request!';
    }

The $to variables and others you use as parameters for mail are only used once. Furthermore the $message variable is overwritten, removing the original content. Variable reuse makes stuff hard to track though. I just do this all as arguments in the mail function (snippet):

    $sendResult = mail(
        'support@loaidesign.co.uk',
        'New Message From: ' . $name,
        "Name:

           number: $number

           date: $date

           select: $select

           radio: $radio

           checkbox: $checkbox

           Email: $email

           Message: $message",
        "MIME-Version: 1.0 r\n Content-type: text/html; charset=utf-8 \r\n From: '.$email . '\r\n"
    );

All those returns have their html-markup in common. We sure can extract this into a common helper:

function getResponse($message, $isError) {
    return '' . $message . '';
}

Final remarks & further points of improvement:

Indention of three characters is unusual. Most times its two or four.
This can of course be further refactored: Especially creating local variables from $_POST is a point of repetation. Maybe it could be handled as an array. That'd make mass-checking (preg_match) easier: just loop over all input variables.
If desired, this could of course be moved into a dedicated mailer class
The functions depend on the global state ($_POST). A next step would be to pass the required input variables as argument (maybe an array :)). This would make this function reusable as well.
Returning formated strings limits reusability to HTML output. Throwing expections or error codes puts this into the responsibility of the caller, not the callee.

My final result:

function getPostValue($key) {
    return isset($_POST[$key]) ? $_POST[$key] : null;
}

function getResponse($message, $isError) {
    return '' . $message . '';
}

function sendMail() {
    if ('POST' !== $_SEVER['REQUEST_METHOD']) {
        return '';
    }

    if ($_SESSION['token'] !== $_POST['token']) {
        return '0';
    }

    $_SESSION['token'] = '';

    $name = getPostValue('name');
    $email = getPostValue('email');
    $message = getPostValue('message');
    $number = getPostValue('number');
    $date = getPostValue('date');
    $select = getPostValue('select');
    $radio = getPostValue('radio');
    $checkbox = getPostValue('checkbox');

    if (!isset($name, $email, $message, $number, $date, $select, $radio, $checkbox)) {
        $response = getResponse('Error', true);
    }

    if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
        return getResponse('Invalid Email Address!', true);
    }

    $spam_pattern = "/[\r\n]|Content-Type:|Bcc:|Cc:/i";
    if (!preg_match($spam_pattern, $name)
        || !preg_match($spam_pattern, $number)
        || !preg_match($spam_pattern, $date)
        || !preg_match($spam_pattern, $radio)
        || !preg_match($spam_pattern, $checkbox)
        || !preg_match($spam_pattern, $select)
        || !preg_match($spam_pattern, $message)) {
        return getResponse('Invalid Request!', true);
    }

    $sendResult = mail(
        'support@loaidesign.co.uk',
        'New Message From: ' . $name,
        "Name:

           number: $number

           date: $date

           select: $select

           radio: $radio

           checkbox: $checkbox

           Email: $email

           Message: $message",
        "MIME-Version: 1.0 r\n Content-type: text/html; charset=utf-8 \r\n From: '.$email . '\r\n"
    );

    if (true === $sendResult) {
        return getResponse('Success! Your submission has been sent.', false);
    } else {
        return getResponse('Error! There was a problem with sending.', true);
    }
}


Checking number of words in a string and assigning them to a value I am learning Java and trying to write a program to check the number of words in a string and am trying append them to create username and email id. If the number of words in the name are 3 then the first name is the 1st word and last name is the 3rd word, where as when the number of works in the name are 2 the First name is the 1st word and the Last name is the 2nd word. To differentiate this I've used the IF condition. I just wanted to find out if the code is good enough or if there's a better way of doing it. Code is as below :

import java.io.*;

public class CreateUsername {
    public static void main(String[] args) throws IOException{

        BufferedReader br = new BufferedReader(new InputStreamReader(System.in));

        System.out.println("Provide Name");
        String Username = br.readLine();

        char[] arr = Username.toCharArray();
        int i = 0;
        boolean notCounted = true;
        int counter = 0;
        while (i < arr.length)
        {
            if (arr[i] != ' ')
            {
                if (notCounted)
                {
                    notCounted = false;
                    counter++;
                }
            }
            else
            {
                notCounted = true;
            }
            i++;
        }
        System.out.println("words in the string are : " + counter);

        //Split the names and assign the Windows Username
        String FirstName = "";
        String firstLetterSurname = "";
        String Surname = "";
        String WindowsUsername = "";
        String EmailID = "";
        if(counter == 2)
        {
            System.out.println("Inside for count 2");
            FirstName = Username.split(" ")[0];
            Surname = Username.split(" ")[1];
            firstLetterSurname = String.valueOf(Surname.charAt(0));
            WindowsUsername = FirstName + "" + firstLetterSurname.toUpperCase();
            EmailID = FirstName + "." + Surname + "@test.com";
        }
        if(counter == 3)
        {
            FirstName = Username.split(" ")[0];
            Surname = Username.split(" ")[2];
            WindowsUsername = FirstName + "" + firstLetterSurname.toUpperCase() ;
            EmailID = FirstName + "." + Surname + "@test.com";
        }
        System.out.println("Windows Username is : " + WindowsUsername);
        System.out.println("Email ID is : " + EmailID);
    }
}

The code is pretty clean, if you started coding very recently, you can be quite happy with it.

But please note that some of your variables names start with an uppercase letter, as a reminder java use SCREAMING_SNAKE_CASE for static final variables, CamelCase for class name (or object type if you prefer), and camelCase (aka lowerCamelCase) for local variables, fields and method name.

Also if the user input only one word then your code fails (which is not necessarily bad nor wrong)... silently (which is bad).

You should print a warning message in such case.

Step by step review

public class CreateUserName {

CreateUserName sounds like a complete sentence which you should avoid for a class name. Consider renaming your class to UserNameCreator, UserNameParser, etc...

BufferedReader br = new BufferedReader(new InputStreamReader(System.in));

That's quite complicated just to read something from the console consider using Scanner for such a simple use case

char[] arr = Username.toCharArray();
int i = 0;
boolean notCounted = true;
int counter = 0;
while (i < arr.length)
{
    if (arr[i] != ' ')
    {
        if (notCounted)
        {
            notCounted = false;
            counter++;
        }
    }
    else
    {
        notCounted = true;
    }
    i++;
}

First and foremost, you should put that into its own function.
Secondly, good job in considering the case where multiple space are following one another :) sadly, if you indeed have multiple space it'll fail later in your code :( maybe you shouldn't only consider space but also tab ? (char for tab is '\t')

Thirdly, you didn't have to use an array, there is the method charAt inside String.

String FirstName = "";
String firstLetterSurname = "";
String Surname = "";
String WindowsUsername = "";
String EmailID = "";

Copying this part just to remind you to use camelCase ;)

if(counter == 2)
{
    //...

Use if / else if and consider adding a else to print a warning message if the input of the user is invalid. Never trust user inputs ;)

As said earlier, if the user input multiple space in a rows, this part may fail as you'll use charAt(0) on the empty string.

Also, firstLetterSurname is never written to if counter is 3 which is likely a bug.

Lastly, you shouldn't split the variable twice. Use split once and store the result in a temporary variable like this :

String[] splitted = userName.split(" ");
FirstName = splitted[0];
Surname = splitted[1];

Special note

Remember that people's names aren't as easy to manipulate as they seem : some people have space in their first name, others have multiple "middle name" (in France for example, we can have as many surname as wanted, though we use only one).

Also some family name may not be case sensitive (Victor D'Hondt is sometime written Victor d'Hondt or even Victor D'hondt). Something to consider if you ever want to add name comparison.


Let's play Find that File Isn't it annoying when you can't find a file you recently created? Especially when you either have a very disorganized drive or a directory with lots of files.

Even if you don't care, isn't it nice to have a program be able to locate a file for you?

I did this because I have files that I can't seem to locate, but I know roughly where it is. The FileFinder class can:

Find your file
Copy it to somewhere else

The only feature-lack:

Files with the same name in a different directory are not handled well.

Only one of the files will be returned.

I will add that feature once I can think of a way to implement it.

import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.StandardCopyOption;
import java.util.HashMap;
import java.util.Map;
import java.util.regex.Pattern;

public class FileFinder {

    protected final File root;
    protected Map<String, File> files;

    protected final Pattern matchingRegex;

    public FileFinder(File root, String matchingRegex) {
        if (root.isFile()) {
            throw new IllegalArgumentException("root must be a directory.");
        }
        this.root = root;
        this.files = getAllFiles(root);
        this.matchingRegex = Pattern.compile(matchingRegex);
    }

    protected Map<String, File> getAllFiles(File root) {
        File[] files = root.listFiles();
        Map<String, File> result = new HashMap<>();
        for (File file : files) {
            if (file.isDirectory()) {
                result.put(file.getName(), file);
                result.putAll(getAllFiles(file));
            } else if (matchingRegex.matcher(file.getName()).matches()) {
                result.put(file.getName(), file);
            }
        }
        return result;
    }

    public File getRoot() {
        return root;
    }

    public File find(String fileName) throws FileNotFoundException {
        File result = files.get(fileName);
        if (result == null) {
            throw new FileNotFoundException("Cannot find file " + fileName
                    + " in root.");
        }
        return result;
    }

    public boolean copy(File file, File dest) {
        if (dest.isFile()) {
            throw new IllegalArgumentException(
                    "The destination must be a directory.");
        }
        if (!(file.exists() && dest.exists())) {
            throw new IllegalArgumentException(
                    "One or more of the files do not exist.");
        }
        try {
            Files.copy(file.toPath(), new File(dest, file.getName()).toPath(),
                    StandardCopyOption.REPLACE_EXISTING,
                    StandardCopyOption.COPY_ATTRIBUTES);
        } catch (IOException e) {
            return false;
        }
        return true;
    }

    public void findAndCopy(File dest, String fileName)
            throws FileNotFoundException {
        copy(find(fileName), dest);
    }

}

Concerns:

Is this OOP enough?
Anything in the java.nio package that makes this easier?
Anything else?

Bug

In the constructor, matchingRegex must be initialized before calling getAllFiles(root);, otherwise a NullPointerException is thrown if the root folder contains subfolders.

Design

Constructor

I agree with @holroy about the fact that there are too many things done in the constructor. Its job is too heavy, instead of just initialize some things. Moreover, it makes the FileFinder instance unusable for other parameters, e.g. to search in another folder or with another regex, one will need to instantiate a new FileFinder.

getAllFiles(File)

It should be named getAllMatchingFiles, because in the current implementation it returns only the files with names matching the regex and all the directories. By the way, it is not very clear why the directories are put there.

The implementation of this method is correct in general, but is coded entirely with before-Java-7 style. Indeed, java.nio would be a lot of help here.

First of all, there is FileVisitor interface. Using a SimpleFileVisitor prevents us from pre-calculating Map<String, File> files. It also solves the problem of files with same names in different folders:

public Collection<Path> findFiles(String fileName) throws IOException {
  final Collection<Path> foundFiles = new ArrayList<>();
  Files.walkFileTree(this.root.toPath(),
                     new SimpleFileVisitor<Path>() {
      @Override
      public FileVisitResult visitFile(Path file, BasicFileAttributes attrs)
                throws IOException {
        if (file.getFileName().toString().equals(fileName)) {
            foundFiles.add(file);
        }
        return FileVisitResult.CONTINUE;
      }
    });
  return foundFiles;
}

This implementation will return all the files having fileName. If you want to get them by regex, it's achieved with a little change in the if condition.

If we push a bit deeper and try to benefit from Java 8, it becomes really concise:

public Collection<Path> findFiles8(String fileName) throws IOException {
  return Files.find(this.root.toPath(),
                    Integer.MAX_VALUE, // NB! should be more reasonable :)
                    (file, attrs) -> {
         return Files.isRegularFile(file)
           && file.getFileName().toString().equals(fileName);
    }).collect(Collectors.toList());
}

In fact, Files.find(args) can do almost all the job of FileFinder...

copy(args)

I think that this method should be defined elsewhere, because it does not correspond to the functionality of FileFinder.

It's a good idea to use Files.copy() inside it (and even better would be instead of it). Since this is a public method, the validation of the arguments should be done on non-nullability of file and dest. The existence of the files will be checked in Files.copy(), with the respective exceptions thrown.

The target Path object can be created easier with dest.toPath().resolve(file.getName()), no need to instantiate a File.


PHP function to obtain model_name from Namespace\To\ModelName Would there be a more clear/efficient way to write

public function getModelName()
{
    return substr(strtolower(substr(strrchr(preg_replace('/(?<!\ )[A-Z]/', '_$0', __CLASS__), "\\"), 1)), 1);
}

for example:

__CLASS__: Namespace\To\SettingOption

Output: setting_option

You tagged laravel, so this answer assumes you are using Laravel.

Laravel has two built in helper functions that will help you do this. class_basename() will return the name of the class with the namespace stripped off, and snake_case() will convert a string to snake case.

Combine them, and you get:

public function getModelName()
{
    return snake_case(class_basename(__CLASS__));
}


Process bytes and check for start key I am working with some wireless communication in Arduino. My data will be wirelessly received like so:

97
97
97
...
...

979797 is the address of the transmitter. I have written the following code to check the address first before processing the data. My main concern is speed of computation.

void printBuffer(const uint8_t *serialBuffer)
{
  //print contents of buffer
  for (int i = 0; i<100; i++)
    Serial.println(serialBuffer[i]); //remove the print in binary option to decode
}

void processBytes(const uint8_t value)
{
  static uint8_t serialBuffer[bufferSize]; //buffer to hold values
  static unsigned int startPosition=0;

  if(startPosition < bufferSize)
  {
    serialBuffer[startPosition] = value;
    startPosition++;
  }

  else
  {
    printBuffer(serialBuffer);
    startPosition = 0;
  }

}//end of processBytes
void loop() {

  //check if data is available in serial buffer
  while(RX.available())
  {
    //check for start key
    address(RX.read())
    //process data
    processBytes(RX.read());
  }
}
void address(const uint8_t value)
{
  int counter = 0;
  int newVal = value;
  while(value == 97 && counter < 3)
  {
    counter++;
    newVal = RX.read();
  }
}

EDIT:
The data packet just has the following components: start key (979797) and 68 bytes of data. There is no termination key. Also, the transmitter transmits one byte at a time. Thus, each value received is less than 256. Hence why I am saving the data into a uint8_t[] array.

Other than the address() method, most of it was borrowed from Gammon. Here is the reference page for the arduino commands I am using. Are there ways I can optimize this computation?

Bugs

There are several things wrong with this function:

void address(consst uint8_t value)
{
  int counter = 0;
  int newVal = value;
  while(value == 97 && counter < 3)
  {
    counter++;
    newVal = RX.read();
  }
}

You never use the variable newVal, which is a mistake.  Most likely you meant the while loop to read:

while (newVal == 97 && counter < 3)

As it stands, your code will always read 3 extra bytes if the first is 97, even if the second byte is not 97.  You can also get rid of the newVal variable and just use value, if you change the type of value to make it non-const.
const is misspelled.
When you encounter a series of three 97 bytes, you actually read 4 bytes.  The first byte is already read and passed into this function.  The function then reads 3 extra bytes.  This means that the first byte of the "message" will be missing when you call processBytes() later on.
Your message protocol is unclear.  From your code, it appears that the message protocol must be 97 97 97 byte 97 97 97 byte which is extremely wasteful.  If that is not the protocol, there are problems with how you handle the message stream.  For example, the address() function strips all 97s out of the message stream, whether or not they are part of a sequence of three.  Also, if the address() function does not detect a sequence of three 97s, it always discards one byte out of the message stream.  I'd need to see an example of a proper message stream in order to advise on a solution.


Code for saving / loading a class object As I was advised, I broke my code into 2 classes. Could you take a look at the repository class and report what problems this code has

public class JsonSharedPreferences {
private static Gson gson = new Gson();
private Context context;

public JsonSharedPreferences(Context context) {
    this.context = context.getApplicationContext();
}

public <T> T loadObject(Class<T> classType, String key) {
    SharedPreferences settings = context.getSharedPreferences(key, Context.MODE_PRIVATE);
    String json = settings.getString(key, null);
    return gson.fromJson(json, classType);
}

    public <T> void saveObject(T objective, String key) {
        SharedPreferences settings = context.getSharedPreferences(key, Context.MODE_PRIVATE);
        String json = gson.toJson(objective);
        settings.edit().putString(key, json).apply();
    }
}

Why saving reference to Context and then create SharedPreferences each time? There's duplicate code for creating SharedPreferences You could just use context in constructor to create member SharedPreferences to use in your methods. If you want to keep it as it is, at least extract duplicate code into separate method.
I am missing null checks. I expect to get very obscure behaviour when passing null for keys or classes. Test for null and throw NullPointerException early, be fail-fast. For example checkNotNull or some NotNull annotation to help better fighting against null. Another way to fight is to use kotlin, which doesn't allow null on compiler level unless you make variable optional.
I don't like that gson is private and static.

That means there is no way to configure gson. Would be better if this was member property and I would be able to pass different configuration of gson. Can add consructor overloads for that. Maybe this is overdoing it for your case, but I usually configure Gson quite a lot, have my own TypeAdapters and I wouldn't be able to use my Gson configuration with your class.
At first I was concerned about thread-safety Gson is thread-safe, so there's no argument against static in this sense. Always check for that when you make something static like this.

Pretty sure that when saving preferences in android, you first put key and then value. Your saveObject has it the other way around, that is imho confusing. I'd switch parameters in loadObject too so that key is always first in those cases, but guess that's a bit subjective.
Immutability makes code simpler to use and read. If something is final, you don't need to keep be alert if it changed anytime. context member variable can be final and all method parameters can be final too. After refactoring possible SharedPreferences and Gson instances can also be final - set in constructor.


Kotlin reflection to generate one class from class similar with similar properties This code is intended to generate a class (domain) from a class with class with similar/identical properties (api). Motivation is to speed development time when using clean architecture.

import org.joda.time.DateTime
import org.joda.time.DateTimeZone
import javax.inject.Inject
import kotlin.reflect.KClass
import kotlin.reflect.KParameter
import kotlin.reflect.full.declaredMemberProperties
import kotlin.reflect.full.findAnnotation

class DomainApiReflectionAdapter<DOMAIN : Any, API : Any> constructor(
        val clazz: KClass<DOMAIN>
) {

    fun fromApi(apiModel: API): DOMAIN {
        return apiToDomain(apiModel, clazz)
    }

    inline fun <API : Any, DOMAIN : Any> apiToDomain(apiModel: API, clazz: KClass<DOMAIN>): DOMAIN {
        val factory = Factory(clazz)
        val apiProperties = toPropMap(apiModel)
        val constructorMap = mutableMapOf<KParameter, Any?>()
        for (param in factory.params) {
            param.findAnnotation<AdapterValues>()?.let {
                val iclass = param.type.classifier as KClass<*>
                println(iclass)
                constructorMap[param] = innerFactory.simpleConstruct(apiProperties)
            } ?: {
                if (param.name in apiProperties) {
                    constructorMap[param] = when {
                        param.type.classifier == List::class -> {
                            val itype = param.type.arguments[0].type!!.classifier as KClass<*>
                            (apiProperties[param.name] as List<*>).map {
                                it?.toDomain(itype)
                            }
                        }
                        param.type.classifier == DateTime::class -> DateTime(apiProperties[param.name] as Long, DateTimeZone.forID("UTC"))
                        else -> apiProperties[param.name]
                    }
                }
            }()
        }

        return factory.construct(constructorMap)
    }

    fun <K : Any> toPropMap(model: K): Map<String, Any?> {
        return model.javaClass.kotlin.declaredMemberProperties
                .map { prop ->
                    prop.name to prop.get(model)
                }.toMap()
    }

    companion object {
        inline fun <reified DOMAIN : Any, API : Any> instance(): DomainApiReflectionAdapter<DOMAIN, API> {
            return DomainApiReflectionAdapter(DOMAIN::class)
        }
    }

    inner class Factory<DOMAIN : Any>(clazz: KClass<DOMAIN>) {
        private val constructor = clazz.constructors.first()
        val params = constructor.parameters

        fun construct(map: Map<KParameter, Any?>): DOMAIN {
            return constructor.callBy(map)
        }

        fun simpleConstruct(apiProperties: Map<String, Any?>): DOMAIN {
            val innerMap = mutableMapOf<KParameter, Any?>()
            for (ip in params) {
                innerMap[ip] = apiProperties[ip.name]
            }
            return construct(innerMap)
        }
    }
}

fun <DOMAIN : Any, API : Any> API.toDomain(clazz: KClass<DOMAIN>): DOMAIN {
    val adapter = DomainApiReflectionAdapter<DOMAIN, API>(clazz)
    return adapter.fromApi(this)
}

abstract class Adapter<A, B> : (B) -> A

@Target(AnnotationTarget.VALUE_PARAMETER)
annotation class AdapterClass()

@Target(AnnotationTarget.VALUE_PARAMETER)
annotation class AdapterValues(val parameters: Array<String>)

inline functions

inline function are great when:

You want to surround a lambda with a little code
You want to use reified types.

They are not great when you have a large function, as every call to that function will be replaced with the body of the function.

Therefor, inlining apiToDomain seams the wrong choice.
You have another function: API.toDomain.

associate

I found the following in your code:
    map{ it.a to it.b }.toMap()

This can replaced by:

associate{ it.a to it.b }

(it can be used if you have one parameter)

associateWith

In kotlin 1.3, you can use associateWith for simpleConstruct:

val map = listOf(1,2,3).associateWith{ it.toString }
println(map) // [1:"1", 2:"2", 3:"3"]

when

your when looks like

when {
    param.type.classifier == List::class ->
    param.type.classifier == DateTime::class ->
    else ->
}

this can be simplified to:

when(param.type.classifier) {
    List::class ->
    DateTime::class ->
    else ->
}

run

in your function, you use {...}(). run{} does exactly the same, but you can't forget to invoke it. This is however a personal choice.

function bodies

I personnaly don't like functions where the body starts with return:

fun fromApi(apiModel: API): DOMAIN {
    return apiToDomain(apiModel, clazz)
}

you can rewrite this as:

fun fromApi(apiModel: API): DOMAIN = apiToDomain(apiModel, clazz)

or even like:

fun fromApi(apiModel: API) = apiToDomain(apiModel, clazz)

typealias

I dont know exactly the purpose of

abstract class Adapter<A, B> : (B) -> A

but maybe you can use a typealias?

typealias Adapter<A, B> = (B) -> A

constructing

I thing you forgot a keyword in front of your constructor? As you wrote it out and you have a companion-object that creates a type of your class...
Ps. If you want to construct in your companion-object, but it should look like a constructor-call, you can use operator fun invoke(...)

Hope this helps a bit


Class that represents starting points and endpoints of a game I am creating my first HTML5 game using JavaScript and Phaser framework. I have a class (or more accurately a prototype) called MajorPoint that represents the starting point and the end point of the game. I initially put those two together, as they only differed from each other by the visible frame. Now as the game is more fleshed out though, the endpoint has got a few more rules that aren't needed by the starting point.

Should I now split this into two? I'm not sure what to do with it, because if I split this into two (StartPoint and EndPoint), those two prototypes would have a lot (relatively speaking) of identical code between each other. On the other hand, now there is almost as much code that is only needed by half of the objects.

The objects will not change their state during the game, so they stay as they are created the whole time they exist.

var MajorPoint = function(game, x, y, state, callback, callbackContext) {
  Phaser.Sprite.call(this, game, x, y, 'start-end', 0);
  this.state = state;
  this.anchor.setTo(0.5, 0.7);
  this.scale.setTo(0.5, 0.5);

  //If state is 'end' but end is not reachable, hides this object.
  //If state is 'end' and end is reachable, enables input for this object.
  if (state === 'end') {
    this.frame = 1;
    if (!game.data.isEndReachable()) {
      this.visible = false;
    } else {
      this.inputEnabled = true;
      if (callback !== undefined) {
        this.events.onInputDown.add(callback, callbackContext);
      }
    }
  }
};

MajorPoint.prototype = Object.create(Phaser.Sprite.prototype);
MajorPoint.prototype.constructor = MajorPoint;

You can create EndPoint as a more specific version of MajorPoint and move the extra logic there.

var MajorPoint = function(game, x, y, state, callback, callbackContext) {
  Phaser.Sprite.call(this, game, x, y, 'start-end', 0);
  this.state = state;
  this.anchor.setTo(0.5, 0.7);
  this.scale.setTo(0.5, 0.5);
};

MajorPoint.prototype = Object.create(Phaser.Sprite.prototype);
MajorPoint.prototype.constructor = MajorPoint;

var EndPoint = function(game, x, y, state, callback, callbackContext) {
  MajorPoint.call(this, game, x, y, 'start-end', 0);

  //If end is not reachable, hides this object.
  //If end is reachable, enables input for this object.
  this.frame = 1;
  if (!game.data.isEndReachable()) {
    this.visible = false;
  } else {
    this.inputEnabled = true;
    if (callback !== undefined) {
      this.events.onInputDown.add(callback, callbackContext);
    }
  }
};

EndPoint.prototype = Object.create(MajorPoint.prototype);
EndPoint.prototype.constructor = EndPoint;

If callback is only valid when it's omitted or a function, you can drop the !== undefined and just test that it's truthy. This will avoid setting an invalid callback when someone accidentally passes null.

if (callback) {
  ...
}


Walking over a N branching tree The aim of this code, written in Python3.6, was to write a function that will non-recursively walk over a N-branching tree structure.

It works by using a list of indices that keep track of visited nodes and by retracing prior steps when a tree branch ends. The function non_recurs visits nodes top down left to right (as far as I can tell).

non-recurs allows the caller to provide a function to the func argument which will be applied to to each node. The end_only argument will determine if the supplied function will be applied to all nodes (end_only=False) or only nodes at the end of branches (end_only=True).

1st I would like to know if this attempt is actually capable of walking over N-branching trees and 2nd I would like to know what you think about my implementation.

NOTE: There are two sections to my code below. The first is to generate N-branching trees and is not the focus of this code review. But is required to generate my demonstrated output

EDIT

I added IndexError to the except of the try block

tree generation code

from itertools import product

# CODE to create test tree's for walking
def assign(tree, idx, value):
    new_idx = idx[:]
    assignment_idx = new_idx.pop()
    ref = tree
    for i in new_idx:
        ref = ref[i]
    ref[assignment_idx] = value
    return tree

def n_branch_tree(height, branches):
    idx = ([i] for i in range(branches))
    tree = list(range(branches))
    count = 0
    node = 0
    while count < height:
        for i in idx:
            # mutates tree
            assign(tree, list(i), list(range(node, node+branches)))
            node += branches
        count += 1
        idx = product(range(branches), repeat=count)
    return tree

tree walk code

# Code to walk over tree

def walk(tree, idx):
    """Return tree node at provided index

    args:
        tree: tree to index
        idx: index of desired node

    returns: node
    """
    for i in idx:
        tree = tree[i]
    return tree

def non_recurs(tree, func=print, branches=2, end_only=True, ):
    """Non-recursively walk n-branching tree

    args:
        tree: n-branching tree
        func: function that takes tree node as first argument
        branches: The number of branches each node has
        end_only: Default is True. When  True will only apply func to
                end nodes. When False will apply func to all Nodes

    """
    branches = branches - 1  # Because index's start at 0
    idx = [0]
    node = None
    while True:
        # print(idx)
        try:
            node = walk(tree, idx)
        except (TypeError, IndexError):
            # Could not find node at index
            try:
                # Walk back up tree until a node has not
                # been fully explored
                while idx[-1] == branches:
                    idx.pop()
            except IndexError:
                # Means all nodes in index have been fully explored
                break

            # Increase index if current index is under node branch limit
            if idx[-1] < branches:
                idx[-1] += 1

            # Apply func to end node
            if end_only and node:
                func(node)
                node = None
        else:
            idx.append(0)

            # Apply func to all nodes
            if not end_only:
                func(node)

if __name__ == '__main__':

        tree = n_branch_tree(height=3, branches=3)

        print(tree)

        value_store = []

        non_recurs(tree, func=value_store.append, branches=3, end_only=True)

        print(value_store)

Outputs

[[[18, 19, 20], [21, 22, 23], [24, 25, 26]], [[27, 28, 29], [30, 31, 32], [33, 34, 35]], [[36, 37, 38], [39, 40, 41], [42, 43, 44]]]
[18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44]

About your solution

Your tree generation code is wrong. It doesn't return a tree, it returns a multidimensional list instead. There is a good, detailed article about a tree data structure: Everything you need to know about tree data structures.

The main idea of tree - the connection of nodes. All begins from the root - the singular node at the first level, that contains references to the all nodes at the next level, which in their turn have references to the next level nodes, so on. Thus, every node should have as minimum as two fields: one for value and one for list of its childs (terminology used in trees).

Edit after the comment [start]:

Actually, the tree generation code returned value is resembling the tree, especially taking into account that the Python lists and numbers are objects. It has root object with 3 childs, each child has three childs too, so on. (when height=3, branches=3). At the last level it has objects with a number value.

Thus, it complies the first tree data structure requirement: the connection of nodes (from my own definition :)). But not all nodes have value - only the last level's nodes do (the leaves in the tree terminology). So, you anyway can't walking through tree, changing or printing all nodes value, because some nodes don't have them:

root[
    2-nd lvl[         4-th lvl with values
            3-rd lvl[18, 19, 20],
                    [21, 22, 23],
                    [24, 25, 26]
            ],
            [
                    [27, 28, 29],
                    [30, 31, 32],
                    [33, 34, 35]
            ],
            [
                    [36, 37, 38],
                    [39, 40, 41],
                    [42, 43, 44]
            ]
    ]

Edit after the comment [end]:

I didn't throughout investigation of your tree walk code, but by glance it uses the the wrong tree idea as well as the tree generation code and therefore, can't work correctly.

My partial solution

I read your requirements and wrote my own solution, that creates tree, walks through it and applies a passed function to each node. It can't process only end nodes though, but this functionality is easy to add.

class Tree(object):

    def __init__(self):
        self.value = None
        self.childs = None

    # Build tree recursively
    # All nodes doesn't have values, just child list
    # Values will be set by the passed function
    def grow_tree(self, height, child_num):
        if height < 2:
            return

        self.childs = []

        for i in range(child_num):
            child = Tree()
            self.childs.append(child)
            child.grow_tree(height - 1, child_num)

    # Walk through tree iteratively
    def walk_tree(self, func):

        all_current_level_nodes = [self]

        while all_current_level_nodes:
            all_next_level_nodes = []

            for node in all_current_level_nodes:
                func(node)
                if node.childs:
                    all_next_level_nodes.extend(node.childs)

            all_current_level_nodes = all_next_level_nodes

### Recursive implementation
###
#   def walk_tree(self, func):
#       func(self)
#
##      if isinstance(self.childs, list):
#       if self.childs:
#           for child in self.childs:
#               child.walk_tree(func)

Testing

### Functions for passing to the "walk_tree" method
def set_values(node):
    node.value = set_values.cnt
    set_values.cnt += 1

def print_values(node):
    print(node.value)

def print_id(node):
    print(id(node))

def square_values(node):
    node.value = node.value ** 2

tree = Tree()

tree.grow_tree(2, 3)

tree.walk_tree(print_id)

# Function is an object. Add the "cnt" field to this object.
# It will work as a static variable, preserve counter between calls.
set_values.cnt = 1
tree.walk_tree(set_values)

tree.walk_tree(print_values)

tree.walk_tree(square_values)

tree.walk_tree(print_values)

Output

# "print_id" function was applied on each node while walk_tree
139632471400688
139632471400800
139632471400856
139632471400912
# "set_values"
# then "print_values" were applied
1
2
3
4
# "square_values"
# then "print_values" were applied
1
4
9
16


Hide and un-hide rows then sort on Worksheet_Activate event We used this VBA code mainly to hide blank rows and unhide non-blank rows. After that, the second code sorts the rows by a defined column value once the worksheet activates. This process takes too much time with this code.

Could anyone help me optimize this code and make it faster? (The worksheet contain an average of 500 rows).

Private Sub Worksheet_Activate()
HideRows
Sortingrisk

End Sub

Sub HideRows()
Dim rRange As Range, rCell As Range
Dim strVal As String

   Set rRange = Worksheets(12).Range("A10:A500")

    For Each rCell In rRange
      strVal = rCell
      rCell.EntireRow.Hidden = strVal = vbNullString
    Next rCell

End Sub

Sub Sortingrisk()

    ActiveWorkbook.Worksheets("Control Implementation Plan").AutoFilter.Sort. _
      SortFields.Clear
   ActiveWorkbook.Worksheets("Control Implementation Plan").AutoFilter.Sort. _
      SortFields.Add Key:=Range("G10:G1000"), SortOn:=xlSortOnValues, Order:= _
      xlDescending, DataOption:=xlSortNormal
   With ActiveWorkbook.Worksheets("Control Implementation Plan").AutoFilter.Sort
     .Header = xlYes
      .MatchCase = False
       .Orientation = xlTopToBottom
       .SortMethod = xlPinYin
       .Apply
    End With
End Sub

There are two things that I think can easily be done.

Find out where the data actually ends rather than always using row 500. Use a function like this:

Function LastRowNum(sheetName As String, columnLetter As String) As Long

    With ThisWorkbook.Worksheets(sheetName)
        LastRowNum = .Range(columnLetter & .Rows.Count).End(xlUp).Row
    End With

End Function

If you are going to use an AutoFilter in the SortingRisk proc then why not use it to hide the empty rows? It should be much quicker than looping through cells using VBA.


Auth token SSL implementation <?php

define('AES_256_CBC', 'aes-256-cbc');

// both stored in a file on server
$encryption_key = openssl_random_pseudo_bytes(32);
$iv = openssl_random_pseudo_bytes(openssl_cipher_iv_length(AES_256_CBC));

$user_id = 1123;
$exp     = time() + (365 * 24 * 60 * 60);

$data = [
    "user_id" => $user_id,
    "exp"     => $exp,
];
$json = json_encode($data);

$token = openssl_encrypt($json, AES_256_CBC, $encryption_key, 0, $iv);
$base = base64_encode($token);
// at this point send the token to the client to use for further auth
$tokenDecoded = base64_decode($base);
$clear = openssl_decrypt($tokenDecoded, AES_256_CBC, $encryption_key, 0, $iv);
$sessionData = json_decode($clear);

?>

Everything occurs over an SSL HTTPS connection with signed certificate.

Is my implementation safe?

It is not just the safety of your code that you should be worried about. You need a holistic approach; your question suggests to me that you have not looked at the bigger picture. Read this article and you will see you need to approach your question differently. For one, it depends on your server's configuration.

Depending on how important the security requirements are for you, I'd suggest using a third party service; either one which takes the problem off your hands entirely, or if that's not an option, have a consultant guide you to making a setup that will stay secure even over time while code changes and cracks can slip through unnoticed. There are some services that can try to find holes in your system, but they are only of limited use.

If you need a better answer than this, I think you're going to have to give a lot more info on the context of your code; where it's used, how it's used, etc.


Searching a dictionary After some advice, which method do you think is better to use when searching a dictionary for the matching value(s)?

Method One

Console.WriteLine("ENTER A PERSON/NUMBER TO SEARCH FOR");
string input = Console.ReadLine().ToUpper();
bool found = false;

if (myPhoneBook.ContainsKey(input)) // used with a persons name and returns number
{
    Console.WriteLine(myPhoneBook[input]);
    found = true;
}
if (!(myPhoneBook.ContainsKey(input))) //searches the number and returns the name
{
    var searchingfornumbers = from num in myPhoneBook
                              where num.Value == input
                              select num.Key;

    foreach(var person in searchingfornumbers)
    {
        Console.WriteLine(person);
        found = true;
    }
}
if(found == false)
{
    Console.WriteLine("NO RECORD FOUND");
}

Method Two

Console.WriteLine("ENTER A PERSON/NUMBER TO SEARCH FOR");
string input = Console.ReadLine();
bool found = false;
var query0 = from person in myPhonebook //searches for the number, returns name
             where person.Value == input
             select person.Key;

foreach(var person in query0)
{
    Console.WriteLine(person);
    found = true;
}

var query1 = from person in myPhonebook //searches for the name, returns numbers
             where person.Key == input
             select person.Value;

foreach (var person in query1)
{
    Console.WriteLine(person);
    found = true;
}
if(found == false)
{
    Console.WriteLine("NO RECORD FOUND");
}
Console.ReadKey();

I don't like either of them because they look like multiple numbers could be assigned to multiple persons which is obviously wrong.

You can get only one result for a search because you are not searching with wildcards so consider this. A person can have multiple numbers so for a person you can have a single hit. If you search for an exact number you'll also get a single hit. This can be made clear by naming the method appropriately: FindExact. Later you can add another one that can search with wildcards (patterns).

class Phonebook
{
    private readonly Dictionary<string, List<string>> _entries =
        new Dictionary<string, List<string>>();

    public void Add(string person, string number)
    {
        List<string> numbers;
        if (_entries.TryGetValue(person, out numbers))
        {
            numbers.Add(number);
        }
        else
        {
            _entries[person] = new List<string> { number };
        }
    }

    public KeyValuePair<string, List<string>> FindExact(string personOrNumber)
    {
        List<string> numbers;
        return
            _entries.TryGetValue(personOrNumber, out numbers)
            ? new KeyValuePair<string, List<string>>(personOrNumber, numbers)
            : _entries.FirstOrDefault(x => x.Value.Contains(personOrNumber));
    }
}

Currently searching for a persons would be faster because it is a key in the dictionary. You could add another dictionary that maps numbers to persons and make the number search as fast as person search by looking at the key instead of searching all values:

private readonly Dictionary<NUMBER, PERSON> _numberPerson...

In your second solution you don't need a dictionary at all because use don't use it like it should be used. You loop twice over it to find either value. In this case you can also use a list.

If you want to use a dictionary then use it appropriately otherwise it doesn't make any sense.

Use two dictionaries to make the exact search even faster but don't loop twice over it.

You can also replace both queries with a single one:

var query =
    from x in myPhonebook
    where x.Key == input || x.Value == input
    select x;

Even better like so:

var result = myPhonebook.FirstOrDefault(x => x.Key == input || x.Value == input);


TypeScript simple localStorage cache I have a simple localStorage cache class written in TypeScript, would love to have some insights on any way/aspect you think it can be improved (making it more readable, maintainable, usable, etc.):
export interface CacheItem<T> {
  data: T;
  expiration: number;
}

class SimpleLocalStorageCache<T> {
  constructor(private key: string, private durationInSeconds: number) {}

  get(): CacheItem<T> | null {
    const cache = localStorage.getItem(this.key);

    if (!cache) {
      return null;
    }

    const parsedCache = JSON.parse(cache);

    if (Date.now() >= parsedCache.expiration) {
      return null;
    }

    return parsedCache;
  }

  update(data: T): void {
    const durationInMilliseconds = this.durationInSeconds * 1000;

    localStorage.setItem(
      this.key,
      JSON.stringify({
        data,
        expiration: Date.now() + durationInMilliseconds,
      })
    );
  }
}

export default CacheSimpleLocalStorageCache;

Really appreciate any feedback on how to improve.

I took a look at it and will elaborate on my few findings below.
First of all, your default export is wrong, but most likely you knew about it and you have already fixed it in your local version (might be also some pasting issue) - it's a no brainer. SimpleLocalStorageCache vs. CacheSimpleLocalStorageCache. Also I don't think it's necessary to export the interface CacheItem as it's most likely an internal interface to be used. Do not leak it, to prevent misuse :-)
Next, looking at the constructor constructor(private key: string, private durationInSeconds: number) {} it's quite nice that you've used constructor assignment, thumbs up for that. What I think is a bit weird though, is the fact, that you've decided that the consumer of the API has to pass in seconds. That's a bit odd if we look at for example the parameters of setTimeout.
Another thing that bugs me with the seconds/calculation is, that I would except it to be converted to milliseconds inside the constructor. That's a one time calculation and isn't required to be done each time a consumer of this API is calling the update function.
I noticed that I can pass in functions which get's lost:
import Storage from "yourStorageImplementation";

const TWO_SECONDS = 2;
const instance = new Storage("someKey", TWO_SECONDS);
instance.update(() => console.log("this will be lost!"));

Fetching the exact same, results in this unexpected behavior:
instance.get() // -> { data: undefined, expiration: /*some value */ }

If you don't support persisting functions, which is fine, prevent the consumer of the API to pass in such data. This could be achieved by e.g. adjusting the generic to only allow strings, booleans, and objects with key, value pairs maybe extend/reduce this to your needs:
class SimpleLocalStorageCache<T extends string | number | boolean | { [key: string]: string | number | boolean }> { ... }

Alternatively you could do filter out just any function like:
type IsNotFuction<X> = X extends (...args: any[]) => any ? never : X;

class SimpleLocalStorageCache<T> {
  ...
  update(data: IsNotFuction<T>): void {
    const durationInMilliseconds = this.durationInSeconds * 1000;

    localStorage.setItem(
      this.key,
      JSON.stringify({
        data,
        expiration: Date.now() + durationInMilliseconds,
      })
    );
  }
}

// Usage, isn't quite handy:
const storage1 = new SimpleLocalStorageCache<() => void>("someKey", 2); // It's required to provide the generic here, meeeh!
console.log(storage1.update(() => alert("bla"))) // Errors here!
console.log(storage1.get());

I do like the solution and most of my write up is opinionated. Below are some suggestions I came up with while fiddling around with your solution:
Suggestion: Do you intend to extend it for sessionStorage and let the user decide the caching strategy? Is there a plan to properly support functions?
Suggestion: What do you think about a fluent API for this extension. Wouldn't it be nice to support something like: new Storage("key", 2).update(/* some data */).get(); to immediately fetch the result again?


Using descriptors with instance variables to store a human's age I am studying descriptors. They work with class varibles out of the box, but I want them to operate on instance variables too, so I did this:

class NumberDescriptor(object):
    def __init__(self,name):
        self.name = name

    def __set__(self, instance, value):
        if not isinstance(value, int):
            raise ValueError("%s is not a number." % value)
        else:
            setattr(instance, self.name, value)

    def __get__(self, instance, owner):
        return getattr(instance, self.name)

class Human(object):
    age = NumberDescriptor("_age")

    def __init__(self,age):
        self.age = age

a = Human(12)
print a.age
b = Human("osman")
print b.age

Is this the right way to go about this?

That looks reasonable; but you should handle the case of when the descriptor is invoked from the class: Human.age.  In that case, __get__ is invoked with None as an argument to instance.  If you've no better use for it; you can just return the descriptor itself:

def __get__(self, instance, owner):
    if instance is None:
        return self
    else:
        return getattr(instance, self.name)

Another thing you can do, which is sometimes preferred, is to make the descriptor figure out its own name by inspecting the class it's attached to;  this is a little more complicated, of course, but has the advantage of allowing you to specify the "name" of the descriptor attribute just one time.

class NumberDescriptor(object):
    def snoop_name(self, owner):
        for attr in dir(owner):
            if getattr(owner, attr) is self:
                return attr
    def __get__(self, instance, owner):
        if instance is None:
            return self
        name = self.snoop_name(self, owner)
        return getattr(instance, '_'+name)
    def __set__(self, instance, value):
        name = self.snoop_name(self, type(instance))
        setattr(instance, '_' + name, int(value))

class Human(object):
    age = NumberDescriptor()

Other variations to get a similar effect (without using dir()) would be to use a metaclass that knows to look for your descriptor and sets its name there.


"Causes" Puzzle solution (network of words with Levenshtein distance 1) The "Causes" puzzle is to recursively find, starting with the word "causes", the number of words that can be formed by substituting one letter.

sub addMember {
my ( $self, $newMember ) = @_;
my $len = length($newMember->{'_data'});
my $newMemberIndex = push(@{$self->{'_MemberList'}}, $newMember) - 1;
$self->{'_MemberHash'}->{$newMember->{'_data'}} = \$newMember;

#Fetch bucket of same length and check for distance
my $memberList = $self->{'_Bucket'}->getBucket($len);

foreach my $index (@$memberList){
  if($index eq $newMemberIndex) {
    next;
  }
  my $memberObj = $self->{'_MemberList'}[$index];
  if($self->isFriend($newMember->{'_data'}, $memberObj->{'_data'}) eq true) {
    $newMember->addNeighbour($index);
    $memberObj->addNeighbour($newMemberIndex);
  }
}

#Fetch bucket of length - 1 and check for distance
$memberList = $self->{'_Bucket'}->getBucket($len-1);
foreach my $index (@$memberList){
  if($index eq $newMemberIndex) {
    next;
  }
  my $memberObj = $self->{'_MemberList'}[$index];
  if($self->isFriend($newMember->{'_data'}, $memberObj->{'_data'}) eq true) {
    $newMember->addNeighbour($index);
    $memberObj->addNeighbour($newMemberIndex);
  }
}
#Fetch Bucket of length + 1 and check for distance
$memberList = $self->{'_Bucket'}->getBucket($len+1);
foreach my $index (@$memberList){
  if($index eq $newMemberIndex) {
    next;
  }
  my $memberObj = $self->{'_MemberList'}->[$index];
  if($self->isFriend($newMember->{'_data'}, $memberObj->{'_data'}) eq true) {
    $newMember->addNeighbour($index);
    $memberObj->addNeighbour($newMemberIndex);
  }
}

#Add to the member list
$self->{_Bucket}->addToBucket($len, $newMemberIndex);

}

Here is one idea: You could first put the words into a hashmap, the length of them being the key. O(n)

Then, since you are only interested in "causes", grab that word and make a node and remove it from the hashmap.

Then, a recursive function

adds all friends to the node that are in the hashmap (only looking at the relevant lengths)
removes those from the hashmap
calls itself on those new nodes

The end result is the social graph of "causes".

It really depends what you are interested in in improving though: For instance, one could assume the whole thing is only called once for a fixed string, or it could be called many many times for different inputs. Depending on that, one might precompute more.


Calling a button's OnClick method elsewhere - Good Practice? I have two buttons on an ASP.NET page that have separate OnClick methods in the code behind. In one of the methods, if a certain condition is met, the entire process of the other method should be executed.

Here is a simplified example of my code, which works as I intend in my actual implementation:

// OnClick="ThisButton_OnClick" for ThisButton
protected void ThisButton_OnClick(object sender, EventArgs e)
{
    // do stuff
    if(condition)
        ThatButton_OnClick(null, null);
    else
        // do stuff
}

// OnClick="ThatButton_OnClick" for ThatButton
protected void ThatButton_OnClick(object sender, EventArgs e)
{
    // do stuff
}

My question is this: is it good practice to do something like this, where you call a method specifically designed for OnClick without clicking that button, or should I be more explicit (perhaps have a separate helper function that is called by ThatButton_OnClick and in the if)?

I'd suggest something like the following which is simple enough for even a novice like me to understand. Is there a real need to simulate button click? You're not testing buttons, are you?

// OnClick="ThisButton_OnClick" for ThisButton
protected void ThisButton_OnClick(object sender, EventArgs e)
{
    // do stuff
    if(condition)
        DoStuffThatButtonWouldDo(arg);
    else
        // do stuff
}

// OnClick="ThatButton_OnClick" for ThatButton
protected void ThatButton_OnClick(object sender, EventArgs e)
{
    DoStuffThatButtonWouldDo(arg);
}

private static DoStuffThatButtonWouldDo(fancy arguments)
{
    //TODO: do all the stuff that the that button would do
}


Using Models and Entities in MVP pattern I'm a beginner in MVP design pattern and In a MVP triad, I have

Model - AdvanceInfo

View - AdvanceForm

Presenter - AdvancePresneter

and Entity class Advance as follows.

 public class Advance
{
    public int AdvanceID { get; set; }
    public decimal AdvanceAmount { get; set; }
    public DateTime AdvanceEnteredDate { get; set; }
    public DateTime AdvanceProcessDate { get; set; }
}

So Should my model be as follows...

class AdvanceInfo
{
    public int AdvanceID  { get { return Advance.AdvanceID; } }
    public decimal AdvanceAmount { get { return Advance.AdvanceAmount; } set { Advance.AdvanceAmount = value; } }
    public DateTime AdvanceEnteredDate  { get { return Advance.AdvanceEnteredDate; } set { Advance.AdvanceEnteredDate = value; } }

    public int EmployeeID { get; set; }
    public string NameWithInitials { get; set; }
    public string BankAccountName { get; set; }
    public string BankAccountNumber { get; set; }

    public Advance Advance = new Advance();
}

or else...

class AdvanceInfo
{
    public int AdvanceID { get; set; }
    public decimal AdvanceAmount { get; set; }
    public int EmployeeID { get; set; }
    public string NameWithInitials { get; set; }
    public string BankAccountName { get; set; }
    public string BankAccountNumber { get; set; }
    public DateTime AdvanceEnteredDate { get; set; }
    public DateTime AdvanceProcessDate { get; set; }

}

Note:
* I'm using entity class Advance as an aggregate class in DeductionInfo class as well.
* I'm not using any frameworks like EF. Only VS. (Since this is academic purpose project)

I hope my question is clear; actually what I want to know is how entity classes are used to build model? or else can we use Model classes alone?

It's not clear exactly what you're referring to as Model classes, I think you're over-thinking it.

I'd go with the else snippet, the POCO (Plain Old CLR Object) that does nothing but exposing auto-properties. The snippet that exposes a public Advance field doesn't look right. Do not expose public fields; a class should expose properties, not fields.

Your remark about only using VS isn't clear either: if you're not using EF, that's fine, but this means your data access code is using some data access library - Visual Studio isn't one, it's your IDE; if you're using things under System.Data, you're using ADO.NET.

If I understand correctly what you're doing, you have a bunch of tables/entities that have columns/properties in common, and you're wondering if composition would be better than repeating the properties on all classes. You forgot an option: inheritance.

Composition is normally favored over inheritance, but in the case of POCO classes composition can greatly complicate matters, and if you're instantiating your POCO's from an SqlReader it doesn't really matter how that's done, since you completely own the process.

Things are different with EF - composition would lead to a database model similar to this:

With inheritance, you have this (I hope I got the UML right):

Which involves 3 classes, but works off only 2 tables: Advance isn't really an entity, it's just a base class with properties that are common to DeductionInfo and AdvanceInfo.

Hence, if what you intend to do is to model 2 tables, I'd do this:

public abstract class AdvanceBase
{
    public int AdvanceID { get; set; }
    public decimal AdvanceAmount { get; set; }
    public DateTime AdvanceEnteredDate { get; set; }
    public DateTime AdvanceProcessDate { get; set; }
}

The class is made abstract, to enforce the concept that such a class shouldn't be instantiated and serve as a base class for derived types instead:

public class AdvanceInfo : AdvanceBase
{
    public int EmployeeID { get; set; }
    public string NameWithInitials { get; set; }
    public string BankAccountName { get; set; }
    public string BankAccountNumber { get; set; }
}

The AdvanceInfo class inherits the AdvanceID and everything else from AdvanceBase, so you don't need to specify them again.

Then you'll have:

public class DeductionInfo : AdvanceBase
{
    /* DeductionInfo members */
}

And here also, the class will inherit the AdvanceBase members so you only need to code the DeductionInfo-specific members in that class.

And if/when you switch to EF, you can reuse the exact same POCO classes as entities, without changing anything.

Side note, I think the naming of the classes is somewhat bad - I'd call AdvanceInfo just Advance, and DeductionInfo would be simply Deduction.


Reducing allocations in arbitrary precision calculations with Julia I am new to Julia and have only written in python before which is most likely also reflected by my coding style.
Unfortunately, my applications require very high numerical precision which is why I am resorting to BigFloat and BigInt in Julia. For some reason, this leads to a major increase in allocations besides the obvious increase in computational time.
I am wondering where my code can be improved.
I have attached the current version of my code below with example parameters that allow for execution under 1s.
using Combinatorics

function gamma_mpnq(m::Int, p::Int, n::Int, q::Int)::BigFloat
    return sqrt(factorial(big(m)) * factorial(big(n))) / (big(2)^(p + q) * factorial(big(m - 2 * p)) * factorial(big(n - 2 * q)) * factorial(big(q)) * factorial(big(p)))
end

function general_binomial(alpha::Float64, k::Int)
    """
    Implementation of the generalized binomial.
    """
    _prod = BigFloat(1.0)
    for kk in 1:k
        _prod *= (alpha - kk + 1.)
    end
    return _prod / factorial(big(k))
end

function construct_M(nmax::Int, mmax::Int)::Array
    """
    Construct the matrix M containing all moments <C^n * C*^m> up to nmax and mmax.
    """
    _M = zeros(BigFloat, nmax, mmax)
    for mm in 1:nmax
        for nn in 1:mmax
            if nn == 1
                _M[nn, mm] = 1. / doublefactorial(BigInt(2*mm - 1))
            elseif mm == 1
                _M[nn, mm] = 1. / doublefactorial(BigInt(2*nn - 1))
            else
                _M[nn, mm] = (((nn-1) * _M[nn-1, mm] + (mm-1) * _M[nn, mm-1]) / (2. * (nn-mm)^2 + (nn-1) + (mm-1)))
            end
        end
    end
    return _M
end

function maclaurin_exp(n::Int, m::Int, p::Int, q::Int, M::Array, cutoff::Int)::BigFloat
    """
    Calculate the expectation value of ((1+C)/(1+C*))^((n-m)/2) * C^p * C*^q
    using the Maclaurin expansion and the matrix containing all moments M.
    """
    inner::BigFloat = 0.0
    @views for nn in 1:cutoff
        for mm in 1:cutoff
            inner += (general_binomial((n-m)/2., mm-1) * general_binomial((m-n)/2., nn-1) * M[q+nn, p+mm])
        end
    end
    return inner
end

function Hmn(m::Int, n::Int, M::Array, cutoff::Int)::Float64
    """
    Calculates a single entry of the Hmn matrix.
    """
    ppmax = div(m, 2)
    qqmax = div(n, 2)
    outer = 0.0
    for pp in 0:ppmax
        for qq in 0:qqmax
            outer += (gamma_mpnq(m, pp, n, qq) * maclaurin_exp(n, m, pp, qq, M, cutoff))
            # println(pp, " ", qq, " ", outer)
        end
    end
    return outer
end

dim = 128
cutoff = 64
M = construct_M(dim+cutoff, dim+cutoff)
@btime Hmn(12, 2, M, cutoff)

```

The following is a workaround for your speed and memory issue rather than a general review of your code.
Profiling is useful for investigating the source of performance issues, and Julia's standard library includes a profiling module (guide, doc).
Here's a basic use of it: (I'm using @eval just to interpolate in the construction of M)
using Profile
@eval @profile Hmn(12, 2, $(construct_M(192, 192)), 64)

julia> Profile.print()
Overhead ╎ [+additional indent] Count File:Line; Function
=========================================================
    ╎963  @Base/task.jl:356; (::IJulia.var"#15#18")()
...
    ╎    ╎   963  In[1]:61; Hmn(::Int64, ::Int64, ::Arr...
    ╎    ╎    1    In[1]:4; gamma_mpnq(::Int64, ::Int6...
...
  18╎    ╎    962  In[1]:46; maclaurin_exp(::Int64, ::I...
   1╎    ╎     1    In[1]:0; general_binomial(::Float6...
    ╎    ╎     14   In[1]:11; general_binomial(::Float6...
...
    ╎    ╎     786  In[1]:13; general_binomial(::Float6...
...
    ╎    ╎     97   In[1]:15; general_binomial(::Float6...
...

I've removed all the lines that point to code in external files. Of the 963 samples collected in Hmn, more than 800 were spent within calculations of general_binomial.
Looking at how general_binomial(alpha, k) is used, I noticed that it is called repeated with only a small number of different alpha and k values. Since it is a pure function (i.e., it does not interact with external state), basic memoization can be applied to speed it up.
We can use a Dict to cache the results (make sure it is defined with concrete key and value types). To allow for easy benchmarking, we'll define it as a global constant; for performance, it is important that global variables are constant (or otherwise type-constrained in code using it) so that type inference works (also, the compiled code can avoid an extra lookup).
Here's a simple way to do this. Rename general_binomial to _general_binomial, then create the Dict and redefine general_binomial to cache the results. Like so:
@isdefined(general_binomial) && Base.delete_method.(methods(general_binomial))

const GENERAL_BINOMIAL_CACHE = Dict{Tuple{Float64, Int}, BigFloat}()

function general_binomial(alpha, k)
    return get!(GENERAL_BINOMIAL_CACHE, (alpha, k)) do
        _general_binomial(alpha, k)
    end
end

Benchmarking shows a substantial improvement in speed and memory use using the parameters you provided.
Before:
julia> @benchmark Hmn(12, 2, $(construct_M(192, 192)), 64)
BenchmarkTools.Trial:
  memory estimate:  435.29 MiB
  allocs estimate:  8543119
  --------------
  minimum time:     787.497 ms (5.88% GC)
  median time:      791.734 ms (5.83% GC)
  mean time:        792.690 ms (5.76% GC)
  maximum time:     802.070 ms (5.17% GC)
  --------------
  samples:          7
  evals/sample:     1

After: (the cache is cleared before each sample by empty!(GENERAL_BINOMIAL_CACHE))
julia> @benchmark Hmn(12, 2, $(construct_M(192, 192)), 64) evals=1 setup=empty!(GENERAL_BINOMIAL_CACHE)
BenchmarkTools.Trial:
  memory estimate:  12.74 MiB
  allocs estimate:  239309
  --------------
  minimum time:     10.684 ms (0.00% GC)
  median time:      12.844 ms (0.00% GC)
  mean time:        14.181 ms (7.58% GC)
  maximum time:     32.631 ms (14.03% GC)
  --------------
  samples:          353
  evals/sample:     1


Is my blog controller too "Fat"? I would like some advice which relates to the "fat model skinny controller" concept in Rails.

See my show action in the Micropost controller

 def show
     @micropost = Micropost.find(params[:id])
     @tags = @micropost.tag_counts_on(:tags)
     @responses =  @micropost.responses
     @response = current_user.responses.build if member_signed_in?
     impressionist(@micropost)
     related_posts = Micropost.tagged_with(@micropost.tag_list, :on => :tags, :any => true).take(5)
     @cleaned_related_posts = Micropost.assemble_related_posts(related_posts, @micropost)
 end

This action does a number of things.
1. It shows a micropost
2. It gets all the tags (acts as taggable) from the micropost (to display them)
3. It builds the response - response form is on the same page
4. It impressions the micropost (impressionist gem)
5. Lastly, it builds a list of related microposts. I tried to clean this up by using a class method in my micropost class as follows

def self.assemble_related_posts(related_posts = [], micropost)
  @cleaned_related_posts = []
  related_posts.each do |post|
    unless post == micropost
      @cleaned_related_posts.push(post)
    end
   end
  @cleaned_related_posts
end

The related posts variable above INCLUDES the micropost being shown - no point in that, so I wrote a class method to remove it.

This show action is pretty fat, and my RubyMine keeps showing me a warning that the controller should call only one method.

I want to improve my skills (I am fairly new to Rails). Is there a "Rails way" to shift all this logic into the model.....should it be done? Are some controllers simply going to a bit on the fat side?

The only thing I can see is moving the related/cleaned posts method into an instance method on Micropost. I'm also not sure what the implementation of Micropost.tagged_with is like, but I bet that could be moved into an instance method. Without that, I might suggest:

class Micropost < ActiveRecord::Base
  def cleaned_related_posts(limit = -1)
    posts = Micropost.tagged_with(tag_list, :on => :tags, :any => true).reject { |p| p == self }
    posts = posts.take(limit) if limit > 0
    posts
  end
end

Then your controller becomes:

def show
  @micropost = Micropost.find(params[:id])
  @tags = @micropost.tag_counts_on(:tags)
  @responses =  @micropost.responses
  @response = current_user.responses.build if member_signed_in?
  impressionist(@micropost)
  @cleaned_related_posts = @micropost.clean_related_posts 5
end

It's not really much of a savings, but it does at least centralize the logic for related posts. This controller isn't really fat to begin with.

  ... RubyMine keeps showing me a warning that the controller should call only one method.

That's just plain silly. IDE suggestions should be taken with a grain of salt (and a wedge of lime, and a shot of tequila).


Dividing by the total count based on occupation I have this query where I am working out the percentage of peoples occupation. I have a sub query that works out the total count of people codes and that result is what I used to divide by. I could hardcode this but the count could change.

It just looks a bit messy and I am wondering if there is a nicer way to do the sub query.

SELECT
    CONVERT(VARCHAR(20),COUNT(CO.code)*100 /
(
SELECT
COUNT(CO.Code)
FROM
TABLE1 K
INNER JOIN TABLE2 CO ON(CO.id= K.id AND CO.status= 'Something' AND K.statuscode = 0  AND CO.statuscode =0))
)+'%',

Here is the final query. I needed the sub query because of the group by which if I did the count in the SELECT, it would just return the number of people who had a specific occupation.

SELECT
    CONVERT(VARCHAR(20),COUNT(CO.code)*100 /
(
SELECT
COUNT(CO.Code)
FROM
TABLE1 K
INNER JOIN TABLE2 CO ON(CO.id= K.id AND CO.status= 'Something' AND K.statuscode = 0  AND CO.statuscode =0))
)+'%',
K.occupation
FROM
TABLE 1 K
LEFT OUTER TABLE 2 CO  ON(CO.id = K.id AND CO.status = 'Something' AND K.statuscode = 0 AND CO.statuscode = 0)
GROUP BY
K.occupation
ORDER BY
COUNT(CO.Code) DESC

Is there a nicer/more efficient way to do this?

Aliases

The aliases you have used in this query, K and CO, are meaningless. I would recommend using either the full table name, if it is short enough, or using a meaningful alias, such as People, Jobs or Fruit for a tables containing information regarding People, Jobs and Fruit respectively.

I would recommend always including AS when using an alias to prevent confusion.

X = 0, X= 0 or X =0?

Okay first things first, you have examples of X= 0, X = 0 and X =0. You should choose find a style you like and can read well and stick with it. I'd recommend X = 0. I find it easier to understand where each side of the statement ends with the separating spaces.

Joins

Next, we have this bit:

INNER JOIN TABLE2 CO ON(CO.id= K.id AND CO.status= 'Something' AND K.statuscode = 0  AND CO.statuscode =0))

Firstly, you don't need brackets around the ON part of this. Adding the unnecessary brackets can just end up confusing you. If you had used a combination of ANDs and ORs in the ON clause, then you would use brackets to make sure the statement is executed as you intended.

Secondly, you are checking that both statuscodes are 0 in the ON clause. The correct way to do this would be to make sure you only bring back records where the statuscodes are the same, and check one of the statuscodes in the WHERE clause.

Thirdly, CO.status = 'Something' should also be moved into the WHERE clause.

Lastly, is there a reason that the subquery uses INNER JOIN and the main query uses LEFT OUTER JOIN?

A Suggestion

Might I suggest removing the subquery and calculating the total number of jobs and storing the result in a variable? This should speed things up a little and make the main query a little easier on the eyes.

Bringing It All Together

This is what I ended up with:

DECLARE @Total_Jobs INT =
(
    SELECT COUNT(CO.Code)
    FROM TABLE1 AS K
    INNER JOIN TABLE2 AS CO
    ON CO.id = K.id AND CO.statuscode = K.statuscode
    WHERE CO.status = 'Something'
    AND K.statuscode = 0
)

SELECT CONVERT(VARCHAR,COUNT(CO.code)*100/@Total_Jobs)
FROM TABLE1 AS K
LEFT OUTER JOIN TABLE2 CO
ON CO.id = K.id AND CO.statuscode = K.statuscode
WHERE CO.status = 'Something'
AND K.statuscode = 0
GROUP BY K.occupation
ORDER BY COUNT(CO.Code) DESC


shell library for Debian package releases This is a bash library intended to be used in a CI/CD pipeline to make finding the next release number for a Debian package semi-automatic.  I figured out how to use a JSON file as a metadata store since the CI/CD pipeline I'm working with doesn't provide any metadata storage across jobs.

I'm basically happy with how this is working.  We've used it for a few weeks without finding any bugs yet.  I'm happy that it passes shellcheck, but I'd also like to have some linting for ensuring that the code is documented adequately.  I'm not sure if my use of jq makes this too hard to maintain or if it just that using jq like this is so new to me.

deblib.sh

#!/bin/bash

# TODO: generate $PKGJSON from Debian repo metadata
PKGJSON=/opt/org_foo/var/packaging/packaging.json

# deb-release-number
# ------------------
#
# Arguments:
# 1. (required) package name
# 2. (required) package version ("upstream" version)
# 3. (optional) architecture (defaults to amd64)
# 4. (optional) packager (defaults to 1org_foo)
#
# Testing:
# Test by running
#       bash deblib.sh debug
# or passing any argumenet in to activate the "verification" section below

function deb-release-number {
        # read arguments
        PKGNAME=${1?provide package name as first argument}
        PKGVER=${2?provide package version as second argument}
        PKGARCH=${3:-amd64}
        PACKAGER=${4-1org_foo}

        # find latest release of this package/version
        RELEASE=$(jq -r ".\"latest_version\".\"${PKGNAME}\".\"${PKGVER}\"" <"$PKGJSON" )
        if [[ "$RELEASE" == "null" ]];
        then
                # start at 0 so the first release ends up being 1 after the increment below
                RELEASE=0
        fi
        ## echo RELEASE=$RELEASE

        # increment
        RELEASE=$(( RELEASE + 1 ))
        LONGPKGVER="${PKGVER}-${PACKAGER}-${RELEASE}" # seperated with dashes
        PKGFILENAME="${PKGNAME}_${LONGPKGVER}_${PKGARCH}.deb" # seperated with underscores
        echo "new release $RELEASE of $PKGNAME creates filename $PKGFILENAME"

        # backup
        cp "$PKGJSON" "${PKGJSON}.bak"

        # update release_version (into tmp file)
        jq ".\"latest_version\".\"${PKGNAME}\".\"${PKGVER}\" = \"${RELEASE}\"" <"$PKGJSON" >"${PKGJSON}.tmp"
        ## grep "$PKGVER" "${PKGJSON}.tmp"

        # update built_packages (out of tmp file into main file)
        jq ".\"built_packages\" += [\"${PKGFILENAME}\"]" <"${PKGJSON}.tmp" >"$PKGJSON"
        ## grep "$PKGNAME" "$PKGJSON"

        return $RELEASE
}

# verification
if [[ $# -gt 0 ]];
then
        deb-release-number librdkafka1 1.0.0
        deb-release-number foo 1.1.0 noarch
fi

Example

An example of usage:

source deblib.sh
deb-release-number 'foo-stuff' "${GIT_TAG#v}" 'amd64'
PKGRELEASE=$?

Some lints:

Uppercase names are by convention only for exported variables.
You can use getopt to parse parameters. This should make it easier to handle optional parameters, and also makes it easy to avoid positional parameters (which makes commands more self-documenting).
Use More Quotes™
I would guard against errors and accidents by using set -o errexit -o noclobber -o nounset -o pipefail at the top of the script.
returning anything other than an exit code is a problem. You function should instead output the value (and ensure other output goes to standard error). One issue is the semantics of conflating exit codes. The other is the lingering bug because exit codes wrap around after 255.
You can use mktemp --directory to create a temporary directory to store intermediate results. Even better would be to pass results in a pipeline to avoid any temporary files.


Safer way to wait amount of time I'm writing a helper for asynchronous operations.
By default, winjs does not provide a way to wait x seconds before the execution of another function. So I decided to write a helper.

(function asyncOperation() {
    "use strict";

    // -------------------------------------------------------------------
    //   awaitTime(time_milliseconde)
    //  Description:
    //   Waits a specified amount of time.
    //  Returns:
    //   //
    //  Usage:
    //   foo.helpers.asyncOperation.awaitTime(1000).done(function(){
    //      //your logic
    //   });
    //
    // -------------------------------------------------------------------
    function awaitTime(milliseconds) {
        if(!milliseconds || +milliseconds) {
            // need to ensure that milliseconds can be used
            milliseconds = 0;
        }

        // create a new promise and use set time out to wait x milliseconds.
        return new WinJS.Promise(function (complete) {
            setTimeout(function () {
                // informe the promise that it ended.
                complete();
            }, milliseconds);
        });
    }

    WinJS.Namespace.define("foo.helpers.asyncOperation", {
        awaitTime: awaitTime
    });
})();

I want my code to be as safe as possible. That's why I'm using "+value" in my "if".
I saw that in some cases the performance of the setTimeout is not good.
Also, is it better to define the function and assign it to the namespace at the end? (http://msdn.microsoft.com/en-us/library/windows/apps/hh967793.aspx)

So, I want to know if my code can be refactored/safer.

I think WinJS already provides this:

WinJS.Promise.timeout(timeout) creates a promise that is completed asynchronously after the specified timeout, essentially wrapping a call to setTimeout within a promise.

See also  http://msdn.microsoft.com/en-us/library/windows/apps/br229729.aspx

If you wish to continue with your own function, then you should review how promises ought to work, I don't think you are using the promises object the way it was intended.


Refactor embedded switch A particular application may refer to a specific property in one of three languages in different contexts: in lowercase English, in uppercase English, and in Hebrew. To facilitate ease of converting one language to another, this monstrosity exists:

public string translatePersonType(string personType, string lang)
{

    switch (personType)
    {
        case "employee":
        case "EMPLOYEE":
        case "עובד":
            switch (lang)
            {
                case "eng":
                    return "employee";
                case "ENG":
                    return "EMPLOYEE";
                case "heb":
                    return "עובד";
            }
            break;

        case "contractor":
        case "CONTRACTOR":
        case "קבלן":
            switch (lang)
            {
                case "eng":
                    return "contractor";
                case "ENG":
                    return "CONTRACTOR";
                case "heb":
                    return "קבלן";
            }
            break;

        case "supplier":
        case "SUPPLIER":
        case "ספק":
            switch (lang)
            {
                case "eng":
                    return "supplier";
                case "ENG":
                    return "SUPPLIER";
                case "heb":
                    return "ספק";
            }
            break;

        case "customer":
        case "CUSTOMER":
        case "לקוח":
            switch (lang)
            {
                case "eng":
                    return "customer";
                case "ENG":
                    return "CUSTOMER";
                case "heb":
                    return "לקוח";
            }
            break;

        default:
            return "";

    }// end switch (personType)

    return "";

}// end translatePersonType

Can this be refactored to be more concise and maintainable? For reference, this is the database field behind the property:

personType SET('CUSTOMER','SUPPLIER','EMPLOYEE', 'CONTRACTOR') NOT NULL

Thanks.

This kind of switch statements can be refactored using maps/dictionaries. I would do it like this:

Edit: I actually made a couple of mistakes. After adding more unit tests I came up with a hopefully better version.

//V. 2:
public static string TranslatePersonTypeBetterRefactored(string personType, string lang)
{
    var allDicts = new Dictionary<string, Dictionary<string, string>>
                        {
                            {
                                "heb", new Dictionary<string, string>()
                                            {
                                                { "employee", "עובד" },
                                                { "contractor", "קבלן" },
                                                { "supplier", "ספק" },
                                                { "customer", "לקוח" }
                                            }
                            },
                            {
                                "eng", new Dictionary<string, string>()
                                            {
                                                { "עובד", "employee" },
                                                { "קבלן", "contractor" },
                                                { "ספק", "supplier" },
                                                { "לקוח", "customer" }
                                            }
                            },
                            {
                                "_eng", new Dictionary<string, string>()
                                            {
                                                { "employee", "employee" },
                                                { "contractor", "contractor" },
                                                { "supplier", "supplier" },
                                                { "customer", "customer" }
                                            }
                            }
                        };

    Dictionary<string, string> dict;
    string tran;

    if ((allDicts.TryGetValue(lang.ToLower(), out dict) &&
         dict.TryGetValue(personType.ToLower(), out tran)) ||
        (allDicts.TryGetValue(string.Format("_{0}", lang.ToLower()), out dict) &&
         dict.TryGetValue(personType.ToLower(), out tran)))
    {
        var shouldBeUpper = lang.Any(c => char.IsUpper(c));

        return shouldBeUpper ? tran.ToUpper()
                             : tran;
    }

    return string.Empty;
}

//V. 1 (buggy):
public static string TranslatePersonTypeRefactored(string personType, string lang)
{
    var engToUpper = lang == "ENG";
    var engToLower = lang == "eng";

    if (engToUpper && personType.Any(c => char.IsLower(c)))
        return personType.ToUpper();

    if (engToLower && personType.Any(c => char.IsUpper(c)))
        return personType.ToLower();

    var hebrewToEnglishWords = new Dictionary<string, string>
                        {
                            { "עובד", "employee" },
                            { "קבלן", "contractor" },
                            { "ספק", "supplier" },
                            { "לקוח", "customer" }
                        };

    var hebrewToEngTranslation = hebrewToEnglishWords[personType];

    return engToLower ? hebrewToEngTranslation
                      : hebrewToEngTranslation.ToUpper();
}


Maximum Priority Queue using Heap I have written this program for Maximim Priority Queue after studying from Introduction to Algorithm . I want to improve my code. Is there any other method to implement Priority Queue instead of Heap?

#include <iostream>
#include <vector>
#include <algorithm>

template<typename T>
void max_heapify(std::vector<T>& array, size_t index)
{
  size_t largest;
  size_t left  = (2*index) + 1;
  size_t right = left + 1;

  if(left < array.size() && array[left] > array[index])
    largest = left;
  else
    largest = index;

  if(right < array.size() && array[right] > array[largest])
    largest = right;

  if(largest != index)
  {
    int tmp = array[index];
    array[index] = array[largest];
    array[largest] = tmp;
    max_heapify(array, largest);
  }
}

template<typename T>
void build_max_heap(std::vector<T>& array)
{
  for(int64_t i = (int64_t(array.size())/2) - 1; i >= 0; i--)
  max_heapify(array, i);
}

template<typename T>
T heap_maximum(std::vector<T>& array)
{
  return array[0];
}

template<typename T>
T heap_extract_max(std::vector<T>& array)
{
  if(array.size() < 0)
  {
    std::cerr << "Heap Underflow \n";
    return -1;
  }
  else
  {
    T max = array[0];
    array[0] = array[array.size() - 1];
    array.erase(std::remove(array.begin(), array.end(), array[0]),
                  array.end());
    array.shrink_to_fit();
    max_heapify(array, 0);
    return max;
  }
}

template<typename T>
void heap_increase_key(std::vector<T>& array, size_t index, T value)
{
  if(value < array[index])
  {
    std::cerr <<"New value is smaller than the current value\n";
    return;
  }
  else
  {
    array[index] = value;
    while(index > 0 && array[(index/2) - 1] < array[index])
    {
      std::swap(array[index], array[(index/2) - 1]);
      index = (index/2) - 1;
    }
  }
}

template<typename T>
void max_heap_insert(std::vector<T>& array, T value)
{
  array.resize(array.size() + 1);
  array[array.size() - 1] = -1;
  heap_increase_key(array, array.size() - 1, value);
  build_max_heap(array);
}

int main()
{
std::vector<int> v({1, 2, 6, 3, 7});
build_max_heap(v);
std::cout << "Max-Heap: ";
for(size_t i = 0; i < v.size(); i++)
{
  std::cout << v[i] << " ";
}
std::cout << "\n";
std::cout << "Extract-Maximum: " << heap_extract_max(v) << "\n";
std::cout << "New Heap: ";
for(size_t i = 0; i < v.size(); i++)
{
  std::cout << v[i] << " ";
}
max_heap_insert(v, 10);
std::cout << "\nNew Heap after inserting 10: ";
for(size_t i = 0; i < v.size(); i++)
{
  std::cout << v[i] << " ";
}
std::cout << '\n';
}

Advice 1

array.shrink_to_fit();: don't do it. If the implementation obeys strictly that request, you will waste a lot CPU cycles.

Advice 2

Whenever you publish your results, make sure that your algorithms/data structures do not write to standard output.

Advice 3

You can micro-optimize your max_heapify. Instead of swapping \$n\$ times, do a shift (see image below). When you have to swap, you sacrifice 3 assignments, so the total work is \$3n\$. If you do a shift instead, you make \$n + 1\$ assignments.

Advice 4

T heap_extract_max(std::vector<T>& array)
{
  if(array.size() < 0)
  {
    std::cerr << "Heap Underflow \n";
    return -1;
  }

I believe bad things will happen when T is not an integer.


Implementation of a shift operation for Set in JavaScript that emulates the one for arrays I discovered that the JavaScript sets are ordered but don't offer a method to take the first element out of the set. For the arrays this operation exists and it's called shift.

A very basic implementation can be written in this way:

function shiftSet(s) {
    let res;
    for (let v of s) {
        res= v;
        break;
    }
    s.delete(res);
    return res;
}

Do you see a better way to write it?

Another way to do it is via the iterator function.

function shifySet(set) {
    const first = set[Symbol.iterator]().next().value;
    set.delete(first);
    return first;
}

I was going to check if there was an item, but it seams pointless. Works the same as your function shifts the first entry, returning it, or undefined if not found.

I would make a slight change to your version

function shiftSet(set) {
    for (const res of set) {
        set.delete(res);
        return res
    }
}


Correct way to extend Bootstrap Modal - lock, unlock I'm trying to extend bootstrap 3 modal window with ability to disable closing it.

I've built this code:

(function ($, window, document, undefined) {
    var oldHide = $.fn.modal.Constructor.prototype.hide;

    $.fn.modal.Constructor.prototype.hide = function (_relatedTarget) {
        if (this.isLocked) return;
        return oldHide.call(this, _relatedTarget);
    };

    $.fn.modal.Constructor.prototype.lock = function (_relatedTarget) {
        this.isLocked = true;
        e = $.Event('lock.bs.modal', {
            relatedTarget: _relatedTarget
        });
        this.$element.trigger(e);
    };

    $.fn.modal.Constructor.prototype.unlock = function (_relatedTarget) {
        this.isLocked = false;
        e = $.Event('unlock.bs.modal', {
            relatedTarget: _relatedTarget
        });
        this.$element.trigger(e);
    };

})(jQuery, window, document);

Working demo

After clicking "lock," you won't be able to close modal until you click "unlock."

It this way of extending bootstrap modal correct? What are potential dangers of doing this?

And how can I make this better?

EDIT:

This is my final version based on suggestions.

Looks fine to me, except that you failed declare the e variable in the lock/unlock functions. So it's an automatic global, which isn't good.

With a little bit of cleanup:

(function ($) {
  var _original = $.fn.modal.Constructor.prototype.hide;

  function trigger(target, name, relatedTarget) {
    target.trigger($.Event(name, { relatedTarget: relatedTarget }));
  }

  $.extend($.fn.modal.Constructor.prototype, {
    hide: function (_relatedTarget) {
      if (this.isLocked) return;
      return _original.call(this, _relatedTarget);
    },

    lock: function (_relatedTarget) {
      this.isLocked = true;
      trigger(this.$element, 'lock.bs.modal', _relatedTarget);
    },

    unlock: function (_relatedTarget) {
      this.isLocked = false;
      trigger(this.$element, 'unlock.bs.modal', _relatedTarget);
    }
  });
})(jQuery);