My simple implementation of a DI container I decided to make my own light DI container.

I know about Zenject, etc. I wanted a very light analog.

I'm concerned about a few things:

1) is it correct to use struct instead of class for reference in this context?

2) every time you access _player.Reference, there is a check for null (in the UnityEngine.After the destruction of the Object does not equal null, so use SafeIsUnityNull)

3) Double point. Can I overload the operator so that instead of _player.Reference.AAAAAAAAAAAAA();, it works _player.AAAAAAAAAAAAA(); ??

Container itself:

    public static class DIContainer
    {

        private static readonly Dictionary<System.Type, Object> _references = new Dictionary<System.Type, Object>();

        public static void Bind<T>(T value) where T : Object
        {

            _references[typeof(T)] = value;

        }

        public static T Resolve<T>() where T : Object
        {

            if (_references.TryGetValue(typeof(T), out Object value))
                return (T)value;

            throw new System.InvalidCastException($"DIContainer: invalid resolve object {typeof(T)}");

        }

    }

Field reference:

    public struct DIReference<T> where T : Object
    {

        private T _reference;
        public T Reference => !_reference.SafeIsUnityNull() ? _reference : _reference = DIContainer.Resolve<T>();

    }

Installer:

    public abstract class SceneInstaller : MonoBehaviour
    {
        private void Awake() => InstallBindings();
        protected abstract void InstallBindings();

    }

public class LevelInstaller : SceneInstaller
{

}

Using

Bind:

    [SerializeField] private PlayerController _player;

    protected override void InstallBindings()
    {

        DIContainer.Bind(_player);

    }

Reference:

public class Test
{

    private DIReference<PlayerController> _player;

    private void Foo()
    {

        _player.Reference.AAAAAAAAAAAAA();

    }

}

What you're actually doing is implementing Service Locator which is quite opposite to dependency injection. Please refer to [THIS] discussion about it.

Please use already existing solution.

Few points from my side, just to point out why I think that's wrong for your case:

Why do you think you need more lightweight DI container? Have you performed benchmarks and they proved that your bottle neck is DI injection?
Why have you choosen struct for DIReference? Struct is not good choice for a field, because you're unintenionally (I presume) allocating it on heap. Please refer to [THIS] discussion.
Why would you need DIReference anyway? It's only complicating your code. You don't need to store it as field anywhere - instances that you need are already kept in _references in DIContainer.
Just to point out, as your _references is static readonly field, then this Dictionary is singleton. It causes few potential problems, to name two:

Is it guaranteed that every time you install bindings there won't be any other threads that trying to do same thing?
Every instance in this dictionary is also singleton-ish. What I mean by that - as soon as first class will install PlayerController into your _references in DIContainer then it'll be available for all classes that uses your DIContainer class. Yet you can still overwrite it. Why would that be a problem? It could cause huge GC pressure, because you'll constantly overwriting PlayerController in your dictionary and GC will have to clean that up which could lead to stopping all threads from running. You definitely want to avoid that especially in games.

I want to strongly suggest to use already existing solution for DI. If you're worried about performance please refer to [THIS] benchamrks.


Counting the frequency of letters in a string slices While preparing a solution for the CodeChef - magic tricks, an easy question about counting the frequency of given character from the string slice, I came up with the following solution:

input_str = input("")
t = int(input(""))
for i in range(t):
    c, l1, l2 = input("").split()
    l, r = int(l1),int(l2)
    print(input_str[l-1:r].count(c))

But It turns out to be inefficient because it's not meeting the time constraint. How can I better implement it?

(here, the indexing starts from 1)

Rewriting your code with Caridorc's suggestions gets me something like

input_str = input()

iterations = int(input())
for _ in range(iterations):
    try:
        character, start, end = input().split()
    except ValueError:
        raise ValueError("Need exactly three items (character, start, end)")

    start, end = int(start), int(end)
    print(input_str[start-1:end].count(character))

Note that you should probably put this in a main method.

The slowest part is going to involve the inner iteration; namely

    try:
        character, start, end = input().split()
    except ValueError:
        raise ValueError("Need exactly three items (character, start, end)")

    start, end = int(start), int(end)
    print(input_str[start-1:end].count(character))

Getting input can't get much faster, I'd assume, so this line to blame is

    print(input_str[start-1:end].count(character))

If start-1 and end are wildly different (they can be up to \$10^6-1\$ different) then this will take a long time. For a maximum of \$10^6\$ iterations, this could take a while.

What you should do instead is pre-process the string so you can quickly do

    print(number_between(start, end, letter))

How? One way is to build cumulative lists, such as

string:  a  b  c  b  a  c  b  a  b  a
     a:  1  1  1  1  2  2  2  3  3  4
     b:  0  1  1  2  2  2  3  3  4  4
     c:  0  0  1  1  1  2  2  2  2  2

That way you can quickly it calculate with

def number_between(start, end, letter):
    return counts[letter][end] - count[letter][start-1]

This initial generation might take a long time, though, as you need to generate a list for every possible letter - and there are a lot of those. If you only generate a list for which there is a letter in the string, you've improved your worst case but you're still generating a large square. This could be expensive.

One other option is to store indexes:

string:  a  b  c  b  a  c  b  a  b  a
     a:  0           4        7     9  =  [0, 4, 7, 9]
     b:     1     3        6     8     =  [1, 3, 6, 8]
     c:        2        5              =  [2, 5]

Then you can do a binary search on the list to find the number of instances before an index, and do

def number_between(start, end, letter):
    number_before_end = ... # binary search
    number_before_start = ... # binary search

    return number_before_end - number_before_start

Depending on how much startup costs affect you, this could either be better or worse than the original strategy.

I'll stick with the second of these, since it's less problematic in the face of Unicode. The test suite seems to ignore anything but lowercase ASCII, but I'm not willing to make such a horrible assumption.

So, first we want a bunch of lists indexed by character. This will be easiest as a defaultdict(list), since they all start out empty:

indices = defaultdict(list)

Then we want to go over the characters in the string and add the respective indices to the particular list:

for i, character in enumerate(input_string):
    indices[character].append(i)

We want to do a binary search to find the counts. This is just bisect_right and bisect_left, depending on whether you want to be inclusive:

def number_between(start, end, counts):
    number_before_start = bisect_left(counts, start)
    number_before_end = bisect_right(counts, end)

    return number_before_end - number_before_start

Now all that's needed is input, giving:

from bisect import bisect_left, bisect_right
from collections import defaultdict

def preprocess(input_string):
    indices = defaultdict(list)

    for i, character in enumerate(input_string):
        indices[character].append(i)

    return indices

def number_between(start, end, counts):
    return bisect_right(counts, end) - bisect_left(counts, start)

def main():
    indices = preprocess(input())

    for _ in range(int(input())):
        try:
            character, start, end = input().split()
        except ValueError:
            raise ValueError("Need exactly three items (character, start, end)")

        # Change to 0-based indexing
        start, end = int(start) - 1, int(end) - 1

        print(number_between(start, end, indices[character]))

main()


Client-server application for logging running processes My first client-server application. It's a simple Python script that gets a list of running processes from Server_1 and writes it to a datetime-named file on Server_2, every 5 seconds.
I am planning on refactoring this code into one or two functions, maybe one for securing the connection and one for implementing the logic.
I also know nothing about security best-practices, and I'm looking for some pointers. Any feedback on any aspect of the code is welcome.

import os
import paramiko
from time import sleep
from datetime import datetime

SERVER_1 = os.getenv('HOST_1') or ""
SERVER_1_PASS = os.getenv('PASS_1') or ""
SERVER_2 = os.getenv('HOST_2') or ""
SERVER_2_PASS = os.getenv('PASS_2') or ""

ssh = paramiko.SSHClient()
ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy())
ssh.connect(SERVER_1, username='root', password=SERVER_1_PASS)

ssh2 = paramiko.SSHClient()
ssh2.set_missing_host_key_policy(paramiko.AutoAddPolicy())
ssh2.connect(SERVER_2, username='root', password=SERVER_2_PASS)

while True:

    now = datetime.now()
    dt_string = now.strftime("%d-%m-%YT%H:%M:%S")

    stdin, stdout, stderr = ssh.exec_command("ps -aux")

    processes = stdout.readlines()
    output = [line.strip() for line in processes]

    ftp = ssh2.open_sftp()
    try:
        file = ftp.file(dt_string, "a", -1)
        file.write('\n'.join(output))
        file.flush()
        ftp.close()
    except IOError as e:
        print("Could not write to file")
        print(e)

    sleep(5)
