I CAN HAS EULR 14, PLZ? A few days ago I found myself learning lolcode. It turns out it's (almost) a real language and capable of handling (pseudo-)serious problems. So, let's try some (pseudo-)serious code this time.

Project Euler 14

A Collatz sequence is defined for the set of positive integers:

n → n/2 (n is even)
n → 3n + 1 (n is odd)

Using this rule, a sequence can be generated.

Which starting number, under one million, produces the longest chain?

OBTW
    WHICH STARTIN NUMBR, UNDR WAN MILLION, PRODUCEZ TEH LONGEST CHAIN?
TLDR

HAI 1.3

  HOW IZ I collatz YR currentNumber
    I HAS A cowntr ITZ 1
    IM IN YR collatzLoop
      BOTH SAEM 0 AN MOD OF currentNumber AN 2, O RLY?
        YA RLY, currentNumber R QUOSHUNT OF currentNumber AN 2
        NO WAI, currentNumber R SUM OF PRODUKT OF currentNumber AN 3 AN 1
      OIC
      cowntr R SUM OF cowntr AN 1
      BOTH SAEM currentNumber AN 1, O RLY?
      YA RLY, FOUND YR cowntr, OIC
    IM OUTTA YR collatzLoop
  IF U SAY SO

  HOW IZ I longestCollatz YR maxNumber
    VISIBLE maxNumber
    I HAS A maxLengthFound ITZ 0
    I HAS A currentLength ITZ 0
    I HAS A longestAtValue ITZ 0
    I HAS A cowntr ITZ 1
    IM IN YR maxCollatzLoop
      currentLength R I IZ collatz YR cowntr MKAY
      DIFFRINT currentLength AN SMALLR OF currentLength AN maxLengthFound, O RLY?
      YA RLY, maxLengthFound R currentLength, longestAtValue R cowntr, OIC
      cowntr R SUM OF cowntr AN 1
      BOTH SAEM cowntr AN maxNumber, O RLY?
      YA RLY, FOUND YR longestAtValue, OIC
    IM OUTTA YR maxCollatzLoop
  IF U SAY SO

  VISIBLE I IZ longestCollatz YR 1000000 MKAY

KTHXBYE

Took only 3 minutes to execute on my machine, lightning fast!

Of-course, in any real language with a real implementation, this should take under 5 seconds. But lolcode isn't very fast and it's lacking a lot of performance constructs. For example, usually you'd memoization or recursion. I'm pretty sure the latter can't be done and the former would get very ugly very fast.

How can I take advantage of the features that are available in this language to the fullest extent to improve the performance of my program?

Indentation

  BOTH SAEM currentNumber AN 1, O RLY?
  YA RLY, FOUND YR cowntr, OIC

You're missing indentation here. In more mainstream languages, this construct looks like so:

  if (currentNumber == 1)
  return cowntr;

This is hard to read, and would be better if it were indented:

  if (currentNumber == 1)
      return cowntr;

  BOTH SAEM currentNumber AN 1, O RLY?
      YA RLY, FOUND YR cowntr
  OIC

Plus it's a good idea to just use braces in the first place.

The same thing goes for the constructs later on:

  DIFFRINT currentLength AN SMALLR OF currentLength AN maxLengthFound, O RLY?
  YA RLY, maxLengthFound R currentLength, longestAtValue R cowntr, OIC

  BOTH SAEM cowntr AN maxNumber, O RLY?
  YA RLY, FOUND YR longestAtValue, OIC

These also need proper indentation.

If you don't want to use the brace style, then just indent the YA RLY case one level. That way you get the indented look of mainstream languages...

if (condition)
    do thing

... which is easier to quickly parse than a wall of left-aligned code.

Bugs

Collatz of 1 doesn't return 1. It returns 3.

Loops

Looking at your code some more, I get the feeling that you went out of your way to dodge the regular loop structure as it didn't do what you wanted it to do the last time. But as a result you have to write your own incrementer and your own exit condition.

I think you'd be better off by using the regular loop structure. You can rely on the return.

HOW IZ I collatz YR currentNumber
  I HAS A cowntr ITZ 1
  IM IN YR collatzLoop
    BOTH SAEM 0 AN MOD OF currentNumber AN 2, O RLY?
      YA RLY, currentNumber R QUOSHUNT OF currentNumber AN 2
      NO WAI, currentNumber R SUM OF PRODUKT OF currentNumber AN 3 AN 1
    OIC
    cowntr R SUM OF cowntr AN 1
    BOTH SAEM currentNumber AN 1, O RLY?
    YA RLY, FOUND YR cowntr, OIC
  IM OUTTA YR collatzLoop
IF U SAY SO

becomes

HOW IZ I collatz YR currentNumber
  IM IN YR collatzLoop UPPIN YR cowntr
    BOTH SAEM currentNumber AN 1, O RLY?
    YA RLY,
      FOUND YR SUM OF cowntr AN 1
    OIC
    BOTH SAEM 0 AN MOD OF currentNumber AN 2, O RLY?
      YA RLY, currentNumber R QUOSHUNT OF currentNumber AN 2
      NO WAI, currentNumber R SUM OF PRODUKT OF currentNumber AN 3 AN 1
    OIC
  IM OUTTA YR collatzLoop
IF U SAY SO

And this loop...

I HAS A cowntr ITZ 1
IM IN YR maxCollatzLoop
  currentLength R I IZ collatz YR cowntr MKAY
  DIFFRINT currentLength AN SMALLR OF currentLength AN maxLengthFound, O RLY?
  YA RLY, maxLengthFound R currentLength, longestAtValue R cowntr, OIC
  cowntr R SUM OF cowntr AN 1
  BOTH SAEM cowntr AN maxNumber, O RLY?
  YA RLY, FOUND YR longestAtValue, OIC
IM OUTTA YR maxCollatzLoop

Becomes

IM IN YR maxCollatzLoop UPPIN YR cowntr WILE BOTH SAEM maxNumber AN BIGGR OF cowntr AN maxNumber
  I HAS A collatzNumbar ITZ SUM OF cowntr AN 1
  currentLength R I IZ collatz YR collatzNumbar MKAY
  DIFFRINT currentLength AN SMALLR OF currentLength AN maxLengthFound, O RLY?
  YA RLY,
    maxLengthFound R currentLength
    longestAtValue R collatzNumbar
  OIC
IM OUTTA YR maxCollatzLoop
FOUND YR longestAtValue

The reason my versions are longer is because I opted to split your multiple statements to individual lines.

Version number

OBTW
    WHICH STARTIN NUMBR, UNDR WAN MILLION, PRODUCEZ TEH LONGEST CHAIN?
TLDR

HAI 1.3

I think I said something about this before, but I'll say it again; version number goes at the top. It's needed for the interpreter. Even if they work fine without the version number at the top, I think it'd be best to put the version number at the top so that interpreters don't have to go searching for it.


Password-Lock a Single Page with PHP - How did I do? I just wrote some PHP to let a client lock off a their one-page site from the public using a password. No users and no database. Just checking the password the user enters and testing it against the chosen phrase. This should be simple (right?) but I havn't built this sort of thing before. Let me know what you think, and whether it could be done better. Thanks!

includes/session.php

class Session {

private $logged_in;

function __construct() {
    session_start();
    $this->check_login();
}

// Return whether they are logged in.
public function is_logged_in() {
    return $this->logged_in;
}

// When initialized, check to see if the user is logged in.
private function check_login() {
    if (isset($_SESSION['logged_in'])) {
        // If logged in, take this action
        $this->logged_in = true;
    } else {
        // If not logged in, take this action
        $this->logged_in = false;
    }
}

// Set the session. User will remain logged in.
public function login() {
    $_SESSION['logged_in'] = 1;
    $this->logged_in = true;
}

// Log out. Unset session and destroy it.
public function logout() {
    unset($_SESSION['logged_in']);
    $this->logged_in = false;
    session_destroy();
}

public function check_pass($pass) {
    $pass = @strip_tags($pass);
    $pass = @stripslashes($pass);

    if ($pass !== 'TheChosenPassword') return false;

    return true;
}
}

login.php:

require_once('includes/session.php');

// If already logged in just send them to index.php
if ($session->is_logged_in()) {
    header('Location: index.php');
    exit;
}

// Create the var in the global scope.
$msg = 'This page is private.<br> Please enter the password.';

// If the form was submitted ...
if (isset($_POST['submit'])) {

    // Make sure they entered a pass
    if (!$_POST['pass']) {
        $msg = "Please enter a password.";
    } else {

        // If they entered a pass, grab it and trim it.
        $pass = trim($_POST['pass']);

        // Now check the pass
        if ($session->check_pass($pass)) {
            $session->login();
            header('Location: index.php');
            exit;
        } else {
            $msg = "Sorry, that password is incorrect.";
        } // Endif checkpass
    } // endif isset(pass)

} // Endif isset(post)

?> <!-- The form is bellow -->

index.php

require_once('includes/session.php');
if (!$session->is_logged_in()) {
    header('Location: login.php');
    exit;
}

You can probably guess from the code that the form just has an <input> with a name of 'pass' and a submit button with a name of 'submit'. Also a <small> tag in which i echo $msg. So, any thoughts on how this could be improved? Did I make any glaring mistakes? Thanks for the help!

A few things, mostly around logged_in:

private function check_login() {
    if (isset($_SESSION['logged_in'])) {
        // If logged in, take this action
        $this->logged_in = true;
    } else {
        // If not logged in, take this action
        $this->logged_in = false;
    }
}

Simply:

$this->logged_in = isset( $_SESSION['logged_in'] );

Also, the accessor method is_logged_in is being used quite well, yet there is a lack of a  corresponding set_logged_in. Add a set accessor, even if private, and use it exclusively.

Directly assigning the value of an instance variable in more than one place violates the DRY principle. That is, the assignment of $this->logged_in = <<value>> should exist only once: within the accessor. For example:

private function set_logged_in( $logged_in ) {
  $this->logged_in = $logged_in;
}

Once the accessor is in place, use it:

private function check_login() {
  $this->set_logged_in( isset($_SESSION['logged_in']) );
}

After the logged in status is using accessors exclusively, you can remove the logged_in variable completely and rely only on the $_SESSION['logged_in'] value. Think about what this implies for the check_login() method.

Next:

if ($pass !== 'TheChosenPassword') return false;

return true;

Simply:

return $pass === 'TheChosenPassword';


Finding Isogram Word An isogram (also known as a "nonpattern word") is a word or phrase without a repeating letter, however spaces and hyphens are allowed to appear multiple times.

The IsIsogram() method takes a string and returns boolean value.

INPUT: anik      OUTPUT: true
INPUT: A nika    OUTPUT: false

Approach 01: Using Dictionary<TKey, TValue>

static bool IsIsogram(string str)
    {
        //create a dictionary with 26 keys and assign false as default value
        var storeOccurance = new Dictionary<int, bool>();
        for (int i = 0; i <= 25; i++)
        {
            storeOccurance[i] = false;
        }

        str = Regex.Replace(str, "[ -]", ""); //ignore whitespace and dash

        //iterate over each character of the string
        foreach (var ch in str.ToLower())
        {
            //(ch - 'a') is used to calculate the key. Check if the character key has false as it's value,
            //meaning they were never stored
            if (!storeOccurance[ch - 'a'])
            {
                storeOccurance[ch - 'a'] = true; //assign true when they are stored in the dictionary
            }
            else //otherwise, if a character key has true as it's value then it means that it exists in the dictionary
            {
                return false; //return false and exits the iteration..Multiple occurance in dictionary
                               //means the string has repeating character so it's not Isogram
            }
        }
        return true;
    }

