String replacement in OCaml This is part of my first OCaml program.

Its job is to replace a set of placeholder characters with umlauts. Taking the German word Ruebe as an example, the program turns it into Rübe. Other examples are Moewe -> Möwe and aendern -> ändern

Generally, everytime the program encounters the characters ae, oe, and ue it turns them into umlauts. There is an exception though: If the preceding character is a vowel, it does not change the word. This ensures words like Treue remain as they are (since there is no word Treü).

I've used the library Re2 for regular expressions. Re2 doesn't implement lookarounds, which was my first thought for checking for preceding vowels. This is the reason for the function replace_if_not_after_vowel.

I am happy for all suggestions, especially if they help make the code more idiomatic or simpler.

To compile the program, I used the command ocamlbuild -use-ocamlfind -package re2 -package core -tag thread myCompiledFile.byte

open Core.Std
open Re2.Std
open Re2.Infix

(* If the word contains an umlaut placeholder like "ue" we replace that with the proper umlaut "ü". Except if there is a vowel directly before the placeholder like in "Treue" *)
let placeholders_to_umlauts = [("ue", "ü"); ("oe","ö"); ("ae","ä"); ("Ue", "Ü"); ("Oe","Ö"); ("Ae","Ä")]

(* A regex that matches a vowel *)
let vowel = ~/"[aeiou]"

(* Applies a list of changes to a word *)
let rec apply_changes word changes =
  match changes with
  | [] -> word
  | change :: rest -> apply_changes (change word) rest

