C++ Standard-Thread Threadpool I have written a small and simple ThreadPool class for rather simple multithreading applications.

The Threadpool class manages a vector with the actual threads.
The Thread struct is used to store the thread's function.
ThreadPoolElement keeps track of the thread and its state.

#pragma once

#include <vector>
#include <thread>
#include <mutex>

namespace frm { namespace util {

template <typename F, typename ... Args>
struct Thread {
    Thread(F&& f)
    : func(f) {
    }
    std::function<void(Args...)> func;
};

class Threadpool {

public:

    Threadpool(const size_t& capasity = std::thread::hardware_concurrency())
        : m_capacity(capasity) {
        m_threads.reserve(m_capacity);
    }

    virtual ~Threadpool() {
        for (auto& t : m_threads) {
            t.thread.join();
        }
    }

    template <typename Thread, typename ... Args>
    void getThread(Thread& t, Args... args) {

        while (m_threads.size() >= m_capacity) {
            cleanup();
        }

        std::lock_guard<std::mutex> lock(m_mutex);
        m_threads.emplace_back(ThreadPoolElement());
        ThreadPoolElement& elem = m_threads[m_threads.size() - 1];
        elem.thread = std::thread([&] (ThreadPoolElement* e, Thread& t, Args... args) {
            t.func(args...);
            e->is_done = true;
        }, &elem, t, args...);
    }

    size_t activeThreads() const {
        return m_threads.size();
    }

    size_t reservedThreads() const {
        return m_capacity;
    }

    void cleanup() {
        for (int i = 0; i < m_threads.size(); i++) {
            if (m_threads[i].is_done) {
                std::lock_guard<std::mutex> lock(m_mutex);
                m_threads[i].thread.join();
                m_threads.erase(m_threads.begin() + i--);
            }
        }
    }

private:

    struct ThreadPoolElement {
        std::thread thread;
        volatile bool is_done = false;
    };

    size_t m_capacity;
    std::mutex m_mutex;
    std::vector<ThreadPoolElement> m_threads;

};

} }

My questions are pretty simple:

Has this implementation any significant downsides that would make it a 'Please do not use this' implementation?
What are the most important parts I'm missing?

I found a few issues, here they are (in order in which they were found). The list may seem long, but this does not mean that you did a bad job. Instead, it means that the feedback is detailed ;)

If some issue starts with [major], it means that it would stop me from using your class.

[major] There is zero documentation.
The Thread struct is not really a thread. In fact, it just stores a single std::function member. Why did you name it like that?
The Threadpool has a virtual destructor. This is not necessary, because it does not make sense to inherit from it. I suggest you make it non-virtual.
The prefix get usually indicates that you return something from it. However, getThread does not return anything. Even though t is passed by reference, you do not change it. Maybe addTask is a better name.
[major] If the thread pool is full, getThread blocks until one of the threads is free. This is bad. In addition, you use an active loop to block, which means that waiting takes 100% of one CPU core. This is really bad. The whole point of threads and thread pools is that the user does not need to wait. I suggest that you copy or move the task (your std::function) into the pool and execute it as soon as some thread is free. This way, getThread can return immediately.
[major] The thread pool currently creates one std::thread for each task that is added. However, creating threads has some overhead and this will make a big difference if you have many small tasks. I suggest that you create m_capacity worker threads that grab the tasks from some container. This transforms your problem into a typical producer-consumer problem (adding a task is producing work, which is then consumed by the threads).
[major] You are using std::function<void(Args...)> to store a single task. This means that the thread pool does not support return values. A common pattern is that addTask returns a std::future which will receive the return value after some thread completed the task. Additionally, thread pool users can use the std::future to check whether the task completed and they can use it to wait for completion.
[major] The getThread function uses references of t. This means that problems will occur if t is destroyed while the thread is still running. This is bad, because t is passed by the user, so the pool has no control over its lifetime. The non-existing documentation would have been a good place to mention that t must not be destroyed before the task is finished :) Typically, you would std::move the task, which transfers the control to the pool.
If I understand m_mutex correctly, it guards m_threads against race conditions. This means that you want to lock the mutex whenever you use m_threads. Since reading from a variable while another thread modifies it is undefined behavior, you need to lock the mutex in some more places. There are some usages of m_thread or of its elements where the mutex is not locked (and I do not mean constructor/destructor).
The ThreadPoolElement has a volatile bool. Depending on compiler and platform, volatile may do what you think it does. However, you can not rely on that. You should use std::atomic<bool> instead.
Threadpool has a lower case p, but ThreadPoolElement has an upper case P.

I suggest that you refactor the Threadpool such that you do not need to create a new std::thread object for each task. Typically, a thread pool has a queue with pending tasks and a fixed number of worker threads. The worker threads grab tasks from the queue and complete them. This way, it is possible to add many tasks without the overhead of creating new threads.

You may want to look into the std::packaged_task and std::future classes, since they can simplify your implementation. You can probably get rid of the Thread and ThreadPoolElement classes.


hide/show options in selectbox I've got a code that is working in my environment.
Nevertheless it is pretty ugly.

This is what I am trying to do.
I get a selectbox from a side with a lot of city names.
I filter some names and push them into an array and hide them. The other ones are also pushed into an array, but shown.

Now I append two options to the end of the selectbox, one is displayed, the other one hidden.

When someone clicks the option all, the options from the array otherCities are shown and defaultCities are hidden. the option all is hidden as well, less is shown.

This behavior works the other way round, too.

How can I simplify my code? Any help is welcome.

var otherCities = new Array();
var defaultCities = new Array();
var selectBoxCity = jQuery("select[name='city']");

if(selectBoxCity.size() != 0) {
        selectBoxCity.prepend('<option value="" selected="selected">-- Please Select --</option>');
    otherCities = new Array();
    defaultCities = new Array();
    var cityArray = ['OE', 'MK', 'SI', 'BO', 'DO', 'BM', 'HER', 'GE', 'HH'];
    var options = jQuery("select[name='city'] option");
    options.each(function() {
        var o = jQuery(this);
        if (jQuery.inArray(o.attr("value"), cityArray) == -1) {
            otherCities.push(o.hide());
        } else {
            defaultCities.push(o.show());
        }
    });
    selectBoxCity.find("option").end().append("<option value='all'>+</option>");
    selectBoxCity.find("option").end().append("<option value='less'>-</option>");

}

var selectBoxCity = jQuery("select[name='city']");
selectBoxCity.find("option:first").attr("selected", true);

$("select[name='city'] option[value='less']").hide();

if(selectBoxCity.val() == "all") {
    jQuery.each(otherCities,function() {
        $(this).show();
    });
    jQuery.each(defaultCities,function() {
        $(this).hide();
    });
    $("select[name='city'] option[value='all']").hide();
    $("select[name='city'] option[value='less']").show();
}
if(selectBoxCity.val() == "less") {
    jQuery.each(otherCities,function() {
        $(this).hide();
    });
    jQuery.each(defaultCities,function() {
        $(this).show();
    });
    $("select[name='city'] option[value='all']").show();
    $("select[name='city'] option[value='less']").hide();
}

Here's something that seems to work OK (here's a demo)

jQuery(function ($) {
  var select   = $("select[name=city]"),
      defaults = select.children(".default"),
      others   = select.children(":not(.default)");

  others.remove();

  select.on("change", function (event) {
    var value = select.val();
    if(value === "more") {
      defaults.remove();
      select.append(others);
    } else if(value === "less") {
      others.remove();
      select.append(defaults);
    }
  });
});

Everything else is stored in the markup, like so:

<select name="city">
  <!-- default options -->
  <option class="default">A</option>
  <option class="default">B</option>
  <option class="default">C</option>
  <option value="more" class="default">Show more</option>

  <!-- expanded options -->
  <option>D</option>
  <option>E</option>
  <option>F</option>
  <option>G</option>
  <option>H</option>
  <option>I</option>
  <option value="less">Show less</option>
</select>

As I said in the comments, .hide and .show doesn't seem to have any effect on <option> elements. Neither does visibility: hidden. In either case, they all show up in the select dropdown. You can disable options, but then they're still there, just greyed out.
Hence, this code forcibly removes/appends the options.

Of course, it might make more sense to simply remove/append only the non-default options. To me it makes more sense that the default options are a subset of all the options, not an entirely different set of options.

Furthermore, it might be good to let the more/less options be added by JS, so clients that don't support JS don't see them.

Putting all that together, you get this.
Without JS, you'll just see a full list. With JS, you get the more/less options and all that jazz.


