Recursive flattening of Swift sequences In Flatten to get all child controls of certain type in a UIView, methods were discussed to
recursively flatten a tree-like structure in Swift, resulting in an array
of all elements.

Motivated by that thread, I have written a flatten function which creates
a sequence instead. This can be an advantage if the resulting elements are to be post-processed (e.g. filtered), because they can be created
"lazily" or "on-demand", instead of creating an array with all
resulting elements first.

func sequentialFlatten<S : SequenceType>
    (seq : S, children : (S.Generator.Element) -> S) -> SequenceOf<S.Generator.Element>
{
    return SequenceOf {
        () -> GeneratorOf<S.Generator.Element> in

        // Current generator, or `nil` if all sequences are exhausted:
        var gen : S.Generator? = seq.generate()
        // Queue of generators for sequences which still have to be enumerated:
        var queue : [S.Generator] = []

        return GeneratorOf {

            while gen != nil {
                if let next = gen!.next() {
                    queue.append(children(next).generate())
                    return next
                }
                // Current generator is empty, try to deque next one:
                gen = queue.first // `nil` if queue is empty
                if (gen != nil) {
                    queue.removeAtIndex(0)
                }
            }
            return nil
        }
    }
}

The function arguments are an initial sequence, and a closure which
transforms any sequence element into a new sequence (which may be
empty). One can view seq as a set of root nodes in a tree
or forest,  and children as the mapping from each node to its children.

(Swift has a related flapMap() function which takes a sequence and a
transformation as argument, but that does not work recursively, and
returns an array and not a sequence.)

Example 1:

let someView : UIView = ...
let views = sequentialFlatten([someView], { $0.subviews as! [UIView] })
let labels = lazy(views).filter( { $0 is UILabel } ).map( { $0 as! UILabel } )

creates a sequence of all views in the view hierarchy of someView,
and then extracts all labels from that sequence. (Note that no intermediate array of all views is created.)

Example 2:

let seq = sequentialFlatten([0], { n in n < 50 ? [ 2*n+1, 2*n+2] : []})
for x in seq { println(x) }

is an unconventional method to print the numbers 0 ... 100.

I wrote this function mainly for my own educational purpose, to get
more familiar with Swift sequences and generators. All feedback is
welcome, for example:

Naming (function and variables).
Swifty-ness: things which should be done different in Swift.
Implementation: can the same be achieved easier, perhaps using
available functions from the Swift standard library?
Can sequentialFlatten() be implemented in a way that it calls itself
recursively?

This question is more than a year old, so a few things have changed in Swift since. Most notably, SequenceOf and GeneratorOf have been renamed to AnySequence and AnyGenerator respectively.

I'd rename gen to generator so that in the while loop, you can do:
while let gen = generator {
    ...
}

That way, you don't have to force unwrap gen.

You need to declare generator and the elements of queueas AnyGenerators for this to work. That way they are ensured to be reference types and gen and generator refer to the same instance.
// Current generator, or `nil` if all sequences are exhausted:
var generator: AnyGenerator<S.Generator.Element>? = AnyGenerator(seq.generate())
// Queue of generators for sequences which still have to be enumerated:
var queue: [AnyGenerator<S.Generator.Element>] = []