(* Replaces replace_this with replacement inside the text if the preceding character not a vowel. Since Re2 doesn't implement lookarounds we can't use a negative lookbehind *)
let replace_if_not_after_vowel replace_this replacement text =

  Re2.replace_exn ~/replace_this text ~f:(fun regex_match ->
   (* Returns true if there is a vowel at the given position in the text *)
   let is_vowel text pos =
     if pos >= 0 && pos < String.length text then
       let maybe_vowel = String.get text pos in
       Re2.matches vowel (Char.to_string maybe_vowel)
     else false
   in

   (* Get the position in the text where the regex matched *)
   let match_pos, _ = Re2.Match.get_pos_exn ~sub:(`Index 0) regex_match in

   (* Replace the placeholder if doesn't follow a vowel *)
   if is_vowel text (match_pos -1 ) then replace_this else replacement
   )

let change_word word =
  (* Those are the changes that we will apply to the word *)
  let changes = List.map placeholders_to_umlauts ~f:(
    fun (placeholder, umlaut) -> replace_if_not_after_vowel placeholder umlaut
  )
  in

  apply_changes word changes

  let () =
    (* We want to change this word into "Übergrößenträgertreue" *)
    let word = "Uebergroeßentraegertreue" in
    let new_word = change_word word in

    printf "new word: %s\n" new_word

Re version, since it was amusing to do. My version uses a slightly different technique: I build one big regexp, with two groups and replace everything in one go without any additional checking. If Re.replace provided slightly more control (per-group substitution) it would avoid the concatenation.

I used the combinators for building the regexp, instead of the symbolic version, because that's much more readable, really.

let map_to_umlauts =
  [ "ue","ü" ; "oe","ö" ; "ae","ä" ; "Ue","Ü" ; "Oe","Ö" ; "Ae","Ä" ]

let regexp =
  let open Re in compile @@
  seq [
    group @@ alt [ bow ; compl [no_case @@ set "aeiouy"] ] ;
    group @@ alt (List.map (fun (s,_) -> str s) map_to_umlauts) ;
  ]

let replace s =
  let f subs =
    Re.get subs 1 ^ List.assoc (Re.get subs 2) map_to_umlauts
  in
  Re.replace ~f regexp s

let () =
  print_endline @@ replace Sys.argv.(1)

On your version, apart from the change in algorithm, I have only one comment: You really don't need a regexp to check if a char is a voyel. ;)

Also, do note all of this is playing fast and, more important, very very loose, with Unicode.


My first accumulators Notes

I'm working my way through SICP, and as I got very confused by the section on folds, I decided to try to implement foldr in scheme and javascript to understand how it works differently with immutable and mutable variables.

I'm mostly looking for advice on whether or not what I've done makes sense idiomatically in each language, but any other feedback would be very much appreciated.

Scheme

;Foldr implemented so I can understand it
(define (foldright function initial collection)
        (if (not (pair? collection))
             initial
            (foldright function (function initial (car collection)) (cdr collection))))

;Test Case (should return 10)
(foldright (lambda (x y) (+ x y)) 0 (list 1 2 3 4))

Javascript

function foldr(func, initial, collection) {
    if (collection.length === 0) {
        return initial
    };
    initial = func(initial, collection[0]);
    collection.shift();
    return foldr(func, initial, collection);
};

//Test case (should return 10)
var result = foldr(function (a, b) {
    return a + b;
}, 0, [1, 2, 3, 4]);

console.log(result);

I can only speak to the JS code - which looks pretty good, by the way... except:

You're modifying the array you're folding. In fact, you're truncating it completely due to your use of shift. So your function has destructive side-effects. After the fold, you have your answer, but you've lost the question, so to speak.

I.e.,

var question = [2, 3, 7];
var answer = foldr(function (a, b) { return a * b; }, 0, input);

console.log(answer);   // => 42
console.log(question); // => [] ... oops

Hence it'd be better to do something like this, using slice(1) to get a copy of the array starting from index 1:

function foldl(func, initial, collection) { // not foldr (see below)
    if (collection.length === 0) {
        return initial;
    }
    initial = func(initial, collection[0]);
    return foldl(func, initial, collection.slice(1));
}

By the way: You'll see I've removed the semicolon after the if block - it's not necessary (JS just ignores it, as it does the semicolon after the function body). But I've inserted a semicolon after return initial inside the block. That semicolon isn't strictly necessary either, since JS will see the close-brace following it, and insert its own semicolon. But better to do it properly yourself.

You could also write the condition as simply if (!collection.length), and it could be considered idiomatic. But I prefer your current - strict and explicit - condition myself.

Lastly, I'd prefer the Node.js style of placing function arguments at the end, like foldr(initial, collection, func) only because it avoids the dangling arguments after an inline function. On the other hand, JavaScript itself favors putting function arguments first, as in the built-in Array.prototype.reduce function... it's a tough call (no pun intended).

Update: I hadn't even noticed, until I read your comment above, but yes, your function is actually foldl, not foldr. You're reducing the array from first to last (i.e. from the left). So the first value passed to func is the first element in the array. A foldr function would pass the values in reverse order. A real foldr function could be:

function foldr(func, initial, collection) { // for real this time
    if( collection.length > 1) {
        initial = foldr(func, initial, collection.slice(1));
    }
    return func(initial, collection[0]);
}

You can check the behavior like this:

var func = function (memo, value) {
    memo.push(value);
    return memo;
};

foldl(func, [], [1, 2, 3, 4]); // => [1, 2, 3, 4]
foldr(func, [], [1, 2, 3, 4]); // => [4, 3, 2, 1] (reversed!)

Update 2: While I'm at it, here's a more memory efficient solution, that doesn't use slice and thus doesn't create n-1 arrays along the way. A similar technique can be used for foldl

function foldr(func, memo, collection) {
    var l = collection.length - 1;
    function fold(offset) {
        if( offset < l ) {
            memo = fold(offset + 1);
        }
        return func(memo, collection[offset]);
    }
    return fold(0);
}

Of course, neither this nor the functions above actually check whether the input array is empty (of if it's even an array)... not that that's terribly difficult to do, but I've complicated things enough already, I think :)


Caching data by using the result of first running operation My code handles concurrent requests by waiting for the result of an already running operation. Requests for data may come in simultaneously with same/different credentials (including empty credentials).

For each unique set of credentials there can be at most one GetCurrentInternal call in progress, with the result from that one call returned to all queued waiters when it is ready.

private static readonly Credentials EmptyCredentials = new Credentials
{
    SqlCredentials = null,
    ExchangeCredentials = null,
};

public AgentMetadata GetCurrent(Credentials credentials)
{
    var agentMetadata = new Lazy<AgentMetadata>(() => GetCurrentInternal(credentials));

    var lazyMetadata = (Lazy<AgentMetadata>)MemoryCache.Default.AddOrGetExisting(
                       (credentials ?? EmptyMetadataCredentials).ToString(),
                       agentMetadata,
                       DateTime.Now.AddMinutes(5));

    try
    {
        return (lazyMetadata ?? agentMetadata).Value;
    }
    catch (InvalidOperationException ex)
    {
        _logger.ErrorException(ex, "An error occurs during getting full metadata");
        throw;
    }
}

Is it thread safe? Do I have a bugs? How to create a good key for the AddOrGetExisting?

Credentials its a DataContract that contains other DataContract like SqlCredentials, ExchangeCredentials. And they contains their own strings like user_name and password.

Yes, it seems thread safe.

I run a test to prove it which I give the details at the bottom, but first are some code review comments.

The code is missing the Credentials class which I think is also important for the review. The cache items are added with keys credentials.ToString() and it is important for this method to return the same value for Credentials objects having the same credential values, and distinct values for Credentials instances with different credential values. Is it?
Naming conventions. Class names should be PascalCase and the class AgentMetadata violates this rule. Should be AgentMetaData
To achieve better encapsulation and seperation of concerns, (The creation of an empty credential object should be a concern for the Credentials class, which has control over the internals of itself) instead of having a static readonly Credentials EmptyCredentials field in this class, it is better to have a public static readonly Credentials Empty field (or property getter) in Credentials class.

Instead of this:

Credentials empty = (credentials ?? EmptyMetadataCredentials);

Isn't it better to have this?

Credentials empty = (credentials ?? Credentials.Empty);

The variable names agentMetadata and lazyMetadata are not descriptive enough.

This is somewhat confusing:

return (lazyMetadata ?? agentMetadata).Value;

Having names like the following might eliminate that confusion:

var newLazyInstance = new Lazy<AgentMetadata>(() => GetCurrentInternal(credentials));

var cachedLazyInstance = (Lazy<AgentMetadata>)MemoryCache.Default.AddOrGetExisting(
                           (credentials ?? EmptyMetadataCredentials).ToString(),
                           newLazyInstance,
                           DateTime.Now.AddMinutes(5));

return (cachedLazyInstance ?? newLazyInstance).Value;

And here are the results of the test, which shows the GetCurrentInternal method is invoked a total of 5 times for all the 100 threads.

The test created 100 threads which access the GetCurrent method randomly within 500 ms. There is another thread for removing the item from the cache in 100 ms continuously (This is because, the cache policy doesn't seem to effectively remove the item at the exact point of time of expiration).

The GetCurrentInternal method lasts for a random time between 0 and 500 ms

With the following results, it can be seen that the GetCurrentInternal method is invoked a total of 5 times (which is the number of the cache not containing a Lazy instance for the given credentials, first one because the cache is empty, and for 4 more times because it is removed from the cache)

I can't help myself thinking why the AddOrGetExisting of MemoryCache returns null  when the item is added to the cache. This would have been implemented differently (as returning the added instance as ConcurrentDictionary does)

T:47    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 145,68
T:13    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 146,44
T:20    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 146,54
T:26    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 147,45
T:71    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 148,46
T:62    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 148,55
T:53    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 149,37
T:5 M:GetCurrent    E:AddOrGetExisting.Added?   R:False 146,68
T:22    M:GetCurrent    E:AddOrGetExisting.Added?   R:True  150,66
T:31    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 150,18
T:63    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 154,58
T:50    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 157,15
T:27    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 182,13
T:67    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 182,33
T:43    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 182,18
T:32    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 182,13
T:3 M:GetCurrent    E:AddOrGetExisting.Added?   R:False 187,24
T:55    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 194,8
T:29    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 197,13
T:28    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 214,17
T:4 M:GetCurrent    E:AddOrGetExisting.Added?   R:False 221,11
T:84    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 228,12
T:78    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 232,09
T:19    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 235,1
T:66    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 238,1
T:7 M:GetCurrent    E:AddOrGetExisting.Added?   R:False 242,14
T:61    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 245,11
T:49    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 246,15
T:23    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 250,08
T:85    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 251,1
T:81    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 254,1
T:98    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 254,12
T:44    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 254,13
T:91    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 255,25
T:65    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 257,2
T:59    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 259,33
T:56    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 269,1
T:34    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 271,11
T:89    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 271,12
T:41    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 279,12
T:100   M:GetCurrent    E:AddOrGetExisting.Added?   R:False 280,12
T:11    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 290,11
T:17    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 292,08
T:58    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 293,1
T:14    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 297,09
T:90    M:GetCurrent    E:AddOrGetExisting.Added?   R:True  303,12
T:75    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 312,15
T:46    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 318,17
T:45    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 321,11
T:10    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 322,1
T:92    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 328,1
T:80    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 339,12
T:8 M:GetCurrent    E:AddOrGetExisting.Added?   R:False 341,11
T:51    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 342,11
T:74    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 343,11
T:86    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 351,12
T:68    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 352,11
T:95    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 354,1
T:72    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 359,11
T:48    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 360,11
T:18    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 364,1
T:6 M:GetCurrent    E:AddOrGetExisting.Added?   R:False 374,12
T:35    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 377,09
T:15    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 377,11
T:93    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 378,1
T:33    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 379,1
T:9 M:GetCurrent    E:AddOrGetExisting.Added?   R:False 390,12
T:39    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 391,09
T:12    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 395,11
T:24    M:GetCurrent    E:AddOrGetExisting.Added?   R:True  401,11
T:40    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 402,11
T:30    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 404,09
T:70    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 411,11
T:102   M:GetCurrent    E:AddOrGetExisting.Added?   R:False 412,1
T:52    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 413,11
T:57    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 417,1
T:37    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 426,13
T:69    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 428,1
T:16    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 432,11
T:21    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 438,13
T:96    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 441,11
T:38    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 442,09
T:54    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 456,18
T:82    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 465,12
T:76    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 466,11
T:25    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 470,11
T:36    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 477,09
T:73    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 483,12
T:42    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 483,13
T:101   M:GetCurrent    E:AddOrGetExisting.Added?   R:False 491,13
T:64    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 494,12
T:60    M:GetCurrent    E:AddOrGetExisting.Added?   R:True  502,11
T:97    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 528,2
T:24    M:GetCurrentInternal    E:Invoked   R:AgentMetadata_ID=1    547,76
T:24    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=1    548,3
T:70    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=1    548,36
T:40    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=1    548,32
T:57    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=1    549,27
T:21    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=1    561,53
T:36    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=1    574,77
T:102   M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=1    548,4
T:38    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=1    563,24
T:101   M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=1    575,21
T:77    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 579,16
T:25    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=1    571,16
T:54    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=1    567,6
T:73    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=1    574,79
T:42    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=1    574,8
T:76    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=1    567,61
T:37    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=1    554,64
T:82    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=1    567,6
T:52    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=1    548,38
T:64    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=1    575,44
T:16    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=1    557,74
T:94    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 587,92
T:69    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=1    556,54
T:96    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=1    562,78
T:30    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=1    548,35
T:88    M:GetCurrent    E:AddOrGetExisting.Added?   R:True  616,19
T:103   M:GetCurrent    E:AddOrGetExisting.Added?   R:False 631,16
T:90    M:GetCurrentInternal    E:Invoked   R:AgentMetadata_ID=2    642,16
T:90    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=2    642,37
T:80    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=2    644,54
T:72    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    648,45
T:47    M:GetCurrentInternal    E:Invoked   R:AgentMetadata_ID=3    645,16
T:47    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    650,61
T:26    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    652,63
T:22    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    654,89
T:9 M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    654,43
T:86    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    646,88
T:18    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    648,75
T:6 M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    649,06
T:15    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    650,84
T:74    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    646,62
T:53    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    658,94
T:31    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    662,34
T:46    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=2    642,4
T:62    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    652,69
T:95    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    646,88
T:67    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    664,69
T:71    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    660,52
T:93    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    650,84
T:45    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=2    642,4
T:50    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    652,69
T:23    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    677,68
T:78    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    672,49
T:66    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    672,5
T:4 M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    671,15
T:10    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=2    642,44
T:5 M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    657,11
T:65    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    678,46
T:29    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    669,35
T:81    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    679,46
T:33    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    649,06
T:92    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=2    642,46
T:14    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    688,26
T:13    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    652,62
T:17    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    688,25
T:28    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    671,12
T:89    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    684,54
T:11    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    686,67
T:100   M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    686,67
T:58    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    688,25
T:61    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    676,52
T:20    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    660,51
T:34    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    684,53
T:68    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    646,88
T:85    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    674,46
T:44    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    677,66
T:49    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    673,36
T:32    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    667,5
T:3 M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    669,31
T:41    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    686,66
T:39    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    654,43
T:7 M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    672,5
T:48    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    648,47
T:12    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    655,34
T:19    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    672,5
T:83    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 672,09
T:98    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    680,64
T:63    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    662,4
T:91    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    677,96
T:8 M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=2    644,56
T:75    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=2    642,38
T:43    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    665,46
T:27    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    662,37
T:59    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    680,63
T:55    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    669,34
T:99    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 695,73
T:51    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    646,62
T:56    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    684,53
T:84    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    672,24
T:87    M:GetCurrent    E:AddOrGetExisting.Added?   R:False 691,88
T:35    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=3    650,56
T:60    M:GetCurrentInternal    E:Invoked   R:AgentMetadata_ID=4    988,26
T:60    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=4    988,88
T:77    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=4    988,96
T:94    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=4    989,03
T:97    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=4    988,97
T:88    M:GetCurrentInternal    E:Invoked   R:AgentMetadata_ID=5    1050,3
T:88    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=5    1050,85
T:83    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=5    1050,94
T:87    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=5    1052,18
T:103   M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=5    1050,91
T:99    M:GetCurrent    E:Lazy.Value    R:AgentMetadata_ID=5    1050,97


Asynchronous array methods chaining I am trying to check that a list of files are all images using functional programming.

Some actions I go through are asynchronous, so I have to wait for them to completely finish before passing to a new one.

const filteredFiles = ['favicon.ico', 'site.webmanifest', 'browserconfig.xml'];
const fileBuffers = await Promise.all(
    directory.files
        .filter(file => !filteredFiles.includes(file.path))
        // get a buffer containing the image file
        .map(async file => await file.buffer())
)
const everyFileIsImage = (await Promise.all(
    // check that this buffer has correct mime type
    fileBuffers.map(async buffer => await this.bufferIsImageFile(buffer))
)).every(isImage => isImage);

if (!everyFileIsImage) {
    throw new BadRequestException(`All files except ${filteredFiles} must be images`);
}

Here is the bufferIsImageFile method which is using the file-type library

private async bufferIsImageFile(buffer) {
    const type = await FileType.fromBuffer(buffer);

    return type !== undefined && type.mime.startsWith('image/');
}

I am not all satisfied by the piece of code. But I'm also not sure on how it could be improved.

What I'd like the most would be to avoid using those Promise.all, but is it even possible?

Maybe should I use a for loop instead?

Some actions I go through are asynchronous, so I have to wait for them to completely finish before passing to a new one.

That's right. However, your interpretation of that statement is wrong.

The actions need to be serialized, but only on a per file basis.

const buffer = await file.buffer();
return await this.bufferIsImageFile(buffer);

Concurrency

You can further proceed with Promise.all as you did, but it has a few caveats.

Since any file that is not image means you should return false, not caring about the others. And so you should be able to cancel all the other file scans once you find the first file that is not image. Native promises are not cancelable but I am sure there are user land implementations.
You have no control about the number of files consumers will ask for. If they ask for a lot of files, you start a lot of async tasks and this may consume a lot of memory. You may want to only start a limited amount of tasks at a time and add more once some finishes.

The latter can be solved using Promise.race, but the actual implementation is not that trivial and so I will not write this for you :)

Maybe this SO question might be of help here:
https://stackoverflow.com/questions/42896456/get-which-promise-completed-in-promise-race

Or you may want to use some promises wrapper like bluebird which also provides cancellation mechanism (http://bluebirdjs.com)

Filtering

The filteredFiles list seems to me too hard coded (but it all seems like you pulled it together just for this review, in which case you may consider this irrelevant).

It should be injected to the method somehow, your options include:

method argument (thats awkward tho)
class property injected through constructor
decorator pattern (https://lmgtfy.com/?q=decorator+pattern+nodejs)


Plotting polynomials roots NOTE: See follow up to this question here

I created a simple python script to plot quadratic, cubic and quartic polynomials with integer coefficients between -4 and 4. It uses numpy to find the roots for the polynomials and matplotlib for the actual plotting of the points. Because I want to plot all possible polynomial with integer coefficients between -4 and 4 I figured I needed to do a recursive function with a loop. However if I'm not mistaken this will give a time complexity of \$O(n!)\$.

import matplotlib

# Use Qt4Agg because otherwise matplotlib crashes on my computer running Arch Linux
matplotlib.use('Qt4Agg')
import matplotlib.pyplot as plt
import matplotlib.cm as cm
import numpy as np

min_degree = 2
max_degree = 3
colours = []

def polynomial_scatter(ax, min_x, max_x, min_degree, max_degree, cur_degree, coeff):
    global colours

    if cur_degree <= max_degree:
        for c in range(min_x, max_x):
            new_coeff = coeff + [c]
            polynomial_scatter(ax, min_x, max_x, min_degree, max_degree, cur_degree + 1, new_coeff)
            if cur_degree >= min_degree:
                roots = np.roots(new_coeff)

                # check if no possible solution exists
                if len(roots) == 0:
                    continue

                # the real part of the roots will be our x values, the imaginary parts will be our y values
                x, y = zip(*[(t.real, t.imag) for t in roots])

                # scatter the roots for this polynomial with a colour corresponding to the degree of the polynomial
                ax.scatter(x, y, c=colours[cur_degree - max_degree], alpha=.5, s=1)

if __name__ == "__main__":
    f, ax = plt.subplots()
    colours = cm.rainbow(np.linspace(0, 1, max_degree - min_degree + 1))
    polynomial_scatter(ax, -4, 4, min_degree, max_degree, 0, [])
    plt.savefig("plot.svg", format="svg")
    plt.show()

Here are some pictures showing the result:

if I set max_degree to 5 it takes between 7-15 hours on my computer which is very long. Perhaps there is a way to implement multithreading.

How can I improve the performance of the polynomial_scatter algorithm?

I know that you subsequently posted c++ code for this and that post has answers, one of which is accepted.

I did notice that the code above references the global variable colours. This may be seen as bad programming style because it can cause side effects that may be challenging to detect. Alternatively, function parameters could be used, or else a  class with a class/data/instance member.


Nasty Age Printing Method I have this ugly age printing method. Can you do better?

def age(birth_date)
  today = Date.today
  years = today.year - birth_date.year
  months = today.month - birth_date.month
  (months -1) if today.day < birth_date.day
  if years == 0 && months == 1
    age =  "#{months} month"
  elsif years == 0 && months > 1
    age =  "#{months} months"
  elsif years > 0 && months > 0
    age = years == 1 ? "1 year, #{months} months" : "#{years} years, #{months} months"
  elsif years > 0 && months < 0
    months = months + 12
    years = years - 1
    age = "#{months} months" if years == 0
    age = "1 year, #{months} months" if years == 1
    age = "#{years} years, #{months} months" if years == 1
  end
  age
end

Some notes:

(months -1) if today.day < birth_date.day. Note that this is doing nothing, months - 1 is evaluated and dropped.
There is a age = in every branch of the conditional, and finally age is returned. That's unnecessary and not idiomatic, in Ruby conditionals are expressions, so we don't need to create a variable.
About this:

age = "#{months} months" if years == 0
age = "1 year, #{months} months" if years == 1

It's preferable not to use in-line conditionals when you are dealing with assignments or expressions (it's ok if you are performing side-effects, i.e. fail("error") if x < 0). In general follow functional programming guidelines, specially when dealing with logic (if you do some programming that does not deal with logic, please let me know ;-)). It should look (you could also use a case) something like this:

age = if years == 0
  "#{months} months"
elsif years == 1
  #{months} months" if years == 1
...
end

I'd write:

def age(birth_date)
  today = Time.zone.today
  total_months = (today.year*12 + today.month) - (birth_date.year*12 + birth_date.month)
  years, months = total_months.divmod(12)
  strings = [[years, "year"], [months, "month"]].map do |value, unit|
    value > 0 ? [value, unit.pluralize(value)].join(" ") : nil
  end
  strings.compact.join(", ")
end

Note that, to keep it simple, this code (as yours) ignores the day of the month, which gives a slightly wrong answer.


"Sherlock and the Beast" challenge in Java I have been trying to solve the following problem on hackerrank.com, but when I submit the code it shows unsuccessful submission due to timeout. Can anyone help me out?

Sherlock and the Beast

import java.io.*;
import java.util.*;
import java.text.*;
import java.math.*;
import java.util.regex.*;

public class test {

public static void main(String[] args) {
    Scanner in = new Scanner(System.in);

    int t = in.nextInt();
    int a[] = new int[t];

    // reading test cases

    for( int i=0; i<t; i++){
        a[i] = in.nextInt();
    }

    int n = 0;  // length of decent string

    boolean break1 ; // for breaking 2 for loops after getting the output for each test case

    for( int i=0; i<t; i++){
        n = a[i];
        int testx = n/3;
        int testy = n/5;
        break1 = false;

        for( int x = testx; x >= 0; x--){

            for( int y = testy; y >= 0; y--){

                if( ( x==0 && y== 0 ) ){

                    System.out.println("-1");

                    break1 = true;
                    break;
                }

                else if ( n % (3*x+5*y) == 0){

                    System.out.println( getdecentString(x, y) );

                    break1 = true;
                    break;

                }

            }

            if ( break1 == true)
                break;

        }
    }

}

// function to generate decentString

static String getdecentString(int x, int y){
    String decentString = "";
    x = 3*x;
    y = 5*y;

    for( int j=1; j <= x; j++)
        decentString =  decentString + "5";

    for( int k=1; k <= y; k++)
        decentString =  decentString + "3";

    return decentString;

}

}

As Robert pointed out, your code spends a lot of time in getdecentString. A faster way to generate Strings would be to use the StringBuilder class, e.g.:

StringBuilder decentString = new StringBuilder();
...
for ( int j = 1; j <= x; j++ )
{
    decentString.append( "5" );
}
...
return decentString.toString();


Checking if characters in a string can be rearranged to make a palindrome Can I please have some advice on optimizing, cleaning up my code, and places where I could save space/time?

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

bool pal_perm(char*);
int main()
{
    printf("The output is %sa palindrome.\n", pal_perm("abbas")? "": "not "); //Output: The output is a palindrome.
    printf("The output is %sa palindrome.\n", pal_perm("deeds")? "": "not "); //Output: The output is a palindrome.
    printf("The output is %sa palindrome.\n", pal_perm("dead")? "": "not "); //Output: The output is not a palindrome.
    return 0;
}

bool pal_perm(char* str)
{
    char alpha[256];
    int oddCount =0;
    int size = strlen(str);
    memset(alpha, 0, sizeof(alpha));

    //see how many occurances of each letter
    for(char ch = 'a'; ch <= 'z'; ch++)
    {
        for(int i=0; i < size; i++)
        {
            if(str[i] == ch)
                alpha[str[i]]++;
        }
    }

    //count the number of times a letter only appears once
    for(int j=0; j<256; j++)
    {
        if(alpha[j] == 1 || (alpha[j]%2==1))
            oddCount++;

    }
    //if there is more than one letter that only occurs, then it
    //cannot be a palindrome.
    if(oddCount <= 1)
        return true;
    else
        return false;
}

Strange output

What is a user to think when seeing such output of a program?

The output is a palindrome.
The output is not a palindrome.

I wouldn't know what this program is trying to tell me.

Consider this alternative:

void print_result(char * s)
{
    printf("The characters of \"%s\" %s be rearranged into a palindrome.\n", s, pal_perm(s) ? "can" : "cannot");
}

int main()
{
    print_result("abbas");
    print_result("deeds");
    print_result("dead");
}

Output:

The characters of "abbas" can be rearranged into a palindrome.
The characters of "deeds" can be rearranged into a palindrome.
The characters of "dead" cannot be rearranged into a palindrome.

Though actually I would prefer something much simpler than that:

printf("\"%s\" -> %s\n", s, pal_perm(s) ? "true" : "false");

Producing output:

"abbas" -> true
"deeds" -> true
"dead" -> false

Usability

It would be more interesting if the program took the strings from the command line, instead of using hardcoded values, for example:

int main(int argc, char ** argv) {
    for (int i = 1; i < argc; i++) {
        print_result(argv[i]);
    }
}

For the record, @Law29 suggested another alternative in a comment:

  You can also read from standard input. This lets you either type in words as they come to mind, or use a whole file (there are files of dictionary words, for example). Example:

#define MAX_WORD_SIZE 50
int main(int argc, char ** argv) {
    char buf[MAX_WORD_SIZE]
    while (fgets (buf, MAX_WORD_SIZE, stdin)) {
        print_result(buf);
    }
}

Testing

Getting the implementation right can be tricky.
You revised your post 3-4 times to fix bugs pointed out in comments.
It's good to automate your tests so that they can be repeated easily,
for example by adding methods like these:

void check(char * s, bool expected)
{
    if (pal_perm(s) != expected) {
        printf("expected \"%s\" -> %s but got %s\n", s, expected ? "true" : "false", expected ? "false" : "true");
        exit(1);
    }
}

void run_tests()
{
    check("a", true);
    check("aa", true);
    check("aba", true);
    check("abba", true);
    check("aabb", true);
    check("aabbs", true);
    check("deeds", true);

    check("ab", false);
    check("abc", false);
    check("dead", false);
}

Use boolean expressions directly

Instead of this:

if(oddCount <= 1)
    return true;
else
    return false;

You can simply return the boolean expression itself:

return oddCount <= 1;

Excessive looping

As @DarthGizka explained, instead of this:

for(char ch = 'a'; ch <= 'z'; ch++)
{
    for(int i=0; i < size; i++)
    {
        if(str[i] == ch)
            alpha[str[i]]++;
    }
}

This is identical, but without unnecessary looping:

for(int i=0; i < size; i++)
{
    alpha[str[i]]++;
}

Unnecessary conditions

The first condition is unnecessary:

    if(alpha[j] == 1 || (alpha[j]%2==1))

This is exactly the same:

    if(alpha[j]%2==1)

Too compact writing style

Instead of this:

    if(alpha[j]%2==1)

I suggest to put spaces around operators, and before ( in if statements:

    if (alpha[j] % 2 == 1)

Stop iterating when you already know the result

Once you find two characters with odd number of occurrences,
you can stop iterating and return false.
As such, you don't even need an int oddCount, but a bool seenOdd.
So instead of this:

int oddCount = 0;

//count the number of times a letter only appears once
for (int j = 0; j < 256; j++)
{
    if (alpha[j] % 2 == 1) oddCount++;
}
//if there is more than one letter that only occurs, then it
//cannot be a palindrome.
return oddCount <= 1;

You could write:

bool seenOdd = false;

// scan for odd number of occurrences, stop after seeing two
for (int j = 0; j < 256; j++)
{
    if (alpha[j] % 2 == 1) {
        if (seenOdd) return false;
        seenOdd = true;
    }
}

// less then 2 letters with odd number of occurrences, must be true
return true;


Convert a password to a phonetic string for end users As much as I hate it, sometimes providing passwords to people has to be done electronically. When I do that I try to remove any ambiguity that might be created by the font, user etc. and provide a phonetic translation of the string. For things like 0 and O or l and I which might not be obvious at first.

The function below can take a single string or be piped a string array. I could get crazy with this later to support other languages by making some nested arrays but for now it only supports English.

function Get-PhoneticString{
    param(
        [parameter(ValueFromPipeline=$true, Mandatory=$true)]
        [string]$StringToExplode,
        [string]$Delimiter = " - "
    )
    process{
        # Alphabetic conversions using military phonetics
        $phoneticAlphabet = @{
            "a" = "alfa"; "b" = "bravo"; "c" = "charlie"; "d" = "delta"; "e" = "echo";
            "f" = "foxtrot"; "g" = "golf"; "h" = "hotel"; "i" = "india"; "j" = "juliett";
            "k" = "kilo"; "l" = "lima"; "m" = "mike"; "n" = "november"; "o" = "oscar";
            "p" = "papa"; "q" = "quebec"; "r" = "romeo"; "s" = "sierra"; "t" = "tango";
            "u" = "uniform"; "v" = "victor"; "w" = "whiskey"; "x" = "x-ray"; "y" = "yankee";
            "z" = "zulu"
        }

        $numberAsString = @{
            "0"="zero"; "1"="one"; "2"="two"; "3"="three"; "4"="four"
            "5"="five"; "6"="six"; "7"="seven"; "8"="eight"; "9"="nine"
        }

        $symbolAsString = @{
            "!" = "Exclamation Mark"; '"' = "Double Quote"; "#" = "Number Sign"; "$" = "Dollar"; "%" = "Percent"; "&" = "Ampersand"
            "'" = "Single Quote"; "(" = "Left Parenthesis"; ")" = "Right Parenthesis"; "*" = "Asterisk"; "+" = "Plus"
            "," = "Comma"; "-" = "Minus"; "." = "Period"; "/" = "Slash"; ":" = "Colon"; ";" = "Semicolon"; "<" = "Less Than"
            "=" = "Equality Sign"; ">" = "Greater Than"; "?" = "Question Mark"; "@" = "At Sign"; "[" = "Left Square Bracket"
            "\" = "Backslash"; "]" = "Right Square Bracket"; "^" = "Caret"; "_" = "Underscore"; '`' = "Grave / Accent"
            "{" = "Left Curly Bracket"; "|" = "Vertical Bar"; "}" = "Right Curly Bracket"
        }

        # Convert this string into a character array. For each character find its string equivilent in one of the defined hash tables
        ($StringToExplode.ToCharArray() |  ForEach-Object{
            switch -Regex -CaseSensitive ($_){
                "[a-z]"{$phoneticAlphabet."$_"}
                "[A-Z]"{($phoneticAlphabet."$_").ToUpper()}
                "\d"{$numberAsString."$_"}
                "\s"{$_}# Return whitespace unaltered
                default{ $symbolAsString."$_"}
            }
        }) -join $Delimiter
    }

}