```

SERVER_1_PASS = os.getenv('PASS_1') or ""
SERVER_2_PASS = os.getenv('PASS_2') or ""

I don't think it's a good idea to pass secrets in the environment - that's too easily read by other processes.  Instead, prefer to hold them in a file that's accessible only by the user.  Since we're using SSH, we even already have such a file ($HOME/.ssh/config), though a better choice would be to use public-key authentication.

ssh.connect(SERVER_1, username='root', password=SERVER_1_PASS)

Ouch - why to we need to connect as root?  We should have a dedicated user for this, with the minimum level of capability to perform the task.
What does connect() do when SERVER_1 is the empty string (our default if not passed in environment)?  Is that what we want, or should we error out in that case?


Show and Hide group of classes based on data attributes Somebody please verify this code. Is this efficient? please tell me modular way to accomplish this task.

  Updated

  I want to add show class to data-targetand remove
  show class from data-group Just like happening in an accordion. I wanted to know that this code is perfect for that scenario. I'm new here please let know if did not provide enough information.

$(function () {
  $(document).on("click", ".showHide", function() {
    var hideClasses = $(this).data("group").split(" ");
    var showClasses = $(this).data("target").split(" ");

    $.each(hideClasses, function(k, v) {
      $("."+v).removeClass("show");
    });
    $.each(showClasses, function(k, v) {
      $("."+v).addClass("show");
    });
  });
});

HTML tag

Remeber Password?

  More Info

  Please find the pen I have the working link. Try clicking on login, sign up, forgot password password and the stay relaxed.

CodePen Link

Instead of looping over the showClasses and hideClasses you can do a regex search and replace, then add/remove the class names:

var trimRegex = /^\s+|\s+$/g,
    selectorRegex = /(^| +)([-_a-zA-Z0-9]+)/g,
    callback = function(match, $1, $2) {
        if ($1) {
            return ", ." + $2;
        } else {
            return "." + $2;
        }
    },
    hideSelector = this.getAttribute("data-group")
        .replace(trimRegex, "")
        .replace(selectorRegex, callback),
    showSelector = this.getAttribute("data-target")
        .replace(trimRegex, "")
        .replace(selectorRegex, callback);

$(hideSelector).removeClass("show");
$(showSelector).addClass("show");

Secondly, since the document object exists the moment JavaScript begins executing, there is no need to wait for the document to load:

$(document).on("click", ".showHide", function() { ... });


Speeding up pandas apply I would like to speed up pandas apply function. I have been using swifter . It currently takes about 5 mins for 200000 records using multiprocessing  as below . Is there any way to speed this up further .

def partial_match(source_words, dest_words):
    matched_words = ''
    if any(word in dest_words for word in source_words) :
        match_words_list = set(source_words)&set(dest_words)
        matched_words = ",".join(match_words_list)
    return matched_words

def exact_match(source_words, dest_words):
    matched_words = ''
    if all(word in dest_words for word in source_words) :
        match_words_list = set(source_words)&set(dest_words)
        matched_words = ",".join(match_words_list)
    return matched_words

series_index = ['match_type', 'matched_words'   ]
def perform_match(x):
    match_series = pd.Series(np.repeat('', len(series_index)), index = series_index)
    if x['remove_bus_ending'] == 'Y':
        x['dest_words'] = x['dest_words_2']
    else:
        x['dest_words'] = x['dest_words_1']
    # exact match
    if (x['partial_match_flag'] == 'Y') :
        match_series['matched_words'] = partial_match(x['source_words'], x['dest_words'])
        if match_series['matched_words'] != '':
            match_series['match_type'] = 'Partial Match'
    elif (x['exact_match_2'] == 'Y'):
        match_series['matched_words'] = exact_match(x['source_words'], x['dest_words'])
        if match_series['matched_words'] != '':
            match_series['match_type'] = 'Exact Match'

    return match_series

from multiprocessing import  Pool
from functools import partial
import numpy as np

def parallelize(data, func, num_of_processes=8):
    data_split = np.array_split(data, num_of_processes)
    pool = Pool(num_of_processes)
    data = pd.concat(pool.map(func, data_split))
    pool.close()
    pool.join()
    return data

def run_on_subset(func, data_subset):
    return data_subset.swifter.apply(func, axis=1)

def parallelize_on_rows(data, func, num_of_processes=8):
    return parallelize(data, partial(run_on_subset, func), num_of_processes)

df[match_series]  = parallelize_on_rows(df, perform_match)

below is some sample data

flag1   partial_match_flag  exact_match_flag    source_words    dest_word_2 dest_words_1
0   N   Y   N   [song, la]  [urban, karamay, credit, city, co, kunlun, com...   [ltd, urban, karamay, credit, city, co, kunlun...
1   N   Y   N   [song, la]  [al, abdulah, nasser]   [al, abdulah, nasser]
2   N   Y   N   [song, la]  [al, abdulah, nasser]   [al, abdulah, nasser]
3   N   Y   N   [song, la]  [abdulamir, mahdi]  [abdulamir, mahdi]
4   N   Y   N   [song, la]  [abdullah, al, nasser]  [abdullah, al, nasser]
5   N   Y   N   [song, la]  [abu, al, jud]  [abu, al, jud]
6   N   Y   N   [song, la]  [al, herz, adam]    [al, herz, adam]

flag as boolean
If you change the flags from 'Y' and 'N' to True and False You can use boolean indexing. This should speed up a lot of things already
set
You check for each combination word in dest_words for word in source_words on a list of words. If the check matches, you convert to a set. The containment check would be sped up by checking against a list, but using set comparisons would speed this up a lot.
import typing

def partial_match(
    source_words: typing.Set[str], dest_words: typing.Set[str], index=None
) -> typing.Tuple[typing.Any, typing.Optional[str]]:
    intersection = source_words & dest_words
    if intersection:
        return index, ", ".join(intersection)
    return index, None

def exact_match(
    source_words: typing.Set[str], dest_words: typing.Set[str], index=None
) -> typing.Tuple[typing.Any, typing.Optional[str]]:
    if source_words == dest_words:
        return index, ", ".join(source_words)
    return index, None

The reason I chose to return the index along with it is to be able to reconstruct the series easier when reassembling everything.
Don't touch the original data
You change your source data inplace (by adding columns). Better would be to leave this untouched, and keep the destination words etc in separate series.
Series.where
You can replace calls like this
if x['remove_bus_ending'] == 'Y':
        x['dest_words'] = x['dest_words_2']
    else:
        x['dest_words'] = x['dest_words_1']

with Series.where
a = pd.Series(list("abcd"))
b = pd.Series(list("efgh"))
c = pd.Series([True, True, False, True])
b.where(c, other=a)

0    e
1    f
2    c
3    h
dtype: object

If your data looks like this:
from io import StringIO

import pandas as pd

def setify(s):
    return s.str.strip("[]").str.split(", ").apply(set)

df = pd.read_csv(StringIO(data_str), sep="\s\s+", index_col=False, engine='python')
df["source_words"] = setify(df["source_words"])
df["dest_words_1"] = setify(df["dest_words_1"])
df["dest_word_2"] = setify(df["dest_word_2"])
df["remove_bus_ending"] = df["remove_bus_ending"] == "Y"
df["partial_match_flag"] = df["partial_match_flag"] == "Y"
df["exact_match_flag"] = df["exact_match_flag"] == "Y"

intermediate dataframe
If you want to split the dataframe with arraysplit, you'll need to provide an intermediate form with the info you need:
df_intermediate = pd.concat(
    [
        df["dest_word_2"]
        .where(df["remove_bus_ending"], other=df["dest_words_1"])
        .rename("dest_words"),
        df["source_words"],
    ],
    axis=1,
)

You can even split it immediately according to what matching is needed
df_intermediate_partial = df_intermediate.loc[df["partial_match_flag"]]
df_intermediate_exact = df_intermediate.loc[df["exact_match_flag"]]

applying the function
not parallel:
result_partial = list(
    map(
        partial_match,
        df_intermediate_partial["source_words"],
        df_intermediate_partial["dest_words"],
        df_intermediate_partial.index,
    )
)

results_exact = list(
    map(
        exact_match,
        df_intermediate_exact["source_words"],
        df_intermediate_exact["dest_words"],
        df_intermediate_exact.index,
    )
)

result = pd.Series(result_partial + results_exact)

This should be easy to parallelize. Since I'm no expert on that, I'll leave that to others.
context manager
Most of the examples I found in the multiprocessing documantation work with a context manager that takes care of the closing of the pool
with Pool(processes=4) as pool:
    ... # parallel part of the code


Possible improvements on admin login area? I've created an admin login area for an application I am planning to code, and I've used the following login.html page to let the user type in his data (I left out parts like "id", "placeholder" etc. to make it shorter):

<form method="post" action="login.php">
  <input type="text" name="username">
  <input type="password" name="password">
  <input type="submit" value="Login" name="submit">
</form>

The form is processed as is, no JavaScript interfering here.  The login.php then looks like this:

<?php
  if (isset($_POST['password']) && isset($_POST['userName'])) {
    if ($_POST['password'] == "myPassword" && $_POST['userName'] == "myUsername") {
      if (!session_id()) {
        session_start();
        $_SESSION['logon'] = true;
        header('Location: admin_area.php');
        die();
      }
    } else if ($_POST["password"] == "" || $_POST["userName"] == "") {
      echo "Please enter both a username and a password! You are sent back in 3 seconds";
      echo "<meta http-equiv='refresh' content='3;url=login.html'>";
    } else {
      echo "Wrong username and/or password! You are set back in 3 seconds";
      echo "<meta http-equiv='refresh' content='3;url=login.html'>";
    }
  } else {
    echo "This is a restricted area, you have to log in!";
    echo "<meta http-equiv='refresh' content='3;url=login.html'>";
  }
?>

The admin_area.php file, which the user is then being redirected to, has the following short placeholder code:

<?php
  if (!session_id()) session_start();
  if (!$_SESSION['logon']) {
    echo "This is a restricted area, you have to log in!";
    echo "<meta http-equiv='refresh' content='3;url=login.html'>";
  } else {
    echo "<script src='//ajax.googleapis.com/ajax/libs/jquery/2.0.3/jquery.min.js'></script>";
    echo "<script type='text/javascript' src='session_destroy.js'></script>";
    echo "Hello World!";
    echo "<br>";
    echo "<input type='button' id='exit_button' value='Exit' onclick='window.open(\"\", \"_self\", \"\"); window.close();'>";
  }
?>

The essential part of the session_destroy.js file on this page contains this code snippet:

$(window).on("beforeunload", function() {
  $.get("session_destroy.php");
  return "You have left the page. Your session has been terminated.";
});

And finally, the session_destroy.php, which should terminate all session data, contains the following lines (taken from the PHP manual):

<?php
  if (!session_id()) session_start();
  if (!$_SESSION['logon']) {
    echo "This is a restricted area, you have to log in!";
    echo "<meta http-equiv='refresh' content='3;url=login.html'>";
  } else {
    $_SESSION = array();
    if (ini_get("session.use_cookies")) {
      $params = session_get_cookie_params();
      setcookie(session_name(), '', time() - 42000,
        $params["path"], $params["domain"],
        $params["secure"], $params["httponly"]
      );
    }
    session_destroy();
    header('Location: login.html');
  }
?>

Now this "construct" seems to be working just fine.  I am wondering if there are some improvements on these lines, as I have researched everything myself and I am not sure how "secure" and correct these methods might be.

I am thinking of possible security improvements to the form-processing and session-handling / destroying. Are there any areas in which I should do more research?

Tha major security issue I see is that you do not have any limiting or logs for wrong/success attempts. One can use automated brute force attacks trying all day/night long to guess your username and password. If lack of success, at least you might have your app slowed down, because of the billion requests.

As you requested, I tried to add some code examples around my suggestion:

<?php

//the db interaction is pseudo code, because it depends on the API you are using
/**
 *
 * @param int $success : 0 for fail, 1 for success
 */
function logAttempt($success) {
    $user = $db->escape($_POST['userName']);
    $pass = $db->escape($_POST['password']);
    $session_id = session_id();
    if (!in_array($success, array(0, 1))) {
        return false;
    }
    $db->query("INSERT INTO `log_attempts`
                    (`user`, `pass`, `ip`, `session_id`, `success`, `tried_on`)
                VALUES
                    ('$user', '$pass', '{$_SERVER['REMOTE_ADDR']}', '$session_id', '$success', NOW());
            ");
    return $db->affected_rows > 0;
}

function isAttemptsLimitExceeded() {
    $max_attempts = 5;
    $session_id = session_id();
    $res = $db->query("SELECT
                            COUNT(*) AS cnt
                        FROM
                            `log_attempts`
                        WHERE
                            `session_id` = '$session_id' AND
                            `tried_on` > DATE_SUB(NOW(), INTERVAL 24 HOUR) AND
                            `success` = '0';
                    ");
    $row = $db->fetch($res);
    if ($row['cnt'] >= $max_attempts) {
        return true;
    }
    return false;
}

function banFailedLogins() {
    if(isAttemptsLimitExceeded()) {
        $db->query("INSERT INTO `blacklist`
                        (`ip`, `banned_on`)
                    VALUES
                        ('{$_SERVER['REMOTE_ADDR']}', NOW()");
    }
    return $db->affected_rows > 0;
}

function clearBans() {
    $db->query("DELETE from `blacklist` WHERE `ip` = '{$_SERVER['REMOTE_ADDR']}';");
    return $db->affected_rows > 0;
}

//check for expired bans?
function canAcess() {
    $ban_expire = 24; // hours
    $res = $db->query("SELECT
                            COUNT(*) AS cnt
                        FROM
                            `blacklist`
                        WHERE
                            `ip` = '{$_SERVER['REMOTE_ADDR']}' AND
                            `banned_on` > DATE_SUB(NOW(), INTERVAL $ban_expire HOUR);
                        ");
    $row = $db->fetch($res);
    if ($row['cnt'] > 0) {
        return false;
    }
    else {
        clearBans();
        return true;
    }
}

if (!canAcess()) {
    die("You are banned for failed login attempts");
}
  if (isset($_POST['password']) && isset($_POST['userName'])) {
    if ($_POST['password'] == "myPassword" && $_POST['userName'] == "myUsername") {
      if (!session_id()) {
        session_start();
        $_SESSION['logon'] = true;
        logAttempt(1); //log successfull attempt
        header('Location: admin_area.php');
        die();
      }
    } else if ($_POST["password"] == "" || $_POST["userName"] == "") {
      echo "Please enter both a username and a password! You are sent back in 3 seconds";
      logAttempt(0); //log failed attempt
      banFailedLogins();
      echo "<meta http-equiv='refresh' content='3;url=login.html'>";
    } else {
      echo "Wrong username and/or password! You are set back in 3 seconds";
      logAttempt(0); //log failed attempt
      banFailedLogins();
      echo "<meta http-equiv='refresh' content='3;url=login.html'>";
    }
  } else {
    echo "This is a restricted area, you have to log in!";
    echo "<meta http-equiv='refresh' content='3;url=login.html'>";
  }

?>


Conversion from/to roman numbers I wrote as an exercise in Test-Driven Development a piece of Python code that contains two functions:

roman2dec(roman), that converts a roman number (string) into a decimal number (int)
dec2roman(dec), that converts a decimal number (int) into a roman number (string)

I'd like to read your comments on this code, if there are bad practices, if you would sign it for shipping or if you'd change anything.

import re
import math

# Regular expression used to validate and parse Roman numbers
roman_re = re.compile("""^
   ([M]{0,9})   # thousands
   ([DCM]*)     # hundreds
   ([XLC]*)     # tens
   ([IVX]*)     # units
   $""", re.VERBOSE)

# This array contains valid groups of digits and encodes their values.
# The first row is for units, the second for tens and the third for
# hundreds. For example, the sixth element of the tens row yields the
# value 50, as the first is 0.
d2r_table = [
    ['', 'I', 'II', 'III', 'IV', 'V', 'VI', 'VII', 'VIII', 'IX'],
    ['', 'X', 'XX', 'XXX', 'XL', 'L', 'LX', 'LXX', 'LXXX', 'XC'],
    ['', 'C', 'CC', 'CCC', 'CD', 'D', 'DC', 'DCC', 'DCCC', 'CM']]

def roman2dec(roman):
    """Converts a roman number, encoded in a string, to a decimal number."""
    roman = roman.upper()
    match = roman_re.match(roman)

    if not match:
        raise ValueError

    thousands, hundreds, tens, units = match.groups()
    result = 1000 * len(thousands)
    result += d2r_table[2].index(hundreds) * 100
    result += d2r_table[1].index(tens) * 10
    result += d2r_table[0].index(units)

    return result

def dec2roman(dec):
"""Converts a positive decimal integer to a roman number."""
    if dec == 0:
        return ''

    digit = 0
    rem = dec
    result = ''

    # Length in digits of the number dec
    dec_len = int(math.ceil(math.log10(dec)) + 1)

    # Scan the number digit-by-digit, starting from the MSD (most-significant
    # digit)
    while dec_len > 0:
        # Let's take the current digit
        factor = 10 ** (dec_len - 1)
        digit = rem / factor

        # And remove it from the number
        rem = rem - digit * factor

        if dec_len >= 4:
            # Thousands
            result = result + digit * 'M'
        else:
            # Look in the look-up table
            result = result + d2r_table[dec_len - 1][digit]

        dec_len -= 1

    return result

EDIT: Here is the test suite for dec2roman:

class DecToRoman(unittest.TestCase):
    def testZeroIsEmpty(self):
        self.checkString(0, "")

    def testSingleDigits(self):
        self.checkString(1, "I")
        self.checkString(10, "X")
        self.checkString(50, "L")
        self.checkString(100, "C")
        self.checkString(500, "D")
        self.checkString(1000, "M")

    def testSimpleRepeats(self):
        self.checkString(1, "I")
        self.checkString(2, "II")
        self.checkString(3, "III")
        self.checkString(10, "X")
        self.checkString(20, "XX")
        self.checkString(30, "XXX")

    def testSubtraction(self):
        self.checkString(4, "IV")
        self.checkString(9, "IX")
        self.checkString(40, "XL")
        self.checkString(90, "XC")

    def testOther(self):
        self.checkString(89, "LXXXIX")
        self.checkString(145, "CXLV")
        self.checkString(691, "DCXCI")
        self.checkString(1983, "MCMLXXXIII")
        self.checkString(2412, "MMCDXII")
        self.checkString(3309, "MMMCCCIX")

    def checkString(self, decimal, expected_string):
        self.assertEqual(expected_string, new_dec2roman(decimal))

First of all you should document the fact that your code does not work with numbers above 9999.

Then I think your code will become a bit simpler, if you add a fourth row to your d2r_table for the thousands. To avoid repetition you can use a list comprehension:

d2r_table = [
    ['', 'I', 'II', 'III', 'IV', 'V', 'VI', 'VII', 'VIII', 'IX'],
    ['', 'X', 'XX', 'XXX', 'XL', 'L', 'LX', 'LXX', 'LXXX', 'XC'],
    ['', 'C', 'CC', 'CCC', 'CD', 'D', 'DC', 'DCC', 'DCCC', 'CM'],
    ['M' * i for i in xrange(0,10) ]]

This allows you to not treat the thousands as a special case. So instead of:

thousands, hundreds, tens, units = match.groups()
result = 1000 * len(thousands)
result += d2r_table[2].index(hundreds) * 100
result += d2r_table[1].index(tens) * 10
result += d2r_table[0].index(units)

you can write:

thousands, hundreds, tens, units = match.groups()
result =  d2r_table[3].index(thousands) * 1000
result += d2r_table[2].index(hundreds) * 100
result += d2r_table[1].index(tens) * 10
result += d2r_table[0].index(units)
return result

Now you can easily notice the common pattern here: For each number from 0 to 3 you're taking the ith row of d2r_table, calling index on it with the 3-ith element of groups as the argument, then multiplying it with 10**i and lastly summing the results. You can abstract the common pattern like this if you want:

def value_for_group(i):
    group = match.groups[3-i]
    return d2r_table[i].index(group) * 10**i

return sum(value_for_group(i) for i in xrange(0,4))

You can also replace the magic numbers 3 and 4, with something more meaningful:

num_rows = len(d2r_table)

def value_for_group(i):
    group = match.groups[num_rows - 1 - i]
    return d2r_table[i].index(group) * 10**i

return sum(value_for_group(i) for i in xrange(0, num_rows))

This way your code will work without modification if you ever change d2r_table to account for extended roman numerals.

In your dec2roman function you're using integer arithmetic to iterate over the digits of the number. I think it'd be easier and clearer to just convert the number to a string and iterate over the digits with a for loop. By reversing the d2r_table, you can use zip to iterate over the table and the digits in parallel without any index based loops. This way your dec2roman function would look like this:

result = ''
# Make digits four digits long so it has the same number of digits
# as d2r_table has rows, then convert each digit to an int
digits = [int(digit) for digit in "%04d" % dec]

for digit, d2r_row in zip(digits, reversed(d2r_table)):
    result += d2r_row[ digit ]

return result

You can also use join with a generator expression instead of updating result imperatively:

digits = [int(digit) for digit in "%04d" % dec]
table = reversed(d2r_table)
return ''.join( d2r_row[ digit ] for digit, d2r_row in zip(digits, table) )

Again you might also want to replace the 4 in %04d with len(d2r_table), so your code will automatically adapt to a longer d2r_table.


Toggle nav that switches a banner section I built this toggle nav that switches a banner section between the slider and a map. All works well but I would like to know how I could improve my JavaScript as I am quite new to it.

$(document).ready(function() {

// Banners Toggle
var bannerSlider = $('#banner_slider');
var bannerMap = $('#banner_map');

$('.thm-banner--toggles').find('a').on('click', function(e){
 e.preventDefault();

if ($(this).hasClass('toggle-map')) {
  bannerMap.css('z-index', '2');
  bannerSlider.css('z-index', '1');
}

if ($(this).hasClass('toggle-slider')) {
  bannerMap.css('z-index', '1');
  bannerSlider.css('z-index', '2');
}

});

});

The only thing I would change, without having anymore info about how the page is built, is from this

$('.thm-banner--toggles').find('a').on('click', function(e){

to

$('.thm-banner--toggles').on('click', 'a', function(e){

or this

$('.thm-banner--toggles a').on('click', function(e){


Static to Instance method trampolining with templates I finally have a working trampoline mechanism. Would anyone care to critique it?

Ideone

#include <iostream>
using namespace std;

#include <map>

template<typename T>
class Base
{
public:
    typedef void (T::*PFunc)(void);
    typedef void (*PHandler)(void*);

    using method_map_t = map< string, PHandler >;
    static method_map_t& methods( ) {
        static method_map_t* map_of_methods{};
        if( ! map_of_methods ) map_of_methods = new method_map_t;
        return *map_of_methods;
    }

    static void register_method( string name, PHandler handler ) {
        methods()[name] = handler;
    }

    // generic trampoline
    template<PFunc sig>
    static void Handler( void* pInstance ) {
        T* f = reinterpret_cast<T*>(pInstance);
        (f ->* sig)();
    }
};

...

class Final : Base<Final>
{
public:
    void Foo(){cout<<"got foo";}
    void Bar(){cout<<"got bar";}

    static void init(){
        // register_method populates a table of "extern C" function pointers.
        register_method( "foo", static_cast<PHandler>( &Handler<&Final::Foo> ) );
        register_method( "bar", static_cast<PHandler>( &Handler<&Final::Bar> ) );
    }
};

void InvokeFromC(void* inst, string name) {
    Base<Final>::PHandler h = Base<Final>::methods()[name];
    (*h)(inst);
}

int main() {
    Final* f = new Final{};
    f->init();
    // simulate invoking from C
    InvokeFromC( f, "foo" );

    // your code goes here
    return 0;
}

This question follows from here.

I will maintain my list of improvements in this answer.

There is a more succinct way to expose the mechanism to the consumer:

class Final : Base<Final>
{
    register_method<&Final::Foo>( "foo" ); //, static_cast<PHandler>( &Handler<&Final::Foo> ) );
    register_method<&Final::Bar>( "bar" ); //, static_cast<PHandler>( &Handler<&Final::Bar> ) );
    :

class Base
{
    :
    template<PFunc func>
    static void register_method( string name ) {
        methods()[name] = &Handler<func>;
    }
    :

This has extra advantage in that it avoids the following nasty issue:

  Note: (Eelis on IRC) gcc is known not to be able to resolve addresses
  of function template specializations passed to functions. You can work
  around it either by declaring separate variables for the function
  pointers, or using a dirty cast (or a nice cast like
  boost::implicit_cast)

It doubles the number of templated functions getting generated: now 2 are generated for each method instead of one. But at a small price, it is much cleaner to the consumer!

Also function pointer typedef-s could be rewritten:

//typedef void (T::*PFunc)(void);
//typedef void (*PHandler)(void*);

using PFunc = void (T::*)(void);
using PHandler = void(*)(void);

http://ideone.com/ZFQb1f


Compare two ArrayList in a two way comparison? I have a class called Task which I cannot touch because it's a legacy code and I have two ArrayList made up of Task class which I need to compare. Items can be in any order or there can be duplicates as well in the ArrayList.

What is the best way to compare two ArrayList which has object in it and print out the missing elements as well which are not present in either of the list. Is below code is the right and efficient way to do it? I cannot use any external library for this.

I need to compare my two array list in a two way comparison.

If data is in source but not in actual, then return false and print out missing elements.
If data is in actual but not in source, then return false as well and print out missing elements.

Below is my code:

public boolean compare(List<Task> source, List<Task> actual) {
    Set<Task> intersection = new HashSet<>(source);
    Set<Task> sourceDifference = new HashSet<>(source);
    Set<Task> actualDifference = new HashSet<>(actual);

    intersection.retainAll(actualDifference);

    sourceDifference.removeAll(intersection);
    for (Task t: sourceDifference) {
        System.out.println(String.format("Task %s not present in actual", t));
    }

    actualDifference.removeAll(intersection);
    for (Task t: actualDifference) {
        System.out.println(String.format("Task %s not present in source", t));
    }

    return sourceDifference.isEmpty() && actualDifference.isEmpty();
}

Is there any better and efficient way to do this? I am using Java 7.

How about streams API?

public boolean compare(List<Task> source, List<Task> actual) {
    Set<Task> sourceDifference = source.stream()
          .filter(t->!actual.contains(t))
          .collect(Collectors.toSet());
    for (Task t: sourceDifference) {
        System.out.println(String.format("Task %s not present in actual", t));
    }

    Set<Task> actualDifference = actual.stream()
          .filter(t->!source.contains(t))
          .collect(Collectors.toSet());
    for (Task t: actualDifference) {
        System.out.println(String.format("Task %s not present in source", t));
    }

    return sourceDifference.isEmpty() && actualDifference.isEmpty();
}


Custom code for MD5 in js I wanted to write my own code for md5 in javascript and learn its inner mechanics. (I know it is not trusted anymore, deprecated, ... but I'm just learning).
I didn't want to just copy the code that uses these like 60 repeated lines and over-complicated functions (like here), so I started writing my own implementation:
(Although I have just written this pseudocode in js)

function myMD5(string){
    /*bit rotation*/
    var rot = function(word, shift){return word << shift | word >>> (32 - shift);};
    /*unsigning also cuts to 32bit words*/
    var us = function(word){return word >>> 0;};
    /*function for unsigned adding words mod 2**32*/
    var add = function(wordsarr){
        var res = 0;
        for(var wr = 0; wr < wordsarr.length; wr++)
            res = us(res + us(wordsarr[wr]));
        return res;
    };
    /*converting string to utf8encoded string to bytes to 32bit-words to 512bit padded blocks*/
    var bytes = [];
    string = unescape(encodeURIComponent(string));
    for(var char = 0; char < string.length; char++)
        bytes[char] = string.charCodeAt(char);
    bytes.push(0x80);
    var words = [];
    for(var byte = 0; byte < bytes.length; byte += 4)
        words.push(bytes[byte + 3] << 24 | bytes[byte + 2] << 16 | bytes[byte + 1] << 8 | bytes[byte]);
    while(words.length % 16 != 14)
        words.push(0);
    words.push((bytes.length - 1) << 3);
    words.push((bytes.length - 1) >>> 29);
    /*initialization of the constants*/
    var K = [];
    for(var i = 1; i <= 64; i++)
        K.push(Math.floor(0x100000000 * Math.abs(Math.sin(i))));
    var shif = [0x07, 0x0c, 0x11, 0x16, 0x07, 0x0c, 0x11, 0x16, 0x07, 0x0c, 0x11, 0x16, 0x07, 0x0c, 0x11, 0x16,
                0x05, 0x09, 0x0e, 0x14, 0x05, 0x09, 0x0e, 0x14, 0x05, 0x09, 0x0e, 0x14, 0x05, 0x09, 0x0e, 0x14,
                0x04, 0x0b, 0x10, 0x17, 0x04, 0x0b, 0x10, 0x17, 0x04, 0x0b, 0x10, 0x17, 0x04, 0x0b, 0x10, 0x17,
                0x06, 0x0a, 0x0f, 0x15, 0x06, 0x0a, 0x0f, 0x15, 0x06, 0x0a, 0x0f, 0x15, 0x06, 0x0a, 0x0f, 0x15];
    var a0 = 0x67452301,
        b0 = 0xefcdab89,
        c0 = 0x98badcfe,
        d0 = 0x10325476;
    var F, ix;
    var chunk;
    var A, B, C, D;
    for(var pos = 0; pos < words.length; pos += 16){
        /*chunk = current 512bit block*/
        var chunk = words.slice(pos, pos + 16);
        A = a0;
        B = b0;
        C = c0;
        D = d0;
        for(var rnd = 0; rnd < 64; rnd++){
            /*round type*/
            if(rnd < 16){
                F = (B & C) | (~B & D);
                ix = rnd;
            }
            else if(rnd < 32){
                F = (B & D) | (C & ~D);
                ix = (5 * rnd + 1) % 16;
            }
            else if(rnd < 48){
                F = B ^ C ^ D;
                ix = (3 * rnd + 5) % 16;
            }
            else{
                F = C ^ (B | ~D);
                ix = (7 * rnd) % 16;
            }
            /*prepare for the next round, compute the temporary F*/
            F = add([F, A, K[rnd], chunk[ix]]);
            A = D;
            D = C;
            C = B;
            B = add([B, rot(F, shif[rnd])]);
        }
        /*add to the current state mod 2**32*/
        a0 = add([a0, A]);
        b0 = add([b0, B]);
        c0 = add([c0, C]);
        d0 = add([d0, D]);
    }
    /*return the state words as 16 bytes*/
    return [a0 >>> 24, a0 >>> 16 & 0xff, a0 >>> 8 & 0xff, a0 & 0xff,
            b0 >>> 24, b0 >>> 16 & 0xff, b0 >>> 8 & 0xff, b0 & 0xff,
            c0 >>> 24, c0 >>> 16 & 0xff, c0 >>> 8 & 0xff, c0 & 0xff,
            d0 >>> 24, d0 >>> 16 & 0xff, d0 >>> 8 & 0xff, d0 & 0xff];
};

However, It doesn't work as it should...
I already fixed some mistakes, but now I can't find anything wrong!
Could someone help me troubleshooting?

function myMD5(string){

MD5 itself acts on bits or - for most runtimes - bytes. All string manipulation should take place outside any MD5 function.

/*unsigning also works cuts to 32bit words*/

That's not clear to me. Two verbs in a row...

string = unescape(encodeURIComponent(string));

Oh dear, that throws the principle of least surprise in the blender. Do not suddenly change the string without warning the user.

The you first copy the complete message to bytes, and then to words. This uses a lot of unnecessary memory. There is a reason why most implementations (well, outside of those of JavaScript) use multiple update method and a final digest method. It is to handle larger messages, or messages that are actually split over multiple locations.

for(var i = 1; i <= 64; i++)
    K.push(Math.floor(0x100000000 * Math.abs(Math.sin(i))));

This makes the code smaller, of course, but do you really want to perform that calculation for each MD5 invocation?

var A, B, C, D;

When using variable names that are specific to a specification, then please provide a link to the specification. Otherwise other devs are going to wonder about the special variable names (such as shif).

if(rnd < 16){

Branching is relatively slow on modern processors. This is why you see the unrolled loops that you've coiled up again. To get speed, you want to perform operations chunks by buffering a chunk at a time. Then you want to have the code run as fast as possible.

Fortunately you are using else if which makes it ever so slightly more performant. Over a few runs, the branching should become predictable and a good runtime should make it run faster.

You are returning the result as bytes, which is how it should be. Don't forget to test your code against the official test vectors, and add tests for your own edge cases or your MD5 may still result in a wrong value.

Your code looks fine if you just want to get a feel of what MD5 - a relatively simple crypto hash - looks like. It's good practice. But now you may want to proceed and see what it takes to make it efficient - and unroll your own code yourself.


Tower Hopper problem recursive approach The Tower Hopper problem gives us an array of values representing heights that express how far we can jump from a certain tower, and asks whether there's a way to get from tower[0] (0 indexed) to outside of the array.

For ex, if we have towers = [4, 2, 0, 0, 2, 0], we can jump from towers[0] to towers[4] and then outside of the bounds of the array. Or we could jump from towers[0] to towers[1] to towers[4] and then out of the array.
But if we had towers = [4, 2, 0, 0, 1, 0], there would be no way to hop out of the array and we should return False.

I wrote a recursive function that starts from the last array index.

I'm concerned that the way I wrote the function is not the most efficient. It seems to work but I'm thinking I might not have to pass in that many arguments. I very much welcome any other kind of criticism (style, syntax, design...).

I was also wondering (but maybe I should post this as another question) how to turn this into a dynamic programming format. Hints on how to do that (rather than a straight up solution) would be greatly appreciated.

int main(int argc, char const *argv[])
{

    int num_towers = 6;

    int towers[6] = {5, 4, 3, 3, 1, 0};

    printf("These towers are %s\n", is_hoppable(towers, num_towers - 1, num_towers) ? "hoppable" : "NOT hoppable");

    return 0;
}

int is_hoppable(int* towers, int jumpTower, int targetTower){

    if (jumpTower == 0){
        if (jumpTower + towers[jumpTower] >= targetTower)
            return 1;
        else
            return 0;
    }

    if (jumpTower + towers[jumpTower] >= targetTower)
        return is_hoppable(towers, jumpTower - 1, jumpTower);
    else
        return is_hoppable(towers, jumpTower - 1, targetTower);

}

The recursion stop condition looks clumsy. First, inside the if clause you already know that jumpTower == 0. Second,

    if (condition)
        return 1;
    else
        return 0;

is a long way to say return condition. Suggested rewrite:

    if (jumpTower == 0) {
        return towers[0] >= targetTower;
    }

The two recursive calls differ only by the last parameter. This strongly hints to make the function tail-recursive:

    if (jumpTower + towers[jumpTower >= targetTower) {
       targetTower = jumpTower;
    }
    jumpTower -= 1;
    return is_hoppable(towers, jumpTower, targetTower);

and mechanically eliminate tail recursion (sorry for another shameless self promotion):

    while (jumpTower > 0) {
        if (jumpTower + towers[jumpTower >= targetTower) {
           targetTower = jumpTower;
        }
        jumpTower -= 1;
    }
    return towers[0] >= targetTower;

Now the problem is reduced to a single loop. It doesn't look right. But don't blame me. It is a mechanical rewrite (much like a C compiler would do), so if there's a problem here, there must be a problem in the original code.

The printf line is too long to my taste. I was almost sure that you print int as %s. Then I scrolled.

Returning bool rather than int would clarify intentions.


Code for simplifying the creation of Dimension Objects I have a class that I use to represent a distance called Dimension.

Here is its four constructors:

    /// <summary>
    /// Accepts any valid architectural string value for input.
    /// </summary>
    public Dimension(string passedArchitecturalString)
    {
         storeArchitecturalStringAsInternalUnit(passedArchitecturalString);
    }

    /// <summary>
    /// Accepts standard types for input.
    /// </summary>
    public Dimension(DimensionType passedDimensionType, double passedInput)
    {
        storeAsInternalUnit(passedDimensionType, passedInput);
    }

    /// <summary>
    /// copy constructor - create a new Dimension with the same _intrinsicValue as the passed Dimension
    /// </summary>
    public Dimension(Dimension passedDimension)
    {
        _intrinsicValue = passedDimension._intrinsicValue;
    }

    /// <summary>
    /// Zero Constructor
    /// </summary>
    public Dimension()
    {
        _intrinsicValue = 0;
    }

This question gives some more background on my Dimension Class.

Because it is a class that I use so often and it usually uses inches. I had to write out the one of the full constructors every time that I wanted to use it.

Dimension inchDimension = new Dimension(DimensionType.Inch, 14.1875);

This became a major pain when creating things like my 3 dimensional Point object:

Dimension xDimension = new Dimension(DimensionType.Millimeter, 0);
Dimension yDimension = new Dimension(DimensionType.Millimeter, 0);
Dimension zDimension = new Dimension(DimensionType.Millimeter, 0);

        Point p = new Point(xDimension, yDimension, zDimension);

So I created a class to generate these objects called DimensionGenerator.

public class DimensionGenerator
{

    /// <summary>
    /// Default generator sets to inches
    /// </summary>
    public DimensionGenerator()
    {
        _internalType = DimensionType.Inch;
    }

    /// <summary>
    /// This constructor allows you to specify a static type and change it later
    /// </summary>
    /// <param name="wantedType">DimensionType to use for now</param>
    public DimensionGenerator(DimensionType wantedType)
    {
        _internalType = wantedType;
    }

    DimensionType _internalType;
    public DimensionType DimensionOutputType
    {
        get { return _internalType; }
        set { _internalType = value; }
    }

    public Dimension MakeDimension(double passedValue)
    {
        return new Dimension(_internalType, passedValue);
    }
}

I seem to use it a LOT. its less typing to just create on of these instead of using a Dimension constructor. Especially when writing Unit Tests where numbers may not really matter and all I care about are their equality or etc. Isn't it terribly wasteful to create a new one of these in most methods in my application? Should I make a bunch of static DimensionGenerators that only output a specific type? What should I consider when having a class like this?

The concept/pattern that you are trying to apply to DimensionGenerator is actually a factory.
You can approach this problem in two major ways, the factory which is what you were trying to do and by subclassing.

To make the factory pattern correct your dimension class must not have any public constructor and all instances shall be created by calling the factory method:

public class DimensionFactory
{
    public Dimension MakeInchDimension(double distance){
        return new Dimension(DimensionType.Millimeter, distance);
    }
    //more methods for other types
}

Should there be many of DimensionType that would be impractical to define a method for each type then you could do the factory like you already pretty much do.

public class DimensionFactory{
    private readonly DimensionType dimensionType;
    public DimensionFactory(DimensionType dimensionType){
        this.dimensionType = distance;
    }
    public Dimension MakeDimension(double distance){
        return new Dimension(dimensionType, distance);
    }
}

In this approach you will end up with a factory object per each dimension type and you will have a generic Make method for all types, this is a big pro since all types are built in the same way. In the first approach you would have to change the interface if you wanted to add a new dimension type in this approach you don't have to do anything at all.

You could also sub class Dimension so you would have one class per DimensionType, once again this approach is only feasible if there are few DimesnionTypes. in this approach Dimension shall be abstract:

public InchDimension : Dimension{
     public InchDimension(double distance):base(DimensionType.Inch, distance){}
}

Edit
Asside from creating dimension your major concern was about comparing them. I'll assume that you can only compare objects of the same dimension and that you can convert any dimension to any other dimension. This way you are detaching conversion from comparison and comparison algorithm is solved easier.

public Dimension{
   public DimensionType DimensionType{get; private set;}
   public Dimension(DimensionType type, double value){
       DimensionType = type;
       Value = value;
   }
   public double Value{get; set;}
   public Dimension ConvertTo(DimensionType type){
      //if this dimension type is not consersible to type then throw NotSupportedException()
      //else do the convertion logic between this type and the other.
   }
}

public DimensionComparer : IEqualityComparer<Dimension>{
   public bool Equals(Dimension d1, Dimension d2){
       if(d1.DimensionType != d2.DimensionType){
           throw new NotSupportedException("cannot compare different dimmension types, please convert your dimension first");
       }
       return d1.Value == d2.Value;
   }
   public int GetHashCode(Dimension d){
      return d.Value ^ d.DimensionType.GetHashCode();
   }
}


Comparing arrays with source questions and user answers I'm trying to realize simple testing application. And I ran into a problem. I have two arrays. The first is an array with the initial questions, the second array is the answers of users. And now I need to count the number of correct answers. I wrote the following code. I wonder: how optimal is it and, if it's bad, how to optimize it?
I tried to change the "filter" to "find", but this did not affect the execution time of the code.

const questions = [
 {
  questionId: 1,
  question: 'question?',
  answers: [
   { value: '1', right: true },
   { value: '2', right: false },
   { value: '3', right: false },
   { value: '4', right: false },
  ],
 },
 {
  questionId: 2,
  question: 'question?',
  answers: [
   { value: '1', right: false },
   { value: '2', right: false },
   { value: '3', right: false },
   { value: '4', right: true },
  ],
 },
 {
  questionId: 3,
  question: 'question?',
  answers: [
   { value: '1', right: false },
   { value: '2', right: true },
   { value: '3', right: false },
   { value: '4', right: false },
  ],
 },
 {
  questionId: 4,
  question: 'question?',
  answers: [
   { value: '1', right: true },
   { value: '2', right: false },
   { value: '3', right: false },
   { value: '4', right: false },
  ],
 },
 {
  questionId: 5,
  question: 'question?',
  answers: [
   { value: '1', right: false },
   { value: '2', right: false },
   { value: '3', right: true },
   { value: '4', right: false },
  ],
 },
]

const answersFromUser = [
 {
  questionId: 1,
  value: '1',
 },
 {
  questionId: 2,
  value: '1',
 },
 {
  questionId: 3,
  value: '1',
 },
 {
  questionId: 4,
  value: '1',
 },
 {
  questionId: 5,
  value: '1',
 },
]

const createObj = (questionId, value, right) => {
 return { questionId, value, right }
}

const checkAnswers = (questionsArr, answersArr) => {
 return answersArr.map(answer => {
  const currentQuestion = questionsArr.filter(
   question => question.questionId === answer.questionId
  )

  const currentAnswer = currentQuestion[0].answers.filter(
   answerInCurrentQuestion => answerInCurrentQuestion.value === answer.value
  )[0]

  return createObj(
   answer.questionId,
   currentAnswer.value,
   currentAnswer.right
  )
 })
}

const countRightAnswers = answers => {
 return answers.filter(answer => answer.right).length
}

const countWrongAnswers = answers => {
 return answers.filter(answer => !answer.right).length
}

console.log(checkAnswers(questions, answersFromUser))
console.log("Right Answers " + countRightAnswers(checkAnswers(questions, answersFromUser)))
console.log("Wrong Answers " + countWrongAnswers(checkAnswers(questions, answersFromUser)))

This is not much of a code review answer, but I don't have time for more right now :(

Here's a stripped down version using only arrays for answers, with indexes (zero to four) as the question numbers. Essentially, it seems like much of your object structures aren't needed at all.

const correctAnswers = [1,4,2,1,3]

const userAnswers = [1,1,1,1,1]

const countRightAnswers = answers => answers.filter((a, i) => a == correctAnswers[i]).length

const indexOfRightAnswers = answers => answers.map((a, i) => a == correctAnswers[i] ? i : -1).filter(i => i > -1)

console.log(countRightAnswers(userAnswers))
console.log(indexOfRightAnswers(userAnswers))

Try it online!


Splitting Excel workbook sheets Running Office 2007, I have master workbook with data, which I:

Split master sheet data to new sheets for every salesmen
Create new workbook (with 1 sheet)
Put sheets of salesmen to an array according their manager
Copy array of sheets to new workbook
Delete first sheet on newly created workbook, save it as .xlsx and close

My problem is that it takes more time than I expect, around 8 seconds for one group of salesmen (there are 6 at the end) - altogether with formatting before splitting master sheet data around 60 seconds. I think there is problem in my code and with how Excel manages how and how much RAM it uses.

Master data workbook has 21 MB, .xls format
When I open it, it uses 50 MB of RAM
Then when I run code with some formatting etc it grows to 80 MB
Then I start splitting, where it slowly grows to  87, 95, 103, 110, 125, 132 and then finally to 160 MB, where it copies sheets and close new workbook, then again Excel uses 95 MB of RAM. Merging process creates 6 new workbooks with size from 0.6 to 1.8 MB, where these workbooks contain from 6 to 10 sheets.

Dim wb1 As Workbook, group As Variant, new_wb As Workbook

With Application
    .EnableEvents = False
    .ScreenUpdating = False
    .StatusBar = "Merging .."
End With

Set wb1 = ActiveWorkbook
'
'some formatting on master sheet, splitting to sheets
'adding sheets to an array called "group"
'for every manager do ..
'
Set new_wb = Workbooks.Add(xlWBATWorksheet)
With new_wb
    wb1.Sheets(group).Copy After:=.Sheets(1)
Application.DisplayAlerts = False
    .Sheets(1).Delete
Application.DisplayAlerts = True
.SaveAs C:\ & <name of manager from group> & ".xlsx", FileFormat:=51, .Close
End With

With Application
    .EnableEvents = True
    .ScreenUpdating = True
    .StatusBar = False
End With

However, when I prepared my workbook to the step when copying occurs, and run just one line of code:

wb1.Sheets(Array("Sheet1", "Sheet2","Sheet3","Sheet4")).Copy After:=.Sheets(1)

This way it, copies sheets almost instantly to new a workbook.

I'm not sure what I'm doing wrong. What is the proper way how to copy (Excel/RAM friendly) an array of sheets to a new workbook?

When I run this one line of code, it too uses 160 MB of RAM, only this way it takes enough RAM immediately, not slowly growing more and more.

I had a similar project for a client. I looped a table filter by employee name for the chosen manager. Then I moved the resultant table values to a new sheet and ran activesheet.move to create a new workbook.

I used your 'testname' saveas portion of the code to pass the manager folder variable and employee variable folder within my loop.

Saveas(...  & "/" & str_manager & "/" & str_employee & ".xlsx" )

The system stress calculations are interesting but not needed. That is because using arrays, counting, bubble sorting, loops are most efficient depending on the size of the dataset.  Mine was quicker when using a table filter loop


What changes can I make to simply this function that groups parameter key value pairs I am writing a function that can be used in a ruby script that will match the parameter and values together so the script can use them in an intelligent way. The idea is if you were to run a script with this function it would output the key value pairs of the parameters passed to the script.
For example
my_script.rb -user Jeff -key /path/to/key -config /path/to/config

The expected output would be an array with the key value pairs like
[ {:arg => 'user', :val => 'Jeff'}, {:arg => 'key', :val => '/path/to/key'}, {:arg => 'config', :val => '/path/to/config'} ]

This then could be used in any Ruby script that takes command line arguments and organizes it for them in a consistent way. Then the script can decide what to do with the output and act on those key value pairs passed.
My Question:
Are there are enhancements that can be made for this script and / or is there simpler ways to do this in Ruby? The function is below:
#!/usr/bin/env ruby

class Script_Helper
  def self.get_params (ary)
    args = Array.new
    unless ARGV.empty?
      for i in 0..ARGV.size-1
        if ARGV[i].start_with?('-') and i < ARGV.size + 1
          args.push ({:arg => ARGV[i], :val => ARGV[i+1]})
        end
      end
    end
    return args
  end
end

Consistency
Sometimes you are using whitespace around operators, sometimes you don't. Sometimes you are using whitespace between the message name and the argument list of a message send, sometimes you don't.
You should choose one style and stick with it. If you are editing some existing code, you should adapt your style to be the same as the existing code. If you are part of a team, you should adapt your style to match the rest of the team.
Most communities have developed standardized community style guides. In Ruby, there are multiple such style guides. They all agree on the basics (e.g. indentation is 2 spaces), but they might disagree on more specific points (single quotes or double quotes).
In general, if you use two different ways to write the exact same thing, the reader will think that you want to convey a message with that. So, you should only use two different ways of writing the same thing IFF you actually want to convey some extra information.
For example, some people always use parentheses for defining and calling purely functional side-effect free methods, and never use parentheses for defining and calling impure methods. That is a good reason to use two different styles (parentheses and no parentheses) for doing the same thing (defining methods).
Whitespace around operators
There should be 1 space either side of an operator.
Whitespace in hash literals
In a hash literal, there should be one space after the opening curly brace and one space before the closing curly brace.
New-style hash literals
Well, they are called "new-style" or "Ruby 1.9" hash literals, but really, they were introduced in 2007, and we have Ruby 3.0 soon, so we should really simply be calling them "hash literals" at this point. They are neither new anymore nor is Ruby 1.9 still relevant.
Ruby 1.9 introduced a shortcut syntax for hash literals where keys are symbols, inspired by ECMAScript.
So, this:
{:arg => ARGV[i], :val => ARGV[i+1]}

should be this:
{ arg: ARGV[i], val: ARGV[i+1] }

Frozen string literals
Immutable data structures and purely functional code are always preferred, unless mutability and side-effects are required for clarity or performance. In Ruby, strings are always mutable, but there is a magic comment you can add to your files (also available as a command-line option for the Ruby engine), which will automatically make all literal strings immutable:
# frozen_string_literal: true

It is generally preferred to add this comment to all your files.
No space before parameter list
In a method definition, there should be no whitespace between the method name and the parameter list, e.g. this:
def self.get_params (ary)

should be
def self.get_params(ary)

Ruby allows you to specify a parameter list either in parentheses or with whitespace. If you combine the two, it could get confusing, because Ruby also allows array destructuring in parameter lists:
def foo(((first_of_first, second_of_first), second))
  second_of_first
end

foo([[23, 42], 'Hello'])
#=> 42

Array destructuring is performed by adding an additional set of parentheses. Now, if you have a method with two parameters, you can write it like this:
def foo(bar, baz)
end

or like this:
def foo bar, baz
end

If you write it like this:
def foo (bar, baz)
end

A reader might think that this is a single parameter that uses array destructuring. In fact, Ruby even prints a warning that tells you exactly that:
warning: parentheses after method name is interpreted as an argument list, not a decomposed argument

No space before argument list
In a message send, there should be no whitespace between the message and the argument list, e.g. this:
args.push ({:arg => ARGV[i], :val => ARGV[i+1]})

should be
args.push({:arg => ARGV[i], :val => ARGV[i+1]})

This will bite you as soon as you have more than one argument. Ruby allows you to write the argument list to a message send without parentheses like this:
foo.bar baz, quux

instead of
foo.bar(baz, quux)

The problem arises because the whitespace triggers the first mode, so
foo.bar (baz, quux)

is not interpreted as two arguments baz and quux but as passing arguments with whitespace instead of parentheses and thus as a single argument (baz, quux). Except (baz, quux) is not legal Ruby code, and thus you get an error.
Warnings
The Ruby developers work hard on providing helpful warning messages. You should read them and investigate them. They almost always point to problems or at least potential problems in your code.
For example, running your code generates the following warning message:
warning: parentheses after method name is interpreted as an argument list, not a decomposed argument

Unused parameter
Your get_params method is defined with one parameter ary, but you are not using this parameter anywhere. It is completely useless.
If you have a parameter or a local variable that is intentionally unused, the Ruby community standard is to name it with a leading underscore, e.g. _ary or even just _.
However, in this case, it serves absolutely no purpose and should simply be removed altogether.
Empty literals
Instead of using the new method to create instances of core classes, you should prefer to use literals. E.g. use '' or "" instead of String.new, [] instead of Array.new, and so forth.
This is especially true for empty objects.
args = Array.new

should be
args = []

Prefer each over for / in
Nobody in Ruby uses for / in. In fact, we hardly use any loops at all. I would go so far as to say, when you are using a loop, you are doing it wrong.
The idiomatic way would be to use Range#each instead. Not only is each more idiomatic, but for / in has this weird scoping rule where the loop variable escapes into the surrounding scope, which is almost always not what you want.
So, this:
for i in 0..ARGV.size-1
  if ARGV[i].start_with?('-') and i < ARGV.size + 1
    args.push ({:arg => ARGV[i], :val => ARGV[i+1]})
  end
end

should become this:
(0..ARGV.size-1).each do |i|
  if ARGV[i].start_with?('-') and i < ARGV.size + 1
    args.push ({:arg => ARGV[i], :val => ARGV[i+1]})
  end
end

Actually, there is a lot more that can be improved about that, but we will stick with the purely mechanical transformations for now, and keep our brain switched off.
Modifier if
If you have a conditional expression with only a single expression in the body and no else, it is generally preferred to use the modifier form.
if ARGV[i].start_with?('-') and i < ARGV.size + 1
  args.push ({:arg => ARGV[i], :val => ARGV[i+1]})
end

should be
args.push ({:arg => ARGV[i], :val => ARGV[i+1]}) if ARGV[i].start_with?('-') and i < ARGV.size + 1

Prefer symbolic operators ||, &&, ! over keywords and, or, not
In a boolean expression, you should prefer the symbolic operators over the keywords. The keywords have doubly-unexpected precedence: not only is the precedence of the keywords and and or much lower than the precedence of && and ||, but also and and or both have the same precedence while && has higher precedence than ||. This can be very confusing.
Idiomatic Ruby community standard is to always use the symbolic operators, except for when you actually use boolean logic for control flow, as in
do_something and exit

So,
ARGV[i].start_with?('-') and i < ARGV.size + 1

should be
ARGV[i].start_with?('-') && i < ARGV.size + 1

Redundant return
The return value of a method is the value of the last expression that was evaluated inside the method. Unless you want an early return, it is generally not necessary to use return at all.
Linting
You should run some sort of linter or static analyzer on your code. Rubocop is a popular one, but there are others.
Rubocop was able to detect all of the style violations I pointed out above (plus some more), and also was able to autocorrect all of the ones I listed.
Let me repeat that: I have just spent two pages pointing out how to correct tons of stuff that you can actually correct within milliseconds at the push of a button. I have set up my editor such that it automatically runs Rubocop with auto-fix as soon as I hit "save".
In particular, running Rubocop on your code, it detects 19 offenses, of which it can automatically correct 17. This leaves you with 2 offenses, both of which are very simple.
Here's what the result of the auto-fix looks like:
#!/usr/bin/env ruby
# frozen_string_literal: true

class Script_Helper
  def self.get_params(_ary)
    args = []
    unless ARGV.empty?
      (0..ARGV.size - 1).each do |i|
        args.push({ arg: ARGV[i], val: ARGV[i + 1] }) if ARGV[i].start_with?('-') && (i < ARGV.size + 1)
      end
    end
    args
  end
end

And here are the offenses that Rubocop could not automatically correct:
Offenses:

script_helper.rb:4:1: C: Style/Documentation: Missing top-level class documentation comment.
class Script_Helper
^^^^^

script_helper.rb:4:7: C: Naming/ClassAndModuleCamelCase: Use CamelCase for classes and modules.
class Script_Helper
      ^^^^^^^^^^^^^

1 file inspected, 2 offenses detected

Let's look at the simple one first.
Naming
Class names should be PascalCase, constants should be SCREAMING_SNAKE_CASE, everything else should be snake_case. Files should be named after the primary module or class they define, and they should be named in snake_case as well.
So, the name of your class should be ScriptHelper, not Script_Helper.
Unused parameter, take 2
As mentioned above, the parameter should be removed. Rubocop only automatically renamed it, because removing it could break code that calls this method with an argument.
But since it isn't used, it should simply be removed and all callers fixed to not pass an argument.
Vertical whitespace
Your method could use some vertical whitespace to give the code more room to breathe. I would suggest at least separating the initialization at the beginning of the method, and the return at the end:
def self.get_params(_ary)
  args = []

  unless ARGV.empty?
    (0..ARGV.size - 1).each do |i|
      args.push({ arg: ARGV[i], val: ARGV[i + 1] }) if ARGV[i].start_with?('-') && (i < ARGV.size + 1)
    end
  end

  args
end

"Reader" methods
I re-run Rubocop every time I change something in my code. In fact, I have it set up to run automatically when I save my code, when I commit my code, etc.
Since we now have removed the parameter from the method, it now tells us something new: "Reader" methods or "getter methods" that don't take any arguments and simply return a value should not be named with a get_ prefix. This is unlike Java, for example.
So, the method should simply be named params.
Redundant check
If ARGV is empty, its size will be 0, which means that the range 0..ARGV.size - 1 will be the range 0..-1, which is empty. Iterating over an empty range doesn't do anything, so we can just delete the check for ARGV.empty?
Redundant check 2
You are iterating from 0 to size-1, therefore i will never be greater than size. The check i < ARGV.size + 1 will always be true, and is thus redundant and can be deleted.
Iterators, take 2
We already converted the for loop into an each iterator. However, you are iterating over the indices of ARGV and then pull out the elements using the indices. This is a very FORTRAN way of writing that code. It is much easier to iterate over the elements directly.
Even better, since you always need the to look at the current and the next element, we can iterate over a sliding window of 2 consecutive elements. That's exactly what Enumerable#each_cons does:
ARGV.each_cons(2) do |param, arg|
  args.push({ arg: param, val: arg }) if param.start_with?('-')
end

As above, if ARGV is empty, each_cons will simply not do anything, so we don't need to check.
The "shovel" operator <<
And while we are looking at this code: Array#push is not idiomatic. More precisely, it is only idiomatic if you are using the array as a stack, then you would use Array#push and Array#pop, since those are the standard names for the stack operations.
The idiomatic way of appending something to something else is the shovel operator, in this case Array#<<, so that should be
args << { arg: param, val: arg }

Iterators, take 3
We are still doing a lot of stuff manually. Initialize the empty array, loop over ARGV, append to the array, etc.
Whenever you have the pattern of "Initialize a result, loop over a collection appending to the result, return result", that is a fold. There are two implementations of fold in the Ruby core library, inject and each_with_object. inject is the more functional one, each_with_object is the more imperative one. So, for now, we will use each_with_object here, since the code is still pretty imperative, and that makes the relationship between the old and new code more clear:
args = []

ARGV.each_cons(2) do |param, arg|
  args << { arg: param, val: arg } if param.start_with?('-')
end

args

becomes
ARGV.each_cons(2).each_with_object([]) do |(param, arg), args|
  args << { arg: param, val: arg } if param.start_with?('-')
end

Now, if we want to make it purely functional, it would look like this:
ARGV.each_cons(2).inject([]) do |args, (param, arg)|
  if param.start_with?('-') then args + [{ arg: param, val: arg }] else args end
end

Iterators, take 4
Instead of filtering inside the iteration, we could instead chain iterators and filter first. This works for both our variations:
ARGV
  .each_cons(2)
  .select { |param, _| param.start_with?('-') }
  .each_with_object([]) { |(param, arg), args| args << { arg: param, val: arg } }

ARGV
  .each_cons(2)
  .select { |param, _| param.start_with?('-') }
  .inject([]) { |args, (param, arg)| args + [{ arg: param, val: arg }] }

Excursion: Generality of fold (inject / each_with_object)
When I wrote above that you can rewrite this iteration with inject or each_with_object, that was actually a tautological statement. I didn't even have to read the code to make this statement.
It turns out that fold is "general". Every iteration over a collection can be expressed using fold. This means, if we were to delete every method from Enumerable, except inject, then we could re-implement the entire Enumerable module again, using nothing but inject. As long as we have inject, we can do anything.
Iterators, take 5
So, what we did until now was replace the loop with a low-level iterator, replace the low-level iterator with a higher-level iterator, and decompose the iteration into two chained higher-level iterators.
However, we are still not done. What we are now doing is we take each element from our filtered sliding window collection, modifying it, and putting it into a new collection. So, really, what we are doing is transforming each element, or "mapping" each element to a new element.
This is called map and is also available in Ruby as Enumerable#map.
So, finally, our code looks like this:
ARGV
  .each_cons(2)
  .select { |param, _| param.start_with?('-') }
  .map { |param, arg| { arg: param, val: arg } }
end

What we did here, was to replace the general high-level iterator fold (which can do anything) with a more restricted, more specialized high-level iterator map. By using a more specialized iterator, we are able to better convey our semantics to the reader. Instead of thinking "Okay, so here we have an accumulator, and an element, and we do something with the element, and then append it to the accumulator … ah, I see, we are transforming each element", the reader just sees map and instantly knows that map transforms the elements.
Classes and modules
In your code, you are using a class. However, your class cannot sensibly be instantiated. Therefore, it shouldn't be a class.
It makes more sense for it to be a module instead. Here's how I would write your entire file:
#!/usr/bin/env ruby
# frozen_string_literal: true

module ScriptHelper
  module_function def params
    ARGV
      .each_cons(2)
      .select { |param, _| param.start_with?('-') }
      .map { |param, arg| { arg: param, val: arg } }
  end
end

Linter, revisited
Rubocop actually complains about my inline use of Module#module_function. It wants me to write this instead:
#!/usr/bin/env ruby
# frozen_string_literal: true

module ScriptHelper
  def params
    ARGV
      .each_cons(2)
      .select { |param, _| param.start_with?('-') }
      .map { |param, arg| { arg: param, val: arg } }
  end

  module_function :params
end

I disagree. You should not be afraid to disable or re-configure rules in your linter to fit your style.
However, note that programming is a team sport. If you are modifying code, adopt the existing style. If you are part of a team, adopt the team's style. If you write open source code, adopt the project's style. If you start your own project, adopt the community's style (do not create your own style for your project, until your project is big and successful enough to have its own independent community).
Speaking of community …
Of course, the really correct solution to your problem is to use one of the many, already existing, well-maintained, well-tested, well-documented, well-known command line option parser libraries. Ruby ships with optparse in the standard library, but there are many, many others with some amazing features:

Clamp
Commander
Mixlib::CLI
Optimist
slop

Don't re-invent the wheel.
It is true that NASA has (kind-of) re-invented the wheel for the Perseverance rover, but most of us are not NASA. We don't face the unique challenge of sending a rover to Mars that needs to autonomously drive over rough, unknown terrain, and needs to be as light while at the same time as robust as possible. We just want to get from A to B on a paved road. There are plenty of wheels available off-the-shelf that do that.
Always remember: we are not NASA. It is highly unlikely that a problem we are encountering has never before been seen in the history of humankind. Parsing command line arguments is not a highly-specialized, complex, unique problem, it is a problem that has been solved with standardized libraries over 30 years ago.


Test whether target string is substring of source string The code is self-explanatory. It has \$O(n)\$ complexity, and it tells whether a given string is the substring of the source string. Are there any cases where this algorithm fails? Is there anything that can be improved?

    public static boolean isSubStringOf(char src[], char target[]) {
    int pointTarget=0;
    for (int pointSrc = 0;pointSrc < src.length; pointSrc++) {
        // If target's pointer is at target's length , Voila !
        if (pointTarget == target.length)
            return true;

        //If value at src's pointer equals value at target's pointer increment target's pointer and continue.
        if (src[pointSrc] == target[pointTarget]) {
            pointTarget++;
            continue;
        }
        //However if they are not equal reset target's pointer back to target's beginning.
        else if (src[pointSrc] != target[pointTarget]) {
            pointTarget = 0;
        }
    }
    // To handle right corner case
    return pointTarget == target.length;
}

A BUG

Your method fails on string "baaaba" whenever searching for "aab". The problem here is that when you reset pointTarget to 0, you may ignore a prefix of the pattern.

NAMING

I suggest you rename src to text or string, and rename target to pattern.

CODE

After simplifying your code, you may get something like:

public static boolean isSubstringOf(char string[], char pattern[]) {
    int pointTarget = 0;

    for (int cursor = 0; cursor < string.length; cursor++) {
        if (pointTarget == pattern.length) {
            break;
        }

        if (string[cursor] == pattern[pointTarget]) {
            pointTarget++;
        } else {
            cursor -= pointTarget - 1;
            pointTarget = 0;
        }
    }

    return pointTarget == pattern.length;
}

The above runs in time \$\mathcal{O}(nm)\$, where \$n\$ is the length of the text to search and \$m\$ is the length of the pattern string. If you want to find the pattern in the text in linear time, you could use Knuth-Morris-Pratt algorithm.


Alphabet cypher in Rust This is my attempt at an alphabet cypher in Rust. I'm unsure if iteration and pushing to a mutable is the best way to handle building the output; it seems like a good candidate for a map but I can't figure out how to do it. Can anyone help?

use std::env;

fn main() {
    let keyword = env::args().nth(1).unwrap();
    let body = env::args().nth(2).unwrap();
    let transpose_table = get_transpose_table(keyword);

    let mut output = String::new();
    for (input_idx, input_char) in body.char_indices() {
        let transpose_distance = transpose_table[input_idx % transpose_table.len()];
        let transposed_char_idx = char_to_alpha_idx(input_char) + transpose_distance;
        output.push(alpha_idx_to_char(transposed_char_idx % 26));
    }

    println!("{}", output);
}

fn get_transpose_table(keyword: String) -> Vec<usize> {
    return keyword.chars()
                  .map(|character| char_to_alpha_idx(character))
                  .collect();
}

fn char_to_alpha_idx(character: char) -> usize {
    return "abcdefghijklmnopqrstuvwxyz".find(character)
                                       .expect("Bad char for char_to_alpha_idx");
}

fn alpha_idx_to_char(loc: usize) -> char {
    return "abcdefghijklmnopqrstuvwxyz".chars()
                                       .nth(loc)
                                       .expect("Bad loc for alpha_idx_to_char");
}

Disclaimer: I'm a Rust beginner with a background in C and C++. Now that I've lost my credibility, let's have a look at your code.

Use rustfmt, clippy and rustc

There are (at least) three helpful tools when one writes Rust code: rustfmt, clippy and the compiler rustc itself.

Rustfmt provides a common ground

The compiler doesn't yield any warnings, but rustfmt will change your code.
As many Rust developers use rustfmt (or cargo fmt, which calls the former), you should also make a habit of using it. That way others will feel right at home if they look at your code, and the same will hold for you when you look at theirs.

Clippy's warnings

clippy on the other hand will catch common mistakes. In this case its mostly the redundant returns in your idx functions:

fn get_transpose_table(keyword: String) -> Vec<usize> {
    keyword
        .chars()
        .map(|character| char_to_alpha_idx(character))
        .collect()
}

fn char_to_alpha_idx(character: char) -> usize {
    "abcdefghijklmnopqrstuvwxyz"
        .find(character)
        .expect("Bad char for char_to_alpha_idx")
}

fn alpha_idx_to_char(loc: usize) -> char {
    "abcdefghijklmnopqrstuvwxyz"
        .chars()
        .nth(loc)
        .expect("Bad loc for alpha_idx_to_char")
}

There is no need to return here. Next, you never consume the String in get_transpose_table, so we could borrow it instead with &String. But in the end, we never use any String-specific functionality, therefore we can just use &str.

While we're at it, map(|x| func(x)) is just map(func), so let's change that as-well:

fn get_transpose_table(keyword: &str) -> Vec<usize> {
    keyword.chars().map(char_to_alpha_idx).collect()
}

All those small improvements were reported by clippy.

Use T::with_capacity if you know how many elements you will have

For your main algorithm, you should use String::with_capacity if you know the number of characters you want to store:

let mut output = String::with_capacity(input.len());

Nothing else in your code changes.

Closures and readability

Apart from better names (message instead of body, body_index or message_ index instead of input_index) that's all we can do with this algorithm. We could transform it into a closure:

let output: String = message
    .char_indices()
    .map(|(index, message_char)| {
        let transpose_distance = transpose_table[index % transpose_table.len()];
        let transposed_char_idx = char_to_alpha_idx(message_char) + transpose_distance;
        alpha_idx_to_char(transposed_char_idx % 26)
    })
    .collect();

but we only removed mutability. So let's get back to the drawing table. Why do we need index? Because we want to know which usize we should use from the transpose_table. We would like to traverse a cycle of those values at the same time as our original message.

That's exactly what cycle() and zip() are for:

let output: String = transpose_table
    .iter()
    .cycle()
    .zip(message.chars())
    .map(|(transpose_distance, message_char)| {
        let transposed_char_idx = char_to_alpha_idx(message_char) + transpose_distance;
        alpha_idx_to_char(transposed_char_idx % 26)
    })
    .collect();

Whether that's easier to understand than your original variant is left to you. However, we can also get rid of the intermediate Vec:

let output: String = keyword
    .chars()
    .map(char_to_alpha_idx)
    .cycle()
    .zip(message.chars())
    .map(|(transpose_distance, message_char)| {
        let transposed_char_idx = char_to_alpha_idx(message_char) + transpose_distance;
        alpha_idx_to_char(transposed_char_idx % 26)
    })
    .collect();

This is a handful, but it works exactly as your previous variant.

Encapsulate functionality in functions

Now, my main critique point is that you don't provide a single function for your cipher. It's hard to test your functionality at the moment, so we better add one and some documentation:

/// Applies [The Alphabet Cipher] given by `keyword` to `message`.
///
/// Both `message` and `keyword` must only contain lowercase alphabetic ASCII
/// characters.
///
/// # Panics
/// Panics if any non-lowercase or non-alphabetic ASCII character is encountered.
///
/// # Examples
/// The character 'a' in the keyword won't change the input:
/// ```
/// assert_eq!(alphabet_cipher("aaaaa", "hello"), "hello");
/// ```
///
/// They keyword gets cycled if its  shorter than the message:
/// ```
/// assert_eq!(alphabet_cipher("a", "hello"), "hello");
/// assert_eq!(alphabet_cipher("b", "hello"), "ifmmp");
/// assert_eq!(alphabet_cipher("ab","hello"), "hflmo");
/// assert_eq!(alphabet_cipher("ba","hello"), "iemlp");
/// ```
///
/// [The Alphabet Cipher]: https://en.wikipedia.org/wiki/The_Alphabet_Cipher
fn alphabet_cipher(keyword: &str, message: &str) -> String {
    keyword
        .chars()
        .map(char_to_alpha_idx)
        .cycle()
        .zip(message.chars())
        .map(|(offset, message_char)| {
            let index = char_to_alpha_idx(message_char) + offset;
            alpha_idx_to_char(index % 26)
        })
        .collect()
}