The four lines where you try to dequeue the first generator could be written in one line if Arrays had a popFirst function. (Which it should but weirdly doesn't.) So we can define that ourselves in an extension. (though this really is a matter of preference)
extension Array {
    mutating func popFirst() -> Generator.Element? {
         if let elem = first {
            removeAtIndex(0)
            return elem
        }
        return nil
    }
}

Now we can write:
generator = queue.popFirst()

This one might be going a bit far because we're extending Array just to get rid of 3 lines, but I really think it improves readability. In the end it's your call.

Put together, that gives us:
func sequentialFlatten<S : SequenceType>(seq : S, children : (S.Generator.Element) -> S) -> AnySequence<S.Generator.Element> {
    return AnySequence {
        () -> AnyGenerator<S.Generator.Element> in

        // Current generator, or `nil` if all sequences are exhausted:
        var generator: AnyGenerator<S.Generator.Element>? = AnyGenerator(seq.generate())
        // Queue of generators for sequences which still have to be enumerated:
        var queue: [AnyGenerator<S.Generator.Element>] = []

        return AnyGenerator {

            while let gen = generator {
                if let next = gen.next() {
                    queue.append(AnyGenerator(children(next).generate()))
                    return next
                }
                // Current generator is empty, try to deque next one:
                generator = queue.popFirst() // `nil` if queue is empty
            }
            return nil
        }
    }
}

This version works exactly As your old version, which is breadth-first. This less than ideal, since a breadth first search takes more memory. Additionally, you asked:

Can sequentialFlatten() be implemented in a way that it calls itself recursively?

Let's see if we can't kill two birds with one stone.
SequentialFlatten(), a recursive approach
Here's my thinking, if we want to go depth first, we must explore the subtree attached to an element before visiting its siblings. We can solve this problem recursively, by maintaining two generators at each level. one for the sequence that was passed in, and another one for the children of the top level element we last generated:
var mainGen = seq.generate()
// Current generator, or `nil` if all sequences are exhausted:
var generator: AnyGenerator<S.Generator.Element>?

Initially, generator is nil, so we'll definitely use a guard to unwrap it and return the first element of the main generator if it's nil:
guard let gen = generator else {
    if let elem = mainGen.next() {
        generator = // get the generator for elem's children
        return elem
    }
    return nil
}

This is where recursion comes in:
generator = sequentialFlatten(children(elem), children: children).generate()

It might have been better to define a local function that return a Generator and make that recursive instead of wrapping the Generator up in a Sequence only to then get the Generator again, but I think either way works.

Now all that's left to do is return the right element if generator is not nil and create a new generator when our current one is empty. This can all be solved in one go:
guard let gen = generator, let elem = gen.next() else {
    if let elem = mainGen.next() {
        generator = sequentialFlatten(children(elem), children: children).generate()
            return elem
        }
        return nil
    }
    return elem

We now have the next element of the generator and the generator being nil or empty can be treated the same.

Putting this all together gives us:
func sequentialFlatten<S : SequenceType>(seq : S, children : (S.Generator.Element) -> S) -> AnySequence<S.Generator.Element> {
    return AnySequence {
        () -> AnyGenerator<S.Generator.Element> in

        var mainGen = seq.generate()
        // Current generator, or `nil` if all sequences are exhausted:
        var generator: AnyGenerator<S.Generator.Element>?

        return AnyGenerator {

            guard let gen = generator, let elem = gen.next() else {
                if let elem = mainGen.next() {
                    generator = sequentialFlatten(children(elem), children: children).generate()
                    return elem
                }
                return nil
            }
            return elem
        }
    }
}

which is 5 lines of code shorter and requires O(log n) storage instead of O(n).


Abstract the (duplicate?) behavior between "sort" and "replace" I have written these two functions that have a similar process.  The first is meant to "split" a string on a given character and the second is meant to "replace-all" instances of a character in a given string.  The behavior between the two seems very similar - can you think of a good way to abstract that behavior into a third, reusable, function?  (which could be executed by calling, for example, (where-found sought-letter searchee-string thing-to-do)).

(defun split (separator splittee)
  (let ((next-index (position separator splittee)))
    (if next-index
      (cons (subseq splittee 0 next-index) (split separator (subseq splittee (1+ next-index))))
      (list splittee))))

(defun replace-all (replacand replacee replacor)
  (let ((next-index (position replacand replacee)))
    (if next-index
      (concatenate 'string (subseq replacee 0 next-index) replacor (replace-all replacand (subseq replacee (1+ next-index)) replacor))
      replacee)))

What do you think?  Is this better than the original code, or is it too complicated?

(defun where-found (sought-letter the-string to-do-onroute to-do-on-leaf)
  (let ((next-index (position sought-letter the-string)))
    (if next-index
      (apply to-do-onroute
         (list (subseq the-string 0 next-index)
           (subseq the-string (1+ next-index))))
      (apply to-do-on-leaf (list the-string)))))

(defun split (separator splittee)
  (where-found separator splittee
           (lambda (pre-string post-string) (cons pre-string (split separator post-string)))
           (lambda (leaf-string) (list leaf-string))))

(defun replace-all (to-be-replaced-string the-patient-string the-replacement-string)
  (where-found to-be-replaced-string the-patient-string
           (lambda (pre-string post-string)
         (format t "pre: ~a post: ~a the-replacement-string: ~a ~%" pre-string post-string the-replacement-string)
         (concatenate 'string pre-string the-replacement-string (replace-all to-be-replaced-string post-string the-replacement-string)))
           (lambda (leaf-string) leaf-string)))

(format t "the cat is here: ~a ~%" (replace-all #\Space "the cat is here" ""))
(format t "the cat is here: ~a ~%" (split #\Space "the cat is here"))

I think the later versions are unnecessarily complicated and verbose.

My first idea was to just concatenate the splitted list parts in replace-all – for you can do (concatenate 'string seqs ... ) – but that may not be very efficient.

You can use map or loop for replace-all. This map version works for lists too:

(defun replace-all (seq from to)
  (map (type-of seq) (lambda (char) (if (equal from char) to char)) seq))

Naming things clearly can sometimes be hard, and I think sometimes being terse helps readability more than being too-explicit-and-specific. That if statement above could need some better naming of things...

There is an utility library called split-sequence, that has many useful options, like

remove empty subsequences
only split count times
return the index position (useful for further processing)

I'm still learning Lisp, so you may want to hear see other answers also.


Checking datasets for tables and rows Is there a cleaner way of checking a dataset for tables/rows before attempting to read them?

If ds.Tables.Count > 0 Then
    If ds.Tables(0).Rows.Count > 0 Then

        'do something with the the rows at this point

    End If
End If

Edit to clarify - I am talking specifically about the two bool checks to see if the dataset acutally contains any data in table(0).

I'm doing a couple of assumptions here, but I hope I'm on target.

If you just want the first column of the first row, you should use SqlCommand.ExecuteScalar instead of reading it into a dataset.

For instance:

using (var cn = new SqlConnection("..."))
{
    using (var cmd = new SqlCommand("SELECT statusCode FROM table WHERE whatever", cn)
    {
        cn.Open();
        return (string)cmd.ExecuteScalar();
    }
}

(Notice the using statement if you don't know it. It automatically disposes the connection and command)

You could also look into using SqlCommand.ExecuteReader which returns an SqlDataReader. The latter is cleaner and has better performance than the dataset method too.

using (var reader = cmd.ExecuteReader())
{
    while(reader.Read())
    {
        var value = reader.GetString(0);
    }
}


Setting periodicity in the week in Google Calendar I am trying to write a generic JavaScript subroutine to set the periodicity in the week:

var items = {"0": "none", "1":"Daily", "2": "Every weekday (Monday to Friday)", "3": "Weekly"};

I would like to improve this to make it more generic and readable if possible.

var callback = selectDay(idValue);
createSelect(items, idValue, callback);

var createSelect = function (items, idValue) {
    var selElem = document.createElement("select");
    $.each(items, function (key, value) {
        var ov = document.createElement("option");
        ov.value = key;
        ov.appendChild(document.createTextNode(value));
        selElem.appendChild(ov);
    });
    $(idValue).prepend(selElem);
};

var selectDay = function (idValue, element) {
    var element = $(idValue);
    var childElements = idValue + '_';
    element.find('select').change(function () {
        if($(this).val() === '0') {
            element.find('label').hide();
            $(this).closest('div').find('input').attr("checked", false);
        }
        if($(this).val() === '1') {
            element.find('label').hide();
            $(this).closest('div').find('input').attr("checked", true);
        }
        if($(this).val() === '2') {
            element.find('label').show();
            for (var i = 0; i < 5; i += 1) {
                $(childElements + i).attr("checked", true);
            }
            for (var i = 5; i < 7; i += 1) {
                $(childElements + i).attr("checked", false);
            }
        }
        if($(this).val() === '3') {
            element.find('label').show();
            $(this).closest('div').find('input').attr("checked", false);
            $(childElements + (new Date()).getDay()).attr("checked", true);
        }
    });
};

var items = {"0": "none", "1":"Daily", "2": "Every weekday (Monday to Friday)", "3": "Weekly"};
var idValue = '#contest_data_updatePeriodicity_days';
createSelect(items, idValue);
selectDay(idValue);

My questions are:

How can I ameliorate this piece of code?
How can make a callback in order to make something like this?

JSFiddle

Use prop() instead of attr().
See the first point.
You're declaring var selectDay = function(idValue, element), your element argument is probably a typo? You don't need it.
See the first point.
Use switch instead of all those if. Or at least cache the $(this).val() result.
See the first point.
You don't need element.find(), you could use the context (somehow cleaner).

Overall, here is some code to give you the idea:

var selectDay = function (idValue) {
    $('select', idValue).change(function() {
        switch ($(this).val()) {
            case '0':
                $('label', idValue).hide();

                // Use prop(), not attr().
                $(this).closest('div').find('input').prop('checked', true);
                break;
        }
    });
};

Now you may notice something: this doesn't have exactly the same behavior as your code. Guess why :-)

A friend of mine says that whenever you use a switch case, you can actually use a mapping object. This might be another way to do it. The mapping object would be something like:

var mapping = {
    '0': {
        'label': 'hide',
        'checked': false
    },
    '1': {
        'label': 'hide',
        'checked': true
    }
};

And then you code accordingly to this object.


Testing code that changes the DOM on page load I am in the process of writing tests for my JS code with CasperJS and Mocha. I have a function that changes the DOM and gets run on page load (through jQuery's document.ready event). I'm wondering what the best practice is for testing this kind of code. For example, I have a function removeElements that gets run before my tests start:

removeElements: function() {
    // Remove the original onsubmit attribute.
    $("#LoginForm").attr("onsubmit", "");
    $("#btnEnter").attr("onclick", "");

    // Modify form inputs.
    $("label[for=fieldPassword]").remove();

    $("#fieldPassword").remove();

    $("input[name=username][type=hidden]").remove();
}

Do you think I should only test that those elements have already been removed from the page from the execution of the code above? I am thinking that I should first test that those elements were there initially, call the function, then test that those elements and attributes were removed.

I suppose my real question is: what do I need to test, and what can I safely assume works without testing when it comes to DOM-altering code?

I think the real issue you are trying to solve is how to test code that has reach over the whole document. Just judging by your small snippet of code, you appear to be at least using "classes". Regardless of how you've written your application, you want to put yourself into this position when unit testing DOM manipulations:

describe("Something", function() {
    var div, something;

    beforeEach(function() {
        div = document.createElement("div");
        something = new Something(div);
    });

    it("modifies the DOM", function() {
        div.innerHTML = [
            '<form onsubmit="submit()">',
                '<label for="fieldPassword">Password</label>',
                '<input type="password" id="fieldPassword">',
                '<input type="hidden" name="username">',
                '<input type="submit" id="btnEnter" onclick="click()" value="Enter">',
            '</form>'
        ].join("");

        something.removeElements();

        expect(div.querySelector("form").onsubmit).toBe("");
        expect(div.querySelector("label")).toBe(null);
        expect(div.querySelector("input[type=password]")).toBe(null);
        expect(div.querySelector("input[type=hidden]")).toBe(null);
        expect(div.querySelector("input[type=submit]").onclick).toBe("");
    });
});

The code under test should have access to some sort of root element, which it then operates on. The beforeEach in your tests would create a new DIV tag, and a new instance of the object you want to test. This class should take the DIV tag as a constructor argument, ideally, but in the very least assign it as a property. Then your test is completely isolated from the rest of the DOM tree. There is no need to clean up these elements because they are all disconnected from the main document.

function Something(element) {
    this.element = $(element);
}

Something.prototype.removeElements = function() {
    this.element.find("#LoginForm").attr("onsubmit", "");
    this.element.find("#btnEnter").attr("onclick", "");

    // Modify form inputs.
    this.element.find("label[for=fieldPassword]").remove();

    this.element.find("#fieldPassword").remove();

    this.element.find("input[name=username][type=hidden]").remove();
};


Wireless Rickroll using Arduino This is a script I made for the ESP8266 that uses the Wi-Fi chip and creates a new Access Point every 5 seconds in order to display a message on WiFi lists on nearby devices that looks like this:

The code is:

#include <ESP8266WiFi.h>

const char* ssids[] = {"Never gonna give you up","Never gonna let you down","Never gonna run around","Never gonna make you cry","Never gonna say goodbye","Never gonna tell a lie"};
const char* pass = "pass_goes_here";
void setup() {
  // put your setup code here, to run once:
  Serial.begin(9600);
  int currentssidno = 0;
  while (true)
  {
    const char* ssid = ssids[currentssidno];
    Serial.print("SSID: ");
    Serial.println(ssid);
    WiFi.softAP(ssid, pass);
    delay(5000);
    WiFi.softAPdisconnect(false);
    currentssidno = currentssidno + 1;
    if (currentssidno == 6) //please change this count if you change the amount of ssids
    {
      currentssidno = 0;
    }
  }
}

void loop() {
}

The code is also on GitHub.

How can I improve this code? This is my second or third project with Arduino and I pretty much made it in 15 minutes and looking at it now, some parts feels like dirty code (especially the part where I switch reset the SSID counter), how can I improve this?

Extra resources: Wi-Fi Library, ESP8266 Arduino Core

No particular comment on the Arduino-specific or Wifi-specific stuff, but here:

currentssidno = currentssidno + 1;
if (currentssidno == 6) //please change this count if you change the amount of ssids
{
  currentssidno = 0;
}

Two things. First, you can rephrase this conditional logic as simply

currentssidno += 1;
currentssidno %= 6;  // please change this, etc.

or equivalently

currentssidno = (currentssidno + 1) % 6;  // please change this, etc.

depending on where you'd prefer to repeat the name currentssidno. (I'd probably rename that variable to i and then use the latter form.)

Secondly, don't hard-code magic numbers like 6, if you can express them as "non-magic" numbers such as "the size of this particular array I've got".

// Put this macro in your toolkit. You'll use it frequently.
#define DIM(arr) (sizeof (arr) / sizeof (arr)[0])

/* ... */
currentssidno = (currentssidno + 1) % DIM(ssids);

Now you no longer need the comment, because the code no longer has that weird subtle dependency on the length of the ssids array. It Just Works, no matter what ssids you want to use.


DFS algorithm + unit test Please comment on my DFS implementation and test, I would like to get comments about algorithm correctness.

  Depth-first search (DFS) is an algorithm for traversing or searching tree or graph data structures. One starts at the root (selecting some arbitrary node as the root in the case of a graph) and explores as far as possible along each branch before backtracking.

using System;
using System.Collections.Generic;
using System.Linq;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace JobInterviewTests
{
    [TestClass]
    public class DFSUnitTest
    {
        [TestMethod]
        public void CheckCorrectOrder()
        {
            GraphNode root = new GraphNode("A");
            root.Neighbors.Add(new GraphNode("F"));
            root.Neighbors.Add(new GraphNode("E"));
            var temp = new GraphNode("B");
            var temp2 = new GraphNode("C");
            var temp3 = new GraphNode("D");
            temp2.Neighbors.Add(temp3);
            temp.Neighbors.Add(temp2);
            root.Neighbors.Add(temp);

            List<string> result =TraverseDFS(root);
            Assert.AreEqual("A",result[0]);
            Assert.AreEqual("B",result[1]);
            Assert.AreEqual("C",result[2]);
            Assert.AreEqual("D",result[3]);
            Assert.AreEqual("E",result[4]);
            Assert.AreEqual("F",result[5]);
        }

        private List<string> TraverseDFS(GraphNode root)
        {
            if (root == null)
            {
                return null;
            }

            List<string> result = new List<string>();
            Stack<GraphNode> s = new Stack<GraphNode>();
            s.Push(root);
            while (s.Any())
            {
                GraphNode curr = s.Pop();
                result.Add(curr.Value);
                foreach (var node in curr.Neighbors)
                {
                    s.Push(node);
                }
            }
            return result;
        }
    }

    public class GraphNode
    {
        public string Value { get; set; }
        public List<GraphNode> Neighbors { get; set; }

        public GraphNode(string s)
        {
            Value = s;
            Neighbors = new List<GraphNode>();
        }
    }
}

The implementation of depth-first traversal is correct.
But there are several elements of this program and tests that could be improved.

Prefer empty collections over null values

When the input graph is empty,
it would be better to return an empty list instead of null.
Collections are used for iterating over its elements.
When a collection variable can be null,
that's troublesome, because before you can iterate over it,
you have to check that it's not null.

Constraints and limitations

The implementation will only work with directed graphs without cycles,
otherwise you will have an infinite loop.
This is important to include in the description.
A common follow-up question at an interview is how to make it work in graphs with cycles.

Weak tests

The test knows too much about the implementation.
There are 6 possible valid depth-first traversals of the example graph,
and the test knows exactly which one to expect.
It all comes down to the evaluation order of neighbor,
in your implementation it's the reverse order that they were added.
Either this implementation should be documented,
or the test should not depend on it.

The example graph is not a good choice to demonstrate depth-first traversal,
because A F E B C D would be valid both as depth-first and as breadth-first traversal.
A more expressive example would be:

    A
   / \
  B   C
 /     \
D       E

With this example all depth-first and breadth-first traversals would be distinct.

Lastly, a technical detail,
instead of comparing the elements of a list one by one to values,
you could compare entire lists, which would be a lot easier to type.

List<string> expected = new List<string>(new string[] { "A", "B", "C", "D", "E", "F" });
List<string> result = TraverseDFS(root);
Assert.AreEqual(expected, result);

Don't provide setters if you don't really need

The fields of GraphNode are never modified,
therefore the setters are unnecessary.


Project Euler 22: Names scores by Loki Started working through Project Euler this weekend.

Project Euler 22

  Using names.txt (right click and 'Save Link/Target As...'), a 46K text
  file containing over five-thousand first names, begin by sorting it
  into alphabetical order. Then working out the alphabetical value for
  each name, multiply this value by its alphabetical position in the
  list to obtain a name score.

  For example, when the list is sorted into alphabetical order, COLIN,
  which is worth 3 + 15 + 12 + 9 + 14 = 53, is the 938th name in the
  list. So, COLIN would obtain a score of 938 × 53 = 49714.

  What is the total of all the name scores in the file?

#include <iostream>
#include <fstream>
#include <iterator>
#include <string>
#include <set>
#include <numeric>
#include "PunctFacet.h"

using ThorsAnvil::Util::PunctFacet;

long scoreName(std::string const& name)
{
    return std::accumulate(std::begin(name), std::end(name), 0L,
                           [](long v1, char x){return v1 + x - 'A' + 1;});
}

int main()
{
    // Open a file that considers " and , as space and thus ignores them
    std::ifstream   data;
    data.imbue(std::locale(std::locale(), new PunctFacet(std::locale(), "\",")));
    data.open("euler/e22.data");

    // read all the names in a set (its sorted)
    std::set<std::string> names{std::istream_iterator<std::string>(data),
                                std::istream_iterator<std::string>()};

    // Calculate the result
    long    score   = 0;
    long    loop    = 1;
    for(auto name: names) {
        score += (loop * scoreName(name));
        ++loop;
    }
    std::cout << score << "\n";
}

PunctFacet.h

#ifndef THORSANVIL_UTIL_PUNCT_FACET_H
#define THORSANVIL_UTIL_PUNCT_FACET_H

#include <locale>
#include <string>
#include <sstream>

namespace ThorsAnvil
{
    namespace Util
    {

// This is my facet that will treat the characters in `extraSpace`
// as space characters and thus ignore them with formatted input
class PunctFacet: public std::ctype<char>
{
    public:
        typedef std::ctype<char>    base;
        typedef base::char_type     char_type;

        PunctFacet(std::locale const& l, std::string const& extraSpace)
            : base(table)
        {
            std::ctype<char> const&  defaultCType  = std::use_facet<std::ctype<char> >(l);

            // Copy the default value from the provided locale
            static  char data[256];
            for(int loop = 0;loop < 256;++loop) { data[loop] = loop;}
            defaultCType.is(data, data+256, table);

            // Modifications to default to include extra space types.
            for(auto space: extraSpace) {
                table[space]  |= base::space;
            }
        }
    private:
        base::mask  table[256];
};

    }
}

#endif

Results

> g++ -O3 -std=c++14 euler/e22.cpp
> time ./a.out
871198282

real    0m0.018s
user    0m0.009s
sys     0m0.007s

#include <numeric>!

I see no reason why individual score is computed via std::accumulate, but the total uses a loop. I recommend std::inner_product there, at least for consistency.

Similarly, the

        for(int loop = 0;loop < 256;++loop) { data[loop] = loop;}

is

        std::iota(data, data + 256, 0);

Of course, a magic 256 shall be replaced by a symbolic name.

I don't feel comfortable with std::set used just to avoid sorting. It  obscures the intention. You seem to have the same feeling, and put the clarifying comment. I recommend to be explicit: read the names into a vector, and sort it. BTW, it would also reduce space requirements.


Certainty of two comparative values (A/B results certainty) I'm trying to output the percentage of certainty of two comparatives data.

Here's the resulting code:

/**
 * Original code from https://vwo.com/js/significanceCalculator.js
 */
function calculatePValue (c_t, c_c, v_t, v_c) {
    var d1 = 0.0498673470,
        d2 = 0.0211410061,
        d3 = 0.0032776263,
        d4 = 0.0000380036,
        d5 = 0.0000488906,
        d6 = 0.0000053830;

    var c_p = c_c / c_t;
    var v_p = v_c / v_t;

    var std_error = Math.sqrt((c_p * (1 - c_p) / c_t) + (v_p * (1 - v_p) / v_t));
    var z_value = (v_p - c_p) / std_error;

    var a = Math.abs(z_value);
    var p_value = 1.0 + a * (d1 + a * (d2 + a * (d3 + a * (d4 + a * (d5 + a * d6)))));

    p_value *= p_value;
    p_value *= p_value;
    p_value *= p_value;
    p_value *= p_value;
    p_value = 1.0 / (p_value + p_value);
    if (z_value >= 0)
        p_value = 1 - p_value;

    if (p_value > 0.5)
        p_value = 1 - p_value;

    p_value = Math.round(p_value * 1000) / 1000;
    return p_value;
}

function getPercentage(totalA, convA, totalB, convB) {
    var p = calculatePValue(totalA, convA, totalB, convB);
    if (p < 0,5) {
        return 'B, with certainty of : ' + ((1-p)*100).toFixed(2) + '%';
    } else {
        return 'A, with certainty of : ' + (p*100).toFixed(2) + '%';
    }
}

For example, if you try with:

getPercentage(1000, 120, 500, 90);

you'll get:

  "B, with certainty of : 99.90%"

The code for calculating P was extracted from here. I simply took it as a percentage value to display either A or B is better.

What do you think? Is this ok?

The code below doesn't reproduce your code entirely, it just illustrates what I mentioned earlier in the comments:

function example() {
    var values = [0.0498673470,
                  0.0211410061,
                  0.0032776263,
                  0.0000380036,
                  0.0000488906,
                  0.0000053830];
    function sum(a, b) { return a + b; }
    function sqdiff(a, b) { return (b - a) * (b - a); }
    function avg(data) {
        return data.reduce(sum) / data.length;
    }
    function curry(f) {
        var args = [].slice.call(arguments);
        args.shift();
        return function () {
            return f.apply(null, args.concat([].slice.call(arguments)));
        };
    }
    function std(data) {
        var a = avg(data);
        return Math.sqrt(data.map(curry(sqdiff, a)).reduce(sum) /
            data.length);
    }
    console.log("Standard deviation: " + std(values));
}

You, most likely, won't want to write functions like curry yourself though. There are libraries out there which already do such (and many more) useful things. I believe that Lodash already has one.


Testing whether a string contains valid JSON by calling json_decode() I had a code review and got told by the senior programmer that it was very bad programming practice to use the json_decode() function of php to determine if a string was not in JSON format.

My argument was that the json_decode function has a fail case (returning NULL) and this exists for reasons such as the purpose I was using it for.

I got sat down one on one and told to never pass data to a function that was not intended for the function (in this case referring to passing a hex string to the json_decode() function).

Is what I did bad coding practice? How should I have written the code?

Here is the php code of what I basically did:

//example JSON Data
/*
$ExampleData = '
{
    "test": "48656c6c6f20576f726c64"
}
';
*/

//the most common case is that a hex string sent to the php script
$ExampleData = "48656c6c6f20576f726c64";

$jsonData = json_decode($ExampleData);

//one in every 1000 responses is JSON data
if($jsonData)
{
    //extract varaibles from the json
    print ("JSON data recieved");
}
else
{
    //response is hex so carry out normal function
    print ("Hex Data recieved");
}

try it online

I strongly agree with comment by @Iwrestledabearonce.  When dealing with input, there are normally other means to understand content type (content type headers, additional input fields, or other means to express an explicit contract).  Having forked code paths like this probably means you have a bad design and you should make sure the contract with data source is clear around how to convey what type of data is being passed.  That doesn't mean that the data passed still doesn't need to be validated as hex or JSON input, just that you shouldn't have to rely on performing JSON decoding or regex matching to determine your input data type.

This is especially exacerbated by the fact you mention 1 in 1000 request may have JSON.  Do you really want to perform json_decode() step on all 999 other requests?


Anti-Bot comment system With some advice from SO, I developed this system, which I think is quite strong for bots to automatically post comments.

index.php

 <form name="f" action="poster.php" method="post">
  <input type="text" name="text"/>

  <input type="text" name="key" id="key" value=""/>

  <input type="submit">
 </form>

getToken.php

<?php
 $key=date("Y-m-d H:i:s");
 $hash=sha1($key.'mySecretKey');
 echo $key.'#'.$hash;
?>

poster.php

<?php
if (!isset($_POST['key']))
 exit;

$parts = explode('#',$_POST['key'],2);
$key = $parts[0];
$hash = $parts[1];

$date1 = $key;
$date2 = date("Y-m-d H:i:s");
$diff = abs(strtotime($date2) - strtotime($date1));
$years   = floor($diff / (365*60*60*24));
$months  = floor(($diff - $years * 365*60*60*24) / (30*60*60*24));
$days    = floor(($diff - $years * 365*60*60*24 - $months*30*60*60*24)/ (60*60*24));
$hours   = floor(($diff - $years * 365*60*60*24 - $months*30*60*60*24 - $days*60*60*24)/ (60*60));
$minuts  = floor(($diff - $years * 365*60*60*24 - $months*30*60*60*24 - $days*60*60*24 - $hours*60*60)/ 60);
$seconds = floor(($diff - $years * 365*60*60*24 - $months*30*60*60*24 - $days*60*60*24 - $hours*60*60 - $minuts*60));

if ($seconds < 5)
 echo $seconds.' Too fast, must be a naughty bot
';
else if ($seconds>5 && $seconds < 600)
 echo $seconds.' In time
';
else
 echo $seconds.' time out
';

if ($hash == (sha1($key.'sou')))
 echo $_POST['text'];
else
 echo 'You are a bot !';
?>

365*60*60*24 should be a constant.
You should really improve your variable naming.

$date2 = date("Y-m-d H:i:s");

date2? Does that say anything? currentTime is more like it. Always describe what variables contain, not what they are.
Isn't $diff already the number of seconds?
Most part of the date/time checking could be rewritten to:

$seconds = time() - strtotime($key);


Efficiently detect datacenter based on server hostname In our company we have a machine hostname as  -

dbx111.dc1.host.com
dbx112.dc2.host.com
dcx113.dc3.host.com

Here dc1, dc2, dc3 are our datacenter and we will be having only three datacenter. But it might be possible that they can have more dots in between separated by another domain.

Now I need to find in which datacenter my current machine is in. I will be running below code in some machine as shown above.. And below code will tell me which datacenter my code is running in. But is there any way to modify the below code to make it more efficient? I guess we can write this much better -

String datacenter = getCurrentDatacenter();

private String getCurrentDatacenter(){

    String s_hostName = null;;
    try {
        s_hostName = InetAddress.getLocalHost().getCanonicalHostName();
    } catch (UnknownHostException e) {
        s_logger.debug(LogLevel.ERROR+"\t"+e.getStackTrace().toString());
    }

    if (s_hostName != null && !s_hostName.isEmpty()){

        s_hostName.toUpperCase();
        if (s_hostName.contains(DatacenterEnum.DC1.name())){
            currentDatacenter = DC1;
        }
        else if (s_hostName.contains(DatacenterEnum.DC2.name())){
            currentDatacenter = DC2;
        }
        else if (s_hostName.contains(DatacenterEnum.DC3.name())){
            currentDatacenter = DC3;
        }
    }

    return currentDatacenter;
}

Below is my ENUM -

public enum DatacenterEnum {
    DC1, DC2, DC3;

    private static final Map<Integer, DatacenterEnum> BY_CODE_MAP = new LinkedHashMap<Integer, DatacenterEnum>();

    static {
    for (DatacenterEnum rae : DatacenterEnum.values()) {
        BY_CODE_MAP.put(rae.ordinal(), rae);
    }
    }

    public static String forCode(int code) {
    return BY_CODE_MAP.get(code).name();
    }
}

There are a few things which I think will simplify your code a lot, and it all centers around your Enum.

Firstly, there is no need in your enum to have the BY_CODE_MAP system. Remove it. You can very efficiently (and with the identical results) use the method:

    public static String forCode(int code) {
        return (code >= 0 && code < values().length) ? values()[code].name() : null;
    }

The values() array in an enum will always be indexed in the same order as the ORDINAL of each Enum.

The second thing, is that you should only be calculating the Datacenter location once for each JVM. Having it as a method which is called potentially multiple times, each time interrogating the java.net.* layer for hostnames, is inefficient.

I suggest you add a static initializer method to your Enum that 'knows'.

Consider the following changes to your enum, and replace the logic in your code:

private String getCurrentDatacenter(){
    DatacenterEnum us = DatacenterEnum.getCurrentDatacenter();
    return us == null ? null : us.name();
}

Then, your Enum will look like (I have commented out the code I think is redundant):

import java.net.InetAddress;
import java.net.UnknownHostException;

public enum DatacenterEnum {
    DC1, DC2, DC3;

//    private static final Map<Integer, DatacenterEnum> BY_CODE_MAP = new LinkedHashMap<Integer, DatacenterEnum>();

//    static {
//        for (DatacenterEnum rae : DatacenterEnum.values()) {
//            BY_CODE_MAP.put(rae.ordinal(), rae);
//        }
//    }

    public static String forCode(int code) {
        return (code >= 0 && code < values().length) ? values()[code].name() : null;
//        return BY_CODE_MAP.get(code).name();
    }

    private static final DatacenterEnum ourlocation = compareLocation();

    private static DatacenterEnum compareLocation() {
        String ourhost = getHostName();
        for (DatacenterEnum dc : values()) {
            String namepart = "." + dc.name().toLowerCase() + ".";
            if (ourhost.indexOf(namepart) >= 0) {
                return dc;
            }
        }
        return null;
    }

    private static final String getHostName() {
        try {
            return InetAddress.getLocalHost().getCanonicalHostName().toLowerCase();
        } catch (UnknownHostException e) {
            // s_logger.debug(LogLevel.ERROR+"\t"+e.getStackTrace().toString());
            return "localhost";
        }
    }

    public static DatacenterEnum getCurrentDatacenter() {
        return ourlocation;
    }

    public static void main(String[] args) {
        System.out.println(DatacenterEnum.getCurrentDatacenter());
    }

}

If you ever have additional datacenters, you can simply add a new name to the enum and the rest will work. Also, you can add values that are not the same naming convention... for example, if you add the ENUM "EU1" for your first european datacenter, it will match aaa.eu1.xxx.yyy


An attempt on unit testing in C++ I have no much experience with C++, so it is about time to start actually doing something about it. All in all, I coded this tiny unit test library:

assert.h:

#ifndef ASSERT_H
#define ASSERT_H

#include <iostream>

#define ASSERT(CONDITION) assert(CONDITION, #CONDITION, __FILE__, __LINE__);
#define REPORT            assert.report();
#define TOTAL_ASSERTIONS  assert.get_total_number_of_assertions()
#define FAILED_ASSERTIONS assert.get_number_of_failed_assertions()

class Assert {
public:
    bool operator()(const bool condition,
                    const char *const condition_text,
                    const char *const file_name,
                    const int line_number);

    size_t get_total_number_of_assertions() const;
    size_t get_number_of_failed_assertions() const;
    void report() const;

private:
    size_t m_total_assertions;
    size_t m_failed_assertions;
};

// Can't think of anything better than a global.
extern Assert assert;

#endif  // ASSERT_H

assert.cpp:

#include "assert.h"
#include <iostream>

bool Assert::operator()(const bool condition,
                        const char *const condition_text,
                        const char *const file_name,
                        const int line_number)
{
    if (!condition)
    {
        m_failed_assertions++;
        std::cerr << "'" << condition_text << "' is not true in file "
                  << "'" << file_name << "' at line " << line_number << "."
                  << std::endl;
    }

    m_total_assertions++;
    return condition;
}

size_t Assert::get_number_of_failed_assertions() const
{
    return m_failed_assertions;
}

size_t Assert::get_total_number_of_assertions() const
{
    return m_total_assertions;
}

void Assert::report() const
{
    std::cout << "[TOTAL ASSERTIONS: "
              << m_total_assertions
              << ", FAILED ASSERTIONS: "
              << m_failed_assertions
              << ", PASS RATIO: ";

    if (m_total_assertions == 0)
    {
        std::cout << "N/A";
    }
    else
    {
        std::cout << ((float) m_failed_assertions) / m_total_assertions;
    }

    std::cout << "]" << std::endl;
}

Assert assert;

main.cpp:

#include "assert.h"

static bool shit()
{
    return true;
}

int main(int argc, char** argv) {
    ASSERT(3 == 3)
    ASSERT(4 == 2)
    ASSERT(2 != 1)
    ASSERT(shit() && false)
    ASSERT(4 & 0)
    REPORT
    std::cout << TOTAL_ASSERTIONS << std::endl;
    std::cout << FAILED_ASSERTIONS << std::endl;
    return 0;
}

So, what should I fix in order to get to more idiomatic use of the language?

I don't see anything wrong with your code as it is: formatting is consistent and tidy, functions do what they need to do and no more, no apparent bugs.

One nit: your header file includes <iostream> but doesn't need it - you should remove that.

Having said that, "polluting" the global namespace with an object named assert isn't a good idea: assert is defined as a macro in <cassert>, you'll get all sorts of problems if your users include that, directly or indirectly. (Having a file called assert.h isn't a great idea either, that's the name of the C standard library assert header.)

Also your ASSERT macro doesn't do what the standard assert macro does (calls std::abort if the condition doesn't check), so you're likely to confuse people.

While the ASSERT macro itself is necessary to pull in the context information (AFAIK there's no other way to do that), the other three are a bit gratuitous and would benefit from being plain functions. Don't use macros when they provide no real benefit.

Here's a sketch about how you could clean that up a bit and avoid these issues (I'm not very good with names, so YMMV):

In a header, declare four plain functions inside a namespace:

bool check(bool, char const*, char const*, int): same as your Assert::operator() function
void report() (or possibly void report(std::ostream&) for a bit more flexibility)
int total_checks() and int failed_checks().

Also in that header define the CHECK function-like macro as you did your ASSERT, but using Testing::check instead.
In an implementation file, inside the namespace:

Define two static ints to store the total and failed counts
Define all the functions you declared.

This avoids the name clash, and "hides" the global state (the state is still global, so you still need to worry about thread-safety and such, it is just not as visible anymore).

Note: I don't think there's anything fundamentally wrong with using a class rather than plain functions in a namespace, and a class approach would be better if you ever intend to have more than one (e.g. one "test counter" per thread). But as it is, just plain old functions and static data seems enough.

Sample incomplete implementation:

testing.h

#pragma once

#define CHECK(x) Testing::check((x), #x, __FILE__, __LINE__)

namespace Testing
{
bool check(bool, char const *, char const *, int);
int total();
int failed();
}

testing.cpp

#include "testing.h"

#include <iostream>

namespace Testing
{
static int total_checks;
static int failed_checks;

bool check(bool ok, char const *what, char const *file, int line)
{
  total_checks++;
  if (!ok) {
    failed_checks++;
    std::cerr << "Failure of " << what << " at " << file << ":" << line
              << std::endl;
  }
  return ok;
}

int total() { return total_checks; }
int failed() { return failed_checks; }
}

Use case:

#include "testing.h"

#include <iostream>

int main()
{
  CHECK(1==1);
  CHECK(1==0);
  std::cout << Testing::failed() << " / " << Testing::total() << std::endl;
}


naive suffix tree construction in Python I'm not certain if my implementation is correct, it appears to be, but looking for any feedback as far as readability and efficiency goes. Obviously using dictionaries isn't particularly efficient space wise, but how else could you achieve O(1) lookup?

import os

def get_suffixes(string):
    return [string[i:] + "$" for i in range(len(string) + 1)]

def shared_prefix(string_a, string_b):
    return os.path.commonprefix([string_a, string_b])

def construct_suffix_tree(string):
    tree = {}
    for i in range(len(string) + 1):
        suffix = string[i:] + "$"
        insert_suffix(suffix, tree)
    return tree

def insert_suffix(string, suffix_tree):
    if len(suffix_tree) == 0:
        suffix_tree[string] = []
        return suffix_tree

    found_match = False
    for key in list(suffix_tree):
        prefix = shared_prefix(string, key)
        n = len(prefix)
        if len(prefix) > 0:
            found_match = True
            key_suffix = key[n:]
            string_suffix = string[n:]
            del suffix_tree[key]
            suffix_tree[prefix] = [key_suffix, string_suffix]

    if not found_match:
        suffix_tree[string] = []
    return suffix_tree

my_string = "banana"
print(construct_suffix_tree(my_string))

It's pretty readable, and all comments are pretty minor ones.

You can probably squeeze out a little performance increase, but not much. One thing you could do is to skip setting variables like n = len(prefix) (you also don't use n in the following line).

You could also consider collections.deque instead of list where relevant, which can sometimes give you a little extra speed. Or if you have the option to use list comprehension, do so.

And doing if suffix_tree: instead of if len(suffix_tree) == 0: is also faster; ca 30 ns vs 85 ns on my machine (Python 3.8.1).

On the time and space complexity, you have to make a judgement call if time or space is more important; only other way you could have O(1) lookup would be with a set.

Finally, you don't need to keep track of found_match---you can just use a for-else with a break-statement.

PS. You should be wary of mutating objects you're iterating over.


Revised media ingest script link to original question: Simple media ingest script

As per answers I have refactored my code and would like to get feedback on the current structure.

The purpose of this tool is to allow someone to copy media from cards (SD cards, CFast, etc) using rsync and to also allow someone to complete incomplete transfer that they have initiated.

I've chosen to use prompts because it's more user friendly for the environment the script will be used in. The intended users are not command line tool friendly.

I'm not very familiar with bash and am quite out of practice with code in general so I would like to check behind myself. I received excellent feedback in the previous post and have addressed several of the points raised.

#!/usr/local/bin/bash
set -e
set -u

# prompt user for input
prompt_user(){
        while [[ -z "${!2:-}" ]]
            do
            read -r -p "$1" "$2"
        done
}

# prompt user for additional rsync options
add_rsync_options(){
    while true;
    do
        read -r -p "Add rsync options? [y/n] " add_options
        case $add_options in
            [Yy]* )
                read -r -p $'Enter additional rsync options:\n' rsync_options
                break;;
            [Nn]* )
                break;;
            *) echo "Please enter y or no!"
        esac
 done
}

# make target directory for transfer
make_directory(){
    echo  $'Follow the prompt to create a project directory.\n'

    prompt_user $'Path of target directory?\n' target_directory
    prompt_user $'Brand Prefix?\n' brand_prefix
    prompt_user $'Project Name?\n' project_name
    prompt_user $'Media Type?\n' media_type
    prompt_user $'Location?\n' location
    prompt_user $'Employee?\n' employee

    destination_path=${target_directory}/$(date +'%Y%m%d')_${brand_prefix}_${project_name}_${media_type}_${location}_${employee}

    echo "Creating directory: ${destination_path}"

    mkdir -p "${destination_path}" "${destination_path}/logs"
}

# run rsync command
run_rsync(){
    echo $'Follow the prompt to complete the rsync command.\n'

    prompt_user $'Path to source media?\n' source_path

# if partial ingest indicate pre-existing target directory
    if [[ "$option" == "2" ]]; then
        prompt_user $'Target directory?\n' target_directory
        destination_path=$target_directory
    fi

    add_rsync_options

    echo $'Running rsync command:\n'

    rsync -r --info=progress2 --log-file="${destination_path}/logs/$(date +'%Y%m%d')_transfer_log.txt" ${rsync_options:-} "${source_path}/" "${destination_path}"

}

# clear terminal for readability
clear

# start
while true
do
# read user input and run appropriate functions
        read -r -p $'Enter [1] to start an ingest or [2] to complete a partial ingest.\n' option
        case $option in
            1 )
                make_directory
                run_rsync
                break;;
          2 )
                run_rsync
                break;;
          * )
                echo $'Please enter a valid option!\n';;
        esac
done

Error handling is at a minimum still I believe and I'm not too sure what would even be relevant to handle past what's implemented. I'm not very experienced in that regard so resources are appreciated!

Thanks!

What is $option in run_rsync?

option is read in the global scope to decide if user wants a full ingest or continue a partial ingest. In that loop that reads the prompt, the meaning of option is understandable.

Referring to $option in run_rsync is just too far away. It's not clear anymore where it comes from and what it means. It would be better if the prompt loop passed the decision to run_rsync as a parameter, to make it perfectly clear. For example:

while true
do
    read -r -p $'Enter [1] to start an ingest or [2] to complete a partial ingest.\n' option
    case $option in
        1 )
            make_directory
            run_rsync full
            break;;
        2 )
            run_rsync partial
            break;;
        * )
            echo $'Please enter a valid option!\n';;
    esac
done

And then in run_rsync:

run_rsync() {
    local ingestType=$1

    echo $'Follow the prompt to complete the rsync command.\n'

    prompt_user $'Path to source media?\n' source_path

    if [[ "$ingestType" = "partial" ]]; then
        prompt_user $'Target directory?\n' target_directory
        destination_path=$target_directory
    fi

    # ...

Inconsistent terminology

In make_directory, the term "target directory" refers to the base directory in which a timestamped sub-directory will be created, with brand name, project name, and so on, also in the name.

In run_rsync, in case of partial ingest the user is prompted for "target directory", but here it will mean the full destination path. If would be better to name it as such.

Inconsistent formatting

The code uses inconsistent indentation.
For example the content of prompt_user is over-indented, compared to other functions.
The body of the while loop in the global scope is over-indented,
and also haphazardly indented.
It would be easier to read if indenting was consistent throughout.

Unnecessary comments

Most comments in the program state the obvious.
They are just noise,
and it would be better to remove them.


Count Sort Algorithm I'm new to programming, but want to build up a good habit of code review so that I can develop best practice. It's been couple of weeks I'm coding in C and my knowledge, so far, is data types, conditions, arrays, loop and, very minimal, pointers.

I'm creating multiple search and sorting algorithms - not only to understand them better, but also solidifying my understanding of concepts.

Please review and provide your advice so that I can get better:

#include <stdio.h>
#include <cs50.h>

int main(void)
{
printf("How many numbers do you want to enter? ");
int len = get_int();

int input[len]; // creating an array to store values

for (int i=0; i<len; i++)
{
    printf("Enter %ith number: ", i+1);
    input[i] = get_int();
}

// determining minimum and maximum value
int min = input[0];
int max = input[0];

for (int i=1; i<len; i++)
{
    if (input[i]>max)
    {
        max = input[i];
    }
    if (input[i]<min)
    {
        min = input[i];
    }
}

// creating an array for all the digits needed
int digits[max+1];

for (int i=0; i<max+1; i++)
{
    digits[i] = 0; //giving all the elements of the array a default value
}

// counting numbers
for (int i=0; i<len; i++)
{
    digits[input[i]] = digits[input[i]] + 1; // increasing the value of default digit element (default is 0) by 1 each time
}

printf("Minimum = %i\n", min);
printf("Maximum = %i\n", max);
printf("Inputed integers are: ");

for (int i=0; i<len; i++)
{
    printf("%i ", input[i]);
}

printf("\n");

// count sort algorithm
int j=0;
for (int i=0; i<max+1; i++)
{
    if (digits[i] == 0) // if the default value is zero, the value is disregarded, because it's not used
    {
        //
    }
    else
    {
        do
        {
            input[j] = i; // updating the element's value with i
            j++; // updating j's value so that the next integer is stored in an updated address
            digits[i] = digits[i] - 1; // decreasing the count in digits by 1, tells that it's values are being accounted for
        }
        while(digits[i] != 0); //when digits hit 0, it becomes useless again and breaks the loop
    }
}

printf("The sorted numbers are: ");
for (int i=0; i<len; i++)
{
    printf("%i ", input[i]);
}
printf("\n");
}

You haven't provided cs50.h, so I have to guess its contents. I guess that it contains

int get_int(); // probably { int k; scanf("%d", &k); return k; }

for this review, since this is the only unknown function in your code.

Either way, let us get started. While completely up to you, the innards of a function are usually indented, just like they are for a conditional construct. So all of your main would be indented:

int main(void)
{
    printf("How many numbers do you want to enter? ");
    int len = get_int();

    int input[len]; // creating an array to store values

    /* .. */

While your program only uses a single large main, where it doesn't seem to matter, it's tremendously helpful if your program consists of multiple functions. for example, you could have split your functionality into several, reusable functions:

void read_n_numbers(int * destination, size_t n);
void count_digits(const int * source, size_t n, int min, int max, int * digits);
void print_digit_count(const int * digits, int min, int max);

By the way, those additional min and max parameters? They already show a flaw with your algorithm. But more on that later.

Instead, let us focus on some things to be wary of.

Know what variable length arrays are

int input[len]; // creating an array to store values

This lines uses two C99 features: C++-style comments and variable length arrays. The comments don't pose any problem. The VLA on the other hand may. VLAs are often stored in an area close to your other variables, like len above. That area is rather limited in size, which can lead to problems if len is too large. Also, with C11, they got optional. Your compiler might or might not support those.

They're sufficient for a small toy program, but you have to make sure that things don't get too large.

Give a variable the correct type and make it constant if it shall not change

We're still just in the first few lines of your program. But did you check whether len is negative? Let's say the user inputs -1. We end up with

int input[-1];

Uh oh… If this was a program you want to use in a productive environment, you would check the sign of len, and, after the discussion above, also check that tit doesn't get too large (or dismiss VLA).

We can provide a small little helper to make sure that we get only positive integers:

size_t get_positive_int() {
    int k = get_int();
    while(k <= 0) {
        printf("Number must be positive, please try again: ");
        k = get_int();
    }
    return k;
}

Now we have an ease of mind when we get our len:

const size_t len = get_positive_int();

What? Why is len now size_t? Why is it const? Well, it's a size_t because that is the proper data type to measure the size of something. You can think of it as a unsigned long, but it's size depends on your system.

And why is len now const? Because it prevents accidental errors like this:

for( int i = 0; i < len++; ++i)
//                     ^^
//                  whoops

It seems unlikely that you accidentally change len. But why take a risk? const puts an ease on your mind.

Make exclusive branches exclusive

We're still following your function top-to-bottom. And while your for finding the minimum and maximum of the range works, it's not really friendly to our PC:

for (int i=1; i<len; i++)
{
    if (input[i]>max)
    {
        max = input[i];
    }
    if (input[i]<min)
    {
        min = input[i];
    }
}

You start with max = min. However, throughout your program, the following property will hold: min <= max. So if the input[i] is greater then max, it will certainly not be lesser than min. We can reflect that with an else:

for (int i=1; i<len; i++)
{
    if (input[i] > max)
    {
        max = input[i];
    }
    else if (input[i] < min)  // << here
    {
        min = input[i];
    }
}

Now we only check whether input[i] is smaller than min if it wasn't aleady greater than max.

An error in disguise

Now we come to the part where your code is actually wrong. Sorry.

Let's say your user inputs the following numbers:

-3
-2
-1
0
1

What can go wrong with the following declaration?

// creating an array for all the digits needed
int digits[max+1];

You will have only memory for exactly two digits. That's not enough! You have to calculate the actual number of possible digits. Luckily, that is simple:

const int number_of_possible_digits = max - min + 1;

size_t digits[number_of_possible_digits]; // size_t because we're counting things again

// use `memset(digits, 0, sizeof(digits));` alternatively

for(int i = 0; i < number_of_possible_digits; ++i)
{
    digits[i] = 0;
}

"But wait", I here you say. "If I have now the following input, I would not have enough space for my digits:"

  10
  11
  12

"I would have only space for three digits, but in my assignment I use digits[input[i]]!" But we're not done yet. Keeping the example with negative numbers from above in mind, what goes wrong in your actual counting?

for (int i=0; i<len; i++)
{
    digits[input[i]] = digits[input[i]] + 1; // increasing the value of default digit element (default is 0) by 1 each time
}

You've probably guessed it. With our negative values, we end up with digits[-3]. This doesn't work. We have to use another technique for our access. Luckily, that's easy again:

for (int i=0; i<len; i++)
{
    digits[input[i] - min]++;
    // digits[ -3 - (-3)] = digits [-3 + 3] = digits[0]. All is well.
}

We have to adjust your algorithm slightly:

for (int i = 0, j = 0; i < number_of_possible_digits; i++)
{
    while (digits[i] != 0)
    {
        input[j] = i + min; // min now added here
        j++;
        digits[i]--;
    }
}

All we had to do is to add min to our index in our digits. Let's check that with an example:

-5
-2
1
1
1
1
1
1
1
1
10

The number of possible digits is 16 (10 - (-5) +1). After counting them, we end up with a digits array that looks like this:

{1,0,0,1,0,0,8,0,0,0,0,0,0,0,0,1}

Does input[0] get the correct value?

input[0] = 0 + -5

Yes it does.

To simplify a condition/loop

Allright, I changed a lot here, I have to admit. But the most important change is input[j] = i + min. What other changes did I do?

I've reduces j's scope. It is only available in the loop, so that you don't accidentally use it at some other point
I've removed all unecessary if and else.

I will show how I did the latter, so it gets easier to understand. First, if we do nothing in an if, but everything in an else, it makes sense to exchange those, e.g. go from

if (digits[i] == 0)
{
}
else
{
    // other stuff
}

to

if (digits[i] != 0)
{
    // other stuff
}

Next, if we look at our do-while, we will see that the conditions are the same:

if (digits[i] != 0)
{
    do
    {
        // other stuff
    }
    while(digits[i] != 0);
}

This means we can change it to a while loop, since the condition olds true. That's guaranteed by the if:

if (digits[i] != 0)
{
    while(digits[i])
    {
        // other stuff
    }
}

Now we remember that a while loop will not even get entered once, if the condition doesn't hold. So we can get rid of the if and we end up with simply

while(digits[i])
{
    // other stuff
}

Don't repeat yourself

You print the sorted number twice. And maybe you want to print the digits at some point, too. Create a function for that:

void print_numbers(const int * source, size_t length) {
    for (size_t i = 0; i<length; i++)
    {
        printf("%i ", source[i]);
    }
}

Keep in mind that that function won't work with digits if they are a size_t array.

To put things together

So, was your algorithm correct? For positive numbers, the answer is yes. But as soon as the user is allowed to input negative numbers, the answer was "no". However, the greatest weak point of your program is that it is a single large function. You don't need to split it into too small functions, but the actual counting algorithm kind of gets lost in the noise of the rest.

So for a future exercise, try to write

void counting_sort_inplace(int * values, size_t length){
    // ...
}

so that

counting_sort_inplace(input, len);

does exactly what your second part of the main did before.


Representing a combatant's equipment using the decorator pattern Is this a valid implementation of Decorator Pattern? Are there any possible improvements? Also, it is possible to remove decorations in a simple way?

 public interface Enemy {
    void fight();

    String shoutIdentity();
}

public class UnarmedEnemy implements Enemy {

    @Override
    public void fight() {
        System.out.println("I'm fighting with you!");
    }

    @Override
    public String shoutIdentity() {
        return "I'm your enemy! ";
    }
}

public class EnemyDecorator implements Enemy {

    private Enemy enemy;

    public EnemyDecorator(Enemy enemy) {
        this.enemy = enemy;
    }

    @Override
    public void fight() {
        enemy.fight();
    }

    @Override
    public String shoutIdentity() {
        return enemy.shoutIdentity();
    }
}

public class BowDecorator extends EnemyDecorator {

    public BowDecorator(Enemy enemy) {
        super(enemy);
    }

    @Override
    public void fight() {
        super.fight();
    }

    @Override
    public String shoutIdentity() {
        return super.shoutIdentity() + "I have a bow! ";
    }
}

public class ShieldDecorator extends EnemyDecorator {

    public ShieldDecorator(Enemy enemy) {
        super(enemy);
    }

    @Override
    public void fight() {
        super.fight();
    }

    @Override
    public String shoutIdentity() {
        return super.shoutIdentity() + "I have a shield! ";
    }
}

public class SwordDecorator extends EnemyDecorator {

    public SwordDecorator(Enemy enemy) {
        super(enemy);
    }

    @Override
    public void fight() {
        super.fight();
    }

    @Override
    public String shoutIdentity() {
        return super.shoutIdentity() + "I have a sword! ";
    }
}

public class HorseDecorator extends EnemyDecorator {

    public HorseDecorator(Enemy enemy) {
        super(enemy);
    }

    @Override
    public void fight() {
        System.out.println("I'm NOT fighting with you!");
    }

    @Override
    public String shoutIdentity() {
        return super.shoutIdentity() + "I have a horse! I'm fleeing! ";
    }
}

public class App {
    public static void main(String[] args) {
        Enemy enemy = new UnarmedEnemy();
        System.out.println(enemy.shoutIdentity());
        enemy.fight();
        System.out.println("\nYour enemy found a shield..\n");
        enemy = new ShieldDecorator(enemy);
        System.out.println(enemy.shoutIdentity());
        enemy.fight();
        System.out.println("\nYour enemy found a bow..\n");
        enemy = new  BowDecorator(enemy);
        System.out.println(enemy.shoutIdentity());
        enemy.fight();
        System.out.println("\nYour enemy found a sword..\n");
        enemy = new SwordDecorator(enemy);
        System.out.println(enemy.shoutIdentity());
        enemy.fight();
        System.out.println("\nYour enemy found a horse..\n");
        enemy =  new HorseDecorator(enemy);
        System.out.println(enemy.shoutIdentity());
        enemy.fight();
    }
}

Implementation is OK, note: I will add method which adds new behaviour dynamically, for example:

public class ShieldDecorator extends EnemyDecorator {

    ...
    private String addShield() {
        return "I have a shield! ";
    }

    @Override
    public String shoutIdentity() {
        return super.shoutIdentity() + addShield();
    }
}


Implementing StrStr() This is a naive implementation - I know that there are algorithms like KMP

However I was trying to implement it as best as I can.

string test = "giladdarmonwhatareyoudoing";
int index = StrStr(test, "are");

//Returns an index to the first occurrence of str2 in str1,
//or a -1 pointer if str2 is not part of str1.

public int StrStr(string test, string strToFind)
{
    for (int i = 0; i < test.Length; i++)
    {
        if (test[i] == strToFind[0])
        {
            int j;
            for (j = 0; j < strToFind.Length; j++)
            {
                if (test[i + j] != strToFind[j])
                {
                    break;
                }
            }
            if (j == strToFind.Length)
            {
                return i;
            }
        }
    }
    return -1;
}

Algorithm

your implementation has some serious bugs and unexpected behaviour.

If passing String.Empty as second parameter, it throws an IndexOutOfRangeException
If passing the unique ending part of the first parameter appended by at least one character to the second parameter, it throws an IndexOutOfRangeException

string test = "giladdarmonwhatareyoudoing";
int index = StrStr(test, "ng1");

If you are passing null as either first or second parameter a NullReferenceException is thrown. Here it would be better to throw an ArgumentNullException by using a guard clause.

if (test == null) { throw new ArgumentNullException("test"); }
if (strToFind == null) { throw new ArgumentNullException("strToFind"); }

your implementation could be improved by

checking if strToFind.Length > test.Length
checking if test.Length - i > strToFind.Length

Naming

You shouldn't use hungarian notation. Consider to rename strToFind to searchForm or searchArgument.

Refactoring

After extracting the inner loop to a separate method, removing the now unneeded if (test[i] == strToFind[0]) and implementing the above we will get

public int StrStr(string value, string searchArgument)
{
    if (value == null) { throw new ArgumentNullException("value"); }
    if (searchArgument == null) { throw new ArgumentNullException("searchArgument"); }
    if (searchArgument.Length == 0) { return 0; }

    int searchLength = searchArgument.Length;
    int length = value.Length;

    if (searchLength > length) { return -1; }

    for (int i = 0; i < length; i++)
    {
        if (length - i < searchLength) { return -1; }

        if (IsMatchAtIndex(value, searchArgument, i)) { return i; }
    }
    return -1;
}

private bool IsMatchAtIndex(String value, String searchArgument, int startIndex)
{
    for (int j = 0; j < searchArgument.Length; j++)
    {
        if (value[startIndex + j] != searchArgument[j])
        {
            return false;
        }
    }
    return true;
}

I prefer the above, but you could also add

if (length - i < searchLength) { return -1; }

inverted as condition to the for loop like

for (int i = 0; i < length && length - i >= searchLength; i++)
{
    if (IsMatchAtIndex(value, searchArgument, i)) { return i; }
}


Determine if an array is the reverse of a second array I tried to do it by using ArrayUtils.reverse() but I couldn't figure out how to compare this with the other array. So I wrote this instead:

public class Main {

  public static void main(String[] args) {

int[] array = new int[] {1, 2, 3, 4, 5, 6};

int[] tempArray = new int[] {6, 5, 4, 3, 2, 1};

boolean result = true;

for (int i = 0; i < array.length / 2; i++) {
  int temp = array[i];
  array[i] = array[array.length - i - 1];
  array[array.length - i - 1] = temp;
}

for (int i = 0; i < array.length; i++) {
  if (array[i] != tempArray[i]) {
    System.out.println("nope,not reversed");
    result = false;
    break;
  }
}

if (result) {
  System.out.println("yep,reversed");
}
  }
}

Is there a simpler solution? I'm skeptic because I think breaking the loop and setting a boolean value is a bit unnecessary.

The first comment, is that it would be preferable to factor this into a dedicated method, like areReversed(int[] array1, int[] array2). It would clarify the intention of the code. And, as it turns out, this will point out some improvements about the method itself.

It is currently modifying in-place the first array, in order to reverse it. If you now consider the code to be in a method, you could have

int[] array1 = new int[] { 1, 2, 3, 4, 5, 6 };
int[] array2 = new int[] { 6, 5, 4, 3, 2, 1 };
areReversed(array1, array2); // OK, prints that they are reversed versions
// some code
areReversed(array1, array2); // What? Now it prints that they aren't reversed?!

Calling the method a second time with the same arguments would not have the same result. This would be very surprising, especially so that one doesn't expect that method to behave in such a way. It is always preferable to adhere to the principle of least astonishment.
We want to know whether the two arrays are reversed, and we may want to do more than printing a String if it is the case. Make the method return a boolean that the calling code can use.

The insight is that you do not need to reverse an array, and determine whether the two are equal, for this. First of all, if they don't have the same length, there is no need to do anything, it is already known for sure that the result is false. Then, you can directly compare the arrays, by traversing one in a given direction and the other one in the opposite direction.

B   A   C            B   A   C            B   A   C
|                        |                        |
----x----                x                ----x----
        |                |                |
C   A   B            C   A   B            C   A   B

 Step 1               Step 2                Step 3

The upper part goes from left to right, and it compares elements with the lower part going from right to left. If there's a mismatch at any point, we know the arrays are not reverse of each other. And since we know they have the same length, all element of both arrays will have been considered.

A simple code to do this would be

public boolean areReversed(int[] array1, int[] array2) {
    if (array1.length != array2.length) {
        return false;
    }
    int length = array1.length;
    for (int i = 0; i < length; i++) {
        if (array1[i] != array2[length - i - 1]) {
            return false;
        }
    }
    return true;
}

That you can later use with:

if (areReversed(array, tempArray)) {
    System.out.println("yep,reversed");
} else {
    System.out.println("nope,not reversed");
}

You'll note that this implementation somewhat takes care of the concern you had with the code: "breaking the loop and setting a boolean value". In fact, there is no boolean variable: we return directly from the method once we know for sure what to return. The return inside the loop is really for optimization purposes; once a mismatch is encountered, there is no need to process further the arrays, since it is known that the result can only be false at that point. This is also what the initial break was doing.

Small final note. Instead of

int[] array = new int[] { 1, 2, 3, 4, 5, 6 };
int[] tempArray = new int[] { 6, 5, 4, 3, 2, 1 };

you could just have

int[] array = { 1, 2, 3, 4, 5, 6 };
int[] tempArray = { 6, 5, 4, 3, 2, 1 };

which is a bit more concise, and makes the array declaration a tad more readable.


Rspec test to confirm rejection of four invalid dates require_relative '../price_entry_class.rb'
require_relative '../exceptions.rb'
require 'spec_helper'

describe PriceEntry do
    it "throws expection on invalid date format" do
        expect { described_class.date_from_input_to_storage("20010101") }.to raise_error(InvalidInputData)
        expect { described_class.date_from_input_to_storage("zażółć") }.to raise_error(InvalidInputData)
        expect { described_class.date_from_input_to_storage(nil) }.to raise_error(InvalidInputData)
        expect { described_class.date_from_input_to_storage(-19_199) }.to raise_error(InvalidInputData)
    end
end

Is it OK to have multiple expectations in a single Rspec test? rubocop-rspec claims that it would be preferable to separate this test into 4:

spec/price_entry_class_spec.rb:14:2: C: Example has too many expectations [4/1].

Claim that it is always better to have single assert in every unit test is quite surprising for me. Is there a good reason to split test like this into four separate ones?

This test is testing only one thing at a time (is obviously malformed input resulting in exceptions).

I can imagine rewriting it to

["20010101", "zażółć", nil, -19_199].each do |malformed_input|
    expect { described_class.date_from_input_to_storage(malformed_input) }.to raise_error(InvalidInputData)
end

but I am not convinced that it is improving readability, maintainability or anything else.

The reason is that if this test fails, you'll have to separately determine which of the 4 sub-expectations has failed.

Depending on the output of your test reporter, this may or may not be a practical problem.

  This test is testing only one thing at a time (is obviously malformed input resulting in exceptions).

Actually, you're testing:

invalid digits raise an error
non-date characters raise an error
nil raises an error
negative numbers raise an error

That said, if you're essentially doing manual fuzzing and don't care about those distinctions -- though I'd say they seem useful in this particular case -- then I don't see an issue with this.

Testing rules and best practices are helpful, but don't get too bogged down in them.  If you know your own testing goals, and understand why it might be ok to depart from a best practice, by all means do so.


Finding distance between vectors of matrices So here is the problem:

Given 2D numpy arrays 'a' and 'b' of sizes  n×m  and  k×m
respectively and one natural number 'p'. You need to find the
distance(Euclidean) of the rows of the matrices 'a' and 'b'. Fill the
results in the  k×n  matrix. Calculate the distance with the following
formula
$$
D(x, y) = \left( \sum _{i=1} ^{m}
\left| x_i - y_i \right|^p
\right) ^{1/p}
; x,y \in R^m
$$
(try to prove that this is a distance). Extra points for writing
without a loop.

And here is my solution:
import numpy as np

def dist_mat(a, b, p):
    result = []
    print(result)
    for vector in b:
        matrix = a - vector
        print(matrix)
        result.append(list(((matrix ** p).sum(axis=1))**(1/p)))
    return np.array(result)

a = np.array([[1, 1],
              [0, 1],
              [1, 3],
              [4, 5]])
b = np.array([[1, 1],
              [-1, 0]])
p = 2
print(dist_mat(a, b, p))

I'm not sure about using Python list and then converting it into np.array, is there a better way?

This is an old enough question that shooting for the extra points is hopefully not going to step on anyone's toes. So let me be the broadcast guy, i.e. this is a loop-free version.
It is perhaps better to first read either the numpy documentation on broadcasting, or a related answer of mine.
We start with solving the one-column case.
In this case the matrices are of size n x 1 and k x 1. We need to turn these into a matrix of size k x n.
Well, to get there by broadcasting, we need to take the transpose of one of the vectors. The problem calls for the first one to be transposed. Thus we have the matrix a.T of size 1 x n and b of size k x 1.
Then the solution is just
    # shape is (k, n)
    (np.abs(a.T - b) ** p) ** (1/p).

The case with multiple columns
The matrices are of size n x m and k x m. To be able to reuse the previous idea, we have to turn these into arrays of shape 1 x n x m and k x 1 x m. This can be done using np.expand_dims:
    # shape is (k, n, m)
    np.expand_dims(a, 0) - np.expand_dims(b, 1)

Looks good. All that remains is to take the absolute value, then the pth power, then sum up along the last dimension, lastly take pth root.
    # shape is (k, n)
    (np.abs(np.expand_dims(a, 0) - np.expand_dims(b, 1))**p).sum(axis=-1)**(1/p)


Solving for a seed value in R I'm trying to reproduce a gbm model which was estimated without a set.seed value. To do so I need to determine what seed was used, which I can figure out based on one of the summary metrics from the estimated model (as shown below).

require(MatchIt)
require(gbm)
data("lalonde")
i <- 1
while(!(tmp$rel.inf[1] == 82.3429390)){
          gps <- gbm(treat ~ age + educ + nodegree + re74 + re75,
                     distribution = "bernoulli",
                     data = lalonde, n.trees = 100,
                     interaction.depth = 4,
                     train.fraction = 0.8, shrinkage=0.0005,
                     set.seed(i))
          tmp <- summary(gps, plotit=F)
          cat(i,"\n")
          i <- i + 1
}

I think it would be very helpful both for this specific use case and for general future reference to know of any more efficient way of carrying this out. A multicore solution might be a good way to go; I'm researching that myself now. Or perhaps there's a way to improve it by using apply?

It seems that you are looping through seeds to find the one that causes a randomized procedure's output to match the output from a previous run.

If you had set the random seed immediately before running the randomized procedure and have simply forgotten the seed you used, then this in theory would work; all you need to do is loop through the billion or so possible input seeds until one matches. There's no real way to speed up the process (beyond parallelizing, which would be easy because the problem is embarrassingly parallel). apply is just a wrapper on a loop, so that would not speed up the process.

Unfortunately, more likely than not you did not set the random seed immediately before running the code. Therefore you would really need to test all the internal states of the pseudorandom number generator (PRNG) that you used to find the one that matches the results. Unfortunately there are intractably many internal states; for instance, the most popular implementation of the Mersenne Twister, which you are likely using, has a period of 2^19937 - 1, meaning it has at least that many possible internal states. Clearly it's impractical to test this many states, so it's probably hopeless to try to match an exact PRNG state if you hadn't set the seed immediately prior to running your randomized procedure.


Model for different kinds of print jobs I am trying to calculate the print cost base on different paper size, single side or double side. So here is the detail:

A4, job type single sided:
*15 cents - black and white page
*25 cents - color page.

A4, job type double sided:
*10 cents - black and white page
*20 cents - color page.

Also need to support for other paper sizes will be added in the future.

And according to my design, developer can just create a A5 class for example to support other paper size, and add other condition in the factory class.

Could someone review my code and help me on whether I have to use interface instead of abstract class?

Here is my code:

PageBase:

public abstract class PageBase {
    abstract double GetCost(int total, int color, boolean isSingleSide);
    abstract void CalculateUnitPrice(boolean isSingleSide);
}

A4Page Class:

public class A4Page extends PageBase {
    public double blackAndWhitePrintUnitCost;
    public double colorPrintUniCost;

    @Override
    public double GetCost(int total, int color, boolean isSingleSide) {
        CalculateUnitPrice(isSingleSide);
        return color* colorPrintUniCost + (total-color)* blackAndWhitePrintUnitCost;
    }

    @Override
    public void CalculateUnitPrice(boolean isSingleSide) {
        if (isSingleSide) {
            this.blackAndWhitePrintUnitCost = 0.15;
            this.colorPrintUniCost = 0.25;
        }
        else {
            this.blackAndWhitePrintUnitCost = 0.10;
            this.colorPrintUniCost = 0.20;
        }
    }
}

PageFactory:

public class PageFactory {

    public PageBase GetPage(String pageType) {
        switch (pageType.toUpperCase()) {
            case "A4":
                return new A4Page();
            default:
                return new A4Page();
        }
    }
}

Main:

public class Main {
    public static void Main() {
        //read
        PageFactory pageFactory = new PageFactory();
        PageBase page = pageFactory.GetPage("A4");
        page.GetCost(0,0,false);
    }
}

Design

you should consider to define the properties of the page in factory, that's where it should be located.

public PageBase GetPage(String pageType) {
    switch (pageType.toUpperCase()) {
        case "A4": return new Page(0.15, 0.25, 0.10, 0.20);
}

Programming

 1. Page

you should not try to subclass the page since it is not required, the only difference between each page are the prices (so far). Therefore you don't need subclasses - it is just a page (not a base Page or anything else)

And even more you should could instanciate the class with all properties during construction (as shown above, dependency injection)

public class Page {

    private final double blackWhiteSinglePageUnitCost;
    private final double colorSinglePageUnitCost;
    private final double blackWhiteDoublePageUnitCost;
    private final double colorDoublePageUnitCost;

    public Page(double bwSingle, double colorSingle, double bwDouble, double colorDouble){
        blackWhiteSinglePageUnitCost= bwSingle;
        colorSinglePageUnitCost= bwDouble;
        blackWhiteDoublePageUnitCost= bwSingle;
        colorDoublePageUnitCost= colorDouble;
    }

what can be kept ist GetCost but should be renamed int getCost() but you can totally skip CalculateUnitPrice, see below

 2. PageType

if you use a certain format you should use Enums to define the Format of the sheet. that would free you from abusing Strings as datatypes

enum PageType {A4, A5, LETTER };

 3. bringing things together

when we use all these hints from above we have to adjust some more code, to make the programm run again

Page.class must be modified we can calculate the price on-the-fly because we already know anything required to calculate the price:

public double getCost(int total, int color, boolean isSingleSided){
    int bw = total - color;
    if(isSingleSided){
        return bw * blackWhiteSinglePageUnitCost + color * colorSinglePageUnitCost ;
    else{
        return bw * blackWhiteDoublePageUnitCost + color * colorDoublePageUnitCost ;
    }

}

PageFactorymust be modified - first the method signature must be java conform, namely getPage not GetPage. then you switch over the enum type instead of an String. That lets you handle invalid inputs.

public Page getPage(PageType pageType) {
    switch (pageType) {
        case PageType.A4:
            return ...;
        case PageType.A5:
            return ...;
        case PageType.LETTER:
            return ...;
    }
}

if you use this in action (in your main method ) it would be finally that appearance:

public class Main {
    public static void Main() {
        PageFactory pageFactory = new PageFactory();
        Page page = pageFactory.getPage(PageType.A4);
        double cost = page.getCost(0,0,false);
    }
}


Is it "legit" to use a Linq query in a razor view? I have a Linq query in my razor view like below. Its just getting contacts that belong in the group.

@using (var tabs = Html.Bootstrap().Begin(new Tabs("TabStripId")))
{
    foreach (var g in Model.Groups)
    {
        @tabs.Tab(g.GroupName)
    }

    foreach (var g in Model.Groups)
    {

        using (tabs.BeginPanel())
        {

            using (Html.Bootstrap().Begin(new Table().Striped()))
            {
               ///Table header here

               ///The line in question
               var m = Model.Contacts.Where(x => x.GroupId == g.Id);

               foreach (var item in m)
               {
                 /// Row output here
               }
            }
        }
    }
}

The code I have works just fine, but the question is: Is it good practice to be using Linq in a view like that? If not, how would that be handled in the model and/or controller?

This is not an ideal way to do things. The model passed to your view should only have what you need for the view.

There are a few different things going on here. One is searching for the information and two is displaying it in your view. The solution will depend on the context in which it is happening.

One option is that you can change your service to be searchable on GroupId. This way only the information you need is passed back.

If you get an entire object back from your service and just want to filter the contacts on that more complex model, you could create a repository that does that filtering or use a ViewModel.

The main thing here is that you never want to do this type of filtering in your view. Only send your view the data that it needs.


Find the lowest and second lowest values in an array of numbers Trying to find way to do this with only one pass through the array, so I won't run a sort on it and take arr[0..1]. Not sure how to make it look better:

def LowestSecondLowest(arr)
  lowest,second_lowest = nil,nil

  arr.each do |n|

    if second_lowest.nil?
      second_lowest=n
    elsif lowest.nil?
      if n>second_lowest
        lowest=second_lowest
        second_lowest=n
      elsif n<second_lowest
        lowest=n
      end
    elsif n<lowest
      second_lowest=lowest
      lowest=n
    elsif n>lowest && n<second_lowest
      second_lowest=n
    end
  end

  "#{lowest} #{second_lowest}"
end

Because Array lets you put repeated elements of same value, I believe that the two lowest elements in [1, 2, 1] are [1, 1], not [1, 2]. This is the very same behavior one would face if sorting the array and getting the two first elements.
Also I believe that in [1] there is a lowest and no 2nd lowest (also, the very same behavior if sorting and getting the first two).

With this in mind, I changed your code to obey these premises (and make it a bit more readable if I may say). Take a look.

def two_lowest arr
  # if arr has no elements, there is no answer
  # If arr has only one element, this is the lowest
  if arr.size < 2 then
    return arr.first, nil
  end

  lowest, second_lowest = nil, nil

  arr.each do |n|
    if lowest.nil? or n < lowest
      # if we have no lowest or we found an element lower than current lowest,
      # update our lowest and 2nd lowest
      second_lowest = lowest
      lowest = n
    elsif second_lowest.nil? or n < second_lowest
      # if we have no 2nd lowest or we found an element between lowest and 2nd
      # lowest, update our 2nd lowest
      second_lowest = n
    end
  end

  return lowest, second_lowest
end

If you want to test it

tests = [
  [],
  [1],
  [1, 1],
  [1 ,2],
  [1, 2, 1],
  [1, 2, 3],
  [1, 1, 2, 3],
  [3, 2, 1, 1],
]

for test in tests
  puts "In #{test}:"
  puts "#{two_lowest test}"
  puts
end


Nested if-else within a for-loop using np.concatenate to create batches for a neural network I am having a for-loop in python 3.5. Inside the loop, I have multiple if-else statements in which I append some information. I am using this piece of code to create batches for a neural network built in Tensorflow.

Here is my code:

for generated_lemma, classified_lemma, token_hidden, token_hidden_generator in zip(
        tokens_generated,
        tokens_classified,
        token_dnn_output,
        token_generator_output
):
    if generated_lemma.replace('#', '|') not in self.lemmas_word2vec_model:
        if classified_lemma.replace('#', '|') not in self.lemmas_word2vec_model:
            if classified_lemma == '<UNK>':
                x.append(
                    np.concatenate([
                        self.embedding_unknown_lemma,
                        self.embedding_unknown_token,
                        token_hidden,
                        token_hidden_generator
                    ]
                    )
                )
            else:
                x.append(
                    np.concatenate([
                        self.embedding_unknown_lemma,
                        self.embedding_unknown_lemma,
                        token_hidden,
                        token_hidden_generator
                    ]
                    )
                )
        else:
            x.append(
                np.concatenate([
                    self.embedding_unknown_lemma,
                    self.lemmas_word2vec_model[classified_lemma.replace('#', '|')],
                    token_hidden,
                    token_hidden_generator
                ]
                )
            )
    elif classified_lemma.replace('#', '|') not in self.lemmas_word2vec_model:
        if classified_lemma == '<UNK>':
            x.append(
                np.concatenate([
                    self.lemmas_word2vec_model[generated_lemma.replace('#', '|')],
                    self.embedding_unknown_token,
                    token_hidden,
                    token_hidden_generator
                ]
                )
            )
        else:
            x.append(
                np.concatenate([
                    self.lemmas_word2vec_model[generated_lemma.replace('#', '|')],
                    self.embedding_unknown_lemma,
                    token_hidden,
                    token_hidden_generator
                ]
                )
            )
    else:
        x.append(
            np.concatenate([
                self.lemmas_word2vec_model[generated_lemma.replace('#', '|')],
                self.lemmas_word2vec_model[classified_lemma.replace('#', '|')],
                token_hidden,
                token_hidden_generator
            ]
            )
        )

My code works fine, although it is a bit slow. Hence, I would like to know if there was a more "pythonic" way to achieve the same thing?

My aim is to improve both style and performance.

Your code is coupling independent ideas, this is making your code at least twice as long as you need it to be, as you have to duplicate your if statements.
And so I refactored your code in the following way:

Move x.append(np.concatenate(...)) out of the if statements. This is as you're needlessly duplicating this across them all.
Make the array passed to (1) outside the ifs, this is as you always add token_hidden and token_hidden_generator.
Replace once. generated_lemma = generated_lemma.replace('#', '|') and you don't have to call replace again.
Move all the generated_lemma code into a single if-else. Leading to:

generated_lemma = generated_lemma.replace('#', '|')
if generated_lemma in self.lemmas_word2vec_model:
    gen_lemma = self.lemmas_word2vec_model[generated_lemma]
else:
    gen_lemma = self.embedding_unknown_lemma

Cleanup the rest of the ifs.

This got me:

for generated_lemma, classified_lemma, token_hidden, token_hidden_generator in zip(
    tokens_generated,
    tokens_classified,
    token_dnn_output,
    token_generator_output
):
    generated_lemma = generated_lemma.replace('#', '|')
    if generated_lemma in self.lemmas_word2vec_model:
        gen_lemma = self.lemmas_word2vec_model[generated_lemma]
    else:
        gen_lemma = self.embedding_unknown_lemma

    classified_lemma = classified_lemma.replace('#', '|')
    if classified_lemma in self.lemmas_word2vec_model:
        class_lemma = self.lemmas_word2vec_model[classified_lemma]
    elif classified_lemma == '<UNK>':
        class_lemma = self.embedding_unknown_token
    else:
        class_lemma = self.embedding_unknown_lemma

    x.append(np.concatenate([
        gen_lemma,
        class_lemma,
        token_hidden,
        token_hidden_generator
    ]))

This may be faster, due to (3), however, I doubt it'd have any serious speed-ups. I don't really know any of the data types, and so can't comment on whether you need np.concatenate, or a faster way to add to x.