So a sample call and output would look like this

"K0D3R3v13w" | Get-PhoneticString
KILO - zero - DELTA - three - ROMEO - three - victor - one - three - whiskey

It only supports the basic ASCII printable character set. Any uppercase alphabetical characters will have all uppercase strings returned. If the character is not alphanumeric it will be assumed that it is a symbol in the set. If that character is not covered at all then it will show as empty in the resultant string and means something needs to be updated to account for it. Since I am using this for passwords that should cover near all supported characters.

Your code looks pretty good to me. I applaud your using datastructures to hold the mappings. A lot of developers would have used a humongous switch statement there, but I think as a general rule, datastructures are to be preferred over code as they makes things clearer and more flexible.

It's a good programming principle to break things down into smaller functions. That makes the code easier to read and maintain. It makes your code self-documenting, which means you don't need to rely on comments so much.

We can put the inner logic of your blob of code into its own function:

function wordFromChar($char)
{
    switch -Regex -CaseSensitive ($char){
        "[a-z]"{$phoneticAlphabet."$char"}
        "[A-Z]"{($phoneticAlphabet."$char").ToUpper()}
        "\d"{$numberAsString."$char"}
        "\s"{$char}# Return whitespace unaltered
        default{ $symbolAsString."$char"}
    }
}

And then call it like this:

$words = $StringToExplode.ToCharArray() |  ForEach-Object {
    wordFromChar $_
}

$words -join $Delimiter

We can improve the code in the wordFromChar function. The regular expressions such as [a-z] are repeating the information that is already contained in the hash table. That violates the DRY Principle. ("Don't repeat yourself.") This is an important principle which you can look up.

I'd be inclined to put all the hash tables into a single hash table. I don't see why there should be separate tables. Having separate tables just means we have more stuff to deal with but without any real benefits. Assuming that we make a single hash table called $charMap, we could rewrite the function like this:

function wordFromChar($char)
{
    if ($charMap.Contains($char))
    {
        $result = $charMap[$char]
    }
    else
    {
        throw "Can't handle '$char'"
    }

    if (isUppercase $char)
    {
        $result = $result.ToUpper()
    }

    $result
}

(If there was a good reason to keep the separate hash tables, we could certainly do that. We could just put them all in an array. That is, we would have an array of hash tables. That would make the lookup logic a little more complicated, but only by a little.)

isUppercase is a new function that could look like this:

function isUppercaseChar($char)
{
    $char -cmatch "^[A-Z]$"
}

-cmatch is a case-sensitive regular expression match.

I haven't tested any of the code above, by the way.


Class to interact with cookies Recently I needed to manipulate with cookies using JavaScript, so I wrote a class:

function Cookies() {
    var cookieLifeTime = null;

    this.setCookie = setCookie;
    this.getCookie = getCookie;
    this.deleteCookie = deleteCookie;
    this.setLifeTime = setLifeTime;

    function setLifeTime(lifeTime) {
        cookieLifeTime = lifeTime;
    }

    function setCookie(name, value) {
        if (this.lifeTime !== null) {
            var date = new Date();
            date.setTime((date.getTime() + cookieLifeTime));
            var expires = '; expires=' + date.toGMTString();
        } else {
            var expires = '';
        }

        document.cookie = name + '=' + value + expires + '; path=/';
    }

    function getCookie(name) {
        name += '=';
        var ca = document.cookie.split(';');

        for(var i=0; i < ca.length; i++) {
            var c = ca[i];

            while (c.charAt(0) === ' ')
                c = c.substring(1, c.length);

            if (c.indexOf(name) === 0)
                return c.substring(name.length, c.length); //Edited. Reduced not extra iterations
        }

        return null;
    }

    function deleteCookie(name) {
        setCookie(name, '', -1);
    }
}

I am not a JavaScript developer so maybe I've over-complicated things. Are there parts that can be optimized (I mean readability mostly)? I am also interested if there could be some edge-cases where this script will fail.

As Dvorak said in the comments, your code is fine as-is. Still, I have some suggestions:

I'd forego the *Cookie suffix on the function names. Seems too verbose, considering the constructor is called Cookies already. Only trick is that you can't have function called delete as that's a reserved word, but remove works fine too.
I think you have a bug: In deleteCookie you call your own setCookie with a negative lifetime - but setCookie doesn't take a lifetime argument, so the cookie won't be deleted as expected.
I'd fix this by adding such a 3rd argument to setCookie but making it optional:

function setCookie(name, value, lifetime) {
  var date, expires = '';
  // default to cookieLifeTime if no arg was passed
  if( typeof lifetime !== 'undefined' ) {
    lifetime = cookieLifeTime);
  }
  if( lifetime ) {
    date = new Date();
    date.setTime(date.getTime() + lifetime);
    expires = '; expires=' + date.toGMTString()
  }
  document.cookie = name + '=' + value + expires + '; path=/';
}

The getCookie can be made considerably shorter with a bit or regular expression magic.

function getCookie(name) {
  var pattern = new RegExp(name + '=([^;]+)', ''),
      match = document.cookie.match(pattern);
  if(match) {
    return match[1];
  }
  return null;
}

As RoToRa says in another answer, a simple object literal might be neater, if you're willing to either skip the cookieLifeTime instance variable, or make it completely global. Here's how I'd probably write it:

// lowercase, as it's no longer a constructor
window.cookies = {
  set: function (name, value, lifetime) {
    var date, expires = '';
    // for mysterious reasons `NaN` is a number in JS,
    // so check for NaN too
    if( typeof lifetime === 'number' && !isNaN(lifetime) ) {
      date = new Date();
      date.setTime(date.getTime() + lifetime);
      expires = '; expires=' + date.toGMTString()
    }
    document.cookies = name + '=' + value + expires + '; path=/';
  },

  get: function (name) {
    var pattern = new RegExp(name + '=([^;]+)', ''),
        match = document.cookie.match(pattern);
    if(match) {
      return match[1];
    }
    return null;
  },

  remove: function(name) {
    window.cookies.setCookie(name, '', -1);
  }
};


Creating bootstrap modals that can run JavaScript in MVC5 I have an MVC5 & C# application using the Razor engine.

In my application I have a view, called "Index.cshtml". In this view I want to present a bootstrap modal to the user.

When presented to the user, this modal will have buttons that should be capable of running JS functions and methods using jQuery.

I have achieved this in the past using a dynamic method.

This means that I have a Main view (Index.cshtml), that contains an empty element (

):

Package List

This empty tag is then filled via an Ajax request to the server:

//Show Edit Package modal
    $(".btn.btn-default.editPackage").click(function () {

        $.ajax({
            url: $(this).data('url'),
            type: 'GET',
            cache: false,
            success: function (result) {
                $('#editModal').html(result).find('.modal').modal({
                    show: true
                });
            }
        });
        return false; //prevent browser defualt behavior
    });