The code snippets in the comments also get tested by cargo test, but we can also add some more tests to check that our functions work, for example:

#[test]
fn alphabet_cipher_rot_a_to_z() {
    assert_eq!(
        alphabet_cipher("abcdefghijklmnopqrstuvwxyz", "aaaaaaaaaaaaaaaaaaaaaaaaaa"),
        "abcdefghijklmnopqrstuvwxyz"
    );
}

There are more tests one can come up with, but those are left as an exercise.

User friendly interface

You use unwrap() quite a lot. However, that leaves a user with unhelpful error messages:

$ ./cypher test
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', ...

$ ./cypher test test12345
thread 'main' panicked at 'Bad char for char_to_alpha_idx', ...

Those error messages aren't that helpful. They're fine for toy programs, but you should view unwrap as "I know that this will be Ok or Some, and it's fine to panic if it isn't, because I don't know how to continue". However, even in those cases, the end user should know how to continue. Keep that in mind when you write larger programs.


Aligning text in Haskell This Haskell program just aligns the text lines on the given substrings.

I took inspiration from here, but my code is not golfed.

import Data.Function
import Data.Ord
import Data.List
import Data.List.Split

alignOn :: String -> [String] -> [String]
alignOn char lines = map padline lines
  where
    partBeforechar = head . splitOn char
    longestLengthBeforeChar = maximum $ map (length . partBeforechar) lines
    padline line = replicate offset ' ' ++ line
      where
        offset = longestLengthBeforeChar - (length (partBeforechar line))

