Recursive method to reverse Linked List I want to optimise this code and improve it using advanced C++.

#include <iostream>

template <class T>
class LinkedList
{
  struct Node
  {
    T data;
    Node * next;
    Node(T value) : data(value), next(nullptr) {}
  };
  Node *head;

public:
  LinkedList() : head(nullptr) {}
  ~LinkedList();
  void insert(T);
  void printList();
  void recursiveReverse()
  {
    Node *temp = head;
    head = recursiveReverse(temp);
  }
private:
  Node* recursiveReverse(Node* head)
  {
    if(head == nullptr)
      return nullptr;

    if(head->next == nullptr)
      return head;

    Node *firstElement = head;
    Node *secondElement = firstElement->next;
    head = firstElement->next;
    firstElement->next = nullptr; //unlink first node
    Node *remainingList = recursiveReverse(head);
    secondElement->next = firstElement;
    return remainingList;
  }
};

template <class T>
void LinkedList<T>::insert(T data)
{
  Node *node = new Node(data);
  Node *tmp = head;
  if(tmp == nullptr)
  {
    head = node;
  }
  else
  {
    while(tmp->next != nullptr)
    {
      tmp = tmp->next;
    }
    tmp->next = node;
  }
}

template <class T>
void LinkedList<T>::printList()
{
  Node *node = head;
  while(node)
  {
    std::cout << node->data << " ";
    node = node->next;
  }
  std::cout<<"\n";
}
template <class T>
LinkedList<T>::~LinkedList()
{
  Node *tmp = nullptr;
  while(head)
  {
    tmp = head;
    head = head->next;
    delete tmp;
  }
  head = nullptr;
}

int main()
{
  LinkedList<int> ll1;
  ll1.insert(2);
  ll1.insert(3);
  ll1.insert(4);
  ll1.insert(5);
  ll1.insert(6);
  ll1.printList();
  ll1.recursiveReverse();
  ll1.printList();
}

No warnings with very pedantic clang flags, good job!

You have this in the destructor:

tmp = head;
head = head->next;
delete tmp;

You can replace this with a call to std::exchange:

delete std::exchange(head, head->next);

Use move semantics. Node::Node unnecessarily copies value into data. It should be data(std::move(value)) so that no copy is made. Same goes for LinkedList::insert and so on.
Single characters should be in single quotes, not double quotes. I won't claim it's more efficient, because any compiler worth its salt is going to optimize the call to std::strlen.
In LinkedList::recursiveReverse the first overload, you don't need to  copy head because the other overload you're calling isn't modifying its arguments (no pass by reference). You should also be able to merge both functions.
It might be "better" to keep a pointer to the last node in your linked list. That way, insertion is O(1) instead of O(n).
Mark functions that do not throw noexcept, if you compile with exceptions.
Mark classes that one shouldn't inherit from with final.
You could provide a constructor taking an std::initializer_list, so you could specify the elements of the list directly on construction.
Sometimes you use the implicit conversion to bool for pointers and sometimes not. It might be better to be consistent.
You can use in-class initializers for example for Node::next.
You can provide an iterator interface, so that your container works with the range for loop. For example, you can provide a public iterator, const_iterator alias to basically pointers to the individual elements inside nodes and provide begin and end as functions.
Please provide a copy and move constructor and assignment operator. This is known as the Rule of 5. If I copy your list, then I will probably get a double delete error at runtime, but this is not guaranteed.


Given a string and a word dict, find all possible sentences I have been trying to figure out how to meet space and time complexity requirements for the following leet code question. I have come up with two versions, one that meets the time requirements and another that meets the space requirements. Both versions fail on the same test case (the last test case shown below). Can anyone suggest how to reconcile both space and time requirements? What am I missing?
Question and examples

Given a non-empty string s and a dictionary wordDict containing a list of non-empty words,
add spaces in s to construct a sentence where each word is a valid dictionary word. Return
all such possible sentences.
Note:
The same word in the dictionary may be reused multiple times in the segmentation.
You may assume the dictionary does not contain duplicate words.
Example 1:
Input:
s = "catsanddog"
wordDict = ["cat", "cats", "and", "sand", "dog"]

Output:
[
  "cats and dog",
  "cat sand dog"
]

Example 2:
Input:
s = "pineapplepenapple"
wordDict = ["apple", "pen", "applepen", "pine", "pineapple"]

Output:
[
  "pine apple pen apple",
  "pineapple pen apple",
  "pine applepen apple"
]

Explanation: Note that you are allowed to reuse a dictionary word.
Example 3:
Input:
s = "catsandog"
wordDict = ["cats", "dog", "sand", "and", "cat"]

Output:
[]

Code that fails time limit
class Solution:
    def wordBreak(self, s: str, wordDict: 'List[str]') -> 'List[str]':
        if not wordDict:
            return []

        wD = set(wordDict)
        max_offset = len(max(wD, key=len))
        paths = []

        def dfs(st, path):
            if st == len(s):
                paths.append(path)
                return

            for en in range(st + 1, st + max_offset + 1):
                if s[st:en] in wD:
                    dfs(en, path + [(st, en)])

        dfs(0, [])
        return [' '.join([s[x[0] : x[1]] for x in path]) for path in paths]

Code that fails space limit
class Solution:
    def wordBreak(self, s: str, wordDict: 'List[str]') -> 'List[str]':
        from collections import defaultdict

        if not wordDict:
            return []

        wD = set(wordDict)
        memo = defaultdict(list)
        memo[len(s)] = [[(len(s), len(s) + 1)]]

        for i in range(len(s) - 1, -1, -1):
            for j in range(i + 1, len(s) + 1):
                if s[i:j] in wD and j in memo:
                    for l in memo[j]:
                        memo[i].append([(i, j), *l])
        return [' '.join([s[ind[0] : ind[1]] for ind in x[:-1]]) for x in memo[0]]

Testing (the last test case fails each of the above versions)
import pprint

inps = [
    ("catsanddog", ["cat", "cats", "and", "sand", "dog"]),
    ("pineapplepenapple", ["apple", "pen", "applepen", "pine", "pineapple"]),
    ("catsandog", ["cats", "dog", "sand", "and", "cat"]),
    ("a", ['a']),
    ("abc", ['abc', 'a', 'b', 'c']),
    ("hellow", []),
    (
       "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
       [
           "a",
           "aa",
           "aaa",
           "aaaa",
           "aaaaa",
           "aaaaaa",
           "aaaaaaa",
           "aaaaaaaa",
           "aaaaaaaaa",
           "aaaaaaaaaa",
       ],
    ),
]

sol = Solution()
for s, wd in inps:
    print(f"Doing s[{s}] and wd[{wd}]...")
    ans = sol.wordBreak(s, wd)
    pprint.pprint(ans)

I have figured out how to do this problem and achieve the time and space requirements. It is faster than 64% of submissions but only more space efficient than 5% of submissions (woops). I ended up modifying the code that fails the space requirements. Here is my solution:

class Solution:
    def wordBreak(self, s: str, wordDict: 'List[str]') -> 'List[str]':
        from collections import defaultdict

        if not wordDict:
            return []

        wD = set(wordDict)
        childs = defaultdict(list)
        childs[len(s)] = [None]

        for i in range(len(s) - 1, -1, -1):
            for j in range(i + 1, len(s) + 1):
                if s[i:j] in wD and j in childs:
                    childs[i].append(j)

        paths = []

        def dfs(nd, path):
            if nd is None:
                paths.append(path)
                return

            for ch in childs[nd]:
                dfs(ch, ' '.join([path, s[nd:ch]]))

        dfs(0, '')
        return [x.strip() for x in paths]

Any optimizations are still welcome.


Provably fair coin flip system in Laravel I made a provably fair 1v1 coin flip game in Laravel and I want to be sure it's safe (cannot be hacked) and that the outcome is random (50/50).

At first Player 1 starts the bet and the hash will be created. The hash is a SHA256 of the $secret, which will be revealed to the players after the game. This hash ensures that the game outcome depends on the client and not on the server (provably fair). Does it matter which hash type I use? Are they all equally safe?

$secret = str_random(40);
$bet = new Bet;
$bet->user_id = Auth::id();
$bet->amount = $amount;
$bet->secret_key = $secret;
$bet->secret_hash = hash('sha256', $secret);
$bet->save();

So after that, player 2 will be able to join player 1's bet. When player 2 accepts the bet, this is what happens:

$(document).delegate('.flipcoin-btn', 'click', function() {
    // When player 2 clicks the button
    SubmitFlipCoin($(this).attr('id'), CreateClientKey());
});

function CreateClientKey()
{
    var outStr = "", newStr;
    while(outStr.length < 20)
    {
        newStr = Math.random().toString(36).slice(2);
        outStr += newStr.slice(0, Math.min(newStr.length, (20 - outStr.length)));
    }
    console.log("Client string created: "+outStr.toUpperCase());
    return outStr.toUpperCase();
};

function SubmitFlipCoin(id, client)
{
    StopMoneyUpdate = 1;

    $.ajax({
        method: 'POST',
        url: flipcoin_link,
        data: {id: id, clientstr: client, _token: token},
        dataType: 'JSON',
    })
    .done(function(result) {
        if(result.hasOwnProperty("error")) {
            ShowBetError(''+result.error+'');
        }
    })
    .fail(function(error) {
        ShowBetError('An error occurred while flipping the coin, please try again.');
    });
}

As you can see, the client (player 2) generates a random string, which will be send to the server. The server will then make an HMAC hash of bet_id+clientstring with the $secret as key. The outcome of the game is determined by this HMAC hash. It takes the second number of the HMAC hash. If the result is 0 to 4 then player 1 wins, and if result is 5 to 9 player 2 will win.

$resulthash = hash_hmac('sha512', $request->clientstr."-".$bet->id, $bet->secret_key);
$resultnumber = filter_var($resulthash, FILTER_SANITIZE_NUMBER_INT);
if($resultnumber[1] < 5) {
    $result = 0;
}
else {
    $result = 1;
}

EDIT: I did some testing to see if the outcome is really 50/50 and it seems pretty accurate.

$dealer = 0;
$player = 0;
$times = 1000000;

for($i = 0; $i < $times; $i++) {
    $result = filter_var(hash('sha512', rand()), FILTER_SANITIZE_NUMBER_INT)[1];
    if($result < 5) {
        $dealer++;
    }
    else {
        $player++;
    }
}

echo "Dealer: ".$dealer/$times * 100;
echo "
Player: ".$player/$times * 100;

This was the outcome that I got:

Dealer: 49.9891
Player: 50.0109

So why are you saving both the $secret and the hash of the $secret in the DB?

$bet->secret_key = $secret;
$bet->secret_hash = hash('sha256', $secret);

Also how does the hash of the $secret allow you to ensure that the outcome is fair? Do you ever use this hash anywhere?

Also carrying out any critical operations that you expect to remain secure on a client that allows someone to view and alter the code via a console cannot be considered secure. Basically the game should be run entirely on the server with input from the clients, such as the bet amount and bet acceptance, and then the results can be distributed to the clients for display.

