Reading a file in Python I currently have this code to read lines from a file

def getLinesFromFile(filename):
    content = []

    # with open(filename, "r+") as file:
    #   for line in file:
    #       line = line.rstrip()
    #       if line:
    #           content.append(line)

    file = None
    try:
        file = open(filename, "r+")
        for line in file:
            line = line.rstrip()
            if line:
                content.append(line)
    except IOError:
        print "[ERR] File does not exist"
    finally:
        if file:
            file.close()

    return content

Please take note of the commented out code using with. I really wanted to use with but I can't find any good resource on how to catch exceptions with with. So I did it using try except.

Question: Is there no better way to do this? Or any comments?

P.S. I come from a Java background and this is my first time writing in Python so I am more inclined to try-except as of the moment.

You can use try except IOError on with the same way you did for open.

def getLinesFromFile(filename):
    content = []

    try:
        with open(filename, "r+") as file:
            for line in file:
                line = line.rstrip()
                if line:
                    content.append(line)
    except IOError:
        print "[ERR] File does not exist"

    return content

Using with is a good idea, but I'm not sure if you understood what it does. with automatically ensures that a file is always closed safely. So even if the script encounters an error then you'll close it. This means that you only need to catch an error if the file doesn't exist, which is why we use except IOError like this.

There are other notes too. In Python it's recommended that you use snake_case, not camelCase like in java. So you should call your function get_lines_from_file.

There's no need to use "r+", that's for reading and writing a file, but you only read it. Instead you could use 'r' but since that's the default you can just leave out the mode argument and do open(filename).

Don't use the name file, as that's shadowing Python's built in file constucting function. Python allows you to use a lot of the built in method's names in case you want to overwrite them, but it's best not to unless it's necessary. Using f is fine in such a small function.

You can also build your list easier with something called a list comprehension. List comprehensions are like for loops collapsed into a single expression that results in a list. In your case, you first need to get every line from the list and rstrip it, so that'd look like this:

content = [line.rstrip() for line in f]

However you also want to ignore empty lines, so you'll want to add an if condition at the end:

content = [line.rstrip() for line in f if line.rstrip()]

Now, you don't even need to actually store this as content. You can return it directly instead. Though, doing this means that you should manually return an empty list when the IOError is raised, otherwise Python will default to returning None. (It might be better to return None in the case of an error, but that depends on how you're using this)

Lastly, add a docstring to explain what the function does. They're a similar idea to javadocs but less structured and formal. Here's how I'd write the function:

def get_lines_from_file(filename):
    """Return a list of rstripped lines from filename

    Returns an empty list if the file doesn't exist."""

    try:
        with open(filename) as f:
            return [line.rstrip() for line in f if line.rstrip()]
    except IOError:
        print "[ERR] File does not exist"
        return []


This technique for functional programming, and testing functional programming I've been playing around with functional programming and testing with jest recently.

An issue I've run into, is that it's hard to mock ES6 module exports directly. See this Stack Overflow question for more details.

The solution I've come up with is to inject the dependency functions into the function that is calling it, via .bind.

Here's an example:

import axios from "axios";
const BASE_URL = "https://jsonplaceholder.typicode.com/";
const URI_USERS = 'users/';

/**
* These are the implementation of our functions, which take functions as arguments.
*/

export const _fetchUsers = async function (_makeApiCall, _URI_USERS) {
    return _makeApiCall(_URI_USERS);
}

export const _fetchUser = async function (_makeApiCall, _URI_USERS, id) {
    return _makeApiCall(_URI_USERS + id);
}

export const _fetchUserStrings = async function (_fetchUser, _parseUser, ...ids) {
    const users = await Promise.all(ids.map(id => _fetchUser(id)));
    return users.map(user => _parseUser(user));
}
export const _makeApiCall = async function (uri) {
    try {
        const response = await axios(BASE_URL + uri);
        return response.data;
    } catch (err) {
        throw new Error(err.message);
    }
}

export function _parseUser(user) {
    return `${user.name}:${user.username}`;
}

/**
 * Real exports
 */
export const makeApiCall = _makeApiCall;
export const parseUser = _parseUser;

export const fetchUsers = _fetchUsers.bind(null, _makeApiCall, URI_USERS);
export const fetchUser = _fetchUser.bind(null, _makeApiCall, URI_USERS);
export const fetchUserStrings = _fetchUserStrings.bind(null, _fetchUser, _parseUser);

And a test:

/**
 * Our most complicated test
 *
 */

describe("_fetchUserStrings", () => {

    describe("happy flow", () => {

        const fetchUserMock = jest.fn((i) => Promise.resolve({
            username: "foo",
            name: "bar"
        }));
        const parseUserMock = jest.fn(user => "string");
        const fetchUserStrings = _fetchUserStrings.bind(null, fetchUserMock, parseUserMock);

        it("returns an array of three strings", async () => {

            expect.assertions(3);
            const result = await fetchUserStrings(1, 2, 3);

            // I'm being a bit lazy here, you could be checking that
            // The strings are actually there etc, but whatevs.

            expect(fetchUserMock).toHaveBeenCalledTimes(3);
            expect(parseUserMock).toHaveBeenCalledTimes(3);
            expect(result).toHaveLength(3);
        })

    });
});

My question is - is this a super clean way of writing functional JavaScript, or is this overkill that is easily solved another way?

If you're interested, there are more examples here.

or is this overkill that is easily solved another way?

_fetchUser and _fetchUsers appear to be the same function; save for the id parameter.

If interpret the question correctly, _fetchUser can be substituted for _fetchUsers (which can be removed entirely) by utilizing default parameters instead of .bind().

For example

async function _makeApiCall (uri) {
  try {
    const response = await Promise.resolve(uri);
    return response;
  } catch (err) {
    throw new Error(err.message);
  }
}

async function _fetchUsers({makeApiCall = _makeApiCall, _URI_USERS = 'abc', id = ''} = {}) {
  return await makeApiCall(_URI_USERS + id);
}

export default _fetchUsers;

<script type="module">
  import _fetchUsers from './script.js';
  (async() => {
    console.log(await _fetchUsers() // 'abc'
               , await _fetchUsers({id:'def'}) // 'abcdef'
               );
  })();
</script>

plnkr


Overloading Java function (insert into binary tree) I have a binary tree below and it seems to make sense to overload the insert() method. If insert() is called without a Node specified, it will just work with head, otherwise if it receives a Node, it will do the insert in the subtree starting with that Node. This second variant is then called recursively, see below.

The repetition that happens in the code below seems wrong to me (not DRY). Is there a better way to do this or is this a limitation in Java?

public class probBinaryTree {
    public Node head;

    public probBinaryTree() {
        this.head = null;
    }

    private class Node {
        int data;
        Node left;
        Node right;
        int size;

        public Node(int d) {
            this.data = d;
            this.left = null;
            this.right = null;
            this.size = 1;
        }
    }

    public Node insert(int d) {
        if (head == null) {
            head = new Node(d);
            return head;
        }
        head.size++;
        if (head.left == null) return insert(head.left, d);
        if (head.right == null) return insert(head.right, d);
        if (head.left.size <= head.right.size) return insert(head.left, d);
        if (head.left.size > head.right.size) return insert(head.right, d);
        return null;
    }

    private Node insert(Node n, int d) {
        if (n == null) {
            n = new Node(d);
            return n;
        }
        n.size++;
        if (n.left == null) return insert(n.left, d);
        if (n.right == null) return insert(n.right, d);
        if (n.left.size <= n.right.size) return insert(n.left, d);
        if (n.left.size > n.right.size) return insert(n.right, d);
        return null;
    }
}

Just have one insert method call the other. In your code, that would look like:

public final class BinaryTree {

    public Node head;

    public BinaryTree() {
        this.head = null;
    }

    private class Node {
        private final int data;
        private final Node left;
        private final Node right;
        private int size;

        public Node(final int d) {
            this.data = d;
            this.left = null;
            this.right = null;
            this.size = 1;
        }
    }

    public Node insert(final int d) {
        if (this.head == null) {
            this.head = new Node(d);
            return this.head;
        }

        return this.insert(this.head, d);
    }

    private Node insert(final Node n, final int d) {
        if (n == null) {
            return new Node(d);
        }

        n.size++;
        if (n.left == null) {
            return this.insert(n.left, d);
        }
        if (n.right == null) {
            return this.insert(n.right, d);
        }
        if (n.left.size <= n.right.size) {
            return this.insert(n.left, d);
        }
        if (n.left.size > n.right.size) {
            return this.insert(n.right, d);
        }
        return null;
    }
}


Reading input from the console in F# In the process of learning F#, I wrote this code that reads lines of input from the console and stores them in a list.

As far as I can tell the code works correctly, but since I'm new to functional programming, I'm not sure if it is written as well as it could have been.

Should I have read characters iteratively instead of recursively?
Is there a better alternative to using a StringBuilder object?
Does my code follow proper functional style?

open System

/// Appends a character to the string builder and returns the new builder.
/// Characters 10 and 13 (newline and carriage return) are ignored.
/// Returns the updated StringBuilder.
let appendChar char (builder : Text.StringBuilder) =
    match char with
    | 10 | 13 -> builder
    | n ->
        let c = Convert.ToChar(n)
        builder.Append(c)

/// Finishes building a string by clearing the StringBuilder
/// and appending the string to the end of the list of strings.
/// Empty strings are ignored.
/// Returns a tuple containing the lines and the new StringBuilder.
let finishString lines (builder : Text.StringBuilder) =
    let s = builder.ToString()
    let l = builder.Length
    let newBuilder = builder.Clear()
    match l with
    | 0 -> (lines, newBuilder)
    | _ -> (lines @ [s], newBuilder)

/// Handles a character by appending it to the builder and taking an appropriate action.
/// If char is a newline, finish the string.
/// Returns a tuple containing lines and the new builder.
let handleChar char lines builder =
    let newBuilder = appendChar char builder
    match char with
    | 10 -> finishString lines newBuilder
    | c -> (lines, newBuilder)

/// Gets all the lines from standard input until end of input (Ctrl-Z).
/// Empty lines are ignored.
/// Returns a list of strings read.
let rec getLines lines builder =
    match Console.Read() with
    | -1 -> lines
    | c ->
        let tuple = handleChar c lines builder
        let newLines = fst tuple
        let newbuilder = snd tuple
        getLines newLines newbuilder

[<EntryPoint>]
let main argv =
    Text.StringBuilder()
    |> getLines []
    |> ... and so on

Some small comments

    let tuple = handleChar c lines builder
    let newLines = fst tuple
    let newbuilder = snd tuple
    getLines newLines newbuilder

can become

    let newlines,newbuilder = handleChar c lines builder
    getLines newLines newbuilder

although, if you aren't using getLines anywhere else, there is a good argument for making it

handleChar c lines builder |> getLines

by makeing getLines take its argument in tupled form.

The magic number 10 appears a few times, so I would add a literal like

[<Literal>]
let newline = 10

and then you can pattern match against it.

Also,

Lines @ [s]

is bad as @ is very slow.  You are better to use