main = do
  mapM putStrLn $ alignOn "," ["Programming, Puzzles", "And, Code Golf"]
  mapM putStrLn $ alignOn ", " ["Code, Review", "And Other, improvements"]
  mapM putStrLn $ alignOn "abc" ["Example abc bar", "foo abc Example"]

Output:

Programming, Puzzles
        And, Code Golf

     Code, Review
And Other, improvements

Example abc bar
    foo abc Example

Use -Wall

When you are evaluating your code, you should use Haskell -Wall verbosity.

ghc -Wall mycode.hs

or

runghc -Wall mycode.hs

For example, it will tell you:

Unneeded imports (the import of ... is redundant)
Inappropriate use of variable names (this binding for ... shadows the existing binding)
Missing type signatures (top-level binding with no type signature ...)
etc.

This is generally good advice.

Specifying imported functions and types in import statements helps future reading of your code to determine why a particular import is needed.

For example:

import Data.List.Split (splitOn)

Use hlint

hlint is another useful tool to help you improve your code. For example, it would tell you that parentheses are unnecessary in the following case:

longestLengthBeforeChar - (length (partBeforechar line))

That could be replaced by:

longestLengthBeforeChar - length (partBeforechar line)

Or that, you shouldn’t use mapM if you don’t intend to use the result. You should use mapM_ (note the underscore) to make it clear.

