string to integer (implement atoi) Implement atoi to convert a string to an integer.

  Requirements for atoi: The function first discards as many whitespace
  characters as necessary until the first non-whitespace character is
  found. Then, starting from this character, takes an optional initial
  plus or minus sign followed by as many numerical digits as possible,
  and interprets them as a numerical value.

  The string can contain additional characters after those that form the
  integral number, which are ignored and have no effect on the behavior
  of this function.

  If the first sequence of non-whitespace characters in str is not a
  valid integral number, or if no such sequence exists because either
  str is empty or it contains only whitespace characters, no conversion
  is performed.

  If no valid conversion could be performed, a zero value is returned.
  If the correct value is out of the range of representable values,
  INT_MAX (2147483647) or INT_MIN (-2147483648) is returned.

The following is my code:

int atoi(const char *str) {
    int sign = 1;
    long long i = 0, j = 0;

    while ((*str) != '\0' && isspace(*str)) {
        ++str;
    }

    if (((*str)!='\0') && ((*str) == '+' || (*str) == '-')) {
        if ((*str) == '-') {
            sign = -1;
        }
        ++str;
    }

    if (((*str) != '\0') && (!isdigit(*str))) {
        return 0;
    }

    while (((*str) != '\0') && (isdigit(*str))) {
        i = i * 10 + (*str - '0');
        j = i * sign;
        cout << j << endl;
        if (j > INT_MAX) {
            return INT_MAX;
        }
        if (j < INT_MIN) {
            return INT_MIN;
        }

        ++str;
    }

    return j;
}

You seem to have too many checks for '\0'. They don't add anything. Also too many brackets, but better too many than too few.

Also, is long long guaranteed to be bigger than int? I don't think it is. Here's another version that doesn't need a bigger representation than that of int:

int atoi(const char *str)
{
    int n = 0;
    int sign = 1;

    while (isspace(*str)) {
        ++str;
    }
    if (*str == '-') {
        sign = -1;
        ++str;
    } else if (*str == '+') {
        sign = 1;
        ++str;
    }
    while (isdigit(*str)) {
        if (n > INT_MAX/10) { /* EDIT: protect against overflow */
            break;
        }
        n *= 10;
        int ch = *str - '0';

        if (n > INT_MAX - ch) {
            break;
        }
        n += ch;
        ++str;
    }
    if (isdigit(*str)) {
        return sign == 1 ? INT_MAX : INT_MIN;
    }
    return sign * n;
}

EDIT: I added a check to prevent overflow of the n *= 10

EDIT2: improved to avoid using an unsigned int - it was a hack


HttpRequest class using AsyncTask for Android application I am using the following class to retrieve a http response, which contains data for my Android App. Everything works fine, but there is a performance issue. I get the following warning when requesting this class:

  Skipped 36 frames!  The application may be doing too much work on its main thread.

I used an AsyncTask to run the network connection on a different thread, but it is still saying I'm doing too much work on the main thread. Is there something you can point out to me that is slowing down my program?

package com.lakshan.gamematch.util;

import java.io.BufferedReader;
import java.io.InputStream;
import java.io.InputStreamReader;

import org.apache.http.HttpEntity;
import org.apache.http.HttpResponse;
import org.apache.http.client.HttpClient;
import org.apache.http.client.methods.HttpPost;
import org.apache.http.impl.client.DefaultHttpClient;

import android.os.AsyncTask;
import android.util.Log;

public class HttpRequest {

String url_to_task = "";
String string_result = "hasn't returned a value";