Generic NSMapTable replacement written in Swift This is my attempt at writing a generic NSMapTable with weak keys and strong values (after your feedback I'll be trying to write Strong-Key/Weak-Value and Weak-Key/Weak-Value variants to have the complete functionality of NSMapTable with generics). To my knowledge it works correctly.

public class WeakKeyDictionary<K: AnyObject, V where K: Hashable> {

    private var dict = Dictionary<HashableWeakBox<K>, V>()
    public var block: (V)->() = { _ in }

    public init() {}
    public init(dictionary: Dictionary<K, V>) {
        for (k, v) in dictionary {
            setValue(v, forKey: k)
        }
    }

    public subscript(key: K) -> V? {
        get { return valueForKey(key) }
        set { setValue(newValue, forKey: key) }
    }

    public func valueForKey(key: K) -> V? {
        return dict[HashableWeakBox(key)]
    }
    public func setValue(newValue: V?, forKey key: K) {
        let hashableBox = HashableWeakBox(key)

        if let value = newValue {
            let watcher = DeallocWatcher { [weak self] in
                if let v = self?.dict[hashableBox] { self?.block(v) }
                self?.dict[hashableBox] = nil
            }

            objc_setAssociatedObject(key, unsafeAddressOf(self), watcher, objc_AssociationPolicy(OBJC_ASSOCIATION_RETAIN_NONATOMIC))
            dict[hashableBox] = value
        }
        else {
            objc_setAssociatedObject(key, unsafeAddressOf(self), nil, 0)
            dict[hashableBox] = nil
        }
    }
    public func removeValueForKey(key: K) -> V? {
        objc_setAssociatedObject(key, unsafeAddressOf(self), nil, 0)
        return dict.removeValueForKey(HashableWeakBox(key))
    }

    public var count: Int { return dict.count }
    public var isEmpty: Bool { return dict.isEmpty }
    public var keyValues: [(K, V)] {
        var list = [(K, V)]()
        for (k, v) in dict { list.append((k.value!, v)) }
        return list
    }
    public var keys: [K] {
        var list = [K]()
        for k in dict.keys { list.append(k.value!) }
        return list
    }
    public var values: [V] {
        return Array(dict.values)
    }

    deinit {
        for box in dict.keys {
            objc_setAssociatedObject(box.value, unsafeAddressOf(self), nil, 0)
        }
    }
}

private func == <T: Hashable>(l: T, r: T) -> Bool { return l.hashValue == r.hashValue }

private class HashableWeakBox<T: AnyObject where T: Hashable>: Hashable {
    weak var value: T?
    let hashValueWhenNil: Int

    init(_ v: T) {
        value = v
        hashValueWhenNil = v.hashValue
    }

    var hashValue: Int { return value?.hashValue ?? hashValueWhenNil }
}

private class DeallocWatcher {
    let callback: ()->()
    init(_ c: ()->()) { callback = c }
    deinit { callback() }
}

The trick needed in order to be notified when one of the keys disappears is to add a watcher object (DeallocWatcher) as an associated object of the key (this only works on Swift 1.2, if I'm not mistaken). That way, whenever the key disappears, so does the watcher object, which in turn warns the dictionary class when deiniting.

The block property should, if needed, be set by the code using the class, and provides a way to be notified whenever a key is removed from the dictionary.

I'd like to know if this is reliable and works correctly in all cases. I'd also like to have a better idea of under which conditions can this be used across multiple threads. Finally, I'd like feedback on how I can better follow Swift guidelines and implement common "Swift-isms".

(Not a full review, but some remarks and suggestions.)

Generally, your code looks correct to me, I could not see obvious
errors or problems – for the single-threaded usage.

You cannot
assume that it will work correctly when used from multiple threads
simultaneously.
Cocoa mutable collection types are not thread-safe (see Thread Safety Summary),
and – as far as I know – the same applies to Swift collections
(references: Swift: process Array in parallel using GCD, Swift mutable array thread safety, Swift Arrays are not Threadsafe).

In

let watcher = DeallocWatcher { [weak self] in
    if let v = self?.dict[hashableBox] { self?.block(v) }
    self?.dict[hashableBox] = nil
}

self is (conditionally) unwrapped three times. I would use
optional binding instead so that it is unwrapped only once.
You could also take advantage of the fact that removeValueForKey()
returns the old value (or nil):

let watcher = DeallocWatcher { [weak self] in
    if let s = self {
        if let v = s.dict.removeValueForKey(hashableBox) {
            s.block(v)
        }
    }
}

When a value for key is removed in

    else {
        objc_setAssociatedObject(key, unsafeAddressOf(self), nil, 0)
        dict[hashableBox] = nil // <-- not needed!
    }

then the first line already causes the value to be removed from dict,
because disassociating the associated object fires the DeallocWatcher
callback. So the second line is not needed.

The keys and keyValues properties can be simplified using map():

public var keyValues: [(K, V)] {
    return map(dict) { (k, v) -> (K, V) in (k.value!, v) }
}

public var keys: [K] {
    return Array(dict.keys.map { $0.value! })
}

You can also easily add conformance to additional protocols to make
your WeakKeyDictionary more look and feel like a real dictionary.

If you add the Printable protocol

public class WeakKeyDictionary<K: AnyObject, V where K: Hashable> : Printable {

    // ...
    public var description: String  {
        return "[ " + ", ".join(map(dict) { (k, v) in "\(k.value!) : \(v) " }) + "]"
    }
}

then println(wkdict) produces an output similar to a dictionary.

And with SequenceType

public class WeakKeyDictionary<K: AnyObject, V where K: Hashable> : Printable, SequenceType {

    // ...
    public func generate() -> GeneratorOf<(K, V)> {
        var gen = dict.generate()
        return GeneratorOf {
            if let (k, v) = gen.next() {
                return (k.value!, v)
            }
            return nil
        }
    }
}

you can enumerate keys and values like

for (k, v) in wkdict {
    println((k, v))
}


Upgrading animation code I understand that this animation code is outdated:

[UIView beginAnimations:@"Move" context:nil];
[UIView setAnimationCurve:UIViewAnimationCurveEaseIn];
[UIView setAnimationDelay:0.08];
self.view.frame = CGRectMake(0, 0, self.view.frame.size.width, self.view.frame.size.height);
[UIView commitAnimations];

What are the modern best practices for achieving the same result?

In iOS 4 and later you are encouraged to use animation blocks.

[UIView animateWithDuration: 0.5f
            delay: 0.08f
            options: UIViewAnimationCurveEaseIn
            animations: ^{
                self.view.frame = CGRectMake(0, 0, self.view.frame.size.width, self.view.frame.size.height);
            }
            completion: ^(BOOL finished){
               // any code you want to be executed upon animation completion
            }
];

One advantage of using block-based animation is that

  When this code executes, the specified animations are started immediately on another thread so as to avoid blocking the current thread or your application’s main thread.

This means that the rest of your application will not be "locked up" while your animation is executing.


toggleSaveStream button template I've got a view which has a corresponding template. The template looks something like like:

        <button id="saveStream" class="button-icon button-small save <%= userLoaded ? '' : 'disabled' %>" title="<%= userLoaded ? saveStreamMessage : cantSaveNotSignedInMessage %>">

        </button>

        <button id="clearStream" class="button-icon button-small clear" title="<%= chrome.i18n.getMessage('clearStream') %>" >

        </button>

I've passed into the template some variables such as userLoaded. This allows me to render the initial state of the saveStream button.

Now, the state of userLoaded has changed and I would like to update the button. I don't want to render my entire template again because streamItems is an expensive list to render.

So, I wrote the following:

initialize: function() {
    this.listenTo(User, 'change:loaded', this.toggleSaveStream);
},

toggleSaveStream: function () {
    var userLoaded = User.get('loaded');
    var templateHelpers = this.templateHelpers();

    this.ui.saveStreamButton.toggleClass('disabled', userLoaded);
    this.ui.saveStreamButton.attr('title', userLoaded ? templateHelpers.saveStreamMessage : templateHelpers.cantSaveNotSignedInMessage);
}

Which serves to keep my button up to date.

I'm unhappy with this solution because the logic for the disabled state of the button is duplicated into two spots. Is this a valid concern? How to handle?

Button is rendered disabled in template and I don't want to re-render whole template to change button state. How to stay DRY?

From looking at it, the name of your function toggleSaveStream is lying, calling it twice will not make it toggle, it will look at the the status of the user and set the status accordingly. So I would simply call it updateSaveStreamButton.

Since it does that, you could call it at first, de-activating the button, and then once the user is loaded call it again.

Then you would simply have

<button id="saveStream" class="button-icon button-small save disabled>" title="">

And then

initialize: function() {
    this.updateSaveStreamButton();
    this.listenTo(User, 'change:loaded', this.updateSaveStreamButton);
},

updateSaveStreamButton: function () {
    var userLoaded = User.get('loaded');
    var templateHelpers = this.templateHelpers();

    this.ui.saveStreamButton.toggleClass('disabled', userLoaded);
    this.ui.saveSelectedButton.attr('title', userLoaded ? templateHelpers.saveStreamMessage : templateHelpers.cantSaveNotSignedInMessage);
}

This sounds too easy, so feel free to point what I am missing ;)


DB-to-Java value mapper In my company, I've inherited some Java library that I'm now writing tests to, refactoring and fixing Sonar issues.

One particular point that Sonar is complaining about is a big chaining of if/else if/else statements, that is used in a class responsible for parsing values that come from the DB to the corresponding values used in Java. The cyclomatic complexity of this method is 32, where Sonar is configured to a threshold of 15.

I would like some ideas of a better way to write this. Maybe some design-pattern that could be applied.

Since this is a pretty critical part of the application, it would be interesting not to add a considerable overhead.

private Object getValue(final Class<?> type, final int index, final ResultSet resultSet) throws SQLException {
    Object value = null;

    if (Boolean.class.isAssignableFrom(type) || boolean.class.isAssignableFrom(type)) {
        String v = resultSet.getString(index);
        if (v != null) {
            value = "Y".equalsIgnoreCase(v);
        }
    } else if (Byte.class.isAssignableFrom(type) || byte.class.isAssignableFrom(type)) {
        value = resultSet.getByte(index);
    } else if (Byte[].class.isAssignableFrom(type) || byte[].class.isAssignableFrom(type)) {
        value = resultSet.getBytes(index);
    } else if (Short.class.isAssignableFrom(type) || short.class.isAssignableFrom(type)) {
        value = resultSet.getShort(index);
    } else if (Integer.class.isAssignableFrom(type) || int.class.isAssignableFrom(type)) {
        value = resultSet.getInt(index);
    } else if (Long.class.isAssignableFrom(type) || long.class.isAssignableFrom(type)) {
        value = resultSet.getLong(index);
    } else if (Float.class.isAssignableFrom(type) || float.class.isAssignableFrom(type)) {
        value = resultSet.getFloat(index);
    } else if (Double.class.isAssignableFrom(type) || double.class.isAssignableFrom(type)) {
        value = resultSet.getDouble(index);
    } else if (BigDecimal.class.isAssignableFrom(type)) {
        value = resultSet.getBigDecimal(index);
    } else if (Timestamp.class.isAssignableFrom(type)) {
        value = resultSet.getTimestamp(index);
    } else if (Time.class.isAssignableFrom(type)) {
        value = resultSet.getTime(index);
    } else if (Date.class.isAssignableFrom(type)) {
        value = resultSet.getDate(index);
    } else if (LocalTime.class.isAssignableFrom(type)) {
        String vlrString = resultSet.getString(index);
        if (StringUtils.length(vlrString) == 6) {
            value = LocalTime.of(Integer.valueOf(vlrString.substring(0, 2)), Integer.valueOf(vlrString.substring(2, 4)), Integer.valueOf(vlrString.substring(4)));
        }
    } else if (LocalDate.class.isAssignableFrom(type)) {
        Timestamp timeStamp = resultSet.getTimestamp(index);
        value = timeStamp == null ? null : timeStamp.toLocalDateTime().toLocalDate();
    } else if (LocalDateTime.class.isAssignableFrom(type)) {
        Timestamp timeStamp = resultSet.getTimestamp(index);
        value = timeStamp == null ? null : timeStamp.toLocalDateTime();
    } else if (String.class.isAssignableFrom(type)) {
        value = resultSet.getString(index);
    } else if (type.isEnum()) {
        value = DomainUtils.get((Class<Enum>) type, resultSet.getString(index));
    } else {
        value = resultSet.getObject(index);
    }

    if (resultSet.wasNull() && !type.isPrimitive()) {
        value = null;
    }

    return value;
}

I think, performance wise, you won't get much better than the if-then-else construct. Since isAssignableFrom is not a direct lookup, but a rather complex task, but your runtime types are actually limited, it may be helpful to build a cache around the result. So, if you found a type match, keep that in a direct lookup cache to avoid walking the if-then.

To help with code-maintenance, I would refactor the comparisions into seperate stateless comparator classes, each will implement one function, the getValue method.

The list of comparators will be initialized at some early point in the lifecycle of your application and put in a list. The method getValue will be reduced to a for loop over the comparator-list.

Adding another type is now reduced to changing a comparator class. This makes it very obvious, what you have changed even in changelogs, since the class is mentioned in the committed-files-list.

To help maintenance even more, you could introduce either a search of your classpath for the comparator interface or introduce an annotation to mark the implementations for automatic building of the list.

This will give you an almost self-maintaining code for the given task. Whenever you need a new type, you just add it with the right interface/annotation and it will magically be seen on the next run.

This would be your main entry point.

public class ValueGetter {

    private static List<TypeConverter> converter;

    static {
        // might require synchronize
        converter = new ArrayList<>(1);
        converter.add(new FloatConverter());
        // ... add more or build using reflection or whatever
    }

    public Object getValue(final Class<?> type, final int index, final ResultSet resultSet) throws SQLException {
        Object value;

        for (TypeConverter c : converter) {
            value = c.getValue(type, resultSet, index);
            if (value != TypeConverter.NOTTYPE) {
                return value;
            }
        }

        throw new IllegalArgumentException("Cannot convert to requested type");
    }
}

This is the interface mentioned.

public interface TypeConverter {

    public static final String NOTTYPE = "not of this type";

    Object getValue(Class<?> type, ResultSet resultSet, int index) throws SQLException;
}

This is taking the float if-else

public class FloatConverter implements TypeConverter {
    @Override
    public Object getValue(Class<?> type, ResultSet resultSet, int index) throws SQLException {
        if (Float.class.isAssignableFrom(type) || float.class.isAssignableFrom(type)) {
            return resultSet.getFloat(index);
        }

        return NOTTYPE;
    }
}


Querying the same fields for many different values on AppEngine I'm creating a gradebook.  Many students earn a grade in many standards each.  I'm hoping to get help analyzing the code below for efficient use of asynchronous result fetching. ofy is an Objectify object, which is basically just a convenience wrapper over the async fetching capabilities of AppEngine.

public List<StandardScoreDAO> getStandardScores(
        ArrayList<Long> studentIdLongs, ArrayList<Long> standardIdLongs)
{
    ObjectifyService.register(StandardScoreDAO.class);
    Objectify ofy = ObjectifyService.begin();

    List<Iterator<StandardScoreDAO>> results = new ArrayList<Iterator<StandardScoreDAO>>();

    for (Long studentId : studentIdLongs)
    {
        for (Long standardId : standardIdLongs)
        {
            results.add(ofy.query(StandardScoreDAO.class)
                    .filter("measuredUserId =", studentId)
                    .filter("standardId =", standardId)
                    .iterator());
        }
    }

    ArrayList<StandardScoreDAO> ret = new ArrayList<StandardScoreDAO>();

    for (Iterator<StandardScoreDAO> i : results)
    {
        while (i.hasNext())
        {
            ret.add(i.next());
        }
    }

    return ret;
}

As you can see, if there are 100 studentIds and 100 standardIds, this is 10,000 queries.  AppEngine will only run 10 in parallel. Aiee! I don't know if it's at all realistic to expect this to finish in 30 seconds (testing soon), but I wonder if anyone sees a much better way to organize this.

With Objectify, you have the option to use the "in" operator in a query. Moreover, it becomes clear from the second block of nested loops that you are only looking for the intersection of the two sets of measuredUserIds and standardIds. So, a single statement should do the trick:

public List<StandardScoreDAO> getStandardScores(
        ArrayList<Long> studentIdLongs, ArrayList<Long> standardIdLongs) {
    ObjectifyService.register(StandardScoreDAO.class);
    Objectify ofy = ObjectifyService.begin();

    Iterator<StandardScoreDAO> i = ofy.query(StandardScoreDAO.class)
                    .filter("measuredUserId in", studentIdLongs)
                    .filter("standardId in", standardIdLongs)
                    .iterator();

    List<StandardScoreDAO> ret = new LinkedList<StandardScoreDAO>();

    while (i.hasNext()) {
        ret.add(i.next());
    }

    return ret;
}

If you access a list merely by iteration (and not by random access or by index), and do not know its size on creation, LinkedList might give you a performance benefit.


Interpreting strings with C-style escapes I am working on a compiler that does a pretty heavy work on strings in order to get them correctly displayed. So far I have an algorithm that transforms a given string like (In plaintext):

"This is a \"string\""

After matching the complete token I get something like the following (C string):

"This is a \\\"string\\\""

And the same goes for hexadecimal literals and common escape characters (\r \n \t \b), the code I've came is this one:

#include <stdint.h>

int is_hex(int c) {
  return (is_digit(c) || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F'));
}

static char get_nibble(char T) {
  if (T >= 'A' && T <= 'F')
    return (T - 'a') + 10;
  else if (T >= 'a' && T <= 'f')
    return (T - 'A') + 10;
  return (T - '0');
}

char *post_string(char *in, uint32_t *len, const char **err) {
  if (!in || !len)
    return NULL;

  char *out = in, *tmp = in; uint32_t _len = 0; const char *_err = NULL;
  char esc = 0;
  while (in && *in && !_err) {
    if (*in == '\\') {
      ++in;
      switch (*in) {
        case '"': esc = '"'; ++in; break;
        case 'r': esc = '\r'; ++in; break;
        case 'n': esc = '\n'; ++in; break;
        case 't': esc = '\t'; ++in; break;
        case 'b': esc = '\b'; ++in; break;
        case 'x':
        case 'X': {
          ++in;
          if (!is_hex(*in) || !is_hex(in[1]))
            _err = "Invalid character on hexadecimal escape.";
          if (_err)
            break;
          esc = ((get_nibble(*in) << 4) | (get_nibble(in[1]))) & 0xff; in += 2;
        } break;
        case '\'': esc = '\''; ++in; break;
        case '\\': esc = '\\'; ++in; break;
        case 0:
        default:
          _err = "Unexpected '\\' with no escape sequence."; break;
      }
      if (!_err)
        tmp[_len] = esc, ++_len;
    }
    else {
      tmp[_len] = *in; ++in, ++_len;
    }
  }

  out[_len] = 0;  /* Set the end of string. */
  *len = _len;    /* Set the final string length. */
  if (err)        /* Set error message back. */
    *err = _err;
  return out;
}

  On above implementation I did make sure that a null terminated string is passed to the implementation.

There are no requirements but would be great if no allocation gets done on this step, also a standard implementation should be achieved (Not sure if mine is standard though).

Below is a test main:

#include <stdlib.h>

int main(void) {
  char t[] = "This is a \\\"string\\\"";
  uint32_t len = 0;
  printf("Before: %s\n", t);
  post_string(t, &len, NULL);
  printf("After : %s\n", t);
  return 0;
}

How can I improve this code?

Use <cctype> for character classification

Instead of defining is_hex() (and depending on abcdef and ABCDEF being contiguously coded), better to use the standard isxdigit().

get_nibble makes a similar assumption - it's arguably safer to use a switch for this.

On the whole, the logic looks good.  I'm not a fan of names beginning with _ - even if you do fully understand the rules about reserved identifiers, many of your readers will not!

Some expressions can be simplified with post-increment.  Consider this fragment:

if (!_err)
    tmp[_len] = esc, ++_len;
} else {
    tmp[_len] = *in; ++in, ++_len;
}

That could be

if (!_err)
    tmp[_len++] = esc;
} else {
    tmp[_len++] = *in++;
}