Again, like -Wall, hlint suggestions are generally good advice you should follow.

Factorize

When you often see a pattern in your code, it may be an alert that it needs
factorization.

For example:

mapM putStrLn $ alignOn "," ["Programming, Puzzles", "And, Code Golf"]
mapM putStrLn $ alignOn ", " ["Code, Review", "And Other, improvements"]
mapM putStrLn $ alignOn "abc" ["Example abc bar", "foo abc Example"]

could be replaced by:

let tests = [ (","  , ["Programming, Puzzles", "And, Code Golf"])
            , (", " , ["Code, Review", "And Other, improvements"])
            , ("abc", ["Example abc bar", "foo abc Example"])
            ]

mapM_ (putStr . unlines . uncurry alignOn) tests

(better: use QuickCheck and put the test code outside your module)

Naming convention

You should use appropriate names for your parameters. Naming char a parameter which is in fact a String is misleading. It also does not give any indication on the role of this parameter. It could be renamed separator, for example.

You should also avoid to use a name that will shadow another one. You use lines in your function as a parameter, but lines is a well defined function (the counterpart of unlines).

Warning

Some Haskell functions like head can generate exceptions:

head [] -- Exception: Prelude.head: empty list

When you use it in your code, you should ensure an empty list will never occur (since it comes right after the splitOn function, it is safe to use it).