This Ajax request returns the Modal that I want to show:

Edit Package MyPackage

          <form class="form-horizontal">

              Package Name:

                <input type="text" class="form-control" id="package-name" placeholder="MyPackageName">

          </form>

          <button type="button" class="btn btn-default" data-dismiss="modal">Close</button>
          <button type="button" class="btn btn-primary" id="savePackageChangesBtn">Save changes</button>

And then the user can have fun clicking in the modal's buttons and seeing how things change. This is achieved via jQuery linked to the modals save button:

$(document).ready(function() {
  $("#savePackageChangesBtn").click(function() {
    $('.modal').modal({
      show: true
    });
    return false; //prevent browser defualt behavior
  });
});

However, after discussion with the community (see this answer), we concluded that was a poor design pattern.
Therefore, I must ask, what is the easiest way of achieving this objective?

Simply put, you are using AJAX to return markup when you should be using AJAX to fetch data. If you are doing a per-item fetch using AJAX, you should be fetching more package detail information and returning it in an object, not as a munged HTML string. If you already have all the data you need when the page is loaded, then you should not be making additional AJAX calls, you should be doing all the work in Javascript.

I think Vsevolod Goloviznin made a good suggestion, to render the modal on the page once. That's good because it does not make the server have to generate a bunch of crap, but if you use straight-up jQuery you end up writing a bunch of messy procedural code to update the markup and event handlers (destroy and reassign) during the selection callback of your packages table.

Client-side templating is your friend. Here's an article that seems to be right up your alley:

http://www.smashingmagazine.com/2012/12/05/client-side-templating/


Rock-Paper-Scissors implementation I built a simple Rock Paper Scissors game in Python as my first "project", and I want a feedback from you. I want your opinion on how I write my code, is it okay and readable enough? PEP8 laws, things to improve or preserve?

Computer Choice - Generating a random choice with the random module and the random.randomchoice() method from a list.

User Choice - Taking an Input from the user as a character (R, P and S) and uppercasing the character.

Finally, I'm checking who is the winner with if conditions.

import random

choices = ['Rock', 'Paper', 'Scissors']
bot_choices = random.SystemRandom().choice(choices)

def result(user, bot):
    return f'\nComputer Choice: {bot}\nYour Choice: {user}'

class RPS:
    def __init__(self):
        self.user_choice = str(input('(R)ock, (P)aper, (S)cissors: ')).upper()
        if not self.user_choice or self.user_choice not in ['R', 'P', 'S']:
            print('Invalid Input!')
        else:
            self.get_winner()
            print(result(self.user_choice, bot_choices))

    def get_winner(self):
        if self.user_choice == 'R' and bot_choices == 'Paper' or self.user_choice == 'P' and bot_choices == 'Scissors' or self.user_choice == 'S' and bot_choices == 'Rock':
            print('Result: Computer Won!')
        elif self.user_choice == 'R' and bot_choices == 'Rock' or self.user_choice == 'P' and bot_choices == 'Paper' or self.user_choice == 'S' and bot_choices == 'Scissors':
            print('Result: Tie!')
        else:
            print('Result: You Won!')

if __name__ == '__main__':
    game = RPS()

Echoing all of VincentRG's points, with a little clarification and demonstration:

The only reason to wrap code in a if __name__ == '__main__' block is to keep it from being executed when your module is imported by another module.  By that token, the fact that you initialize bot_choices outside of that block means that those random choices will be determined only once at the time of import, so the importing module will get the same result every time it calls RPS().  This is probably not what you want.  It doesn't affect the way your program runs since you don't actually have any other modules, but if you're going to check for __main__ you should understand why you're doing that and apply the same logic to other parts of your code.
This does not need to be a class and therefore should not be.  Here's how to write it as a single simple function (this is mostly just taking your existing code and moving it around):

import random

def rock_paper_scissors() -> None:
    bot_choice = random.SystemRandom().choice(['Rock', 'Paper', 'Scissors'])

    user_choice = str(input('(R)ock, (P)aper, (S)cissors: ')).upper()
    if user_choice not in ['R', 'P', 'S']:
        print('Invalid Input!')
        return

    if (user_choice == 'R' and bot_choice == 'Paper'
            or user_choice == 'P' and bot_choice == 'Scissors'
            or user_choice == 'S' and bot_choice == 'Rock'):
        print('Result: Computer Won!')
    elif user_choice == bot_choice[0]:
        print('Result: Tie!')
    else:
        print('Result: You Won!')

    print(f'\nComputer Choice: {bot_choice}\nYour Choice: {user_choice}')

if __name__ == '__main__':
    rock_paper_scissors()

More notes:

Not necessary to check user_choice for truthiness if you're already checking it for membership in a collection of truthy values.
The name bot_choices is confusing if it's a single choice.  Changed it to bot_choice.
There's no reason to name the choices variable since you never use it again after picking bot_choice.
You can simplify the tie check way down by just checking the user choice against the first letter of the bot choice.
This would be simpler yet if you defined the choices as an Enum and had the player and bot use the same enum instead of their own hardcoded strings.

If you have no need to reuse this function or this module, this script doesn't even need to define a function.  You could have a file that just looks like:

import random

bot_choice = random.SystemRandom().choice(['Rock', 'Paper', 'Scissors'])

user_choice = str(input('(R)ock, (P)aper, (S)cissors: ')).upper()
if user_choice not in ['R', 'P', 'S']:
    print('Invalid Input!')
    exit()

if (user_choice == 'R' and bot_choice == 'Paper'
        or user_choice == 'P' and bot_choice == 'Scissors'
        or user_choice == 'S' and bot_choice == 'Rock'):
    print('Result: Computer Won!')
elif user_choice == bot_choice[0]:
    print('Result: Tie!')
else:
    print('Result: You Won!')

print(f'\nComputer Choice: {bot_choice}\nYour Choice: {user_choice}')

and it would behave exactly the same.

Here's how you might use an Enum to specify the choices:

import enum
import random

class Choice(enum.Enum):
    ROCK = "R"
    PAPER = "P"
    SCISSORS = "S"

what_beats = {
    Choice.SCISSORS: Choice.ROCK,
    Choice.ROCK: Choice.PAPER,
    Choice.PAPER: Choice.SCISSORS,
}

bot_choice = random.choice([c for c in Choice])

try:
    user_choice = Choice(input('(R)ock, (P)aper, (S)cissors: ').upper())
except ValueError:
    print('Invalid input!')
    exit()

if what_beats[user_choice] == bot_choice:
    print('Result: Computer Won!')
elif user_choice == bot_choice:
    print('Result: Tie!')
else:
    print('Result: You Won!')

print(f'\nComputer Choice: {bot_choice.name.title()}'
      f'\nYour Choice: {user_choice.name.title()}')


Interchange all occurrences of a character and its predecessor Given a string like "abcdbcfbc" , I want all the occurrences of 'b' to be interchanged with their previous character.
The output should be: "bacbdcbfc"

I've come up with a temporary solution:

function punjabi_alternate($string) {
    $len = mb_strlen($string);
    $sploded = array();
    while($len-- > 0) {
        $sploded[$len] = mb_substr($string, $len, 1);
    }
    ksort($sploded);
    $len = mb_strlen($string);
    for ($i=0; $i < $len; $i++) {
        if( strcmp($sploded[$i], "b") == 0){
            $temp = $sploded[$i];
            $sploded[$i] = $sploded[$i-1];
            $sploded[$i-1] = $temp;
        }
    }
    return join('', $sploded);
}

Here I'm using mb_substr as my string contains unicode characters. The b is just an example.
In my production version I'm using this instead of "b"

html_entity_decode("&#x0A3F;")

It is working as expected.
I'd appreciate if there could be a cleaner way to do it.
Also the strings passed to the function can contain as many as 10,000 - 20,000 characters.
So, I'd prefer a preg_replace function with regex to do the same.

You want to do a string replacement operation, so use preg_replace().  The key is to use backreferences in the replacement.  Also, since you seem to be interested in processing Unicode, be sure to use the u flag for the regex.

If, as you say, 'b' is just an example, then you can make it a default parameter.

function punjabi_alternate($string, $char='b') {
    return preg_replace("/(.)($char)/u", '\2\1', $string);
}


Haskell - saving content of HTTP request in selected file As a Haskell beginner I'm currently playing around the IO monad and prepared simple program which gets from the user two information:

URL (http)
Output file path

Then it performs GET request to the provided URL and stores the result in the selected file. HTTP status code of the response is displayed on the standard output
Could you please suggest how such code could be improved? Maybe some places when point-free style can be used? I also feel that the part with confirmation of file overwriting is not very elegant.
import System.IO
import Network.HTTP
import System.Directory

getUrl :: IO String
getUrl = putStr "URL: " >> hFlush stdout >> getLine

confirmOverideOrGetNewPath :: String -> IO String
confirmOverideOrGetNewPath filepath = do
    putStr $ "File " ++ filepath ++ " exists. Override? [y/n] "
    hFlush stdout
    decision <- getLine
    let shouldOverride = (if decision == "y" then True else False)
    case shouldOverride of True -> return filepath
                           False -> putStrLn "\nProvide other path" >> getOutputFilepath

getOutputFilepath :: IO String
getOutputFilepath = do
    putStr "Output file path: "
    hFlush stdout
    filepath <- getLine
    file_exists <- doesFileExist filepath
    case file_exists of True -> confirmOverideOrGetNewPath filepath
                        False -> return filepath

performGetRequest :: String -> IO (String, Int)
performGetRequest url = do
        rsp <- Network.HTTP.simpleHTTP (getRequest url)
        content <- getResponseBody rsp
        responseCode <- getResponseCode rsp
        return (content, responseCodeToInt responseCode)
    where
        responseCodeToInt :: ResponseCode -> Int
        responseCodeToInt code = let (x,y,z) = code in read (mconcat $ show <$> [x,y,z]) :: Int

saveContentToFile :: FilePath -> String -> IO ()
saveContentToFile = writeFile

printHttpStatusCode :: Int -> IO ()
printHttpStatusCode statusCode = putStrLn ("\nStatus code: " ++ show statusCode)
                                 >> putStrLn "Content saved into file"

main :: IO ()
main = do
    url <- getUrl
    filepath <- getOutputFilepath
    (content, statusCode) <- performGetRequest url
    saveContentToFile filepath content
    printHttpStatusCode statusCode

Right from the top I notice you could benefit from some factoring.
prompt :: String -> IO String
prompt str = do
    putStr str
    hFlush stdout
    getLine

