Using matlabpool: Is my embedding correct? A general question: For using matlabpool, I write the line

matlabpool open local 12;

as the first line in my code and the line:

matlabpool close;

as the last. Is this correct?

Seems to be fine according to the documentation.

The only things you need to try are whether 12 is the best number for you, and whether it makes sense to close the matlabpool earlier (you might not gain anything from it for a postprocessing part). Furthermore it is of course always good to check whether it actually gives the speed increase that you are looking for.


Circular Linked List implementation in C++ I want to improve this code.

#include <iostream>
#include <utility>

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

public:
  circular_linked_list() : head(nullptr) {}
  circular_linked_list(const circular_linked_list& cll) = delete; //copy constructor
  circular_linked_list(circular_linked_list&& cll) = delete; //move constructor
  circular_linked_list& operator=(const circular_linked_list& cll) = delete; //copy assignment
  circular_linked_list& operator=(circular_linked_list&& cll) = delete; //move assignment
  ~circular_linked_list();
  void insert_at_begin(T);
  void insert_at_end(T);
  void delete_node(T);
  void print_list();

private:
  struct Node *search(T value)
  {
    Node *node = head;
    while(node->next != head)
    {
      if(node->data == value)
        return node;

      node = node->next;
    }
    std::cerr << "No such element in the List \n";
    return nullptr;
  }
};

template <class T>
void circular_linked_list<T>::insert_at_begin(T value)
{
  Node *node = new Node(std::move(value));
  if(head == nullptr)
  {
    node->next = node;
    head = node;
    return;
  }
  Node *tmp = head;
  while(tmp->next != head)
  {
    tmp = tmp->next;
  }
  tmp->next = node;
  node->next = head;
  head = node;
}

template <class T>
void circular_linked_list<T>::insert_at_end(T value)
{
  Node *node = new Node(std::move(value));
  if(head == nullptr)
  {
    node->next = node;
    head = node;
    return;
  }
  Node *tmp = head;
  while(tmp->next != head)
  {
    tmp = tmp->next;
  }
  tmp->next = node;
  node->next = head;
}

template <class T>
void circular_linked_list<T>::delete_node(T value)
{
  Node *node = search(value);
  Node *tmp = head;
  Node *tail = head;
  while(tail->next != head)
  {
    tail = tail->next;
  }

  if(tmp == node)
  {
    head = tmp->next; //deleting first element
    tail->next = head;
  }
  else
  {
    while(node != nullptr)
    {
      if(tmp->next == node)
      {
        tmp->next = node->next;
        return;
      }
      tmp = tmp->next;
    }
  }
  delete tmp;
}

template <class T>
void circular_linked_list<T>::print_list()
{
  Node *tmp = head;
  while(tmp->next != head)
  {
    std::cout << tmp->data << ' ';
    tmp = tmp->next;
  }
  std::cout << tmp->data << '\n';
}

template <class T>
circular_linked_list<T>::~circular_linked_list()
{
  Node *tmp = nullptr;
  Node *tail = head;
  while(tail->next != head)
  {
    tail = tail->next;
  }
  tail->next = nullptr;

  while(head != nullptr)
  {
    tmp = head;
    head = head->next;
    delete tmp;
  }
}

int main()
{
  circular_linked_list<int> cll1;
  cll1.insert_at_end(2);
  cll1.insert_at_end(3);
  cll1.insert_at_end(4);
  cll1.insert_at_begin(1);
  cll1.insert_at_begin(0);
  cll1.print_list();
  cll1.delete_node(2);
  cll1.print_list();
}

Design

With a singly linked list it is O(n) to find the end (assuming you store the beginning). So it is often nice to track both the beginning and end of the list inside the list object.

Personally I would have gone with a doubly linked list (next/prev). That way you can store just the head but finding the tail (on a circular list) is simply getting the link before the head.

// Head of a list
list->head

// Tail of a list (doubly linked)
list->head->prev;

Another thing you should consider is a sentinel link. This is a link in the list that does not contain data but is always there. Note: The list is empty when it only contains the sentinel element. The advantage to using a sentinel node is that you don't have to worry about nullptr when traversing the list. This drastically reduces the amount of code you need to write.

So from that long description two changes I would make.

Doubly linked list next/prev
Add a sentinel node.

Code Review

Move and Copy

I see you deleted the copy/move operations. Fine if you are still working through it. But in the end you will need to implement these. These are actually quite simple when you learn the tricks (idioms).

As a simple helpping hand:

circular_linked_list(const circular_linked_list& cll)
// This you need to implement in full.
// I leave the details to you (but a full deep copy of the list is needed).

// The assignment operator is easy.
// It is implemented in terms of the copy constructor.
// And the swap operation.
circular_linked_list& operator=(const circular_linked_list& cll)
{
    circular_linked_list    copy(cll);   // Make a copy.
    swap(copy);                          // Call the swap method.
    return *this;
    // Small improvement can be made (look up copy and swap idiom).
}

// Move is easily implemented using a swap.
circular_linked_list(circular_linked_list&& cll) noexcept // dont forget the noexcept
     // Dont forget to initialize the members correctly.
 {
     // Swapping implements the requirements of move.
     // It also provided deferred deletion (which is a nice side affect)
     // allowing potential reuse of memory. But usually the object
     // will be correctly and normally destroyed.
     // It also provides the strong exception guarantee.
     swap(cll);
 }

 circular_linked_list& operator=(circular_linked_list&& cll) noexcept
 {
     swap(cll);
     return *this;
 }

 void swap(circular_linked_list& other) noexcept
 {
     using std::swap;
     // Implement the swap of all members for other into this.
 }

 friend void swap(circular_linked_list& lhs, circular_linked_list& rhs)
 {
      lhs.swap(rhs);
 }

Don't need an explicit constructor here.

  struct Node
  {
    T data;
    Node * next;
    Node(T value) : data(std::move(value)), next(nullptr) {}
  };

If you use the brace initializer you don't need an explicit initializer as long as you use put the members in order.

  Node* node = new Node{ 12, nullptr };

But you may want to explore some alternative constructors.

  // copy Cosntruct node.
  Node(T const& value);        // Pass by const reference and copy in
  Node(T&& value);             // Pass by r-value ref and move in
  template<typename... P>
  Node(P&&... args);           // Pass using the arguments that constructor
                               // T allowing construction in place.

Printing

This is fine.

  void print_list();

But in C++ we usually print using the operator<<. So it is nice to define this operator for your class. Also std::cout is not the only stream you can print too (Files/Strings are two that pop out but lot of things are emulated like streams).

  void print_list(std::ostream& str = std::cout) const;
  friend std::ostream& operator<<(std::ostream& str, circular_linked_list const& data)
  {
       data.print_list(str);
       return str;
  }