Coding convention

Avoid a where clause inside another where clause. It generally indicates the code should be split because your function is complex. Complex functions are harder to debug.

In fact, in your code, the alignOn can be splitted into independent, reusable and testable functions.

For example:

alignOn :: String -> [String] -> [String]
alignOn sep rows = fmap padline rows
    where padding row = longestLengthBefore sep rows - lengthBefore sep row
          padline row = replicate (padding row) ' ' ++ row

lengthBefore :: String -> String -> Int
lengthBefore sep = length . head . splitOn sep

longestLengthBefore :: String -> [String] -> Int
longestLengthBefore sep = maximum . fmap (lengthBefore sep)

Note:

This example is NOT optimized.
fmap should be preferred to map as it is more general.

Generalizing

The padding character could be defined as a parameter.

Optimization

You could rewrite your code to use Text instead of String.


Unit Testing for isPrime function I've decided that I want to take a stab at test first programming. So, before I tackled writing an isPrime function, I wrote this unit test. It's my first and I'm not sure I'm doing this right.

I was thinking that I might want to extract the loops to just two methods that I would pass an array to. One for Assert.IsTrue and one for Assert.IsFalse, but I wasn't sure if that was a good idea in a unit test.

Am I covering my bases here?
What other cases am I missing?
What would you do differently?