That's fairly idiomatic usage, and should be familiar to anyone who's looked at strcpy or the like.  In fact, slightly more idiomatic would be to use a pointer, rather than indexing:

char *post_string(char *const in, size_t *len, const char **err) {
    if (!in || !len)
        return NULL;

    char *out = in, *p = in;
    const char *int_err = NULL;

    while (*p && !int_err) {
        if (*p != '\\') {
            /* normal case */
            *out++ = *p++;
        } else {
            /* escape sequence */
            switch (*++p) {
            case 'a': *out++ = '\a'; ++p; break;
            case 'b': *out++ = '\b'; ++p; break;
            case 'f': *out++ = '\f'; ++p; break;
            case 'n': *out++ = '\n'; ++p; break;
            case 'r': *out++ = '\r'; ++p; break;
            case 't': *out++ = '\t'; ++p; break;
            case 'v': *out++ = '\v'; ++p; break;

            case '"': case '\'': case '\\': *out++ = *p++; case '?': break;

            case 'x': case 'X':
                if (!isxdigit(p[1]) || !isxdigit(p[2])) {
                    int_err = "Invalid character on hexadecimal escape.";
                } else {
                    *out++ = (char)(get_nibble(p[1]) * 0x10 + get_nibble(p[2]));
                    p += 3;
                }
                break;

            default:
                int_err = "Unexpected '\\' with no escape sequence."; break;
            }
        }
    }

    *out = '\0';                /* Set the end of string. */
    *len = p - in;              /* Set the final string length. */
    if (int_err && err)         /* Set error message back. */
        *err = int_err;
    return in;
}

Testing