Then getURL is prompt "URL: " (and maybe doesn't even need to exist separately!) and confirmOverideOrGetNewPath and getOutputFilepath both lose two lines at the top. You'll notice that I also wrote this function with do-notation, it's a stylistic preference but I just don't like to run monads into one line like that, not sure if I have a great reason why. I guess it just feels right to make imperative-y monadic code look like an imperative language would.
The diversion through shouldOverride in confirmOverideOrGetNewPath is unnecessary, just put your expression in the case-statement.
case decision == "y" of
  True -> ...

But then I'd also skip the check for equality entirely and just pattern match on the returned string.
case decision of
    "y" -> return filepath
    "n" -> getOutputFilepath
    _   -> do
      putStrLn "Unrecognized input"
      confirmOverrideOrGetNewPath filepath

I changed the logic to make the behavior more elucidating. If a user actually enters "n", it's not really necessary to tell them to provide a new path since getOutputFilepath will prompt them for a new one immediately. The user presumably expects that, given their input. If they type something other than "y" or "n", instead of requiring them to input a new path, just check again if they're cool with the override.
In getOutputFilepath all I'd note is that file_exists should be camel cased for consistency.
Over the rest of your program, the only thing I'd switch up would be the whole logic around response codes. In performGetRequest I'd just change the return type to be ResponseCode instead of Int, and push your display logic into printHttpStatusCode (which should probably be printResponseCode). Since ResponseCode is just a type alias for tuples, you can also pattern match in the function call (i.e. in your original version responseCodeToInt (x, y, z) = ... instead of the let). And then it also probably makes more sense to have a pure showResponseCode and leave printing up to the caller.
showResponseCode :: ResponseCode -> String
showResponseCode (i, j, k) = concatMap show [i, j, k]

Then I'd just carry the changes through to main and reorder things for clarity and truth in logic.
main = do
    ...
    (content, statusCode) <- performGetRequest url
    putStrLn $ "Response status: " ++ showResponseCode statusCode
    writeFile filepath content
    putStrLn "Content saved"


K&R atoi() variation I'm currently learning C with The C Programming Language by K&R and this is my current exercise:

  Rewrite appropriate programs from earlier chapters and exercises with pointers instead of array indexing. Good possibilities include getline, atoi, itoa, reverse, strindex and getop.

This is my atoi solution with pointers:

#include <ctype.h>

int atoi(const char *nptr)
{
    while(isspace(*nptr++)) // skip white space
        ;

    long long val = 0;
    const char *start = nptr;

    if(*nptr == '-' || *nptr == '+')
        nptr++;

    while(*nptr >= '0' && *nptr <= '9')
        val = val*10 + *nptr++ - '0';

    return (*start == '-') ? -val : val;
}

I'm currently a little bit unhappy with the idea, that my code can't do anything against values smaller than INT_MIN and values greater than INT_MAX, but I can't come up with a solution that fits my expectations of good code.

I could just return INT_MIN and INT_MAX, but then the caller can't decide between the actual values and an error in form of INT_MIN/INT_MAX.
It is also possible to use a second argument - a pointer to an int. The function will write the value into this int and will return 1 if it was successful and 0 if it wasn't.

I want to hear your thoughts on how I should handle this special case.

You have a bug right on the first statement:

while(isspace(*nptr++)) // skip white space
    ;

I didn't vote to close your post as off-topic for broken code to make a point:
it might seem cool to do two things at once in a loop condition (condition + increment),
it's not recommended, for two reasons:

It's best to have one statement per line

A for (;;) is an exception, where it's allowed

The loop condition should be about the condition. Leave it at that, move the incrementing logic inside the loop body in a while loop.

The bug is an off-by-one error: when the first digit is found,
the condition is false, but the pointer will be still incremented,
moving past the first digit. As a result, a string like "123" gets parsed as 23.

The fix is simple, and follows the above recommendations about writing loops:

// skip white space
while (isspace(*nptr)) {
    nptr++;
}

  I'm currently a little bit unhappy with the idea, that my code can't do anything against values smaller than INT_MIN and values greater than INT_MAX, but I can't come up with a solution that fits my expectations of good code.

Hm, the method is declared to return an int...
So it seems you don't like that int is limited within INT_MIN and INT_MAX by the language. There's just one thing to do: change the return type to something bigger, like a long!

Finally, when comparing against a variable to multiple files in the same conditional expression, often it's more readable to put the conditions in numerical increasing order, for example instead of this:

while(*nptr >= '0' && *nptr <= '9')

Write like this:

while('0' <= *nptr && *nptr <= '9')


for_each_cons and for_each_slice, variants of std::for_each working on sliding windows My goal was to replicate Ruby's each_cons and each_slice:

(0..5).each_cons(3) {|a, b, c| print a, b, c, "\n"}
012
123
234
345

I made two functions, for_each_cons, and for_each_slice. They work similarly to their Ruby counterparts, except the slice is always passed to separate arguments. The slice size is deducted from the passed function's number of arguments.

const std::vector<int> vec = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9};
for_each_cons(vec, [](int a, int b, int c){
    printf("%d %d %d\n", a, b, c);
});

for_each_slice(vec, [](int a, int b, int c){
    printf("%d %d %d\n", a, b, c);
});

Outputs:

0 1 2
1 2 3
2 3 4
3 4 5

0 1 2
3 4 5

And here's the implementation. For simple code it seems to generate similar assembly to manual implementations, although it's largely because it requires random access iterators.

#include <utility>

namespace detail
{
    template <typename Fun>
    struct function_params
        : public function_params<decltype(&Fun::operator())>
    {};

    template <typename Class, typename Ret, typename... Args> // for lambdas
    struct function_params<Ret(Class::*)(Args...) const> {
        static constexpr int n_args = sizeof...(Args);
    };

    template <typename Class, typename Ret, typename... Args> // for mutable lambdas
    struct function_params<Ret(Class::*)(Args...)> {
        static constexpr int n_args = sizeof...(Args);
    };

    template <typename Ret, typename... Args> // for function pointers
    struct function_params<Ret(*)(Args...)> {
        static constexpr int n_args = sizeof...(Args);
    };

    template<typename It, typename Fun, int N = function_params<Fun>::n_args, std::size_t... I>
    void for_each_cons_impl(It first, It last, Fun f, std::index_sequence<I...>) {
        for(auto it = first; it + N - 1 < last; ++it)
            f(*(it + I)...);
    }

    template<typename It, typename Fun, int N = function_params<Fun>::n_args, typename Indices = std::make_index_sequence<N>>
    void for_each_cons(It first, It last, Fun f) {
        for_each_cons_impl(first, last, f, Indices());
    }

    template<typename It, typename Fun, int N = function_params<Fun>::n_args, std::size_t... I>
    void for_each_slice_impl(It first, It last, Fun f, std::index_sequence<I...>) {
        for(auto it = first; it + N - 1 < last; it += N)
            f(*(it + I)...);
    }

    template<typename It, typename Fun, int N = function_params<Fun>::n_args, typename Indices = std::make_index_sequence<N>>
    void for_each_slice(It first, It last, Fun f) {
        for_each_slice_impl(first, last, f, Indices());
    }
}

template<typename It, typename Fun>
void for_each_cons(It first, It last, Fun f) {
    detail::for_each_cons(first, last, f);
}

template<typename Rng, typename Fun>
void for_each_cons(Rng &&rng, Fun f) {
    for_each_cons(rng.begin(), rng.end(), f);
}

template<typename It, typename Fun>
void for_each_slice(It first, It last, Fun f) {
    detail::for_each_slice(first, last, f);
}

template<typename Rng, typename Fun>
void for_each_slice(Rng &&rng, Fun f) {
    detail::for_each_slice(rng.begin(), rng.end(), f);
}

Any hints and criticism are welcome.

The two main issues I see are that

You require RandomAccessIterators although these should be implementable with ForwardIterators as well since you never have to skip an element.
This will fail for functors with an overloaded or templated operator() such as those with auto parameters

Also, what happens if the range's length isn't divisible by the step size in the second case?

As 2 isn't really possible to solve since in the overloaded case there may be multiple valid values for n_args you could allow the user to pass the number of arguments as a first parameter. You could then default that value to 0 (which is never valid) and infer the number of parameters as you currently do if it is set to 0.


Complexity, Usability and Readability of FluentAPIs - Sample: FluentMethodInvoker new MethodExecuter()
    .ForAllInvocationsCheckCondition(!Context.WannaShutDown)
        .When(shouldRun).ThenInvoke(x => x.Methods(new Action[] { B.Process, A.Process }))
        .When(shouldRun).ThenInvoke(x => x.Method(B.Process).Repeat(100))
        .Invoke(x => x.Method(C.Process));

I deliberately do not say anything about what this API does. I did show this API my team mates and they did not like it. For them it is unreadable.

What do you think about the API?
What tips do you have?
What do you think about complexitiy, usability and readability of fluent APIs and how to achieve good APIs?

EDIT
Why did I build this API?

Well, I have a bunch of methods that are invoked synchronously when a condition is false. The condition applies foreach method invocation.