using System;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Challenges;
using System.Numerics;

namespace ChallengesTest
{
    [TestClass]
    public class PrimeTest
    {
        [TestMethod]
        public void SmallPrimes()
        {
            int[] numbers = { 2, 3, 5, 7, 11, 13, 17, 19, 23, 29 };

            for (var i = 0; i < numbers.Length; i++)
            {
                Assert.IsTrue(Numbers.isPrime(numbers[i]));
            }

        }

        [TestMethod]
        public void Negatives()
        {
            int[] numbers = { -2, -3, -5, -7, -11, -13, -17, -19, -23, -29 };

            for (var i = 0; i < numbers.Length; i++)
            {
                Assert.IsFalse(Numbers.isPrime(numbers[i]));
            }
        }

        [TestMethod]
        public void PositiveNotPrime()
        {
            int[] numbers = { 4, 6, 8, 9, 10, 12, 14, 15, 16, 18, 20, 21, 22, 24, 25 };

            for (var i = 0; i < numbers.Length; i++)
            {
                Assert.IsFalse(Numbers.isPrime(numbers[i]));
            }
        }

        [TestMethod]
        public void ZeroAndOne()
        {
            Assert.IsFalse(Numbers.isPrime(0));
            Assert.IsFalse(Numbers.isPrime(1));
        }