Approach 02: Using HashSet<T>

static bool IsIsogram(string str)
    {
        //TRICK: HashSet<T>.Add(T) returns false if an item already exists in HashSet<T>,
        //otherwise returns true and add the character to the datastructure

        var charHash = new HashSet<char>();

        foreach (var ch in str.ToLower())
        {
            //check if the charHash.Add() returns false, if it does then terminate
            //as it means the character already exists
            if (!charHash.Add(ch) && char.IsLetter(ch))
                return false;
        }
        return true; //otherwise return true
    }

Approach 03: Using LINQ

static bool IsIsogram(string str)
    {                                    //Suppose "anik a" is a given string
        return str.Where(char.IsLetter) //filter only the letters ||here- only a, n, i, k, a will be taken
                  .GroupBy(char.ToLower) //create group by characters(and transform them to lowercase) || here, there will be a group for each character a, n, i, k
                  .All(g => g.Count() == 1); //for every group, count the number of it's element and check if it's 1
                                            //|| here, 'a' group has 2 elements so it return false though other groups has only one element in their group
    }

Given those above approaches:

Which approach would you recommend regarding readability, clean code and performance?
Is there any scope for improvement in the existing approaches?

I like the second approach since it allows you to exit as soon as a false condition is reached and is simpler than the first.  I would offer one minor optimization, keep the check for isletter separate and only check for duplicate if isletter is true:

static bool IsIsogram(string str)
{
    //TRICK: HashSet<T>.Add(T) returns false if an item already exists in HashSet<T>,
    //otherwise returns true and add the character to the datastructure

    var charHash = new HashSet<char>();

    foreach (var ch in str.ToLower())
    {
        //check if the charHash.Add() returns false, if it does then terminate
        //as it means the character already exists
        if(char.IsLetter(ch))
        {
            if (!charHash.Add(ch))
                return false;
        }

    }
    return true; //otherwise return true
}


PHP: Succint and scalable function to get what financial year we are in I have the below

private function yearMenu() {
    $m = date('m');
    $y = date('Y');

    if (($m>3 && $y==2019) || ($m<4 && $y==2020)){
        $menu = 1;
    }
    if (($m>3 && $y==2020) || ($m<4 && $y==2021)){
        $menu = 2;
    }
    if (($m>3 && $y==2021) || ($m<4 && $y==2022)){
        $menu = 3;
    }
    if (($m>3 && $y==2022) || ($m<4 && $y==2023)){
        $menu = 4;
    }
    if (($m>3 && $y==2023) || ($m<4 && $y==2024)){
        $menu = 5;
    }
    if (($m>3 && $y==2024) || ($m<4 && $y==2025)){
        $menu = 6;
    }

    return $menu;
}

This code works as intended i.e. I want to be able to give each financial year an integer based on 2019-2020 being the base at value 1 and it increasing each year onward

I was wondering if there was a more succinct and scalable way to do this?

Thanks

Ways of improving:

According to information provided the crucial function's purpose is "to return a particular menu number for the current year". Thus, the function name would be more descriptive with name getFinYearMenuNumber.

The preconditions are:

your financial year flows through "April to April" month periods
there's starting year 2019 and ending year 2015 for the whole interval.To make the function more unified those boundaries could be passed as input parameters:

function getFinYearMenuNumber($yearFrom, $yearTo)

The main issue of the current approach is that it'll inevitably perform 5 excessive if checks (they could be at least mutually exclusive if ... else if ... else if ...).

Instead, we can just perform 2 subsequent checks:

if the current year falls in the needed interval
if the current month is following or preceding the starting month (April)

The final version:

function getFinYearMenuNumber($yearFrom, $yearTo) {
    $curr_month = (int) date('m');    # current month number
    $curr_year = (int) date('Y');     # current year
    $curr_date = new DateTime();

    if ((new DateTime("$yearFrom-04")) <= $curr_date && $curr_date <= (new DateTime("$yearTo-04"))) {
        if ($curr_month > 3) {
            return $curr_year - $yearFrom + 1;
        } else if ($curr_month < 4) {
            return $curr_year - $yearFrom;
        }
    }

    return 0;
}

print_r(getFinYearMenuNumber(2019, 2025));


Arrays of hashes to hash Task: convert this array of hashes:

animals = [
  {"cats" => 3},
  {"dogs" => 5 }
]

to this hash:

{
  "cats" => 3,
  "dogs" => 5
}

This is my solution, any feedback is appreciated :-)

animals.reduce({}) {|m,e|
  e.each{|k,v| m[k] = v}; m
}

The snippet e.each{|k,v| m[k] = v} does the same thing as the standard library method Hash#merge!, so the shortest way to write your solution is probably:

animals.reduce({}, :merge!)

Note that with this solution, if two hashes in the source array have duplicate keys, those coming later in the array will take priority and overwrite the duplicate keys from earlier in the array:

[{"egrets" => 17}, {"egrets" => 21}].reduce({}, :merge!)  # => {"egrets"=>21}

Also be aware that merge! is destructive to the original hash, which is fine here since we don't reuse the literal input to reduce. There is a non-destructive version, merge, which is better when the input needs to be preserved.


Sum of two numbers in array The task

  You are given two non-empty arrays representing two non-negative
  integers. The digits are stored in reverse order and each of their
  items contain a single digit. Add the two numbers and return it as an
  array.

  You may assume the two numbers do not contain any leading zero, except
  the number 0 itself.

  Example: Input: [2,4,3] + [5,6,4] Output: [7,0,8]

const a1 = [2,4,3];
const a2 = [5,6,4];

My imperative solution

function addTwoNumbers(a1, a2) {
  const len = a1.length > a2.length ? a1.length : a2.length;
  const res = [];
  let memo = 0;

  const sanitize = n => n ? n : 0;
  for (let i = 0; i < len; i++) {
      let sum = sanitize(a1[i]) + sanitize(a2[i]) + memo;
      memo = sum > 9 ? sum % 9 : 0;
      sum = sum > 9 ? sum % 10 : sum;
      res.push(sum);
  }
  return res;
}

console.log(addTwoNumbers(a1, a2));

My functional solution

const range = num => Array.from(new Array(num), (_, i) => i);

const addTwoNumbers2 = (a1, a2) => {
  const sanitize = n => n ? n : 0;
  const len = a1.length > a2.length ? a1.length : a2.length;
  const {res} = range(len)
  .reduce((acc,i) => {
    let sum = sanitize(a1[i]) + sanitize(a2[i]) + acc.memo;
    acc.memo = sum > 9 ? sum % 9 : 0;
    sum = sum > 9 ? sum % 10 : sum;
    acc.res.push(sum);
    return acc;
  }, {res: [], memo: 0,});
  return res;
};

console.log(addTwoNumbers2(a1, a2));

Bug
Unfortunately both your functions return bad results for many input arguments. You don't carry.
Both should return [0,1] however they return [0]
addTwoNumbers([1], [9]); // >> [0] wrong
addTwoNumbers2([1], [9]);  // >> [0] wrong

And the return for the following should be [6,0,1] but they return [6,6]
addTwoNumbers([9], [7, 9]);  // >> [6, 6] wrong
addTwoNumbers2([9], [7, 9]);  // >> [6, 6] wrong

The reason is that you forget to deal with the carry.
To number?
Konijn's answer is a tempting solution but will also fail as it is limited by JS Number.
addTwoNumbers([1,1,9,9,0,4,7,4,5,2,9,9,1,7,0,0,9], [1,9,9,0,4,7,4,5,2,9,9,1,7,0,0,9])

The correct answer is
[2,0,9,0,5,1,2,0,8,1,9,1,9,7,0,9,9]
However Konijn's solution incorrectly returns.
[0,0,9,0,5,1,2,0,8,1,9,1,9,7,0,9,9]
This is because the second number is Number.MAX_SAFE_INTEGER above which you can not get a reliable result.
BigInt
You could use a BigInt as its size is not limited.
Big ints can be written with the suffix n
 const big = 9907919180215090099079191802150900n;  // Note the suffix n

To convert the reversed number to a big int
 const a = [0,0,9,0,5,1,2,0,8,1,9,1,9,7,0,9,9,0,0,9,0,5,1,2,0,8,1,9,1,9,7,0,9,9];
 const big = BigInt(a.reverse().join(""));

Thus your function would be
 const addTwoNumbers = (a, b) => (
          BigInt(a.reverse().join("")) +
          BigInt(b.reverse().join(""))
      ).toString().split("").reverse();

But that takes the puz out of puzzle me thinks.
Carry
The aim is to carry the remainder of each addition onto the next digit. This is fundamental to all ALU (Arithmetic Logic Units) that are part of almost every modern CPU. (In the case of binary the carry is 1 or 0)
So when you add two values 9 + 9 the result is 8 and carry 1. You then add the 1 to the next digit. If no digit add it to zero. The result is 18.
The function is thus
Example A
function addNumbers(a, b) {
    const sum = [];
    var i = 0, carry = 0;
    while (i < a.length || i < b.length || carry > 0) {
        carry += (a[i] ? a[i] : 0) + (b[i] ? b[i] : 0);
        sum.push(carry % 10);
        carry = carry / 10 | 0;
        i++;
    }
    return sum;
}

Note That it continues adding digits until the carry is zero.
Performance
I am not surprised that the big int solution is nearly 7 times slower than carry method (example A) above, but this is due to the reverses, joins, and split
Big int are not fast (Very slow compared to standard JS numbers), doing the same sum on big int literals is only 5 times faster than the carry method (example A)
NOTES
BigInt is very new and you should check the browser for support before using it.
Big int also sees the arrival of 64bit int arrays (YAY YAY YAY) BigUint64Array, BigInt64Array (sorry MDN has empty pages on these references)


Converting a 12 hour time string to a 24 hour time string Input - Standard input - HH:MM:SS[AM|PM] format.

Examples:

12:00:00AM
12:00:00PM
01:00:10AM
01:00:10PM

The program reads one line per instance.

Output - military format (24 hour format):

00:00:00
12:00:00
01:00:10
13:00:10

using System;

class Solution {
    static void Main(String[] args) {
        var time = Console.ReadLine().Trim();
        MilitaryTime mt = new MilitaryTime(time);
        Console.WriteLine(mt);
    }

    class MilitaryTime{
        string newTime = null;
        string originalTime = null;