if (!Context.WannaShutDown) A.Process();
if (!Context.WannaShutDown) B.Process();
if (!Context.WannaShutDown) C.Process();
if (!Context.WannaShutDown) D.Process();
if (!Context.WannaShutDown) E.Process();
if (!Context.WannaShutDown) F.Process();
if (!Context.WannaShutDown)
{
   if (condition)
   {
       for (int i = 0; i <= 100 i++)
       {
           Z.Process();
       }
   }
...

I do not like such crap code. What I wanted to do is to solve "the problem" once with a class that is responsible for invocating methods. I tried to build an API that is as simple as possible, but it seems I failed.

Kirk Woll makes some excellent points in his comment which could have been an answer IMHO.

  Fluent APIs are not better ipso facto.

You are entirely right in wanting to refactor your given code sample in the edit, but it might be worthwhile to first explore some other possibilities before deciding on using a fluent API.

A first concern is the Context class, as it looks like this is a global which is accessed from any location (A, B, C, ...). If there is any way to remove this global state, that might already be a more important improvement.

Using only your given code sample (assuming a IProcess interface), you could already improve it by writing something like this:

Dictionary<IProcess, int> executeOperations = new Dictionary<IProcess, int>
{
    { A, 1 }, { B, 1 }, { C, 1 }, { D, 1 }, { E, 1 }, { F, 1 },
    { Z, 100 }
}

foreach ( var executeItem in executeOperations )
{
    if ( !Context.WannaShutDown )
    {
        for ( int i = 0; i <= executeItem.Value; ++i )
        {
            executeItem.Key.Process();
        }
    }
}

It's not clear what the condition in your code sample is meant to be, or why only Z needs to check for it, but for demonstration purposes I believe the above example shows there are other ways of refactoring your given code without having to rely on fluent syntax. You could replace the int value in the dictionary to a struct containing more execution parameters.

That said, I believe your supplied sample is still too small to conclusively say whether or not using fluent syntax could be justified.


Powershell duplicate image scanner So I have made this Duplicate image scanner using powershell. It will operate using the SHA256 hashes of the files.
Any tips or suggestions on making this more dynamic/robust/efficient or adding new features would be greatly appreciatied!
$sig=@'
public static void ShowConsoleWindow(int state)
{
  var handle = GetConsoleWindow();
  ShowWindow(handle,state);
}
[System.Runtime.InteropServices.DllImport("kernel32.dll")]
static extern IntPtr GetConsoleWindow();
[System.Runtime.InteropServices.DllImport("user32.dll")]
static extern bool ShowWindow(IntPtr hWnd, int nCmdShow);
'@
$hc=Add-Type -mem $sig -name Hide -Names HideConsole -Ref System.Runtime.InteropServices -Pas
$hc::ShowConsoleWindow(0)
[console]::title="Duplicate Image Scanner (c) Wasif Hasan | Sep 2020"
$eXt=@('.jpg','.png','.gif','.jpeg','.webp','.tiff','.psd','.raw','.bmp','.heif','indd','.svg')
@('system.windows.forms','system.drawing')|%{add-type -as $_}
$s=[windows.forms.form]::new();$s.size=[drawing.size]::new(400,850);$s.StartPosition="CenterScreen";$s.Text="Select drives to scan"
$drives=gdr -p "FileSystem"|select -eXp name
$top=20;$left=50;$drives|%{
$c=$_.split(" ")-join"_";$top += 20
iex "`$$($c) = New-Object System.Windows.Forms.CheckBox;`$$($c).Top = $($top);`$$($c).Left = $($left);`$$($c).Anchor='Left,Top';`$$($c).Parent='';`$$($c).Text='$($_)';`$$($c).Autosize=`$true;if('$_' -in `$drives){`$$c.Checked=`$true};`$s.Controls.Add(`$$c)"}
$ok=New-Object System.Windows.Forms.Button;$ok.Text='OK';$ok.Top=770;$ok.Left=290
$ok.add_click({$s.Close()});$s.Controls.AddRange($ok)
$sa=New-Object System.Windows.Forms.Button;$sa.Text='Select All';$sa.Top=770;$sa.Left=200
$sa.add_click({$s.Controls|?{($_.Checked) -or !($_.Checked)}|%{try{$_.Checked=$True}catch{}}});$s.Controls.AddRange($sa)
$null=$s.ShowDialog()
$choices=$s.Controls|?{$_.Checked}|select -eXp Text
$i=0;$choices|%{$choices[$i]=$_+':\';$i++}
$f=[windows.forms.form]::new();$f.Size=[drawing.size]::new(600,100);$f.StartPosition="CenterScreen";$f.Text="Please wait"
$l=[windows.forms.label]::new();$l.Text="Please wait until the scan is complete........";$l.Font="Segoe UI,16";$l.AutoSize=$true;$f.Controls.AddRange($l)
$null=$f.ShowDialog()
$files=@();$hCols=@();$choices|%{
dir $_ -r|?{$_.eXtension-in$eXt}|%{
$h=get-filehash $_.fullname -a 'SHA256'|select -eXp hash
if($h-in$hCols){$files+=$_.fullName}else{$hCols+=$h}
}};$f.Close()
$del=$files|ogv -t "Duplicate images (Hold CTRL and select the ones to delete)" -p
$del|%{rm "$_" -fo}
[windows.forms.messagebox]::Show("Thanks for using!","Duplicate image scanner","OK","Information")

Let me give you some advice:

First of all you should make your code much more rideable. I am struggling to actually read this code. You should name your variables meaningfully, so the code can be read like English.

Don't write multiple commands on a single line. It is easier to keep track of code when single command per line is used.

When writing PowerShell scripts don't use cmdlet aliases, use full names of cmdlets. Different people use different aliases, but all of them know full names of similes.


Find all integers between m and n whose sum of squared divisors is itself a square Divisors of 42 are : 1, 2, 3, 6, 7, 14, 21, 42. These divisors squared
  are: 1, 4, 9, 36, 49, 196, 441, 1764. The sum of the squared divisors
  is 2500 which is 50 * 50, a square!

  Given two integers \$m\$, \$n\$ (\$1 \le m \le n\$) we want to find all integers between m and n whose sum of squared divisors is itself a square. 42 is such a number.

  The result will be an array of arrays, each subarray having two
  elements, first the number whose squared divisors is a square and then
  the sum of the squared divisors.

I started learning Ruby two weeks ago so I don't know much. This is my first programming language.

Is it possible to rewrite the below code so it runs faster? It currently times out at n>9999.

def list_squared(m, n)

r = (m..n).to_a.map { |z| (1..z).select { |x| z % x == 0} }
#returns the divisors of each number in an array of arrays

squarenumbers = r.map { |x| x.map { |c| c**2 }.inject(:+) }.select { |x| Math.sqrt(x) % 1 == 0 }
#this finds all integers between m and n whose sum of squared divisors  is itself a square

booleans = r.map { |x| x.map { |c| c**2 }.inject(:+) }.map { |x| Math.sqrt(x) % 1 == 0 }
#returns an array of booleans.

indexer = booleans.map.with_index{|x, i| i if x == true }.compact
#returns the index of each of the true values in booleans as an array

unsqr = indexer.map { |x| (m..n).to_a[x] }
#returns the numbers whose squared divisors is a square in an array

unsqr.zip(squarenumbers)
#merges the two arrays together, element for element and creates an array of arrays

end

#for m = 1 and n = 1000 the result would be

#[[1, 1], [42, 2500], [246, 84100], [287, 84100], [728, 722500]]

If you were able to write this after just two weeks, knowing nothing previously about programming, then I would have to say: well done!  This is excellent code for a beginner.  I really like the way you have applied transformations and filters to entire arrays, instead of falling into the trap of proceeding instruction by instruction.

Performance

Where is the performance problem likely to lie?  It's almost certainly with the factoring (calculating r).  For every number z, you test every candidate divisor from 1 to z.  If you have n numbers to factor, then we would say that this step is O(n2).  That indicates a scalability problem.

Calculating squarenumbers is relatively straightforward.  Summing the squares of a list of numbers is easy.  Checking whether their square roots are integer is slightly more expensive, but still a simple operation.

Calculating booleans is also straightforward.  But it's repeating exactly the same calculations as for calculating squarenumbers, which is wasteful.

The last three statements to gather the result are also quite simple.

So, the problem is with factoring.  How can that be improved?  Observe that factors always occur in pairs: 1×42 = 2×21 = 3×14 = 6×7.  Once you discover the factors of \$z\$ up to \$\sqrt{z}\$ by trial-and-error, you automatically know the corresponding larger factors.

Readability

In the first statement, you used z for the dummy variable.  In subsequent statements, you switched to x.  I recommend using the same name for variables that serve the same purpose.

There are some nested map calls.  That makes the code trickier to read.  What can we do about that?  Notice that we test every z (subsequently called "x") independently.  Therefore, there is no need to calculate the divisors of each number in an array of arrays.  We can just consider each number in [m, m + 1, m + 2, …, n] on its own.

Suggested solution

require 'set'

def list_squared(m, n)
  (m..n).map do |num|
    divisors = Set.new((1..Math.sqrt(num)).select { |d| num % d == 0 })
    divisors += divisors.map { |d| num / d }

    sum_sq_divisors = divisors.map { |d| d * d }.inject(:+)
    [num, sum_sq_divisors] if Math.sqrt(sum_sq_divisors) % 1 == 0
  end.compact
end


Billing and recurrent payment db structure I am designing my first payment service and here is my db structure.

Each user can have many businesses, each business can have many products. Some are recurring, some are paid once. Each product is tied to payment method. Some products are always free, and some are paid every X days.

CREATE TABLE `payment_method` (
  `payment_method_id` int(11) NOT NULL auto_increment,
  `payment_method_name` VARCHAR(20) DEFAULT NULL,
  `payment_method_status` ENUM('active','disabled') CHARACTER SET utf8 COLLATE utf8_bin NOT NULL DEFAULT 'active',
  PRIMARY KEY  (`payment_method_id`)
) ENGINE=InnoDB AUTO_INCREMENT=1;

CREATE TABLE IF NOT EXISTS `product_type` (
  `product_type_code` varchar(50) NOT NULL,
  `product_type` varchar(150) NOT NULL,
  `product_type_name` varchar(150) NOT NULL,
  PRIMARY KEY (`product_type_code`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

CREATE TABLE IF NOT EXISTS `product` (
  `product_id` int(11) NOT NULL AUTO_INCREMENT,
  `product_price` decimal(12,2) DEFAULT NULL,
  `product_duration` int(11) DEFAULT NULL,
  `product_currency` varchar(50) NOT NULL,
  `recurring` enum('yes','no') CHARACTER SET utf8 COLLATE utf8_bin NOT NULL DEFAULT 'yes',
  `daily_limit` int(11) DEFAULT NULL,
  `monthly_limit` int(11) DEFAULT NULL,
  `lifetime_limit` int(11) DEFAULT NULL,
  `product_type_code` varchar(50) NOT NULL,
  `product_description` varchar(150) DEFAULT NULL,
  PRIMARY KEY (`product_id`),
  FOREIGN KEY (product_type_code) REFERENCES product_type(product_type_code)
) ENGINE=InnoDB AUTO_INCREMENT=1 DEFAULT CHARSET=utf8;

CREATE TABLE IF NOT EXISTS `product_detail` (
  `product_detail_id` int(11) NOT NULL AUTO_INCREMENT,
  `product_id` int(11) NOT NULL,
  `key` varchar(150) DEFAULT NULL,
  `value` varchar(150) DEFAULT NULL,
  PRIMARY KEY (`product_detail_id`),
  KEY `product_id` (`product_id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

CREATE TABLE IF NOT EXISTS `business_product` (
  `business_product_id` int(11) NOT NULL,
  `product_id` int(11) DEFAULT NULL,
  `business_id` int(11) DEFAULT NULL,
  `added_by_user_id` int(11) NOT NULL,
  `product_price` decimal(12,2) DEFAULT NULL,
  `product_currency` varchar(5) NOT NULL,
  `product_type` varchar(150) NOT NULL,
  `business_product_status` enum('paid','canceled') CHARACTER SET utf8 COLLATE utf8_bin NOT NULL,
  `start_date` date NOT NULL,
  `end_date` date DEFAULT NULL,
  `last_update_date` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,
  PRIMARY KEY (`business_product_id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

CREATE TABLE `user_authorized` (
  `user_authorized_id` int(11) NOT NULL auto_increment,
  `payment_method_id` int(11),
  `authorized`  varchar(150),
  `autopay` enum('yes','no') CHARACTER SET utf8 COLLATE utf8_bin NOT NULL DEFAULT 'yes',
  PRIMARY KEY  (`user_authorized_id`)
) ENGINE=InnoDB AUTO_INCREMENT=1;

CREATE TABLE `billing` (
  `billing_id` int(11) NOT NULL auto_increment,
  `external_id` VARCHAR(100) DEFAULT NULL,
  `invoice_date` datetime,
  `bill_date` datetime,
  `start_date` date,
  `end_date` date,
  `payment_method_id` int(11) default NULL,
  `user_id` int(11) default NULL,
  `authorized` VARCHAR(200) DEFAULT NULL,
  `fee` decimal(12,2) default NULL,
  `notes` varchar(500) default NULL,
  `billing_status` ENUM('new','paid','returned','canceled', 'pending', 'overdue')  CHARACTER SET utf8 COLLATE utf8_bin NOT NULL DEFAULT 'new',
  PRIMARY KEY  (`billing_id`)
) ENGINE=InnoDB AUTO_INCREMENT=1 ;

CREATE TABLE `billing_detail` (
  `billing_detail_id` int(11) NOT NULL auto_increment,
  `billing_id` int(11) NOT NULL,
  `product_id` int(11),
  `business_id` int(11),
  `product_price` decimal(12,2),
  PRIMARY KEY  (`billing_detail_id`)
) ENGINE=InnoDB AUTO_INCREMENT=1 ;

```

What do you think am I missing something?

Consistency

I like the naming convention of the auto-incrementing primary keys to be prefixed with the table name. This allows the use of using (keyname) when joining tables.
Some tables are created with IF NOT EXISTS, other aren't. Is there a strategy behind this?

Design

I am missing tables user and business.
Table business_product has a column product_type which overrides that of the referenced table product_type. Is this as designed?
Perhaps table user_authorized should be renamed to user, update the name of the primary key and referenced foreign keys along. (This has an impact on the constraints below)

Constraints

Table product_detail is missing a foreign key to product.
Table business_product is missing a foreign key to product, user and business.
Table billing is missing a foreign key to user and payment_method.
Table billing_detail is missing a foreign key to billing, product and business.
Table user_authorized is missing a foreign key to user and payment_method.


Class decorator in Python to set variables for the constructor I personally don't like the boilerplate of most __init__ methods:

self.a = a
self.b = b
...

So I thought this would be a nice opportunity to learn a bit more about decorators. As this is my first attempt on a class decorator I'm sure there is a lot to improve so fire away:

Implementation

from collections import namedtuple

def autofill(*args, **kwargs):
    """ This class decorator declares all attributes given into the constructor
    followed by a call of __init__ without arguments (beside reference to self).
    Order is the same than namedtuple with the possibility of default elements.
    Note that in this decorator alters the existing class instance instead of
    returning a wrapper object. """

    def filler(cls, *args, **kwargs):
        """ This is our custom initialization method. Input sanitation and
        ordering is outsourced to namedtuple. """

        for key, val in InputSanitizer(*args, **kwargs)._asdict().items():
            setattr(cls, key, val)

        filler.super_init(cls)

    def init_switcher(cls):
        filler.super_init = cls.__init__
        cls.__init__ = filler
        return cls

    # Taken from http://stackoverflow.com/questions/11351032/named-tuple-and-
    # optional-keyword-arguments
    InputSanitizer = namedtuple('InputSanitizer', args + tuple(kwargs.keys()))
    InputSanitizer.__new__.__defaults__ = tuple(kwargs.values())

    return init_switcher

Some test cases

import unittest

class TestAutoFill(unittest.TestCase):
    @autofill('a', b=12)
    class Foo(dict):
        pass

    def test_zero_input(self):
        with self.assertRaises(TypeError):
            self.Foo()

    def test_one_input(self):
        bar = self.Foo(1)
        self.assertEqual(bar.a, 1)
        self.assertEqual(bar.b, 12)

        bar = self.Foo(a=1)
        self.assertEqual(bar.a, 1)
        self.assertEqual(bar.b, 12)

        with self.assertRaises(TypeError):
            self.Foo(b=1)

        with self.assertRaises(TypeError):
            self.Foo(c=12)

    def test_two_input(self):
        bar = self.Foo(1, 2)
        self.assertEqual(bar.a, 1)
        self.assertEqual(bar.b, 2)

        bar = self.Foo(b=2, a=1)
        self.assertEqual(bar.b, 2)
        self.assertEqual(bar.a, 1)

    def test_other_object_functions(self):
        bar = self.Foo(1)
        bar.c = 3
        bar['key'] = 4

        self.assertEqual(bar.a, 1)
        self.assertEqual(bar.b, 12)
        self.assertEqual(bar.c, 3)
        self.assertEqual(bar['key'], 4)

if __name__ == '__main__':
    unittest.main()

The parameters to autofill could have better names: args gives the argument names to the constructor, and kwargs gives the keyword names and their default values, so perhaps argnames and defaults would be better. This will also help to distinguish them from the parameters to the filler function.
The function filler is used to implement the __init__ method, so its first argument should be named self, not cls.
The original __init__ method is recorded in the super_init property of the filler function. I think that's needlessly tricky. It would be simpler to record it in a local variable:

def init_switcher(cls):
    original_init = cls.__init__

    def init(self, *args, **kwargs):
        for k, v in InputSanitizer(*args, **kwargs)._asdict().items():
            setattr(self, k, v)
        original_init(self)

    cls.__init__ = init
    return cls

The implementation mechanism is quite ingenious! It would not have occurred to me to delegate the argument processing to collections.namedtuple. However, I think that it is clearer to delegate it to inspect.Signature:

from inspect import Parameter, Signature

def autofill(*argnames, **defaults):
    """Class decorator that replaces the __init__ function with one that
    sets instance attributes with the specified argument names and
    default values. The original __init__ is called with no arguments
    after the instance attributes have been assigned. For example:

    >>> @autofill('a', 'b', c=3)
    ... class Foo: pass
    >>> sorted(Foo(1, 2).__dict__.items())
    [('a', 1), ('b', 2), ('c', 3)]

    """
    def init_switcher(cls):
        kind = Parameter.POSITIONAL_OR_KEYWORD
        signature = Signature(
            [Parameter(a, kind) for a in argnames]
            + [Parameter(k, kind, default=v) for k, v in defaults.items()])
        original_init = cls.__init__

        def init(self, *args, **kwargs):
            bound = signature.bind(*args, **kwargs)
            bound.apply_defaults()
            for k, v in bound.arguments.items():
                setattr(self, k, v)
            original_init(self)

        cls.__init__ = init
        return cls
    return init_switcher

There's no test case that checks that the original __init__ method is called.


MallocRaii implementation I'm playing around with RAII a bit lately and I wan't to know if/how I can improve this (quite simple but very helpful) class.

A word to two decisions I've made and why:

No error handling in construct. I don't think throwing a exception would be appropriate so it's up to the user to check valid().
No copy constructor. I don't see how this could be useful. Since two objects can't share the same ptr, a copy could malloc another chunk of memory with same size and copy the old one, but I don't know if this would be useful at all.

template<typename T>
class MallocRaii {
public:
    MallocRaii(size_t size) : ptr(nullptr) {
        this->ptr = (T*)malloc(sizeof(T) * size);
    }

    ~MallocRaii() {
        if(this->ptr != nullptr)
            free(this->ptr);
    }

    MallocRaii(const MallocRaii& other) = delete; //Copy constructor
    MallocRaii& operator=(const MallocRaii& other) = delete; //Copy assignment

    MallocRaii(MallocRaii&& other) { //move constructor
        std::swap(*this, other);
    }

    MallocRaii& operator=(MallocRaii&& other) { //Move assignment
        std::swap(*this, other);
    }

    bool valid() const {
        return ptr == nullptr;
    }

    T* ptr;
};

Use it like this:

MallocRaii<unsigned long> buffer(256);
some_c_api_read_pixel_from_file(buffer.ptr, 256);
//Reads 256 pixels with 4 components from file,
// the class allocated 1024 bytes (256 * sizeof unsigned long)
do_something_with_the_data(buffer.ptr);
return; //class free's memory

You have:
bool valid() const {
    return ptr == nullptr;
}

I think you meant != there. Regardless, you probably don't need this method; and if you must have it, consider spelling it either
operator bool() const { return ptr != nullptr; }
// or else
bool operator==(std::nullptr_t) const { return ptr != nullptr; }

Try the following program:
int main() {
    MallocRaii m(10);
    MallocRaii n(10);
    m = std::move(n);  // infinite loop
}

This is because std::swap is implemented in terms of move-assignment, but you've chosen to implement move-assignment in terms of std::swap! Here are some slides on the subject (full disclosure: I wrote them).

A minor nit:
   if(this->ptr != nullptr)
        free(this->ptr);

You're missing a space after if; and you don't need the test anyway, because free(nullptr) is already guaranteed to do the right thing. Simply write
~MallocRaii() { free(ptr); }

You make ptr a public member variable. This allows anybody to come along and modify it:
MallocRaii m(10);
m.ptr = nullptr;  // oops! memory leak!

Prefer to expose the value of the pointer (but not an actual reference to it) via an accessor, traditionally spelled get().
T *get() const { return ptr; }

Finally, be aware that you could always just use std::unique_ptr<T, decltype(free)> instead of writing your own class; or at least that you could pretty easily implement your MallocRaii class in terms of unique_ptr. This would give you foolproof swap/move/no-copy boilerplate for free (via the Rule of Zero), and would be shorter as well.