        [TestMethod]
        public void BigPrimes()
        {
            int[] numbers = {104677,104681, 104683, 104693, 104701, 104707, 104711, 104717, 104723, 104729 };

            for (var i = 0; i < numbers.Length; i++)
            {
                Assert.IsTrue(Numbers.isPrime(numbers[i]));
            }
        }
    }
}

Methods in C# are written in PascalCase, so let's change isPrime to IsPrime.

You can use LINQ to make the code a bit clearer. Instead of

for (var i = 0; i < numbers.Length; i++)
{
    Assert.IsTrue(Numbers.isPrime(numbers[i]));
}

You can write

Assert.IsTrue(numbers.All(Numbers.IsPrime));

Instead of

for (var i = 0; i < numbers.Length; i++)
{
    Assert.IsFalse(Numbers.isPrime(numbers[i]));
}

You can write

Assert.IsFalse(numbers.Any(Numbers.IsPrime));

or

Assert.IsTrue(numbers.All(n => !IsPrime(n)));

whichever you find easier to read (I prefer the former).

In fact, I would be a bit lazy and copy a big list of prime numbers from somewhere and then write

var primes = Enumerable.Range(0, 1000000).Where(Numbers.IsPrime);
Assert.IsTrue(primes.SequenceEqual(PrimesLessThanOneMillion));

If you take @mleyfman's suggestion to test with randomly generated primes, be sure to seed the random number generator. Unit tests must be reproducible.

That is, never do this

var random = new Random();

but do this

var random = new Random(42);

One of the benefits of unit tests is that if they fail, you know it is due to a change in the code, and can then git bisect (or similar) to find the breaking commit. If a unit test fails due to something that is not a change to the code (e.g. a different RNG seed) you have lost this important benefit.

Edit: as @svick pointed out, using the same seed may not generate the same sequence between different versions or implementations of .NET. See these answers on StackOverflow. If all dev machines (including the build server) are running the same version of .NET, you should be fine.