        string correctHour(string time){
            var ampmLen = 2;
            var ampm = time.Substring(time.Length-ampmLen, ampmLen);
            var hourIndex = 0;
            var hour = time.Split(':')[hourIndex];
            var h = hour;
            if(ampm.Equals("PM")){
                h = (int.Parse(hour) + 12).ToString();
            }
            if(hour.Equals("12") || hour.Equals("24")){
                if(ampm.Equals("AM")){
                    h = "00";
                } else if(ampm.Equals("PM")){
                    h = "12";
                }
            }
            return h;
        }
        string minutesAndSeconds(string time){
            var ampmLen = 2;
            var hourLen = 2;
            var startIndex = hourLen; // since we are taking after hour HH:MM:SS
            return time.Substring(startIndex, time.Length - ampmLen - hourLen);
        }
        public MilitaryTime(string time){
            originalTime = time;
        }
        public override string ToString(){
            if(string.IsNullOrEmpty(newTime)){
                newTime = correctHour(originalTime) + minutesAndSeconds(originalTime);
            }
            return newTime;
        }
    }
}

Is this code smelly?
I re-factored code to this state, but did I miss anything?
Is there a better way to solve this problem, using .net classes and features I might have missed?
Does it look like that it's written by someone junior or new to programming? If yes, then how would an experienced programmer write it?

Here's how to parse dates and times using built-in .Net libraries:

static void Main()
{
    string strTime = "01:00:10AM";
    DateTime time;
    if (DateTime.TryParseExact(strTime, "hh:mm:sstt", null,
        System.Globalization.DateTimeStyles.None, out time))
    {
        Console.WriteLine("Parsed \"{0}\" as {1:HH\\:mm\\:ss}", strTime, time);
    }
    else
    {
        Console.WriteLine("Could not parse \"{0}\" as a DateTime", strTime);
    }
    Console.ReadLine();
}

Some points to note:

I'm not reinventing the wheel.  The .Net DateTime library is the best way to handle points in time.  (Use TimeSpan for duration)
I'm using custom DateTime format strings for both parsing the string and displaying the parsed DateTime result.
I'm using the TryParse pattern the .Net supplies for converting strings to various built-in types: e.g., int.TryParse, double.TryParse, etc.
In the output, I want to display the parsed time as HH:mm:ss (note that capital H is 24 hour time and lowercase h is 12 hour time).  I have to escape the colons in there because of how string.Format works.  You escape colons using backslashes like this HH\:mm\:ss.  The problem is that you then have to escape the backslashes (or use verbatim strings).  You do that with an extra backslash like this HH\\:mm\\:ss


Find all the digits in a string S is a given character string, then I have to find all the digits characters of S. Did I make it unnecessarily complicated?
bits 32

global start

extern exit
import exit msvcrt.dll

segment data use32 class=data
    s db '+', 'r', '0', '8', '8', '1', 'i', '5'
    len equ $ - s
    digits db '012345789'
    d times len db 0
segment code use32 class=code
    start:
        mov ecx, len

        mov esi,0
        mov ebx,0

        jecxz end_prog

        repeat_this:
            push ecx
            mov al, [s + esi]
            mov ecx, 10
            mov edi, digits
            find:
                scasb
                jz found
            loop find

            jmp next

            found:
                mov edi, ebx
                inc ebx
                mov [d + edi], al

            next:
                inc esi
                pop ecx
        loop repeat_this

        end_prog:

        push    dword 0
        call    [exit]

Did I make it unnecessarily complicated?

You can replace
find:
  scasb
  jz    found
  loop  find
  jmp   next

by the simpler
  repne scasb
  jne   next

The use of the SCASB instruction here depends on the direction flag DF being in the clear state (CLD). It is a safe assumption that this will be the case. Nonethesless prudent programmers will at least at program start include one CLD instruction.
If ever you find that you need to reverse the direction for the string primitives by the use of STD, you should always issue a restoring CLD as soon as possible (once the string operation has finished). Why is this? Well, just as you can expect the direction flag to be clear, any library functions that your program invokes expect the same thing. During normal program execution DF=0 is almost true 100% of the time. See.

No need to use the extra register EDI in:
mov   edi, ebx
inc   ebx
mov   [d + edi], al

Just write:
mov   [d + ebx], al
inc   ebx

The digits have well-known ASCII codes. Finding a digit is just a matter of comparing AL with the range 48 to 57. I find scanning that digits string a bit overkill, although at times that could be a good way. When the character codes aren't contiguous.
  mov   ecx, len
  jecxz end_prog
  xor   esi, esi
  xor   ebx, ebx
repeat_this:
  mov   al, [s + esi]
  cmp   al, '0'
  jb    next
  cmp   al, '9'
  ja    next
  mov   [d + ebx], al
  inc   ebx
next:
  inc   esi
  loop  repeat_this

Clearing a register is best done using XOR <reg>,<reg>.

This shorter version uses the string primitives and avoids using the slow LOOP instruction:
    mov   ecx, len
    mov   esi, s
    mov   edi, d
    jmp   next
  repeat_this:
    lodsb
    cmp   al, '0'
    jb    next
    cmp   al, '9'
    ja    next
    stosb
  next:
    sub   ecx, 1
    jnb   repeat_this


Converting embedded JSON into flat table I am new Python user, who decided to use Python to create simple application that allows for converting json files into flat table and saving the output in cvs format. I was wondering if you could give me some advice how I could improve my code to make it work in more efficient way. I am asking since if I convert relatively small files everything works just fine but when I try to convert ~200 MB file it starts to take a while. I am afraid that when I begin to work with bigger files it might take quite some time to convert my datasets.

Here is my code, which I created with help of this great blog post about flattening json objects :

import sys, os, json, tkFileDialog, tkMessageBox
from Tkinter import *
from pandas.io.json import json_normalize

def openFile():
    currdir = os.getcwd()
    filename = tkFileDialog.askopenfilename(
        initialdir = currdir,
        title='Please select a file',
        filetypes=[('JSON file','.json')])

    return filename

def loading_file(path):
    #File path
    file_path = path

    #Loading json file
    json_data = open(file_path)
    data = json.load(json_data)
    return data

#Function that recursively extracts values out of the object into a flattened dictionary
def flatten_json(data):
    flat = [] #list of flat dictionaries
    def flatten(y):
        out = {}

        def flatten2(x, name=''):
            if type(x) is dict:
                for a in x:
                    if a == "name":
                            flatten2(x["value"], name + x[a] + '_')
                    else:
                        flatten2(x[a], name + a + '_')
            elif type(x) is list:
                for a in x:
                    flatten2(a, name + '_')
            else:
                out[name[:-1]] = x

        flatten2(y)
        return out

#Loop needed to flatten multiple objects
    for i in range(len(data)):
        flat.append(flatten(data[i]).copy())

    return json_normalize(flat)

#Outputing normalized data into csv
def csv_out(data, path):
    #creating csv file name
    name = '~/Desktop/' + os.path.basename(os.path.splitext(path)[0]) + '.csv'
    #converting to the csv
    data.to_csv(name, encoding='utf-8') #'~/Desktop/out.csv'

def done():
   tkMessageBox.showinfo('json2csv',"DONE!")

def main():
    filepath = openFile()
    data_file = loading_file(filepath)
    table = flatten_json(data_file)
    csv_out(table, filepath)
    done()

### Application Interface ###
tk = Tk()

#Creating window:
tk.geometry('250x150+600+300')
tk.title('JSON2CSV')

#Creating convert button
convertbutton = Button(tk, text = 'Convert to .csv', command = main)
convertbutton.place(x = 25, y = 50)

tk.mainloop()

Here you will find short example of the json structure I work with:

[{
 "_id": {
   "id": "123"
 },
 "device": {
   "browser": "Safari",
   "category": "d",
   "os": "Mac"
 },
 "exID": {
   "$oid": "123"
 },
 "extreme": false,
 "geo": {
   "city": "London",
   "country": "United Kingdom",
   "countryCode": "UK",
   "ip": "00.000.000.0"
 },
 "viewed": {
   "$date": "2011-02-12"
 },
 "attributes": [{
   "name": "gender",
   "numeric": 0,
   "value": 0
 }, {
   "name": "email",
   "value": false
 }],
 "change": [{
   "id": {
     "$id": "1231"
   },
   "seen": [{
     "$date": "2011-02-12"
   }]
 }]
}, {
 "_id": {
   "id": "456"
 },
 "device": {
   "browser": "Chrome 47",
   "category": "d",
   "os": "Windows"
 },
 "exID": {
   "$oid": "345"
 },
 "extreme": false,
 "geo": {
   "city": "Berlin",
   "country": "Germany",
   "countryCode": "DE",
   "ip": "00.000.000.0"
 },
 "viewed": {
   "$date": "2011-05-12"
 },
 "attributes": [{
   "name": "gender",
   "numeric": 1,
   "value": 1
 }, {
   "name": "email",
   "value": true
 }],
 "change": [{
   "id": {
     "$id": "1231"
   },
   "seen": [{
     "$date": "2011-02-12"
   }]
 }]
}]

I don't know anything about tkinter, so I'm just going to look at your flatten function.

I'm going to start with some overarching comments. When checking for the type of something, use  isinstance instead of type(x) is some_type. Secondly, when you have a trivial loop of the form

list_ = []
for x in iterable:
    list_.append(function(x))

You almost always want to turn it into a list comprehension

list_ = [function(x) for x in iterable]

You have an unnecessary dict.copy which is definitely going to slow things down if you start having very large files.

You can get rid of the for i in range(len(data)) and just use for item in data.

I don't like that out isn't a local variable/parameter, and I find it a bit hacky that it is modified inside of flatten2 without being passed as a parameter.  I think a better idea is to use an input parameter. This might also help speed things up, as Python accesses locals faster than nonlocals.

Now you also have a minor correctness issue - you only handle dict and list, but what if there were a tuple inside? Some user defined type that quacks like a dict? Admittedly if you're parsing json that is unlikely, but this would be a fairly useful general utility to flatten all sorts of nested mappings.  There are all sorts of ways to handle this, but the easiest might be to ask forgiveness instead of permission

try:
    for key, value in x.items():
        # your dict stuff here
except (ValueError, AttributeError): # oh this doesn't support unpacking/the items() method
    for item in x: # treat it like a list

Not necessarily a perfect solution, but worth considering.