Do you have some mathematical proof showing that the distribution of numbers in the second position of a string generated from a HMAC hash after non-number characters are removed is actually random? If not then you cannot assert that this method is truly random and would give you the 50/50 chance you desire.

Since PHP already has a built int cryptographically secure pseudo-random integer generator, int random_int ( int $min , int $max ), why don't your just use that to generate you numbers between 0 and 9?


Lockfree ThreadPool implementation How could this template class ThreadPool be improved?

I use Boost queue to keep std::function's numbers from std::array(simple hashing) from which I can later retrieve the std::function's objects to execute them with operator().
It's not possible to store them directly in boost::lockfree::queue (Stored Object Type must have non-trivial destructor).

That's why I emulate primitive lockfree container with std::array<std::pair<int , ...>.
The GuarderArray template class lacks garbage collector.

#include <iostream>
#include <thread>
#include <mutex>
#include <memory>
#include <atomic>
#include <functional>
#include <boost/lockfree/queue.hpp>
#include <array>

using std::cout ; using std::cin; using std::endl;
using std::mutex; using std::atomic;
using std::array; using std::function;

template <int NN> using guardedArray =  std::array<std::pair<std::atomic<bool>,std::function<void()>>, NN> ;

template <typename T> using lockfreequeue = boost::lockfree::queue<T,boost::lockfree::capacity<1000>>;

class Functor{
    int myNN;
public:
    Functor(int nn):myNN(nn){};
    Functor(std::function<void()> ff) : myff(ff){};
    std::function<void()> myff;
    Functor(const Functor&) = default;
    Functor operator=(const Functor&) ;
    ~Functor() = default;
    void operator()(){
    myff();

    }
};

template <int NN > class GuardedArray{
    guardedArray<NN> myGuardedArray;
public:
    int insertObject(std::function<void()> ff){
    int kk ;
    bool empty;

    do{
    kk = rand () % NN;
    empty = myGuardedArray[kk].first;
    cout << "spining ... " << endl;

    }while(!(std::atomic_compare_exchange_weak(&myGuardedArray[kk].first,&empty,true)));
    myGuardedArray[kk].second = ff;
    return kk;
    };
    std::function<void()> getIndex(int ii){return myGuardedArray[ii].second  ; };
    std::thread garbageCollectorThread;

};

template <int NN > class ThreadPool{

private:
    std::thread tt[NN];
    GuardedArray<1000> myGA;
    bool endFlag;
public:
    ThreadPool():endFlag(false){

    for(int ii = 0 ; ii < NN ; ++ii){
        std::thread localThread(&ThreadPool<NN>:: threadLoop, this);
        tt[ii].swap(localThread);

    }

    };
    void bookTask(std::function<void()> ff){
    int ii = myGA.insertObject(ff);
    mySync.push(ii);
    };

    bool demandedAccess[NN];
    int waitingProcess;
    ~ThreadPool() {endFlag = true; for (int ii = 0 ; ii < 10 ; ii++  ){  tt[ii].join();}   };
    int myWrapper();
    void threadLoop();
    lockfreequeue<int> mySync;

};

template <int NN> void ThreadPool<NN>::threadLoop(){
    std::thread::id kk  = std::this_thread::get_id();
    std::function<void()> fun ;
    int ff;
    while(!endFlag){

    if(!mySync.empty()){
        bool result = mySync.pop(ff);
        if(result){
        fun = myGA.getIndex(ff);
        fun();
        }
    }
    }
}

int main(int argc, char** argv){

    ThreadPool<10> tp;
    Functor ff(( [](){ cout <<"a kuku  "<<endl;  }));
    tp.bookTask(ff);
    std::this_thread::sleep_for(std::chrono::seconds(20));
    return 0 ;
}

Just some stylistic things that stick out to me:

I think it's a bit cumbersome to have all of this:

using std::cout ; using std::cin; using std::endl;
using std::mutex; using std::atomic;
using std::array; using std::function;

You're also still prefixing std:: in places, which defeats the purpose of having this.  Since you could always end up using something not already in this list, you might as well keep prefixing std:: as you're doing already, and remove all of that above.
Some of your indentation and semicolon placing are inconsistent.  In some places you indent or not, and in some places you have a space before the ending semicolon.  Keep the indentation consistent (you should utilize it) and choose what to do with all of your semicolons.
It would be more readable and maintainable to place the template statements on separate lines from the class or function, especially if either statement is lengthy:

template <typename T>
class Class
{
};

template <typename T>
void function()
{
}


Better JS Contact Parser For a site I wrote this JS contact parser. It simply reads contacts and parses links if a known service is identified. Could this be made more compact? How about robustness? (it cannot handle exceptions where the url does not begin with http for instance). What other services should I consider to add?

Here is jsfiddle. And the code is below:

Markup

CSS

span { padding: 10px; }

JS

var contacts = 'guy@gmail.com, guy@owndomain.com, http://twitter.com/guy, http://www.facebook.com/profile.php?id=100002877203519, http://www.linkedin.com/in/coolguy, http://www.myspace.com/coolguy, skype:cool.guy?call, aim:goim?screenname=cool.guy, http://coolguy.com';
var re = / /gi;
contacts = contacts.replace(re, "");
contacts = contacts.split(',');

var picBase = 'http://jqeedu.tuxfamily.org/images/media/';

jQuery(document).ready(function() {

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

});

function addContact(c) {
    cdiv = jQuery("#contacts");
    var i = false;
    var h = '';
    var n = '';

    if(c.indexOf('@gmail.com') != -1) {
        i = 'gmail.png';
        h = 'mailto:'+c;
    }

    if(!i && c.indexOf('http://www.facebook.com/profile.php?id=') == 0) {
        i = 'facebook.png';
        h = c;
    }

    if(!i && c.indexOf('http://www.linkedin.com/in/') == 0) {
        i = 'linkedin.png';
        h = c;
    }

    if(!i && c.indexOf('http://www.myspace.com/') == 0) {
        i = 'myspace.png';
        h = c;
    }

    if(!i && c.indexOf('skype:') == 0 && c.indexOf('?call') != -1) {
        i = 'skype.png';
        h = c;
    }

    if(!i && c.indexOf('aim:goim?screenname=') == 0 ) {
        i = 'aim.png';
        h = c;
    }

    if(!i && c.indexOf('http://twitter.com/') == 0) {
        i = 'twitter.png';
        h = c;
    }

    if(!i && c.indexOf('http://') == 0) {
        i = 'website.png';
        h = c;
    }

    if(!i) {
        i = 'email.png';
        h = 'mailto:'+c;
    }

    n = '';

    cdiv.append(n);

}

Firstly I would change all the occurances of

}