s::Lines

and then reverse the list at the end.

Your format for reading the characters recursively should be fine as it will get optimised as a tail-call.

Of course, your entire program could be replaced by:

let rec procinput lines=
    match Console.ReadLine() with
    | null -> List.rev lines
    | "" -> procinput lines
    | s -> procinput (s::lines)


Rolling median function (Daily Coding Problem 377) Given an array of numbers arr and a window of size k, print out the
  median of each window of size k starting from the left and moving
  right by one position each time.

  For example, given the following array and k = 3:

  [-1, 5, 13, 8, 2, 3, 3, 1]

  Your function should print out the
  following:

  5 <- median of [-1, 5, 13]
  8 <- median of [5, 13, 8]
  8 <- median of [13, 8, 2]
  3 <- median of [8, 2, 3]
  3 <- median of [2, 3, 3]
  3 <- median of [3, 3, 1]

  Recall that the median of an even-sized list is the average of the two middle
  numbers.

Is there any improvement possible in this implementation?

    #include <iostream>
    #include <vector>
    #include <array>
    #include <algorithm>

    template <typename T>
    struct range
    {
        auto begin()const {return  a; }
        auto end()const {return b;}

        T a,b;
    };

    range<const int*>make_range(const int* begin, const int* end)
    {
        return {begin, end};
    }

    range<const int *> make_window(const std::vector<int>& vec, size_t start, size_t end)
    {
        return make_range(vec.data() + start, vec.data() + end);
    }

    int median(int arr[], size_t N)
    {
        //sort the array
        std::sort(arr, arr + N);
        if(N % 2 == 0)
            return (arr[N/2 - 1] + arr[N/2])/2;
        return arr[N/2];
    }

     int main(){
        const auto window_length = 3;
        std::vector<int> input{-1, 5, 13, 8, 2, 3, 3, 1};
        const auto end_length = input.size() - window_length;
        std::array<int, window_length> arr{};

    for(size_t i(0); i < end_length; i++)
    {
        int counter (0);
        const auto window = make_window(input, i, i + window_length);
        for(const auto& value : window)
        {
            std::cout << value << ' ';
            arr[counter++] = value;
        }
        int result = median(arr.data(), arr.size());
        counter = 0;
        std::cout << "\nmedian " << result << '\n';
        std::cout << '\n';
    }

    return EXIT_SUCCESS;
   }

This compiles cleanly and produces reasonable integer output (though not in the format required by the problem statement).

Note that you are required to include <cstdlib> before using EXIT_SUCCESS; failing to do so may be a portability bug.

All that copying and sorting gets expensive as the input and k both get larger.  We should probably create a stateful class that we can update with the new right-most value and the left-most to be removed.  I'd probably implement that with two sets - one holding values lower than the median and one holding higher values.  When we shift the window by one position, then we update the two sets and move an element if necessary to balance them (remember that because a set is sorted, it's easy to find the lowest and highest values using front() and back()).


Performance Improvement for Subtree for another tree I was doing this algo on leetcode: https://leetcode.com/problems/subtree-of-another-tree/

Question:
Given two non-empty binary trees s and t, check whether tree t has exactly the same structure and node values with a subtree of s. A subtree of s is a tree consists of a node in s and all of this node's descendants. The tree s could also be considered as a subtree of itself.

For which I wrote this solution

var isSubtree = function(s, t) {
    const reduceMainTreeToString = JSON.stringify(s)
    const reduceGivenTreeToString = JSON.stringify(t)
    if (reduceMainTreeToString.includes(reduceGivenTreeToString)) return true
    else return false
};

I was super confident that it should be good performance and memory wise but unfortunately, I wasn't even able to cross 50% mark (though I was able to have 100% more memory efficient code).

This is how input s looks like

TreeNode {
  val: 3,
  right: TreeNode { val: 5, right: null, left: null },
  left:
   TreeNode {
     val: 4,
     right: TreeNode { val: 2, right: null, left: null },
     left: TreeNode { val: 1, right: null, left: null } } }

And this is how input t looks like

TreeNode {
  val: 4,
  right: TreeNode { val: 2, right: null, left: null },
  left: TreeNode { val: 1, right: null, left: null } }

This is the constructor:

/**
 * Definition for a binary tree node.
 * function TreeNode(val) {
 *     this.val = val;
 *     this.left = this.right = null;
 * }
 */

Can someone please suggest to me how I can improve the above code?

I was just about to post on your previous version of this question when you pulled it. As you have changed the code my old answer is mute apart from a few points;

The 100% memory efficient is the default value when leetcode has not collect the required minimum stats to provide comparative memory use.

The performance result can vary significantly submitting the very same code. Submit the code several times and you may get a much better score.

The snippet below completed in 62ms and was rated above 99.78% of JavaScript submissions. And got the bogus 100% memory score.

var isNodeEqual = function(a, b) {
    const leftA = a.left !== null, leftB = b.left !== null;
    const rightA = a.right !== null, rightB = b.right !== null;
    if (leftA !== leftB || rightA !== rightB) { return false }
    if ((leftA && a.left.val !== b.left.val) ||
        (rightA && a.right.val !== b.right.val)) { return false }
    return (leftA ? isNodeEqual(a.left, b.left) : true) &&
           (rightA ? isNodeEqual(a.right, b.right) : true);
}
var isSubtree = function(s, t) {
    const stack = [s];
    while (stack.length) {
        const node = stack.pop();
        if (node.val === t.val && isNodeEqual(node, t)) { return true }
        node.left !== null && stack.push(node.left);
        node.right !== null &&  stack.push(node.right);
    }
    return false;
}

Rather than full recursion I used a stack to iterate the main tree, and recursion to test for matching sub tree.