I doubt this is going to help a ton with speed. Short of parallelising this (and I don't think Python would do that effectively with either threads or processes) I'm not seeing a good solution.

One last minor thing - if you have a large and deeply nested json file this might choke with a recursion error. You could fix that by using a list or a queue and keep adding items to it as they're discovered.  Something like this (sketched, may not work right away) might work

to_flatten = [(item, '') for item in y]
for item, name in to_flatten:
    if isinstance(item, dict):
       for key, value in item.items():
            if key == "name":
                to_flatten.append((item["value"], name + item[key] + '_'))
            else:
                to_flatten.append((item[key], name + key + '_'))
    elif isinstance(x, (list, tuple)):
       for member in item:
           to_flatten.append((member, name + '_'))
    else:
        result_dict[name[:-1]] = item

This may also help with speed - I honestly don't know.

One last note - like with most languages, string concatenation is slow in Python. In CPython, the peephole optimizer does some magic with just 2 strings (I wish I could find the source for this) but doesn't for more than that. And according to PEP8,

  For example, do not rely on CPython's efficient implementation of in-place string concatenation for statements in the form a += b or a = a + b. This optimization is fragile even in CPython ... In performance sensitive parts of the library, the ''.join() form should be used instead.

(emphasis and code-formatting added)

You might be able to get a speedup by changing your string concatenation method.

to_flatten.append((item["value"], ''.join([name, item[key], '_'])))


Refactoring a function to return all scores above a certain threshold I am currently learning JavaScript and wanted to fellow coders to review a function that I had written. I am looking for feedback particularly on:

Whether I have followed best practice and JavaScript conventions
Any suggestion on doing it better.

Things I know I need to implement but don't know how:

Validate if array values are numbers

Purpose of function: Return all scores above a certain threshold. Function accepts two arguments. First is an array of scores and second argument is the threshold.

My code:

UPDATED code after putting it through JS lint - I have ignored some erros like moving vars in the loops to the top or not declaring vars in the for loop etc (before JSLint code is here http://jsbin.com/UtOMAyU/1/)

function scoresAbove(scoresArg, thresholdArg) {
    "use strict";
    var output = [],
        nums = scoresArg,
        threshold = thresholdArg;
    //check if scores Arg provided is an array
    if (typeof nums !== "object" || !nums.hasOwnProperty('length')) {
        return "Error: Number(s) not in the expected format. Please provid number(s) in an array format";
    }else if (typeof threshold !== "number") { //check if threshold Arg provided is an array
        return "Error: Threshold number not in the expected format. Please provid a threshold";
    }
    for (var i = 0, len = nums.length; i < len; i++) {
        var currentNum = nums[i];
        if (currentNum > threshold) {
            output.push(currentNum);
        }
    }
    //What to return from func
    if (output == "") {
        return "No scores were above the " + " threshold you set as " + threshold;
    }
    return "The following scores were above the threshold: " + output.join(", ");
}

My 2 cents:

var output should probably be highScores or some such, since it never contains what is returned, nor does it get outputted
nums = scoresArg, threshold = thresholdArg; makes little sense, just use the arguments
function scoresAbove(scoresArg, thresholdArg) , I am not sure what the value is of postpending 'Arg' to the argument names, I would go for simply function scoresAbove(scores, threshold)
The check for Array should really be a separate function, it is completely re-usable
Ditto for Number
For the actual filtering , I would use the built-in filter :

highScores = scores.filter( function(v){ return v > threshold });
Since output is an array, I would not compare with ""
Why the double concatenation here: return "No scores were above the " + " threshold you set as " + threshold;
Arguments of the wrong type should be rare enough that you can throw an exception since something is clearly wrong with the program.

You could try

function scoresAbove( scores, threshold )
{
  "use strict";
  var highScores;

  if( notArray( scores ) )
    throw "scores is not an array";
  if( notNumber( treshold ) )
    throw "treshold is not a number"

  highScores = scores.filter( function(v){ return v > threshold });

  if( !highScores || !highScores.length )
    return "No scores were above the threshold you set as " + threshold;

  return "The following scores were above the threshold: " + output.join(", ");
}

You would have to write the notNumber and notArray function of course.
JSLint migh complain about missing curly braces, to each his own ;)


Data structure to RDF I believe any data structure can be converted to RDF, and I wrote the
subroutine below which sort of does this, at least for arbitrarily
complex hashes.

However, I'm unhappy with it because:

$hash2rdf_count should not be global. I don't see an easy fix for this.
Where I comment "hopefully it's referring to a scalar at this
point!", I'm not actually convinced. Are there better ways to check?
@triplets should not be global. I think I can fix this by passing
it as a parameter, but is there an easier way?

Other notes:

In my specific use case, I'm using JSON::from_json($data) to
convert JSON data to a hash, which I then feed to the subroutine
below. I then print out @triplets and create an SQLite3 DB out of it. So it's really: JSON2hash, hash2rdf, rdf2sql. The entire program: https://github.com/barrycarter/bcapps/blob/master/bc-get-discogs.pl
JSON::GRDDL failed with an out-of-memory error on my data. My
subroutine doesn't. Is there another "prepackaged" solution I could
use?
I realize what I'm doing is a very minimal form of RDF.

=item hash2rdf($ref,$name="")

Given a scalar/array/hash reference $ref, returns RDF triplets recursively.

If $name is given, assigns name $name to $ref. Otherwise, creates a name

Pretty much does what var_dump() does, only in RDF

=cut

sub hash2rdf {
  my($ref,$name) = @_;
  my($type) = ref($ref);

  # if no type at all, just return self (after cleanup)
  unless ($type) {
    $ref=~s/\n/\r/isg;
    $ref=~s/\"/\\"/isg;
    return $ref;
  }

  # name I will give $ref if not given
  unless ($name) {$name = "REF".++$hash2rdf_count;}
  # TODO: making $hash2rdf_count global is bad
  # TODO: making @triplets global is unacceptably bad (but just testing now)
  # my(@triplets);

  if ($type eq "ARRAY") {
    # interim var
    my(@l) = @{$ref};
    # push triplets for my children
    for $i (0..$#l) {push(@triplets, [$name, $i, hash2rdf($l[$i])]);}
    # return the name I gave myself
    return $name;
  }

  if ($type eq "HASH") {
    # interim var
    my(%h) = %$ref;
    for $i (keys %h) {push(@triplets, [$name, $i, hash2rdf($h{$i})]);}
    return $name;
  }

  # hopefully it's referring to a scalar at this point!
  return $$ref;
}

I'm not sure what you're trying to do, so I'll just review the code itself, not what you're trying to do.

I looked into the whole script on GitHub and was shocked to see that you didn't use strict; use warnings. This would usually abort any review, but let's continue for the sake of the argument.

I think it's very nice that you've used POD documentation for your script.

Let's look at this excerpt for a moment:

unless ($type) {
    $ref=~s/\n/\r/isg;
    $ref=~s/\"/\\"/isg;
    return $ref;
}

ref $foo can be false, but a reference (consider $foo = bless \$bar, "0"). Use either length ref $foo or reftype $foo, where reftype is imported from Scalar::Util.
unless is widely considered to be half a design error. if (not ...) is cleared most of the time.
On each of your regexes you specify the flags /isg.

The /i does case insensitive matching  but you don't match anything that has a case. As this flag reduces performance, you should use it sparingly.
The /s flag causes the . to match any character including newlines. It is generally a good idea to use this flag (the default behavior of . is better written as [^\n]). However, you do not use the . metacharacter, so /s is useless here.

In the pattern, " does not have to be escaped as it isn't a metacharacter.
Use proper spacing around operators (usually a single space) to improve readability.

Suggested refactoring:

if (not length $type) {
    $ref =~ s/\n/\r/g;
    $ref =~ s/"/\\"/g;
    return $ref;
}

Now let's take a look at unless ($name) {$name = "REF".++$hash2rdf_count;}:

As per Larry's style guide, you shouldn't use a semicolon if the closing brace is on the same line.
Never use the conditional (...) {BLOCK} form on a single line. Either put the statement on a line of its own, or use the statement modifier form:

$name = "REF" . ++$hash2rdf_count unless length $name;

or

if (not length $name) {
    $name = "REF" . ++$hash2rdf_count;
}

If you only want to overwrite $name if it is undef, you could either test for definedness: if (not defined $name) or since perl 5.10, you can use the defined-or operator //:

$name //= "REF" . ++$hash2rdf_count;

What to do about the $has2rdf_count? The simplest solution that does not involve globals is to declare it outside of the subroutine:

my $hash2rdf_count = 0;
sub hash2rdf {

A better solution (since perl 5.10) is to use state variables:

use feture 'state';
...
state $hash2rdf_count = 0;
$name //= "REF" . ++$hash2rdf_count;

A state variable belongs to a certain scope like my variables, but is initialized only at the first execution.

However, it may be better to take this as another argument (will be discussed later).

my(@l) = @{$ref}  there is no need to perform a copy of the array, and even if you feel the need to do that, please don't use a bad variable name like @l. Especially the charcterl l should be avoided on its own. Also, variable names don't have to be so short. Given an array reference, we can access an entry like $ref->[$i] and get the highest index like $#$ref (or with curlies: $#{$ref}).

By now we know how a line like for $i (0..$#l) {push(@triplets, [$name, $i, hash2rdf($l[$i])]);} can be improved. But I would like you to call builtins like push without parens: They aren't subroutines, but operators.

With a statement modifier:

push @triplets, [$name, $_, hash2rdf($ref->[$_])] for 0 .. $#$ref;

Without:

for my $i (0 .. $#ref) {
    push @triplets, [$name, $i, hash2rdf($ref->[$i])];
}

And then there is a variant with map:

push @triplets, map { [$name, $_, hash2rdf($ref->[$_])] } 0 .. $#$ref;

The same holds for the HASH-reference case. Recommended refactoring there:

for my $key (keys %$ref) {
    push @triplets, [$name, $key, hash2rdf($ref->{$key})];
}

Note that I'm not using $i here, as the hash key won't generally be an integer.

How can we solve the problem that @triplets is a global? We take a reference to that array as another argument:

sub hash2rdf {
    my ($ref, $triplets, $name) = @_;
    ...
    push @$triplets, ...

This means that we have to change the way how we call ourselves when recursing:

hash2rdf($ref->[$i], $triplets)

On the outside, we call the subroutine like

hash2rdf(\@releases, \my @triplets, "root");

Eww, that is ugly (out-arguments always bother me). Let's rename hash2rdf to hash2rdf_inner, and create a wrapper. This is also a good opportunity to pass $has2rdf_count as an argument, and to rename the wrapper to something better.

sub ref2rdf {
  my ($data, $name) = @_;
  my $count = 0;
  my @triplets;
  ref2rdf_inner($data, \@triplets, $name, \$count);
  return @triplets;
}

sub ref2rdf_inner {
  my ($ref, $triplets, $name, $count_ref) = @_;
  ...
  $name //= "REF" . ++$$count_ref;
  ...
  ref2rdf_inner($ref->[$i], $triplets, undef, $count_ref)
  ...
}

We could now dump a data structure like

use feature 'say'; # say is like print, but with newline

for my $triplet (ref2rdf(\@releases, "root")) {
  say join "\t", @$triplet;
}

Happy refactoring!

Edit: This code still has unresolved issues, e.g. hash keys can contain arbitrary strings which may include NUL, LF, tabs, spaces, or quotes. Those would have to be properly sanitized as well.


Basic orbiting planets in P5.js I started studying JavaScript about a week ago and I would like some opinions and advice on my code. This program just creates a canvas and instantiates 5 planets that orbit around a sun, giving them each a different speed, color, and size.

It uses the p5.js library, the code can be run in the p5.js web editor. (You might want to expand the preview panel on the right after pasting the code, before you run the script.)

The code is in different files (class Orbiter, extra math functions and main code) but I'll put it here in one go:

// Converts from degrees to radians.
Math.radians = function(degrees) {
    return degrees * Math.PI / 180;
  };

// Converts from radians to degrees.
Math.degrees = function(radians) {
    return radians * 180 / Math.PI;
  };

class Orbiter {

    constructor(rad, orbitAlt, x = 0, y = 0, orbitAngle = 0, orbitAngleMod = 1, colorR=255, colorG=255, colorB=255, colorA=255){
        this.orbitAngle = orbitAngle; // Angle formed by the radius of the orbit and the x plane.
        this.orbitAngleMod = orbitAngleMod; // Increment/decrement of  orbitAngle
        this.rad = rad; // Radius
        this.orbitAlt = orbitAlt; // Distance to the orbited object's position (Alt for altitude)

        // Position
        this.x = x;
        this.y = y;

        // Color variables
        this.colorR = colorR;
        this.colorG = colorG;
        this.colorB = colorB;
        this.colorA = colorA;
    }

    orbit(object){
        this.x = object.x + this.orbitAlt * cos(Math.radians(this.orbitAngle));
        this.y = object.y + this.orbitAlt * sin(Math.radians(this.orbitAngle));
        this.orbitAngle = this.orbitAngle + this.orbitAngleMod;

        // Reset the angle to 0 after a complete revolution to avoid an ever increasing value.
        if(this.orbitAngle >= 360){
            this.orbitAngle = 0;
        }
    }

    display(){
        noStroke();
        fill(this.colorR, this.colorG, this.colorB, this.colorA);
        return ellipse(this.x, this.y, this.rad, this.rad);
    }

}

let planets = [];
let sun = new Orbiter(100, 0);

function setup(){
    createCanvas(windowWidth-3, windowHeight-3);
    frameRate(144);

    // Set up the Sun's colors and coordinates
    sun.colorR = 255;
        sun.colorG = 200;
        sun.colorB = 0;
    sun.x = windowWidth/2;
    sun.y = windowHeight/2;

    // Instantiate 5 planets
    for(i = 0; i < 5; i++){
        planets[i] = new Orbiter(5 + i * 15, 110 + i*70);
        planets[i].orbitAngleMod= 1.4 - i/7;
        planets[i].orbitAngle= i*5;

        planets[i].colorR = i * 50 + 5;
        planets[i].colorG = 255 - planets[i].colorR;
        planets[i].colorB = 255 - planets[i].colorR;
    }
}

function draw(){
    background(0, 10, 40);

    for(planet of planets){
        planet.orbit(sun);
        planet.display();
    sun.display()
    }

}
<script src="https://cdnjs.cloudflare.com/ajax/libs/p5.js/0.7.3/p5.js"></script>

I'd be really grateful if you could give me some feedback on the structure of my code, whether it's in line with JavaScript best practices, or in general if there's anything you see in there that you think is just wrong or should be written in a different way.

Use of p5.js and JavaScript

Math is part of the standard JavaScript library.  You shouldn't pollute its namespace with your own code, because it could interfere with other JavaScript code on the page.  In this case, Math.degrees() is never used, and you could just use p5.js's radians() function instead of writing Math.radians().

The constructor for Orbiter is very unwieldy, with up to 10 parameters.  You can just eliminate most of them.  Furthermore, you should specify colors as a single p5.Color object rather than as four separate parameters.

There should be a windowResized() handler, so that the window (or the Stack Snippet) can be resized gracefully.

Physics and animation

The object around which a satellite orbits is called its primary.  I suggest that you name the parameter to the orbit() method accordingly.

Your planets' angular velocity is given by planets[i].orbitAngleMod= 1.4 - i/7, which is not consistent with the laws of physics.  According to Kepler's Third Law, \$T^2 \propto r^3\$ (where \$T\$ and \$r\$ are the orbital period and radius), so \$\omega \propto r^{-\frac{3}{2}}\$.  So, your inner planets are not as fast as they should be, and your outer planets are not as slow as they should be.

Handling the revolution completion with if(this.orbitAngle >= 360){ this.orbitAngle = 0; } can lead to round-off glitches.  A modulo operation would be more appropriate.

I don't see the point of initializing the initial angles of the planets to i * 5 — an offset of up to 4° relative to the x axis is hardly visible.

class Orbiter {
    constructor(sizeRadius, orbitRadius, orbitAngle=0) {
        this.sizeRadius = sizeRadius;
        this.orbitRadius = orbitRadius;
        this.orbitAngle = 0; // degrees relative to x axis

        // 2000 is an arbitrary animation speed (which also depends on the frame rate)
        // The -1.5 exponent is due to Kepler's 3rd Law
        this.orbitAngleDelta = 2000 * Math.pow(orbitRadius, -1.5);
        this.x = this.y = 0;
        this.color = 'white';
    }

    orbit(primary) {
        this.x = primary.x + this.orbitRadius * cos(radians(this.orbitAngle));
        this.y = primary.y + this.orbitRadius * sin(radians(this.orbitAngle));
        this.orbitAngle = (this.orbitAngle + this.orbitAngleDelta) % 360;
    }

    display() {
        noStroke();
        fill(this.color);
        return ellipse(this.x, this.y, this.sizeRadius, this.sizeRadius);
    }
}

let planets = [];
let sun = new Orbiter(100, 0);

function setup() {
    createCanvas(windowWidth - 3, windowHeight - 3);
    frameRate(144);

    sun.x = windowWidth / 2;
    sun.y = windowHeight / 2;
    sun.color = color(255, 200, 0);

    // Instantiate 5 planets
    for (i = 0; i < 5; i++) {
        planets[i] = new Orbiter(5 + 15 * i, 110 + 70 * i);
        let red = i * 50 + 5;
        planets[i].color = color(red, 255 - red, 255 - red);
    }
}

function windowResized() {
    resizeCanvas(windowWidth - 3, windowHeight - 3);
    sun.x = windowWidth / 2;
    sun.y = windowHeight / 2;
}

function draw() {
    background(0, 10, 40);
    sun.display()
    for (planet of planets) {
        planet.orbit(sun);
        planet.display();
    }
}
<script src="https://cdnjs.cloudflare.com/ajax/libs/p5.js/0.7.3/p5.js"></script>


Java NIO server Selector loop I have a server that uses Java NIO in non-blocking mode. Right now I'm using a single thread for the selector, and four worker threads to process the bytes after the selector reads them in. I'm going to switch to a thread pool soon enough. Anyway, that's not why I'm here.

Take a look at the code below. I have a state object of type CommunicationStatus called commsState which maintains various parameters relating to the SocketChannel, the number of bytes read, the current working state, whether the message is complete or not, the ByteBuffer for that particular SocketChannel, etc. This works well. The CommunicationStatus object is attached to the SelectionKey when the SocketChannel is registered with the Selector.

When I'm checking key.readyOps() to check what state the channel is in, I have an additional check within the OP_READ and OP_WRITE checks to make sure that the worker threads have finished processing the work before additional bytes are read from or written to the SocketChannel.
So for example, if ((key.readyOps() & SelectionKey.OP_READ) == SelectionKey.OP_READ) returns true but if (commsState.getCurrentState() == CommunicationStatus.STATE_READ) returns false then no bytes will be read at this time.
I find this works really well, but is it the correct way to do things?
Is there a better way I could handle this?
Any other comments or thoughts are appreciated too.

private boolean running;
private ServerSocketChannel serverSocketChannel;

@Override
public void run() {

    try {

        Selector selector = Selector.open();
        serverSocketChannel = ServerSocketChannel.open();
        serverSocketChannel.configureBlocking(false);
        ServerSocket serverSocket = serverSocketChannel.socket();
        serverSocket.bind(new InetSocketAddress(serverPort));

        log.info("Selector Thread: FirmwareServer " + FirmwareServer.SERVER_VERSION
                + " Runnable- Listening for connections on port: " + serverSocket.getLocalPort());

        running = true;

        @SuppressWarnings("unused")
        SelectionKey serverAcceptKey = serverSocketChannel.register(selector, SelectionKey.OP_ACCEPT);

        while (running) {

            try {

                selector.select();
                Set<SelectionKey> selectedKeys = selector.selectedKeys();
                Iterator<SelectionKey> keyIterator = selectedKeys.iterator();

                while (keyIterator.hasNext()) {

                    SelectionKey key = (SelectionKey) keyIterator.next();
                    keyIterator.remove();

                    if (!key.isValid()) {
                        continue;
                    }
                    CommunicationStatus commsState = (CommunicationStatus) key.attachment();

                    if ((key.readyOps() & SelectionKey.OP_ACCEPT) == SelectionKey.OP_ACCEPT) {

                        acceptConnection(selector, key);

                    } else if ((key.readyOps() & SelectionKey.OP_READ) == SelectionKey.OP_READ) {

                        if (commsState.getCurrentState() == CommunicationStatus.STATE_READ) {

                            commsState.setSelectionKey(key);
                            readFromSocketChannel(key);

                        }
                    } else if ((key.readyOps() & SelectionKey.OP_WRITE) == SelectionKey.OP_WRITE) {

                        if (commsState.getCurrentState() == CommunicationStatus.STATE_WRITE) {

                            commsState.setSelectionKey(key);
                            writeToSocketChannel(key);

                        }
                    }
                }

            } catch (IOException e) {
                log.error("Selector IOException Occurred", e);
            }
        }

    } catch (IOException e) {

        log.error("Firmware Selector Thread: An IOException occurred", e);
        System.exit(-1);

    }

}

Iterator<SelectionKey> keyIterator = selectedKeys.iterator();

            while (keyIterator.hasNext()) {

                SelectionKey key = (SelectionKey) keyIterator.next();

This cast here is not needed; you already know that you're going to get SelectionKey out of the keyiterator.

Looking up a bit, you declare Set<SelectionKey> selectedKeys = selector.selectedKeys();, but only use it for the iterator. The variable name doesn't add anything to understanding the code, and you might just want to say Iterator<SelectionKey> keyIterator = selector.selectedKeys().iterator();

(key.readyOps() & SelectionKey.OP_ACCEPT) == SelectionKey.OP_ACCEPT

This sort of statement, I'd wrap in a function. Bit manipulation tends to be very low level stuff and looks out of place in a function responsible for doing high level stuff.


Run length encoding in C# This is the original question:
https://www.geeksforgeeks.org/run-length-encoding/

  Given an input string, write a function that returns the Run Length
  Encoded string for the input string. For example, if the input string
  is “wwwwaaadexxxxxx”, then the function should return “w4a3d1e1x6”.

the only change I did is that if the character appears once there is no need to print 1 after the letter.

Please review for perfomance

using System;
using System.Text;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace StringQuestions
{
    /// <summary>
    /// https://www.geeksforgeeks.org/run-length-encoding/
    /// </summary>
    [TestClass]
    public class RunLengthEncoding
    {
        [TestMethod]
        public void RunLengthEncodingTest()
        {
            string str = "wwwwaaadexxxxxxywww";
            string expected = "w4a3dex6yw3";
            Assert.AreEqual(expected, RLE.Encode(str));
        }
    }

    public class RLE
    {
        public static string Encode(string str)
        {
            str = str.ToLower();
            StringBuilder stringBuilder = new StringBuilder();

            for (int i = 0; i < str.Length; i++)
            {
                int counter = 1;
                if (!char.IsLetter(str[i]))
                {
                    throw new ArgumentException("string should contains only letters");
                }

                while (i < str.Length - 1 && str[i] == str[i + 1])
                {
                    counter++;
                    i++;
                }

                if (counter == 1)
                {
                    stringBuilder.Append(str[i]);
                }
                else
                {
                    stringBuilder.Append(str[i]);
                    stringBuilder.Append(counter);
                }
            }

            return stringBuilder.ToString();
        }
    }
}

One thing I noticed.  You're modifying the loop counter inside the loop to run another loop.  This is one way in a large project to create unseen bugs. By keeping track of the previous character it is fairly easy to code this with only one loop.

Your trapping strings that have other characters besides letters, but you're neglecting empty strings and null.

Here's one way the revised code can look:

static string Encode(string input)
{
    if (input == null || input.Length == 0)
    {
        ArgErrHandler("String must have at least one letter.");
    }
    if (input.Any(x => !char.IsLetter(x)))
    {
        ArgErrHandler("String must have only letters.");
    }
    if (input.Length == 2 || input.Length == 1)
    {
        return (input.Length == 1 || input[0] != input[1]) ? input : $"{input[0]}{2}";
    }
    int limit = input.Length - 1;
    int counter = 1;
    StringBuilder sb = new StringBuilder();
    int i = 1;
    char prev = '\0';
    do
    {

        prev = input[i - 1];
        if (input[i] == prev)
        {
            ++counter;
        }
        else
        {
            if (counter > 1)
            {
                sb.Append($"{prev}{counter}");
            }
            else
            {
                sb.Append(prev);
            }
            prev = input[i];
            counter = 1;
        }
    } while (++i < limit);
    if (input[i] == prev)
    {
        ++counter;
        sb.Append($"{prev}{counter}");
    }
    else
    {
        if (counter > 1)
        {
            sb.Append($"{prev}{counter}{input[i]}");
        }
        else
        {
            sb.Append($"{prev}{input[i]}");
        }
    }
    return $"{sb}";
}
private static void ArgErrHandler(string message)
{
    throw new ArgumentException(message);
}


Scanning multiple huge files in Python I'm trying to implement an algorithm able to search for multiple keys through ten huge files in Python (16 million of rows each one). I've got a sorted file with 62 million of keys, and I'm trying to scan each ten file in dataset to look for a key and its value.

Sorted file key:

a
b
c
d
...

Each dataset file appears like this:

a 1
d 10
g 3
...

Output (if key does not appear in file set val to 0):

<sum_vals>,a,<val_1_file>,<val_2_file>,...,<val_10_file>
<sum_vals>,b,<val_1_file>,<val_2_file>,...,<val_10_file>
....

Here it is my Python algorithm. I scan file containing key and for each match I take its value. For each line in dataset where dataset is an array containing name of files, I'm scanning and creating an output line with 0 if key is not in and <val_in_file> if it matches.  Everything is working fine on a small set of training data, but it takes long and epic time to process on real data.

import os,sys;

files = ["20140601","20140602","20140603","20140604","20140605","20140606","20140607","20140608","20140609","20140610"]

def printToFile(text):
  f = open('processed/20140601','a')
  f.write(text)
  f.close()

def getKey(line):
  data=line.split(" ")
  return data[0]+ " "+ data[1]

def getPageSource(line):
  data=line.split(" ")
  return data[0]

def getPageTitle(line):
  data=line.split(" ")
  return data[1]

def getPageClicks(line):
  data=line.split(" ")
  return data[2]

def searchWindow(dataset,key):
  isHere = 0;
  total = 0;
  acc = getPageTitle(key)
  for f in dataset:
    line_with_keyword = next((line for line in open("dataset/"+f) if key in line),None)
    if line_with_keyword is not None:
        isHere = 1
        click = getPageClicks(line_with_keyword)
    if(isHere == 1):
        acc = acc.strip('\n') + "," + click
        total += int(click)
        isHere = 0
    else:
        acc = acc.strip('\n') + "," + str(0)
        total += 0
        isHere = 0
    printToFile(str(total)+","+acc.strip('\n')+","+"\n")

with open("processed/sorted_keys") as inSortedKey:
  for line in inSortedKey:
    searchWindow(files,getKey(line).strip("\n"));

Some smaller adjustments

By just quickly glancing on the code I would suggest the following actions:

Move the opening of the output file to where you open the inSortedKey file, and let it be open until finished processing the files. Opening and closing files is rather expensive.
Instead of splitting the line multiple times, do it once in the start of searchWindow() and use the different elements when you need them. Possibly skip the intermediate variables all together
Simplify the logic related to the isHere if statements, as you do the same in both cases only that in the second case click will always be 0. Thusly eliminate the duplication of code here
Consider using another level of with open(...) and for another_line instead of the next((line for line ... )), and do early return in that loop to skip lines as soon as possible

A little style advise to close off: Remember that the Python style guide indicates to use snake_case for both variables and function names. :-)

Multiple reads of dataset

For each line in inSortedKey file you read all lines from each of the files in the dataset. That is a lot of extra reading of files!!! Lets say that you have \$n\$ lines in inSortedKeys and \$m\$ files in the dataset, where each file has approximately \$p\$ lines. Then your code does \$O(n\cdot m\cdot p)\$ operations within searchWindows.

With your numbers being: \$n=62.000.000\$, \$m=10\$, and \$p=16.000.000\$.

If you had been able to preread the inSortedKey and extract the keys into a memory table, and then do a single read on each of the other dataset files matching each line against this precomputed key list this number would drop to \$O(n + m \cdot p)\$. That would really speed up your operations, but it does depend on the size of inSortedKey.

Even if you are not able to load all of inSortedKey you would be much better of reading as large a chunk as you are comfortable with into memory, before doing a full read on the dataset. Say you read 100000 lines of inSortedKeys into memory, you would look at \$O(\frac{n}{100000} \cdot m \cdot p)\$ operations doing a slightly heavier operation each time. It would still be a massive improvement.

Consider switching to databases

If you're actually talking about cross-referencing 62 million keys against 160 millions lines in the dataset files, you should seriously consider using databases.

Either permanently, or temporarily. You would most likely get a performance gain if you imported the entire set into a database, did your operations, and wrote the output rather than doing this with plain file operations. Python has excellent support for sqlite or pandas, in addition to multiple other modules and databases, which might help you.

See 200_success' answer for a somewhat related discussion on massive file sizes.


Writing to an HttpURLConnection in a loop I have this method which sends binary data to server. The code works fine, but still, it's structure is kinda stupid: output and input connections are opened like thousands times (depends on data size), closed only once.

I cannot move the code that opens connection outside of the loop because I'm getting strange errors, like the data was never sent to server.

Please help me to restructure this code so it could still work and have a correct structure:

OutputStream os = null;
StringBuffer messagebuffer = new StringBuffer();
HttpURLConnection huc = null;
DataInputStream dis = null;
InputStream in = null;
Path path = null;
byte[] buf = new byte[64 * 1024];
int bytesRead = 0;

try {
  path = Paths.get("D:\\testfile.rar");
  in = Files.newInputStream(path);

  int i = 0;
  URL u = new URL(defaultURL);

  while ((bytesRead = in.read(buf)) != -1) {

    huc = (HttpURLConnection) u.openConnection(); // wrong
    huc.setRequestMethod("POST");
    huc.setRequestProperty("chunk-number", i + "");
    huc.setDoOutput(true); // wrong
    huc.setDoInput(true); // wrong
    os = huc.getOutputStream(); // wrong
    os.write(buf, 0, bytesRead);
    os.flush();
    System.out.println(i++ + " " + bytesRead);

    int ch;
    dis = new DataInputStream(huc.getInputStream()); // wrong

    long len = huc.getContentLength();
    if (len != -1) {
      for (int k = 0; k < len; k++)
        if ((ch = dis.read()) != -1)
          messagebuffer.append((char) ch);
        else {
          // if the content-length is not available
          while ((ch = dis.read()) != -1)
            messagebuffer.append((char) ch);
          }
    }
    Thread.sleep(16);
  }
  dis.close();
  huc.disconnect();

  int statusCode = huc.getResponseCode();
  String message = huc.getResponseMessage();
  messagebuffer.append("status code=" + statusCode + "\n");
  messagebuffer.append("response message=" + message + "\n");

} catch (Exception ex) {
  // throw errors
} finally {
  // closing stuff
}
return messagebuffer.toString();

In my opinion the key way to improve this code is decomposition. Actually, methods with more than 15 java lines are usually smells for me. So

//There should be other signature depending on what does this method actually do
public static String foo(Path path) throws InterruptedException, IOException {
    final StringBuffer messagebuffer = new StringBuffer();
    final byte[] buf = new byte[64 * 1024];
    InputStream in = null;
    try {
        in = Files.newInputStream(path);
        int bytesRead;
        int i = 0;
        while ((bytesRead = in.read(buf)) != -1) {
            dealWithConnection(bytesRead, messagebuffer, buf, i);
            i++;
            Thread.sleep(16);
        }
    } finally {
        closeQuietly(in);
    }
    return messagebuffer.toString();
}

private static void dealWithConnection(int bytesRead, StringBuffer messagebuffer, byte[] buf, int i) throws IOException {
    final URL u = new URL(defaultURL);
    HttpURLConnection huc = null;
    try {
        huc = (HttpURLConnection) u.openConnection();// wrong
        init(huc, i);
        send(huc, buf, bytesRead);
        fillData(huc.getInputStream(), huc.getContentLength(), messagebuffer);
        fillResultInfo(huc, messagebuffer);
    } finally {
        if (huc != null) huc.disconnect();
    }
}

private static void send(HttpURLConnection huc, byte[] buf, int bytesRead) throws IOException {
    OutputStream os = null;
    try {
        os = huc.getOutputStream(); // wrong
        os.write(buf, 0, bytesRead);
        os.flush();
    } finally {
        closeQuietly(os);
    }
}

private static void fillResultInfo(HttpURLConnection huc, StringBuffer messagebuffer) throws IOException {
    int statusCode = huc.getResponseCode();
    String message = huc.getResponseMessage();
    messagebuffer.append("status code=").append(statusCode).append("\n");   //Don't use concatenation when you can append
    messagebuffer.append("response message=" + message + "\n");
}

private static void fillData(InputStream inputStream, int contentLength, StringBuffer messagebuffer) throws IOException {
    DataInputStream dis = null;
    try {
        dis = new DataInputStream(inputStream); // wrong

        //if (contentLength != -1) {    //we don't need this check actually
        int ch;
        for (int k = 0; k < contentLength; k++)
            if ((ch = dis.read()) != -1)
                messagebuffer.append((char) ch);
            else {
                // if the content-length is not available
                while ((ch = dis.read()) != -1)
                    messagebuffer.append((char) ch);
            }
    } finally {
        closeQuietly(dis);
    }
}

private static void init(HttpURLConnection huc, int i) throws ProtocolException {
    huc.setRequestMethod("POST");
    huc.setRequestProperty("chunk-number", i + "");
    huc.setDoOutput(true);
    huc.setDoInput(true); // wrong
}

If you haven't closeQuietly() in your libs, it can be implemented like

private static void closeQuietly(Closeable closeable) {
    if (closeable == null) return;
    try {
        closeable.close();
    } catch (IOException ignored) {
    }
}

It's not so easy task to figure out what does this code do. So further improvement can be easily done. Personally I would start from variables names.

BTW, comments like if the content-length is not available is good signal that there should be some method/variable for this to make code be self documented.


Project Euler Problem 57 in Haskell I'm very new to Haskell, and I would like some feedback on my coding style. Is there anything here that could be made more elegant or concise?

This is my solution to Project Euler Problem 57.

  It is possible to show that the square root of two can be expressed as an infinite continued fraction.

  $$\sqrt 2 = 1 + \dfrac{1}{2 + \dfrac{1}{2 + \dfrac{1}{2 + \ldots}}} = 1.414213\ldots$$

  By expanding this for the first four iterations, we get:

  1 + 1/2 = 3/2 = 1.5

  1 + 1/(2 + 1/2) = 7/5 = 1.4

  1 + 1/(2 + 1/(2 + 1/2)) = 17/12 = 1.41666...

  1 + 1/(2 + 1/(2 + 1/(2 + 1/2))) = 41/29 = 1.41379...

  The next three expansions are 99/70, 239/169, and 577/408, but the eighth expansion, 1393/985, is the first example where the number of digits in the numerator exceeds the number of digits in the denominator.

  In the first one-thousand expansions, how many fractions contain a numerator with more digits than denominator?

numerator =
  3 : 7 : zipWith (+) numerator (map ((*) 2) (tail numerator))

denominator =
  2 : 5 : zipWith (+) denominator (map ((*) 2) (tail denominator))

numDigits :: Integer -> Int
numDigits = length . show

moreDigits :: Integer -> Integer -> Bool
moreDigits x y = (numDigits x) > (numDigits y)

countTrue :: [Bool] -> Int
countTrue = length . filter ((&&) True)

pe_057 = countTrue $
  take 1000 (zipWith moreDigits numerator denominator)

main :: IO ()
main = do
    print pe_057

I like the way you defined numerator and denominator.

I think that using length . show to compute the number of digits is a bit overkill. Do you really need to convert the number to a string to do that? I think that using a logarithm is better.

log10 = logBase 10
numDigits = ceiling . log10 . (+) 1

I don't think that moreDigits is a good name. What about longerThan?

countTrue style doesn't look too functional. What about count :: (a -> bool) -> [a] -> Int? In that way you'll have a function you can use to count the number of items matching a predicate in any list, instead of having to compute first the a list of boolean values.

If you use count you'll need to rearrange change moreDigits :: (Int, Int) -> bool and pe_057.

pe_057 = countTrue moreDigits $
  take 1000 (zip numerator denominator)

I also implemented an alternative solution. The main difference with respect to yours is that I generare a list of (numerator, denominator) pairs instead of having two separate lists.

nextFraction :: (Integer, Integer) -> (Integer, Integer)
nextFraction (n, d) = (2*d + n, d + n)

generator :: [(Integer, Integer)] -> [(Integer,Integer)]
generator (x: xs) = nextFraction x : generator xs

fractions :: [(Integer,Integer)]
fractions = (3,2) : generator fractions

numeratorLonger :: (Integer,Integer) -> Bool
numeratorLonger (n,d) = (numDigits n) > (numDigits d)

numDigits :: Integer -> Int
numDigits n = numDigitsImpl (div n 10) 1

numDigitsImpl :: Integer -> Int -> Int
numDigitsImpl n counter = if next == 0
                             then counter
                             else numDigitsImpl next (counter + 1)
                          where next = div n 10

count :: (a -> Bool) -> [a] -> Int
count f xs = length $ filter id $  map f xs

projectEuler57 n = count numeratorLonger $ take n fractions

main :: IO ()
main = do
  print $ projectEuler57 1000


Shortest regex search and sum for integers Given a text file including different text and integer numbers (text.txt), what's the shortest way of getting the sum of all the numbers out if it? I have:

import re

f = open('text.txt', 'r')
result = 0
for line in f.readlines():
    for value in re.findall('[0-9]+', line):
        result += int(value)
f.close()
print(result)

Which works, but I would like to understand what are the possibilities to make it shorter?

You can use map and sum. It good practice to use with statement when working with  a file.

with open('text.txt', 'r') as f:
    result = sum(map(int, re.findall(r'[0-9]+', f.read())))

print(result)


Comparing columns in pandas different data frames and fill in a new column I have two dataframes:
One contains of company and its corresponding texts. The texts are in lists

**supplier_company_name   Main_Text**

JDA SOFTWARE          ['Supply chains','The answer is simple -RunJDA!']

PTC                    ['Hello', 'Solution']

The second dataframe is texts extracted from the company's website.

      Company            Text
0   JDA SOFTWARE    About | JDA Software
1   JDA SOFTWARE    833.JDA.4ROI
2   JDA SOFTWARE    Contact Us
3   JDA SOFTWARE    Customer Support
4   PTC             Training
5   PTC             Partner Advantage

I want to create the new column in second dataframe if the text extracted from the web matches with the any item inside the list in the Main_Text column of the first data frame, fill True else fill False.

Code:

target = []
for x in tqdm(range(len(df['supplier_company_name']))): #company name in df1
    #print(x)
    for y in range(len(samp['Company']): #company name in df2
        if samp['Company'][y] == df['supplier_company_name'][x]: #if the company name matches
            #check if the text matches
            if samp['Company'][y] in df['Main_Text'][x]:
                target.append(True)
            else:
                target.append(False)

How can I change my code to run efficiently?

I’ll take the hypothesis that your first dataframe (df) has unique company names. If so, you can easily reindex it by said company name and extract the (only one left) Main_Text Series to make it pretty much like a good old dict:

main_text = df.set_index('supplier_company_name')['Main_Text']

Now we just need to iterate over each line in samp, fetch the main text corresponding to the first column and generate a truthy value based on that and the second column. This is a job for apply:

target = samp.apply(lambda row: row[1] in main_text.loc[row[0]], axis=1)


Improving logic for pulling and passing data(); I'm pulling data from a list of images and passing it into an information container after click. After looking at my logic I assume there is a better way to do this. Any ideas would be helpful.

var getData = function() {
  $('.guest-image').html('');
  if ($($this).data("known1") != undefined) {
    $('.guest-data .known-for li').eq(0).html('');
  }
  if ($($this).data("known2") != undefined) {
    $('.guest-data .known-for li').eq(1).html('');
  }
  if ($($this).data("known3") != undefined) {
    $('.guest-data .known-for li').eq(2).html('');
  }
  $('.guest-data .title').text($($this).data("name"));
  $('.guest-data .desc').text($($this).data("desc"));
  $('.guest-data .link').text($($this).data("link"));
  $('.guest-data .link').attr('href', $($this).data("link"));
}

squished and optimized, the code looks like this:

function getData() {
  var guestData = $('.guest-data'),
    knownFors = $('.known-for li', guestData),
    data = $($this).data();
  $('.guest-image').html('');
  $.each(data.knowns, function (i, entry) {
    knownFors.eq(i).html('')
  });
  $('.title', guestData).text(data.name);
  $('.desc', guestData).text(data.desc);
  $('.link', guestData).text(data.link).attr('href', data.link)
}

and here's the explanation

//named functions are better in debugging
function getData() {

  //cache frequently used elements
  var guestData = $('.guest-data'),

  //providing a query context limits jQuery to locate a selector only under the context
  //so in this code, we find .known-for li only under guestData which we already cached
  //instead of traversing the DOM again for each find
    knownFors = $('.known-for li', guestData),

    //jQuery data can be fetched as a JS object by a parameter-less .data()
    data = $($this).data();

    //by informal convention, variables prefixed by $ usually means it
    //references a jQuery object. You may not need wrap it again with
    //a $() call if you are sure it's already a jQuery object

  //there are two ways to build elements in jQuery

  //for performance, string assembly would be faster
  $('.guest-image').html('');

  //for readability, but slow, the jQuery element builder method
  $('.guest-image').html($('', {
    'src': data.image
  }));

  //I also have to note as to why .guest-image was not cached
  //it's simply because it is only used once in the function

  //for your "knowns", I suggest placing them in an array and in order
  //so that you can easily iterate through and attach to your html

  //example of the structure
  /*
  data.knowns = [
    {
      known : 'known here',
      title : 'title here'
    },{
      known : 'known here',
      title : 'title here'
    }
  ];
  */

  //looping through the structure
  $.each(data.knowns, function (i, entry) {

    //string assembly method
    knownFors.eq(i).html('')

    //builder method
    knownFors.eq(i).html($('', {
      'src': entry.known,
      'data-title': entry.title
    }));

  });

  $('.title', guestData).text(data.name);
  $('.desc', guestData).text(data.desc);

  //lastly, some jQuery functions return the element they operated on
  //this makes chaining possible.
  $('.link', guestData).text(data.link).attr('href', data.link);
}

Here's a JS perf performance test. Compared to your code, the builder method is more readable, verbose but 23% slower. The string assembly method is 7% faster than your code.

Seems like it was faster only on my Firefox 18 and Firefox 21 Android (Nightly) and was too eager to conclude.


Custom UI - Seeing which control has focus I'm trying to make a simple UI to better understand how interfaces and the System.Windows.Forms Controls behave.

Like the above namespace, I will be able to get and set the Focus on a particular control, with a helper method to drill down to see which Control (or child Control) actually has the focus. I'm pretty rubbish for recusion.

Here's what I have so far:

public class Control
{
    public bool CanFocus { get; set; }
    public bool HasFocus { get; set; }

    public Control GetFocus()
    {
        if (HasFocus)
            return this;

        foreach (Control c in Children)
        {
            if (c.HasFocus)
                c.GetFocus();
        }

        return null;
    }

}

My thought process is that it should return the immediate control if it has focus. If not, it will recurse through each of its children. If that child has the focus, it will return that child, and if not, will find the next one, repeating until a default case (null) is reached.

I had thought about a ContainsFocus field as well, but it "sort of" overlaps with what I'm doing here. I considered a helper method with the signature bool FindFocus(out Control result), but it felt unwieldy. Also, I am concerned with invoking foreach within a recursive function.

I appreciate any insight into this.

Here's what I have so far

The code you wrote doesn't quite work properly; because, depending on the behaviour (which isn't clearly defined) of your HasFocus property:

Either it will return too early (it returns immediately, if this.HasFocus is true when a child has focus)
Or it will return null (it doesn't descend, if child.HasFocus is false when a grandchild has the focus).

I don't quite understand your question: Control is a System class, so how are you supplying source code for it? I'm guessing you're using it as an example and writing pseudo-code.

To avoid confusion, I'm going to:

Assume you're writing code for the standard System.Windows.Forms.Control class
Assume that Control has the standard ContainsFocus and Focused properties
Write a class which defines GetFocus has a new extension method

I suggest:

public static class ControlExtensions
{
    public static Control GetFocus(this Control control)
    {
        if (control.Focussed)
            return this;

        foreach (Control child in Children)
        {
            if (!child.ContainsFocus)
                continue;
            return GetFocus(child); // recurses
        }

        return null;
    }
}

Or, assuming that it is your own Control class as you wrote it in the OP, I would implement its recursive GetFocus as follows:

public class Control
{
    public bool CanFocus { get; set; }
    public bool HasFocus { get; set; }

    public Control GetFocus()
    {
        if (HasFocus)
            return this;

        foreach (Control child in Children)
        {
            Control found = child.GetFocus();
            if (found != null)
                return found;
        }

        return null;
    }

    public bool ContainsFocus
    {
        get { return GetFocus() != null; }
    }
}

In this second implementation, the GetFocus implementation doesn't call ContainsFocus:

Because ContainsFocus calls GetFocus
Because ContainsFocus descends the tree of children in order to decide whether to return true, so GetFocus might as well do the same (descend the tree of children) but return the actual Control instead of just returning bool.

Beware that your HasFocus implementation (as coded above) isn't good enough: because you must ensure that only one Control has the focus (so if you set the focus, then you need to remove focus from whichever control previously had the focus, if there is one).

  I considered a helper method with the signature bool FindFocus(out Control result), but it felt unwieldy.

It's enough to return null if there is no focus.

You might want to return bool if you want to return additional information; for example:

Customer customer;
bool success = FetchCustomer(customerId, out customer);
if (!success)
{
    // fetch failed because database or network is down
}
else
{
    if (customer == null)
    {
        // fetch successfully says that there is no such customer
    }
    else
    {
        // fetch successfully returned the requested customer
    }
}

In your example however the GetFocus algorithm/implementation cannot 'fail', so returning Control is enough (instead of returning bool as well as having an out parameter).

Having an out parameter is necessary if null is a valid value: for example the Dictionary.TryGetValue method (because a Dictionary can contains nulls, so it needs a bool to say whether it's in the dictionary at all).

Having a bool with an out parameter is also necessary if the type in question is a struct instead of a class: because a struct cannot be null. In this case though, Customer is a class not a struct.

  Also, I am concerned with invoking foreach within a recursive function.

Why? It's not a problem per se.


Unit test for class converting between DTOs and entities Is this correctly written test? Tested class is responsible for converting from dto to entity class and i wanted to test it. Are these tests useful and well designed? I have a problem understanding how should look such classes.

Tested class:

@Service
public class ConverterRegisterUserDto extends Converter<RegisterUserDto,User> {

  private final PasswordEncoder passwordEncoder;

  @Autowired
  public ConverterRegisterUserDto(PasswordEncoder passwordEncoder, ModelMapper modelMapper) {
    super(modelMapper);
    this.passwordEncoder = passwordEncoder;
  }

  @Override
  public User convert(RegisterUserDto dto) {
    User user = modelMapper.map(dto, User.class);
    user.setPassword(encodePassword(dto.getPassword()));
    return user;
  }

  private String encodePassword(String password){
    return passwordEncoder.encode(password);
  }
}

Test:

class ConverterRegisterUserDtoTest {

    private PasswordEncoder passwordEncoder;
    private ConverterRegisterUserDto converterRegisterUserDto;
    private RegisterUserDto registerUserDto;

    @BeforeEach
    void init(){
        ModelMapper modelMapper = new ModelMapper();
        passwordEncoder = new BCryptPasswordEncoder();
        converterRegisterUserDto = new ConverterRegisterUserDto(passwordEncoder, modelMapper);
        registerUserDto = getSampleRegisterUserDto();
    }

    private RegisterUserDto getSampleRegisterUserDto(){
        RegisterUserDto registerUserDto = new RegisterUserDto();
        registerUserDto.setUsername("t_username");
        registerUserDto.setPassword("t_password");
        registerUserDto.setEmail("t_email");
        registerUserDto.setClientVerifyAccountUrl("t_url");
        return registerUserDto;
    }

    @Test
    void encodePassword_correctly(){
        //WHEN
        User user = converterRegisterUserDto.convert(registerUserDto);

        //THEN
        assertNotEquals(registerUserDto.getPassword(), user.getPassword());
        assertTrue(passwordEncoder.matches(registerUserDto.getPassword(), user.getPassword()));
    }

    @Test
    void convert_fromRegisterUserDto_toUserDtoClass(){
        //WHEN
        User user = converterRegisterUserDto.convert(registerUserDto);

        //THEN
        assertEquals(User.class, user.getClass());
    }

    @Test
    void convert_fromRegisterUserDto_isMappingCorrectly(){
        //WHEN
        User user = converterRegisterUserDto.convert(registerUserDto);

        //THEN
        assertEquals(registerUserDto.getUsername(), user.getUsername());
        assertEquals(registerUserDto.getEmail(), user.getEmail());
    }
}

I am trying to understand how to write good tests. Is that a good way? Or am I totally missing an idea of testing?

@BeforeEach
void init(){
    ModelMapper modelMapper = new ModelMapper();
    passwordEncoder = new BCryptPasswordEncoder();

You're using actual implementation classes from your unit test.  This has implications in that you're also effectively testing these classes, so you have potential side effects from changes when you upgrade the library, performance / dependency considerations.  That said, it may be acceptable if you consider these to be reliable library methods with low performance impact/dependencies (nobody mocks out String for example), however if not you might want to consider mocks. I would probably mock out at least the PasswordEncoder, as this would make it easier to control the result of the encoded password.

You're also setting up these dependencies before each test.  There doesn't seem to be any need to do this, the same ModelMapper and PasswordEncoder could be used for all of the tests, so I'd tend to put it in @BeforeAll, or even when initialising the fields.

@Test
void encodePassword_correctly(){

Your other tests have three parts to their names.  It looks like they are 'methodName_source_result'.  MethodName seems to be missing for this test name.  Where possible, consistency really does make it easier for other people to understand to expect.

//THEN
assertNotEquals(registerUserDto.getPassword(), user.getPassword());

You don't really need to assert that the encoded password doesn't match the original password, if you're going to test that it matches with the password encoder as well.  It just adds noise to the test.

assertTrue(passwordEncoder.matches(registerUserDto.getPassword(), user.getPassword()));

This is checking with your encoder that the returned password would match.  As I've said, this is testing more than just your code, however it might be acceptable to you.  I'd prefer a pair of tests that explicitly test this interaction something like this:

@Test
void convert_fromRegisterUserDto_toEncodedPassword() {
    //BEFORE
    when(passwordEncoder.encode(any(String.class)))
        .thenReturn("encodedPassword");

    //WHEN
    ConvertMapper.User user = converterRegisterUserDto.convert(registerUserDto);

    //THEN, assert encodedPassword is mapped into returned user.
    assertEquals("encodedPassword", user.getPassword());
}

@Test
void convert_delegatesPasswordEncoding_toPasswordEncoder() {
    //BEFORE
    registerUserDto.setPassword("unencodedPassword");

    //WHEN
    converterRegisterUserDto.convert(registerUserDto);

    //THEN, verify correct password was encoded
    verify(passwordEncoder).encode("unencodedPassword");
}

Which assumes a mock setup in the field/init:

privatePasswordEncoder passwordEncoder = mock(PasswordEncoder.class);

void init() {
    // default to returning emptyString
    when(passwordEncoder.encode(any(String.class))).thenReturn("")

This test seems a bit redundant:

  assertEquals(User.class, user.getClass());

Do you really care if it's an actual instance of User, rather than a derived type?

assertEquals(registerUserDto.getUsername(), user.getUsername());

assertEquals always seems counter intuitive to me (I want the expected to be on the right).  So I tend to use assertJ:

assertThat(user.getUsername()).isEqualTo(registerUserDto.getUsername());

However, it can also be more descriptive to use constants in your tests to be more expressive.  This allows you to see at a glance where the tested information is coming from:

@Test
void convert_fromRegisterUserDto_isMappingCorrectly() {
    //Before
    registerUserDto.setUsername("UserName");
    registerUserDto.setEmail("A@B.C");

    //WHEN
    ConvertMapper.User user = converterRegisterUserDto.convert(registerUserDto);

    //THEN
    assertEquals("UserName", user.getUsername());
    assertEquals("A@B.C", user.getEmail());
}


Find permutation of a specified lexicographic rank This is one result of my work on Project Euler to learn Java.  I'm interested in how well this conforms to best practices as well as any recommendations on efficiency.  (I'm pretty sure this code is a lot more efficient than it is readable, but it's not deliberately obfuscated, I swear! :) )

import java.util.ArrayList;
import java.util.Arrays;

public class Combinatorics {
    private static ArrayList<Long> factorialList = new ArrayList<Long>(Arrays.asList(1L,1L));

    public static long factorial(int n) {
        if (n < 0) {
            return -1L;
        } else if (n < factorialList.size()) {
            return factorialList.get(n);
        } else {
            int currSize = factorialList.size();
            long currNum = factorialList.get(currSize - 1);
            while (currSize <= n) {
                currNum *= currSize;
                factorialList.add(currNum);
                currSize++;
            }
            return currNum;
        }
    }

    public static boolean factNisLessThanK(int n, long k) {
        if (n < 0) return false;
        if (n < factorialList.size()) return (factorialList.get(n) < k);

        int currSize = factorialList.size();
        long currNum = factorialList.get(currSize - 1);
        while (currSize <= n) {
            currNum *= currSize;
            factorialList.add(currNum);
            currSize++;
            if (currNum >= k) return false;
        }
        return true;
    }

    public static int[] lexicographicPermutation(int digits, long rank) {
        if ((rank < 1) || factNisLessThanK(digits, rank)) {
            int result[] = {-1};
            return result;
            //return "Error";
        }
        rank--;        //using 0 based ranking makes the math easier.
        int result[] = new int[digits];
        int index = 0;
        while (index < result.length) {
            result[index] = (int) (rank / factorial(digits - 1));
            rank %= factorial(digits - 1);
            index++;
            digits--;
        }
        for (int i = result.length - 1; i >= 0; i--) {
            for (int j = i + 1; j < result.length; j++) {
                if (result[j] >= result[i]) result[j]++;
            }
        }
        //return Arrays.toString(result);
        return result;
    }
}

The key to this whole program (actually, class) is the last method: lexicographicPermutation, which returns an int array containing the rankth permutation of a set of digits numbers—the numbers 0 through digits-1, in fact.

So, for example

System.out.println(Arrays.toString(Combinatorics.lexicographicPermutation(4, 11L)));

Prints [1, 3, 0, 2] because:

permutation 1: 0123
permutation 2: 0132
permutation 3: 0213
permutation 4: 0231
permutation 5: 0312
permutation 6: 0321
permutation 7: 1023
permutation 8: 1032
permutation 9: 1203
permutation 10: 1230
permutation 11: 1302

The code also includes a memoized factorial method and a factNisLessThanK method which lazily extends the factorial list only as much as needed to give the answer.  So a call like factNisLessThanK(17, 100L) would only compute up to 5! before returning false (since 5! = 120) instead of computing up to 17!

(I used it to solve Project Euler problem 24, calling lexicographicPermutation(10, 1000000L).)

https://projecteuler.net/problem=24

This is nicely written, readable and efficient.

The most significant improvement I can suggest is to rewrite this as a for loop:

int index = 0;
while (index < result.length) {
    result[index] = (int) (rank / factorial(digits - 1));
    rank %= factorial(digits - 1);
    index++;
    digits--;
}

Because this is essentially a counting loop, and index is not needed outside of it anyway. That is:

for (int index = 0; index < result.length; ++index) {
    result[index] = (int) (rank / factorial(digits - 1));
    rank %= factorial(digits - 1);
    digits--;
}

It's recommended to declare variables using their interfaces, so instead of:

private static ArrayList<Long> factorialList = new ArrayList<Long>(Arrays.asList(1L,1L));

This is better:

private static List<Long> factorialList = new ArrayList<>(Arrays.asList(1L, 1L));

Lastly, a couple of nitpicks:

In lexicographicPermutation, int result[] = {-1} is a redundant local variable
It's recommended to use braces with single-statement if too
There are some redundant parentheses here and there, for example if ((rank < 1) || ...)