if(!i && c.indexOf(...

to

 } else if(c.indexOf(...

To me there doesn't seem to be any difference logically and its cleaner.

Second I would use regexes for the urls (I'm sure someone will comment on how regexes aren't needed/ too slow yadda yadda.)

var regexFBprofile = /^(http\:\/\/)?(www\.)?facebook\.com\/profile\.php\?id=/;
var regexLinkedIn = /^(http\:\/\/)?(www\.)?linkedin\.com\/in\//;
var regexMySpace = /^(http\:\/\/)?(www\.)?myspace\.com/;
var regexTwitter = /^(http\:\/\/)?(www\.)?twitter\.com\//;

another small thing:

re = / /g;
contacts = contacts.replace re, "");

with:

contacts[i].trim();

http://jsfiddle.net/96Gk6/34/


Analysis describing the combination of cards In this class, the order of the last 3 statements in the constructor is important. If you call OrderedCards = OrderCards(cards, wildcardsCount); last, it will crash with NRE.

Is this a bad idea? If so, how should I fix it?

/// <summary>
/// The abstract result of analysis describing the combination of cards
/// </summary>
public abstract class TichuTrick
{

    public List<CardData> OrderedCards { get; private set; }
    public readonly int CardCount;
    public readonly int TrickValue;
    public readonly int HighestCardValue;
    public readonly int WildcardsCount;

    /// <summary>
    /// Constructs a response of analysis
    /// </summary>
    /// <param name="cards">This class works only with 1 wildcard in each trick.</param>
    internal TichuTrick(List<CardData> cards, int cardCount, int wildcardsCount)
    {
        if (wildcardsCount > 1)
        {
            throw new System.ArgumentException("More than one wildcards support, is not yet implemented", nameof(wildcardsCount));
        }
        CardCount = cardCount;
        OrderedCards = OrderCards(cards, wildcardsCount);
        TrickValue = CalculateTrickValue();
        HighestCardValue = CalculateHighestCardValue();
    }

    protected virtual List<CardData> OrderCards(List<CardData> cards, int WildcardCount)
    {
        return cards.OrderBy(x => x.NumericalValue).ToList().CopyList();
    }

    protected virtual int CalculateTrickValue()
    {
        return CalculateHighestCardValue();
    }

    protected virtual int CalculateHighestCardValue()
    {
        return OrderedCards.Last().NumericalValue;
    }

}

The order is not the problem here but rather the fact that you have multiple virtual calls in your constructor. This is really bad because in your derived class, when they are being instantiated, your constructor will first call the base implementation of the virtual functions your base class constructor, which might result  in unexpected behaviour form t he virtual functions.

Also you shouldn't be using fields, use properties instead.

Here is how you can avoid all of your current issues :

public abstract class TichuTrick
{
    private readonly List<CardData> _cards;
    public List<CardData> OrderedCards => OrderCards(_cards, WildcardsCount);

    public int CardCount { get; }

    public int TrickValue => CalculateTrickValue();
    public int HighestCardValue => CalculateHighestCardValue();
    public int WildcardsCount { get; }

    internal TichuTrick(List<CardData> cards, int cardCount, int wildcardsCount)
    {
        if (wildcardsCount > 1)
        {
            throw new System.ArgumentException("More than one wildcards support, is not yet implemented", nameof(wildcardsCount));
        }
        CardCount = cardCount;
        _cards = cards;
        WildcardsCount = wildcardsCount;
    }

    protected virtual List<CardData> OrderCards(List<CardData> cards, int WildcardCount)
    {
        return cards.OrderBy(x => x.NumericalValue).ToList().CopyList();
    }

    protected virtual int CalculateTrickValue()
    {
        return CalculateHighestCardValue();
    }

    protected virtual int CalculateHighestCardValue()
    {
        return OrderedCards.Last().NumericalValue;
    }
}

I also don't think this parameter should be a parameter at all cardCount you can access the card count from the array, but since I'm not 100% sure that this is correct I left it there.


Todo-list practice code accepts title and list I am new to OOP principles and best practices. People often say I write "unpythonic code," so give me your perspective on how to do things better.

class TodoList(object):

    def __init__(self, title=None):
        self.title = title
        self.items = list()

    # depend on _list, accepts _list index numbers
    def __str__(self, tdl=None):

        if tdl is None:
            tdl=range(len(self.items))

        for i in tdl:
            item = "{0}. {1}".format(i, self.items[i])
            print(item)
        return str()

    def __len__(self):
        return len(self.items)

    # list bindings
    def add(self, item):
        self.items.append(item)

    def update(self, key, update):
        self.items[key] = update

    def remove(self, key):
        del self.items[key]

    # search through i, find query, append key
    def search(self, query):
        result = []
        for k, i in enumerate(self.items):
            if query in i:
                result.append(k)
        return self.__str__(result)

Your code looks nice and is properly formatted. Still, a few points could be improved.

Magic methods

As mentionned in ChrisWue's nice answer, the __str__ magic method is supposed to return a string and is not expected to have any side-effects (such as performing some printing). Also, I would not consider it a good idea to change the signature of a usual magic method. Finally, the string returned by the function is empty which does not bring much information to anyone (the string returned by default is somehow more interesting : <__main__.TodoList object at 0x6085be4ce470>).

The best way to improve this is to split your function into 2 functions: one __str__ with a correct behavior and another one. For instance:

def print_list(self, tdl=None):
    if tdl is None:
        tdl=range(len(self.items))

    for i in tdl:
        item = "{0}. {1}".format(i, self.items[i])
        print(item)

def __str__(self, tdl=None):
    return str(self.title)

This also corresponds to a better Separation of Concerns and to the Single Responsibility Principle : each entity should do/be responsible of a single thing.

More about magic methods

Among the methods you've defined, a few of them could be renamed to emulate a container type:

remove corresponds to __delitem__
update corresponds to __setitem__
__len__ is fine.

search

At the moment, search is defined like this (print_list being the function defined above):

def search(self, query):
    result = []
    for k, i in enumerate(self.items):
        if query in i:
            result.append(k)
    return self.print_list(result)

A few improvements could be done.

First, I'd use i as the variable name for the index and not for the item.

Then, the my_list = []; for ...: if ...: my_list.append(...) pattern is usually written using list comprehension which are more concise and clearer once you get used to it. They are also more efficient and considered more pythonic.

def search(self, query):
    result = [i for i, item in enumerate(self.items) if query in item]
    return self.print_list(result)

Finally, I'd consider it poor API to have a search method that doesn't return anything (while performing some printing). I'd like it more if you were to return the results and to have another method to pretty print them if needed.

The final code looks like (I am still not a big fan of print_list):

class TodoList(object):

    def __init__(self, title=None):
        self.title = title
        self.items = list()

    def print_list(self, tdl=None):
        if tdl is None:
            tdl = range(len(self.items))

        for i in tdl:
            item = "{0}. {1}".format(i, self.items[i])
            print(item)

    def __str__(self, tdl=None):
        return str(self.title)

    def __len__(self):
        return len(self.items)

    def add(self, item):
        self.items.append(item)

    def __setitem__(self, key, update):
        self.items[key] = update

    def __delitem__(self, key):
        del self.items[key]

    def search(self, query):
        return [i for i, item in enumerate(self.items) if query in item]

if __name__ == "__main__":
    x = TodoList("Shopping list")
    print(str(x))
    x.add("buy salad")
    x[0] = "buy potato"
    x.add("buy tomato")
    del x[1]
    x.print_list(x.search("potato"))

Going further

If you want to improve your code, you could consider adding documentation with docstrings. The docstring conventions for Python code can be found in a document called PEP 257.


Generate code from full name and index number Does this script look well-written and formulated?

'''
This is for a practice
Get the last letter from first_name, 2nd and 3rd letters from last name
First 5 numbers from index number.
And generate a a unique code.
'''

import sys

first_name = 'first_name'
last_name = 'last_name'
index_number = str('02001201525') # Converting number to str so it can concat

def from_index(index_number):
    for n in index_number.split():
        return n[0:5]

def generate_code(first_name, last_name):
    letters_from_first_name = first_name[-1]
    letters_from_last_name = last_name[0:2]
    n_from_index = from_index(index_number)
    code = letters_from_first_name + letters_from_last_name + n_from_index
    return code # To the world

 print generate_code(first_name, last_name);

 sys.exit() # Is importing the sys module important as i am not iterating

Your code is quite confusing, are you new to Python? You seem to have some misunderstandings about how it works. First of all, no you don't need sys. Python scripts end automatically when they reach the end. You only need to call sys.exit() if you want to end a script in the middle of it for any reason.

Next, you're calling str on a string that's already a string. Based on the other variables you've defined this might be a placeholder, but it's confusing so it's worth saying that you're using placheolder values. I would also put it in as a number, except that you start the number with 0. An int can't have 0 as its first digit, so either you should be getting that number as a string (removing the need for str()) or there's something else going on here. Also I would suggest that you put the comment on a separate line, to be more readable.

# Converting number to str so it can concat
index_number = str('02001201525')

The from_index function is confusing. The name doesn't tell you what it does. It runs split but there's no need to if you just need the first 5 characters. It also probably shouldn't be a function. It has a very specific seeming use, takes no useful parameters and in the end just returns [0:5] on the string that's passed into it. It's easier to just do this:

index_number[0:5]

The only difference between that and your function is that your function would cut the result short if there was white space in the number, but that doesn't sound like it's what you want.

As for your generate_code, it does work for the most part, but is again a lot of lines to do something relatively simple. It also doesn't take index_number as a parameter even though it really should. I'd actually reduce it to one line, like this:

def generate_code(first_name, last_name, index_number):
    return first_name[-1] + last_name[2:4] + index_number[0:5]

Note that you also put in the wrong parameters to get the second and third parameters of last_name. I'm not sure if it was just a typo or misunderstanding, but I'll explain slicing if you need me to. Just comment and I can write up a bit about it.


Rock Paper Scissors game This is a very simple rock paper scissors game implemented in Haskell.

import System.IO (hFlush, stdout)
import System.Random (getStdGen, randomR)

data RPS = Rock | Paper | Scissors deriving (Eq, Show, Read)

instance Ord RPS where
    Rock `compare` Scissors = GT
    Scissors `compare` Rock = LT

    Paper `compare` Rock    = GT
    Rock `compare` Paper    = LT

    Scissors `compare` Paper = GT
    Paper    `compare` Scissors = LT
    _ `compare` _ = EQ

prompt :: String -> IO String
prompt text = do
    putStr text
    hFlush stdout
    getLine

main = do

    gen <- getStdGen

    humanchoice <- prompt "Rock, Paper, or Scissors: "

    let (compchoice, _) = randomR (0,2) gen
        compchoice' = case (compchoice :: Int) of
                          0 -> Rock
                          1 -> Paper
                          2 -> Scissors
        humanchoice' = read humanchoice :: RPS

    case compchoice' `compare` humanchoice' of
        EQ -> putStrLn "Tie."
        GT -> putStrLn "You Lose :("
        LT -> putStrLn "You Win! :)"

All kinds of reviews are welcome, but I am specifically interested in knowing what kind of beginner mistakes I might have done. For example, are there things that I could simplify, or are things that you wouldn't see in a intermediate level haskell code?

As @200_success mentions in a comment, this isn't a kosher use of Ord. I believe though that it is the property of antisymmetry that is broken, not transitivity. \$Rock \leq Paper \leq Scissors \leq Rock\$ means that by transitivity \$Paper \leq Rock\$, which violates antisymmetry since \$Rock \neq Paper\$. This may seem academic, but many library functions will break or produce incoherent results given your instance. Try to guess what sort [Rock, Scissors, Paper] will return. Don't get in the habit of writing bad instances just because you like their namespace.

What you should do instead is just write a grab bag of pure functions and domain-specific datatypes. This would look something like—

data Winner = PlayerOne | PlayerTwo | Tie

shoot :: RPS -> RPS -> Winner
shoot Rock Rock     = Tie
shoot Rock Paper    = PlayerTwo
shoot Rock Scissors = PlayerOne
-- ...

All top-level definitions should have a type signature always, full stop. main is always main :: IO ().

Since you're not doing anything fancy with your StdGen, just use the *IO functions.

main = do
          humanchoice <- prompt "Rock, Paper, or Scissors: "

          compchoice <- randomRIO (0,2)
          -- ...

One typeclass it would be appropriate to instance is Random, so that you can have a more semantic computer-player interface. I'll toss in Enum and Bounded instances because they make the Random instance more future-proof.

data RPS = Rock | Paper | Scissors
    deriving (Eq, Show, Read, Enum, Bounded)

instance Random RPS where
    random g = randomR (minBound, maxBound) g

    randomR (lo, hi) g = first toEnum $ randomR (fromEnum lo, fromEnum hi) g
        where first f (a, b) = (f a, b)

The last thing I would do would be to factor out the result message code from main, it's a little repetitive having putStrLn there three times, and you can conceive of various scenarios like making this two player where you'd want to be able to easily swap around result message generators.

humanTwo :: Winner -> String
humanTwo PlayerOne = "You lose :("
humanTwo PlayerTwo = "You win! :)"
humanTwo Tie       = "Tie."

And with all of those changes you end up with a main function like—

main :: IO ()
main = do
          humanchoice <- fmap read $ prompt "Rock, Paper or Scissors: "
          compchoice <- randomIO
          let winner = shoot compchoice humanchoice
          putStrLn $ humanTwo winner


Have I missed some obvious XSS vulnerabilities? As the question states: have I missed some obvious security vulnerabilities in this code? Particularly interested in XSS...

I'm trying to create something more robust that strip_tags, but simpler than HTML Purifier which just feels like overkill.

class StripTags {
    private $tags;
    private $dom;
    function __construct($html, $tags = null) {
        // Setup tags
        $this->setTags($tags);

        //Initialise document using provided HTML
        $doc = new DOMDocument();
        @$doc->loadHTML($html);         //suppress invalid HTML warnings
        $doc_elem = $doc->documentElement;

        $this->traverseAndRemove($doc_elem);

        // Store DOM for processing in __toString()
        $this->dom = $doc;
    }
    public function __toString() {
        // Remove unwanted DOCTYPE etc
        $str = $this->dom->saveHTML();
        $str = str_replace('<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN" "http://www.w3.org/TR/REC-html40/loose.dtd">', "", $str);
        $str = preg_replace("/^<html><body>/", "", trim($str));
        $str = preg_replace("/<\/body><\/html>$/", "", trim($str));
        return $str;
    }

    private function setTags($t) {
        if (empty($t) or !is_array($t)) {
            $this->tags = array("html", "body", "p", "span", "div", "img", "b", "i", "em", "strong", "h1", "h2", "h3", "h4", "a", "pre");
        } else {
            $this->tags = $t;
        }
        if (!in_array("html",$this->tags)) {
            $this->tags[] = "html";
        }
        if (!in_array("body",$this->tags)) {
            $this->tags[] = "body";
        }
    }

    private function traverseAndRemove(&$elem) {
        // Check node type, attributes and child nodes
        // Reset child/attribute loops if one is removed
        if ($elem->nodeType === XML_ELEMENT_NODE) {
            $tagName = $elem->tagName;
            if (!in_array($tagName, $this->tags)) {
                // Remove any elements in the tags array and stop processing it
                $elem->parentNode->removeChild($elem);
                return;
            }
            // Check attributes for Javascript events and src/hrefs with javascript too...
            if ($elem->hasAttributes()) {
                $attrs = $elem->attributes;
                for ($i=0,$max=$attrs->length; $i<$max; $i++) {
                    $name = $attrs->item($i)->name;
                    if (in_array($name, array("onload", "onclick","onmouseover","onmousemove","onmousehover","onmousedown","onmouseup", "onunload"))) {
                        $elem->removeAttribute($name);
                        $max--;
                        $i--;
                    }
                    if (in_array($name,array("src", "href")) and preg_match("/^javascript:/i", $attrs->item($i)->value)) {
                        $elem->removeAttribute($name);
                        $max--;
                        $i--;
                    }
                }
            }
        }
        // Check all child nodes too
        if ($elem->hasChildNodes()) {
            $children = $elem->childNodes;
            for ($i=0, $max=$children->length; $i<$max; $i++) {
                $this->traverseAndRemove($children->item($i));
                if ($children->length < $max) {
                    $i -= ($max - $children->length);
                    $max = $children->length;
                }
            }
        }
    }
}

It would help if you explained exactly why you think this approach is more robust than using strip_tags. It will certainly be likely to be a lot slower.

From a quick inspection it doesn't seem to filter CSS behaviours nor data URIs.

If it were me I'd be building on to strip_tags rather than trying to start from scratch.


Detect resolution of X display and change font accordingly This is my first attempt at anything functioning within elisp. This script is part of my configuration file that I use across multiple machines, one of which is hi-dpi and the other is not. To avoid having to reset my default font size every time I pull from my remote repository, I thought it would be nice to write a function to detect the size of the screen and adjust the font size accordingly.

(if (and (eq 3840 (x-display-pixel-width))
         (eq 2160 (x-display-pixel-height)))
  (setq-default dotspacemacs-default-font '("Source Code Pro" :size 34))
  (setq-default dotspacemacs-default-font '("Source Code Pro" :size 18)))
)

It feels a bit repetitious, but I am not comfortable enough with the language to even see how I could simplify anything. Initially I tried to set a font-size variable, but it did not seem to be able to traverse functions easily (i.e. it was set at a local scope).

You wrote calls to setq-default twice, with blah-blah-size-34-or-18. Call it just once, with a :size of an if expression that evaluates to 34 or 18.

Put another way, your "then / else" clauses are evaluated for side effects. I'm suggesting to exploit the functional aspect of if, where its return value comes either from the "then" clause or from the "else" clause.


Get application full path I have a JSF project composed by some Maven Modules.

I have one module that needs to read and write some files on server. To do this I need to know the "real" full path of the application/server.

The way it is coded, only the View layer can access this information by this method:

public final static String getRealPath(String relativePath) {
    FacesContext fc = FacesContext.getCurrentInstance();
    ServletContext context = (ServletContext)fc.getExternalContext().getContext();

    return context.getRealPath(relativePath);
}

Can my module (not in View layer) call some Faces methods like FacesContext.getCurrentInstance()? Am I breaking some rule, layer or pattern? Or should I pass this path through all layers as a method parameter?

I guess you'd be better off using Class.getRessource() to access your files. I assume they are packaged with your application.

This method would not be restricted to the view layer (where it actually not belongs). And you won't be concerned with servlet container details.


Split a given number so that their sum adds to another given number Task

  You are given a string S containing only decimal digits ('0' through
  '9') and a number N. Your task is to insert an arbitrary number
  (including zero) of plus signs '+' into S in order to form a valid
  expression whose value is equal to N.

  Input

  The first line of the input contains a single integer T denoting the number of test cases. The description of T test cases follows.
  The first and only line of each test case contains a string S, followed by a space and an integer N.

  Output

  For each test case, print a single line containing one string — your
  solution, i.e. the expression obtained after inserting some plus signs
  into S. If there are multiple solutions, you may output any one. It is
  guaranteed that at least one solution exists.

My code is not very optimized but gives the correct answer. Help me find an optimized version.

import itertools
case=input("")

for i in range(case):

    text,text2=str(raw_input("")).split()

    text=list(text)
    text1=""
    l=[]
    def break_down(text):
        words = text.split()
        ns = range(1, len(words)) # n = 1..(n-1)
        for n in ns: # split into 2, 3, 4, ..., n parts.
            for idxs in itertools.combinations(ns, n):
                    yield [' '.join(words[i:j]) for i, j in zip((0,) + idxs, idxs + (None,))]

    for i in text:
        #to add whitespace in between so that split() functions separates it correctly
        text1=text1+i
        text1=text1+" "

    for x in break_down(text1):
        #this part calls the generator and sums every iteration
        sum=0
        answer=""

        for i in range(len(x)):
            #removing the whitespaces that were added before
            x[i]=x[i].replace(" ","")
        #converts string to int so that it actually adds number not concatenates them
        x=[int(i) for i in x]

        for i in x:
            #calculating sum
            sum+=i
        if sum==int(text2):

            l=x

        #if the sum is equal to given number then it prints it out in the asked manner
        for i in l:
            i=str(i)
            answer+=i
            answer+="+"
    if int(text1.replace(" ",""))==int(text2):
        print int(text1.replace(" ",""))
    else:
        print answer[:len(answer)-1]

Example Input

4
15112 28
120012 33
123 123
15489001 10549

Example Output

15+11+2
1+20+012
123
1548+9001

Python 2

Python 2 is end-of-life, and Python 3 has a lot of advantages now:

f-strings
unicode support
...

More info here, here and on many other places.

If you need python 2.7, try to make your code compatible with both.

Split it into functions

You do that to a limited extent with break_down, but you can do it in a lot more places:

parsing the input
breaking the input into parts
calculating the sum of the parts
output

from __future__ import print_function

Parsing the input

you do input("") a lot. If you skip the "", you can test your input parsing with something like

input_str = '''4
15112 28
120012 33
123 123
15489001 10549'''
input = iter(input_str.split('\n')).__next__

In the parsing of the input, you iterate over the string, insert a space between characters and then split on the space, while all you need to do is list(text).

So I would go for:

def parse_input():
    cases = int(input())
    for _ in range(cases):
        question, answer = input().split()
        yield question, answer

list(parse_input())

[('15112', '28'), ('120012', '33'), ('123', '123'), ('15489001', '10549')]

Splitting the input

This is what your break_down does. But you format its answer as a str with spaces between the numbers. Why not just yield the ints immediately with int(''.join(words[i:j])) instead of ' '.join(words[i:j])

instead of words for the collection of digits, I would use digits as name. Same with ns and n, I don't find their purpose clear by their name, but I can't find a good, clear name instantly either.

For python 2 and 3 portability, you will need to do list(range(...)), because you iterate over ns twice

And your solution with zip((0,) + idxs, idxs + (None,)) is also well found, but not clear what exactly it does. The same idea can be made more clear using the pairwise itertools recipe.

def pairwise(iterable):
    '''
    s -> (s0,s1), (s1,s2), (s2, s3), ...
    https://docs.python.org/3/library/itertools.html#itertools-recipes
    '''
    a, b = itertools.tee(iterable)
    next(b, None)
    return zip(a, b)

def break_down(text):
    digits = list(text)
    ns = list(range(1, len(digits))) # n = 1..(n-1)
    for n in ns: # split into 2, 3, 4, ..., n parts.
        for idxs in itertools.combinations(ns, n):
            idxs = (0, *idxs, None)
            yield tuple(int(''.join(digits[i:j])) for i, j in pairwise(idxs))

Calculating the sum

instead of

    x=[int(i) for i in x]

    for i in x:
        #calculating sum
        sum+=i

you can use the builtin sum (if you don't overwrite the name with a variable like here) total = sum(map(int, x) does the same as these original 3 lines.

But since we already got ints from the previous part instead of strings, just sum will do:

def check_sum(question, target):
    for parts in breakdown(question):
        if sum(parts) == target:
            yield parts

formatting the answer can also be more efficient and clear than manually putting '+' between all parts.

def format_answer(answer):
    return '+'.join(map(str, answer))

Putting it together

def solve_query(query):
    question, target = query
    target = int(target)
    answer = next(find_sums(question, target))
    return format_answer(answer)

and then, behind a main guard:

if __name__ == '__main__':
    queries = parse_input()
    for query in queries:
        print(solve_query(answer))


Bit stream input in Factor My attempt at writing code to input values of arbitrary bitlength based on a bytearray in Factor. Any comments on the correctness or style would be appreciated.

USING: kernel locals accessors math sequences math.bitwise ;

TUPLE: bitreader bytearray curpos curbit ;
: <bitreader> ( bytearray -- bitreader ) 0 0 bitreader boa ;

: advance ( reader -- reader ) 0 >>curbit [ 1 + ] change-curpos ;

: check-advance ( reader -- reader ) dup curbit>> 8 = [ advance ] when ;

:: readsinglevalue ( bits reader -- result ) reader curpos>> reader bytearray>> nth
                                             reader curbit>> dup bits 1 - + swap bit-range
                                             reader [ bits + ] change-curbit check-advance drop ;

: bitsleft ( reader -- result ) curbit>> 8 swap - ;

DEFER: readbits

:: readmultivalue ( bits reader -- result ) reader bitsleft :> bitsextra
                                            bitsextra reader readsinglevalue
                                            bits bitsextra - reader readbits
                                            bitsextra shift + ;

:: readbits ( bits reader -- result ) reader curbit>> bits + 8 < [ bits reader readsinglevalue ] [ bits reader readmultivalue ] if ;

Honestly, the code seems a bit long for what it does. I'm not suggesting anything crazy like code golf, but is there maybe some way to unify (byte_position, bit_position) so a single call advances one bit at a time? I'm looking for a simple invariant that you maintain across calls. The read* functions appear to be on the verbose side.

You have at least four kinds of values: byte_val, bit_val, byte_pos, bit_pos. I encourage clear consistent naming around those. In particular the comments about leaving "result" on the stack are more vague than I'd like to see.


Add a set of querystring params to a URL Example cases:

Need to add x=1, y=2 and querystring variables to the following URLs:

http://www.foo.com/blah/blah?test=1&abc=2
http://www.foo.com/blah

//-- usage
get_link(array('x' => 1, 'y' => 2, 'z' => 'string'));

function get_link( $my_params )
{
    $param_querystring = "";
    $http_host = $_SERVER['HTTP_HOST'];

    //-- get the part of the url before the querystring, if applicable
    $url = explode( '?', $_SERVER['REQUEST_URI'] );

    $request_uri = $url[0];
    $querystring = $url[1];

    foreach ( $my_params as $param_key => $param_value )
    {
        $param_querystring .= $param_key . '=' . $param_value;
    }

    if ( empty( $querystring ) )
    {
        //-- generates foo.com/blah?x=1&y=2&z=string if no
        //-- querystring was present
        $link = $request_uri . '?' . $param_querystring;
    }
    else
    {

        //-- generates foo.com/blah?a=1&b=2&x=1&y=2&z=string if a=1&b=2
        //-- querystring was already present.
        $link = $request_uri . $querystring . '&' . $param_querystring;
    }

    return $link;
}

Your code is very spaced out. It doesn't need to be.
Consider using industry-standard style by using Egyptian braces for control structures like if and foreach.
Get rid of the $http_host variable; you never use it.
Consider generalising your function to accept any URL instead of pulling REQUEST_URI.
You're kind of reinventing the wheel; PHP has the built-in functions parse_str and http_build_query that you can use.
Do you intend to replace existing variables, or just to append them to the request?

Here's an example that takes advantage of the built-in functions, and intelligently merges in conflicts. (ideone)

/**
 *  Like array_merge, but will recursively merge array values.
 *
 *  @param array $a1
 *      The array to be merged into.
 *  @param array $a2
 *      The array to merge in. Overwrites $a1, when string keys conflict.
 *      Numeric keys will just be appended.
 *  @return array
 *      The array, post-merge.
 */
function merge_query_var_arrays($a1, $a2) {
    foreach ($a2 as $k2 => $v2)
        if (is_string($k2))
            $a1[$k2] = isset($a1[$k2]) && is_array($v2) ? merge_query_var_arrays($a1[$k2], $v2) : $v2;
        else
            $a1[] = $v2;
    return $a1;
}

/**
 *  @param string $query_string
 *      The URL or query string to add to.
 *  @param string|array $vars_to_add
 *      Either a string in var=val&[...] format, or an array.
 *  @return string
 *      The new query string. Duplicate vars are overwritten.
 */
function add_query_vars($query_string, $vars_to_add) {
    if (is_string($vars_to_add))
        parse_str($vars_to_add, $vars_to_add);
    if (preg_match('/.*\?/', $query_string, $match))
        $query_string = preg_replace('/.*\?/', '', $query_string);
    parse_str($query_string, $query_vars);

    $query_vars = merge_query_var_arrays($query_vars, $vars_to_add);
    return @$match[0] . http_build_query($query_vars);
}


Fetching lists and finding one with the right name, with fallback on error or no match I have a simple function:

func getList() (sendgrid.RecipientList, bool, agerr.E) {
    lists, err := client.GetRecipientLists()
    if err == nil {
        for _, l := range lists {
            if l.Name == myList {
                return l, true, nil
            }
        }
    }
    return sg.RecipientList{}, false, err
}

but my colleagues ask my to rewrite it into:

func getList() (sendgrid.RecipientList, bool, agerr.E) {
    lists, err := client.GetRecipientLists()
    if err != nil {
        return sg.RecipientList{}, false, err
    }
    for _, l := range lists {
        if l.Name == myList {
            return l, true, nil
        }
    }
    return sg.RecipientList{}, false, err
}

They argue that errors should be returned as fast as possible. Is it really so big problem? What is your opinion?

There's a couple of reasons if err != nil is usually preferred in Go.

It's idiomatic

If you take a look at the source code of Go's standard library you can find it plenty of times, like in bufio's source, for example.

It's more readable in the long run

In your function, the difference between the two is not that big, but the point is valid if you're writing longer functions, where you make multiple calls to other functions that might return errors.

Suppose your function had to call 3 other "failable" functions, handling errors like in the first snippet would look like this:

func getList() (sendgrid.RecipientList, bool, agerr.E) {
    lists, err := client.GetRecipientLists()
    if err == nil {
        // let's call it again, just for fun
        lists, err = client.GetRecipientLists()
        if err == nil {
            // once more!
            lists, err = client.GetRecipientLists()
            if err == nil {
                 // function logic goes here
            }
        }
    }

    return sg.RecipientList{}, false, err
}

You can probably see that lines get much longer, and in this case there's even no actual logic in the snippet I wrote. IMO this reduces readability by a lot, it's harder to separate an error check from function logic at a glance.

Let's try with the other style:

func getList() (sendgrid.RecipientList, bool, agerr.E) {
    lists, err := client.GetRecipientLists()
    if err != nil {
        return sg.RecipientList{}, false, err
    }

    // let's call it again, just for fun
    lists, err = client.GetRecipientLists()
    if err != nil {
        return sg.RecipientList{}, false, err
    }

    // once more!
    lists, err = client.GetRecipientLists()
    if err != nil {
        return sg.RecipientList{}, false, err
    }

    // function logic goes here

    return sg.RecipientList{}, false, err
}

Of course, inverting the condition means that you have to type more returns and you're not really saved from writing all that error handling boilerplate.

At the same time, you can probably notice it's much easier to mentally filter out the actual function logic from the boilerplate. Every check is stand-alone and always looks similar.

As a side note, if you don't like all that boilerplate there's an interesting article on the Go blog that discusses of possible ways to reduce it.


Inefficient (slow) loop with calculations in worksheet I am looking for best practice help. Slow loops seem to be a recurring problem for me and I'd like to learn a better way.  The code itself works as it should, except it is far too slow.

The problem is the worksheet needs to calculate after each B & i value is dropped into "N13" so that "U12", "V12", and "W12" update before being deposited into wsRepository. If I turn Calculation on Manual then my values are no good because they are contingent upon the other worksheet formulas updating (calculating). I think I can copy my worksheets "off-screen", calculate, and then paste my values back "on-screen", but I am not sure how to do this. I've used variants in the paste to do similar things, but I not comfortable with them. There may even be more efficient ways of achieving my desired result that I am unaware of.

Application.ScreenUpdating = False

Dim wsRepository As Worksheet
Dim wsInput As Worksheet
Dim i As Integer

Set wsRepository = ThisWorkbook.Sheets("Repository")
Set wsInput = ThisWorkbook.Sheets("Input")

For i = 4 To 2004

           'add investment amount
           wsInput.Range("N13").Value = wsRepository.Range("B" & i).Value

           'copy back amounts
           wsRepository.Range("E" & i).Value = wsInput.Range("U12").Value
           wsRepository.Range("C" & i).Value = wsInput.Range("V12").Value
           wsRepository.Range("D" & i).Value = wsInput.Range("W12").Value

    Next i

wsInput.Activate

Application.ScreenUpdating = False

I find it scary that the corresponding = True is nowhere in your code, for reasons already mentioned. Whenever I turn off screen updating, I find it's good UX to also specify a status bar message, and change the mouse cursor to a hourglass. Something along these lines:

Public Sub ToggleWaitMode(Optional ByVal waitMode As Boolean = False)
    Application.ScreenUpdating = waitMode
    Application.Calculation = IIf(waitMode, xlCalculationManual, xlCalculationAutomatic)
    Application.StatusBar = IIf(waitMode, "Please wait...", vbNullString)
    Application.Cursor = IIf(waitMode, xlWait, xlDefault)
End Sub

Which makes your procedure stub look like this:

Option Explicit

Public Sub DoSomething()
    On Error GoTo ErrHandler
    ToggleWaitMode True

    'do that thing

CleanExit:
    If Not Application.ScreenUpdating Then ToggleWaitMode
    Exit Sub
ErrHandler:
    ToggleWaitMode
    ' handle errors here
    Resume CleanExit
End Sub

  If I turn Calculation on Manual then my values are no good because...

If I understand properly, you need to update $N$13 some 2,000 times with a value that's in "$B$" & i, and then I guess $U$12, $V$12 and $W$12 need to be recalculated accordingly.

You haven't shown us what these cells contain and what cells their formula is referring to, but if they're the only cells that need to be recalculated when $N$13 changes, then you can force calculation like this:

wsInput.Range("$U$12").Calculate
wsInput.Range("$V$12").Calculate
wsInput.Range("$W$12").Calculate

But that might not speed up anything. You're pretty much stuck, since you need to recalculate these three cells before you can do anything, and you need to do that 2,000 times.

I think you're somewhat misusing VBA here, it looks like you could use 3 hidden columns (say, $AA$4:$AC$2004) and use Excel formulas to automatically calculate the would-be "U", "V" and "W" values for each row; the VBA macro could then just copy values from Input!$AA$4:$AC$2004 to Repository!$C$4:$E$2004.. if a macro is even needed for that.

I would suggest naming the ranges/cells in row 12 - anytime you have a specific cell with a specific meaning, it's always better for the VBA code to refer to the meaning rather than the cells' addresses.

I have no clue what these cells mean, but picture this:

Dim interestRate As Double
interestRate = wsInput.Range("InterestRate").Value

This extra abstraction level somewhat decouples the VBA code from the worksheet structure, which allows you to modify [at least parts of] the worksheet without having to modify the VBA code - for example you could insert another row and now InterestRate is read in row 13 instead of 12, and the VBA code couldn't care less.

You can define names in the [Formulas] Ribbon tab, under the [Defined Names] section. Or you can just select the cell and type its name in the address/names dropdown, just left of the formula bar.

This also has the advantage of making your Excel formulas more readable: instead of =$X$12*$N42 a formula can now look like =InterestRate*$N42

One last thing, I know it's common to call a worksheet variable like wsInput, but I find it sounds backwards and looks Hungarian. I'd call it inputSheet instead; wsRepository would be repositorySheet. Also it wouldn't hurt to rename i for row.


Is this IF ok? Or is there a better way? So, I wanted to avoid an

if (...) { }
else if (...) { }
else { }

scenario to avoid duplicating code. After a little playing, I got the following working (works from the testing i've done). Am wondering if anyone has any feedback on this approach or suggestions on on possible improvement?

Sometimes I think I know what I'm doing, sometimes I don't.

<?php

  // define("MEMBER_LIMIT",0); // no member limit
defined("MEMBER_LIMIT",100); // limit to 100 ppl in group

  // $memcount is set via sn SQL query
$memcount = 9; // number of people currently in group (let's say).

if (!defined("MEMBER_LIMIT") ||
 ((defined("MEMBER_LIMIT") && MEMBER_LIMIT == 0) ||
  (MEMBER_LIMIT > 0 && $memcount>0 && $memcount < MEMBER_LIMIT))) {
  // will do something
}
else {
  // will do something else.
}
?>

How about:

if (!defined("MEMBER_LIMIT") ||  MEMBER_LIMIT == 0 || $memcount < MEMBER_LIMIT) {
  // will do something
}
else {
  // will do something else.
}

The key is to look for both complementary (multiple items that accomplish the same thing by different means) and contradicting criteria (which would interrupt your desired flow of logic) in if statements


FormBuilder Code Make More Genereic I want to construct a form builder for an internal project in mvc4. I am using partial views to load the various controls. However I feel that having ten or 20 action-results in my HomeController would not be the best way to go.
So I am wondering how I could do the following more efficiently.

As you see here I have three different posts happening for three partials views anyway to get this down to one. And what way would be best to save the form generated below. I was thinking of putting each of the actions in a POCO class, which would store the names of the controls etc.

<input id="btnLoad" type="button" value="LoadRegistrationTemplate"/>
<input id="btnDropDown" type="button" value="Load DropDown"/>
<input id="txtarea" type="button" value="Text Area"/>

And also the same case in my HomeController I have the same three different Result Partials.

    [HttpPost]
    public ActionResult HtmlControlTest()
    {
        return PartialView("HtmlControlTest");
    }

    [HttpPost]
    public ActionResult DropDown()
    {
        return PartialView("DropDown");
    }

    [HttpPost]
    public ActionResult TextArea()
    {
        return PartialView("TextArea");
    }

I feel that this would be best method to go by as under each actionresult I could store the control info in a class. But I don't know how I would pass variables to the actionresult.

I would add "data-api" as an attribute to the buttons

<input type='button' data-api='Home/HtmlControlTest' />

Then you could do this with the js

$('input[data-api]').click(function () {

            $.ajax({
                url: $(this).attr('data-api'),
                type: 'POST',
                contentType: 'application/html',
                //data: JSON.stringify(model),
                success: function (content) {
                    $('#divPartialViewContainer').append(content);
                },
                error: function (e) { }
            });
        });


Number of Integer Pairs with a Specific Difference Problem

Adapted from this HackerRank Problem.

  Given N integers, count the number of pairs of integers whose difference is K.

Approach

Given the initial int array, sort it.
Now iterate through the indices in the array using a lower index (starting at 0) and an upper index (starting at 1)`.
Calculate the difference between the value at the upper index value and the value at the lower index.

If the difference is equal to the target difference, increment the count and increment the upper index.
If the difference is less than the target difference, increment the upper index. A difference less than the target
difference implies that the value at the upper index is not large enough.
If the difference is greater than the target difference, increment the lower index. A difference greater than the
target implies that the value at the lower index is not large enough.

Return the count

Implementation

public class PairDifferenceCounter {
    public static int countPairsWithDifference(int[] values, int difference) {
        Arrays.sort(values);

        int count = 0;
        int lowerIndex = 0;
        int upperIndex = 1;

        while (upperIndex < values.length) {
            int pairDifference = values[upperIndex] - values[lowerIndex];

            if (pairDifference == difference) {
                count++;
                upperIndex++;
            } else if (pairDifference > difference) {
                lowerIndex++;
            } else {
                upperIndex++;
            }
        }

        return count;
    }
}

Your solution looks good already. I can only really give 2 small pointers.

The first is based on mdfst13's comment. You don't do any checks on the input parameters.
Since it's for a programming challenge for which we can safely assume correct inputs this is somewhat fine. Although it would probably be better to at least add a comment block on top of the method stating those limitations.

Alternatively you could also check for unwanted inputs at the start of the method and throw a relevant exception. (For example IllegalArgumentException). You can still add a method comment on top to mention those.

Another way is to first sanitise the input parameters with the most logical correction. For example:

if(values == null) {
    values = new int[0];
}
if(difference < 0){
    differences *= -1;
}

That way, even if someone calls the method with invalid input parameters, the program will not crash. This kind of solution is great if you want to have a process running automatically for a long time without looking at it anymore, and don't care if some cases are handled "wrong"(/perfectly right?). The biggest disadvantage is that you don't know if an input is "corrected", so if it should have been handled differently it would be really hard to detect afterwards.

The second small pointer is that your class and method name remind me of this picture:

Since your class just has 1 method, naming it is obviously going to be tricky. I would just keep the Solution as classname like they usually do in hackerrank.

If you do keep the class name, I would at least change the method name. Now you repeat yourself if you call it: PairDifferenceCounter.countPairsWithDifference(someList, difference);

It reads a bit easier if you used something like this: PairDifferenceCounter.count(someList, differene).

Perhaps rename both into something like this: ExtendedListUtils.countPairs(someList, difference);
Which isn't perfect either since it might not be immediatly obvious that it counts the number of pairs with a given distance if you don't call it with a variable named "difference".

I want to stress out once more that these are only minor issues with the code. Well done!


Making a simple session class more secured I just wrote the session class below.  I humbly request for a review on how to make this even more secured/ how sessions are normally written. The code below works, but I would love to make it even more secure and robust.

<?php
session_start();

require_once('classes/function.php');
require_once('classes/user.php');

class Session{

    private $logged_in = FALSE;
    private $id ;
    private $user_data ;
    private $email ;

public function logout(){
    session_start();
    unset($_SESSION['id']);
    unset($this->id);
    session_destroy();
    $logged_in = FALSE;
    Misc::redirect('index.php');
}

public static function is_logged_in(){
    if(isset($_SESSION['id'])){
        $logged_in = TRUE;
        Misc::redirect('127.0.0.1/users/index.php');
    }
}

public static function login($email, $password){
    global $user;
    global $misc;

    if($misc->check_form()){
        echo $this->error;
    }
    if (!MIsc::check_email_format($email)) {
            echo "Email format is invalid";
          }
          $login_user = $user->authenticate($email, $password);
          if($login_user){
            $_SESSION['id'] = $this->id = $user->user_data['id'];
            $logged_in = TRUE;
            Misc::redirect('127.0.0.1/users/index.php');
          }
}
}

$session = new Session;
?>

You don't post the code that interacts with database, there is the main security concern. I hope you use PDO prepared statements, which will keep you safe from SQL Injections.

As for non-security-related problems, there are quite some of them.

Consider creating multiple functions for form submit processing, for example:

function process_form_submit($_POST)
{
    $input = $this->validate_user_input($_POST);
    if ($this->user->authenticate($input['email'], $input['password'])) {
        $this->log_user_in($this->user->getId());
    }
}

Then your functions

do one thing per function
are testable
are able to throw exceptions properly

As for exceptions, you don't just echo problems, do things like

throw new FormSubmitException('Email format is invalid');

and call process_form_submit like this:

try {
    $session->process_form_submit($_POST);
} catch (Exception $e) {
    echo $e->getMessage(); // or any other error handling
}

Why? I suppose your code is in the middle of the page (since you just echo all the errors), what if you want to move it to the top in order to change page title if login fails. Then your echo will show the error above page design and you will have to look for every single echo in all of classes used in order to fix this. Code like I provided will let you move it up and replace a single echo with $error = and the task will be done painlessly. Not even mentioning test-ability improvement.

Don't use global $var;, use Dependency injection instead in constructor or factory:

$session = new Session($user, $misc);

If you don't do that than your session class keeps depending on them implicitly and it makes you unable to just copy it into some other project. If you try - you will get errors here and there saying it is missing something that is not clear from it's initialization. Would not it be better to write a good class once in such a way that you can then use it in other projects?

As a last hint, start documenting your code with phpDocumentor standards in mind. It is a good habit and the sooner you start doing it, the easier it will be for yourself to work with code you write today in a few years, when you won't remember a thing about how it works.


How to handle multiple return statements I am trying to restructure a function more clearly and readable that has multiple return statements.
Usecase
Get a token:

if the token does not exist in the database then generate a new one and return it.
if the token exists  -check if expired, if expired update it with a new one,   -if not expired then return the same token.

Code
 export const GetToken = async () => {

        const db = new DB();

        const check = await db.GetItem();

        if(check.Count === 0) {
            const token = await GenerateToken();
            return token;
        }

        if(check.Count >= 1) {

            const db_token = check.Items[0];
            const isExpired = isTokenExpired(db_token);

            if(isExpired) {
                const token = await UpdateToken(db_token);
                return token;
            }
            return db_token;
        }
    }

There are 3 return statements in this function and there is not a base return function.

I'm no expert in JavaScript, but it seems to me if you changed the scope of token so that it is defined at the beginning of the function and then just modify the value as necessary that you can have only one return statement after all the if statements.


Multithreading synchronization between reading and writing in a channel I was hoping to get some feedback on what the recommended design pattern are for this subject.

public class Channel {

    private AtomicInteger buffer = new AtomicInteger(0);

    public void read() {
        // This function should keep blocking until there is something to read.

        while (buffer.get() == 0) {
            // Maybe a sleep? (Although NetBeans gives a warning for a sleep in a loop)
        }

        buffer.decrementAndGet();
    }

    public void write() {
        // This function should just write (nothing) to the buffer (by increasing the counter of the imaginary buffer).
        buffer.incrementAndGet();
    }
}

Is this safe for use in multithreading? Or is there a better approach?

if multiple threads try to read they may each decrement the buffer if one thread was interrupted right before it could decrement and after it passed the while

Instead use a Semaphore which is designed for just this:

public class Channel {

    private Semaphore sem = new Semaphore(0);

    public void read() throws InterruptedException {
        // This function should keep blocking until there is something to read.

        sem.acquire();
    }

    public void write() {
        // This function should just write (nothing) to the buffer (by increasing the counter of the imaginary buffer).
        sem.release();
    }
}

otherwise you should decrement in a loop:

int read;
do{
    while((read = buffer.get())==0){
    //sleep
    }
}while(!buffer.compareAndSet(read, read-1));


Perl code that breaks a line after fixed number of columns Here is a simple program that I created to print a file by breaking it after a fixed number of columns. I feel it can be optimized but not sure how. Any feedback is great.

#This is a simple program that takes a file and the limits the columns length to
#the number passed in as second argument
if ($#ARGV < 1) {
   printf("Usage: perl %s <name of the input file> <column length>\n", $0);
   exit(-1);
}
$file=$ARGV[0];
open(INFO, $file) or die("Could not open  file.");
$LINE_LENGTH = $ARGV[1];
$count = 0;
foreach $line (<INFO>)  {
    #print $line;
    if (length($line) <= $LINE_LENGTH) {
      printf("%s\n", $line);
    } else {
      $remaining_chars  = length ($line);
      $remaining_line = $line;
      while ($remaining_chars >= $LINE_LENGTH) {
          $cur_str = substr($remaining_line, 0, $LINE_LENGTH - 1 );
          printf("%s\n", $cur_str );
          $remaining_line = substr($remaining_line, $LINE_LENGTH - 1);
          $remaining_chars = $remaining_chars - length($cur_str);
      }
      printf("%s",$remaining_line );
    }
    if ($++counter == 2){
      last;
    }
}
printf("\n");
close(INFO);

I am not sure it can be optimized in a sense of better performance. It could be streamlined though. The initial test for length($line) <= $LINE_LENGTH is just redundant. Compare:

    while (length ($line) > LINE_LENGTH) {
        $prefix = substr($line, 0, $LINE_LENGTH - 1 );
        $line = substr($line, $LINE_LENGTH - 1);
        printf("%s\n", $prefix);
    }
    printf("%s",$line );

PS: I am not familiar with perl enough. If there is a split at position function, you may use it instead of calling substr twice.


jQuery Select Tabs based on URL #ID Activating jQuery tabs from URL #id is www.somedomain.com/index.php#tab3

Given this my best shot, and it does work but I feel it could be far far better.

Please tell me if this is totally shockingly bad JS.

$(document).ready(function() {
//When page loads...
$(".tab_content").hide(); //Hide all content
    //On Click Event
$("ul.tabs li").click(function (e) {
    e.preventDefault();
    $("ul.tabs li").removeClass("active"); //Remove any "active" class
    $(this).addClass("active"); //Add "active" class to selected tab
    $(".tab_content").hide(); //Hide all tab content
    var activeTab = $(this).find("a").attr("href"); //Find the href attribute value to identify the active tab + content
    $(activeTab).fadeIn(); //Fade in the active ID content
});
if(document.location.hash!='') {
    //get the index from URL hash
    var tabName = window.location.hash;
    $(tabName).show(); //Show tab from url hash
    var tabNumber = tabName.substr(4,1); //shorten #tab2 to just last digit "2"
    $("ul.tabs li:nth-child(tabNumber)").addClass("active").show();
} else {

    $("ul.tabs li:first").addClass("active").show(); //Activate first tab
    $(".tab_content:first").show(); //Show first tab content
}

});

Thank you

Well, you could do some refactoring with this code, the only issue I see is that the low level functinality is implemented at the top level of your code. It would be better to extract the logic to separate functions, something like this (I also improved your code formatting a little bit):

$((function() { // Deleted the "document).ready" part, that is not necessary.
    // Hide all content.
    $(".tab_content").hide();

    // If hash is present in the URL, show tab by hash. Otherwise, show the first tab.
    if(document.location.hash!='') {
        showTabByHash();
    }
    else {
        showFirstTab()
    }

    // On the click event of a tab.
    $("ul.tabs li").click(function (e) {
        e.preventDefault();
        showTab($this);
    });
});

function showTab(tab) {
    // Remove any "active" class.
    $("ul.tabs li").removeClass("active");

    // Add "active" class to selected tab.
    tab.addClass("active");

    // Hide all tab content
    $(".tab_content").hide();

    // Find the href attribute value to identify the active tab + content
    var activeTab = tab.find("a").attr("href");

    // Fade in the active ID content.
    $(activeTab).fadeIn();
}

function showTabByHash() {
    // Get the index from URL hash.
    var tabName = window.location.hash;
    // Shorten #tab2 to just last digit "2".
    var tabNumber = tabName.substr(4,1);

    // Why are these two lines needed? Don't they work with the same tab element?
    $(tabName).show(); //Show tab from url hash
    $("ul.tabs li:nth-child(tabNumber)").addClass("active").show();
}

function showFirstTab() {
    // Activate first tab.
    $("ul.tabs li:first").addClass("active").show();

    // Show first tab content.
    $(".tab_content:first").show();
}

I do not see any other problems with the code (of course, it could be generalized a little bit, but that is not your goal if I understand right), apart from your coding style, which was a little bit cluttered in my opinion (I had to read it more than one time to completely understand it).
And if you are developing in Visual Studio, and these functions are going to be used from somewhere else, then adding XML comments to them would generally be a good idea.


Implement Own Vector Class in C++ I am preparing for an interview and came to know about this question: implement a vector class in C++. I thought of how I would write it an interview and included the code below. The things I know I did not cover already are:
1) No use of templates for different data types
2) No use of iterators for iterating.
My point is I wanted to write a simple code and I want to know if that would be sufficient to crack the interview round or not?
Can you please go through the below code and point out the things that I must cover from an interview perspective. Thanks.

class Vector{
        int capacity;
        int sizet;
        int *arr;
    public:
        Vector():capacity(0),sizet(0),arr(new int){}
        Vector(int size):capacity(size),sizet(size),arr(new int[sizet]()){}
        Vector(const Vector &v){            //copy ctor
                capacity = v.capacity;
                sizet = v.sizet;
                arr = new int[sizet];
            for(int i=0;i<sizet;i++)
                arr[i] = v.arr[i];

        }
        int &operator [](int index){            //overloading index[] operator
            return arr[index];
        }
        Vector &operator==(const Vector& v){        //overloading assignment operator
        if(this != v){
                capacity = v.capacity;
                sizet = v.sizet;
                arr = new int[sizet];
            for(int i=0;i<sizet;i++)
                arr[i] = v.arr[i];
            }
            return *this;
        }
        void push_back(int elem){
            if(sizet == capacity){
                if(capacity ==0)
                    capacity++;
                else
                    capacity = 2*capacity;
            }
            arr[sizet++]=elem;
        }
        void pop_back(){
            sizet--;
        }
        void insert(iterator it,int size=1,int val=0){

        }
        int size(){
            return sizet;
        }
        void resize(int n){
        if(sizet < n){                  //if increasing the size
            for(int i=sizet;i<n;i++)
                arr[sizet++]=0;
        }
        else                            //decreasing the size
            sizet = n;
        }
        int at(int index){
            return arr[index];
        }
        int front(){
            return arr[0];
        }
        int back(){
            return arr[sizet];
        }

        ~Vector(){              //dtor
        delete arr[];
        }
    };

Edit: Resolved the naming conflict as was suggested.

You tagged c++11, but your code does not look like C++11. You do not implement move construction and move assignment. You should implement them. Here's a sample implementation using the copy-and-swap idiom.

friend void swap(Vector& a, Vector& b)
{
    using std::swap;
    swap(a.capacity, b.capacity);
    swap(a.sizet, b.sizet);
    swap(a.arr, b.arr);
}

Vector(Vector&& v)
    :Vector{}
{
    swap(*this, v);
}

Vector& operator=(Vector v)
{
    swap(*this, v);
    return *this;
}

This way, you don't have to define a separate copy assignment operator.
Your default constructor sets arr to new int. This makes no sense and cannot be handled by the destructor. You should set arr to nullptr.
Use ++i, not i++, in discarded-result expressions. See Difference between i++ and ++i in a loop?.
Consider using standard algorithms instead of hand-crafted loops when plausible. For example:

for (int i = 0; i < sizet; i++)
    arr[i] = v.arr[i];
}

Can be replaced by

std::copy_n(v.arr, sizet, arr);

You will need to #include <algorithm> for this to work.
Your copy constructor uses assignment instead of member initializer clauses. You should use member initializer clauses uniformly. Now your copy constructor should look like this:

Vector(const Vector& v)
    :capacity{v.capacity},
     sizet{v.sizet},
     arr{new int[v.sizet]} // to avoid dependence on member declaration order
{
    std::copy(v.arr, v.arr + sizet, arr);
}

Where is the const overload for operator[], at, front, and back?
Your push_back is incorrect. It does not allocate any memory. You will get an out-of-range error when capacity exceeds the actual capacity. Same applies to resize.
size() should be const.
Your at does the same job as operator[]. at should check for out-of-range errors and raise an exception if index >= sizet.
front and back should return a reference instead of a value.
The implementation of back is wrong. It should return arr[sizet - 1].


Data validation for to check required fields are mapped Can I break the below foreach and IF statements into  LINQ expression. Also, performance wise which will be better. Please suggest.

foreach (var fieldMappingOption in collectionHelper.FieldMappingOptions
        .Where(fmo => fmo.IsRequired && !fmo.IsCalculated
         && !fmo.FieldDefinition.Equals( MMPConstants.FieldDefinitions.FieldValue)
         && (implicitParents || anyParentMappings
             || fmo.ContainerType == collectionHelper.SelectedOption.ContainerType)))
    {
        if (!collectionHelper.FieldMappingHelpers
                .Any(fmh => fmh.SelectedOption.Equals(fieldMappingOption)))
        {
            requiredMissing = true;
            var message = String.Format(
               "The MMP column {0} is required and therefore must be mapped to a {1} column.",
               fieldMappingOption.Label, session.ImportSource.CollectionLabel);
            session.ErrorMessages.Add(message);
        }
    }

For readability I'd separate this into steps. Instead of doing your query within the declaration of your for loop, do it first.

var fieldMappingOptions = collectionHelper.FieldMappingOptions
    .Where(fmo =>
        fmo.IsRequired
        && !fmo.IsCalculated
        && !fmo.FieldDefinition.Equals(MMPConstants.FieldDefinitions.FieldValue)
        && (implicitParents || anyParentMappings
        || fmo.ContainerType == collectionHelper.SelectedOption.ContainerType))
    .ToArray();

var fieldMappingOptionsWithoutHelpers = fieldMappingOptions
    .Where(fmo => !collectionHelper.FieldMappingHelpers
            .Any(fmh => fmh.SelectedOption.Equals(fmo)));

foreach(var fieldMappingOption in fieldMappingOptionsWithoutHelpers)

What this achieves is that the reader can break this into multiple steps: First, which fieldMappingOptions you're looking for, and second, what you're going to do with them, instead of trying to read all of that at one time.

Any way you can limit how many things that future developer has to follow at the same time will help him or her to read and understand your code more quickly.

That also means that you can potentially break it apart as needed more easily. For example, if the method gets too long, you could separate the first two lines into their own function that returns IEnumerable<FieldMappingOption>.

Also, every time you break up these steps you're going to have to declare a variable or a function, and every time you do that you get to give it a name. If the names are meaningful then your code becomes more self-documenting. That future developer might also be able to skip over parts they don't care about more quickly while still having some idea what those parts are, based on the names of the methods called.

Even some of the conditions in the Where clause could potentially be functions if that makes it more readable.

Just as an example:

public static void RequiresValue(this FieldMappingOption fieldMappingOption)
{
    return fieldMappingOption.isRequired
      && !fmo.IsCalculated
      && !fmo.FieldDefinition.Equals(MMPConstants.FieldDefinitions.FieldValue);
}

Now your Where clause could read:

var fieldMappingOptions = collectionHelper.FieldMappingOptions
    .Where(fmo =>
        fmo.RequiresValue()
        && (implicitParents || anyParentMappings
        || fmo.ContainerType == collectionHelper.SelectedOption.ContainerType))
    .ToArray();

(I have no idea if I got the logic or intent right.)

But that's the gist of my recommendation. Keep LINQ as readable as possible, don't feel like the whole thing has to be in one statement, and keep methods short (15 lines or less.)


Golang implementation of Pascal's triangle How can I improve my code and make it more idiomatic?

package pascal

func NextRow(prevRow []int) []int {
    x := make([]int, len(prevRow)+1, len(prevRow)+1)
    x[0], x[len(prevRow)] = 1, 1
    for i := 0; i < len(prevRow)-1; i++ {
        x[i+1] = prevRow[i] + prevRow[i+1]
    }
    return x
}

func Triangle(x int) [][]int {
    y := [][]int{}
    for i := 0; i < x; i++ {
        if i == 0 {
            y = append(y, []int{1})
        } else {
            y = append(y, NextRow(y[i-1]))
        }
    }
    return y
}

It's interesting that you chose to put the code in its own package. I think it's great - but it does make the code a little more difficult to run as an experiment, or on ideone, or something. So, I encourage you to use packages, but remember that the main function/entry point will need to be in a different directory.

Now, as a package, there should be no reason to have an exported function NextRow. Why have you capitalized it? It should be nextRow.

Then, working through your functions, the next most striking thing is the poor choice of variable names. x's that are "lines", y's that are "triangles", etc. Worse, you use the variable x in two different ways in the two different functions. Renaming some variables made it easier to read.

Finally, the go fmt tool will re-format your code to conform to go conventions. You should always do this for your code. It helps when sharing go code with others.

With those changes, I would consider your code to be reasonably good.

On the other hand, I did mess a bit with the nextRow method to use append(..) instead of direct index assignment. I feel the append helps by making the 1 at the beginning and end of the line more obvious.

I also moved the special-case of the first row outside the loop of the Triangle method. This makes it clearer, I feel.

So, your major issues are code style, and naming. The actual code is otherwise OK. Some tweaks may help readability - but that's something you should decide whether it's better.

I put those changes together in a runnable format in ideone.

package main

import (
    "fmt"
    "strings"
)

func nextRow(prevRow []int) []int {
    current := make([]int, 0, len(prevRow)+1)

    current = append(current, 1)

    for i := 1; i < len(prevRow); i++ {
        current = append(current, prevRow[i-1]+prevRow[i])
    }

    current = append(current, 1)

    return current
}

func Triangle(rows int) [][]int {
    triangle := [][]int{}
    if rows <= 0 {
        return triangle
    }
    triangle = append(triangle, []int{1})
    for i := 1; i < rows; i++ {
        triangle = append(triangle, nextRow(triangle[i-1]))
    }
    return triangle
}

func present(line []int) string {
    words := make([]string, len(line), len(line))
    for i, v := range line {
        words[i] = fmt.Sprintf("%4v ", v)
    }
    return strings.Join(words, " ")
}

func main() {
    tri := Triangle(10)
    for _, line := range tri {
        fmt.Println(present(line))
    }
}


Parsing and extracting all IDs from a document (using regex with capture) I'm interested in parsing and extracting the IDs found in a document.  The IDs are found throughout the document in the this format:

  IDStart=1FG3392MQ9&IDEnd

The "1FG3392MQ9" is an example of such an ID (it will always appear between the IDStart and IDEnd tokens as in the above example). There will be text before and possibly after each occurrence and they could appear once on a given line or many times on a line.

Here's a utility function I have to parse and extract them:

Public Shared Function ParseALLDocumentIDs(text As String) As List(Of String)

   Dim allParsedIDs As New List(Of String)

   Dim IDregex As String = "IDStart=(.+?)&IDEnd"

   Dim matches As MatchCollection = Regex.Matches(text, IDregex, RegexOptions.IgnoreCase Or RegexOptions.Multiline)
   For Each m As Match In matches
       If m.Success Then
           Dim parsedID = m.GetFristCapture()
           If Not String.IsNullOrWhiteSpace(parsedID) Then
               allParsedIDs.Add(parsedID)
           End If
       End If
   Next

   Return allParsedIDs

 End Function

Upon each match, the following extension method is used (defined in another module) to get the first group capture of a match:

<Extension()>
    Public Function GetFristCapture(m As Match) As String

        ' Gets the first group capture "(...)" in a regex match.
        ' Returns empty string if not a match or there is no group capture found

        Dim capturedValue As String = ""

        If m IsNot Nothing AndAlso m.Success AndAlso m.Groups.Count > 1 Then
            ' Note: The first group is the entire match itself,
            ' so Group(1) is what we want, not Group(0)
            If m.Groups(1).Success Then

                capturedValue = m.Groups(1).Value.Trim

            End If
        End If

        Return capturedValue

    End Function

Questions:

How does the Regex look? Am I missing anything?  The idea was to capture all text between any occurrences of those two tokens, and do it non-greedily so that it doesn't capture across multiple tokens.
How does overall approach look with getting these IDs?  Is there a cleaner way?

Currently, you're extracting (non-greedily) everything appearing between the IDStart= and &IDEnd. Since this is an ID, and generally the IDs are stored in alphanumeric character sets; you can easily replace the .*? with a [a-z0-9]+ pattern. The & of the &IDEnd will take care of limiting your result between those two words alone.

However, if that is not your case, you should still replace the .*? with .+? so that you won't receive results/captures for a string like: IDStart=&IDEnd.

If the first rule applied to your case, then you can also remove the .Trim call from your GetFristCapture function.

NOTE: I assume that GetFristCapture is a typo. If not, you should rename it to GetFirstCapture.

The the GetFristCapture function itself, do not create a temporary variable. Just return from the if block itself:

Public Function GetFristCapture(m As Match) As String
    ' Gets the first group capture "(...)" in a regex match.
    ' Returns empty string if not a match or there is no group capture found
    If m IsNot Nothing AndAlso m.Success AndAlso m.Groups.Count > 1 Then
        ' Note: The first group is the entire match itself,
        ' so Group(1) is what we want, not Group(0)
        return m.Groups(1).Value
    End If
    Return ""
End Function

Note that I removed the nested if block as well. With your condition m.Groups.Count > 1, you're obviously getting Group(0) and Group(1). Just return Group(1) from there.


Selects all elements with a specific data attribute and equalizes their heights This script looks for a data attribute applied to elements within a specified wrapper.
It then sets all elements with the attribute to the same height (height of tallest div).
I needed to add a wait time as the script would sometimes get the heights of the divs before the page had fully rendered. Even though the await is set to 0, it works.

There is also an event listener to listen for resize, and adjust the elements as they change size.

I have only been coding JS for a few weeks so I am not great at it.

HTML

Some content

Some content

Some content

Some content

Script

function equalizer(wrap) {
  const equalizeEls = Array.from(wrap.querySelectorAll(`[data-equalize]`));
  const wait = (ms = 0) => new Promise(resolve => setTimeout(resolve, ms));

  //get the largest height in the array
  function getLargestNumber(arr) {
    return Math.max(...arr);
  }

  //get an array of heights
  function getTallest(arr) {
    const allheights = arr.map(x => x.clientHeight);
    return getLargestNumber(allheights)
  }

  function equalizeHeights(els) {
    const largest = getTallest(els);
    return els.forEach(el => {
       el.style.minHeight = `${largest}px`
    });
  }

  async function handleResize() {
    await wait(0); //needs a await to ensure page is fully loaded
    equalizeHeights(equalizeEls)
  }

  window.addEventListener('DOMContentLoaded', handleResize);
  window.addEventListener('resize', handleResize);
}

Running the script

new equalizer(document.querySelector(`#div1`))

data attribute

It's a bit unconventional to use a data attribute to select the affected elements. A class would be the more conventional approach.

Use of template strings

You have two places where you unnecessarily use template strings with backticks (`).

Code

I like the code overall very much. Nice break down into functions. Nice use of modern JS features.

Only one small thing: The return in equalizeHeights is unnecessary.

Timing

The problem with the need for the wait may be due to contents of the divs. If they contain images or use external fonts, then they may lead to resizing of the divs while these are loading. Some points to look out for:

Use the load event instead or additionally to DOMContentLoaded.
Make sure images have a size assigned to them, either in the HTML (height and width attributes) or the CSS.
There a ways to make sure that the text content isn't rendered until the fonts are loaded, but this is not really my area of expertise.

Possible bug/misfeature

There is one problem with the script: It never allows the divs to shrink if the content becomes shorter, for example due to a window resize.

I can think of two possible solutions:

Before checking the size of the divs, remove the min-heights and wait() to allow a re-render.

Or

add a second, inner div inside the equalize divs and use the inner divs client height to determine the largest div.


Design class to handle calls for support functions For the following problem statement, I design following classes.  Can you please tell me if this is the correct implementation or if there can be a better one?

Let's say there is call center with three levels of employees: fresher, technical lead (TL), product manager (PM). There can be multiple employees, but only one TL or PM. An incoming telephone call must be allocated to a fresher who is free. If a fresher can’t handle the call, he or she must escalate the call to technical lead. If the TL is not free or not able to handle it, then the call should be escalated to PM. Design the classes and data structures for this problem.

using System;
using System.Linq;
using System.Collections.Generic;

namespace TestNS
{
    public class Employee
    {
        public string Name { get; set; }
        public bool IsFree { get; set; }
        public Employee Manager { get; set; }
    }

    public class Call
    {
        public Employee AllocatedTo { get; set; }

        public bool HandleCall()
        {
            AllocatedTo.IsFree = false;
            bool isHandled = false;

            // Write custom logic to handle the call & set the value of isHandled

            if (isHandled)
            {
                AllocatedTo.IsFree = true;
            }

            return isHandled;
        }

        public void Escalate()
        {
            AllocatedTo = AllocatedTo.Manager;
            HandleCall();
        }
    }

    public class CallManager
    {
        List<Employee> fresherList = new List<Employee>();
        public void CreateEmployeeList()
        {
            Employee PM = new Employee() { Name = "PM1", IsFree = true };

            Employee TL = new Employee() { Name = "TL1", IsFree = true };
            TL.Manager = PM;

            fresherList = new List<Employee>();
            fresherList.Add(new Employee() { Name = "F1", Manager = TL, IsFree = true });
            fresherList.Add(new Employee() { Name = "F2", Manager = TL, IsFree = true });
            fresherList.Add(new Employee() { Name = "F3", Manager = TL, IsFree = true });
            fresherList.Add(new Employee() { Name = "F4", Manager = TL, IsFree = true });
            fresherList.Add(new Employee() { Name = "F5", Manager = TL, IsFree = true });
            fresherList.Add(new Employee() { Name = "F6", Manager = TL, IsFree = true });
        }

        private Employee FindFreeEmployee()
        {
            Employee e = null;
            foreach (Employee emp in fresherList)
            {
                if (emp.IsFree)
                {
                    e = emp;
                    break;
                }
            }

            return e;
        }

        public void ReceiveAndAllocateCall(Call call1)
        {
            call1.AllocatedTo = FindFreeEmployee();
            call1.HandleCall();
        }
    }
}

I like the Employee class with the built-in Manager chain.
I think Call class should be "dumb" like the Employee. The methods seem like call processing - what the CallManager should be doing.
Id say part of the "call handing process" is in the CallManager and part of it is in Call class. Broken apart like this it's hard to discern/follow the flow of a call-handling.
Should you implement queues? A queue of Calls to model lots of calls coming in before they can get allocated.
The code has allocation and "isFree" as 2 separate things. This suggests to me that we could have a Employee.CallQueue for allocated calls, then set IsFree = false when he picks up the phone.
Also an Employee.CallQueue of assigned calls. The code suggests that allocation and handling are 2 separate things
In ReceiveAndAllocateCall.FindFreeEmployee() could return null so we need code to handle that. Maybe that when the call goes into a waiting queue.

public class CallManager {
     . . .

    // call processing
    call1.AllocatedTo = FindFreeEmployee();

    // what to do if AllocatedTo is null?

    call1.AllocatedTo.IsFree = false;
    call1.IsHandled = false;

    // I see `IsHandled` as a `Call` property. It will persist if we
    // pass it to the manager. As is, `IsHandled` disappears when `HandleCall()` ends and we don't know if the call is handled anymore.

    while (! call1.IsHandled) {
     // Write custom logic to handle the call & set the value of isHandled
     if (! call1.isHandled) {
         if (call1.AllocatedTo.Manager != null) { call1.AllocateTo = AllocateTo.Manager
         }else{ break;}
      }
    } // while

    // if IsHandled == false still, what to do? Hang up and send them a lollypop I guess.

The 1st line, calling FindFreeEmployee() might best be done outside of, or somewhere else in CallManager Imagine a complex process for deciding how to pair calls to employees. Then pass that pair into CallManager, and all call manager does is process the call. - Separation of Responsibilities.