Some of the code appears completely untested (and I've fixed above).  In particular, passing NULL as the err parameter - as in the sample main() - will provoke undefined behaviour if your string is invalid).

Modified code

I've also added code to accept 1-3 octal digits, and modified the hexadecimal reader to accept 1+ digits, and included tests of each error branch.

#include <ctype.h>
#include <limits.h>
#include <stddef.h>

static unsigned int get_nibble(char c)
{
    switch(c) {
        case '0': return 0;
        case '1': return 1;
        case '2': return 2;
        case '3': return 3;
        case '4': return 4;
        case '5': return 5;
        case '6': return 6;
        case '7': return 7;
        case '8': return 8;
        case '9': return 9;
        case 'A': case 'a': return 0xa;
        case 'B': case 'b': return 0xb;
        case 'C': case 'c': return 0xc;
        case 'D': case 'd': return 0xd;
        case 'E': case 'e': return 0xe;
        case 'F': case 'f': return 0xf;
    }
    /* shouldn't happen */
    return 0;
}

char *post_string(char *const in, size_t *len, const char **err) {
    if (!in || !len)
        return NULL;

    char *out = in, *p = in;
    const char *int_err = NULL;

    while (*p && !int_err) {
        if (*p != '\\') {
            /* normal case */
            *out++ = *p++;
        } else {
            /* escape sequence */
            switch (*++p) {
            case 'a': *out++ = '\a'; ++p; break;
            case 'b': *out++ = '\b'; ++p; break;
            case 'f': *out++ = '\f'; ++p; break;
            case 'n': *out++ = '\n'; ++p; break;
            case 'r': *out++ = '\r'; ++p; break;
            case 't': *out++ = '\t'; ++p; break;
            case 'v': *out++ = '\v'; ++p; break;

            case '"': case '\'': case '\\': case '?': *out++ = *p++; break;

            case 'x': case 'X':
                if (!isxdigit(*++p)) {
                    int_err = "Invalid character on hexadecimal escape."; break;
                } else {
                    unsigned int x = 0;  /* assuming int is larger than char */
                    while (isxdigit(*p)) {
                        x *= 0x10;
                        x += get_nibble(*p++);
                        if (x > UCHAR_MAX) {
                            int_err = "Out of range hexadecimal escape."; break;
                        }
                    }
                    *out++ = (char)x; break;
                }

            default:
                if (isdigit(*p) && *p < '8') {
                    /* octal escape */
                    unsigned int x = 0;  /* assuming int is larger than char */
                    for (int i = 0;  i < 3 && isdigit(*p) && *p < '8';  ++i) {
                        x = x * 010 + (unsigned)*p++ - '0';
                    }
                    if (x > UCHAR_MAX) {
                        int_err = "Out of range octal escape."; break;
                    }
                    *out++ = (char)x; break;
                } else {
                    int_err = "Unexpected '\\' with no escape sequence."; break;
                }
            }
        }
    }

    *out = '\0';                /* Set the end of string. */
    *len = (size_t)(p - in);    /* Set the final string length. */
    if (int_err && err)         /* Set error message back. */
        *err = int_err;
    return in;
}

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

int main(void) {
  char t[256] = "This is a \\\"string\\\" with \\x43 \\X063 hex\\"
      "nnewline\\ttab\\t\\?\\'\\\"\\\\octal\\132\\538 \\0null";
  size_t len = 0;
  printf("Before: %s\n", t);
  post_string(t, &len, NULL);
  printf("After : %s\n\n", t);

  const char *err = NULL;
  strcpy(t, "This has a hex error: \\xg");
  printf("Before: %s\n", t);
  post_string(t, &len, &err);
  printf("Error: %s\n\n", err);

  strcpy(t, "Hex beyond ASCII: \\xfff");
  printf("Before: %s\n", t);
  post_string(t, &len, &err);
  printf("Error: %s\n\n", err);

  strcpy(t, "Octal beyond ASCII: \\777");
  printf("Before: %s\n", t);
  post_string(t, &len, &err);
  printf("Error: %s\n\n", err);

  strcpy(t, "not an escape: \\9");
  printf("Before: %s\n", t);
  post_string(t, &len, &err);
  printf("Error: %s\n\n", err);

  strcpy(t, "backslash at end: \\");
  printf("Before: %s\n", t);
  post_string(t, &len, &err);
  printf("Error: %s\n\n", err);
}


Generate random records for table in SQL I'm doing a project in ms sql and I have to fill table with random records.

Here is a definition of table:

create table invoice(
    id_invoice INT PRIMARY KEY IDENTITY(1,1),
    id_employee INT FOREIGN KEY REFERENCES Employee(id_employee),
    id_client INT FOREIGN KEY REFERENCES Client(id_client),
    id_service INT FOREIGN KEY REFERENCES Service(id_service),
    amount MONEY,
    payment_way VARCHAR(20) CHECK (payment_way in ('cash', 'credit_card'))
);

And here is the code that generates random records:

ALTER PROCEDURE generate_invoices
AS
BEGIN
    DECLARE @id_employee INT
    DECLARE @id_client INT
    DECLARE @id_service INT
    DECLARE @amount INT
    DECLARE @payment_way VARCHAR(20)

    SET @id_employee = (SELECT TOP 1 id_employee FROM Employee ORDER BY NEWID())
    SET @id_client = (SELECT TOP 1 id_client FROM Client ORDER BY NEWID())
    SET @id_service = (SELECT TOP 1 id_service FROM Service ORDER BY NEWID())
    EXEC random 1000, 5000, @amount OUTPUT
    IF (@amount % 2 = 1) SET @payment_way = 'cash'
    ELSE SET @payment_way = 'credit card'
    INSERT INTO Invoice VALUES (id_employee, @id_clienta, @id_service, @amount, @payment_way)
END
GO

EXEC generate_invoices
GO 10

The code works obviously, but I would like to ask if there is better way (shorter, more efficient) to do same thing, because I must write over a dozen of similar procedure for other tables.

Randomness
Using NEWID() seems like a pretty smart way of getting random records, and it is. There is however a better function for this purpose. Say hello to the CRYPT_GEN_RANDOM() function.
The CRYPT_GEN_RANDOM() has a parameter that we need to provide, the length of the random string it is going to generate. For what we need, 4 or 5 characters is plenty.
I will update my answer with the proof when I find the time, but my own testing found CRYPT_GEN_RANDOM() generates less predictable records than NEWID().
The Random Stored Procedure
I have no idea what your Random stored procedure entails, but might I suggest an alternate method of generating a random number...
WITH Numbers(I) AS
(
    SELECT 1000

    UNION ALL

    SELECT I+1
    FROM Numbers
    WHERE I<5000
)

SELECT TOP 1 @amount = I
FROM Numbers
ORDER BY CRYPT_GEN_RANDOM(4)
OPTION (MAXRECURSION 0)

First, this generates a table of numbers 1000 to 5000 using a recursive CTE, then selects one randomly using the CRYPT_GEN_RANDOM() that we discussed earlier.
To If or Not to If
Might I also suggest an alternate method of generating the @payment_way. Instead of using an If, might I suggest using a Simple Case Statement.
Personally I find this
SET @payment_way =
CASE @amount % 2
    WHEN 1
    THEN 'cash'
    ELSE 'credit card'
END

To be a more SQL friendly way of writing it.
Putting it all together
This is what I ended up with. It's not any smaller, but it doesn't rely on any external stored procedures.
ALTER PROCEDURE generate_invoices
AS
BEGIN
    DECLARE
        @id_employee INT = (SELECT TOP 1 id_employee FROM Employee ORDER BY CRYPT_GEN_RANDOM(4)),
        @id_client INT = (SELECT TOP 1 id_client FROM Client ORDER BY CRYPT_GEN_RANDOM(4)),
        @id_service INT = (SELECT TOP 1 id_service FROM Service ORDER BY CRYPT_GEN_RANDOM(4)),
        @amount INT,
        @payment_way VARCHAR(20)

    SET @id_employee
    SET @id_client
    SET @id_service

    WITH Numbers(I) AS
    (
        SELECT 1000

        UNION ALL

        SELECT I+1
        FROM Numbers
        WHERE I<5000
    )
    SELECT TOP 1 @amount = I
    FROM Numbers
    ORDER BY CRYPT_GEN_RANDOM(4)
    OPTION (MAXRECURSION 0)

    SET @payment_way =
    CASE @amount % 2
        WHEN 1
        THEN 'cash'
        ELSE 'credit card'
    END

    INSERT INTO Invoice VALUES (@id_employee, @id_clienta, @id_service, @amount, @payment_way)
END
GO


Partitioning and sorting even and odd numbers within an array I solved a problem where it was told to sort an array of numbers such that the even numbers are kept first and the odd numbers should follow. Moreover, the even numbers must be sorted in ascending order among them and the odd numbers as well. Only arrays can be used. My code is as follows , which works fine but it seems bit long to me. I wanted to know if there is anything I can do to shorten it and make it more readable.

#include <stdio.h>

int main(void)
{
    int n,i,k,p,j,temp;

    scanf("%d",&n);

    int arr[n],arr_even[n],arr_odd[n];

    k=0,p=0;
    for(i=0;i<n;i++){
        scanf("%d",&arr[i]);
        if(arr[i]%2==0) arr_even[k++]=arr[i];
        else arr_odd[p++]=arr[i];
    }

    for(i=0;i<k;i++){
        for(j=0;j<=i;j++){
            if(arr_even[j]>arr_even[i]){
                temp=arr_even[i],arr_even[i]=arr_even[j],arr_even[j]=temp;
            }
        }
    }

    for(i=0;i<p;i++){
        for(j=0;j<=i;j++){
            if(arr_odd[j]>arr_odd[i]){
                temp=arr_odd[i],arr_odd[i]=arr_odd[j],arr_odd[j]=temp;
            }
        }
    }

    p=0;
    for(i=0;i<n;i++)
    {
        if(k) arr[i]=arr_even[i],k--;
        else arr[i]=arr_odd[p++];
        }

    for(i=0;i<n;i++)
    {
        printf("%d ",arr[i]);
    }

    return 0;
}

Input:

10
0 5 1 2 3 4 6 12 10 9

Output:

0 2 4 6 10 12 1 3 5 9

Your code is quite complicated. The C standard library provides all the ingredients you need to sort an array. You just need to define a comparison function. All the rest is done by the qsort function from stdlib.h.

The comparison function should look like:

static int even_first(const void *a, const void *b) {
    int left = *(const int *)a;
    int right = *(const int *)b;

    int res = (left % 2 != 0) - (right % 2 != 0);
    if (res == 0)
        res = (left > right) - (left < right);
    return res;
}

The expressions of the form cond1 - cond2 may look strange at first, but they are commonly used in C code in comparison functions like this one.

The benefit over a naïve left - right is that no integer overflow can happen. Integer overflow is a common source of undefined behavior.

To make the code more readable, it's also possible to extract the basic integer comparison into a separate function:

static int compare(int a, int b) {
    return a < b ? -1 : a > b ? +1 : 0;
    // alternatively: return (a > b) - (a < b);
    // alternatively: return a < b ? -1 : a > b;
}

Then, the comparison function becomes:

static int even_first(const void *a, const void *b) {
    int left = *(const int *)a;
    int right = *(const int *)b;

    int res = compare(left % 2 != 0, right % 2 != 0);
    if (res == 0)
        res = compare(left, right);
    return res;
}

This form is much less of a brain twister than the above variant.


Beginner FizzBuzz solution Is this a bad solution to the FizzBuzz thing?

I decided to make it print "Fizz Buzz" for the numbers divisible by 3 and 5 just for practice/understanding.

for (var n = 1; n <= 100; n++) {
var output = "";
if (n % 3 == 0)
    output += "fizz";

if (n % 5 == 0)
    output += "buzz";

if (n % 5 == 0 && n % 3 == 0)
    output = "fizz buzz";

console.log(output || n);
};

Indentation:

You're missing indentation in your code, you should space out each level of indentation.

For example:

for (var n = 1; n <= 100; n++) {
var output = "";
if (n % 3 == 0)
    output += "fizz";

would become:

for (var n = 1; n <= 100; n++) {
    var output = "";
    if (n % 3 == 0)
        output += "fizz";

Use braces

You should wrap your code in braces, so you don't cause any bugs unknowingly:

For example, you (or someone in future that maintains your code) might think this wouldn't log anything, however, "omg" is logged.

if (5 != 5)
    console.log("wow");
    console.log("omg")

Even Apple got caught out by one of these bugs.

Unnecessary logic:

In your block, you check whether the number is divisible by 3, then by 5, then by 3 and 5. You don't need to perform the last check of 3 and 5, the first two make up that one.

Leaving you with:

Implementing all those changes, you would end up with:

for (var n = 1; n <= 100; n++) {
    var output = "";
    if (n % 3 == 0){
        output += "fizz";
    }
    if (n % 5 == 0){
        output += "buzz";
    }
    console.log(output || n);
};


Function that returns index of Forex candle according opening hours (weekly session) The output of the function is as desired. When Forex opens at Sunday, first candle is annotated with 1 and the number increments with every new candle, till weekend, when the imaginary counter is reset.

I am aware of those risks (it is solved elsewhere):

passed datetime must be python datetime
time must be in GMT

My concerns are about readability and optimality of the code. Any advice?

def weektag_from_time(timestamp, freq):
    """
    This function returns index of the current time in Forex session,
    according to chosen granularity.
    Forex session - Sunday 2200 to Friday 2200.
    If the session is off, it should return 0.

    Args:
        * timestamp : python datetime in GMT
        * freq : desired granularity (minute, hour, ...)

    Returns:
        * weektag : integer index in Forex session
    """
    # init
    assert freq in ["H1", "M1"], "Required granularity is not known."
    d = timestamp.weekday()
    h = timestamp.hour
    m = timestamp.minute
    # get tag
    if (d == 5) or (d == 4 and h > 21) or (d == 6 and h < 22):
        # market is closed
        weektag = 0
    else:
        # estimate week day offset in hours and add it to current day hours
        offset = (24*d)+2 if d < 6 else -22
        h += offset
        # set result according granularity
        if freq == "H1":
            weektag = h
        elif freq == "M1":
            weektag = (h*60) + m
    return weektag

The code overall is readable and well documented. Of course, there is always something to improve, here are some things:

the frequencies are currently hardcoded in the function and the "assert" is not actually a strong check and would not prevent an invalid freq value to be used if Python compilation is optimized. I'd define frequencies in an Enum class:

from enum import Enum

class Frequency(Enum):
     HOURLY = "H1"
     MINUTELY = "M1"

Then, as a bonus, if you would use variable type annotations, your IDE would warn you if you would pass an invalid frequency to the weektag_from_time() function.
use day, hour and minute in place of more questionable d, h and m - remember, we are writing programs for people to read, not computers
I would improve on the "is market closed" check defining individual conditions in separate variables (the "Extract Variable" refactoring method), something like

is_saturday = day == 5
is_late_friday = day == 4 and hour > 21
is_early_sunday = day == 6 and hour < 22

market_closed = is_friday or is_late_thursday or is_early_saturday

or, may be even easier and more understandable would be to define an open market time range and reverse the logic. And, it might be a good idea to extract this check into a separate reusable function (I'm sure you have this kind of check in other parts of your project).
now, we may use an early "return" if market is closed and omit the "else" part decreasing the nestedness:

if market_closed:
    return 0

offset = (24 * day) + 2 if day < 6 else -22
hour += offset

# set result according granularity
if freq == Frequency.HOURLY:
    return hour
elif freq == Frequency.MINUTELY:
    return (hour * 60) + minute

according to PEP8, you should have spaces around operators
the offset calculation logic probably needs more explanation - consider adding an extra comment explaining the idea behind calculating it


Simple multiplication quiz I am learning Python, and would like to learn how to make this code simpler and easier to read.

Things I am hoping to learn:

What variables might be appropriate?
How can I make it simpler?
Is there any way to make it more efficient?
What can be put into functions without sacrificing readability?

import random
yn = raw_input("Do you wanna build a mathman? y/n ")
if yn == "n":
    exit()

else:
    while yn =="y":
        x =int( random.choice(range(1,100)))
        y =int( random.choice(range(1,100)))
        a = str(x)
        b = str(y)
        s = 0
        xy = x * y
        print "What is " + a  + " x " + b + "?"
        ua = int(raw_input("What is the answer? "))
        while ua != xy:
            print "You are wrong, try again "
            ua = int(raw_input("Try again- "))
            s = s - 1
        yn = raw_input("You are correct, would you like to go again? y/n ")
        s = s + 1
        t = str(s)
    print "Your score is " + t

First of all, you have the following construct:

if yn == "n":
    exit()
else:
    lots
    of
    code

But if yn equals "n", the exit() will have been executed, and you won't reach any other lines anyway. So you can just write

if yn == "n":
    exit()

lots
of
code

This is like an 'early return' (except not in a function, but in the main body of code).

Second, you have

x =int( random.choice(range(1,100)))
...
a = str(x)
...
print "What is " + a + " x " + b + "?"

The spacing on the line with x = is off. It should be

x = int(random.choice(range(1, 100))

That is: no spaces next to ( and ). A space on each side of the =, and a space after the ,.

Furthermore, you should really use string formatting, allowing you to write

print "What is {} + {}?".format(x, y)

This way you don't need intermediate variables a and b.

I assume ua means user_answer here:

print "What is " + a  + " x " + b + "?"
ua = int(raw_input("What is the answer? "))

Why not just write

problem = "What is {} x {}? ".format(x, y)
user_answer = int(raw_input(problem))

You're basically asking a question, and then "What is the answer to the question?". Better to just ask the question alone.

Instead of writing xy, you could write solution or answer.

Another thing of note: the score (I assume that's what you mean with s) gets reset to 0 each time you get a new problem statement, but the score (t) is only printed at the end of the rounds.

That is, you have (pseudo-code)

while play_again:
    ...
    s = 0
    answered = ...
    while answered != solution:
        answered = ...
        s -= 1  # You had s = s - 1, that is equivalent to s -= 1
    s += 1
    t = str(s)
print "Your score is " + t

While instead I think you mean

s = 0
while play_again:
    ...
    answered = ...
    while answered != solution:
        answered = ...
        s -= 1  # You had s = s - 1, that is equivalent to s -= 1
    s += 1
t = str(s)
print "Your score is " + t

Note that the last two lines can be replaced with

print "Your score is {}".format(s)

(Also note: if the user inputs something different from y or n on the first go, you get a NameError: t is referenced before it is assigned because you never enter the while loop).


Grouping Similar Items into buckets I've written some code to group a list of items into arbitrarily sized buckets. If the items are all the same and the and the count is a multiple of the bucket size a single bucket is returned, if the items are different then they are grouped into  buckets of a given size.

For example, with a bucket size of 3:

1,2         => [1,2]
1,2,3,4,5   => [1,2,3][4,5]
1,1,1,1,1,1 => [1,1,1,1,1,1]
1,1,1,1,1   => [1,1,1][1,1]

To achieve this I've written the following .Net extension method:

public static IEnumerable<IList<T>> GroupBySize<T>(this IEnumerable<T> source, int groupSize, Func<T, T, bool> comparer)
{
    var result = new List<List<T>>();

    if (source.IsNullOrEmpty())
        return result;

    List<T> currentGroup = null;

    for (var i = groupSize; i <= source.Count(); i += groupSize)
    {
        var possibleGroup = source.Slice(i - groupSize, groupSize);
        if (possibleGroup.All(x => comparer(x, possibleGroup.First())))
        {
            if (currentGroup == null)
            {
                currentGroup = new List<T>();
                result.Add(currentGroup);
            }

            currentGroup.AddRange(possibleGroup);
        }
        else
        {
            currentGroup = new List<T>();
            currentGroup.AddRange(possibleGroup);
            result.Add(currentGroup);
            currentGroup = null;
        }
    }

    var remainingItems = source.Count() % groupSize;
    if (remainingItems > 0)
    {
        result.Add(source.Slice(source.Count() - remainingItems, remainingItems).ToList());
    }

    return result;
}

public static IEnumerable<T> Slice<T>(this IEnumerable<T> source, int start, int count)
{
    return source.Skip(start).Take(count);
}

public static bool IsNullOrEmpty<T>(this IEnumerable<T> items)
{
    // Taken from :http://haacked.com/archive/2010/06/10/checking-for-empty-enumerations.aspx
    return items == null || !items.Any();
}

The code passes the tests I've written, but it all (including the method name) feels a bit clunky.

How could this be improved?

I'm not sure you could make this code less clunky, it seems to me the code is reasonably simple, considering what it has to do.

But I can see other problems in your code:

The comparer parameter is probably not necessary in most cases, you can use EqualityComparer<T>.Default for this. But optional comparer makes sense to me.
LINQ extension methods are usually lazy and that's what I would expect from this method too. For example, if I do something like collection.GroupBySize(3).First(), it's not necessary to create the whole (possibly very long) result, only to discard most of it. To do this, you can use yield return.
You iterate the source collection (using LINQ methods) a lot. This can have terrible performance, especially if the source collection is something like a result of a LINQ database query, because it queries the database many times, which is not necessary. To fix this, you should write your method around a single foreach (var item in source) and don't use methods like Count(), First(), or Skip().


Extracting and normalizing URLs in an HTML document I have written code to get all urls on a webpage & put them in a set, and would like tips on simple changes I can make to increase its performance.

soup = BeautifulSoup(html_doc)
for link in soup.find_all('a'):
    url = link.get('href')
    if url is None or ' ' in url or '<' in url or '>' in url:
        continue
    if url.startswith('//'):
        url = url.replace('//', 'http://')
    if url.startswith('/'):
        url = hostname + url
    if '?' in url:
        url = url.split('?')[0]
    if '#' in url:
        url = url.split('#')[0]
    if url.endswith('/'):
        url = url[:-1]
    if url.endswith(excluded_extensions):
        continue
    if url.startswith(hostname):
        urls_set.add(url)

Some stuff you could perhaps do differently:

# your code
if url is None or ' ' in url or '<' in url or '>' in url:
    continue

# the alternative
if url is None or any(char in url for char in ' <>'):
    continue

Also, you can call the split method directly, without the if statement, as it will return a single item list with the full string inside if the character is not in the string:

# your code
if '?' in url:
    url = url.split('?')[0]
if '#' in url:
    url = url.split('#')[0]

# the alternative
for splitter in '?#':
    url = url.split(splitter, 1)[0]

Notice the micro-optimization of using the second argument of split, so that the string is only split at the first occurrence if there is more than one.


N-Repeated Element in Size 2N Array The task
is taken from leetcode

  In a array A of size 2N, there are N+1 unique elements, and exactly
  one of these elements is repeated N times.

  Return the element repeated N times.

  Example 1:

  Input: [1,2,3,3] Output: 3

  Example 2:

  Input: [2,1,2,5,3,2] Output: 2

  Example 3:

  Input: [5,1,5,2,5,3,5,4] Output: 5

  Note:

  4 <= A.length <= 10000 0 <= A[i] < 10000 A.length is even

My solution

var repeatedNTimes = function(A) {
    const set = new Set();
    for (const n of A) {
        if (set.has(n)) { return n; }
        set.add(n);
    }
};

I didn't do much with the information size 2N, there are N+1 unique elements, and exactly one of these elements is repeated N times. I think these information is not needed to solve the problem efficiently. I also wonder whether there is a bitwise or purely math based solution to the task.

In the worst case (the repeated element occupies the second half of the array), the set will accommodate all N non-repeated elements, therefore the space complexity of the solution is \$O(N)\$.

Your intuition is correct; the solution doesn't use the important information. It is used to find out how far apart the repeated elements are. If a distance between them is at least \$d\$, the array would be at least \$d\cdot(N-1) + 1\$ long. Since we know that it is \$2N\$ long, we can conclude that \$d \le \dfrac{2N-1}{N-1}\$, which is effectively 2 for N > 2. It is enough to compare each a[i] with a[i+1] and a[i+2]. The space complexity is now constant.


Hackerrank: Count the number of palindromes in a given string I am solving a problem for Hackerrank where I have to count the number of palindromes in a given string.
I have the following code which counts the number of palindromes accurately but times out for large strings. How do I make the code more time-efficient?
def countPalindromes(s):
    num_palindrome = 0

    for i in range(len(s)):
        for j in range(i + 1, len(s) + 1):
            substring = s[i: j]
            if len(substring) == 1:
                num_palindrome += 1
            else:
                for idx in range(len(substring)):
                    if substring[idx] != substring[-idx-1]:
                        num_palindrome -= 1
                        break
                num_palindrome += 1

    return num_palindrome

For each position in the string your code checks all substrings that start at that position. For example, for s = 'a2b3bb3443bab' and i = 3, the code checks '3', '3b', '3bb', '3bb3', ..., '3bb3443bab'. That is O(n^2) strings to palindrome check.
Instead, for each position in the string, check for palindomes that are centered on that position. For example, for s = 'a2b3bb3443bab' and i = 3, check the substrings '3', 'b3b', and '2b3bb'.  Stop there, because no other string centered on i = 3 can be a palindrome.  It also needs to check for palindromes with an even length such as at i = 4, where we check 'bb', '3bb3' and stop at 'b3bb34'.  I think it's O(n*p) where p is the average length of a palindrome (which is less than n).  It seems to take about 2/3 as long as you code.
def pcount(s):
    count = 0

    for i in range(len(s)):
        # count the odd-length palindromes
        # centered at s[i]
        j=i
        k=i
        while j>=0 and k<len(s) and s[j] == s[k]:
            count += 1
            j -= 1
            k += 1

        # count the even-length palindromes
        # centered at s[i:i+1]
        j = i
        k = i + 1
        while j>=0 and k<len(s) and s[j] == s[k]:
            count += 1
            j -= 1
            k += 1

    return count


Best combination of a sum between two parameters in an array of objects I am trying to build a simple algorithm to achieve the best combination of values based on two params in an array of objects.
The whole code is available here
First I have an array holding objects. The items which will be analyzed and packed into volumes together. All of the values are in percentages.
const nonStackableItems = [
    {
      id: 0,
      m2: 15,
      m3: 11,
      weight: 20
    },
    {
      id: 1,
      m2: 25,
      m3: 12,
      weight: 42
    },
    {
      id: 2,
      m2: 50,
      m3: 13,
      weight: 40
    },
    {
      id: 3,
      m2: 65,
      m3: 14,
      weight: 25
    }
  ];

The idea is to combine these objects into packages, respecting the maximum of 100% for each value (except for m3 in this case, as all the packages should have enough height in this first analysis).
The approach that I gave is just not good enough, as I am sorting this array by m2 then by weight, call the function and then compare the length of both, and then choose the result that creates less volumes.
This is the piece of code that does the calculations:
function nSVolHelper(arr, vol, param1, param2) {
    let newVol = {
      id: vol.length,
      m2: 0,
      m3: 0,
      weight: 0
    };
    for (let i = 0; i < arr.length; i++) {
      const validation =
        newVol[param1] + arr[i][param1] < 100 &&
        newVol[param2] + arr[i][param2] < 100;
      if (validation) {
        newVol.m2 += arr[i].m2;
        newVol.m3 += arr[i].m3;
        newVol.weight += arr[i].weight;
      }
      if (!validation) {
        return nSVolHelper(arr.slice(i), [...vol, newVol], param1, param2);
      }
    }
    vol.push(newVol);
    return vol;
  }

  function nSVol(arr, param1, param2) {
    return nSVolHelper(arr, [], param1, param2);
  }

I would like to find a way of doing this without taking as a base the order of the array, to get the finest calculation without running unnecessary code, and I get the feeling that this approach of mine is just not reliable as well.
As an example, for better understanding:
If I pass the array sorted by weight I get a create 2 volumes.
console.log(
    "call",
    nSVol(
      nonStackableItems.sort((a, b) => a.weight - b.weight),
      "weight",
      "m2"
    )

If I pass this same array sorted by m2 I get 3 volumes.
console.log(
    "call",
    nSVol(
      nonStackableItems.sort((a, b) => a.m2 - b.m2),
      "weight",
      "m2"
    )

This was the best approach that I could find, but I feel that there could be another way of thinking of this.

You're right that your solution wouldn't be the most accurate. If I'm understanding your question right, what you just described is the Multi-dimensional 0-1 knapsack problem (see the "Multi-dimensional knapsack problem" heading in this wikipedia page). Solving these kinds of problems is not easy for computers to do quickly, and the best algorithm to use can largely depend on the type of data you're expecting to receive.
Are there very, very few items you're ever going to work with? How important is speed? Can you get away with a simple brute-force method, and try every possibility?
How much does accuracy matter? Maybe the particular use case doesn't necessarily need the best answer, just a good enough answer. If so, the code you wrote would work just fine.
Otherwise, I would read up a bit on the knapsack problem, and try googling "Multi-dimensional 0-1 knapsack problem" (multidimensional because you are working with multiple limits, not just weight. 0-1 because you either use the item or you don't, you can't use 1/2 an item in a package). My quick searching showed that there's a number of technical articles out there that address this exact issue. Knowing the name of the algorithm you need should help with finding more resources on it :).


Submitting a form via JSON I have a file named feedback.js that I would like to improve.

function feedback(feedbackform) {

    // collect the feedbackform data while iterating over the inputs
    var data = {};
    for (var i = 0; i < feedbackform.length; i++) {
        var input = feedbackform[i];
        if (input.name && input.value) {
            data[input.name] = input.value;
        }
    }

    // construct an HTTP request
    var xhr = new XMLHttpRequest();
    xhr.open(feedbackform.method, feedbackform.action);
    xhr.setRequestHeader('Content-Type', 'application/json; charset=UTF-8');
    // send the collected data as JSON
    xhr.send(JSON.stringify(data));
    feedbackform.send.disabled = true;

    xhr.onloadend = function () {
        if (xhr.status == 200) {
            feedbackform.send.value = "Sent!";
            feedbackform.send.className = "success";
        } else {
            feedbackform.send.value = "Failed to send";
            feedbackform.send.className = "fail";
        }
    };

    return false;
}

Is there a modern way to collect form inputs? E.g. json enctype?
Is disabling the submit button a good idea to prevent multiple form submissions?
If the form fails to send, what's a good recourse (to prevent data loss of the filled form) I can give the user? Perhaps munge it into a mailto: command?

Are there any other tweaks I can make without relying on any dependencies, such as trying to reduce code size?

Fetch API is now supported by Firefox, Chrome and Opera, you could use it instead of the ugly XMLHttpRequest. How ironic, by the way, is using XMLHttpRequest to make a JSON reqest?

var data = {};

Since you likely won't be reassigning this variable, you could use const instead. The same is true for var xhr = new XMLHttpRequest();.

for (var i = 0; i < feedbackform.length; i++) {
    var input = feedbackform[i];
    if (input.name && input.value) {
        data[input.name] = input.value;
    }
}

First, that's a perfect use case for a for...of loop. Secondly, if you want to check if a string is empty, it's better to explicitly compare it with an empty string.

I would rewrite this loop like this:

for (let input of feedbackform) {
  if (input.name !== "" && input.value !== "") {
    data[input.name] = input.value
  }
}

feedbackform.send.disabled = true;

It's better to disable the whole form, instead of just the submit button. Did you know that you can submit a form by pressing Enter while focused in an input?

xhr.onloadend = function () {}

Oh come on, it's the 21st century! Why do you still use .onevent properties? You should totally drop that and try jQuery .addEventListener(). See addEventListener vs onclick.

You can also use an arrow function here.

I would rewrite it like this:

xhr.addEventListener("loadend", () => {
  // event listener code
})

if (xhr.status == 200)

Always use three-equals operator (===) for equality comparisons. See Does it matter which equals operator (== vs ===) I use in JavaScript comparisons?.

Besides, 200 isn't the only status code which means that request was successful. There is for example status code 204—No Content. Therefore, you should be checking whether the status code is in 200-299 range:

if (xhr.status >= 200 && xhr.status < 300)

return false;

What purpose does it serve?


Wandering water ways An entry for the August 2016 Community Challenge

I slightly modified the input and output to suit the snippet tool, and also because I really like the mapped output with the basins.

I have been golfing a lot recently, which always impacts my coding style. You will find a number of possibly too smart one-liners. I am looking for more gracious alternatives.

//1 liner helper functions
const $ = id => { return document.getElementById(id); },
      trim = s => { return s.trim(); },
      splitNumbersOnSpace = s => { return s.split(' ').map(Number); },
      join = a => { return a.join('') };

//DOM elements
const input = $('input'),
      output = $('output'),
      output2 = $('output2'),
      go = $('go');

go.addEventListener('click', () => {

  const depths = input.value.split('\n').map(trim).map(splitNumbersOnSpace),
        basins = Array(depths.length).fill().map(()=>{return [];}),
        basinDepths = new Map();
  let nextBasin = 1;

  //compare a spot with the lowest neighbour thus far
  function compareNeighbour( lowestSpotSoFar, attemptRow, attemptCol ){
    try {
      if( depths[attemptRow][attemptCol] < lowestSpotSoFar.value ){
        lowestSpotSoFar.row = attemptRow;
        lowestSpotSoFar.col = attemptCol;
        lowestSpotSoFar.value = depths[attemptRow][attemptCol];
      }
    } catch (e) {
      //This is bound to happen due to out of bounds access requests
    }
  }

  //For a given spot, try and find a lower neighbour
  function lowestNeighbour(spot){
    //Assume we are in a sink
    const lowest = {
      row: spot.row,
      col: spot.col,
      value: depths[spot.row][spot.col]
    }
    //Test the assumption
    compareNeighbour( lowest, spot.row + 1, spot.col );
    compareNeighbour( lowest, spot.row - 1, spot.col );
    compareNeighbour( lowest, spot.row, spot.col + 1 );
    compareNeighbour( lowest, spot.row, spot.col - 1 );
    return lowest;
  }

  //Mark a whole path as belong to a basin
  function markPath( path , basin ){
    basinDepths.set( basin , ( basinDepths.get( basin ) || 0 ) + path.length );
    while(path.length){
      let spot  = path.shift();
      basins[spot.row][spot.col] = basin;
    }
  }

  //Recursively wander on the map towards a sink
  function wander( path ){
    const spot = lowestNeighbour( path[0] );
    //We are on a sink
    if( spot.row == path[0].row && spot.col == path[0].col ){
      markPath( path , nextBasin++ );
    //We hit a known path to a sink
    }else if ( basins[spot.row][spot.col] ){
      markPath( path , basins[spot.row][spot.col] );
    //Any other case, keep wandering
    }else {
      path.unshift(spot);
      wander(path);
    }
  }

  //Analyze the depth map
  for( let row = 0 ; row < depths.length ; row++ ){
    for( let col = 0 ; col < depths[row].length ; col++ ){
      //We already know know the basin this spot belongs to
      if( basins[row][col] )
        continue;
      //We will now wander on a path to a sink
      wander( [{row,col}] );
    }
  }

  //Spit it out
  output.innerHTML = basins.map( join ).join( '
' );
  output2.innerHTML = 'Sizes: ' + Array.from( basinDepths.values() ).sort().join(' ');
});
#output, #input {
  font-family: monospace;
}
<textarea id="input" rows="7">
    1 0 2 5 8
    2 3 4 7 9
    3 5 7 8 9
    1 2 5 4 3
    3 3 5 2 1</textarea>

  <button id="go">Analyze</button>

Output

I've been looking at this code for a good long while now, and I must admit I don't have much in terms of a review. It's all nicely done!

What few quibbles I have are minor:

output and output2 aren't really the best names, are they?
Also, basinDepths appears to actually hold basin sizes. The code already deals with "actual" depths, so the name's needlessly confusing.
Speaking of, "depth" is perhaps a slightly confusing term in this case. It's actually altitudes, and the rain flows from high to low. By calling it "depths", one would expect it to be the other way around: Water flowing to greater depth values.
The one-liners aren't all that useful. $ is fine (though maybe using document.querySelector would be nicer, as one might expect $ to behave jQuery-ish), but the rest are really only called once, as far as I can tell. So I would argue that this:

const depths = input.value
  .split('\n')
  .map(line => line.trim().split(' ').map(Number));

would be as clear as using extracted methods. Yes, it's nice to name and encapsulate the operations being performed, and the original line really reads quite well. Still... eh, just seems excessive when there's no re-use of said operations.
Conversely, if you want to go all-in on English-like legibility, a lines function should perhaps take the place of split('\n')?
Your use of whitespace is... eclectic? I'm used to reproaching people for not using enough whitespace, but here it's the opposite. Maybe you're over-correcting after too much code golf? :)
I just find it curious that a line like this:

function markPath( path , basin ){

has tons of space in the argument list, but nothing between ){ (esp. since you do use a space for x => { y } declarations... except in that place where you don't).
Or that your else-if looks like this:

}else if (...){

when most would probably write:

} else if(...) {

instead. Indeed you do write if(...) { in  other places.

Again, it's not a disaster, but it is inconsistent and odd.


Simple interval timer in Python I created a script for interval timer with tkinter gui.
Here the part that is displaying time looks to be done not in
the best way. Any ideas would be greatly appreciated.

This timer is intended for timing HIIT/Tabata workouts.
User provides working time, rest time, preparation time
in seconds and integer number of intervals and upon hitting
start button displays work/rest seconds countdown.
After each interval a beep sound is played and a song
after all of them are over.
Also there options to change color of background and text.

from tkinter import *
from tkinter.colorchooser import *
from PIL import ImageTk, Image
import os

master = Tk()

def label_message(gt, wt, rt, it):

    res=[]

    for i in range(int(gt),-1,-1):
        res.append('Get Ready!: \n'+ str(i))
    res.append('')

    for i in range(int(it)):
        for j in range(int(wt),-1,-1):
            res.append('Round: \n' + str(i) +'\n Work : \n'+ str(j))
        res.append('')
        for k in range(int(rt),-1,-1):
            res.append('Round: \n' + str(i) +'\n Rest : \n'+ str(k))
        res.append('')

    res.append('Over!')

    return res

def window_color():
    color = askcolor()
    #print (color)
    widgets=[label1,label2,label3,label4,label5,master]
    for i in widgets:
        i.configure(background=color[1])

def text_color():
    color = askcolor()
    #print (color)
    widgets=[label1,label2,label3,label4,label5]
    for i in widgets:
        i.configure(fg=color[1])

def return_entry():

    wt = e1.get()
    rt = e2.get()
    gt = e3.get()
    it = e4.get()

    texts=label_message(gt, wt, rt, it)

    for i in texts:
        if i:
            master.after(1000, label5.config(text=i))
            master.update()
        else:
            master.bell()

    os.startfile('Jean Jacques Goldman - Asterix Et Obelix Contre Cesar (Generique Debut).mp3')

master.configure(background='white')
master.resizable()
master.title='Timer'

label1 = Label(master, text="Working Time", bg='white')
label2 = Label(master, text="Rest Time", bg='white')
label3 = Label(master, text="Get Ready Time", bg='white')
label4 = Label(master, text='Intervals', bg='white')
label5 = Label(master, text='time', height=5,  font=('Times New Roman', 15), bg='white')

img = ImageTk.PhotoImage(Image.open('clock.jpg'))
panel = Label(master, image = img)
panel.grid(column=0, row=0, rowspan=11, columnspan=3)

button1=Button(text='Window Color', command=window_color)
button1.grid(row=0, column=4, sticky=N+S+E+W)

button2=Button(text='Text Color', command=text_color)
button2.grid(row=0, column=5, sticky=N+S+E+W)

label1.grid(row=1, column=4)
label2.grid(row=2, column=4)
label3.grid(row=3, column=4)
label4.grid(row=4, column=4)
label5.grid(row=7, column=4, columnspan=2, rowspan=2)

e1 = Entry(master)
e2 = Entry(master)
e3 = Entry(master)
e4 = Entry(master)

e1.grid(row=1, column=5)
e2.grid(row=2, column=5)
e3.grid(row=3, column=5)
e4.grid(row=4, column=5)

button3=Button(text='Start', command=return_entry)
button3.grid(row=10, column=4, sticky=N+S+E+W)

button4=Button(text='Quit', command=master.quit)
button4.grid(row=10, column=5, sticky=N+S+E+W)

master.after(1000, None)
mainloop()

Don't use wildcard imports

I know that most tkinter tutorials show you to do from tkinter import *, but they are wrong. PEP8 recommends not use use wildcard imports, and I think that's especially true with tkinter.

Instead, use import tkinter as tk, and then prefix all tk classes with tk.. For example:

master = tk.Tk()
...
label1 = tk.Label(master, text="Working Time", bg='white')

You are using after incorrectly

This statement does not do what you think it does:

master.after(1000, label5.config(text=i))

The above is exactly the same as this:

result = label5.config(text=i)
master.after(1000, result)

My advice is to always have after call a function. So, write a function that takes two arguments, a label and a value, and updates the label with a value. Then, use after to call that function.

def update_label(label, value):
    label.configure(text=value)
...
master.after(1000, update_label, label5, i)

Give your widgets explicit masters / parents

Instead of this:

button1=Button(text='Window Color', command=window_color)

Do this:

button1=Button(master, text='Window Color', command=window_color)

While tkinter will use the default root window if you leave it out, PEP8 says that explicit is better than implicit. Plus, it makes it clear what your intentions are. Otherwise, someone reviewing your code might wonder if you're intentionally putting the widget in the root window or if it's a bug.

Remove useless code

I don't know what you think this is doing, but whatever that is, it's probably not doing it. You can remove this line of code:

master.after(1000, None)


Design watchdog, reconnecting to server I have console application that sends and process XML data. I have created a fault mechanism that the connection drops. I go in a "fault" mode. And try to reconnect to my server. I'm sending keepalives and so is the server. When I'm not receiving any keepalives and any others messages in a timespan of 2 minutes the programs enters a fault mode. Is this the correct method of doing such operations?

This is my watchdog class (reduced code).

internal class Watchdog
{
    private Timer _timer = new Timer(1000);
    public bool Status
    {
        get { return _status; }
        set
        {
            _status = value;
            OnStatusChanged(new CustomEventArgs(value));
        }
    }

    protected virtual void OnStatusChanged(CustomEventArgs e)
    {
        var handler = StatusChanged;
        if (handler != null) handler(this, e);
    }

    public void Stop()
    {
        _timer.Enabled = false;
    }

    public void SetStatus()
    {
        _latestAction = DateTime.Now;
    }

    public void SetManually()
    {
        _latestAction = DateTime.Now.AddDays(-1); //fool the timer
        Status = true;
    }

    void _timer_Elapsed(object sender, ElapsedEventArgs e)
    {
        if ((DateTime.Now - _latestAction) > TimeSpan.FromSeconds(22))
        {
            Status = true;
        }
        if ((DateTime.Now - _latestAction) < TimeSpan.FromSeconds(22))
        {
            Status = false;
        }
    }
}

This is other class where I call the handler. And try to reconnect.

    void _watchdog_StatusChanged(object sender, CustomEventArgs e)
    {
        if (e.Status) //no message has been processed in two minutes or nothing can be sent, something can be wrong
        {
            _watchdog.Stop(); //stop timer otherwise this method has not enough time to execute and will be recalled
            Console.WriteLine(DateTime.Now + " Error mode initiated!");
            if (!Connection.Connected)
            {
                ((IDisposable) Connection).Dispose();
                openConnection(); //try to reopen conn
                if (Connection.Connected)
                {
                    initialize(); //init all settings
                    _watchdog.SetStatus(); //set live tick
                }
            }
            _watchdog.Start();
        }
        else //fault has been restored.
        {
            Console.WriteLine(DateTime.Now + " Error has been resolved!");
        }
    }

And the event handlers where I receive messages I set a live tick.

    private void _tcpReader_AlarmResponseComplete(object sender, CustomEventArgs e)
    {
        Console.WriteLine("alarmresponse received");
        _processAlarmResponsesToDatabase.Add(e.AlarmResponse);
        _watchdog.SetStatus();
    }

And where I try to send a message

    private void send(string msg) //TODO: must be set to private is now so for testing purpose public
    {
        if (Connection.Connected)
        {
            if (!_tcpReader.IsStopped)
            {
                _writer.Write(msg);
                _writer.Flush();
            }
            else
                Console.Error.WriteLine(
                    "Unable to send message to the server, a request has been made to terminate the connection.");
            _watchdog.SetStatus(); //give live tick to watchdog
        }
        else
        {
            Console.Error.WriteLine("Unable to send message, not connected to the server.");
            _watchdog.Stop(); //the next statement generates an event and the timer would generate the same events wich causes to many events
            _watchdog.SetManually(); //set manually that connection has been terminated
        }
    }

I'll express my thoughts step by step reading your code (just a note: you should post a complete compilable code, at least of your main class Watchdog, if you take out what you think is not relevant then it is much harder to understand what you're doing).

  internal class Watchdog : IDisposable

internal modifier (unless Watchdog is a nested class) is superfluous. Moreover if you're not supposed to derive any class from Watchdog I'd declare it as sealed (note that it's an internal class then you can simply remove it when you will need to derive, there is not any public interface to respect).

sealed class Watchdog : IDisposable

IDisposable interface usually comes together with a common pattern implementation, I see no reason to avoid that (even in simple cases):

~Watchdog() {
    Dispose(false);
}

public void Dispose() {
    Dispose(true);
    GC.SuppressFinalize(this);
}

private void Dispose(bool disposing) {
    try {
        if (disposing)
            _timer.Elapsed -= _timer_Elapsed;
    }
    finally {
        _timer.Dispose();
    }
}

  private Timer _timer = new Timer(1000);

_timer is initialized only here (or, better, in constructor) then it should be market as readonly:

private const int TimerTickInMilliseconds = 1000;
private readonly Timer _timer;

public Watchdog() {
    _timer = new Timer(TimerTickInMilliseconds);
    _timer.Elapsed += _timer_Elapsed;
}

  public bool Status { ... }

In your setter you do not check if value changed, if you repeatedly assign true to Status then you'll fire StatusChanged each time.

public bool Status {
    get {
        return _status;
    }
    set {
        if (_status != value) {
            _status = value;
            OnStatusChanged(EventArgs.Empty);
        }
    }
}

That said Status name is pretty misleading. What does it mean? Does it indicate whether timer is enabled? It actually means watchdog is running and after a fixed amount of time watched operation didn't complete. Make it clear, change its name to, for example, Hanged.

public bool Hanged { ... }

  public void SetStatus() { ... }

Your methods have too vague name, as for Status property it should be clear what they do. I'd suggest to rename them:

public void Stop() { ... }
public void Start() { ... }

SetManually() is pretty weird (IMO) but I didn't read caller code (so far) so I'd temporary...drop it. Externally they may simply set Hanged to true.

  void _timer_Elapsed(object sender, ElapsedEventArgs e) { ... }

You're performing this check:

if (a > b)
    doThis();
if (a < b)
    doThat();

It may be simplified to (note that we're also handling a == b case).

if (a > b)
    doThis();
else
    doThat();

You're also setting Status (now Hanged) to false but you don't actually need it, its value must be set in Start() method. I'd also rename this method to something slightly more meaningful and move out 22 seconds magic number:

public void Start() {
    _timer.Enabled = true;
    Hanged = false;
}

private void OnTimerTick(object sender, ElapsedEventArgs e) {
    if (DateTime.Now - _watchedActivityStartTime > Timeout)
        Hanged = true;
}

Few notes:

Timeout may be private readonly TimeSpan Timeout = TimeSpan.FromSeconds(22) or a public accessible property.
You do not need to set Hanged to false (see previous paragraph).
I renamed _latestAction to _watchedActivityStartTime trying to make clear what it contains.
You may consider (it depends which kind of watchdog you want to implement) to also stop the timer when you detected that task hanged (just add a call to Stop()).
If you need to also detect if operation recover (after signaling it was hanged) then you should add this logic in this method (checking Hanged status and adding another DateTime _lastHangOccurence and eventually adding a Reset() method to update _watchedActivityStartTime).

Note that you do not need to perform this check each second, timer can directly be initialized to = new Timer((int)Timeout.TotalMilliseconds) and stopped after hang has been detected (no need to check if (DateTime.Now - _watchedActivityStartTime > Timeout)):

public Watchdog() {
    _timer = new Timer((int)Timeout.TotalMilliseconds);
    _timer.Elapsed += TimerOnElapsed;
}

private void OnTimerTick(object sender, ElapsedEventArgs e) {
    Hanged = true;
    Stop();
}

One final minor note: you should consider to group your methods (somehow), you're now mixing public/protected/private methods/variables in apparently random order.

I skip review of calling code because there isn't (really!) enough context to suggest anything but basic design notes.

All these said I'd consider to change your overall design. As it is now you have an utility class (Watchdog) that is nothing more than a timer. All the logic is spanned between this and callers. A better (IMO) design should centralize all this logic. Just a proof of concept (I omit not relevant code) of one way to do it:

sealed class Watchdog : IDisposable {
    public Watchdog(Action action) {
    }

    public void Start() {
    }

    public event EventHandler Hanged;
}

Used like this:

var watchdog = new Watchdog(TryConnect);
watchdog.Hanged += OnConnectionHanged;
watchdog.Start();

I said one way because (according to your usage pattern) you may have completely different interfaces (you may, for example, have a Start() method that accepts two delegates). The main point is: remove all watchdog logic from callers, what they have to provide are just two actions to call:

One action to execute, this is the watched task.
One action to call when watched task hanged.

Everything else must stay inside your Watchdog class.


Validating email confirmation and sending different emails to admins I wrote a method that will confirm email with token send to user emails. It will also send different email depending on the condition for the registered user. If anyone would write the same method, how would you write this code snippet?

[AllowAnonymous, Route("confirm-email", Name = AccountControllerRoute.GetConfirmEmail)]
public async Task<ActionResult> ConfirmEmail(int accountId, string token)
{
    if (accountId == 0 || token == null)
        return HttpNotFound();

    var account = await AccountManager.FindByIdAsync(accountId);

    if (account == null)
        return HttpNotFound();

    if (account.EmailConfirmed)
        return View("ConfirmEmail"); //return page that will say the token has expired

    var result = await AccountManager.ConfirmEmailAsync(accountId, token);

    var emailBody = string.Empty;
    if (result.Succeeded)
    {
        if(account.Institution == null)
        {
            // send message notifying admin that a new institution needs review and setup
            emailBody = $"{account.UserName}|{account.profile.contact}|{account.profile.institutionName}|{account.profile.institutionAddress}";
            await AccountManager.SendEmailAsync(account.Id, EmailNames.NewInstitutionRequest, "");

            return View("ConfirmEmail");
        }

         // Pass Name, Username
        emailBody = $"{account.Name}|{account.UserName}";

        if (account.Institution.IsClient)
        {
            if (account.Institution.Licence.AllowedAccounts == 0)
            {
                await AccountManager.SendEmailAsync(account.Id, EmailNames.NewUserRequest, emailBody);
                return View("ConfirmEmail"); // send a different message notifying admin about exceeding allow accounts
            }

            // send message notifying admin that a new user request for access
            await AccountManager.SendEmailAsync(account.Id, EmailNames.NewUserRequest, emailBody);
            return View("ConfirmEmail");
        }
        else
        {
            // send message notifying admin that a new user has registered
            await AccountManager.SendEmailAsync(account.Id, EmailNames.NewUserNotificationForSA, emailBody);

            return View("ConfirmEmail");
        }
    }

    return View("Error");
}

Also, could there be a better way to just pass an object rather than a string with a separator that I have to use to parse them, once passed to a method where it send an email to the user?

public class PostalEmailService : IIdentityMessageService
{
    public Task SendAsync(IdentityMessage message)
    {
        if (message == null)
            throw new ArgumentNullException("message");

        EmailBase email;
        switch (message.Subject)
        {
            case EmailNames.ResetPassword:
                email = new ResetPasswordEmail {To = message.Destination, Link = message.Body};
                break;

            case EmailNames.NewUserRequest:
                string name = message.Body.Split('|')[0];
                string userName = message.Body.Split('|')[1];
                email = new NewUserRequestEmail { To = "dummy@gmail.com", Name = name, UserName = userName };
                break;
            default:
                throw new InvalidOperationException("Unknown email '{0}'.".FormatWith(message.Subject));
        }

        return email.SendAsync();
    }
}

I'm a bit confused by this:

if (account.EmailConfirmed)
    return View("ConfirmEmail"); //return page that will say the token has expired

Your comment doesn't correspond with the actual code, especially when that same view is returned elsewhere.

The if (result.Succeeded) should be inverted to avoid 30 lines of indentation. Why not do if (!result.Succeeded) and return View("Error"); immediately afterwards?

Also, result is a fairly meaningless name.

I don't know what you're doing with emailBody, but judging from its format and the comments it looks like you're using it to pass arguments in a delimiter-separated string. That's odd, to say the least.

I also don't see the point of compiling it inside if(account.Institution == null) and then not using its value.

What happens inside if (account.Institution.Licence.AllowedAccounts == 0) looks to be exactly the same as what happens if that isn't the case.

What is EmailNames? An enum? I don't think that class should have a plural name, and it doesn't look like it is an "email name".


Conversion between QString and BSTR I'm currently working on a Qt project writing Excel files through a third party DLL.
As the third party DLL has a COM interface, I have to do lots of conversions between QString and BSTR...

I wrote the following two methods to do the conversions:

QString ExcelClass::bstrToQString(const BSTR &bStr) const
{
        _bstr_t convert(bStr, false);
        return QString::fromUtf16(reinterpret_cast<const ushort*>((const WCHAR*)convert));
}

BSTR ExcelClass::qstringToBstr(const QString &qstring) const
{
        WCHAR* const pBuffer = new WCHAR[1000000]; //We have to deal with very large strings...
        MultiByteToWideChar(CP_ACP, MB_PRECOMPOSED, qstring.toLocal8Bit().data(), -1, pBuffer, 1000000);
        BSTR result = SysAllocString(pBuffer);
        delete [] pBuffer;
        return result;
}

Even if those methods do their job properly, I have the following questions:

Are those conversions completely correct or can they cause errors in rare cases?
Is there any other (better/easier) way to convert?
Can I somehow avoid allocating pBuffer every time I call the function?

The line WCHAR* const pBuffer = new WCHAR[1000000]; seems to slow the program down significantly, as I am calling the method ~60000 times... I can speed it up a lot when I do the conversion (qstringToBstr()) directly inside the calling function but I want it capsuled in this method...
(converted directly inside the caller, memory was only allocated once per 5-column-line, so still ~12000 times, but that was much faster...)

why not use the toWcharArray?

BSTR ExcelClass::qstringToBstr(const QString &qstring) const
{
        WCHAR* const pBuffer = new WCHAR[qstring.size()+1]; //large enough to hold the string plus null terminator
        qstring.toWCharArray(pBuffer);
        pBuffer[qstring.size()]=0;//terminate with 0
        BSTR result = SysAllocString(pBuffer);
        delete [] pBuffer;
        return result;
}

or simply preallocate result with SysAllocStringLen(&result, qstring.length()); and translate into that

BSTR ExcelClass::qstringToBstr(const QString &qstring) const
{
        BSTR result= SysAllocStringLen(0, qstring.length());
        qstring.toWCharArray(result);
        return result;
}


Node js single mongodb connection I want my entire node application to use one single mongodb connection, at least that's what I think is better. I wrote this little script and want some feedback on it. Especially if what I'm doing makes sense.

This is my mongodb.js file

'use strict';

// Mongodb module.
var MongoClient = require( 'mongodb' ).MongoClient;

// Database.
var db;

// Working variable.
var working;

// Callbacks array.
var arr = [];

// Exported variable.
module.exports = function( callBack ) {
    if ( db ) {
        callBack( db );
        return;
    }

    arr.push( callBack );

    if ( working ) return;

    working = true;

    var MongoDB = MongoClient.connect( 'mongodb://127.0.0.1:27017/test',
        function( err, res ) {
            if( err ) throw err;

            console.log( "Connected to MongoDB!" );

            db = res;

            for ( var i = 0; i < arr.length; i++ ) {
                arr[i]( res );
            }

            arr = [];
        }
    );
}

This is a file where I include the mongodb.js

var collection;
new require( 'mongodb.js' )(
    function( db ) {
        collection = db.collection( 'test' );
    }
);

If you make your custom callback node style (i.e. function(err, result)), you can simplify this greatly:

'use strict';
var MongoClient = require('mongodb').MongoClient,
    db;

module.exports = function connectToMongo(url, callback) {
    if (db) { callback(null, db); }
    else {
        MongoClient.connect(url, function(err, conn) {
            db = conn;
            callback(err, conn);
        });
    }
}

And use it:

var connectToMongo = require('./mongodb.js');

/* ... */

connectToMongo('mongodb://127.0.0.1:27017/test', function(err, db) {
    collection = db.collection('test');
});

An even better option would be to use Promises. I use bluebird in my projects, but we'll soon have native Promise support (bluebird will probably still be better).

var mongo = require('mongodb'),
    Promise = require('bluebird');

Promise.promisifyAll(mongo);
Promise.promisifyAll(require('mongodb/lib/mongodb/db').Db.prototype);

var db;

module.exports = function connectToMongo(url) {
    if (!db) {
        db = mongo.connectAsync(url); //This returns a promise!
    }
    return db;
};

Then use it like

var connecttoMongo = requiure('./mongodb.js');
connectToMongo('mongodb://127.0.0.1:27017/test').then(function(conn) {
    //Do stuff here
});