I also avoided truthy and falsey statements as they introduced coercion overhead when testing for null or Object    eg if(node.left) { is slower than if(node.left !== null) {


ES6 Promise with the "co" module I was trying to use the ES6 Promise with the "co" module to write the async code without callback. Though actual async call is neat and clean, I still have to add wrapper method to return Promise object which made it little bulky. Any feedback to improve this code quality will be appreciated.

crud.js
'use strict';
const db = require("./dbmanager").db;
const co = require("co");

module.exports = {

  getAllDocuments: (collectionName)=> {

    //This line of code returning "Promise" object make code bulky. Now I have two wrapper, one for "Promise" and second for "co".
    return new Promise((resolve, reject) => {

      co(function*() {
        let docs = yield db.collection(collectionName).find().toArray();
        resolve(docs);

      }).catch((err) => {
        reject(err);
      });

    });
  }

};

book.js

'use strict';
const dbcrud   = require("./../mongodb/crud");
const co = require("co");
module.exports = {

  getTransactions: (callback) => {
    co(function*() {

      var books = yield dbcrud.getAllDocuments('books');
      callback(null, books);

    }).catch((err) => {
      console.log("Error " + JSON.stringify(err));
      callback(err, null);
    });
  }

};

After going through the "co" GitHub code, I was able to remove the extra boilerplate code. I made following changes to improve my code:

Used the "co" promise and return it in my async function. This help to remove the extra "Promise" wrapper code.
To resolve the "co" promise, I returned the "Promise" as per fails/success scenario.
When making call to MongoDB native driver, used ".catch" to catch the error there itself and apply logic.

Following is my final version which is neater and cleaner using the "co" module:
crud.js
'use strict';
const db = require("./dbmanager").db;
const co = require("co");

module.exports = {

  getAllDocuments: (collectionName)=> {
      //return the "co" promise instead of using "Promise" boilerplate code
      co(function*() {
        let docs = yield db.collection(collectionName).find().toArray().catch(function(err) {
        //in case of error, return the custom message and log the error returned by mongodb driver
        console.log('Error occurred to get documents', err);
        return Promise.reject(new Error(`Error occurred to get documents`)); //return the Promise which will be used by "co"
      });
        return Promise.resolve(docs); //return the Promise which will be used by "co"

      });
  }
};

book.js
'use strict';
const dbcrud   = require("./crud");
const co = require("co");
module.exports = {

  getTransactions: (req, res) => {
   //use "co" to make asyn function call without callback
    co(function*() {
      let transactions = yield dbcrud.getTransactions('transactions');
      res.json(transactions);
    }).catch((err) => {
      console.error(`[ERROR:Controller:API:getTransactions]: error`, err);
      res.status(500);
      res.json({error: err.message});
    });
  },

};


Code to join filesystem path strings in PHP Following code has some issues:

$args = func_get_args();
$paths = array();
foreach ($args as $arg) {
    $paths = array_merge($paths, (array)$arg);
}

$paths = array_map(create_function('$p', 'return trim($p, DIRECTORY_SEPARATOR);'), $paths);
$paths = array_filter($paths);
return DIRECTORY_SEPARATOR . join(DIRECTORY_SEPARATOR, $paths);

Unnecessary array_merge usage
Using depecated create_function function

Would using modern PHP 5.6+ features (anonymous functions etc.. ) help to improve the code?

Seems like the problem has been over thought.  This would be how I would write it.

<?php

//$args = func_get_args();
$args = array('foo/', '/bar', 'fiz');

$paths = [];

foreach ($args as $arg) {
    $paths[] = trim($arg, DIRECTORY_SEPARATOR);
}

$paths = array_filter($paths);

echo DIRECTORY_SEPARATOR . join(DIRECTORY_SEPARATOR, $paths);

I don't see any need to leverage any exclusive PHP 5.6+ features to solve this.


Chat server implemented using Java NIO I am learning Java's NIO and as an exercise, I have implemented a simple chat server. I would like to ask if I am using NIO's features in the correct way.

public class ChatServer implements Runnable {
    private static final int BUFFER_SIZE = 256;
    private static final int DEFAULT_PORT = 10523;
    private static final String WELCOME_MESSAGE = "Welcome to NioChat!\n";

    private final int mPort;
    private ServerSocketChannel mServerChannel;
    private Selector mSelector;
    private ByteBuffer mBuffer = ByteBuffer.allocate(BUFFER_SIZE);
    private final ByteBuffer mWelcomeBuffer = ByteBuffer.wrap(WELCOME_MESSAGE
            .getBytes());

    public ChatServer(int port) throws IOException {
        mPort = port;
        mServerChannel = ServerSocketChannel.open();
        mServerChannel.socket().bind(new InetSocketAddress(port));
        mServerChannel.configureBlocking(false);
        mSelector = Selector.open();
        mServerChannel.register(mSelector, SelectionKey.OP_ACCEPT);
    }

    public static void main(String[] args) throws IOException {
        ChatServer server = new ChatServer(DEFAULT_PORT);
        new Thread(server).start();
    }

    @Override
    public void run() {
        try {
            System.out.println("Server started on port " + mPort);

            Iterator<SelectionKey> iter;
            SelectionKey key;
            while (mServerChannel.isOpen()) {
                mSelector.select();
                iter = mSelector.selectedKeys().iterator();
                while (iter.hasNext()) {
                    key = iter.next();
                    iter.remove();

                    if (key.isAcceptable())
                        handleAccept(key);
                    if (key.isReadable())
                        handleRead(key);
                }
            }
        } catch (IOException e) {
            System.out.println("IOException, server on port " + mPort
                    + " terminating. Stack trace:");
            e.printStackTrace();
        }
    }

    private void handleAccept(SelectionKey key) throws IOException {
        SocketChannel sc = ((ServerSocketChannel) key.channel()).accept();
        String address = (new StringBuilder(sc.socket().getInetAddress()
                .toString())).append(":").append(sc.socket().getPort())
                .toString();
        sc.configureBlocking(false);
        sc.register(mSelector, SelectionKey.OP_READ, address);
        sc.write(mWelcomeBuffer);
        mWelcomeBuffer.rewind();
        System.out.println("Accepted connection from: " + address);
    }

    private void handleRead(SelectionKey key) throws IOException {
        SocketChannel ch = (SocketChannel) key.channel();
        StringBuilder sb = new StringBuilder();

        mBuffer.clear();
        int read = 0;
        while ((read = ch.read(mBuffer)) > 0) {
            mBuffer.flip();
            byte[] bytes = new byte[mBuffer.limit()];
            mBuffer.get(bytes);
            sb.append(new String(bytes));
            mBuffer.clear();
        }
        String msg;
        if (read < 0) {
            msg = key.attachment() + " left the chat.\n";
            ch.close();
        } else {
            msg = key.attachment() + ": " + sb.toString();
        }

        System.out.println(msg);
        broadcast(msg);
    }

    private void broadcast(String msg) throws IOException {
        ByteBuffer messageBuffer = ByteBuffer.wrap(msg.getBytes());
        for (SelectionKey key : mSelector.keys()) {
            if (key.isValid() && key.channel() instanceof SocketChannel) {
                SocketChannel channel = (SocketChannel) key.channel();
                channel.write(messageBuffer);
                messageBuffer.rewind();
            }
        }
    }
}

Bad support for fragmented tcp packets

StringBuilder sb = new StringBuilder();

mBuffer.clear();
int read = 0;
while ((read = ch.read(mBuffer)) > 0) {
    mBuffer.flip();
    byte[] bytes = new byte[mBuffer.limit()];
    mBuffer.get(bytes);
    sb.append(new String(bytes));
    mBuffer.clear();
}

The above implementation only works reliable with messages smaller than 567 (IPV4) or 1200 (ipv6) bytes, because larger messages can come in fragmented in multiple chunks with large intervals in case of packet loss. This will result is half send chat messages that are broadcasted

Using default charset

You are not specifying any charset when reading the messages, this will result in platform and even java dependent behaviour when reading special characters.

Manually using stringbuilder

String address = (new StringBuilder(sc.socket().getInetAddress()
                .toString())).append(":").append(sc.socket().getPort())
                .toString();

You don't need to use stringbuilder if you already have all the used arguments on 1 line, the compiler will automatically add a stringbuilder for you.

String address = sc.socket().getInetAddress() + ":" + sc.socket().getPort();


Remove adjacent duplicates in golang This is an exercise in a book which ask me to implement

  Write an in-place function to eliminate adjacent duplicates in a []string slice.

I am relatively new to golang and I am not sure if my implementation is correct and effective or not.

func removeAdj(strings []string) []string {
    for i := 0; i < len(strings); i++ {
        dup := false
        lastJ := i
        for j := (i + 1); j < len(strings); j++ {
            if strings[i] == strings[j] {
                dup = true
                lastJ = j
            } else {
                break
            }
        }

        if dup {
            strings[i] = ""
            first := strings[:i]
            second := strings[lastJ+1:]

            strings = append(first, second...)
            i = -1
        }
    }

    return strings
}

Tests:

Input:  [a z x x z y]
Output: [a y]
Input:  [g e e k s f o r g e e g]
Output: [g k s f o r]
Input:  [c a a a b b b a a c d d d d]
Output: []
Input:  [a c a a a b b b a c d d d d]
Output: [a c a c]

Your implementation, to me at least, doesn't seem correct. You need to remove adjacent duplicates, but looking at your last example, the sequence acaaabbacdddd completely removes the b and d characters from the slice. You're also using an awful lot of code to do a simple thing. What I'd do is quite simply this:

Iterate over the slice from index 0 to the next to last character
For each character, iterate over the remainder of the slice (nested loop) until you find a character that doesn't equal the current index
For each character at the current position + 1 that matches the current one, remove it, as it's an adjacent duplicate.

The code itself is quite simple:

func dedup(s []string) []string {
    // iterate over all characters in slice except the last one
    for i := 0; i < len(s)-1;i++ {
        // iterate over all the remaining characeters
        for j := i+1; j < len(s); j++ {
            if s[i] != s[j] {
                break // this wasn't a duplicate, move on to the next char
            }
            // we found a duplicate!
            s = append(s[:i], s[j:]...)
        }
    }
    return s
}

Given an input like [g e e k s f o r g e e g], the output of this is [g e k s f o r g e g]

Demo

The only trickery here is this line: s = append(s[:i], s[j:]...). What this effectively does is reassign the slice s to contain i values starting at 0 (so if i is 2, the slice will be [g, e]). The second part is creating a slice starting at j, until the end of s. Again, if j is 2, this slice will be all values starting at offset 2 until the end ([e k s f o r g e e g]).

So let's look at an actual example:

i == 1
j == i+1 (2)
s[i] == e, s[j] ==e`

We have a duplicate, so we'll reassign s like so:

 s = append(s[:1], s[2:]...)`

This means we're appending [e k s f o r g e e g] to [g], removing the duplicate e. Job done.

Note:

I've used []string here, but it should go without saying that a slice of characters is probably best represented as either []byte or []rune (for full UTF-8 support). Regardless of the type you end up using, the code above will work with with any type that can be compared with the == operator


Prefix- and z-functions in C++ (string algorithms) Let us define prefix-function(string)[i] as the length of the largest prefix of string, which is also a suffix of string[1..i], and z-function(string)[i] is the length of the largest prefix of string, which is also a prefix of string[i..].

Unfortunately, there is very little information on them in English, but I found a presentation on Knuth-Morris-Pratt algorithm containing description of prefix-function and an article on z-function.

Here is my implementation of prefix-function and z-function and conversions between them in linear time on the length of string using pure C++. Comments are appreciated! In particular, I'm interesting how to determine a string from z-function directly.

std::vector<int> prefix_function (const std::string& str) {
    std::vector<int> prefs (str.size(), 0);
    for (int i = 1; i < str.size(); ++i) {
        int pref = prefs[i-1];
        while (pref > 0 && str[i] != str[pref]) {
            pref = prefs[pref-1];
        }
        if (str[i] == str[pref]) {
            ++pref;
        }
        prefs[i] = pref;
    }
    return prefs;
}

std::vector<int> z_function(const std::string& str) {
    std::vector<int> zfunc (str.size(), 0);
    zfunc[0] = str.size();
    for (int i = 1, left = 0, right = 0; i < str.size(); ++i) {
        if (i <= right) {
            zfunc[i] = std::min (right - i + 1, zfunc[i - left]);
        }
        while (i + zfunc[i] < str.size() && str[zfunc[i]] == str[i + zfunc[i]]) {
            ++zfunc[i];
        }
        if (i + zfunc[i] - 1 > right) {
            left = i;
            right = i + zfunc[i] - 1;
        }
    }
    return zfunc;
}

std::string from_prefix_function(const std::vector<int>& prefs) {
    std::string str (prefs.size(), '.');
    char current_symbol = 'a';
    for (int i = 0; i < prefs.size(); ++i) {
        if (prefs[i] > 0) {
            str[i] = str[prefs[i] - 1];
        } else {
            str[i] = current_symbol++;
        }
    }
    return str;
}

std::vector<int> prefix_to_z(const std::vector<int>& prefs) {
    return z_function(from_prefix_function(prefs));
}

std::vector<int> z_to_prefix(const std::vector<int>& z_func) {
    std::vector<int> prefs (z_func.size(), 0);
    for (int i = 1; i < z_func.size(); ++i) {
        prefs[i + z_func[i] - 1] = std::max(prefs[i + z_func[i] - 1], z_func[i]);
    }
    for (int i = z_func.size() - 2; i >= 0; --i) {
        prefs[i] = std::max(prefs[i+1] - 1, prefs[i]);
    }
    return prefs;
}

This will be a somewhat strange answer but the thing is that I can't find anything wrong with your code: it is not that easy to understand what it is doing, but it is only because the algorithms are not the most straightforward in the world; if we compare your code and the algorithms pseudo-code, it's easy to see that they are performing the same computations. Everything seems well-written, the language is used as it should be used... In the end, I only have a few almost-irrelevant remarks left:

The only obvious thing you could do is to templatize your algorithm so that it works for any std::basic_string specialization and not only for std::string.
If you strive for speed and can overcome a little less readability (generally, you don't), you could try to change this piece of code:

if (str[i] == str[pref]) {
    ++pref;
}

into this one:

pref += str[i] == str[pref];

But even then, time it, time it again, and I'm pretty sure that the compiler is somehow smart enough to get rid of that branch for you and that it won't impact the overall speed of the algorithm in any significant way.
You are slightly inconsistent in the way you call the functions: you wrote std::min (/* ... */) but also std::max(/* ... */). You should either put a space everywhere or remove it everywhere, but be consistent.

Well, as you see, there is little to say about what you wrote. It's some pretty good code. Great job writing it! :)


Simple k-means implemention using Python3 and Pandas Is there anything I can improve? The distance function is Pearson correlation.

import os
import pandas as pd
import numpy as np
from pandas import Series, DataFrame

def corrpairs(df1, df2):
    """
    Pairwise correlation for columns of two data frames
    :param df1:
    :type df1:
    :param df2:
    :type df2:
    :return:
    :rtype: pandas.core.frame.DataFrame
    """
    return df1.apply(lambda x: df2.corrwith(x))

import pdb
def kcluster(cols, k=4):
    """
    K Means clustering algorithm, applied to columns of a data frame.
    Using Pearson correlation as the distance function.
    :param rows:
    :type rows: pandas.core.frame.DataFrame
    :param k:
    :type k: int
    :return:
    :rtype: list[int]
    """
    cols = cols.astype(float)
    nrow, ncol = cols.shape
    nuclear0 = cols.iloc[:, :k]
    nuclear0.columns = range(k)
    nuclear0 += np.random.randn(np.prod(nuclear0.shape)).reshape(nuclear0.shape)

    correlations = corrpairs(cols, nuclear0)
    groups = correlations.idxmax(axis=0)
    nuclear1 = []
    for i in range(k):
        sub_cols = cols.loc[:, groups == i]
        sub_mean = sub_cols.mean(axis=1)
        nuclear1.append(sub_mean)
    nuclear1 = pd.concat(nuclear1, axis=1)

    while ((nuclear0 - nuclear1).abs() > 0.00001).any().any():
        print(nuclear0)
        print(nuclear1)
        print((nuclear0 - nuclear1).abs())
        nuclear0 = nuclear1
        correlations = corrpairs(cols, nuclear0)
        groups = correlations.idxmax(axis=0)
        nuclear1 = []
        for i in range(k):
            sub_cols = cols.loc[:, groups == i]
            sub_mean = sub_cols.mean(axis=1)
            nuclear1.append(sub_mean)
        nuclear1 = pd.concat(nuclear1, axis=1)

    return groups

You have a few problems with your documentation.

First off, your documentation is incomplete in a few places.

For example, in your corrpairs function, you didn't fill in any of your documentation, except for the rtype part.

And, in your kcluster function, you only filled in type rows, type k, and rtype.

Finally, also in kcluster, you called the parameter "rows" in the documentation and called it "cols" in the function signature. Choose one and stick with it.

Documentation is a very important part of every function.

import pdb
def kcluster(cols, k=4):

You should not have an import in the middle of your code; all the importing should be done at the very top of your code like you were doing before.

This review was just mean to point out practices. I had trouble understanding the content of the code (a better documentation probably would've helped).


Reproduce the system() function of linux My mentor says to reproduce the system() function of Linux exactly as given in the man pages. I have written the below code. Could someone review this code and provide me any suggestions for improvising. I am a beginner. Thanks :).

 #include<stdio.h>
#include<unistd.h>
#include<sys/types.h>
#include<sys/wait.h>
#include<error.h>
int my_system(const char *command);

int my_system(const char *command)
{
    int ret = 0;

    ret = execl("/bin/sh", "sh", "-c", command, (char *)NULL);
    if (ret == -1)
        error(1, 0, "error occcured in the execl() system call\n");
}

int main(int argc, char *argv[])
{
    pid_t pid;
    int ret;
    int wstatus;

    if (argc > 2)
        error(1, 0, "Too many arguments\n");
    pid = fork();
    if (pid == -1) {
        error(1, 0, "error in creating the sub-process\n");
    } else if (pid == 0) {
        ret = my_system(argv[1]);
    } else {
        wait(NULL);
    }
    return 0;
}

```

Enable compiler warnings and fix all of them

I immediately see that your function my_system(), which should return an int, does not contain a return statement. That means the return value is undefined. If you enable compiler warnings, the compiler will complain about such mistakes.

Don't add unnecessary forward declarations

Why add a forward declaration of my_system() right before the actual implementation? This is an unnecessary repetition of code. It's best to always try to not repeat yourself.

Read the man page again and pay attention to the details

You implemented the call to execl() exactly as written in the man page, but that is only the easy part. You have to ensure that the return value and the behavior in the presence of errors is exactly as the man page describes.

In particular, system() does not call error() when there is an error, instead the return value and the value of errno is used to convey the type of error that has occurred.

Create a test suite before making any changes to my_system()

You could change my_system() right now to fix any immediately obvious errors, but the temptation is very high that afterwards you consider the implementation to be complete. Instead, try to create a test suite first that calls my_system() in different ways to ensure that its behavior matches that given in the man page.
Also, instead of making assumptions of what the required behavior is, in this case you can just check what the actual system() function does.

So such a test suite could look like:

#include <assert.h>
...
const char *commands[] = {
    "/bin/true",
    "/bin/false",
    // add more commands here
    "",
    NULL,
};

for (int i = 0; ; i++) {
    int my_ret = my_system(commands[i]);
    int my_errno = errno;
    int ret = system(commands[i]);
    assert(my_ret == ret);
    if (ret == -1)
        assert(my_errno == errno);

    if(!commands[i])
        break;
}
```


Static variable Current in Application I was finding myself frequently calling getApplication() and casting it as MyApplication so that I could call a method in MyApplication.  I decided to declare the static variable Current in MyApplication and set it in the constructor.  Now I don't have to do the casting.

Are there any pitfalls to taking this approach?

public class MyApplication extends Application {
    public static MyApplication Current;
    public MyApplication(){
        super();
        Current = this;
    }
    public void doSomething(){

    }
}

public class MyActivity extends ListActivity {
    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        MyApplication.Current.doSomething();
    }
}

Below is, I think, the normal approach for getting the application and calling the method.

public class MyApplication extends Application {
    public void doSomething(){

    }
}

public class MyActivity extends ListActivity {
    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        ((MyApplication)getApplication()).doSomething();
    }
}

With this solution there is a danger that somebody accidentally change MyApplication.current to null or an invalid reference.

I'd override the getApplication method in the MyActivity class with a covariant return type:

@Override
public MyApplication getApplication() {
    return (MyApplication) super.getApplication();
}

According to the Code Conventions for the Java Programming Language, Current should be current.


Encrypt a file using Botan library I need to build something that is not very complicated, but efficient. I don't have much time to debug.

I don't know if this method is strong enough to encrypt a data to be displayed in a Qr code.

It will be compiled in Qt.

    #include <botan/botan.h>
    #include <botan/aes.h>
    #include <botan/kdf.h>
    #include <botan/pbkdf2.h>
    #include <botan/hmac.h>
    #include <botan/sha160.h>

    using namespace std;
    using namespace Botan;

    // .....

    bool BotanWrapper::EncryptFile(QString Source, QString Destination)
    {
    try
    {
    //Setup the key derive functions
    PKCS5_PBKDF2 pbkdf2(new HMAC(new SHA_160));
    const u32bit PBKDF2_ITERATIONS = 8192;

    //Create the KEY and IV
    KDF* kdf = get_kdf("KDF2(SHA-1)");

    //Create the master key
    SecureVector<byte> mMaster = pbkdf2.derive_key(48, mPassword.toStdString(), &mSalt[0], mSalt.size(),PBKDF2_ITERATIONS).bits_of();
    SymmetricKey mKey = kdf->derive_key(32, mMaster, "salt1");
    InitializationVector mIV = kdf->derive_key(16, mMaster, "salt2");

    string inFilename = Source.toStdString();
    string outFilename = Destination.toStdString();
    std::ifstream in(inFilename.c_str(),std::ios::binary);
    std::ofstream out(outFilename.c_str(),std::ios::binary);

    Pipe pipe(get_cipher("AES-256/CBC/PKCS7", mKey, mIV,ENCRYPTION),new DataSink_Stream(out));
    pipe.start_msg();
    in >> pipe;
    pipe.end_msg();

    out.flush();
    out.close();
    in.close();

    return true;
   }
   catch(...)
   {
    return false;
    }
   }

I don't know the Botan library, so I'll stick to some general recommendations.

Don't include whole namespaces at global scope, especially the std namespace.

The only use of Qt in this function is to accept QString arguments, with which we do nothing but convert to standard strings.  So why not remove the dependency, and allow the function to be used in non-Qt projects, by accepting the filenames as const std::string&?  We can keep the interface by providing a (header-only, trivially inlinable) wrapper separately as a convenience for use with Qt:

//encrypt.h
bool EncryptFile(const std::string& source, const std::string& destination);

//encrypt-qt.h
bool EncryptFile(QString source, QString destination)
{ return EncryptFile(source.toStdString(), destination.toStdString(); }

There's certainly no need to use c_str() when passing to the streams' constructors - a std::string is fine (and expected) there.

I'd probably go one further with the overloads, and provide a version that accepts a pair of streams, so that we could use this for network streaming or in-memory encryption:

//encrypt.h
bool EncryptFile(std::istream& source, std::ostream& destination);

bool EncryptFile(const std::string& source, const std::string& destination);
{
    try {
        std::ifstream in(source, std::ios::binary);
        std::ofstream out(destination, std::ios::binary);
        return EncryptFile(in, out);
    } catch (...) {
        return false;
    }
}

Why do we return true, even if writing the output failed?  For example, try passing /dev/full as the destination.  We could fix that:

return in && out;

Alternatively, we could arrange for the streams to throw exceptions:

    in.exceptions(std::ifstream::failbit | std::ifstream::bad);
    out.exceptions(std::ifstream::failbit | std::ifstream::bad);


Using RSpec subject to setup a test I find myself using subject blocks in my tests to setup example because it is succinct. However is this considered bad form and can it have undesired  consequences?

require 'spec_helper'

describe "stations/index" do

  before(:each) do
    stub_user_for_view_test
    assign(:stations, [
      stub_model(Station),
      stub_model(Station)
    ])

  end

  context "when not an admin" do
    subject {
      render
      rendered
    }

    it { should match /[s|S]tations/ }
    it { should have_selector('.station', :minimum => 2) }
    it { should_not have_link 'Edit' }
    it { should_not have_link 'Delete' }
  end

  context "when an admin" do

    subject {
      @ability.can :manage, Station
      render
      rendered
    }

    it { should have_link 'Edit' }
    it { should have_link 'Delete' }
  end

end

There are no unintended consequences, but it is a little unusual and may surprise the reader.  However, it is a small surprise.

It usually communicates intent well to use subject to declare the subject, and before to setup preconditions unrelated to the subject.  For preconditions that involve the subject, those are often better in a before block as well, but it's not too surprising to find them in the subject block.

So, instead of this:

subject {
  @ability.can :manage, Station
  render
  rendered
}

this:

before(:each) do
  @ability.can :manage, Station
  render
end

subject {rendered}

I used do...end for the multi line block and {} for the single line block; this is a common (although not ubiquitous) practice.


Code to redirect console IO to strings This code allows calling a method while redirecting the IO to strings.  This is useful for unit testing where you have to direct something to the console or examine output that comes from it.  I assume these have been written a lot, but I couldn't find examples.

I'd like advice on how to make them generally useful and whether the style seems decent.  There are two methods-- one sends a string to System.in while leaving the output alone, the other redirects both input and output, with the option that if the input string passed is null, it leaves the input alone.  This covers all three cases.

Invocations look like this:

enterInput(inputString, callingClassString, methodString, typeStrings[], argsForCall ...);
captureIO(inputString, callingClassString, methodString, typeStrings[], argsForCall ...);

Code:

/**
 * Invoke the specified method using the specified string as console input.
 * Note that the calling class and types must be fully qualified.
 *
 * @param inputString               Console input to be fed to the method
 * @param callingClassString        Class of the method to invoke
 * @param method                    Name of the method to invoke
 * @param typeStrings               Types the method takes (to distinguish it)
 * @param args                      Arguments to pass to the method
 * @return                          The console output captured
 */
public static void enterInput(String inputString,
        String callingClassString, String method, String[] typeStrings,
        Object... args) {
    try {
        Class<?>[] typeClasses = new Class<?>[typeStrings.length];
        for (int i = 0; i < typeStrings.length; i++) {
            typeClasses[i] = Class.forName(typeStrings[i]);
        }

        Method m = Class.forName(callingClassString).getMethod(method, typeClasses);
        InputStream origIn = System.in;
        System.setIn(new ByteArrayInputStream(inputString.getBytes()));
        m.invoke(null, args);
        System.setIn(origIn);
    } catch (Exception e) {
        throw new Error(e);
    }
}

/**
 * Invoke the specified method and return its console output as a string.
 * If inputString is not null, it is used as console input.
 * Note that the calling class and types must be fully qualified.
 *
 * @param inputString               Console input to be fed to the method.
 *                                  If null, leaves this input stream alone
 * @param callingClassString        Class of the method to invoke
 * @param method                    Name of the method to invoke
 * @param typeStrings               Types the method takes (to distinguish it)
 * @param args                      Arguments to pass to the method
 * @return                          The console output captured
 */
public static String captureIO(String inputString, String callingClassString, String method,
        String[] typeStrings, Object ... args) {
    String output = null;

    try {
        Class<?>[] typeClasses = new Class<?>[typeStrings.length];
        for (int i=0; i<typeStrings.length; i++) {
            typeClasses[i] = Class.forName(typeStrings[i]);
        }

        Method m = Class.forName(callingClassString).getMethod(method, typeClasses);
        InputStream origIn = System.in;
        PrintStream origOut = System.out;

        if (inputString != null)
            System.setIn(new ByteArrayInputStream(inputString.getBytes()));
        ByteArrayOutputStream baos = new ByteArrayOutputStream();
        System.setOut(new PrintStream(baos));
        m.invoke(null, args);
        output = baos.toString();

        System.setOut(origOut);
        if (inputString != null)
            System.setIn(origIn);
    } catch (Exception e) {
        throw new Error(e);
    }
    return output;
}

This (and reflection in general) completely breaks refactoring, static code analysis and many more things and should therefore be avoided. (Imagine what happens if you find out a better naming for a function or you change the arguments.)

If you really need this behavior, you could use (I assume jUnit 4+) hooks before and after class/test case:

@BeforeClass
public static void setUpBeforeClass() throws Exception {
}

@AfterClass
public static void tearDownAfterClass() throws Exception {
}

@Before
public void setUp() throws Exception {
}

@After
public void tearDown() throws Exception {
}

and do whatever is necessary.
Besides this, as already written, I would suggest to modify your methods, too. It could be a better approach to separate the view (prints) from model/controller (functions doing something).


ReentrantLock with priorities for waiting threads I am trying to have a ReentrantLock with another way to determinate which thread will have the lock in the waiting queue. ReentrantLock implementation manages a Fair/Unfair policy, that's not what I want.

I would like to give a priority when I call the .lock() method, and this priority will determinate the order of current thread in the waiting threads queue.

I wrote a class, based on a PriorityBlockingQueue, and I'd like your opinion.

import java.util.Date;
import java.util.concurrent.PriorityBlockingQueue;

/**
 * Class to lock a resource.
 * When multiple threads are waiting for the resource to be unlocked, the one with the greatest priority will have the resource.
 * When two threads have the same priority, the first added thread will lock the resource.
 *
 */
public class PrioritizedLock {

  private PriorityBlockingQueue<PrioritizedThread> waitingQueue = new PriorityBlockingQueue<PrioritizedThread>();

  private volatile PrioritizedThread activeThread;

  private volatile Object syncStartNext = new Object();

  private volatile boolean shutdown = false;

  public PrioritizedLock() {
    startQueueWatcher();
  }

  /**
   * Starts a queue watcher, which takes PrioritizedThread from the PriorityBlockingQueue and notify them to wake them up.
   */
  private void startQueueWatcher() {
    new Thread(new Runnable() {

      @Override
      public void run() {
        while (!shutdown) {

          /*
           * Wait until a PrioritizedThread is added to the PriorityBlockingQueue
           */
          while (activeThread == null) {
            try {
              activeThread = waitingQueue.take();
            } catch (InterruptedException e1) {
              //
            }
          }

          /*
           * Notify the thread to wake up (now it has locked the resource)
           */
          synchronized (activeThread) {
            activeThread.notify();
          }

          /*
           * Wait until the resource has been released from the active thread
           */
          while (activeThread != null) {
            synchronized (syncStartNext) {
              try {
                syncStartNext.wait();
              } catch (InterruptedException e) {
                //
              }
            }
          }

        }
      }
    }).start();
  }

  /**
   * Waits until the thread is woken up by the Queue Watcher thread
   * @param thread
   */
  private void wait(PrioritizedThread thread) {
    synchronized (thread) {

      while (activeThread != thread) {
        try {
          thread.wait();
        } catch (InterruptedException e) {
          //
        }

      }
    }

  }

  static class PrioritizedThread implements Comparable<PrioritizedThread> {
    Thread thread;

    int priority;

    Date date = new Date();

    public PrioritizedThread(Thread aThread, int aPriority) {
      thread = aThread;
      priority = aPriority;
    }

    @Override
    public int compareTo(PrioritizedThread other) {
      int diffPriorities = this.priority - other.priority;

      if (diffPriorities == 0) {
        return date.compareTo(other.date);
      }
      return diffPriorities;
    }
  }

  /**
   * Waits until the resource is locked by the current thread
   * @param priority Priority of the lock, the less has the greatest priority
   */
  public void lock(int priority) {
    PrioritizedThread prioritizedThread = new PrioritizedThread(Thread.currentThread(), priority);

    waitingQueue.add(prioritizedThread);
    wait(prioritizedThread);
  }

  /**
   * Unlock the resource
   */
  public void unlock() {
    activeThread = null;

    synchronized (syncStartNext) {
      syncStartNext.notify();
    }
  }

  /**
   * Trigger a shutdown of the lock,
   */
  public void shutdown() {
    shutdown = true;
  }

}

After calling shutdown(), the shutdown boolean may never get checked. The Thread may be stuck in waitingQueue.take().

It's probably not a good idea to catch and swallow InterruptedException.

Considering those two points together, it makes sense to remove the Thread and use Executors.newSingleThreadExecutor() instead, and delegate to its shutdown() method.

Finally, it's fairly low-level and error prone to use wait() and notifyAll(). Using notify() is even riskier than notifyAll(). I'd recommend using a Condition instead.


Efficient solution to Project Euler 14 (Longest Collatz sequence) This is an iterative review. I asked for reviews on a Collatz sequence generator here. I tried to address all of the suggestions that were made.

I've added a new function get_longest_collatz(n), which can be used to solve Project Euler problem 14 (the question states: which number under a million has the longest Collatz sequence?)

The solution makes use of memoization.

def collatz(n):
    """
    Generator for collatz sequence beginning with n.
    n must be 0, 1, 2 ...

    >>> list(collatz(1))
    [1]

    >>> list(collatz(10))
    [10, 5, 16, 8, 4, 2, 1]
    """
    yield n
    while n > 1:
        n = n // 2 if n % 2 == 0 else 3 * n + 1
        yield n

def get_longest_collatz(n):
    """
    Returns the number less than n with the longest collatz sequence.

    >>> get_longest_collatz(1000)
    871
    """
    lengths = {} # Keys are natural numbers, values are lengths of collatz sequence
    for i in range(n):
        for count, x in enumerate(collatz(i)):
            if x in lengths:
                # The sequence length for x has already been found
                lengths[i] = count + lengths[x] # Add onto previously found sequence length
                break
        else:
            lengths[i] = len(list(collatz(i))) # Store full length of collatz sequence
    return max(lengths, key=lengths.get) # Key of memo with largest value

n must be 0, 1, 2 ...

Should be:

n must be 1, 2, 3 ...

Or say that:

n must be a positive integer.

Your code doesn't give an infinite loop, but it doesn't make sense really that you return [0] for your function. If you "allowed" 0 the collatz funciton would return an infinite list of 0s. (Why?)


Rails Helper to display rating in stars Using Rails 6. Here's a piece that I wrote just to display number of stars. Obviously I am disgusted by my own code. How would you refactor?

# application_helper.rb
module ApplicationHelper
  def show_star_rating(rating)
    zero_star_icon_name = "star"
    full_star_icon_name = "star_fill"
    half_star_icon_name = "star_lefthalf_fill"
    total_stars = []
    round_by_half = (rating * 2).round / 2.0
    (round_by_half.to_i).times { total_stars << full_star_icon_name }

    if round_by_half - round_by_half.to_i == 0.5
      total_stars << half_star_icon_name
    end

    if total_stars.size != 5
      (5 - total_stars.size).times { total_stars << zero_star_icon_name }
    end

    total_stars
  end
end

# show.html.erb
<% show_star_rating(agent_review.rating).each do |star| %>
  <i class="f7-icons"><%= star %></i>
<% end %>

You can make use of the Array.new, passing in the maximum number of stars you want to show, and defaulting all the stars to empty. Then, you can fill in the number of full stars you need. Then, finally, thanks to Numeric's divmod returning either 0 or 1 for the number of half stars you need, you make one more pass and fill in the number of half stars you need:

module StarHelper
  EMPTY_STAR_ICON = 'star'.freeze
  FULL_STAR_ICON = 'star_fill'.freeze
  HALF_STAR_ICON = 'star_lefthalf_fill'.freeze

  def full_and_half_star_count(rating)
    (rating * 2).round.divmod(2)
  end

  def stars(rating, max_stars: 5)
    full_stars, half_stars = full_and_half_star_count(rating)

    Array.new(max_stars, EMPTY_STAR_ICON).
      fill(FULL_STAR_ICON, 0, full_stars).
      fill(HALF_STAR_ICON, full_stars, half_stars)
  end
end


Caching lookup values in an e-commerce website I am working on an e-commerce website. I have a CarMake dropdown which contains car make names and their corresponding Id, for example: { 1:Alfa romeo}, { 2:Audi }, { 3:BMW }, etc

I want to keep this lookup in cache, so I won't have to make a trip to DB every time I want to get the corresponding Name or Id...

So in my infrastructure layer, I read the data from DB and keep it in cache, I am using a static CarMakeLookup class:

public static class CarMakeLookup
{
    private static readonly Dictionary<short, string> _carMakeIdLookup;
    private static readonly Dictionary<string, short> _carMakeNameLookup;

    static CarMakeLookup()
    {
        _carMakeIdLookup = new Dictionary<short, string>();
        _carMakeNameLookup = new Dictionary<string, short>();

        using (var context = ApplicationDbContext.Create())
        {
            DropdownRepository dropdownRepository = new DropdownRepository(context);

            var carMakes = dropdownRepository.GetCarMakes();
            foreach (var carMake in carMakes)
            {
                _carMakeIdLookup[carMake.CarMakeId] = carMake.CarMakeName;
                _carMakeNameLookup[carMake.CarMakeName.ToLower()] = carMake.CarMakeId;
            }
        }
    }

    public static string GetCarMakeName(short carMakeId)
    {
        if (carMakeId <= 0)
        {
            return string.Empty;
        }

        if (_carMakeIdLookup.ContainsKey(carMakeId))
        {
            return _carMakeIdLookup[carMakeId];
        }

        return string.Empty;
    }

    public static short GetCarMakeId(string carMakeName)
    {
        if (!string.IsNullOrEmpty(carMakeName))
        {
            string lowerCaseCarMakeName = carMakeName.ToLower();
            if (_carMakeNameLookup.ContainsKey(lowerCaseCarMakeName))
            {
                return _carMakeNameLookup[lowerCaseCarMakeName];
            }
        }

        return -1;
    }

    public static Dictionary<short, string> GetCarMakeLookup()
    {
        return _carMakeIdLookup;
    }
}

I also need to display car make in a dropdown in my View... so I need a List<SelectListItem> to be displayed as a dropdown. So in my Application layer I have another static class which holds this list:

public static class CarCache
{
    static CarCache()
    {
        var selectCarMake = new SelectListItem { Value = "-1", Text = "Select a car", Disabled = true, Selected = true };
        CarMakeItemsIncludingSelect = new List<SelectListItem>(); // Dropdown's first option should be "select a car"
        CarMakeItemsIncludingSelect.Insert(0, selectCarMake);

        foreach (KeyValuePair<short, string> carMakeKeyPair in CarMakeLookup.GetCarMakeLookup())
        {
            var selectListItem = new SelectListItem { Value = carMakeKeyPair.Key.ToString(), Text = carMakeKeyPair.Value };
            CarMakeItemsIncludingSelect.Add(selectListItem);
        }
    }

    public static List<SelectListItem> CarMakeItemsIncludingSelect { get; private set; }

    public static string GetCarMakeName(short carMakeId)
    {
        return CarMakeLookup.GetCarMakeName(carMakeId);
    }

    public static short GetCarMakeId(string carMakeName)
    {
        return CarMakeLookup.GetCarMakeId(carMakeName);
    }
}

I have a few more dropdowns like this in my application, for example HouseType, JobType, etc

Everything works fine, the only problem is that my application startup time is slow... I assume one problem is that when I start debugging the application, the application starts building all these static cache classes, and since these classes are static I cannot inject the DbContext into them... so each dropdown should build its own DdContext instance, get corresponding data from DB and Dispose of DbContext. There is no performance problem with the production server...

Any recommendation on how to improve this cache?

Global static state makes your code untestable...

I would define your car make lookup as:

public class CarMakeLookup
{
    public CarMakeLookup(IEnumerable<(short Id, string Make)> data)
    {
        Makes = data.ToDictionary(d => d.Id, d => d.Make);
        Ids = data.ToDictionary(d => d.Make, d => d.Id);
    }

    public IReadOnlyDictionary<short, string> Makes { get; }
    public IReadOnlyDictionary<string, short> Ids { get; }
}

And read it with:

public class CarMakeReader : IReader<CarMakeLookup>
{
    public CarMakeReader(Func<ApplicationDbContext> context) =>
        Context = context;

    Func<DbContext> Context { get; }

    public Task<CarMakeLookup> ReadAsync()
    {
        using (var context = Context())
            return new CarMakeLookup(
                from cm in context.CarMakes
                select (cm.CarMakeId, cm.CarMakeName));
    }
}

Where IReader<> is simply:

public interface IReader<TSet>
{
    Task<TSet> ReadAsync();
}

There should be also an extension method Cache defined nearby:

public static class Reader
{
    public static IReader<TSet> Cache<TSet>(this IReader<TSet> source) =>
        new CachingReader<TSet>(source);
}

Where:

class CachingReader<TSet> : IReader<TSet>
{
    public CachingReader(IReader<TSet> source) =>
        Lookup = new Lazy<Task<TSet>>(() => source.ReadAsync());

    Lazy<Task<TSet>> Lookup { get; }
    public Task<TSet> ReadAsync() => Lookup.Value;
}

Now register you reader in the IoC container as a decorated singleton:

containerBuilder.RegisterInstance(ctx => new CarMakeLookup(
    ctx.Resolve<Func<ApplicationDbContext>>())
    .Cache()).AsImplementedInterfaces();


How to parse a collection of objects in Backbone? In my Backbone.Collection I need to parse the response before to render it in Backbone.View.  The following code works, but it will be great to have some suggestions:

// response is array of object
// [{id:1, prop: null },{id:2, prop: "bar" }]

// the output parsed_response can be
//[{id:1, prop: null, isProp: true },
// {id:2, prop: "bar", isProp: false }]

parse: function(response)
{
    var parsed_response;

    parsed_response = _(response).clone();

    var parsed_response = _.map(parsed_response, function (obj) {
        return function (_obj) {
            if (_obj.prop === "" || typeof _obj.prop === "null" ) {
                _obj.isProp = false;
            }
            else {
                _obj.isProp = true;
            }
            return _obj;
        }(obj)
    });

    return parsed_response;
}

My questions are:

Is there a way to improve the code?  Also renaming the name of variables to make it more clear.
Since this code should be reused from other collections, what is the best way to generalise it?

Based on the way your parse function works you're worried about mutability. Your code can be much shorter. Also, your use of _.clone is wasted since it's only a shallow copy. I've rewritten parse code and now it looks like this:

// response is array of object
// [{id:1, prop: null },{id:2, prop: "bar" }]

// the output parsed_response can be
//[{id:1, prop: null, isProp: true },
// {id:2, prop: "bar", isProp: false }]

parse: function(response)
{
    return _.map(response, function(obj) {
        obj = _.clone(obj);
        obj.isProp = obj.prop !== "" && obj.prop !== null;
        return obj;
    });
}

If you want further explanation let me know :)

The only thing I'm concerned with is how your prop works? Can you just check for a falsey value here (i.e. !obj.prop) or do you really need to explicitly check for an empty string or null?

In term of re-usability, you're covered here. parse() doesn't reference this so it can more or less be called statically from anywhere in your code. But be careful where you use it from, if you start calling this all over the place, it would make more sense to refactor this parse() function to a more generic object.


Your code has two nested loops for each array row, which means that you are having to scan every row multiple times to get your result. You can almost get what you want, but with a linear algorithm, doing the following:

d = {}
for idx, val in enumerate(c):
    d.setdefault(val, []).append(idx)

>>> d
{1: [0, 3, 4], 2: [1], 3: [2]}

This is missing the empty lists for items not present in the original lists. You can create those by doing:

for j in range(max(d.keys())): # Don't need max+1, the max is already there
    d.setdefault(j, [])

>>> d
{0: [], 1: [0, 3, 4], 2: [1], 3: [2]}

If your list has n items, and its max was m, your original algorithm was doing O(n*m) operations, while this will keep that to O(n+m).

Putting the whole thing together for 2D arrays I would put all of the above in a function and then go with a dict comprehension that called it, so something like:

def list2dict(a):
    ret = {}
    for idx, val in enumerate(a):
        ret.setdefault(val, []).append(idx)
    for val in range(max(ret.keys())):
        ret.setdefault(val, [])
    return ret

result = {idx : list2dict(val) for idx, val in enumerate(a)}


Squaring numbers in an array I want to know if there is a better way to square numbers in an array. This is what I wrote:

#include <iostream>
using std::cout;
using std::endl;

void square(int []);

int main()
{
    int array[] = {2,4,6,8,10};

    square(array);
    for(auto i : array)
        cout << i << " ";

}

void square(int array[])
{
    int size = sizeof(array)/sizeof(array[0]);
    for(int i=0; i< size; i++)
        array[i] *= array[i];
}

int size = sizeof(array)/sizeof(array[0]);

Just so you know, sizeof() returns size_t.
You can use a vector instead of an array.

vector<int> data = {2,4,6,8,10};

Simplify squaring using std::transform() and use a lambda expression
    in place of a function object:

#include <iostream>
#include <algorithm>
#include <functional>

using std::cout;
using std::transform;
using std::vector;

int main()
{

    vector<int> data = {2,4,6,8,10};

    transform(data.begin(), data.end(), data.begin(), [](int x){return x*x;});

    for(auto i : data)
        cout << i << " ";

}


CSV to HTML Converter I have written a little program that converts a CSV-File to an HTML-Table. It works for my purposes. But are there parts in my code that can be written more clean? Can you improve maybe the performance? Are there maybe any bugs? I searched for bugs and fortunately I did not find some.
Postscript
Maybe I should have provided some background information: I am working on a database documentation that I am writing as an HTML document, because I dont like Word-documents. However, creating a tabular description of the columns with dozens of tags is painful. That is why I wrote this script: Now I only have to export the table information as CSV and can convert it directly without having to enter many tags myself. This is why there are no HTML and body tags: The tables created should not be separate HTML documents, but parts of a single, large HTML document.
CsvToHtmlTable.java
import java.io.BufferedReader;
import java.io.FileReader;
import java.io.IOException;
import java.io.FileWriter;
import java.util.List;
import java.util.ArrayList;

public class CsvToHtmlTable {
    public static void main(String[] args) {
        // print info and show user how to call the program if needed
        System.out.println("This program is tested only for UTF-8 files.");
        if (args[0].equalsIgnoreCase("help") || args[0].equalsIgnoreCase("-help") || args.length != 2) {
            System.out.println("java CsvToHtmlTable <input file> <output file>");
            System.out.println("Example: java CsvToHtmlTable nice.csv nice.html");
            System.exit(0);
        }

        String csvFile = args[0];
        String outputFile = args[1];

        // read lines of csv to a string array list
        List<String> lines = new ArrayList<String>();
        try (BufferedReader reader = new BufferedReader(new FileReader(csvFile))) {
            String currentLine;
            while ((currentLine = reader.readLine()) != null) {
                lines.add(currentLine);
            }
        } catch (IOException e) {
            e.printStackTrace();
        }

        //embrace  and
 for lines and columns
        for (int i = 0; i < lines.size(); i++) {
            lines.set(i, "
" + lines.get(i) + "
");
            lines.set(i, lines.get(i).replaceAll(",", "	"));
        }

        // embrace
 and

        lines.set(0, "
" + lines.get(0));
        lines.set(lines.size() - 1, lines.get(lines.size() - 1) + "
");

        // output result
        try (FileWriter writer = new FileWriter(outputFile)) {
            for (String line : lines) {
                writer.write(line + "\n");
            }
        } catch (IOException e) {
            e.printStackTrace();
        }
    }
}

How to call the program:
java CsvToHtmlTable ExampleInput.csv ExampleOutput.html

ExampleInput.csv
Name,Vorname,Alter
Ulbrecht,Klaus Dieter,12
Meier,Bertha,102

ExampleOutput.html

Name	Vorname	Alter

Ulbrecht	Klaus Dieter	12

Meier	Bertha	102

CSVReader
Reading a CSV file can be a complex task.  While many CSV files are just comma-separated-values, if a value contains a comma, it would be surrounded by double quotes, and if the value contains double quotes, the double quotes themselves are doubled.
To handle these more that just a basic CSV files, you really should use a CSV library, such as OpenCSV (com.opencsv:opencsv:5.0) or Apache Commons CSV (org.apache.commons:commons-csv:1.7).
HTML
Valid HTML
Your code essentially just writes
...table data...
.  This isn't proper HTML.  You're missing ... tags around the entire document, and ... around the content.  You should probably also have a ..., perhaps with a nice <title>...</title>.
Escaping
If your CSV data contains any special characters, like <, >, and &, you really must escape them in the generated HTML table.
Table Headings
It looks like the first line of your table contains headings, not data.  The first table row should probably be formatted with ...	 tags instead of ...	 tags.
Line by Line processing
You are reading the entire CSV file into memory, and only when it has been loaded in its entirety are you writing it back out as HTML.  This is very memory intensive, especially if the CSV file is huge!
Instead, you could:

open the CSV
open the HTML file
write the HTML prolog
for each line read from CSV file:

format and write line to HTML file

write HTML epilog

Untested, coding from the hip, without handling quoting in CSV or escaping any HTML entities in output:
        try (BufferedReader reader = new BufferedReader(new FileReader(csvFile));
             FileWriter writer = new FileWriter(outputFile)) {

            writer.write("
\n");

            String currentLine;
            while ((currentLine = reader.readLine()) != null) {
                writer.write("
");

                for(String field: currentLine.split(","))
                    writer.write("" + field + "	");

                writer.write("
\n");
            }

            writer.write("
\n");

        } catch (IOException e) {
            e.printStackTrace();
        }

XML & XSLT
You may want to consider creating a CSV to XML translator.
Your XML output might look like:
<data input-file='ExampleInput.csv'>
  <person>
    <Name>Ulbrecht</Name>
    <Vorname>Klaus Dieter</Vorname>
    <Alter>12</Alter>
  </person>
  <person>
    <Name>Meier</Name>
    <Vorname>Bertha</Vorname>
    <Alter>102</Alter>
  </person>
</data>

And then you could use an XSLT Stylesheet to translate the XML to HTML, possibly in a browser without ever writing the HTML to a file.


Select case string comparison I have a select case that checks against a string variable.  It's starting to get large and I am wondering if there is a better way of doing this?

Here is a sample of the Select Case statement:

Select Case Param.ToLower
    Case "dashboard_home_vm"
        Return _Locator.DashboardHome_VM
    Case "job_list_vm"
        Return _Locator.JobList_VM
    Case "job_add_vm"
        Return _Locator.JobAdd_VM
    Case "client_add_vm"
        Return _Locator.ClientAdd_VM
    Case "client_list_vm"
        Return _Locator.ClientList_VM
    Case "client_report_vm"
        Return _Locator.ClientReport_VM
    Case "employee_add_vm"
        Return _Locator.EmployeeAdd_VM
    Case "employee_certification_vm"
        Return _Locator.EmployeeCertification_VM
    Case "employee_list_vm"
        Return _Locator.EmployeeList_VM
    Case "employee_report_vm"
        Return _Locator.EmployeeReport_VM
    Case "employee_role_vm"
        Return _Locator.EmployeeRole_VM
    Case "employee_timeslip_vm"
        Return _Locator.EmployeeTimeslip_VM
    Case "product_add_vm"
        Return _Locator.ProductAdd_VM
    Case "product_category_vm"
        Return _Locator.ProductCategory_VM
    Case "product_list_vm"
        Return _Locator.ProductList_VM
    Case "product_type_vm"
        Return _Locator.ProductType_VM
    Case "product_unit_vm"
        Return _Locator.ProductUnit_VM
    Case "tracking_stock_vm"
        Return _Locator.TrackingInStock_VM
    Case "tracking_list_vm"
        Return _Locator.TrackingList_VM
    Case "tracking_site_vm"
        Return _Locator.TrackingOnSite_VM
    Case "vendor_add_vm"
        Return _Locator.VendorAdd_VM
    Case "vendor_list_vm"
        Return _Locator.VendorList_VM
    Case "vendor_report_vm"
        Return _Locator.VendorReport_VM
    Case Else
        Return Nothing
End Select

This actual list will be about 50 - 80 comparisons.  Is this the fastest way to do this or is there a better way?

A classic solution to this problem is to use a data table - in your case probably in form of a dictionary. It depends of what type your values are - I assume they are some sort of view models? I'm also going to assume they have a common base class:

One time setup:

Dim dict As New Dictionary(Of String, ViewModelBase)
dict.Add("dashboard_home_vm", _Locator.DashboardHome_VM)
...

You can look them up simply by

Dim vm As ViewModelBase
If (dict.TryGetValue(Param.ToLower, vm)) Then
    return vm
End If
return Nothing

Adding a new view model would be achieved by simply adding a new entry to the dictionary.


Simple worker class program I Have written this program to add workers into a list and print them. I'd like a review of this. I'm especially concerned if the method Addworker should be inside the class or in the main program.

Person:

class Person
{
    public uint NumberId;
    public string Name;
    public string LastName;
    public uint Age; // Age Should Be Beetween 18 And 40

    public uint numberId
    {
        set { this.NumberId = value; }
        get { return this.NumberId; }
    }

    public string name
    {
        set { this.Name = value; }
        get { return this.Name; }
    }

    public string lastName
    {
        set { this.LastName = value; }
        get { return this.LastName; }
    }

    public uint age
    {
        set
        {
            if (value < 18 || value > 40)
            {
                throw new ArgumentOutOfRangeException();
            }
            this.Age = value;
        }

        get { return this.Age; }
    }
}

Worker:

class Worker:Person
{
    private uint WageHour;
    private uint Total_Hours;

    public uint wageHour
    {
        set { this.WageHour = value; }
        get { return this.WageHour; }
    }

    public uint total_Hours
    {
        set { this.Total_Hours = value; }
        get { return this.Total_Hours; }
    }

    public double SalaryCalcul()
    {
        return this.wageHour * this.total_Hours;
    }

    public override string ToString()
    {
        return string.Format("Name : {0} Last Name : {1} Age : {2} WageHour : {3} Total Hours : {4} Salary : {5}",
                          this.name,this.lastName,this.age,this.wageHour,this.total_Hours,this.SalaryCalcul().ToString());

     }
}

Main program:

class Program
{
    static List<Worker> WorkersList = new List<Worker>();

    static void Main(string[] args)
    {
        do
        {
            Console.Clear();

            Console.WriteLine("1 - Add A Worker .");
            Console.WriteLine("2 - Print List .");

            int Choice = int.Parse(Console.ReadLine());

            switch (Choice)
            {
                case 1:
                    {
                        AddWorker();
                        break;
                    }

                case 2:
                    {
                        Console.Clear();

                        foreach (Worker W in WorkersList)
                        {
                            Console.WriteLine(W.ToString());
                        }

                        Console.ReadKey();

                        break;
                    }
            }
        }
        while (true);

    }

    static void AddWorker()
    {
        Worker W = new Worker();

        Console.WriteLine("Enter ID : ");
        W.numberId = uint.Parse(Console.ReadLine());

        Console.WriteLine("Enter Name : ");
        W.name = Console.ReadLine();

        Console.WriteLine("Enter Last Name : ");
        W.lastName = Console.ReadLine();

        try
        {
            Console.WriteLine("Enter Age : ");
            W.age = uint.Parse(Console.ReadLine());
        }
        catch (Exception)
        {
            Console.WriteLine("Age Should Be Between 18 and 40 ");
        }

        Console.WriteLine("Enter Hour Wage : ");
        W.wageHour = uint.Parse(Console.ReadLine());

        Console.WriteLine("Enter Total Hours : ");
        W.total_Hours = uint.Parse(Console.ReadLine());

        WorkersList.Add(W);
    }
}

Properties

Currently, you're defining a public field, and then creating a property which can get and set that public field, like this:

public int SomeNumber;
public int SomeNumberProperty
{
     get { return this.SomeNumber; }
     set { this.SomeNumber = value; }
}

Firstly, the field that the property sets and gets should be private, if you choose to not use auto-properties, like this:

private int _SomeNumber;
public int SomeNumberProperty
{
    get { return this._SomeNumber; }
    set { this._SomeNumber = value; }
}

Now, while this is something that you can do, in the case of your code, there is no need to manually define the getters and setters in your properties. You can simply define an auto-property, like this:

public int SomeNumber { get; set; }

The C# compiler will automatically generate a private field in which the value for the property is stored. Do note, this value can only be accessed through the property's getters and setters.

If you're using C# 6 or later, you can also define a default value for the property, like this:

public int SomeNumber { get; set; } = 42;

Constructors

All of your classes that represent something, like a Person or a Worker are missing something rather important - a constructor. A constructor allows you to initialize the values of specific properties or fields when the object is instantiated. For example, here's a representation of a Person class using auto-properties and a constructor:

public class Person
{
    public int Age { get; set; }
    public string Name { get; set; }

    public Person(int age, string name)
    {
        this.Age = age;
        this.Name = name;
    }
}

The constructor in question is this portion of code right here:

public Person(int age, string name)
{
    this.Age = age;
    this.Name = name;
}

It should be relatively self-explanatory what this chunk of code is doing. It's called when an instance of Person is created and sets the value of each property to the corresponding argument passed. An example of creating a new instance of this class might look like this:

Person person1 = new Person(42, "Bill");

By using the constructor to initialize values, it means that you don't have to initialize properties/fields like this:

Person person1 = new Person();
person1.Age = 42;
person1.Name = "Bill";

Side note: You can also use the object initializer syntax instead of constructors, but I'd recommend sticking with constructors for now.

Other nitpicks

The following method is the perfect candidate to be converted into a property:

public double SalaryCalcul()
{
    return this.wageHour * this.Total_Hours;
}

All this method does is return a value, so we can define a property like this:

public double Salary
{
    get { return this.wagehour * this.Total_Hours; }
}

If you are using a C# 6 or later, you can simply write the above like this:

public double Salary => this.wagehour * this.Total_Hours;

Simply attempting to convert user input to an integer isn't the best idea. What if the user enters a string, like "Hello"? To ensure that no errors are caused due to faulty user data, you should set up something like this:

int convertedUserInput;
string userInput = Console.ReadLine();

if(Int32.TryParse(userInput, out convertedUserInput))
{
    // the user entered a proper integer
}
else
{
    // the user did not enter a proper integer
}

As a side note, properties should also be named in PascalCase, along with fields.


My game of life implementation I'm looking for comments on both the micro (individual lines of code) and macro (overall design) scale. My input/output format is a single string, with newlines delimiting rows and '*'/' ' denoting alive and dead, respectively.

from itertools import product
from enum import Enum
class Direction(Enum): forwards = 1; backwards = 0

class State:
    table = {'*':True, ' ':False}

    def convert(self, grid, direction):
        if direction == Direction.forwards:
            table = self.table
        elif direction == Direction.backwards:
            table = {value:key for (key,value) in self.table.items()}
        else:
            raise ValueError("Invalid conversion direction!")

        return [[table[x] for x in row] for row in grid]

    def __init__(self, grid):
        grid = list(filter(None, grid.split('\n')))
        self.rows = len(grid); self.cols = len(grid[0])

        if not all(len(grid[i]) == self.cols for i in range(self.rows)):
            raise ValueError("Invalid game state!")

        self.grid = self.convert(grid, Direction.forwards)

    def __getitem__(self, i):
        return self.grid[i]

    def __str__(self):
        return '\n'.join(map(lambda x: ''.join(x), self.convert(self.grid, Direction.backwards)))

def game(state):
    ''' Takes a state in the input format and yields states in the output format. '''

    state = State(state)

    def alive(x,y):
        try:
            return state[x][y]
        except IndexError:
            return False

    while True:
        state.grid = [
            [2 <= sum(alive(xx,yy) for xx, yy in product([x-1,x+1], [y-1,y+1])) <= 3
             for (y,col) in enumerate(row)]
            for (x,row) in enumerate(state.grid)
        ]
        yield str(state)

Regarding code style, it's better to respect the PEP8 and use Flake8 to check your code.

For instance, replace:

from itertools import product
from enum import Enum
class Direction(Enum): forwards = 1; backwards = 0

by

from itertools import product
from enum import Enum

class Direction(Enum):
    forwards = 1
    backwards = 0

Regarding implementation:

When you raise an exception, always give the reason:

raise ValueError("Invalid conversion direction {0!r}! Only backwards or forwards.".format(direction))

In fact, convert() function could be “protected" because it is not used outside the class. And there is no need to check that the direction value is something else than backwards and forwards.

By convention, you could rename the alive function to is_alive because it is a predicate (it returns a Boolean). This function should be a method of State class because it use state variable.

For the same reason, the game function should also be a method. I don't like its name because we cannot understand that it's a generator. Something like next_state could be better.

At the end you calculate the sum of alive() values which are booleans. I consider this to be a bad practice: it's better to use 0 and 1.


Shifting bits in assembly (nasm) I wrote a function which takes a byte and shifts the bits around. That is, there shall be the same number of 1 bits and 0 bits in the output but the bits will be shifted in position:

global _main
default rel

section .text

_main:
    ; Given 1 byte, shift the positions of the bits in the form 74652103
    mov bl, 0x5A
    ; 0101 1010

    shift_bits:
    ; 1. bit 7 stays same
    ; 2. bit 4 (0x10) gets shifted left by 2 - and with 0001 0000, shift left by 2, or with result
    ; 3. bit 6 (0x40) gets shifted right by 1
    ; 4. bit 5 (0x20) gets shifted right by 1
    ; 5. bit 2 (0x4) gets shifted left by 1
    ; 6. bit 1 (0x2) gets shifted left by 1
    ; 7. bit 0 (0x1) gets shifted left by 1
    ; 8. bit 3 (0x8) gets shifted right by 3
    ; begin 0101 1010
    ; expect: 0110 0101
    ; end 0110 0101
        mov al, bl
        mov cl, 0
        and al, 0x80
        or cl, al
        mov al, bl
        and al, 0x10
        shl al, 2
        or cl, al
        mov al, bl
        and al, 0x4
        shl al, 1
        or cl, al
        mov al, bl
        and al, 0x2
        shl al, 1
        or cl, al
        mov al, bl
        and al, 0x1
        shl al, 1
        or cl, al
        mov al, bl
        and al, 0x8
        shr al, 3
        or cl, al
        mov al, bl
        and al, 0x2
        shl al, 1
        mov al, bl
        and al, 0x40
        shr al, 1
        or cl, al
        mov byte [A], cl ; Store result in A

    go_out:
    mov     rax, 0x2000001 ; exit - macOS specific!
    mov     rdi, 0
    syscall

section .data
A: db 0

Is there a more efficient way to do this with fewer instructions or more performant ones?

The operation count can be reduced, by using that some bits are moved by the same distance. Bits 0, 1 and 2 can be shifted left in one go. 5 and 6 can be shifted right in one go.

Some shift/bitwise-OR combinations can be written as lea, that would be bad for Pentium 4 but very good on Ryzen and Ice Lake and fine on Haswell/Skylake. Since this is 64bit code it is best to use lea with a 64bit address and then implicitly discard the top half of the result by writing it to a 32bit destination, explicitly using a 32bit address would generate a useless address size override prefix.

The 8 bit operations have a disadvantage on several CPUs that limits their efficiency, they're usually not inherently slow per-se but the problem is that writing to the "low" 8 bit register may have a dependency on the whole register (the "high" registers are an other story and have their own issues), something like mov al, bl doesn't "decouple" the new al from the old al on such CPUs because the write is merged into rax. So unless there is a good reason for writing to an 8 bit register, I would recommend avoiding it, to avoid such odd edge cases of the microarchitecture.

Here is a possible implementation:

    ; keep bit 7 at bit 7
    mov     eax, ebx
    and     eax, 128
    ; shift bits 0, 1, 2 left by 1 and combine
    mov     ecx, ebx
    and     ecx, 7
    lea     eax, [rax + 2*rcx]
    ; shift bit 4 left by 2 and combine
    mov     ecx, ebx
    and     ecx, 0x10
    lea     eax, [rax + 4*rcx]
    ; shift bit 3 right by 3 and combine
    mov     ecx, ebx
    and     ecx, 8
    shr     ecx, 3
    or      eax, ecx
    ; shift bits 5 and 6 right by 1 and combine
    shr     ebx
    and     ebx, 0x30
    or      eax, ebx
    ; store result
    mov [A], al


Combination aggregate/greatest-n-per-group SQL query on simplified data set I've been reading the top 'greatest-n-per-group' answers on StackOverflow but haven't yet found any answers that fit my specific case.

The following code works, but it seems overly-complicated for what I want to achieve. The nested/self-joined derived tables seem wrong but I don't know how to simplify things without resorting to a correlated sub-query.

Goal: For each article, I want to select the url (as determined by the page with the lowest page_number), page count and word count.

SQL Fiddle

Schema:

-- tables
CREATE TABLE article (
  id int PRIMARY KEY
);
CREATE TABLE page (
  id int PRIMARY KEY,
  url text,
  page_number int,
  word_count int,
  article_id int REFERENCES article
);

-- data
INSERT INTO article (id) VALUES (1), (2), (3);
INSERT INTO page (id, url, page_number, word_count, article_id) VALUES
  (1, '/1/1', 1, 250, 1), (2, '/1/2', 2, 230, 1), (3, '/1/3', 3, 55, 1),
  (4, '/2/1', 1, 300, 2),
  (5, '/3/1', 1, 1000, 3), (6, '/3/2', 2, 1000, 3);

Query:

SELECT
  first_page.url,
  count(*) AS page_count,
  sum(page.word_count) AS word_count,
  page.article_id
FROM
  page
  JOIN (
    SELECT
      page.url,
      page.article_id
    FROM
      page
      JOIN (
        SELECT
          min(page_number) AS page_number,
          article_id
        FROM page
        GROUP BY article_id
      ) AS first_page ON
        first_page.article_id = page.article_id AND
        first_page.page_number = page.page_number
  ) AS first_page ON first_page.article_id = page.article_id
GROUP BY
  page.article_id,
  first_page.url;

A common solution is based on Window Functions, as all three are based on the same window they're calculated in a single step:

SELECT url, page_count, word_count, article_id
FROM
 (
   SELECT
      url,
      article_id,
      -- both aggregations rewritten as Group Aggregates
      count(*)         over (partition by article_id) as page_count,
      sum(word_count)  over (partition by article_id) AS word_count,
      -- find the minimum page number
      page_number =
      min(page_number) over (partition by article_id) as is_min_page
   FROM page
 ) as dt
WHERE is_min_page; -- return minimum page only

See fiddle


Self-made linked list implementing List I have to make a linked list as a quasi-homework task, about which I have already asked two questions about here and here. Since the whole code is really big (400 lines approx.), I will include only the few functions I am asking you to review, although, if you need any more code, I will upload it.

Here is the .add(int, T) function:

public void add(int index, T data){
    if (getNode(index) == null){
        throw new NoSuchElementException();
    } else if (index<0 || index>=size){
        throw new NullPointerException();
    }

    Node<T> crntNode;
    Node<T> newNode = new Node<T>(data);
    crntNode = getNode(index);
    newNode.index = index;

    if (crntNode.previousNode != null) {
        crntNode.previousNode.nextNode = newNode;
    } else {
        firstNode = newNode;
    }
    crntNode.previousNode = newNode;
    newNode.nextNode = crntNode;
    crntNode = newNode;

    while (crntNode.nextNode != null){
        crntNode.index++;
        crntNode = crntNode.nextNode;
    }

    size++;
}

The .get(int) function:

public T get(int index){
    if(isEmpty() || index < 0 || index >= size){
        return null;
    }

    Node<T> crntNode;
    int count = size - index - 1;
    crntNode = lastNode;

    while (count > 0) {
        crntNode = crntNode.previousNode;
        count--;
    }

    return crntNode.data;
}

And the .remove(int) function:

public T remove(int index) {
    if (getNode(index) == null || isEmpty()) {
        return null;
    } else if (size == 1){
        size = 0;
        Node<T> crntNode = firstNode;
        firstNode = null;
        lastNode = null;
        return crntNode.data;
    }

    Node<T> crntNode = getNode(index);

    if (crntNode.nextNode != null){
        if (crntNode.previousNode != null){
            crntNode.previousNode.nextNode = crntNode.nextNode;
        } else {
            firstNode = crntNode.nextNode;
            firstNode.previousNode = null;
            size--;
            return crntNode.data;
        }
    }
    if (crntNode.previousNode != null){
        if (crntNode.nextNode != null) {
            crntNode.nextNode.previousNode = crntNode.previousNode;
        } else {
            lastNode = crntNode.previousNode;
            lastNode.nextNode = null;
            size--;
            return crntNode.data;
        }
    }
    crntNode = crntNode.nextNode;
    size--;

    for(int i = index; i < size-1; i++){
        crntNode = crntNode.nextNode;
        crntNode.index--;
    }

    return crntNode.data;
}

I thought a lot about improving these functions, but I can't seem to find a better concept. The reason might be my lack of knowledge, in which case, I hope you could help me.

Is this code efficient and error-safe enough to be used in real programming situations?

add paramater check

else if (index<0 || index>=size){
    throw new NullPointerException();
}

This isn't necessary, because get already catches these cases. You do the same in remove, where you check for isEmpty, which you already checked via the call to get.

You should also try to be consistent with your exceptions. get throws a NoSuchElementException in this case, while add throws  NullPointerException.

Oracles implementation throws an IndexOutOfBoundsException which makes a lot of sense (and if you want to stay close to the List interface, you should use it as Tavian Barnes pointed out), but you could make a case for a NullPointerException or IllegalArgumentException if you wanted to. NoSuchElementException on the other hand would not match the description (Thrown by the nextElement method of an Enumeration).

Don't return null

The only occasion you should return null is if the data inside the node is null. If a value is requested that doesn't exist, throw an exception. You violate this in get and remove, which will make it really difficult for a user in case they do store null in the list.

Naming

crntNode: just add the extra characters, its not that much longer: currentNode.

Assign value when declaring variables

Instead of

Node<T> crntNode;
Node<T> newNode = new Node<T>(data);
crntNode = getNode(index);

do this:

Node<T> crntNode = getNode(index);
Node<T> newNode = new Node<T>(data);

You do the same thing in get.

Performance

To increase performance, save the previous result of expensive method calls in local variables. For example, getNode is expensive, and you call it twice in add as well as remove.

index

You should get rid of index (not the parameter, but the node field).

In a linked list, nodes don't have an index, they only have a next node (and maybe previous if its doubly linked). Maintaining the index adds a lot of complexity and reduces performance, which can be avoided.

so just remove index, you are not using it anywhere anyways.