        public String getFromURL (String url) {
            url_to_task = url;
            new MyTask().execute();
            while (string_result.equals("hasn't returned a value")) {
                try {
                    Thread.sleep(10);
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
            return string_result;
        }

        private class MyTask extends AsyncTask <Void, Void, Void> {
            @Override
            protected Void doInBackground(Void... params) {
                InputStream is = null;

                //Download JSON data from url
                try {
                    HttpClient httpclient = new DefaultHttpClient();
                    HttpPost httppost = new HttpPost(url_to_task);
                    HttpResponse response = httpclient.execute(httppost);
                    HttpEntity entity = response.getEntity();
                    is = entity.getContent();
                    Log.e("entity", entity.getContent().toString());

                } catch (Exception e) {
                    Log.e("log_tag", "Error in http connection " + e.toString());
                }

                //Convert response to string
                try {
                    BufferedReader reader = new BufferedReader (new InputStreamReader(is, "iso-8859-1"), 8);
                    StringBuilder sb = new StringBuilder();
                    String line = null;
                    while ((line = reader.readLine()) != null) {
                        sb.append(line + "\n");

                    }
                    is.close();
                    string_result = sb.toString();
                } catch (Exception e) {
                    Log.e("log_tag", "Error converting result" + e.toString());
                }
                return null;
            }

            @Override
            protected void onPostExecute(Void result) {
                super.onPostExecute(result);
            }
        }
}

By default AsyncTask tasks execute sirially on the default executor and you should at least override the execute to make sure that all requests are run in parallel unless specified otherwise
Apache HTTP library is considered deprecated in Android and you should try to at least use HttpUrlConnection if not a library that can make this happen with zero boilerplate code sych as Volley, OkHttp or Retrofit
Using Thread.sleep on the main thread is the best way you have to cause these warnings.
The system has about 16.8 milliseconds to draw anything on the screen and you just took away 10 of them for nothing while blocking the system from using the thread.
please use the given onPreExecute and onPostExecute to show progress activity to the user in any way you see fit and never EVER sleep on the main thread


Simplifying member functions in Blackjack game For my display functions in my Blackjack game, I want to control each player's stats output based on whether a turn is in progress or has ended.  Normally, you could only see one card that your opponent(s) hold, while the rest are only revealed at the end of each turn.  My code does that well, but I'm having trouble making it more concise.
The display function in Game is called to display each player's stats (name, chips, and hand).  It takes a Boolean argument to determine if it should display each CPU's partial stats (during each turn) or full stats (after each turn).  At the function call in Game, the bool argument is passed to the respective display function in Player, where the human player and CPU player have individual display procedures.  The display functions in Player primarily display the chip amounts and card rank totals.  They also call respective Hand display functions which, again, solely depend on the Boolean argument for displaying the CPUs' stats.
How could I simplify this code to achieve the same output (shown below the code)?
Game.cpp
displayStats(false); // function call elsewhere in Game

void Game::displayStats(bool showCPUResults) const
{
    players[0].displayPlayerStats(); // displays player stats

    for (int i = 1; i < numPlrs; i++)
        players[i].displayCPUStats(showCPUResults); // displays CPU stats
}

Player.cpp
void Player::displayPlayerStats() const
{
    // always shown
    std::stringstream ss;
    std::cout << "\n* " << name << ": ";
    std::cout << "($" << chips << ") ";

    // displays two-digit card rank total (always shown)
    if (totalCardsValue <= 9)
        ss << "(0" << totalCardsValue << ")  ";
    else
        ss << "(" << totalCardsValue << ")  ";

    std::cout << ss.str();

    // displays human player's hand
    playerHand[0].displayPlayerHand();
}

void Player::displayCPUStats(bool showResults) const
{
    // always shown
    std::stringstream ss;
    std::cout << "\n* " << name << ": ";
    std::cout << "($" << chips << ") ";

    if (!showResults)
    {
        std::cout << "      ";
        playerHand[0].displayCPUHand(showResults);
    }
    else
    {
            // only shown after each turn
        if (totalCardsValue <= 9)
            ss << "(0" << totalCardsValue << ")  ";
        else
            ss << "(" << totalCardsValue << ")  ";

        std::cout << ss.str();

            // displays CPU's hand
        playerHand[0].displayCPUHand(showResults);
    }
}

Hand.cpp
void Hand::displayPlayerHand() const
{
    // displays player's full hand (always)
    for (unsigned i = 0; i < cards.size(); i++)
        std::cout << cards[i] << " ";
}

void Hand::displayCPUHand(bool showCPUHand) const
{
    // displays CPU's first card (always)
    std::cout << cards[0] << " ";

    // displays the rest of the CPU's cards (only after each turn)
    if (showCPUHand)
    {
        for (unsigned i = 1; i < cards.size(); i++)
            std::cout << cards[i] << " ";
    }
}

Output during a turn...

Jamal: ($1000) (23)  [8C] [6H] [9C]
CPU1: ($1000) [AD]

...and after a turn...

Jamal: ($1000) (23)  [8C] [6H] [9C]
CPU1: ($1000) (21)  [AD] [KH]

If you want to make things more consice, you could change a few things:

Number of card printed on 2 digits:

if (totalCardsValue <= 9)
    ss << "(0" << totalCardsValue << ")  ";
else
    ss << "(" << totalCardsValue << ")  ";

could be written

ss << ((totalCardsValue <= 9) ? "(0" : "(") << totalCardsValue << ")  ";

or

ss << "(" << std::setw(2) << std::setfill('0') << totalCardsValue << ")  ";

No need for temporary std::stringstream.
I think you could just std::cout directly without using std::stringstream ss.
Removing duplicated code: void Hand::displayPlayerHand() does the same as void Hand::displayCPUHand(true). This would probably better if you were naming this method display and calling the parameter displayAllCards or displayOnlyFirst. Also, you might want to give it a default value.

Corresponding code for Hand.cpp is:

void Hand::display(bool displayAllCards) const
{
    // displays the first card
    std::cout << cards[0] << " ";

    // displays the rest of the cards if required
    if (displayAllCards)
    {
        for (unsigned i = 1; i < cards.size(); i++)
            std::cout << cards[i] << " ";
    }
}

Removing duplicated code: things can be changed in Player.cpp too as Player::displayCPUStats and  Player::displayPlayerStats really look alike.

Edited to finish my answer: Indeed, displayPlayerStats() is nothing but displayCPUStats(true). Here again, it might be worth changing the names of methods and variables.

Corresponding code for Player.cpp is:

void Player::displayStats(bool displayAllCards) const
{
    std::cout << "\n* " << name << ": ";
    std::cout << "($" << chips << ") ";

    if (displayAllCards)
    {
        std::cout << ((totalCardsValue <= 9) ? "(0" : "(") << totalCardsValue << ")  ";
    }
    playerHand[0].displayHand(displayAllCards);
}


Project Euler # 25 The 1000 digit Fibonacci index The Fibonacci sequence is defined by the recurrence relation:

  Fn = Fn−1 + Fn−2, where F1
  = 1 and F2 = 1.

  Hence the first 12 terms will be:

  F1 = 1
  F2 = 1
  F3 = 2
  F4 = 3
  F5 = 5
  F6 = 8
  F7 = 13
  F8 = 21
  F9 = 34
  F10 = 55
  F11 = 89
  F12 = 144

  The 12th term, F12, is the first term to contain three digits.

  What is the index of the first term in the Fibonacci sequence to
  contain 1000 digits?

from time import time

def fib(n):
    """returns index of the first fibonacci of length n digits."""
    fibs = [0, 1]
    while len(str(fibs[-1])) < n:
        fibs.append(fibs[-1] + fibs[-2])
    return len(fibs) - 1

if __name__ == '__main__':
    time1 = time()
    print(fib(1000))
    print(f'Time: {time() - time1} seconds.')

It's possible to calculate the nth Fibonacci number directly thanks to Binet's formula.

Due to floating point errors, the formula doesn't give correct values if n is too large, but it works fine in order to calculate the number of digits.

from math import log10, floor, ceil

def fibonacci_digits(n):
    if n < 2:
        return 1
    ϕ = (1 + 5**0.5) / 2
    return floor(n * log10(ϕ) - log10(5) / 2) + 1

It's possible to calculate the inverse of this function. This way, you can find the first Fibonacci with k digits directly, without any loop:

def euler25(k):
    if k < 2:
        return 1
    ϕ = (1 + 5**0.5) / 2
    return ceil((k + log10(5) / 2 - 1) / log10(ϕ))

It seems to return the correct answer for any k in less than 1 μs :

>>> euler25(1000)
4782
>>> euler25(10_000)
47847
>>> euler25(100_000)
478495
>>> euler25(100_000_000_000_000_000)
478497196678166592

The digits are the same as in:

>>> 1 / log10(ϕ)
4.784971966781666


Steam API JSON function Upon coming face to face with the mighty cross-origin request block, and needing to get a JSON file from an external source, I decided to use the Yahoo Query Language in order to achieve this.

I'm starting to wonder however whether this code is really the most efficient way of doing it, considering the source can be in JSON, XML and Steams own language.

function getID64(name) {
    var Site = 'https://api.steampowered.com/ISteamUser/ResolveVanityURL/v0001/?key=' + key + '&vanityurl=' + name;
    //console.debug("getID64 - " + Site);
    var yql = 'https://query.yahooapis.com/v1/public/yql?q=' + encodeURIComponent('select * from json where url="' + Site + '"') + '&format=json&callback=?';
    //console.debug("getID64 - " + yql);
    $.getJSON(yql, function(data) {
        app.id64 = data.query.results.response.steamid;
        //console.debug("getID64 - " + app.id64);
        //From here onwards it's just changing some elements on the webpage and starting the next function
        document.getElementById("Steam64ID2").innerHTML += " " + app.id64;
        document.getElementById("Steam64ID").value += "" + app.id64;
        document.getElementById("SteamID2").innerHTML += " " + document.getElementById("SteamID").value;
        getFriendList(document.getElementById('Steam64ID').value);
    });
}

Considering for the first half I am only trying to get and parse the file, is there an easier way of doing this?

Upon coming face to face with the mighty cross-origin request block, and needing to get a JSON file from an external source, I decided to use the Yahoo Query Language in order to achieve this.

Using YQL just to get a JSON and parse it sounds like a dirty workaround.
I don't know the right answer about that part.
You might be better off asking on stackoverflow.com instead,
to overcome your original problem without resorting to such workaround.

About the code posted, these are only minor nitpicks:

Where does key come from? Perhaps it should be a method parameter
User lowercase for variable names (site (or url) instead of Site)
You look up the 'Steam64ID' element twice. It would be better to do it once and store in a variable to reuse, for example:

var steam64ID = document.getElementById("Steam64ID").value += "" + app.id64;
getFriendList(steam64ID);

The function is called getID64 which sounds like a getter, so it should return something, but it actually doesn't.


PHP - First Login System I am quite new to PHP, and with the help of a few tutorials have made this login system.

However I know its not as efficient and compact as it could be, also I am looking for ways to improve the security of this system. As I noticed when testing that If I simply changed the url from the login page, I could bypass the whole login process without having to login. I'm not really sure how I would go about fixing this, I tried creating session variables at the bottom of the script but not really sure if this is the way to go about it.

I would be very grateful for any feedback and if someone could help me improve this code, to make it more secure and maybe more efficient?

<?php

session_start();

if (isset($_POST['submit'])) {

    require_once 'config.php';

    $username = mysqli_real_escape_string($connect, $_POST['username']);
    $password = mysqli_real_escape_string($connect, $_POST['password']);

    //Error handlers
    //Check if inputs are empty
    if (empty($username) || empty($password)) {
        header("Location: ../index.php?login=empty");
        exit();
    } else {
        $sql = "SELECT * FROM users WHERE username='$username'";
        $search = mysqli_query($connect, $sql);
        $validate = mysqli_num_rows($search);
        if ($validate < 1) {
            header("Location: ../index.php?login=error");
            exit();
        } else {
            if ($row = mysqli_fetch_assoc($search)) {
                //De-hashing the password
                $hashcheck = password_verify($password, $row['password']);
                if ($hashcheck == false) {
                    header("Location: ../index.php?login=error");
                    exit();
                } elseif ($hashcheck == true) {
                    //Log in the user here
                    $_SESSION['u_id'] = $row['userid'];
                    $_SESSION['u_first'] = $row['firstname'];
                    $_SESSION['u_last'] = $row['lastname'];
                    $_SESSION['u_email'] = $row['email'];
                    $_SESSION['u_uid'] = $row['username'];
                    header("Location: ../home.php");
                    exit();
                }
            }
        }
    }
} else {
    header("Location: ../index.php?login=error");
    exit();
}
?>

Security

You are escaping user input, which prevents SQL injection. It's good enough, but really not the recommended way to do this as it's too error-prone (whenever a project does this, it's basically guaranteed that you can find injections). Use prepared statements instead. It's not only more secure, but also leads to nicer code.

Guard Clauses

You have a lot of nested ifs, which makes it difficult to see what happens when (for example, I have to scroll all the way down to see what happens if it's a GET request).

If you use guard clauses / return early instead, your code will be much more readable. It might look like this:

if (!isset($_POST['submit'])) {
    header("Location: ../index.php?login=error");
    exit();
}

if (empty($username) || empty($password)) {
    header("Location: ../index.php?login=empty");
    exit();
}

// select the user

if (!$userExists) {
    header("Location: ../index.php?login=error");
    exit();
}

// verify the password

if ($validPassword) {
    header("Location: ../index.php?login=error");
    exit();
}

// set the session, redirect

Now it's very clear what happens when.

Duplication and functions

Following a  header redirect with exit is very good practice. You can make sure that you always do this, simplify the code, and make it easier to modify later by extracting that to it's own function (eg redirectError(Error error)).

True or false

Something is either true or false. There is no reason to do an else check on true if you already did one on false.

Note though that some equals checks can be unexpected, so you should always use strong comparison (===) if you don't have a reason not to.


Joining tables with EF Here is my database:

Here is my viewModel:

public class groupsFoldersViewModel
    {
        private DAL.educationSocialNetworkEntities DB = new DAL.educationSocialNetworkEntities();
     public  List<DAL.sharedFolders_table> sharedFolders { get; set; }

     public groupsFoldersViewModel(int gid , int uid)
     {
         DAL.group_table group = DB.group_table.Find(gid);

         if (group != null)
         {

             sharedFolders = group.sharedFolders_table.ToList();
         }

     }
    }

Which way of joining table is better and faster?

One heavy query for joining tables in ViewModel constructor
Using something like this in a razor page:

@foreach (var f in Model.sharedFolders)
{
    <a> @f.folder_table.folderName </a>
}

At the end of the day, only you can tell what performs best. But there are two things that in most cases will help improve performance.
1. Reduce the number of queries.
Essentially, you do
DAL.group_table group = DB.group_table.Find(gid);
sharedFolders = group.sharedFolders_table.ToList();

And in the view, for each sharedFolders_table you diplay
sharedFolders_table.folder_table.folderName

That's one query for each sharedFolders_table.
So you end up having 1 + 1 + n queries.
No doubt, it's better to turn these into one. For example:
sharedFolders = DB.sharedFolders_table
                  .Include(s => s.sharedFolders_table)
                  .Where(s => s.groupId == gid).ToList();

But since you only seem to use folder_table.folderName it should be better yet to get only folder_table objects:
sharedFolders = DB.sharedFolders_table
                  .Where(s => s.groupId == gid)
                  .Select(s => s.folder_table)
                  .ToList();

(Note that this changes your model data type.)
2. Use a projection
These queries return complete entities. But in the end, you only display one field (let's ignore the fact that in reality you probably do more). So you may as well get that field only.
Reducing the number of fields in a query can considerably improve performance, especially when records are wide and many joins are involved. I often see people focus on reducing the number of records, but reducing the number of fields is equally important.
In your reduced example, it's enough to get only the names from the database:
sharedFolders = DB.sharedFolders_table
                  .Where(s => s.groupId == gid)
                  .Select(s => s.folder_table.folderName)
                  .ToList();

(again, this changes the model type)
Now you have one lean and mean query, instead of 1 + 1 + n wide queries.


Program to implement Merge Sort Algorithm So, I just learned the Merge Sort algorithm from Youtube and tried implementing it. Is this the most efficient way to do it?

public class MergeSort {

private static void merge(int[] left, int[] right, int[] arr) {
int i=0, j=0, k=0;
while(i<left.length && j<right.length) {
if(left[i]<right[j]) {
    arr[k++]=left[i++];
    }
else {
    arr[k++]=right[j++];
    }
}
while(i<left.length)
    arr[k++] = left[i++];
while(j<right.length)
    arr[k++] = right[j++];
}

public static void mergeSort(int[] arr) {
if(arr.length<2)
    return;

int mid = arr.length/2;
int[] left = new int[mid];
int[] right;

//checking wether array is even or not, based on which right array length           is defined
int rightLength;
if((arr.length&1)==0) //if even
    rightLength = mid;
else
    rightLength = mid+1;
right = new int[rightLength];

//assign values to left and right arrays from the original array
for(int i=0;i<mid;i++)
    left[i] = arr[i];

for(int j=0;j<rightLength;j++)
    right[j] = arr[j+mid];

mergeSort(left);
mergeSort(right);
merge(left,right,arr);

}

}

Is this the most efficient way to do it?

No.  First thing, if efficiency is your goal, you don't use mergesort for small inputs.  An insertion sort is lower overhead for small inputs.  I don't know where the crossover point is, but it is almost certainly higher than one.  So

if(arr.length<2)
    return;

would become something like

if (arr.length < MINIMUM_MERGESORT_SIZE) {
    insertionSort(arr);
    return;
}

Allocating and copying arrays

int rightLength;
if((arr.length&1)==0) //if even
    rightLength = mid;
else
    rightLength = mid+1;
right = new int[rightLength];

This seems rather complicated.  Consider

int[] right = new int[arr.length - left.length];

Same result.  Less code.  I don't know that it changes the efficiency that much, as the operations are pretty cheap.  But it does ensure that you don't accidentally create an error by messing up the length calculation.

What has a better chance of improving efficiency is combining the allocation above with the copying.  So get rid of the declarations and

for(int i=0;i<mid;i++)
    left[i] = arr[i];

for(int j=0;j<rightLength;j++)
    right[j] = arr[j+mid];

Now we can just say

    int[] left = Arrays.copyOfRange(numbers, 0, numbers.length / 2);
    int[] right = Arrays.copyOfRange(numbers, left.length, numbers.length);

This will take advantage of other optimizations, like copying blocks of memory at once.  It likely also avoids clearing the new array, as we copy over those values anyway.

I changed the name of arr to numbers, which I find more descriptive.  Another complaint you'll hear about arr as a name is it makes everyone sound like a pirate when they say it.  But more professionally, abbreviating by a small number of characters doesn't help much.  You could have just said array.

I added more indentation.  Usually we indent every time we have a { and remove indent every time we }.

Alternatives

Of course, if we really wanted to sort efficiently, we'd just say

    Collections.sort(numbers);

We'd let it work out what method and how to split up the arrays.  But you may be trying this purely as an exercise.


Point in a polygon algorithm I am implementing a Point in polygon algorithm.

Inputs:

M, N: size of the matrix
poly: a list of tuples that represent the points of polygon.

Output:

A mask matrix which is ones everywhere, except the point in the polygon should be 0.

import numpy as np
import cv2

def getABC(x1, y1, x2, y2):
    A = y2 - y1
    B = x1 - x2
    C = A*x1 + B*y1
    return (A, B, C)

def polygon(M, N, poly):
    out = np.ones((M, N))*255

    n = len(poly)
    for i in range(M):
        intersection_x = i
        intersection_y = []

        # check through all edges
        for edge in range(n + 1):
            v1_x, v1_y = poly[edge % n]
            v2_x, v2_y = poly[(edge + 1) % n]

            A1, B1, C1 = getABC(v1_x, v1_y, v2_x, v2_y)
            A2 = 1
            B2 = 0
            C2 = i

            # find intersection
            det = A1*B2 - A2*B1
            if (det != 0):
                tmp = (A1 * C2 - A2 * C1)/det
                if tmp >= min(v1_y, v2_y) and tmp <= max(v1_y, v2_y):
                    intersection_y.append(tmp)

    intersection_y = list(set(intersection_y))
    print intersection_y

    if len(intersection_y) == 1:
        intersection_y.append(intersection_y[0])

    for k in range(1, len(intersection_y), 2):
        out[intersection_x, intersection_y[k - 1]:intersection_y[k]] = 0

    return out

poly = [(10,20), (10,40), (30,20), (30,40)]

out = polygon(100,100, poly)

cv2.imwrite("out.png", out)

Is this the optimal algorithm? I use a scanning line, find the intersection with edges and make all the point between the intersections to be zero.

When you say "points of polygon", I am assuming you are referring to vertices.

I don't think what you have is the fastest. A simple improvement to this could be to divide your matrix in to a grid of p x p cells, where p is a parameter, and classify each grid-cell as completely inside or completely outside of the polygon. This can be done by checking if all the vertices of the grid-cell are inside the polygon. This way, large regions inside grid cells can be directly labeled as inside or outside, without needing to test every point inside the cell against all the edges of the polygon. For the grid-cells that are not completely inside the polygon, you just go by what you are doing. The optimal choice of p depends on the size of polygons you may encounter.

More details about the grid method here (with comparisons to other strategies):
http://erich.realtimerendering.com/ptinpoly/

There could be better ones. I just suggested a simple improvement, which works well in practice.


Simplified if return suggestion from Android Studio I have this very simple method:

private boolean isTheParticipantTalking(ParticipantID participantId){
    if(mCurrentParticipant != null) {
        return participantId.equals(mCurrentParticipant.getId());
    }
    return false; // no one is talking
}

I receive a warning stating that I can simplify the code in the following manner:

private boolean isTheParticipantTalking(ParticipantID participantId){
    return mCurrentParticipant != null && participantId.equals(mCurrentParticipant.getId());
}

In your opinion, is this change desirable? I understand that we go from 4 lines of code to just 1, but I find my way more understandable in one blick.

I prefer your method, because it's a lot clearer. But I would turn the if around (first check the input, then the normal action):

private boolean isTheParticipantTalking(ParticipantID participantId){
    if(mCurrentParticipant == null) {
        return false; // no one is talking
    }
    return participantId.equals(mCurrentParticipant.getId());
}

Another question is if this is actually the best approach.

Is a non-existing participant talking? That question really doesn't make so much sense.

You could use Objects.requireNonNull which seems to have been written exactly for cases like this, or throw your own NullPointerException if you want to.


The Next Palindromic number I have incremented an integer in the function nextpalin() and converted it into a std::string for finding whether it's a palindrome or not.

If it is a palindrome I return it. The code is giving TLE on SPOJ but working fine on other online IDEs; how to make this code more efficient?

#include <iostream>
#include <string.h>
using namespace std;
int nextpalin (int num){
    while (num++) {
        string str = to_string (num); /// int to string conversion
        int l = str.length()-1;
        int s = 0;
        while( s<l ){
              if (str[s]!=str[l]) break;
              else {
                    s++;
                    l--;
                    }
        if (s>=l) return num;}
                }
   }

int main () {
    int t;
    cin >> t;
    while (t--){
    int num;
    cin >> num;
    if (num==0)  cout << "1" << endl;
   else {
       if (num<9)  cout << num+1 << endl;
       else  cout << nextpalin( num) << endl;}
     }
}

Well, my first impression is that your formatting is broken. Generally, before you even consider checking in a new revision to your local source-control or showing it to anyone, the code should be properly formatted, preferably automatically.
Importing big / open-ended namespaces is a bad idea. Read "Why is “using namespace std;” considered bad practice?" for the reasons.
You are missing the header for std::string. The unused header for C-Strings does not make up for that omission.
nextpalin() is quite a non-obvious Abbreviation, nor does it follow any of the conventions for multi-word-names. Why abbreviate at all?
Mark functions and variables only used in the local TU as TU-local using static. That reduces chances for conflicts and encourages inlining.
nextpalin() uses brute-force without any consideration for efficiency. Work smarter not harder, and you might avoid TLE.
Consider using an unsigned type when a negative number makes no sense. Can your input be negative? I doubt it, but you should know better.
Don't arbitrarily restrict the domain of your function without good reason, and document it if you do. Specifically, why doesn't it handle zero, and what business does main() have dealing with it instead?
Always check whether input succeeded. There are any number of ways it could fail.

First, there are smarter ways to check whether a given number is a palindrome in some arbitrary base than conversion to string. See "Palindrome numbers - Project Euler Problem 4". Not that it matters, because incrementing and testing is incredibly inefficient already.

Next, there are smarter ways to go from a number to the next one which is a palindrome than just checking one after the other. One possible algorithm:

n < 0 || n >= max_palindrome => 0
Increment n.
Take the upper half of the number (including middle if odd size).
If it is smaller than the reversed lower half (including middle if odd size), increment it.
Re-generate the bottom half from the top half to get the full number you wanted.

Finding the biggest palindrome, necessary for wrap-around and avoiding overflow:

template <unsigned long long base = 10, class T>
constexpr bool is_power(T n) {
    static_assert(base >= 2, "The base must be at least 2.");
    constexpr T base = BASE;
    if (n > 0)
        while (!(n % base))
            n /= base;
    return n == 1;
}

template <unsigned long long BASE = 10, class T>
constexpr T last_palindrome(T n) {
    static_assert(base >= 2, "The base must be at least 2.");
    constexpr T base = BASE;

    n = n > 0 ? n - 1 : std::numeric_limits<T>::max();
    if (is_power<BASE>(n))
        return n - 1;

    T top = n, rbottom = 0;
    for (; n >= base; n /= base * base) {
        rbottom = rbottom * base + top % base;
        top /= base;
    }
    if (n)
        rbottom = rbottom * base + top % base;

    top -= top > rbottom;

    for (n = n ? top / base : top; n; n /= base)
        top = top * base + n % base;
    return top;
}

An implementation of the algorithm itself:

template <unsigned long long BASE = 10, class T>
constexpr T next_palindrome(T n) {
    static_assert(base >= 2, "The base must be at least 2.");
    constexpr T base = BASE;
    constexpr T biggest = last_palindrome<BASE>(T());

    if (n < 0 || biggest > 0 && n >= biggest)
        return 0;
    ++n;

    T top = n, rbottom = 0;
    for (; n >= base; n /= base * base) {
        rbottom = rbottom * base + top % base;
        top /= base;
    }
    if (n)
        rbottom = rbottom * base + top % base;

    top += top < rbottom;

    for (n = n ? top / base : top; n; n /= base)
        top = top * base + n % base;
    return top;
}


Achieve error differentiation in Promises Background

I have a REST API using MongoDB, Node.js and Express that makes a request to my NoSQL DB and depending on different results, I want to differentiate which error I send the customer.

Problem

The current version of my code has a generic error handler and always sends the same error message to the client:

api.post("/Surveys/", (req, res) => {
        const surveyJSON = req.body;
        const sender = replyFactory(res);

        Survey.findOne({_id: surveyJSON.id})
            .then(doc => {
                if(doc !== null)
                    throw {reason: "ObjectRepeated"};

                //do stuff
                return new Survey(surveyJSON).save();
            })
            .then(() => sender.replySuccess("Object saved with success!"))
            .catch(error => {
                /*
                 * Here I don't know if:
                 * 1. The object is repeated
                 * 2. There was an error while saving (eg. validation failed)
                 * 3. Server had a hiccup (500)
                 */
                sender.replyBadRequest(error);
            });
    });

This is a problem, because the client will always get the same error message, no matter what and I need error differentiation!

Research

I found a possible solution, based on the division of logic and error/response handling:

https://stackoverflow.com/questions/26076511/handling-multiple-catches-in-promise-chain

However, I don't understand a few things:

I don't see how, at least in my example, I can separate the logic from the response. The response will depend on the logic after all!
I would like to avoid error sub-classing and hierarchy. First because I don't use bluebird, and I can't subclass the error class the answer suggests, and second because I don't want my code with a billion different error classes with brittle hierarchies that will change in the future.

My idea, that I don't really like either

With this structure, if I want error differentiation, the only thing I can do is to detect an error occurred, build an object with that information and then throw it:

.then(doc => {
    if(doc === null)
        throw {reason: "ObjectNotFound"};

    //do stuff
    return doc.save();
})
.catch(error => {
    if(error.reason === "ObjectNotFound")
        sendJsonResponse(res, 404, err);
    else if(error.reason === "Something else ")
        sendJsonResponse(/*you get the idea*/);
    else //if we don't know the reasons, its because the server likely crashed
        sendJsonResponse(res, 500, err);
});

I personally don't find this solution particularly attractive because it means I will have a huge if then else chain of statements in my catch block.

Also, as mentioned in the previous post, generic error handlers are usually frowned upon (and for a good reason imo).

Questions

How can I improve this code?

Would it help to rewrite it along the lines of:

function notFoundErrorHandler(error) {
  if(error.reason === "ObjectNotFound")
      return {code: 404, error:err);
}
function someOtherErrorHandler(error) {
  if(error.reason === something else")
      return {code: 499, error:err);
}
function serverErrorHandler(error) {
  // No if because this is a catch all
  return {code: 500, error:err);
}

let handlers = [notFoundErrorHandler,
                someOtherErrorHandler,
                serverErrorHandler     ];

Survey.findOne({_id: surveyJSON.id})
  ...
  .catch(error => {
    respondToError(error, handlers);
  } );

function respondToError(error, handlers) {
  for (let handler of handlers) {
    let {code, error} = handler(error);
    if (code) {
       sendJsonResponse(res, code, error);
       return;
    }
  }
  console.error("Shouldn't get here");
}


Transfer the format of an old string to a new string First, I've extended the String class to detect uppercase and lowercase characters:

class String

    def is_uppercase?
         self == self.upcase
    end

    def is_lowercase?
        self == self.downcase
    end

end

I then use these instance methods to fulfil my main goal of transferring the format of an old string to a new string:

def maintain_format(instring, outstring)
      instring_array = instring.split(//)
      outstring_array = outstring.split(//)

      both_arrays = instring_array.zip(outstring_array)

      both_arrays.each do |character|
        if character[0] == nil || character[0].is_lowercase?
          character[1].downcase! if character[1]
        elsif character[0].is_uppercase?
          character[1].upcase! if character[1]
        end
     end

     outstring_array.join("")
end

It works well and does exactly what I want:

2.0.0p247 :002 > maintain_format('jimmy','Jam')
 => "jam"
2.0.0p247 :003 > maintain_format('Jimmy','xam')
 => "Xam"
2.0.0p247 :004 > maintain_format('realLylongname','shorter')
 => "shorTer"
2.0.0p247 :005 > maintain_format('rSeHorteR','muchmuchmuchlongerfoos')
 => "mUcHmuchMuchlongerfoos"

However, what I love about ruby is how it is sort self-documenting if you use sensible variable and method names and hash keys. With this code it is a bit hard to follow regarding the 1character[0]character[1]` stuff. I attempting it with hashes but got in a bit of a mess. How would you tackle this problem?

Also interested in any ruby tricks to make this take up less lines.

Note this is part of a rails project, so all rails methods are available to me.

First off, I'd rename those two String methods. You obviously had a plan in mind when you added them (i.e. that the strings you'd be checking would be 1 character long), but from a more general perspective, it's fraught with ambiguity.

For instance, is the string "Hello, world" uppercase? Or is it lowercase? Is it sentence-case? Titlecase? So your methods, simple as they are, are perhaps too simple to make sense when applied outside the (narrow) usage you've tailored them for. As such, they probably shouldn't be global String methods. Certainly, you don't need to extend a core class like String just for a simple == comparison.

Also, Ruby convention is to not use an is_ prefix on interrogatory methods like those. The nil? method, for instance, isn't called is_nil?. Same for one?, any?, empty?, include? and so on.

As for your maintain_format method, here's what came to mind:

There's a String#chars method, which'll give you an array of chars - no need for split
Instead of zipping, use a loop and use an index - characters in a string can be accessed just like elements in an array.
And if you're going to loop, no need to loop further than the shortest of the two strings. Or, if you zip the arrays like you do right now, make sure you break the loop when one string or the other "runs out"
You only need to check for uppercase. Everything else gets downcased by default. A character can only be upper or lowercase, so if it's not uppercase, you already know that it's lowercase; no need for extra ifs (but see the update below)

I'd suggest something like

def maintain_format(template, string)
  string = string.downcase              # do this right away
  template = template[0..string.length] # shorten the template, if necessary
  template.chars.each_with_index do |char, index|
    next unless char == char.upcase     # skip lowercase chars
    string[index] = string[index].upcase
  end
  string
end

Perhaps there's a smarter way to detect upper-/lowercase chars, but I couldn't quite think of one that'd be as simple and handle unicode etc.

Lastly: Naming (again). maintain_format is a bit off to me; you're applying a format, copying a format, or something along those lines. (The title of your question also calls it "transfer" rather than "maintain". But transfer would suggest that one string loses its format, when it's applied to the other, so it's not quite right either.)

Ironically, it might make more sense to add this method to String, since it's somewhat more generally applicable. Besides, a call like template.assimilate(other_string) (besides sounding ominous, would make it much clearer which string is the template, and which string will be changed. Right now, it can quickly become confusing which argument is which. But again, better naming might alleviate that.

Update: As 200_success rightly points out in the comments, it's surprising that a string that's longer than the template string gets downcased rather than simply keep whatever case it had. And code shouldn't surprise.

Hence it might be better to do this:

def maintain_format(template, string)
  string = string.dup  # still want a duplicate to work on
  template = template[0..string.length]
  template.chars.each_with_index do |char, i|
    string[i] = char == char.upcase ? string[i].upcase : string[i].downcase
  end
  string
end

You can still achieve the original behavior by simply downcasing the string yourself before passing it to the method.


Creating a repetitive string in Common Lisp I needed to have a Lisp function that could produce a string of a certain length, created by repeated concatenations of a another string given as argument (so for example by giving 10 and "abc" I could obtain "abcabcabca"). I came up with a perverse idea of using a circular list of characters in combination with the loop macro. I'm not really aiming at great performance, but I'm wondering what are the disadvantages of this approach and what could be improved. I also added a switch indicating at which end to start filling the string.

(defun pad-with-string (n str &key (from-end nil))
  "Create a string of length `n` filled with repeating instances of `str`.
If `from-end` is T the string is filled from the end."
  (labels ((circular-list (&rest elements)
         (let ((cycle (copy-list elements)))
           (nconc cycle cycle))))
    (loop :repeat n
       :for c
       :in (apply #'circular-list (coerce (if from-end (nreverse str) str) 'list))
       :collect c
       :into chars
       :finally (return (coerce (if from-end (nreverse chars) chars) 'string)))))

Not a bad approach.

I can see one arguable bug, and have a few style notes on your code though. Lets start with the bug:

CL-USER> (defparameter a "test")
A
CL-USER> (pad-with-string 6 a :from-end t)
"tsetts"
CL-USER> a
"tset"
CL-USER>

Is that what you were expecting? It's happening because you call nreverse on your input argument if from-end is passed. When I've got a situation where I have to mutate parameters, I follow the Scheme convention and name the procedure with a trailing bang (so, like pad-with-string!). Your comments state that you don't care about efficiency though, which leads me to believe you could just use the standard reverse rather than nreverse.

You're using apply on circular list, but you're defining it locally and only calling it in one place. In this situation, I'd kill the &rest and just call the function

...
  (labels ((circular-list (elements)
         (let ((cycle (copy-list elements)))
           (nconc cycle cycle))))
    (loop :repeat n
       :for c
       :in (circular-list (coerce (if from-end (nreverse str) str) 'list))
...

You've got the (if from-end (reverse a) a) pattern in a couple of places. I'd pull that out into an additional local definition.

...
       (maybe-reverse (thing)
         (if from-end (reverse thing) thing)))
    (loop :repeat n
       :for c
       :in (circular-list (coerce (maybe-reverse str) 'list))
       :collect c
       :into chars
       :finally (return (coerce (maybe-reverse chars) 'string)))))

thanks to Rainer for pointing this out

The result of the collect call is returned implicitly at the end of the loop unless you give it an explicit name. Which means you can avoid finally (return ...) by wrapping those transformations around the loop itself:

...
(coerce
 (maybe-reverse
  (loop :repeat n
     :for c
     :in (circular-list (coerce (maybe-reverse str) 'list))
     :collect c))
 'string)))

Not a hard and fast rule, but people typically use symbols rather than keywords for loop words. So,

...
(coerce
 (maybe-reverse
  (loop repeat n
     for c
     in (circular-list (coerce (maybe-reverse str) 'list))
     collect c))
 'string)))

Again, there's not really a standard for this, but I tend to like putting conditions and their effects/modifiers on the same line.

...
(coerce
 (maybe-reverse
  (loop repeat n
     for c in (circular-list (coerce (maybe-reverse str) 'list))
     collect c))
 'string)))

So,

(defun pad-with-string (n str &key (from-end nil))
  (labels ((circular-list (elements)
             (let ((cycle (copy-list elements)))
               (nconc cycle cycle)))
           (maybe-reverse (thing)
             (if from-end (reverse thing) thing)))
    (coerce (maybe-reverse
              (loop repeat n
                 for c in (circular-list (coerce (maybe-reverse str) 'list))
                 collect c))
            'string)))


Reflecting and counting points on a 2D grid First number indicates the number of points, followed by N points
  (x,y). Then the next number indicates Q num_queries,  followed by Q queries:

  Reflect all points between point i and j both including along the
  X axis. This query is represented as "X i j"
  Reflect all points between point i and j both including along the Y axis. This query is
  represented as "Y i j"
  Count how many points between point i and j both including lie in each of the 4 quadrants. This query is
  represented as "C i j"

  Sample Input

4
1 1
-1 1
-1 -1
1 -1
5
C 1 4
X 2 4
C 3 4
Y 1 2
C 1 3

  Sample Output

1 1 1 1
1 1 0 0
0 2 0 1

How can i improve this code for better metrics?

import java.util.ArrayList;
import java.util.Scanner;

/**
 *
 * @author cypronmaya
 */
class Point {

    int x;
    int y;

    Point(int x, int y) {
        this.x = x;
        this.y = y;
    }
}

public class Quad_query {

    private ArrayList<Point> arrayList;

    public Quad_query(int capacity) {
        this.arrayList = new ArrayList<Point>(capacity);
    }

    public static void main(String args[]) {

        Scanner scanner = new Scanner(System.in);
        int num_of_points = scanner.nextInt();
        Quad_query quad_query = new Quad_query(num_of_points);
        for (int i = num_of_points; --i >= 0;) {
            int x = scanner.nextInt();
            int y = scanner.nextInt();
            quad_query.add(x, y);
        }
        int num_of_queries = scanner.nextInt();
        for (int i = num_of_queries; --i >= 0;) {
            char c = scanner.next().charAt(0);
            quad_query.query(c, scanner.nextInt() - 1, scanner.nextInt() - 1);
        }
    }

    public void add(int x, int y) {
        Point point = new Point(x, y);
        arrayList.add(point);
    }

    public void query(char c, int i, int j) {

        switch (c) {
            case 'C':
                queryC(i, j);
                break;
            default:
                queryXY(c, i, j);
                break;
        }
    }

    public void queryC(int i, int j) {

        int quad_1 = 0, quad_2 = 0, quad_3 = 0, quad_4 = 0;
        for (int k = i; k <= j; k++) {
            Point pnt = arrayList.get(k);
            int pnt_x = pnt.x;
            int pnt_y = pnt.y;
            if (pnt_x > 0 && pnt_y > 0) {
                quad_1++;
            } else if (pnt_x < 0 && pnt_y > 0) {
                quad_2++;
            } else if (pnt_x < 0 && pnt_y < 0) {
                quad_3++;
            } else if (pnt_x > 0 && pnt_y < 0) {
                quad_4++;
            }
        }
        System.out.println(quad_1 + " " + quad_2 + " " + quad_3 + " " + quad_4);
    }

    private void queryXY(char c, int i, int j) {

        for (int k = i; k <= j; k++) {
            Point pnt = arrayList.get(k);
            int pnt_x = pnt.x;
            int pnt_y = pnt.y;
            if (c == 'X') {
                pnt = new Point(pnt_x, -pnt_y);
            } else if (c == 'Y') {
                pnt = new Point(-pnt_x, pnt_y);
            }
            arrayList.set(k, pnt);
        }
    }
}

Instead of initializing your arrayList when you create the instance of the class, do it when you have the num_of_points value from the scanner.

arrayList = new ArrayList<Point>(num_of_points);

This will mean the array list doesn't need to resize itself (potentially slow operation). In the example you have given with only 4 points it won't make any difference, but if there were a larger number of points you may save some time!

Where you get the value from the scanner char c you trim the value, which you shouldn't need to do as the Scanner should stop when it sees a delimiter (in this case, a space).

In the queryC method, you have a for loop...

for (int b = 0; b < len; b++) {
    pnt = arrayList.get(b);
}

...that does nothing, so you can remove it.

A few notes on code style...

In the main method your for loops go from X down to 0, but in the query methods you go from 0 up to X. Just wondering why?
You also use (0 - pnt.x) where you could use -pnt.x.

Hope this helps.


Get the tail of a file (the last 10 lines) Print the tail of a file, if the file has less than 10 lines, then everything available is printed.

Any suggestions for improvement are welcome.

package je3.io;

import java.io.IOException;
import java.io.RandomAccessFile;

/**
 * Created by IDEA on 29/01/15.
 */
public class Tail {
    public static String[] tailLines(String filename, int nLinesToRead) throws IOException {
        RandomAccessFile randomAccessFile = new RandomAccessFile(filename, "r");
        long lengthOfFile = randomAccessFile.length();
        long counterFromEnd = 1L;
        long newlineCounterGoal = nLinesToRead;
        int newlineCounter = 0;
        long tailPosition = 0L; // start of the end ;-)

        // If you want to get the last 10 lines,
        // and the last line ends with a newline, then you need to count back 11 newlines
        // if there is no trailing newline, then you only need to count back 10
        randomAccessFile.seek(lengthOfFile - 1L);
        char currentChar = (char) randomAccessFile.readByte();
        if(currentChar == '\n') {
            newlineCounterGoal++;
        }

        while(counterFromEnd <= lengthOfFile) {
            randomAccessFile.seek(lengthOfFile - counterFromEnd);
            if(randomAccessFile.readByte() == '\n') {
                newlineCounter++;
            }
            if(newlineCounter == newlineCounterGoal) {
                tailPosition = randomAccessFile.getFilePointer();
                break;
            }
            counterFromEnd++;
        }
        randomAccessFile.seek(tailPosition);

        String line;
        String[] lines = new String[nLinesToRead];
        int nLine = 0;
        while((line = randomAccessFile.readLine()) != null) {
            lines[nLine++] = line;
        }
        return lines;
    }

    public static void main(String[] args) throws IOException {
        if(args.length != 1) {
            throw new IllegalArgumentException();
        }
        String[] lines = tailLines(args[0], 10);
        for(String line : lines) {
            if(line != null) {
                System.out.println(line);
            }
        }
    }
}

Obviously the tail of the file is harder to do. This is a much more challenging situation than getting the first 10 lines. You have chosen a system that requires complicated byte manipulation to get bytes in to chars, and I don't believe the system will work for non-ascii files where the character encoding of the data may have a second-byte of 0x0A causing a mis-parse of a new-line.

For simplicity sake I would still recommend you use a forward cursor, and a circular buffer. This will work for all files regardless of the size, but the performance will be related to the size. On my system though, even 20MB files were done within moments (0.3 seconds according to the bash 'time' command). So that are not too large (a couple of megabytes), and is easy:

String[] lines = new String[10];
int count = 0;
String line = null;
while ((line = bufferedReader.readLine()) != null) {
    lines[count % lines.length] = line;
    count++;
}

That will load as many as possible lines in to the lines array, limited to 10. It will have the 10-most-recent lines.

Then you can dump those with:

int start = count - 10;
if (start < 0) {
    start = 0;
}

for (int i = start; i < count; i++) {
    System.out.println(lines[i % lines.length];
}

The above code will always work, but for larger files will be slow.

For practical purposes though, I don't believe you will run in to problems, and you should be fine.

I worked a little Java 8 in to the following code as well. Consider the RingBuffer class which allows you to collect the data in to it:

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import java.util.stream.Stream;

public class Tail {

    private static final class RingBuffer {
        private final int limit;
        private final String[] data;
        private int counter = 0;

        public RingBuffer(int limit) {
            this.limit = limit;
            this.data = new String[limit];
        }

        public void collect(String line) {
            data[counter++ % limit] = line;
        }

        public List<String> contents() {
            return IntStream.range(counter < limit ? 0 : counter - limit, counter)
                    .mapToObj(index -> data[index % limit])
                    .collect(Collectors.toList());
        }

    }

    public static final List<String> tailFile(final Path source, final int limit) throws IOException {

        try (Stream<String> stream = Files.lines(source)) {
            RingBuffer buffer = new RingBuffer(limit);
            stream.forEach(line -> buffer.collect(line));

            return buffer.contents();
        }

    }

    public static void main(String[] args) throws IOException {
        tailFile(Paths.get(args[0]), 10).forEach(line -> System.out.println(line));
    }
}


Closed form Fibonacci using Integer I'm implementing the closed form of Fibonacci numbers.
I figure using a custom data type I can do so without leaving Integer.
It isn't more efficient than the "standard" approach as I couldn't figure an efficient way to calculate the power of my custom number type.

But sticking to this algorithm I would be interested in best practices to write this.
In particular I'm uncertain about the use of Num for my FibNum and the use of fromJust.

module Fibo
    (
     fibo
    ) where

import Data.Maybe (fromJust)

-- use a data type similar to complex numbers.
-- The only irrational number are multiples of sqrt 5
-- which will cancel away in the closed form.
data FibNum = FibNum { int::Integer, sq5::Integer }
  deriving (Show)

instance Num FibNum where
  (+) (FibNum a1 b1) (FibNum a2 b2) = FibNum (a1+a2) (b1 + b2)
  (*) (FibNum a1 b1) (FibNum a2 b2) = FibNum intSide rootSide
    where intSide = a1*a2 + 5 * b1 * b2
          rootSide = a1*b2 + b1*a2
  negate (FibNum a b) = FibNum (negate a) (negate b)
  fromInteger n = FibNum (fromInteger n) 0
  -- no need for abs and sign for now
  abs _ = undefined
  signum _ = undefined

funit = FibNum 1 0
-- actual phi and psi are (1 + sqrt 5)/2 and (1 - sqrt 5)/2
-- I would prefer to avoid fractions so I divide at the end
fphi = FibNum 1 1
fpsi = FibNum 1 (negate 1)

-- was originally planning to calculate the binomial coefficients
-- but seeing that I just need n relatively simple multiplications
-- that seem like unnecessary complication
pow :: FibNum -> Integer -> FibNum
pow _ 0 = funit
pow x 1 = x
pow x n = x * pow x (n-1)

-- in the closed form the integer part is always 0
divR5 :: FibNum -> Maybe Integer
divR5 (FibNum 0 n) = Just n
divR5 _ = Nothing

fibo :: Integer -> Integer
fibo n = flip div p2 . fromJust $ divR5 nom
  where nom = pow fphi n - pow fpsi n
        p2 = 2^n

addendum
Thanks a lot for the useful comments.
If it's interesting I put my code in following repo https://github.com/bdcaf/haskell-fibonacci

Opening comments

I'm not a Haskell expert, but I hope I can offer some helpful comments and critiques.

Overall, I think this is mostly good and idiomatic code. You make good use of pattern matching and have pretty clean definitions of your code.

I'm going to first address style and your questions. I'll then address the performance issue, since it actually isn't that hard to fix.

Style

I think most of your style is fine. I just personally prefer to nest where clauses like so

foo x y = x' + y'
  where
    x' = 2*x
    y' = 2*y

How you do it is up to you.

Using Num

I don't think there's much wrong with using Num for FibNum to make things easier. I would just go ahead and implement abs and signum if you do, since they aren't too difficult, although you don't have to since you don't export FibNum (which I think is good). However, Num is just an interface and doesn't officially make any promises about how its operations behave (i.e. it doesn't have any laws).

The data type you've defined is the Ring* Z[√5]. I found a library that has a Ring type class which you could implement for your datatype. You could then use this type class's implementation of (^) instead of pow for a speedup (see my comments regarding performance).

Whether you want to be more precise like this is up to you. Num unofficially is a ring, too, and is the more common type class.

As an addendum, since you are using division, you may wish to consider changing your Integers to Rationals, which would make your type the Quadratic Field Q[√5]. There is also a Field typeclass.

* Don't worry too much about the abstract algebra here if you aren't familiar. What's more important is understanding the interfaces and the promises they make/laws they obey. For example, the order of operation of addition can be rearranged (known as commutativity) in a Ring and Field. You don't need to know anything more than how to implement the functions required, make sure they satisfy the laws, and how to use the interface (I don't really remember my rings and fields anyway, I just googled around a bit).

Using fromJust

I think that the usage of fromJust is unnecessary and serves to undermine the fact that divR5 returns a Maybe. Below, I offer two alternative options.

I would also consider either renaming divR5 to indicate that it doesn't actually divide an arbitrary FibNum by √5, leaving a comment to that effect, or defining it in the where clause of fibo (which is what I would do). If you define it in the where clause, you can't accidentally misuse it elsewhere.

Return a Maybe

fiboMaybe :: Integer -> Maybe Integer
fiboMaybe n = (`div` p2) <$> divR5 nom
  where
    nom = pow fphi n - pow fpsi n
    p2  = 2^n

I like to use the infix form of fmap (and also I prefer to use div in infix), but you can also explicitly case on the result or use do notation.

Give a better error message

If you aren't going to return a Maybe, then you might as well provide a more informative error message than fromJust's.

fiboError :: Integer -> Integer
fiboError n = divR5 nom `div` p2
  where
    nom = pow fphi n - pow fpsi n
    p2  = 2^n
    divR5 (FibNum 0 n) = n
    divR5 _            = error "fiboError: got nonzero integer part in divR5 (this shouldn't happen)"

Improving efficiency

It turns out that it's not too hard to improve your efficiency. If you implement the Ring or Field type class and use the functions provided there, you should see a major speedup. But if you don't want to, we can fix the speed in only a couple lines of code.

The inefficiency in your code comes from your implementation of pow, which, while correct, takes linear time. You can reduce this significantly. Here's how we can fix it. We're going to use the Product Monoid and the function mtimesDefault from Data.Semigroup.

If you aren't familiar with the abstract algebra terminology here, ignore that junk for a second. Here's the lowdown: we're going to take advantage of the fact that for your datatype, multiplication is associative. What does that mean? It means that

(x * y) * z == x * (y * z)

i.e. we can move around parentheses in a product without changing its value. If you can do that, you have something known as a Semigroup. That's all a Semigroup is! A Monoid is a Semigroup where you know there's some element that does nothing when you multiply it. In this case, that element is 1:

1 * x == x * 1 == x

If you have a Monoid (which FibNum is with respect to the multiplication operation) and want to multiply a number by its self n times, mtimesDefault :: (Integral b, Monoid a) => b -> a -> a does this more efficiently than the naive solution.

The Product wrapper type takes a Num a and uses its multiplication operation as the Monoid operation. So to get a faster pow, here's all the code we have to write:

import Data.Monoid    (Product(..))
import Data.Semigroup (mtimesDefault)

powFast :: Integral a => FibNum -> a -> FibNum
powFast n exp = getProduct $ mtimesDefault exp (Product n)

If we replace pow with powFast, your implementation becomes much faster than the "standard" approach too!

Of course, it isn't too hard to write a faster pow function by hand. It's just neat that Haskell has built-in machinery that lets you avoid doing so. If you wanted to figure out how to do it faster by hand, I would hint you to think about exponentiation of regular numbers.

Say I asked you to compute 2^50 by hand. I claim you don't need to take 50 multiplications to give me an answer. Try and think about how you would do it efficiently, taking advantage of the fact that you're only ever multiplying by 2 and that you multiplication is associative.

Here's a hint:

   Think about multiplying exponents with the same base. 2^x * 2^y = 2^(x+y). What about 2^x * 2^x?

and another:

   For example, suppose you get to 2^4 = 16. From here, if you multiply 16 by itself, you'll get 16 * 16 = (2^4) * (2^4) = 2^(2*4) = 2^8. That saved 3 multiplications compared to the naive method of multiplying by 2!

You can find the answer in the implementation of stimesDefault.


Javascript: Profile Lookup from freecodecamp This is my answer in freecodecamp "Basic Javascript: Profile Lookup" (third day trying javascript using Udemy and freecodecamp).

My problem with the code is the huge number of lines on my lookUpProfile function. I would like to minimize the lines of code of my lookUpProfile function, can you suggest me ways to achieve that?

//Setup
var contacts = [
    {
        "firstName": "Akira",
        "lastName": "Laine",
        "number": "0543236543",
        "likes": ["Pizza", "Coding", "Brownie Points"]
    },
    {
        "firstName": "Harry",
        "lastName": "Potter",
        "number": "0994372684",
        "likes": ["Hogwarts", "Magic", "Hagrid"]
    },
    {
        "firstName": "Sherlock",
        "lastName": "Holmes",
        "number": "0487345643",
        "likes": ["Intriguing Cases", "Violin"]
    },
    {
        "firstName": "Kristian",
        "lastName": "Vos",
        "number": "unknown",
        "likes": ["Javascript", "Gaming", "Foxes"]
    }
];

function lookUpProfile(firstName, prop){
// Only change code below this line

  var contain = 0;
  if(firstName == "Akira"){
    contain = 0;
    if(contacts[contain].firstName == firstName)
      if(contacts[contain].hasOwnProperty(prop))
        return contacts[contain][prop];
      else
        return "No such property";
    else
      return "No such contact";
  }
  else if(firstName == "Harry"){
    contain = 1;
    if(contacts[contain].firstName == firstName)
      if(contacts[contain].hasOwnProperty(prop))
        return contacts[contain][prop];
      else
        return "No such property";
    else
      return "No such contact";
  }
  else if(firstName == "Sherlock"){
    contain = 2;
    if(contacts[contain].firstName == firstName)
      if(contacts[contain].hasOwnProperty(prop))
        return contacts[contain][prop];
      else
        return "No such property";
    else
      return "No such contact";
  }
  else if(firstName == "Kristian"){
    contain = 3;
    if(contacts[contain].firstName == firstName)
      if(contacts[contain].hasOwnProperty(prop))
        return contacts[contain][prop];
      else
        return "No such property";
    else
      return "No such contact";

  }
  else
    return "No such contact";

// Only change code above this line
}

// Change these values to test your function
lookUpProfile("Akira", "lastName");

You have very repetitive code. This can be optimized. I'm not sure whether your function name lookUpProfile is the correct one, as you actually look up a property of a Contact and not its whole profile. But if the task is to look up a certain property of a Contact you can simplify the code to this:

function lookUpProfile(firstName, prop) {
    for (let contact of contacts) {
        if (contact.firstName === firstName) {
            return contact[prop];
        }
    }
    return false;
}

How it works

It loops through all contacts:

for (let contact of contacts) {

It tests whether the firstName matches:

if (contact.firstName === firstName) {

In case a match is found it returns the requested property or undefined:

return contact[prop];

In case no match is found it returns false:

return false;

What it returns

This function will have a mixed return value. The code that is calling the function can now deal with the result. It will return one of the following:

false if no Contact with the requested firstName exists
undefined if a Contact matches but the requested property doesn't exists
string|array if a match was found and the property exists

So all those calls will work:

console.log(
    lookUpProfile(),
    lookUpProfile('A'),
    lookUpProfile('Sherlock'),
    lookUpProfile('Sherlock', 'number'),
    lookUpProfile('Kristian', 'likes')
);

The return values are:

false false undefined 0487345643 Array [ "Javascript", "Gaming", "Foxes" ]

What the code doesn't do:

It doesn't check whether there's a choice for Contacts, i.e. if more than one Contact with that name exists. It will always exit at the first match.

jsFiddle Demo

Try before buy

Update "No such property"

Regarding the OP's comment about a different return value, I would suggest keeping the code as is and let other parts worry about a concrete message:

var value = lookUpProfile('Sherlock', 'random');

if ('undefined' === typeof value) {
    console.log('No such property');
};

If your task is to include that text into the function you can replace this line:

return contact[prop];

with:

return 'undefined' === typeof contact[prop] ? 'No such property' : contact[prop];

This is a lot harder to handle, as the string "No such property" could be an actual value of a property.


Animation with 5x5 map of cubes I am trying to learn how to animate with tkinter. The code below is an example I was able to build. It creates a small 5x5 map of cubes, then one of them randomly moves around the screen (preferably on the map, but it can wander off).

Is this a good way to do it?  Is there a more pythonic, or more efficient way of doing it?

from tkinter import *
from time import sleep
from random import randrange

class alien(object):
    def __init__(self):
        self.root = Tk()
        self.canvas = Canvas(self.root, width=400, height=400)
        self.canvas.pack()
        self.map = [[1, 0, 0, 1, 0], [0, 1, 0, 1, 0], [0, 0, 1, 0, 0], [0, 1, 1, 0, 0], [1, 0, 0, 1, 0]]
        self.x = 0
        self.y = 0
        for row in range(len(self.map)):
            for column in range(len(self.map[row])):
                color = "green"
                if self.map[row][column] == 1:
                    color = "black"
                #print(50 * row + 50, 50 * column + 50)
                self.canvas.create_rectangle(50 * row , 50 * column , 50 * row + 50, 50 * column + 50,
                    outline=color, fill=color)
        self.creature = self.canvas.create_rectangle(50 * self.x, 50 * self.y, 50 * self.x + 50, 50 * self.y + 50,
            outline="red", fill="red")
        self.canvas.pack(fill=BOTH, expand=1)
        self.root.after(0, self.animation)
        self.root.mainloop()

    def animation(self):
        while True:
            i = randrange(1, 5)
            print(i)
            if i == 1:
                y = -1
            elif i == 2:
                y = 1
            elif i == 3:
                x = -1
            elif i == 4:
                x = 1
            sleep(0.025)
            print(self.x, self.y)
            self.canvas.move(self.creature, self.x, self.y)
            self.canvas.update()
alien()

Here is anoted version of your code

from tkinter import *
from time import sleep
from random import randrange

# lowercase tkinter suggest you are using python3, in this case
# inheriting (object) is not needed anymore to create newstyle
# class
class alien(object):
    def __init__(self):
        self.root = Tk()
        self.canvas = Canvas(self.root, width=400, height=400)
        self.canvas.pack()
        self.map = [[1, 0, 0, 1, 0], [0, 1, 0, 1, 0], [0, 0, 1, 0, 0], [0, 1, 1, 0, 0], [1, 0, 0, 1, 0]]
        self.x = 0
        self.y = 0
        for row in range(len(self.map)):
            for column in range(len(self.map[row])):
                color = "green"
                #since 0 evaluate to False, you can drop '== 1'
                if self.map[row][column] == 1:
                    color = "black"
                self.canvas.create_rectangle(50 * row , 50 * column , 50 * row + 50, 50 * column + 50,
                    outline=color, fill=color)
        self.creature = self.canvas.create_rectangle(50 * self.x, 50 * self.y, 50 * self.x + 50, 50 * self.y + 50,
            outline="red", fill="red")
        #this call overrides the previous 'canvas.pack'. Remove first.
        self.canvas.pack(fill=BOTH, expand=1)
        self.root.after(0, self.animation)
        self.root.mainloop()

    def animation(self):
        #see below for further explanations on animations.
        while True:
            i = randrange(1, 5)
            print(i)
            #note: randrange ouput from 1 to 5 (included)
            #thus i = 5 is not handled in your switch (might be on purpose)
            if i == 1:
                y = -1
            elif i == 2:
                y = 1
            elif i == 3:
                x = -1
            elif i == 4:
                x = 1
            # after you switch, either x or y (or both) are not
            # initialized
            sleep(0.025)
            # self.x and self.y are never updated
            print(self.x, self.y)
            # move is a relative command (expect delta x,delta y)
            # if self.x,self.y is your creature absolute position,
            # you might want to use 'coords' on (self.x, self.y)
            # or 'move' on (x,y)
            # in addition, your "*50" factor has disapeared.
            self.canvas.move(self.creature, self.x, self.y)
            # it is good practice to use 'update_idletasks' instead
            # of 'update' to avoid race conditions. By the way
            # the use of one of them is seldom required (see below)
            self.canvas.update()
alien()

Regarding how you handle the animation, it could be profitable to run your simulation in the python mainloop. What you does now is to provide your own mainloop (while True: [...] sleep(0.025) [...] canvas.update()). This can work, but will fail as soon as you will want to add interactivity to your application (other widgets, or react to events on canvas). This will not work because the execution path is stuck in alien.animation. The solution there would be to rely on tkinter mainloop and to beg for alien.animation to be repeatedly called.

def animation(self):
    #while True:  no longer needed
        i = randrange(1, 5)
        #[...]
        elif i == 4:
            x = 1
        #sleep(0.025) becomes
        self.root.after(25, animation)
        print(self.x, self.y)
        self.canvas.move(self.creature, x*50, y*50)
        #self.canvas.update() no longer needed


Threaded generalised transform I'm writing an application which works with huge amounts of sequential data, and often found the need to use std::transform. I see two potential improvements to std::transform:

Allow for variable number parameters.
Take advantage of the linear separability of the data by multithreading.

Can anyone suggest any design/performance improvements on my implementation?

threaded_transform.h

#include <vector>
#include <thread>

template<typename InputIterator, typename OutputIterator,
         typename Function, typename... Params>
OutputIterator
trans(InputIterator first, InputIterator last, OutputIterator result,
          Function f, Params... params)
{
    for (; first != last; ++first, ++result)
        *result = f(*first, params...);
    return result;
}

template<typename InputIterator, typename OutputIterator,
         typename Function, typename... Params>
OutputIterator
threaded_transform(unsigned num_threads, InputIterator first,
                   InputIterator last, OutputIterator result,
                   Function f, Params... params)
{
    std::size_t num_values = last - first;
    std::size_t num_values_per_threads = num_values / num_threads;

    std::vector<std::thread> threads;
    threads.reserve(num_threads);

    for (unsigned i = 1; i <= num_threads; ++i) {
        if (i == num_threads) {
            // The last thread processes the remaining values.
            threads.push_back(std::thread(trans<InputIterator, OutputIterator, Function, Params...>,
                                          first, last, result, f, params...));
        } else {
            threads.push_back(std::thread(trans<InputIterator, OutputIterator, Function, Params...>,
                                          first, first + num_values_per_threads, result, f, params...));
        }
        first  += num_values_per_threads;
        result += num_values_per_threads;
    }

    for (auto& thread : threads)
        thread.join();

    return result;
}

main.cpp

#include <vector>
#include "threaded_transform.h"

int main()
{
    auto sum = [] (int a, int b) { return a + b; };

    std::vector<int> values = {1,2,3,4,5,6,7,8,9,10};
    std::vector<int> results;
    results.resize(10);

    threaded_transform(4, values.cbegin(), values.cend(), results.begin(), sum, 10);

    for (auto result : results) {
        std::cout << result << std::endl;
    }
}

You probably want to forward your parameters:

   *result = f(*first, params...);

Try:

   *result = f(*first, std::forward<Params>(params)...);

To go along with forwardign you probably want two versions of trans() on that takes values by reference/value one that takes r-value references:

// Normal parameters.
trans(InputIterator first, InputIterator last, OutputIterator result,
      Function f, Params const&... params)

// R-Value parameters.
trans(InputIterator first, InputIterator last, OutputIterator result,
      Function f, Params&&... params)

Going to main transform function.

I am not sure you in realty want to pass a function and arguments. That's the whole point of the lambda. So you can wrap the function call and its parameters into a function.

threaded_transform(4, values.cbegin(), values.cend(), results.begin(), sum, 10);

// Or would you prefer:

threaded_transform(4, values.cbegin(), values.cend(), results.begin(),
    [](int other){ return add(other, 10);}
);

//Or even
threaded_transform(4, values.cbegin(), values.cend(), results.begin(),
    [](int other){ return 10 + other;}
);

If you do this you should write details about your iterator requirements.

The requirements for std::transform()

template<class InputIterator1, class InputIterator2, class OutputIterator, class BinaryFunction>
OutputIterator transform(InputIterator1 first1, InputIterator1 last1,
                         InputIterator2 first2, OutputIterator result,
                         BinaryFunction binary_op);

Where:
   InputIterator must be a model of Input Iterator.
   OutputIterator must be a model of Output Iterator.

In your threaded implementation you have a more stringent requirement for the output iterator. I believe it needs to be random access iterator.

    OutputIterator must be a model of Random Access Iterator.

See: http://www.sgi.com/tech/stl/RandomAccessIterator.html


Simple Postfix Calculator using Java 8 Inspired by some older questions, I decided to create my own postfix calculator using Java 8. I'd like to have all aspects reviewed.

public enum Operator implements DoubleBinaryOperator {
    PLUS    ("+", (l, r) -> l + r),
    MINUS   ("-", (l, r) -> l - r),
    MULTIPLY("*", (l, r) -> l * r),
    DIVIDE  ("/", (l, r) -> l / r);

    private final String symbol;
    private final DoubleBinaryOperator binaryOperator;

    private Operator(final String symbol, final DoubleBinaryOperator binaryOperator) {
        this.symbol = symbol;
        this.binaryOperator = binaryOperator;
    }

    public String getSymbol() {
        return symbol;
    }

    @Override
    public double applyAsDouble(final double left, final double right) {
        return binaryOperator.applyAsDouble(left, right);
    }
}

public class CalculationFailedException extends RuntimeException {
    private static final long serialVersionUID = 6849565649585489467L;

    public CalculationFailedException() {
        super();
    }

    public CalculationFailedException(final String message) {
        super(message);
    }

    public CalculationFailedException(final Throwable cause) {
        super(cause);
    }

    public CalculationFailedException(final String message, final Throwable cause) {
        super(message, cause);
    }
}

public interface Calculator {
    public double calculate(final String input);
}

public class PostfixCalculator implements Calculator {
    private static final List<String> OPERATORS_LIST = Arrays.stream(Operator.values())
            .map(Operator::getSymbol)
            .collect(Collectors.toList());

    private static final Map<String, Operator> STRING_TO_OPERATOR_MAPPING = Arrays.stream(Operator.values())
            .collect(Collectors.toMap(op -> op.getSymbol(), op -> op));

    private final Stack<Double> numberStack = new Stack<>();

    @Override
    public double calculate(final String input) throws IllegalArgumentException {
        try {
            Arrays.stream(input.split(" ")).forEach(this::processElement);
            return safePop();
        } catch (CalculationFailedException | NumberFormatException ex) {
            throw new CalculationFailedException("Calculation has failed for expression: " + input);
        }
    }

    private void processElement(final String element) {
        Objects.requireNonNull(element);
        if (OPERATORS_LIST.contains(element)) {
            processOperator(STRING_TO_OPERATOR_MAPPING.get(element));
        }
        else {
            processNumber(Double.parseDouble(element));
        }
    }

    private void processOperator(final Operator operator) {
        double right = safePop();
        double left = safePop();
        numberStack.push(operator.applyAsDouble(left, right));
    }

    private void processNumber(final double number) {
        numberStack.push(number);
    }

    private double safePop() {
        if (numberStack.isEmpty()) {
            throw new CalculationFailedException();
        }
        return numberStack.pop();
    }
}

Code can be called for example with:

Calculator calculator = new PostfixCalculator();
double result = calculator.calculate("3 4 +");

Which will give 7.0 as result.

Another example:

Calculator calculator = new PostfixCalculator();
double result = calculator.calculate("5 1 2 + 4 * + 3 -");

Which gives 14.0 as result.

I like how the Functions have simplified the enum, but, you have put logic in the implementation class that belongs in the enum....

The code:

private static final List<String> OPERATORS_LIST = Arrays.stream(Operator.values())
        .map(Operator::getSymbol)
        .collect(Collectors.toList());

private static final Map<String, Operator> STRING_TO_OPERATOR_MAPPING = Arrays.stream(Operator.values())
        .collect(Collectors.toMap(op -> op.getSymbol(), op -> op));

should be part of the Operator enum.... and there should be a static method on the enum:

public static Operator operatorFor(String op) {
    return STRING_TO_OPERATOR_MAPPING.get(op);
}

Then, your calling class would have the code:

private void processElement(final String element) {
    Objects.requireNonNull(element);
    Operator op = Operator.operatorFor(element);
    if (op != null) {
        processOperator(op);
    }
    else {
        processNumber(Double.parseDouble(element));
    }
}

Also, you should be trapping the NumberFormatException closer to where it is thrown....

        processNumber(Double.parseDouble(element));

should be:

try {
    processNumber(Double.parseDouble(element));
} catch (NumberFormatException nfe) {
    throw new CalculationFailedException("Unable to parse value " + element + " as a double.", nfe);
}

That way, you can identify which value failed to parse.


Using 'each' to print chunks of an array Working through Day 2 of Ruby in "7 Languages in 7 Weeks" - the answer to the second question seems acceptable, the first one feels quite wrong. Any suggestions?

Print the contents of an array of sixteen numbers, four numbers at a
time, using just each.

group = 0

(1..16).each do |x|
    print x
    group = group + 1
    if group % 4 == 0
        puts
        group = 0
    end
end

Now, do the same with each_slice in Enumerable (copied directly from http://ruby-doc.org/core-1.9.3/Enumerable.html#method-i-each_slice)

(1..16).each_slice(4) {|x| p x}

No need to save group x already saving it.

(1..16).each do |x|
  print x
  puts if x % 4 == 0
end


Find intersections of overlapping intervals This code finds the intersections of all overlapping intervals.

Example: if [0-20], [15-40], and [25-50] are the 3 intervals then the output should be [15-20] and [25-40].

I could not find an answer with complexity less than \$O(n^2)\$. Please suggest a better solution if one exists. Additionally, assume the input intervals are sorted by their start times.

public static Set<OverlapCoord> getOverlap(List<Interval> intervalList) {
    if (intervalList == null) {
        throw new NullPointerException("Input list cannot be null.");
    }

    final HashSet<OverlapCoord> hashSet = new HashSet<OverlapCoord>();

    for (int i = 0; i < intervalList.size() - 1; i++) {
        final Interval intervali =  intervalList.get(i);

        for (int j = 0; j < intervalList.size(); j++) {
            final Interval intervalj = intervalList.get(j);

            if (intervalj.getStart() < intervali.getEnd() && intervalj.getEnd() > intervali.getStart() && i != j) {
                hashSet.add(new OverlapCoord(Math.max(intervali.getStart(),intervalj.getStart()),
                                             Math.min(intervali.getEnd(), intervalj.getEnd())));
            }
        }
    }

    return hashSet;
}

Because the intervals are already sorted by start time, you can begin the inner loop at j = i + 1 and simplify the if test and drop the call to max:

if (intervalj.getStart() < intervali.getEnd()) {
    hashSet.add(new OverlapCoord(intervalj.getStart(),
                                 Math.min(intervali.getEnd(), intervalj.getEnd())));
}

However, is it possible for three intervals to overlap?

[===============]
        [=============]
             [=============]

If so, this will produce three overlaps:

        [=======]
             [==]
             [========]

Is this desired? What about if the first and third intervals above butted up against each other rather than overlapping themselves?

        [=======]
                 [====]

Should all these cases be merged into the same single overlap?

        [=============]

Here are some better variable names to consider:

intervalList -> intervals
hashSet -> overlaps
intervali -> leftInterval or lowerInterval
intervalj -> rightInterval or upperInterval

I find it's best to leave the type of collection out of the name. First, it makes it easier to change later. And second, you can see it and in any IDE hover over the method to see the required type. I might even drop the Interval suffix from the last two above since this method only deals with intervals.

Finally, there's no reason this method couldn't accept a null list. It should respond just as it does when the list is empty: return an empty set.

if (intervals == null || intervals.isEmpty()) {
    return Collections.<OverlapCoord>emptySet();
}


Custom array indexing Note that this code is not going to be used for anything, it's more of a thought experiment. I've been seeing a lot of NumPy code lately and noticed that they use a custom indexing process, presumably by defining an object's __getitem__ method. I wrote the following class that allows for a multi-dimensional array to be indexed as ls[1,2] instead of ls[1][2].

Is there a better way to write this method? It seems unnecessary to be keeping track of current as the item look-up is descended, but I can't think of a way to avoid it.

class Array(list):
    def __getitem__(self, indices):
        current = super(self.__class__, self)
        for index in indices:
            current = current.__getitem__(index)
        return current

a = Array([[[1,2,3],[4,5,6]],
           [[0,1,2],[9,8,7]]])
print a[1,0,2] # 2

Let me start by saying that there's nothing wrong with your approach, and almost nothing wrong with your implementation. Just two things I would like to remark.

Use of super.

Because of your use of super, you actively prevent subclassing. In Python 2.7, you're supposed to spell the name out long:

current = super(Array, self)

Consider this:

class Brray(Array): pass
b = Brray([[[1,2,3],[4,5,6]],
           [[0,1,2],[9,8,7]]])
print b[1, 0, 2]

This gives us

TypeError: 'int' object is not iterable

By using super(Array, self) instead, everything is fine again.

Subclassing list

You're subclassing list, which causes a = Array(...) to make a shallow copy of the supplied iterable. I myself would prefer composition:

class Array(object):
    def __init__(self, wrapped):
        self._wrapped = wrapped
    def __getitem__(self, indices):
        current = self._wrapped
        for index in indices:
            current = current[index]
        return current

(This is also aided by the fact that I don't like calling 'dunder' (double under) methods directly.


Getting SharePoint users and comparing them with SPList I have a custom SPList that has a field Assigned To, now I can add as many items as I want, and they can have same title and same User they are assigned to, but what I want is a script so that I only get the distinct number of user's email address, e.g. if a user is assigned to more then 1 item, I will get there email address just once, here is the code I wrote down but I don't think if its good enough.

using (SPSite site = new SPSite("www.local.com"))
    using (SPWeb web = site.OpenWeb())
    {
        SPList mySourceList = web.Lists["ListName"];

        SPQuery mySourceListQuery = new SPQuery();

        mySourceListQuery.Query =

            "<OrderBy><FieldRef Name='AssignedTo' />" +
                     "<FieldRef Name='Title' />" +
            "</OrderBy>";

        SPListItemCollection mySourceItemColl = mySourceList.GetItems(mySourceListQuery);

        foreach (SPUser user in web.Users)
        {
            foreach (SPListItem mySourceListItem in mySourceItemColl)
            {
                name = mySourceListItem["AssignedTo"].ToString();

                name = name.Substring(name.IndexOf("#") + 1);

                if (user.Name == name)
                {
                    Console.WriteLine(user.Email);
                }
            }
        }

The my prefixes are unnecessary, I'd call the variables sourceList, sourceListQuery, sourceItems etc.
I'd create a method for this:

name = name.Substring(name.IndexOf("#") + 1);

I guess removeId could be a good name. The method name would help readers to understand the intent of the writer.


Reading configuration variables in a Python project I am new to python, in .NET we use web.config to store environment specific variables... I want to achieve the same for my python program (scraper)
I have created .env file at the root of the project and added the following variable to it:
CHROME_WEB_DRIVER=C:/webdrivers/chromedriver.exe

Note that I have added .env to gitignore as this file changes in different environments.
And I am using decouple package to use this setting from .env file in the constructor of the class:
import scrapy
from decouple import config
from python_json_config import ConfigBuilder
from selenium import webdriver
from selenium.webdriver.chrome.options import Options

class MySpider(scrapy.Spider):
    name = 'my_spider'

    def __init__(self):
        # .env variables
        self.CHROME_WEB_DRIVER = config('CHROME_WEB_DRIVER')

        self.job_highlight_dict = {}
        self.wanted_organisations_dict = {}
        self.output_path = ""
        self.start_urls = []
        chrome_options = Options()
        chrome_options.add_argument('--headless')
        chrome_options.add_argument('--no-sandbox')
        chrome_options.add_argument('--disable-dev-shm-usage')
        self.driver = webdriver.Chrome(self.CHROME_WEB_DRIVER, chrome_options=chrome_options)

        builder = ConfigBuilder()
        config_values = builder.parse_config('config/config.json')
        self.start_urls = config_values.urls
        for o in config_values.organisations:
            self.wanted_organisations_dict[o["brandName"]] = o["organisationName"]
        self.output_path = config_values.output

    def start_requests(self):
        for url in self.start_urls:
            yield scrapy.Request(url=url, callback=self.parse)

    def parse(self, response):

        # parse response

Note that I also have a config.json file, but this file contains configuration variables which are consistent across all environments.
This is how the config.json file looks like:
{
    "urls": [
        "https://www.site-to-scrape-1.com",
        "https://www.site-to-scrape-2.com"
    ],
    "organisations": [
        { "brandName" : "brand-1", "organisationName" : "organisation-1" },
        { "brandName" : "brand-2", "organisationName" : "organisation-2" }
    ],
    "output": "../path/to/s3/bucket"
}

I have not included all of spider class, but it is the main class of this project which crawls the given urls and then parse them. I am not sure if the constructor of the spider is the best place to read config.json and .env files?

Add a declarative format for the config file, possibly. If you encounter errors, the output will look messy today (KeyError is never fun for an end user)
I would say init is a good time, but unclear if it's the right place. It's looking messy. I would either (1) split out parsing the config file into a helper method or (2) consolodate some lines. (1) is better if you expect to add more config.
I would consider making the chrome driver at the start of 'parse', as this is more of a "can fail" type operation--it will make writing tests easier.
Consolidating:

'builder' is used only once, don't declare it
don't assign to self.wanted_organizations_dict, or self.output_path, and self.start_urls twice. just initialize them once, correctly, in one line each.
self.job_highlight_dict is not initialized. i don't know what this is, but are you sure you want to declare it here?
self.CHROME_WEB_DRIVER does not appear to be used outside this, change it to a local variable or inline it, unless you move driver init to parse time
declare self.wanted_organisations_dict in a single-line dict comprehension, not 3 lines.
Initialize chrome_options in a single line, not 4.


Extracting a database name from a file I am developing a project that converts SQL server file to MySQL file based on some assumptions. I have written a function that takes the string source as input and it extracts the database name. Is there any way to improve the code?

public String getDatabaseNameFromMySqlServerFile(String source) {

    String sourceFile = null;
    String databaseName = "";
    Path file = Paths.get(source);
    try (InputStream in = Files.newInputStream(file);
            BufferedReader reader
            = new BufferedReader(new InputStreamReader(in))) {
        String line = null;
        while ((line = reader.readLine()) != null) {
            if (line.toLowerCase().contains("create database")) {
                String[] Arrline = line.split(" ");
                for (String s : Arrline) {
                    if (!s.toLowerCase().equals("create") && !s.toLowerCase().equals("database")) {
                        databaseName = s;
                        return s;
                    }
                }
            }
            sourceFile = sourceFile + line;
        }
    } catch (IOException x) {
        System.err.println(x);
    }

    return null;
}

databaseName is a pointless local variable: you can simply delete all lines using it.

The same goes for sourceFile.

Arrline and s are poorly named variables, and s is lowercased twice:

String[] Arrline = line.split(" ");
for (String s : Arrline) {
    if (!s.toLowerCase().equals("create") && !s.toLowerCase().equals("database")) {
        return s;
    }
}

Cleaning these up the code becomes:

String[] segments = line.split(" ");
for (String origSegment : segments) {
    String segment = origSegment.toLowerCase();
    if (!segment.equals("create") && !segment.equals("database")) {
        return origSegment;
    }
}

The current method of extracting the database name is quite awkward and inefficient. Look at these steps:

lowercase the line
check if it contains a string
split the line to tokens
for each token

if the lowercased token is neither "create" nor "database" then return it

You can do much better using regular expressions:

Pattern DBNAME_PATTERN = Pattern.compile("create +database +(\\w+)", Pattern.CASE_INSENSITIVE);

private String extractDatabaseName(String input) {
    Matcher matcher = DBNAME_PATTERN.matcher(input);
    if (matcher.find()) {
        return matcher.group(1);
    }
    return null;
}

The initialization of line is unnecessary,
because it gets assigned on the next line anyway:

String line = null;

So a simple declaration is enough:

String line;


Send commands over SSH to server Here's a very small and basic script that sends commands over SSH to another computer on my network:

#!/bin/python
import sys, os
commands = ""
for i in sys.argv[1:]:
        commands += " " + i;

if len(sys.argv) <= 1:
        os.system("sshpass -p VerySecrectPassword ssh pi@172.16.0.141")
else:
        os.system("sshpass -p VerySecrectPassword ssh pi@172.16.0.141" + commands)

Am I being very "Pythonic"? Is it better to use a for loop to join all the commands or the join function?

commands = " ".join(sys.argv[1:])

Personally, I think the for loop reads better, but that's just my opinion and I'm a newbie. Which is considered the better way to do things?

Furthermore:

Are there any other areas in this script that could be improved?
Is it a bad idea to store the password in the script (located at /usr/bin)?

Note: This script is stored on a computer where I am the only user (except root).

To answer your questions: Yes.

Your code could be boiled down to:

#!/bin/python
import sys
import os

os.system("sshpass -p VerySecrectPassword ssh pi@172.16.0.141" + " " .join(sys.argv[1:]))

Note that I also put the imports on two lines, as recommended by PEP8, Python's official style-guide.

But I would actually re-expand it to:

#!/bin/python
import sys
import os

data = {"user": "pi",
        "host": "172.16.0.141",
        "password": "VerySecret",
        "commands": " " .join(sys.argv[1:])}

command = "sshpass -p {password} ssh {user}@{host} {commands}"
os.system(command.format(**data))

This gives you some easier ways to modify the configuration. It would also be easier to implement reading this configuration from a file now.

Note that bash (or whatever shell you use) will not care about the trailing whitespace in case commands is the empty string.

The next step would be using a ssh library, like @Simon recommended in his comment.

One way to do it would be first setting up password-less login and then using this answer and a comment within it:

#!/bin/python
import sys
import paramiko

rpi = {"username": "pi",
       "hostname": "172.16.0.141"}
command = " " .join(sys.argv[1:])}

ssh = paramiko.SSHClient()
ssh.load_system_host_keys()
ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy())
ssh.connect(**rpi)
ssh_stdin, ssh_stdout, ssh_stderr = ssh.exec_command(command)

Here the ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy()) is needed because paramiko does not read the hosts file correctly.