Perfectly good search.

  struct Node *search(T value)
  {
    Node *node = head;
    while(node->next != head)
    {
      if(node->data == value)
        return node;

      node = node->next;
    }

But this may have been easier to write as a for loop:

    for(Node *node = head; node->next != head; node = node->next)
    {
      if(node->data == value) {
        return node;
      }
    }

Insert

Both insert operations have a lot of shared code.
It would be better if you factor out the shared code into a separate function.

As noted in the design section. The code can be highly simplified by using a sentinel node. Also the loop to find the end should be removed by either using an end marker in the list or by using a doubly linked list.

Quick exit from delete.

If this returns nullptr then is there any reason to continue?

  Node *node = search(value);

Lot of extra work in delete.
I don't think you actually need tail.

Bug in print.

If the list is empty then your print is going to hit several places where it de-references a nullptr.

Destructor can be simplified.

You only need to loop over the list once to delete it. You don't need to unlink the tail from the head to make it work. Comparing pointers will still work after a pointer has been deleted. You just can't de-reference it after deleting.

template <class T>
circular_linked_list<T>::~circular_linked_list()
{
    Node* last = nullptr;
    Node* next
    for(Node* loop = head; loop != last; loop = next) {
        last = head;
        next = head->next;
        delete loop;
    }
}


Breaking group of items into rows then ensure consistent height of each row element I'm working on a product category page where the product images have inconsistent heights so I've been asked to improve the page appearance without changing the images.

The page contains x number of products in a 3 column grid.

I decided to set the height of the image wrapper to the height of the image with the maximum height in each row, then use flex to vertically center the images.

How could I improve my JavaScript?

/**
 *  Ensure all rows appear the same height even if they contain products with
 *  small images.
 */

const imgWrappers = Array.from(document.querySelectorAll('.summary-image-wrapper'));
const rows = [];

// 3 Products per row - First group the image wrappers into groups of 3
while (imgWrappers.length !== 0) {
    rows.push({
        elements: imgWrappers.splice(0, 3),
    });
}

// Then establish which element is the tallest in it's group
rows.forEach((row) => {
    let height = 0;

    row.elements.forEach((el) => {
        const currHeight = el.firstElementChild.getAttribute('height');
        height = currHeight > height ? currHeight : height;
    });

    row.height = height;

    // Set the height of each element to that of the tallest element in the row
    row.elements.forEach((el) => {
        el.style.height = `${row.height}px`;
    });
});

Example title

                 POA

EDIT:
I discovered an error within my original code due to string comparison. Take the following row of 3 images and their height:

Image a: 365

Image b: 91

Image c: 401

Given the above images using the code originally given 'image b' will be considered the largest. This is because el.firstElementChild.getAttribute('height') returns a string, when comparing strings javascript does it "alphabetically". This means that js looks at the first character of the string then the second and so on. When comparing two numberic strings if a string starts with a 9 it will always be the largest!

'10000000000' > '9' //false
'89999999999' > '9' //false
'91' > '9' //true

To ensure the original code works the value stored in currHeight needs to be converted to a number e.g currHeight = Number.parseInt(el.firstElementChild.getAttribute('height')); before comparison.

Keep it simple
Your algorithm is too complex, with too many iterations and uses too much memory. It can be simplified using only a single outer iteration to get a row, and the row's max height. Then an inner loop to set the new height values. (see rewrite)
Some points

document.querySelectorAll returns an iterable object. There is no need to convert it to an array. You can use for of or any of the iteration features on it. EG document.querySelectorAll("query string").forEach( and [...document.querySelectorAll("query string")]

You only need to use .getAttribute when the attribute is not part of the DOM. const height = element.height is more concise.

You can use the Math function max  to get the maximum of 2 or more values at a time. maxHeight = Math.max(maxHeight, element.height)

Don't pollute the global scope. Use a self invoking function to encapsulate your variables.  ;(()=>{ /* your code in here */ })();

Rewrite
;(() => {
    "use strict";
    const itemsPerRow = 3;
    const query = ".summary-image-wrapper";
    const minHeight = 100; // Maybe a min height just in case

    const row = [];
    var maxHeight = minHeight;

    document.querySelectorAll(query)
        .forEach((imgWrap, i, imgWrappers) => {
            row.push(imgWrap);
            maxHeight = Math.max(imgWrap.firstElementChild.height , maxHeight);

            if (row.length === itemsPerRow || i === imgWrappers.length - 1) {
                row.forEach(imgWrap => imgWrap.style.height = `${maxHeight}px`);
                maxHeight = minHeight;
                row.length = 0;
            }
        });
})();


The quest for a succint, readable expression of Next in a sequence I have a need to express something like this:

while(bytesToWrite > 0)
{
    // get the NEXT location to write from a Key/Value collection of locations

    // read a packet from a file at the specified location

    // write some bytes to the packet

    bytesToWrite -= bytesWritten
}

This was my first attempt:

var dictionary = new Dictionary<long, long>() { .. }
...

while(bytesToWrite > 0)
{
    var pin = dictionary.Take(1).First();
    Debug.Print(pin.Key.ToString() + ", " + pin.Value.ToString());

   // Write the bytes using Key and Value for location information.

    dictionary = dictionary.Skip(1)
    bytesToWrite -= bytesWritten;
}

But it doesn't look terribly efficient to me.  It looks like I'm making a new collection each time through the loop, just to peel off a single item, and the Take(1).First() and dictionary = dictionary.Skip(1) syntax is a bit awkward.

If I put my original collection in a LinkedList, I can do this:

var map = new LinkedList<KeyValuePair<long, long>>(dictionary);
...
var pin = map.First;
while (pin != null && bytesToWrite > 0)
{
    // Value.Key and Value.Value??
    Debug.Print(pin.Value.Key.ToString() + ", " + pin.Value.Value.ToString());

    // Write the bytes

    pin = pin.Next;
    bytesToWrite -= bytesWritten;
}

Eww.  Value.Key and Value.Value is not exactly intuitive.

I can write a couple of wrapper classes, like so:

public class StateList<T>
{
    LinkedList<T> list;
    LinkedListNode<T> currentNode;

    public StateList(IEnumerable<T> source)
    {
        list = new LinkedList<T>(source);
    }

    public StateList<T> Create<T>(IEnumerable<T> source)
    {
        return new StateList<T>(source);
    }

    public T First
    {
        get
        {
            var node = list.First;
            return Value(node);
        }
    }

    public T Last
    {
        get
        {
            var node = list.Last;
            return Value(node);
        }
    }

    public T Next
    {
        get
        {
            var node = currentNode.Next;
            return Value(node);
        }
    }

    public T Previous
    {
        get
        {
            var node = currentNode.Previous;
            return Value(node);
        }
    }

    public T Value(LinkedListNode<T> node)
    {
        if (node != null)
        {
            var value = node.Value;
            currentNode = node;
            return value;
        }
        return default(T);
    }
}

public static class StateList
{
    public static StateList<T> Create<T>(IEnumerable<T> source)
    {
        return new StateList<T>(source);
    }
}

and get the code look like this:

var map = StateList.Create(dictionary);
...
var pin = map.First;
while (bytesToWrite > 0)
{
    // Ah, that's better.
    Debug.Print(pin.Key.ToString() + ", " + pin.Value.ToString());

    // Write the bytes

    // Pin is taken from the map, not from the previous pin.
    pin = map.Next;
    bytesToWrite -= bytesWritten;
}

Ah, that's much better.  The question is, is all that ceremony really worth it?  I feel like I'm missing something simple and obvious.

I'd also like comments on the relative merit of the StateList class, whether it is a satisfactory approach from a good practices viewpoint, or a terrible abomination.

It sounds like what you want is a foreach over the Dictionary, combined with a break for the bytesToWrite condition.

foreach (var pin in dictionary)
{
    if (bytesToWrite <= 0)
        break;

    Debug.Print(pin.Key + ", " + pin.Value);

    // Write the bytes using Key and Value for location information.

    bytesToWrite -= bytesWritten;
}


My implementation of (open) Knight's Tour This is my implementation of the (open) Knight's Tour on a 5v5 board. My original assignment for CS was to solve the Knight's Tour from any startings position (0,0 -> 4,4). The goal for myself was to make this class as clean as it could be. I would like some feedback (and constructive criticism!) on the code and its performance.

I have used the StdDraw class from Princeton to display the 5v5 board with graphics.

private final int N;
private final int startX;
private final int startY;
private boolean[][] visited;
private boolean done;
private final int[][] moves = {{1, -2}, {2, -1}, {2, 1}, {1, 2}, {-1, 2},
        {-2, 1}, {-2, -1}, {-1, -2}}; //x, y.
private int[][] board;
private int total;

public Board(final int N, final int startX, final int startY) {
    this.N = N;
    this.startX = startX;
    this.startY = startY;
    visited = new boolean[N][N];
    board = new int[N][N];
    init();
}

private void init() {
    for (int x = 0; x < N; x++) {
        for (int y = 0; y < N; y++) {
            visited[x][y] = false;
            board[x][y] = -1;
        }
    }
    done = false;
    total = N * N;
}

public boolean solve() {
    board[startX][startY] = 0;
    return solve(startX, startY, 0);
}

private boolean solve(int x, int y, int currentMove) {
    if ((x < 0) || (x >= N) || (y < 0) || (y >= N)) return false;
    if (done || visited[x][y]) return false;

    visited[x][y] = true;
    board[x][y] = currentMove;

    final List<int[]> moves = movesList(x, y);

    if (moves.isEmpty())
        return false;

    if (hasVisitedAll()) {
        done = true;
    }

    StdDraw.setPenColor(Color.BLUE);

    if (x == startX && y == startY)
        StdDraw.setPenColor(StdDraw.RED);

    StdDraw.filledCircle(x + 0.5, y + 0.5, 0.25);
    StdDraw.show(1_000); //1sec per next move

    for (final int[] m : moves) {
        int x2 = m[0];
        int y2 = m[1];

        if (solve(x2, y2, currentMove + 1)) {
            board[x2][y2] = currentMove + 1;
            return true;
        } else if (isLegitMove(x2, y2)) {
            visited[x2][y2] = false;
        }
    }
    return done;
}

private boolean hasVisitedAll() {
    int count = 0;
    for (final int[] v : board) {
        for (final int v2 : v) {
            if (v2 >= 0) count++;
        }
    }
    return (count == total);
}

private List<int[]> movesList(final int x, final int y) {
    final List<int[]> move = new ArrayList<>();
    for (int[] m : moves) {
        int x2 = m[0];
        int y2 = m[1];
        move.add(new int[]{x + x2, y + y2});
    }
    return move;
}

private boolean isLegitMove(final int x, final int y) {
    return ((x > 0 && x < N)) && ((y > 0 && y < N));
}

final List<int[]> moves local to solve shadows private final int[][] moves member. While it may not affect the functionality, it surely makes code harder to understand.
I don't see how moves.isEmpty() could ever be satisfied.
Dealing with visited squares looks suspiciously redundant:

The all squares visited condition is directly derived from currentMove value: currentMove == total, so there is no need to scan the entire visited array.
Similarly, a square visited condition could be implicitly derived from the corresponding board[][] value. Initially, it is -1 for unvisited squares - and you may maintain this invariant by resetting the board[][] to -1 instead of setting visited[][] to false.

This means that you may eliminate visited[][] array.
An x > 0 condition in isLegitMove looks strange, as if the indexing starts with 1 (according to the rest of the program logics it starts with 0). I am not sure I understand why x == 0 makes the move illegitimate. Looks like a potential bug.


Scraping the full content from a lazy-loading webpage I've written a script in python in combination with selenium which is able to scrape 1000 links from a webpage in which lazy-loading method is applied for that reason it displays it's content 20 at a time and full content can only be seen when it is made to scroll downmost. However, my script can scroll the webpage to the end. After collecting the 1000 links from main page, it then gets to each individual link to scrape the Name of CEO and Web address of that organization. It is working great now. I tried to make the whole thing accordingly. Here is the full code:

from selenium import webdriver
import time

def get_links(driver):

    driver.get('http://fortune.com/fortune500/list/')

    while True:
        driver.execute_script("window.scrollTo(0, document.body.scrollHeight);")
        time.sleep(3)

        links = [posts.get_attribute("href") for posts in driver.find_elements_by_xpath("//li[contains(concat(' ', @class, ' '), ' small-12 ')]//a")]
        if (len(links) == 1000):
                break

    for link in links:
        process_links(driver, link)

def process_links(driver, sub_links):

    driver.get(sub_links)

    for items in driver.find_elements_by_xpath('//div[contains(@class,"company-info-card-table")]'):
        Name = items.find_element_by_xpath('.//div[contains(@class,"small-7")]/p[@class="remove-bottom-margin"]')
        Web = items.find_element_by_xpath('.//div[contains(@class,"small-9")]/a')

        print(Name.text, Web.get_attribute("href"))

if __name__ == '__main__':

    driver = webdriver.Chrome()
    try:
        get_links(driver)
    finally:
        driver.quit()

Code Flow:

you are redefining links on every iterations of the while loop - you basically need to do it once
as a while loop exit condition, we can use the fact that there are line numbers in the company list grid - we can simply wait for the number 1000 to show up while scrolling
I would also create a class to have the driver and WebDriverWait instance shared across the class instance methods
instead of a hardcoded 3 second delay, use an Explicit Wait with a condition of the last line number to change - this would be much faster and reliable overall

Code Style:

posts variable name does not actually correspond to what it is - name it company_link instead
Name and Web violate PEP8 Python naming guidelines
process_links should be process_link - since you are processing a single link at a time. And, actually, we can name it get_company_data and let it return the data instead of printing it

Locating Elements:

don't use XPaths to locate elements - they are generally the slowest and the least readable
for the company links, I'd better use a more readable and concise ul.company-list > li > a CSS selector
in the process_links method you don't actually need a loop since there is a single company being processed. And, I think, you can generalize and return a dictionary generated from a company web page data dynamically - data labels to data values

Here is the modified working code:
from pprint import pprint

from selenium import webdriver
from selenium.webdriver.common.by import By
from selenium.webdriver.support.wait import WebDriverWait
from selenium.webdriver.support import expected_conditions as EC

class Fortune500Scraper:
    def __init__(self):
        self.driver = webdriver.Chrome()
        self.wait = WebDriverWait(self.driver, 10)

    def get_last_line_number(self):
        """Get the line number of last company loaded into the list of companies."""
        return int(self.driver.find_element_by_css_selector("ul.company-list > li:last-child > a > span:first-child").text)

    def get_links(self, max_company_count=1000):
        """Extracts and returns company links (maximum number of company links for return is provided)."""
        self.driver.get('http://fortune.com/fortune500/list/')
        self.wait.until(EC.visibility_of_element_located((By.CSS_SELECTOR, "ul.company-list")))

        last_line_number = 0
        while last_line_number < max_company_count:
            self.driver.execute_script("window.scrollTo(0, document.body.scrollHeight);")

            self.wait.until(lambda driver: self.get_last_line_number() != last_line_number)
            last_line_number = self.get_last_line_number()

        return [company_link.get_attribute("href")
                for company_link in self.driver.find_elements_by_css_selector("ul.company-list > li > a")]

    def get_company_data(self, company_link):
        """Extracts and prints out company specific information."""
        self.driver.get(company_link)

        return {
            row.find_element_by_css_selector(".company-info-card-label").text: row.find_element_by_css_selector(".company-info-card-data").text
            for row in self.driver.find_elements_by_css_selector('.company-info-card-table > .columns > .row')
        }

if __name__ == '__main__':
    scraper = Fortune500Scraper()

    company_links = scraper.get_links(max_company_count=100)
    for company_link in company_links:
        company_data = scraper.get_company_data(company_link)
        pprint(company_data)
        print("------")

Prints:
{'CEO': 'C. Douglas McMillon',
 'CEO Title': 'President, Chief Executive Officer & Director',
 'Employees': '2,300,000',
 'HQ Location': 'Bentonville, AR',
 'Industry': 'General Merchandisers',
 'Sector': 'Retailing',
 'Website': 'www.walmart.com',
 'Years on Fortune 500 List': '23'}
------
{'CEO': 'Warren E. Buffett',
 'CEO Title': 'Chairman & Chief Executive Officer',
 'Employees': '367,700',
 'HQ Location': 'Omaha, NE',
 'Industry': 'Insurance: Property and Casualty (Stock)',
 'Sector': 'Financials',
 'Website': 'www.berkshirehathaway.com',
 'Years on Fortune 500 List': '23'}
------
...


Infix to postfix implementation using a stack I have implemented a "infix-to-postfix" function that takes as input a list representing the parsed expression, resulted from applying a certain regular expression to an expression, and I would like to have your opinion regarding its correctness and efficiency. Good suggestions are of course more than well-accepted!

For making you able also to test directly my algorithm, I decided to include also the regular expression and other important details.

import re

# higher number => higher precedence
OPERATORS  = {
    "+": 1,
    "-": 1,
    "*": 2,
    "/": 2,
    "%": 2,
    "^": 3
}

PARENTHESIS = set(["(", ")"])

REGEX = "(\d+|\w+|[()\-+*/^%])"

def parse(e):
    """Parses a string expression e to a infix represation list."""
    return re.findall(REGEX, e)

def infix_to_postfix(infix):
    stack = []
    postfix = []

    for c in infix:
        if c in OPERATORS:

            if len(stack) > 0:
                top = stack[-1]

                if top in OPERATORS.keys():
                    if OPERATORS[c] > OPERATORS[top]:
                        stack.append(c)
                    else:
                        while top in OPERATORS.keys() and OPERATORS[top] >= OPERATORS[c]:
                            op = stack.pop()
                            postfix.append(op)

                            if len(stack) > 0:
                                top = stack[-1]
                            else:
                                break
                        stack.append(c)
                else:
                    stack.append(c)
            else:
                stack.append(c)

        elif c in PARENTHESIS:

            if c == ")":
                if len(stack) > 0:
                    top = stack[-1]

                    while top != "(":
                        try:
                            # pop throws an IndexError if the list is empty
                            r = stack.pop()
                            postfix.append(r)  # Adding what's in between ( ) to the postfix list
                            top = stack[-1]
                        except IndexError:
                            raise ValueError("'(' not found when popping")

                    stack.pop()  # Removes ( from the top of the stack
                else:
                    raise ValueError("')' cannot be added to the stack if it is empty")
            else:
                stack.append(c)  # c == '('
        else:

            postfix.append(c)

        #print("Stack:", stack)
        #print("Postfix:", postfix)

    while len(stack) > 0:
        top = stack.pop()

        if top in OPERATORS.keys():
            postfix.append(top)

    return postfix

Please, don't tell me anything about the docstrings, because I actually removed them from my code to make it clearer.

In general, I prefer efficiency to clarity, so I am not so interested in list or generator comprehensions if they don't make the code more efficient. Anyway, you could also suggest them, if you feel that they would turn the code more "pythonic".

The first thing I notice is the deep nesting in infix_to_postfix. Deep nesting like that is generally undesirable. We'll get to that later.

Second, for the parenthesis, my instinct would be to say: "Use frozenset instead of set.". But after some benchmarking: set is actually fastest on my machine, so probably also on yours. (I also checked using a tuple instead, again slower than using a set). However, a more Pythonic declaration of the set would be PARENTHESIS = {"(", ")"}.

Regarding the regular expression and the method parse: it could be faster by using a compiled regex instead of using re.findall. Also, convention has it that you use raw strings for regular expressions.

REGEX = re.compile(r"(\d+|\w+|[-+*/^%()])")
def parse(e, REGEX=REGEX):
    return REGEX.findall(e)

The REGEX=REGEX trick is an optimization trick preventing a lookup to the global dictionary. Using compiled regular expressions also gains a bit of speed. (On the other hand: I don't see parse being used, so that's probably ok).

Since it's not documented, I'm going to assume the calling convention will be

postfix_tokens = infix_to_postfix(parse(infix_tokens)

Now, let's start analysing the large method infix_to_postfix. First of all, we could apply the same trick as we did for REGEX to speed up the lookup for OPERATORS. That's going to make your code a bit more efficient.

It's a tad large, so let's first talk about the if c in OPERATORS branch.

if len(stack) > 0:
    top = stack[-1]

    if top in OPERATORS.keys():
        if OPERATORS[c] > OPERATORS[top]:
            stack.append(c)
        else:
            while top in OPERATORS.keys() and OPERATORS[top] >= OPERATORS[c]:
                op = stack.pop()
                postfix.append(op)

                if len(stack) > 0:
                    top = stack[-1]
                else:
                    break
            stack.append(c)
    else:
        stack.append(c)
else:
    stack.append(c)

All these else conditions look the same. Let's see if we can simplify it a bit. Let's start by flipping the if and else branch in if OPERATORS[c] > OPERATORS[top]. (> becomes <=).

if len(stack) > 0:
    top = stack[-1]

    if top in OPERATORS.keys():
        if OPERATORS[c] <= OPERATORS[top]:
            while top in OPERATORS.keys() and OPERATORS[top] >= OPERATORS[c]:
                op = stack.pop()
                postfix.append(op)

                if len(stack) > 0:
                    top = stack[-1]
                else:
                    break
            stack.append(c)
        else:
            stack.append(c)
    else:
        stack.append(c)
else:
    stack.append(c)

Now, we have the following construct:

if cond:
    A()
    B()
 else:
    B()

Which we can rewrite to

if cond:
    A()
 B()

Applying that several times we get

if len(stack) > 0:
    top = stack[-1]

    if top in OPERATORS.keys():
        if OPERATORS[c] <= OPERATORS[top]:
            while top in OPERATORS.keys() and OPERATORS[top] >= OPERATORS[c]:
                op = stack.pop()
                postfix.append(op)

                if len(stack) > 0:
                    top = stack[-1]
                else:
                    break

stack.append(c)

Wow, we saved a few lines of code. If we're lucky, it's also going to gain us some performance.

Next, you have top in OPERATORS.keys(). This can be replaced by top in OPERATORS (as that's how dict containment checking works). Furthermore, len(stack) > 0 can be replaced by stack.

Furthermore, you write op = stack.pop() followed by postfix.append(op). You can simplify this to postfix.append(stack.pop()).

if stack:
    top = stack[-1]

    if top in OPERATORS:
        if OPERATORS[c] <= OPERATORS[top]:
            while top in OPERATORS and OPERATORS[top] >= OPERATORS[c]:
                postfix.append(stack.pop())

                if stack > 0:
                    top = stack[-1]
                else:
                    break

stack.append(c)

Let's continue our focus on the nested if top in OPERATORS. Now, for symmetry, we replace OPERATORS[c] <= OPERATORS[top] with OPERATORS[TOP] >= OPERATORS[C]. (I'm not showing the if stack: and stack.append(c) anymore).

if top in OPERATORS:
    if OPERATORS[top] >= OPERATORS[c]:
        while top in OPERATORS and OPERATORS[top] >= OPERATORS[c]:
            postfix.append(stack.pop())

            if stack > 0:
                top = stack[-1]
            else:
                break

We can merge the first two conditionals, because there are no else branches, using

if cond_a:
    if cond_b:
        ...

to

if cond_a and cond_b:
    ...

We get

if top in OPERATORS and OPERATORS[top] >= OPERATORS[c]:
    while top in OPERATORS and OPERATORS[top] >= OPERATORS[c]:
        postfix.append(stack.pop())

        if stack > 0:
            top = stack[-1]
        else:
            break

Now the similarity should be obvious, we can drop the if.

Zooming out again:

def infix_to_postfix(infix):
    stack = []
    postfix = []

    for c in infix:
        if c in OPERATORS:

            if stack:
                top = stack[-1]

                while top in OPERATORS and OPERATORS[top] >= OPERATORS[c]:
                    postfix.append(stack.pop())

                    if stack:
                        top = stack[-1]
                    else:
                        break

            stack.append(c)

        elif c in PARENTHESIS:

            if c == ")":
                if len(stack) > 0:
                    top = stack[-1]

                    while top != "(":
                        try:
                            # pop throws an IndexError if the list is empty
                            r = stack.pop()
                            postfix.append(r)  # Adding what's in between ( ) to the postfix list
                            top = stack[-1]
                        except IndexError:
                            raise ValueError("'(' not found when popping")

                    stack.pop()  # Removes ( from the top of the stack
                else:
                    raise ValueError("')' cannot be added to the stack if it is empty")
            else:
                stack.append(c)  # c == '('
        else:

            postfix.append(c)

        #print("Stack:", stack)
        #print("Postfix:", postfix)

    while len(stack) > 0:
        top = stack.pop()

        if top in OPERATORS.keys():
            postfix.append(top)

    return postfix

The first branch of the conditional is cleaned up nicely. Let's consider the second branch, most specifically the handling of ")".

You're checking if len(stack) > 0, and throwing a ValueError otherwise because of an empty stack. What you actually mean is "')' cannot be added to the stack if there is no '(' in the stack". That's already checked by the other branch, so this condition is superfluous. However, we can't just remove it just yet, because top = stack[-1] might then throw an IndexError. So, let's continue and see if we get another opportunity to fix it later.

In the while loop, we see a try/except. The except has a raise, so we might as well move it outside the loop. (I took the liberty to also remove the temporary variable r).

top = stack[-1]
try:
    while top != "(":
        postfix.append(stack.pop())
        top = stack[-1]
except IndexError:
    raise ValueError("'(' not found when popping")

stack.pop()  # Removes the ( from the top of the stack

Looking at efficiency: you're storing stack[-1] in a temporary variable, and then loading it again for the while condition. We can fix that

try:
    while stack[-1] != "(":
        postfix.append(stack.pop())
except IndexError:
    raise ValueError("'(' not found when popping")

stack.pop()  # Removes the ( from the top of the stack

Remember the if we wanted to remove? We can actually do that now!

        ...
        elif c in PARENTHESIS:

            if c == ")":
                try:
                    while stack[-1] != "(":
                        postfix.append(stack.pop())
                except IndexError:
                    raise ValueError("'(' not found when popping")

                stack.pop()  # Removes ( from the top of the stack
            else:
                stack.append(c)  # c == '('
        else:
            ...

The code is now shaping up nicely. Let's consider the final while loop. Here you're poping until it's empty. Why not just loop over it in reverse? (The stack is going to be destructed anyway). Iteration is probably cheaper than mutation.

    for token in reversed(stack):
        if token in OPERATORS.keys():
            postfix.append(token)

In fact, it would probably be more efficient to use a generator comprehension here.

     postfix.extend(token for token in reversed(stack) if token in OPERATORS)

In the end, we end up with

def infix_to_postfix(infix):
    stack = []
    postfix = []

    for c in infix:
        if c in OPERATORS:

            if stack:
                top = stack[-1]

                while top in OPERATORS and OPERATORS[top] >= OPERATORS[c]:
                    postfix.append(stack.pop())

                    if stack:
                        top = stack[-1]
                    else:
                        break

            stack.append(c)

        elif c in PARENTHESIS:

            if c == ")":
                try:
                    while stack[-1] != "(":
                        postfix.append(stack.pop())
                except IndexError:
                    raise ValueError("'(' not found when popping")

                stack.pop()  # Removes ( from the top of the stack
            else:
                stack.append(c)  # c == '('
        else:

            postfix.append(c)

        #print("Stack:", stack)
        #print("Postfix:", postfix)

    postfix.extend(token for token in reversed(stack) if token in OPERATORS)

    return postfix

And we can probably shave some extra lines off it even more, but I don't think that's necessary because now it is a bit more understandable.

As for the points @GarethRees mentioned: those should also be taken care of, as it does not make much sense like this. Probably best to ask for a new code-review after you made these changes.


Loop termination check for multiple types of host reachability tests I have 2 booleans: host.isReachableOnLan, and host.isReachableOnWan

I have a listener onItemClick doing work on network on button event

Then I have an anonymous thread than runs a while loop, evaluating the booleans each iteration with the evalFlag method.

This works, but feels hackish and wrong. Is there a more elegant way of accomplishing the same goal? Hopefully one without my evalFlag method.

I would also like to combine the last two if statements at the top of my onItemClick method, because they feel redundant, but they're there for evalFlag.

Host host;

private boolean evalFlag(int type) {
    switch (type) {
    case 0:
        return !host.isReachableOnLan && !host.isReachableOnWan;
    case 1:
        return host.isReachableOnLan;
    case 2:
        return host.isReachableOnWan;
    }
    return false;

}

@Override
public void onItemClick(AdapterView<?> arg0, View arg1, int arg2, long arg3) {
    host = MainActivity.mainActivity.listAdapter.getItem(arg2);
    int tmp = -1;

    if (!host.isReachableOnLan && !host.isReachableOnWan) {
        new WakeOnLan(host, host.wolBroadcast).start();
        new WakeOnLan(host, host.wanHostName).start();
        tmp = 0;
    }

    else if (host.isReachableOnLan) { // want to combine this statement
        new ExecuteSshCommand(host, host.sshShutdownCommand).start();
        tmp = 1;
    }

    else if (host.isReachableOnWan) { // with this one
        new ExecuteSshCommand(host, host.sshShutdownCommand).start();
        tmp = 2;
    }

    final int type = tmp;
    new Thread() {
        @Override
        public void run() {
            MainActivity.mainActivity.swipeRefreshLayout.setRefreshing(true);
            host.isLoading = true;

            MainActivity.mainActivity.runOnUiThread(new Runnable() {
                @Override
                public void run() {
                    MainActivity.mainActivity.listAdapter.notifyDataSetChanged();
                }
            });
            int timeSum = 0;
            HostCheck checker = new HostCheck(MainActivity.mainActivity, host, false);

            while (evalFlag(type) && timeSum < checker.loopTimeout) {
                final long startTime = System.currentTimeMillis();
                checker.run();
                if(host.isReachable())
                    try {
                        sleep(500);
                    } catch (InterruptedException e) {
                        // TODO Auto-generated catch block
                        e.printStackTrace();
                    }
                timeSum += (int) (System.currentTimeMillis() - startTime);
            }
            MainActivity.mainActivity.runOnUiThread(new Runnable() {
                @Override
                public void run() {
                    MainActivity.mainActivity.listAdapter.notifyDataSetChanged();
                }
            });

            MainActivity.mainActivity.swipeRefreshLayout.setRefreshing(false);
            host.isLoading = false;
        }
    }.start();

Good names make everything better

public void onItemClick(AdapterView<?> arg0, View arg1, int arg2, long arg3) {

This signature was clearly generated by a tool. Even though you only need one of the arguments and it is used on the first line of the function doesn't mean the arguments shouldn't have good names.

What happens if I need to use the long argument later? I haven't been in this code for a while, but I just have to add this one simple line. I'm pretty sure arg2 was the long. Oops, arg3 is the long and the int gets implicitly converted and the complier doesn't care. If the arguments had real names, it would be much harder to make this mistake.

That is some what of a contrived example because you could just look at the signature to see that arg3 is the long. But how about this. I haven't had to implement an OnItemClickListener before. I don't know what the arguments are supposed to mean. Why is there a long and an int? Since you didn't take the time to give the arguments real names, I need to look at the documentation to see what each argument means.

public void onItemClick(AdapterView<?> parent, View view, int position, long id) {

Seeing that gives me a good idea of the context. Then only if I need more information do I have to look at the documentation.

type also doesn't help me know anything about what this variable is doing. Add the fact that it is an int and there tons of possible values, how do I know what the right value is and I use it properly?

enum HostConnectivity { Lan, Wan, Both }

// ...

final HostConnectivity connectionType = tmp;

Now it is clearer what kind of type we are dealing with and what each value means. This also removes the magic numbers from evalFlag(). It makes it easy to see if the switch does the wrong check for one of the cases.

Using tmp can be acceptable when it is a short lived variable that actually is temporary. (See the middle section to see how it can be removed and solve another problem.)

evalFlag() is a very generic name. It sounds like it is doing some operation on the flag variable. But there is no flag variable. Also, flags are generally throughout of as boolean, either set or not. Your type has multiple states.

What the method is doing is performing a check of the host's connectivity. it happens to be using the type to decide what checks to perform. The while loop isn't concerned with how the type is being used, it cares about what the boolean result. checkHostConnetivity() would be a much better name.

Sub-functions make life easier

Extracting the type selection code into a sub-function.

private HostConnectivity chooseConnectivity() {
    if (!host.isReachableOnLan && !host.isReachableOnWan) {
        return HostConnectivity.Both;
    }
    else if (host.isReachableOnLan) {
        return HostConnectivity.Lan;
    }
    else {
        return HostConnectivity.Wan;
    }
}

Now there is no need for a temporary variable. Also, by separating the choosing from the initialization, you can remove the repeated code that you had a comment about wanting to combine.

final HostConnectivity connectionType = chooseConnectivity();
switch(connectionType) {
  case Both:
    new WakeOnLan(host, host.wolBroadcast).start();
    new WakeOnLan(host, host.wanHostName).start();
    break;
  case Wan:
  case Lan:
    new ExecuteSshCommand(host, host.sshShutdownCommand).start();
    break;
}

Instances never die

There are a number of times when you create a new instance, never store the reference, and just start the operation. You should be careful with this. You need to make sure that these things can not run for ever. The OnItemClickListener might not be able to know if/when the thread should stop running. But it is very likely that you might want to kill the tread before it has completed. It is not clear what WakeOnLan and ExecuteSshCommand are doing and if they might also have a potential to run for a long time. Or maybe they fail and need to be checked later.

It is possible that not storing the references is not a problem in this case. But this type of action should be done with care.


open multiple tabs using selenium I am using the following script to read search terms from a file and then search those terms in google (each search result in it's own tab).
from selenium import webdriver

browser = webdriver.Chrome()

# first tab
browser.get('https://google.com')

with open("google-search-terms.adoc") as fin:
    for line_no, line in enumerate(fin):
        line = line.strip()
        line = line.replace(' ', '+')
        line = line.replace('&', '%26')
        browser.execute_script(
            "window.open('https://www.google.com/search?q="+line+"');")

How can I make this code better?

Functions
Create individual smaller functions doing a single task.
Enumerations
The enumeration for file does not make sense. line_no servers no purpose.
if __name__ block
For scripts, it is a good practice to put your executable feature inside the if __name__ == "__main__" clause.
A rewrite of the same code might look like:
from selenium import webdriver

FILE_NAME = "google-search-terms.adoc"

def search_google(browser, term: str):
    term = term.replace(' ', '+').replace('&', '%26')
    browser.execute_script(f"window.open('https://www.google.com/search?q={term}');")

def process_file(filename: str) -> str:
    with open(filename) as fin:
        for line in fin:
            yield line.strip()

def main():
    browser = webdriver.Chrome()
    browser.get('https://google.com')
    for search_term in process_file(FILE_NAME):
        search_google(browser, search_term)

if __name__ == "__main__":
    main()


Area of the triangle generated by the hands of a clock Problem explanation:

Let’s suppose we have a clock that has an hour hand 3 units long, a minute hand 4 units long, and a second hand 5 units long. The hour hand moves once every hour, the minute hand moves once every minute, and the second hand moves once every second.
Therefore, exactly every second, the triangle defined by the ends of the hands changes its area.

Which is the maximum area between two given times?

Input: A sequence of hours in sets of 2 in the format hh:mm:ss.

Output: The maximum area defined by the ends of the hands in square units.

For a more comprehensive explanation: https://jutge.org/problems/P17681_en/statement

My code:

I have written a function that takes as arguments the position of the hands in radians and returns its area (trigonometrically intensive).

double area(double secRad, double minRad, double houRad){
    double alfaSM = atan( (5-4*cos(abs(secRad-minRad)))/(4*sin(abs(secRad-minRad))) );
    double alfaSH = atan( (5-3*cos(abs(secRad-houRad)))/(3*sin(abs(secRad-houRad))) );
    double alfaMH = atan( (4-3*cos(abs(minRad-houRad)))/(3*sin(abs(minRad-houRad))) );

    double distSM = abs(5*sin(alfaSM)+4*sin(abs(secRad-minRad)-alfaSM));
    double distSH = abs(5*sin(alfaSH)+3*sin(abs(secRad-houRad)-alfaSH));
    double distMH = abs(4*sin(alfaMH)+3*sin(abs(minRad-houRad)-alfaMH));

    double s = (distSM+distSH+distMH)/2;

    return s*(s-distSM)*(s-distSH)*(s-distMH);
}

My main function reads the input from the user and calculates the area for each second in the range of time the user has entered, stores the maximum area and finally prints it.

int main(){
    char z;
    double hI,mI,sI, hF,mF,sF;
    while(cin >> hI >> z >> mI >> z >> sI >> hF >> z >> mF >> z >> sF){
        double maxArea = 0;
        for(int i = 3600*hI+60*mI+sI; i <= 3600*hF+60*mF+sF; i++){
            double cacheArea = area( 2*pi*((double)(i%60)/60) , 2*pi*(double)((i%3600)/60)/60, 2*pi*(double)((i%43200)/3600)/12);
            if(cacheArea > maxArea) maxArea = cacheArea;
        }
        cout << fixed << setprecision(3) << sqrt(maxArea) << endl;
    }
}

Write this at the beginning so you can test the program:

#include <iostream>
#include <cmath>
#include <iomanip>
#define pi 3.14159265358979323846
using namespace std;

Where am I now and what is left?

The algorithm of the code above works as intended, but it isn't efficient enough. The online judge throws a time limit exceeded error.

I have managed to reduce the number of calculations of area needed by supposing that if the range of time is more than an hour, we just need to check all the combinations for the first hour, since the maximum area will be there and will be appearing again every hour(the relative positions repeat).

I have also implemented a variable that checks if the separation between hands of the clock in the time its checking is smaller than the separation of the hands in the maximum area found until that moment. If it is, the program skips the area calculation.

These changes seem to improve performance considerably (from 43,200 area calculations to ~1,500 in the complete spin of 12 hours), but the judge still doesn't admit it.

I seem to need a cleaner solution, a mathematically prettier one, but I'm struggling to find one.

This is an addendum to Oscar's answer. Just doing some math.

Let t = 0 be 12 o'clock, when all hands are coinciding and the area is 0. We need to calculate the area as a function of t. Let s, m, h be the angles of second, minute and hour with respect to the trigonometric reference. Then
$$s=\frac{\pi}2-\frac{\pi}{30}t\\ m=\frac{\pi}2-\frac{\pi}{1800}t\\ h=\frac{\pi}2-\frac{\pi}{21600}t$$
and let S, M, H be the lengths of clock's hands. Then the area is
$$A=\frac12\left|SM\sin\alpha+MH\sin\beta+HS\sin\gamma\right|$$
where
$$\alpha=s-m,\;\beta=m-h,\;\gamma=h-s$$
Therefore we have
$$\alpha=-\frac{59}{60}\cdot\frac{\pi}{30}t\\ \beta=-\frac{11}{720}\cdot\frac{\pi}{30}t\\ \gamma=\frac{719}{720}\cdot\frac{\pi}{30}t$$
where t is a positive integer. As you see, doing some boring math can sometimes reduce the amount of codes needed for a job.

P.S. and here is a plot of A vs. t. It is periodic indeed.


Validating input variables I have written the following JavaScript code and want to ask if it is written as per best practices. Please recommend any changes to be made in the code. The code is taking a few variables as input and then validating if all of them have some value and not blank/null.

var var1 = false;
var missingVAL = false;
var validationMsg = "";

host       = scriptletContext.get("host");
user    = scriptletContext.get("user");
serverID        = scriptletContext.get("serverId");
runID              = scriptletContext.get("runId");

if (host == false)
{
  validationMsg = "serverHostName (serverHostName) is missing.";
  var1 = true;
}

if (user == false)
{
    if (var1 == true)
    {
        validationMsg = validationMsg + " Requestor user name (requestorUserName) is missing.";
    }
    else
    {
        validationMsg = "Requestor user name (requestorUserName) is missing.";
    }
    var1 = true;
}

if (serverID == false)
{
    if (var1 == true)
    {
        validationMsg = validationMsg + " (vascoServerId) is missing.";
    }
    else
    {
        validationMsg = "(vascoServerId) is missing.";
    }
    var1 = true;
}

if (runID == false)
{
    if (var1 == true)
    {
        validationMsg = validationMsg + " OAC run type (runTypeId) is missing.";
    }
    else
    {
        validationMsg = "OAC run type (runTypeId) is missing.";
    }
    var1 = true;
}

if (var1 == true)
{
    scriptletContext.putGlobal("validationMsg", validationMsg);
}

scriptletContext.putGlobal("missingVAL", var1);

A quick note on style, it's much more common in JavaScript code to have the opening curly bracket on the same line as the expression that precedes it, rather than on a new line.

var1, based on the code, indicates whether there has been a validation problem, and to keep the same logical flow, we could name it instead isInvalid for more clarity.

Along the same lines, missingVAL is never used; perhaps you meant to use that one instead of var1?

It's a good practice to use strict equality/inequality (=== and !==) instead of == and != in many cases, to avoid some unpredictable behavior that is otherwise caused by the JavaScript engine.

For instance:

1 == "1"  //true
1 === "1" //false
null == undefined  //true
null === undefined //false
"" == 0  // true
"" === 0 //false

Also, when comparing values to true or false, it's simpler to just let the truthiness or falseness of the value be the condition, such as:

// instead of this:
if (someValue == true && someOtherValue == false) { ... }
// simply do this: (note the use of not "!" operator)
if (someValue && !someOtherValue) { ... }

You are using the same validation routine 4 separate times; instead of copying the code, it's simpler to make one function that does the work then just call it 4 times.

Your validations do this:

Check that the input value is not false (empty, undefined or null)
If it fails, add a message into a string noting the validation failed
Make the validation flag variable true to indicate there is a validation problem.

Step (3) could reasonably be removed by using the validationMsg, since if there have been no validation failures by the end, it will still be an empty string.

We could write a function like this: (code comments added for explanation)

function validate(value, messageIfInvalid) {
    // null, undefined, 0 and "" evaluate to false
    if (!value) {
        // empty string evaluates to false and non-empty to true
        if (validationMsg) {
            // if not empty, add a space at the end
            validationMsg += " ";
        }
        // add the messageIfInvalid
        validationMsg += messageIfInvalid;
    }
}

This function introduces a side effect which is to modify the globally-scoped variable validationMsg, which makes it less predictable. It would be generally better to instead pass it the validationMsg as argument and then return the validationMsg with any applicable modifications made by the function, like so:

// also pass validationMsg as argument
function validate(value, validationMsg, messageIfInvalid) {
    if (!value) {
        if (validationMsg) {
            validationMsg += " ";
        }
        validationMsg += messageIfInvalid;
    }
    // return the validationMsg, whether or not it was modified
    return validationMsg;
}

Then at the very end of the script, we can check whether validationMsg is an empty string or not to find out if there have been any invalid inputs.

// this condition will be true unless validationMsg is an empty string
if (validationMsg) {
    scriptletContext.putGlobal("validationMsg", validationMsg);
    scriptletContext.putGlobal("missingVAL", true);
} else {
    scriptletContext.putGlobal("missingVAL", false);
}

There is one potential issue with this logic, in that if a value checked as invalid/false, but the messageIfInvalid argument was passed an empty string, then no message would be added to validationMsg. We can add another condition to address this special case and just add a generic validation message, like so:

if (!value) {
    if (!messageIfInvalid) {
        messageIfInvalid = "a value is missing";
    }
    if (validationMsg) {
        validationMsg += " ";
    }
    validationMsg += messageIfInvalid;
}

This is what it looks like with all the above applied.

host = scriptletContext.get("host");
user = scriptletContext.get("user");
serverId = scriptletContext.get("serverId");
runId = scriptletContext.get("runId");

function validate(value, validationMsg, messageIfInvalid) {
    if (!value) {
        if (!messageIfInvalid) {
            messageIfInvalid = "a value is missing";
        }
        if (validationMsg) {
            validationMsg += " ";
        }
        validationMsg += messageIfInvalid;
    }
    return validationMsg;
}

var validationMsg = "";

validationMessage = validate(host, validationMessage, "serverHostName (serverHostName) is missing.");
validationMessage = validate(user, validationMessage, "Requestor user name (requestorUserName) is missing.");
validationMessage = validate(serverId, validationMessage, "(vascoServerId) is missing.");
validationMessage = validate(runId, validationMessage, "OAC run type (runTypeId) is missing.");

if (validationMsg) {
    scriptletContext.putGlobal("validationMsg", validationMsg);
    scriptletContext.putGlobal("missingVAL", true);
} else {
    scriptletContext.putGlobal("missingVAL", false);
}


Python calculate arrangements of sequence I have just started thinking about probabilities. A Problem that come up was how to calculate all the potential arrangements for a given sequence. By arrangement I mean unique permutation.

I initially used this method:

from itertools import permutations

sequence = '11223344'
len(set(permutations(sequence)))
# 2520

But for long sequences this can take a long time! (or run out of memory)

I came up with this function arrangements

from math import factorial
from functools import reduce
from operator import mul

def arrangements(sequence):
    return factorial(len(sequence))/reduce(mul,
           [factorial(sequence.count(i)) for i in set(sequence)])

# arrangements(sequence)
# 2520.0

My thinking is this:

For a given length sequence with all unique items there are factorial(len(sequence)) permutations.

For every repeated item in the sequence there will be factorial(#repeats) that will result in the same permutation.

My function calculates all permutations / all repeated permutations.

I'm sure I have reinvented an already existing standard python function somewhere. I'd like to know if my thinking is sound and the implementation makes sense.

Wouldn't itertools.arrangements be cool?

Notes

I'd expect arrangements to return the unique permutations of sequence, not just how many there are.
If it returns a number, it should be an integer.
You could use collections.Counter instead of counting the integers again and again.
You're right, it would be nice to have itertools.unique_permutations. In the meantime, I often come back to this SO answer.

Possible refactoring

from math import factorial
from functools import reduce
from collections import Counter
from operator import mul

def count_unique_permutations(sequence):
    count_permutations = factorial(len(sequence))
    repetitions = (factorial(v) for v in Counter(sequence).values())
    return count_permutations // reduce(mul, repetitions)


Von Neumann random bytes debiasing I need to create a method to debias biased bytes from the method get_raw_bytes(length) using von Neumann debiasing method.

Von Neumann method takes two bits at a time. If the input is 01 it will output 0, if the input is 10, it will output 1, if the input is 00 or 11, it won't output anything.

Example:

Raw      : [11010110, 11001001, 01101000]
Debiased : [00110011]

Here's my current code:

def get_raw_bytes(length):
    return open('/dev/hwrng', 'rb').read(length)

def get_debiased_bytes(length):
    arr_debiased_bytes = []
    debiased_byte = 0
    bit_counter = 0
    while len(arr_debiased_bytes) < length:
        raw_bytes = get_raw_bytes(length * 5)
        for byte in raw_bytes:
            for k in range(0, 8, 2):
                bit1 = byte >> k & 1
                bit2 = byte >> k + 1 & 1
                if bit1 != bit2:
                    debiased_byte = debiased_byte << 1 | bit1
                    bit_counter += 1
                    if bit_counter == 8:
                        arr_debiased_bytes.append(debiased_byte)
                        debiased_byte = 0
                        bit_counter = 0
    return bytes(arr_debiased_bytes[:length])

I think my code still can be improved and tidied up. Can anyone help me with this?

In get_raw_bytes you should use with to close the file.
Without using a with or .close the file is not guaranteed to close and so can cause problems depending on how nice Python is feeling on being to you.

Don't leave bugs to change.
The code is fairly good, but get_debiased_bytes is doing three things at once.

Converting bytes (\$2^8\$) to crumbs (\$2^2\$).

for byte in raw_bytes:
    for k in range(0, 8, 2):
        bit1 = byte >> k & 1
        bit2 = byte >> k + 1 & 1

Debiasing the crumbs to bits.

if bit1 != bit2:
    debiased_byte = debiased_byte << 1 | bit1

Joining bits to bytes.

debiased_byte = debiased_byte << 1 | bit1
bit_counter += 1
if bit_counter == 8:
    arr_debiased_bytes.append(debiased_byte)
    debiased_byte = 0
    bit_counter = 0

Bundling all this together is making your code harder to read.
When extracting the above functions I would use generator functions.
This has a couple of benefits:

In Python the iterator pattern has a lot of sugar.
By using a generator we have more options on how to deal with the length.
We can truncate the size on the input, or only take length amount of items from the sequence after it's been made.

I'm not a fan of using byte >> k to get the bits / crumb.

Since bit operators are not commonly used their order of precedence can be confusing.
This doesn't help when you're mixing both bitwise and integer operators.

By using this you're adding a requirement for naming k when there is no need.
You can extract the nibble in a readable form by using byte & 0b11.
The code to debiase the crumbs is smart, but not exactly readable.

We know only two of the four states can yield data.
And so just hard coding the values can allow people to just read the code and figure it out faster.

By using the 0b prefix this can make the code really quite simple.
The code for building the bytes is ok. It's fairly easy to read.
The code for building the bytes can silently swallow data.

I personally would not be a fan of this. Instead I would yield the byte regardless.
There are two forms this can take, padding the bits on the left or right.

def bytes_to_crumbs(bytes):
    for byte in bytes:
        for _ in range(4):
            yield byte & 0b11
            byte >>= 2

def debiase_crumbs(crumbs):
    for crumb in crumbs:
        if crumb == 0b10:
            yield 1
        elif crumb == 0b01:
            yield 0

def bits_to_bytes(bits):
    """Just discard."""
    try:
        while True:
            byte = next(bits)
            for _ in range(7):
                byte <<= 1
                byte |= next(bits)
            yield byte
    except StopIteration:
        pass

def bits_to_bytes(bits):
    """Pad to the right - 1111000."""
    try:
        while True:
            byte = next(bits)
            for _ in range(7):
                byte <<= 1
                byte |= next(bits, 0)
            yield byte
    except StopIteration:
        pass

def bits_to_bytes(bits):
    """Pad to the left - 00001111."""
    try:
        while True:
            byte = next(bits)
            for _ in range(7):
                try:
                    bit = next(bits)
                except StopIteration:
                    yield byte
                    return
                byte <<= 1
                byte |= bit
    except StopIteration:
        pass

def get_debiased_bytes(length):
    with open('/dev/hwrng', 'rb') as f:
        _bytes = f.read(length)
    return bytes(bits_to_bytes(debiase_crumbs(bytes_to_crumbs(_bytes))))


Hours:Minutes Addition I wrote this simple page to help at work, and as an exercise to get more familiar with JavaScript.

I'm especially interested in help with style and structure - is this "standard"?

I know the input isn't validated at all - feel free to make suggestions here but this isn't my main concern at this time.

behavior.js:

var output_prefix = 'Total duration is: ';
var entries = [];
var list_display = null;

function addDuration() {
    var input_text = document.getElementById("input_text");
 if (1) {  //add validation, input must be hh:mm
     var hhmm = input_text.value.split(":");
     var input_val = Number(hhmm[0]*60) + Number(hhmm[1]);
  entries.push(input_val);
     updateTotalDisplay();
     var new_node = document.createElement("P");
     new_node.appendChild(document.createTextNode(formatTime(input_val)));
     list_display.appendChild(new_node);
 } else {
     alert("not a number");
 }
 input_text.value = "";
 input_text.focus();
}

function popDuration() {
    entries.pop();
 updateTotalDisplay();
 list_display.removeChild(list_display.lastChild);
}

function updateTotalDisplay() {
    var total = 0;
    for (var i = 0; i<entries.length; i++) {
     total = total + entries[i];
 }
    document.getElementById("totaltimetext").innerHTML = output_prefix + formatTime(total);
    return;
}

function formatTime(num_minutes) {
    var time_str = "" + Math.floor(num_minutes/60).toString() + ":";
 var mm = num_minutes%60;
 if (mm>=0 && mm<10) {
     time_str += "0";
 }
 time_str += mm;
    return time_str;
}

window.onload = function() {
    list_display = document.getElementById("duration_list");
    updateTotalDisplay();
 document.getElementById("add_button").addEventListener("click", addDuration);
 document.getElementById("pop_button").addEventListener("click", popDuration);
}
<!DOCTYPE html>

Simple Hours:Minutes Adder

Enter duration in format hh:mm and select ok, running total at top

text

    <form>
        <input id="input_text" type="text"></input>
        <input id="add_button" type="button" value="add">
        <input id="pop_button" type="button" value="pop item">
    </form>

HTML5 style

I'm not sure if there's a standard for HTML formatting.
I think it usually depends on the team.
You might be interested in Google's style guide for HTML.

It seems logical to indent  and  deeper than ,
so instead of:

<!DOCTYPE html>

Simple Hours:Minutes Adder

This would be better:

<!DOCTYPE html>

Simple Hours:Minutes Adder

HTML5 validation

In HTML5, the <input> tag should not have a closing </input>.
The first <input> tag here violates that:

<input id="input_text" type="text"></input>
<input id="add_button" type="button" value="add">
<input id="pop_button" type="button" value="pop item">

There are some other HTML5 violations, for example missing <title>.
Use a validator to catch these.

JavaScript style

Indenting using two spaces is a very common practice, so that's good.

In general your JavaScript style seems fine, clean, easy to read.
Some minor violations:

pointless return; at the end of updateTotalDisplay
total = total + entries[i] can be simplified to total += entries[i]
instead of setting totaltimetext to output_prefix + formatTime(total), it would be better to have output_prefix in the HTML document, and only update the changing part, in a dedicated field

Use booleans

Instead of this:

  if (1) { //add validation, input must be hh:mm

Better use proper booleans:

  if (true) { //add validation, input must be hh:mm

But even better would be to replace the comment with a function call:

function is_valid(hhmm) {
   //implement validation, input must be hh:mm
   return true;
}

var hhmm = input_text.value.split(":");
if (is_valid(hhmm)) {

Improving the logic

Instead of re-summing the minutes stored in entries,
it would be more efficient to keep track of the running total,
and simply add the newly added number of minutes.

Improving formatTime

Instead of:

  if (mm >= 0 && mm < 10) {

It's generally more readable to arrange the terms in increasing order:

  if (0 <= mm && mm < 10) {

But actually, the 0 <= mm seems pointless.
It's reasonable to expect that this function will receive already validated number of minutes (0 or positive).

The function could also use better variable names,
and some string operations can be written simpler,
and some a variable can be eliminated:

function formatTime(total_minutes) {
  var hours = Math.floor(total_minutes / 60);
  var minutes = total_minutes % 60;
  if (minutes < 10) {
    minutes = '0' + minutes;
  }
  return hours + ':' + minutes;
}


Newbie Map/Reduce word frequency counter I present you my first ever module to learn some Erlang awaiting your scrutiny . It does a word frequency count using map/reduce . I'm an Erlang noob, so I would very much like feedback about:

Unneeded verbosity / code

Erlang 'style' mistakes

Things that can be done more elegantly

-module(main).

-export([map/1]).
-export([main/0]).
-export([reducer_wrapper/1]).
-export([reducer/2]).

-define(NUMBER_OF_PARTITIONS, 100).

main() ->
    Lines = readlines('pg20417.txt') ,
    lists:foreach( fun main:map/1, Lines ),
    start_reduce().

%% signals all reducer_wrappers that all emits are received, and the actual reduce can be started
start_reduce() ->
    lists:foreach( fun(PartitionNr) -> get_reducer(PartitionNr) ! start_reduce end, lists:seq(1, ?NUMBER_OF_PARTITIONS) ).

%% emits every word with cnt = 1
map(Line) ->
    Words = string:tokens( string:to_lower(Line), " .,;:\n()?"),
    lists:foreach ( fun(Word) -> get_reducer( Word, ?NUMBER_OF_PARTITIONS ) ! {emit, {Word, [1]}} end, Words ).

%% returns the reducer Pid
get_reducer( PartitionNr ) ->
    ProcessName = list_to_atom( lists:concat( [PartitionNr, '_reducer'] ) ),
    case whereis( ProcessName ) of
    undefined ->
        Pid = spawn(?MODULE, reducer_wrapper, [[]]),
        register( ProcessName, Pid ),
        Pid;
    Pid ->
        Pid
    end.

get_reducer( Key, NumberOfPartitions ) ->
    get_reducer(  erlang:phash2(Key) rem NumberOfPartitions + 1 ).

%% the reducer wrapper proces
reducer_wrapper(KeyValueTuples) ->
    receive
    { emit, KeyValueTuple } ->
        reducer_wrapper( [KeyValueTuple] ++ KeyValueTuples );
        start_reduce ->
        Dictionary = lists:foldl( fun main:reducer/2, dict:new(), KeyValueTuples ),
        show_all( dict:to_list(Dictionary) )
    end.

reducer( { Key, [Cnt] }, Dictionary ) ->
    case dict:is_key( Key, Dictionary ) of
    true ->
        dict:store( Key, dict:fetch( Key, Dictionary ) + Cnt, Dictionary );
    false ->
        dict:store( Key, Cnt, Dictionary )
    end.

readlines(FileName) ->
    {ok, Device} = file:open(FileName, [read]),
    get_all_lines(Device, []).

get_all_lines(Device, Accum) ->
    case io:get_line(Device, "") of
        eof  -> file:close(Device), Accum;
        Line -> get_all_lines(Device, Accum ++ [Line])
    end.

show_all([Head|Tail]) ->
        io:format("~p~n", [Head]),
        show_all(Tail);

show_all([]) ->
        void.

A few initial points:

Your mapping phase is all done on one process, so you're not getting any parallelism. You should fire off M mappers to do the mapping for you, and give them bits of the data to map over. They can pass this on to one of R reducers who do the reduction.
[KeyValueTuple] ++ KeyValueTuples is slow as you're creating lists and concatenating them together. Instead, just prepend values to the list in O(1) time: [KeyValueTuple|KeyValueTuples]
The way you're spawning/creating the reducer process is a bit unnecessary. If I were you I'd spawn R reducers up front and dish out the work in a round-robin fashion. This will give you better control of load distribution.
Instead of calling dict:is_key then, if true, calling dict:fetch do both of them in one step by calling dict:find, then matching against {ok, Value} or error. This saves on the double-lookup.

As a general comment, you've really tied together both map/reduce and word counting. Ideally what you'd want to do is have a structure/module which knows about how to do map/reduce by itself, and be ignorant of the work that's going on. You would then have another module which is able to invoke a map/reduce job by passing in the Map function, the Reduce function, the number of mappers and reducers and the data that's being processed.

I realise you're learning, so you don't need to do all this. But down the track I'd recommend attempting to abstract away the map/reduce stuff into a reusable module.

Hope that helps.
OJ

PS. were you hoping for someone to show you how they'd code it up? I'm new to this code review site and I'm not sure whether I should be posting "fixed" code or not :)


Function for iterating out an unordered list of custom taxonomy terms I've created a function that generates an
 of my wordpress custom taxonomy terms, displaying parent > child > second child heirachy. Functionally it is working perfectly and replaces my old hard-coded menu.

The problem is I'm inexperienced and recognise it as an ugly, inefficient function but could not figure out how to make it better. I tried use 'While()' loops to simplify things but came unstuck. Could someone help me understand how to improve this. The final output (including CSS and jQuery to collapse deactivated menu items) is included below.

//Display Navigation Menu

function taxonomy_nav(){

  //Get Parent Terms
  $getParents = [
    'taxonomy'     => 'kernal_category',
    'parent'        => 0,
    'number'        => 0,
    'hide_empty'    => false
  ];

  //Assign parent terms to variable
  $parent_terms = get_terms( $getParents );

  //Loop through parent terms
  foreach ( $parent_terms as $parent_term ) {

    //Store children terms
    $childrenCheck = get_term_children( $parent_term->term_id, $parent_term->taxonomy );

    //If parent term has children...
    if ( ! is_wp_error( $childrenCheck ) && ! empty ( $childrenCheck ) ) {

      //Output parent term and start new children list
      echo '
'. $parent_term->name.'';
      echo '
';

        //Loop through child items and output name
        foreach ( get_terms( 'kernal_category', array( 'hide_empty' => false, 'parent' => $parent_term->term_id ) ) as $child_term) {

            //Check if children have children
            $secondChildrenCheck = get_term_children( $child_term->term_id, $child_term->taxonomy );

              //Generate list for second children
              if ( ! is_wp_error( $secondChildrenCheck ) && ! empty( $secondChildrenCheck ) ) {

                echo '
' . $child_term->name.'';
                echo '
';

                //If child has child, look through child
                foreach ( get_terms( 'kernal_category', array( 'hide_empty' => false, 'parent' => $child_term->term_id ) ) as $second_child_term) {

                 echo '
'.$second_child_term->name . '
';

                }

                echo '
';
                echo '
';

              } else {

              echo '
'. $child_term->name.'
';

              }
        }

      echo '
';
      echo '
';

    } else {

      echo '
'. $parent_term->name.'
';

    }

  }

As pointed out, there is a lot of code repetition and this is a big problem when any changes get made.  It's so easy to quickly make a change to one piece of code and miss the 3 other versions of it which do the same thing.

Rather than factor out these parts of code into functions though, you can re-factor the code into a recursive function.  The idea being that you just use the same code to do each layer of the process rather than loops within loops calling helper functions to do the more common tasks.

One other thing I have changed is to build the output into a string rather than echo it out directly.  I would normally then return this string to the calling code and let that do whatever it wants with it.  This allows more control over the output generated, also allows the output to be re-used (although not sure how that would be necessary in this case) rather than having to re-generate the output each time.  But as I'm not sure how this is called, I just echo the combined string at the end.

function taxonomy_nav( string $category = null, array $settings = null  ) : string {
    if ( $category == null )    {
        $settings = [
            'taxonomy'     => 'kernal_category',
            'parent'        => 0,
            'number'        => 0,
            'hide_empty'    => false
        ];
        $parent_terms = get_terms( $settings );
    }
    else    {
        $parent_terms = get_terms( $category, $settings );
    }

    foreach ( $parent_terms as $parent_term ) {
        $list =  '
'. $parent_term->name.'';

        $childrenCheck = get_term_children( $parent_term->term_id, $parent_term->taxonomy );

        if ( ! is_wp_error( $childrenCheck ) && ! empty ( $childrenCheck ) ) {
            $list .= '
'
                . taxonomy_nav ( 'kernal_category', array( 'hide_empty' => false, 'parent' => $parent_term->term_id )  )
                . '
';
        }

        $list .= '
';
    }

    echo $list;
}

As you can see, in the if ( ! is_wp_error( $childrenCheck ) block, it just starts the sub list and then calls itself to do the processing for any child nodes.

This also shows how the list item itself is only ever generated by the one line of code, not repetition and any changes are easily identified.


Limit characters by line without splitting word I got this question in an interview. Given a string of words, print it such as each line has at most limit characters, if a word does not fit on the line, print it on the next line and so on.
This is my implementation:
public static void main(String[] args) {
        String s = "Even aside from the rain and wind it hadn't been a happy practice session. Fred and George, who had been spying on the Slytherin team, had seen for themselves the speed of those new Nimbus Two Thousand and Ones. They reported that the Slytherin team was no more than seven greenish blurs, shooting through the air like missiles.";
        wordWrapper(s, 11);

    }

    public static void wordWrapper(String s, int limit) {
        // Time complexity: O(n) - n size of s
        // Space complexity: O(n) - n number of words in s
        int charCount = 0;
        int i = 0;
        String[] words = s.split(" ");
        while (i < words.length) {
            if (charCount + words[i].length() > limit) {
                System.out.println();
                charCount = 0;
            }
            charCount += words[i].length();
            System.out.print(words[i] + " ");
            i++;
        }
    }

I'm trying to reduce the space complexity, and just use pointers instead of split(), this is what I have so far, I'm thinking of keeping track of the last whitespace and going back to insert a line before that word when the limit is reached, if you run the code the new line is inserted correctly but half of the word would still print before the new line. Please let me know what I'm missing. I also would love to look at better approaches. Thank you!
public static void wordWrapper1(String s, int limit) {
    int i = 0;
    int lastWhitespaceIdx = -1;
    int lineCount = 0;
    if (s.length() == 0) {
        return;
    }
    while (i < s.length()) {
        if (Character.isWhitespace(s.charAt(i))) {
            lastWhitespaceIdx = i;
        }
        if (lineCount + 1 > limit) {
            if (!Character.isWhitespace(s.charAt(i + 1))) {
                i = lastWhitespaceIdx + 1;
                System.out.println();
                lineCount = 0;
                continue;
            } else {
                System.out.println();
                lineCount = 0;
            }
        }
        lineCount += 1;
        System.out.print(s.charAt(i));
        i++;
    }
}

String s = "Even aside from the rain and wind it hadn't been a happy practice session. Fred and George, who had been spying on the Slytherin team, had seen for themselves the speed of those new Nimbus Two Thousand and Ones. They reported that the Slytherin team was no more than seven greenish blurs, shooting through the air like missiles.";

s is not a good variable name. My rule is that you're only allowed to use single-character variable names when dealing with dimensions (yes, that also disallows i/j/k for loops, too).

public static void wordWrapper(String s, int limit) {

The naming is off, this is not a "word wrapper" it's a function that does wrap a string by words, so more like wrap or wrapText.

        // Time complexity: O(n) - n size of s
        // Space complexity: O(n) - n number of words in s

I'm not sure that is correct. I could be very well mistaken here because I rarely need to think about complexity limitations at all, but I think it is closer to "O(n + m)", with "n" being the number of characters (the split), and "m" being the number of words (your loop). Space should be "O(n*2)", with "n" being the number of characters in the string, as you need to keep each character twice in memory, once in the original, and once in the word string.
However, most JVMs are implementing "substrings", which means that the array of codepoints is only held one in memory, and if you do a substring, you will receive a new String with the same array but different start/end index.

        int charCount = 0;
        int i = 0;
        String[] words = s.split(" ");
        while (i < words.length) {
            if (charCount + words[i].length() > limit) {
                System.out.println();
                charCount = 0;
            }
            charCount += words[i].length();
            System.out.print(words[i] + " ");
            i++;
        }

This code could be simplified to a for-each loop:
int currentLineLength = 0;

for (String word : text.split(" ")) {
    if ((currentLineLength + word.length()) > maxCharactersPerLine) {
        System.out.println();
        currentLineLength = 0;
    }

    System.out.print(word + " ");
    currentLineLength = currentLineLength + word.length();
}

Note that your implementation, and this revised one, has three problems which should be dealt with at some point:

The spaces are not taken into account for the maximum line length.
It prints an extra space at the end of each line.
How a word should be treated that is longer than the maximum line length.

...and just use pointers instead of split(),...

There is no such thing as "a pointer for strings" in Java. Though, you most likely mean, to iterate over the String and keep track of the index, but "pointer" is a heavily prejudiced word, and I would stay clear of it and instead use "index".

int lastWhitespaceIdx = -1;

Don't shorten names just because you can, it does make the code harder to read in the end of the day.

int lineCount = 0;

The name of this variable is incorrect, it does not count lines, it counts characters on the current line, it should be named accordingly.

while (i < s.length()) {

Not sure why you use a while when a for would be perfect for what you're doing here.

System.out.print(s.charAt(i));

Printing single characters at a time can be quite wasteful, depends on whether the stream supports some sort of buffer or not.

You could use String.indexOf(String) and String.index(String, int) to iterate over the string and simplify your logic. Then use substring to extract the part of the String between the spaces and print that, or print that part character by character.


Get exactly `n` unique randomly sampled rows per category in a Dataframe I want to get exactly n unique randomly sampled rows per category in a Dataframe. This proved to be involve more steps than the description would lead you to believe.
n = 4
df = pd.DataFrame({'category': [1, 2, 3, 1, 2, 3, 1, 2, 3, 1, 2, 1],
                   'value' : range(12)})

category_counts = df['category'].value_counts()
categories_with_count_above_threshold = category_counts[category_counts >= n].index

# rows with infrequent categories are filtered out
df = df[df['category'].isin(categories_with_count_above_threshold)]

# sample exactly x rows per category
df = df.groupby('category').sample(n)

This goes through the whole DataFrame quite a few times. With a bigger DataFrame this can become quite time consuming. Is there a way to simplify this expression?
PS: requires pandas >= 1.1 for the DataFrameGroupBy.sample

Have you tried using DataFrameGroupBy.filter()  (I don't have recent pandas on this machine so I can't test it).
n = 4
df = pd.DataFrame({'category': [1, 2, 3, 1, 2, 3, 1, 2, 3, 1, 2, 1],
                   'value' : range(12)})

# sample exactly x rows per category
df = df.groupby('category').filter(lambda x:x.size >= n).sample(n)


Simple LinkedList implementation I am brushing up my Data Structure knowledge (for preparation for internship interviews) and I started by implementing a very simple Linked List class. This is by no means intended to be able to replace the Java library just to be something robust enough for making me understand and remember how to implement a Linked List.

LinkedList class with private Node class:

public class LinkedList<T> {

    private Node _first;
    private int _size;

    public LinkedList() {
        _first = null;
        _size = 0;
    }

    public int getSize() {
        return _size;
    }

    public void add(T data) {
        Node current = _first;

        if (current == null) {
            _first = new Node(data);
            _size++;
            return;
        }

        while (current.getNext() != null) {
            current = current.getNext();
        }

        current.setNext(new Node(data));
        _size++;
    }

    public void add(T[] array) {
        for (T data : array) {
            add(data);
        }
    }

    public void remove(T data) {
        Node current = _first;
        Node next = current.getNext();

        if (_first.getData().equals(data)) {
            if (_size == 1) {
                _first.setData(null);
                _size--;
                return;
            }
            _first.setData(null);
            _first = _first.getNext();
            _size--;
            return;
        }

        while (next != null) {
            if (next.getData().equals(data)) {
                current.setNext(next.getNext());
                next = null;
                _size--;
                return;
            }
            current = next;
            next = current.getNext();
        }
    }

    private class Node<T> {

        private T _data;
        private Node _next;

        public Node(T data) {
            _data = data;
            _next = null;
        }

        public void setData(T data) {
            _data = data;
        }

        public T getData() {
            return _data;
        }

        public void setNext(Node next) {
            _next = next;
        }

        public Node getNext() {
            return _next;
        }
    }
}

Anything I could do to make it better?

raw types

Try to avoid raw types. private Node fieldname should be private Node<T> fieldname, same for function returns and method arguments.

Bug

If I do this:

MyLinkedList<String> list = new MyLinkedList<>();
list.remove("foo");

I get a NullPointerException. This is because you first access data of the root, and then check what the size is. It should be the other way around (and checking if the root is null would be a bit clearer).

You also shouldn't set the data to null, you should remove the node, otherwise you have a root node that contains null (also, your list size is wrong from there on out).

It also doesn't make that much sense to first set the data, and then override the node. In that case, just remove the setting of the data.

remove non existing element

If a non existing element is removed, you could throw a NoSuchElementException (it's what Java lists do).

Setting values to null

next = null; this isn't necessary, you are returning right afterwards.

Naming

it is a bit uncommon to start a field name with _.
I would conform to the names of the List interface (getSize -> size, add(array) -> addAll(array).


Recursive and Non-recursive SelectionSort This is a naive attempt of writing a recursive and non-recursive versions for SelectionSort(). My goal is mainly to present an elegant, easy-to-understand, idiomatic code, and therefore, performance is a distant priority. Please comment away!

// Find the index at which the minimum value
// exists inside the Array
function findMinIndex(a){
    return a.reduce((iMin, x, i, arr) => x < arr[iMin]? i : iMin, 0);
}

// Remove the minimum value from the array
// Return the value removed
function removeMin(a){
    idx = findMinIndex(a);
    minVal = a[idx];   // for [1, -5, 3], minVal = -5
    a.splice(idx, 1);  // [1, -5, 3] -> [1, 3]
    return minVal;
}

// Selection sort, in recursive mode
// As the name suggests, we select and 'splice' it
// away from the array, and recursively
// concatenate it to get the final result
function selectRecursive(a) {
    if (!a.length) return [];   // terminating case
    minVal = removeMin(a);      // remove the smallest
    console.log(v, a);
    return [minVal].concat(selectRecursive(a));
}

var myl = [1, 2, 3, 99, 22, 55, 5];
selectRecursive(myl);

OUTPUT

1 [ 2, 3, 99, 22, 55, 5 ]
2 [ 3, 99, 22, 55, 5 ]
3 [ 99, 22, 55, 5 ]
5 [ 99, 22, 55 ]
22 [ 99, 55 ]
55 [ 99 ]
99 []
[ 1, 2, 3, 5, 22, 55, 99 ]

In addition, a non-recursive (less intuitive in my opinion) version of SelectionSort (using push, slice, splice and the spread operator) is presented below.

function selectionSort(a) {
  var length = a.length;
  for (var i = 0; i < length; i++) {
    a.push(  // select and append at the end
      ...a.splice(
        findMinIndex( // get min in a sliced 'a'
          a.slice(0, length - i)), 1)
    );
  }
  return a;
}

I'll take this a function at a time before looking at your end goal.

function findMinIndex(a){
    return a.reduce((iMin, x, i, arr) => x < arr[iMin]? i : iMin, 0);
}

The largest problem with this function is that it fails for empty arrays. findMinIndex([]) incorrectly returns 0, an index which does not exist. If the array is empty, I'd suggest returning a flag value of -1.

Since undefined is less than -Infinity, we then have to swap the comparison around to make it work with essentially the same logic.

function findMinIndex(a){
    return a.reduce((iMin, x, i, arr) => x > arr[iMin]? iMin : i, -1);
}

I would argue that this is still more complex than it needs to be. There is no need to use the arr parameter as we can just use a from the function call. (Some may disagree with me here) While we are at it, it improves the clarity to rename a to arr.

function findMinIndex(arr){
    return arr.reduce((iMin, x, i) => x > arr[iMin]? iMin : i, -1);
}

Next up:

function removeMin(a){
    idx = findMinIndex(a);
    minVal = a[idx];   // for [1, -5, 3], minVal = -5
    a.splice(idx, 1);  // [1, -5, 3] -> [1, 3]
    return minVal;
}

How this works if pretty obvious, which is good. The one issue is that you create the global variable minVal. It looks like you missed a let. Some people would tell you to avoid functions which mutate their parameters, and while I generally do this myself, since this function is very clear about what it is doing it probably isn't an issue. However, I would probably rewrite this as a one liner, or just inline it as you did in your answer.

function removeMin(a) {
    return a.splice(findMinIndex(a), 1)[0];
}

Now for your recursive function. It also creates a global variable minVal but besides that looks good to me! Really nothing to say here.

The non-recursive selection sort could certainly use some work.

function selectionSort(a) {
  var length = a.length;
  for (var i = 0; i < length; i++) {
    a.push(  // select and append at the end
      ...a.splice(
        findMinIndex( // get min in a sliced 'a'
          a.slice(0, length - i)), 1)
    );
  }
  return a;
}

Personally, I would prefer just simple for loops in this case. No need for all those function calls.

function selectionSort(a) {
    for (let i = 0; i < a.length; i++) {
        let iLow = i;
        for (let j = i + 1; j < a.length; j++) {
            if (a[j] < a[iLow]) iLow = j
        }
        // Fancy destructuring to swap indexes
        [a[i], a[iLow]] = [a[iLow], a[i]]
    }
    return a;
}

Lastly, I can't help but mention a.sort() though I know that's not the point of this exercise.


String representation of Tree I have a Tree class, defined as follows:

public class Tree<T> {
    private T value;
    protected List<Tree<T>> children = new ArrayList<Tree<T>>();
    /* ... */
}

I want to implement the toString() method in this class to show the String representation of this Tree, e.g.:

└── root
    ├── child1
    │   └── grandchild1
    └── child2

I implemented in this way:

@Override
public String toString() {
    StringBuilder sb = new StringBuilder();
    buildString(sb, "", true);
    return sb.toString();
}

private void buildString(StringBuilder sb, String prefix, boolean isTail) {
    sb.append(isTail ? "└── " : "├── ").append(value).append(System.lineSeparator());
    prefix = prefix + (isTail ? "    " : "│   ");

    for (int i = 0; i < children.size() - 1; i++) {
        sb.append(prefix);
        children.get(i).buildString(sb, prefix, false);
    }
    if (children.size() >= 1) {
        sb.append(prefix);
        children.get(children.size() - 1).buildString(sb,prefix, true);
    }
}

I am looking for possible improvements or suggestions.

Some suggestions (by far not complete as the other answer(s) show):

You don't need to specify the type twice:

protected List<Tree<T>> children = new ArrayList<Tree<T>>();
protected List<Tree<T>> children = new ArrayList<>();

Create a new parameter maxDepth to limit the function nesting level.
It should be set to a default value to follow the principle of least astonishment: Nobody would expect his program to crash just because he triggered the string conversion somehow.
With very big trees the nested function calls can become a problem.
But I wouldn't worry about it for two reasons:

The function is not named archiveTree. As a user, I wouldn't expect it to handle each and every scenario of a tree.
The default value maxDepth should reasonably prevent out of memory scenarios as long as the user doesn't manually provide disproportional values. If he does, it's his fault and not yours anymore.

You sometimes have inconsistent spacing, e.g.:

buildString(sb,prefix, true);
buildString(sb, prefix, true);

You could specify the capacity argument with a guessed value to the constructor of StringBuilder to prevent some unnecessary capacity allocations. However, as long as you haven't identified this as a noticeable bottleneck of your application, I would consider it to be premature optimization.