Use if/else/elif conditionals to write a basic Rock Paper Scissors game I have the following code for the Rock, Paper, Scissor game. The code works fine in Python 2.7. Are there other more concise, more readable, or more pythonic ways of solving this problem in Python?

player1 = raw_input ("?")
player2 = raw_input ("?")

if (player1 == 'rock' and player2 == 'scissors'):
    print "Player 1 wins."

elif (player1 == 'rock' and player2 == 'rock'):
    print "Tie"

elif (player1 == 'scissors' and player2 == 'paper'):
    print "Player 1 wins."

elif (player2 == 'scissors' and player2 == 'scissors'):
    print "Tie"

elif (player1 == 'paper' and player2 == 'paper'):
    print "Tie"

elif (player1 == 'paper' and player2 == 'scissors'):
    print "Player 2 wins."

elif (player1 == 'rock'and player2 == 'paper'):
    print "Player 2 wins."

elif (player1 == 'paper' and player2 == 'rock'):
    print "Player 2 wins."

elif (player1 == 'scissors' and player2 == 'rock'):
    print "Player 2 wins."
else:
    print "This is not a valid object selection."

Here are some suggestions:

We have three if statements that end in "Tie". We could take all three conditions at once ((player1 == 'scissors' and player2 == 'scissors') or ...), or we could notice that we want the two players to have picked the same thing. So immediately we can simplify these three statements:

if (player1 == player2):
    print "Tie"

That's a lot shorter and simpler, and it doesn't depend on what choices are available. This would work just as well in a game with five or fifty options.
There are still six if statements left, for the cases where the two players pick different things. As suggested by Joel in the comments, a dictionary explaining the hierarchy between the different choices might be useful.

beats = {
    'scissors': 'rock',
    'rock': 'paper',
    'paper': 'scissors',
}

Note that I've done it the other way round to Joel: I think 'rock' = beats['scissors'] looks more natural, but I don’t think it makes too much of a difference.

Now we have a very natural way to describe the cases when the players don't pick the same thing:

if (player1 == player2):
    print "Tie"
elif (player1 == beats[player2]):
    print "Player 1 wins."
elif (player2 == beats[player1]):
    print "Player 2 wins."

Where once there were nine if statements, now there are just three, and this can be made to work for any variant on the game. By swapping out beats, we could play Rock-Paper-Scissors-Lizard-Spock, Bear-Hunter-Ninja, Bacon-Lettuce-Tomato, or anything else we wanted.

This is a good idea for programming in general: we've separated the game logic from the specific choices. Keeping them separate makes our program easier to write and to debug.
I like to wrap self-contained blocks of code (like this game logic) into their own functions.

def play_game(player1, player2, beats):
    """Returns the result of the game with player1 vs player2."""
    if (player1 == player2):
        return "Tie"
    elif (player1 == beats[player2]):
        return "Player 1 wins."
    elif (player2 == beats[player1]):
        return "Player 2 wins."

Now the top and bottom matter. This gets the players’s moves. By separating this from the game logic, we can work on this without worrying about whether we've broken the game.

To get the player's move, you've used:

player1 = raw_input("?")
player2 = raw_input("?")

This is the right way to do it, but the "?" is unnecessarily ambiguous. You should be clear what you want. To quote from The Zen of Python:

  Explicit is better than implicit.

So let’s write a full question here, so that it's clear what the choices mean:

player1 = raw_input("What is player 1's move? ")
player2 = raw_input("What is player 2's move? ")

To validate the input, you add an else block at the end of the code:

else:
    print "This is not a valid object selection."

You'd be better off validating the input when you receive it. Right now, the players have no way of telling which of them entered the wrong thing.

Here's one way you might do that:

player1 = raw_input("What is player 1's move? ")

while player1 not in ['rock', 'paper', 'scissors']:
    player1 = raw_input("That is not a valid object selection. Please try again. ")

Here we continue asking until we get something we like. But we can be more helpful. For example, you could tell the player what choices are available (you could even print this before the players make their initial choices).

While we're looking at this, we can use beats.keys() to get the list of possible choices, rather than typing them out every time. Here's an example:

print "The choices are " + str(beats.keys())

player1 = raw_input("What is player 1's move? ")
while player1 not in beats.keys():
    player1 = raw_input("I didn't get that. Please choose from " + str(beats.keys()))

As an additional point, you might want to do some sort of “fuzzy” matching on the input. The program needs to get the string "rock" precisely, but it’s pretty clear what somebody who types "Rock" or "rock\n" means. String methods like lower() and strip() might be useful here.
Finally, consider putting this code inside a special section if __name__ == '__main__'.

Any code in this section only gets used if the file is run directly (e.g., if somebody types $ python rockpaper.py at the command line). On the other hand, if you use import rockpaper in another script, it gets ignored. Right now, anybody who import's this script starts playing rock-paper-scissors (which they may not want).

There's more information on this around the web or on Stack Overflow.

So this is the sort of thing we might be left with:

def play_game(player1, player2, beats):
    """Returns the result of the game with player1 vs player2."""
    if (player1 == player2):
        return "Tie"
    elif (player1 == beats[player2]):
        return "Player 1 wins."
    elif (player2 == beats[player1]):
        return "Player 2 wins."

if __name__ == '__main__':
    beats = {
        'scissors': 'rock',
        'rock': 'paper',
        'paper': 'scissors',
    }

    print "The choices are " + str(beats.keys())

    player1 = raw_input("What is player 1's move? ")
    while player1 not in beats.keys():
        player1 = raw_input("I didn't get that. Please choose from " + str(beats.keys()))

    player2 = raw_input("What is player 2's move? ")
    while player2 not in beats.keys():
        player2 = raw_input("I didn't get that. Please choose from " + str(beats.keys()))

    print play_game(player1, player2, beats)

It looks more complicated than a stack of conditionals, but this is a lot more maintainable and portable than before.


HackerRank: Climbing the Leaderboard Below is my solution to Climbing the Leaderboard on HackerRank:

  Alice is playing an arcade game and wants to climb to the top of the
  leaderboard. Can you help her track her ranking as she beats each
  level? The game uses Dense Ranking, so its leaderboard works like
  this:

  The player with the highest score is ranked number 1 on the
  leaderboard.
  Players who have equal scores receive the same ranking
  number, and the next player(s) receive the immediately following
  ranking number.

  For example, four players have the scores 100, 90, 90, and 80.
  Those players will have ranks 1, 2, 2, and 3, respectively.

  When Alice starts playing, there are already \$n\$ people on the
  leaderboard. The score of each player \$i\$ is denoted by \$s_i\$. Alice plays for \$m\$
  levels, and we denote her total score after passing each level \$j\$ as \$alice_j\$.
  After completing each level, Alice wants to know her current rank.

  You are given an array, \$scores\$, of monotonically decreasing leaderboard
  scores, and another array, \$alice\$, of Alice's cumulative scores for each
  level of the game. You must print \$m\$ integers. The \$j^{th}\$ integer should
  indicate the current rank of alice after passing the \$j^{th}\$ level.

For example, when \$scores\$ is:

100 100 50 40 40 20 10

And \$alice\$ is:

5 25 50 120

The rankings for each score of Alice are 6, 4, 2, 1.

My code below works for most test cases,
except the last few where it times out.

function dedupe(arr, n) {
  const result = [];

  for(let i = 0; i < n; i++) {
    if(!result.includes(arr[i])) {
      result.push(arr[i]);
    }
  }

  return result;
}

const cache = {};

function binarySearchRankings(input, keySearch, min, max) {
  if(keySearch > input[0]) {
    return 1;
  }

  if(cache[keySearch]) {
    return cache[keySearch];
  }

  let mid = 0;

  while(min <= max) {

    mid = Math.ceil((min + max) / 2);

    if(keySearch === input[mid]) {
      const result = ++mid;
      cache[keySearch] = result;
      return result;
    } else if(keySearch >= input[mid]) {
      max = mid - 1;
    } else {
      min = mid + 1;
    }
  }

  if(keySearch <= input[mid]) {
    cache[keySearch] = mid + 2;
    return mid + 2;
  }

  if(keySearch > input[mid]) {
    cache[keySearch] = mid + 1;
    return mid + 1;
  }

  return mid;
}

const deduped = dedupe(scores, n);

for(var i = 0; i < m; i++) {
  const a = binarySearchRankings(deduped, alice[i], 0, deduped.length -1);

  console.log(a);
}

Performance

It's the dedupe implementation that drags the performance of your solution down.
It's because Array.prototype.includes() does a linear search,
so your implementation becomes \$O(n^2)\$.
If you rewrite this function to be \$O(n)\$,
the solution will pass all tests.

Using arrays

The dedupe function takes as parameters an array and its size.
The size parameter is unnecessary and potentially confusing,
because you can always access that with .length.

Binary search

A common practice when implementing binary search over a list is to return the index of a value if it is found,
otherwise return a negative value from which the insertion point can be computed using the formula -1 -ret.
It would be cleaner to implement binarySearch(arr, value, low, high) in
a way to follow this common contract,
and another rank function that would use the result of binarySearch to compute the correct rank.

Also, the implementation is a bit complicated.
Simpler techniques exist, and the cache is not needed either.
Consider this, for example:

function binarySearch(arr, value, lowIndex, highIndex) {
    if (lowIndex > highIndex) {
        return -(lowIndex + 1);
    }

    let midIndex = lowIndex + Math.trunc((highIndex - lowIndex) / 2);
    if (value == arr[midIndex]) {
        return midIndex;
    }
    if (value > arr[midIndex]) {
        return binarySearch(arr, value, lowIndex, midIndex - 1);
    }
    return binarySearch(arr, value, midIndex + 1, highIndex);
}


python recursive regex optimization I parse a big source code directory (100k files). I traverse every line in every file and look for function calls via regex matching.

I know that using regex to parse languages is a terrible idea. However, I'm looking for simplicity and not interested in the whole parse tree. Moreover, the regex pattern is generic and applies to all languages with the func(arg) syntax.

Following some profiling that I did, my hotspot is of course the function that responsible of processing a single line and search matches in that line. In big code projects, this function is called about 25 million times.
I was wondering weather there is room for optimization in the following 9 lines, or is this as good as it gets:

def get_line_symbols(self, line):
    symbols = []
    matches = self.index_pattern.finditer(line)
    for match in matches:
        while match:
            symbols.append(match.group(1))
            symbol_args = match.group(2)
            match = self.index_pattern.search(symbol_args)
    return symbols

self.index_pattern is re.compile('(\w+)(\([^\)]*\)?)')

I can improve the performance by 25% or so, by stopping the regular expression after the opening parenthesis, and so avoiding the need for a while loop or indeed a function at all:

get_line_symbols = re.compile(r'(\w+)\(').findall

(Note that it's a good idea to use the r modifier on regular expression strings, to avoid the risk of accidentally writing something like '\b', which needs to be written as r'\b' or '\\b' if it's to mean a word boundary rather than the backspace character.)

I get a further speedup by switching from the built-in re library to the third-party regex library. Using both of these improvements is about three times as fast as the code in the post.


Mocking IElasticClient in unit tests I'm trying to unit test how my class responds to a NEST 1.7 IElasticClient's return values, using NUnit 2.6.4 and Moq 4.2. I feel mocking the Get function requires very "heavy" It.IsAny calls, and am mainly looking for feedback on how to mock IElasticClient in a way that my tests remain readable.

My code is mostly 100% actual code, with the exception of:

The Repository and Person classes are simplified (I'm mainly looking for feedback on the Test method);
I'm expecting a generic Exception but in reality I've got a custom exception class there;

Here's the code:

public class Person
{
    public string Id { get; set; }
    public string FullName { get; set; }
}

public class PeopleRepository
{
    private IElasticClient client;

    public PeopleRepository(IElasticClient Client)
    {
        this.client = Client;
    }

    public Person Get(string id)
    {
        var getResponse = client.Get<Person>(p => p.Id(id));

        if (getResponse.Source == null)
        {
            throw new Exception("Person was not found for id: " + id);
        }

        return getResponse.Source;
    }
}

[TestFixture]
public class PeopleRepositoryTests
{
    [Test]
    public void RetrieveProduct_WhenDocNotFoundInElastic_ThrowsException()
    {
        var clientMock = new Mock<IElasticClient>();
        var getRetvalMock = new Mock<IGetResponse<Person>>();

        getRetvalMock
            .Setup(r => r.Source)
            .Returns((Person)null);

        clientMock
            .Setup(c => c.Get<Person>(It.IsAny<Func<GetDescriptor<Person>, GetDescriptor<Person>>>()))
            .Returns(getRetvalMock.Object);

        var repo = new PeopleRepository(clientMock.Object);

        Assert.Throws<Exception>(() => repo.Get("invalid-id"));
    }
}

Any way I can improve the way I'm mocking IElasticClient?

It's fine. Yes, in an ideal world, we'd verify that the id we passed into the PeopleRepository.Get() method is the same one that's getting passed to the client.Get() method, but that's also getting down into testing the internal implementation of the method. What you really care about here is that when the Person isn't found, a specific exception is thrown, so in this particular case it's fine.

Remember, we want to tests for specific results, not the internal implementation.

I'm going to quote Jon Skeet's comment to a related SO question.

  I would test that the usage of those functions do the right thing, rather than testing that the exact same function is used. – Jon Skeet Oct 8 '12 at 9:33


Use of concurrency to generate combinations I have the following package to generate combinations from a slice of bytes:

//Package combinations generates combinations from a slice.
package combinations

//CombIter generates combinations of bytes of given length from a
//list of elements. It returns values by a channel outC
//An empty list indicates that no further combinations are available
func CombIter(elements []byte, length int, outC chan []byte) {
    for i := 0; i < len(elements); i++ {
        if length == 1 {
            outC <- []byte{elements[i]}
        } else {
            inC := make(chan []byte)
            go combIter(elements[i+1:], length-1, inC)

            for next := <-inC; len(next) != 0; next = <-inC {
                outC<- append([]byte{elements[i]}, next...)
            }
        }
    }
    outC <- []byte{}
}

//Comb collects combinations of length k from a byte slice l.
//It returns a slice containing (len(l) C k) slices all of length k
func Comb(l []byte, k int) [][]byte {
    c := make(chan []byte)
    go combIter(l, k, c)
    o := [][]byte{}
    for next := <-c; len(next) != 0; {
        o = append(o, next)
        next = <-c
    }
    return o
}

I test it with

func TestComb(t *testing.T)
    actual := Comb([]byte{'a', 'b', 'c', 'd'}, 2)
    expected := [][]byte{{'a', 'b'}, {'a', 'c'}, {'a', 'd'}, {'b', 'c'}, {'b', 'd'}, {'c', 'd'}}
    if !reflect.DeepEqual(actual, expected) {
        t.Errorf("Got  %v\nWant %v", actual, expected)
    }
}

The algorithm is an adaption of a python program: https://stackoverflow.com/a/2837693/2709093

I am particularly concerned with the use of concurrency, and the possibility of creating a disruptive number of goroutines. Is this a useful application of concurrency?

Go routines are cheap to make and run. So your solution would work perfectly fine.

To your question whether it's actually a useful application of concurrency I would say no.
You're creating channels and go routines to then instantly wait for it's next result anyway.

Right below your linked python answer to that other question there's a nice java answer with a slightly different approach. I suggest combining the 2 approaches.

I'm not used to writing go code so the following will probably need a couple of fixes but should give you an idea how to do it correctly yourself:

func CombIter(input []byte, acc []byte, length int, outC chan []byte) {
    for i := 0; i < len(elements)-length+1; i++ {
        if length == 1 {
            outC <- append([]byte{acc, input[i]})
        } else {
            for j:= i+1; j < len(elements); j++ {
                combIter(input[j:], append([]byte{acc, input[j]}), length-1, inC)
            }
        }
    }
}

You might also have to check the stopping condition for the outer while loop. You want this one to stop when there aren't enough elements left to get a result of the correct length. I didn't run this code so it could be off by 1.

If you need that outC <- []byte{} line to have a stopping condition it may make most sense to provide a helper method:

func CombIter(input []byte, length int, outC chan []byte) {
    combIter(input, []byte{}, length, outC);
    outC <- []byte{};
}


Optimal string literal tokenizing algorithm I have a tokenizer, where one of the token types happens to be a C# string type. It takes a TextReader as its input, which is created from a StringReader, and it needs to support the basic stuff C# strings supports, such as "jo dude", "joe \"dudes\"", "this is a backslash \\ token" and of course "this is carriage return \r\nthis is next line"

Currently my method looks like this;

private string ReadSingleLineStringLiteral ()
{
    StringBuilder buffer = new StringBuilder ();
    int nextChar = _reader.Read ();
    while (nextChar != -1) {
        buffer.Append ((char)nextChar);
        if (nextChar == '"' &&
            (buffer.Length == 1 ||
                 buffer [buffer.Length - 1] != '\\' ||
                 (buffer.Length - buffer.ToString ().TrimEnd ('\\').Length) % 2 == 0)) {
            break;
        }
        nextChar = _reader.Read ();
    }
    if (buffer [buffer.Length - 1] != '"')
        throw new ArgumentException ("unclosed string literal in hyperlisp file");
    return buffer.ToString ().Substring (0, buffer.Length - 1)
        .Replace ("\n", "\r\n") // normalizing carriage returns
        .Replace ("\r\r\n", "\r\n")
        .Replace ("\\\"", "\"")
        .Replace ("\\\\", "\\");
}

It works I think, however, I suspect this is far from optimal, among other things, I hate that buffer.ToString ().TrimEnd ('\\').Length) % 2 part of it.

The _reader is the TextReader object, that at the entry of my methods are at the first character within the content of my string literal.

Can anyone figure out the optimal solution for parsing a C# type of string literal, and optimize my method?

Please don't tell me to use some library, since I don't want to bring in the overhead of any libraries to perform a task I think should be easily possible to implement with some 10-20 lines of code.

For bonus points, I'd love to also have a solution to my "multiline string literal algo" too. This method should read string the same way C# reads them when given @"some string". Currently it looks like this:

private string ReadMultiLineStringLiteral ()
{
    StringBuilder buffer = new StringBuilder ();
    int nextChar = _reader.Read ();
    while (nextChar != -1) {
        buffer.Append ((char)nextChar);
        nextChar = _reader.Peek ();
        if (nextChar != '"' && (buffer.Length - buffer.ToString ().TrimEnd ('"').Length) % 2 == 1)
            break;
        nextChar = _reader.Read ();
    }
    if (buffer.Length == 0 || buffer [buffer.Length - 1] != '"')
        throw new ArgumentException ("unclosed multiline string literal in hyperlisp close to end of hyperlisp");
    return buffer.ToString ().Substring (0, buffer.Length - 1)
        .Replace (@"""""", @"""")
        .Replace ("\n", "\r\n") // normalizing carriage returns
        .Replace ("\r\r\n", "\r\n");
}

Both methods starts on the first character of the actual string content, e.g. for string @"xyz" the multiline method has the TextReader reader position being at "x", for string "abc" the singleline string literal method has the reader position being at "a".

This is probably unnecessary to say, but obviously Regex is a no-no.

A rule of thumb for performance in C# is to avoid allocations. In this case, we should try to remove buffer.ToString ().TrimEnd ('\\'), as well as the calls to Substring and Replace that appear at the end.

Of course you need to set performance goals and profile the code yourself on realistic input.  Single line strings can reasonably be expected to be quite short (say 200 chars?), so what you have already might be performant enough, but I'll illustrate an alternative way.

First let's make this a static method so it will be easier to unit test. As I mentioned in the comments, there's a bug in the code -- unit testing can help you catch such bugs.

private static string ReadSingleLineStringLiteral(TextReader reader)

And a convenience method for testing

private static string ReadSingleLineStringLiteral(string line)
{
    using (var reader = new StringReader(line))
    {
        return ReadSingleLineStringLiteral(reader);
    }
}

Now let's just handle the simple case, where there are no escape characters. Let's write some unit tests

Assert.AreEqual("", ReadSingleLineStringLiteral("\""));
Assert.AreEqual("jo dude", ReadSingleLineStringLiteral("jo dude\""));
Assert.Throws<ArgumentException>(() => ReadSingleLineStringLiteral(""));

And write the simplest thing that gets them to pass

var sb = new StringBuilder();
for (var c = reader.Read(); c != -1; c = reader.Read())
{
    switch (c)
    {
    case '"':
        return sb.ToString();
    default:
        sb.Append((char)c);
        break;
    }
}

throw new ArgumentException("Unexpected end of input");

Single line strings shouldn't contain new lines, so let's add a new test case

Assert.Throws<ArgumentException>(() => ReadSingleLineStringLiteral("\n\""));

And a matching case to our method

    switch (c)
    {
    case '"':
        return sb.ToString();
    case '\n':
        throw new ArgumentException("Single line string contains new line");
    default:
        sb.Append((char)c);
        break;
    }

Now let's move on to escape sequences.

Assert.AreEqual("jo \"dude\"", ReadSingleLineStringLiteral("jo \\\"dude\\\"\""));
Assert.AreEqual("this is a backslash \\ token", ReadSingleLineStringLiteral("this is a backslash \\\\ token\""));
Assert.AreEqual("this is a carriage return \r\nthis is next line", ReadSingleLineStringLiteral("this is a carriage return \\r\\nthis is next line\""));

We add another case

    switch (c)
    {
    case '"':
        return sb.ToString();
    case '\\':
        AppendEscapeCharacter(reader, sb);
        break;
    case '\n':
        throw new ArgumentException("Single line string contains new line");
    default:
        sb.Append((char)c);
        break;
    }

And the corresponding method

private static void AppendEscapeCharacter(TextReader reader, StringBuilder sb)
{
    var c = reader.Read();
    switch (c)
    {
    case -1:
        throw new ArgumentException("Unexpected end of input");
    case '"':
    case '\'':
    case '\\':
        sb.Append((char)c);
        break;
    case 'n':
        sb.Append('\n');
        break;
    case 'r':
        sb.Append('\r');
        break;
    default:
        throw new ArgumentException(string.Format("Invalid escape sequence '\\{0}'", (char)c));
    }
}

There are more cases to handle, but this should be enough to get you started.


Visitor Pattern in C++ 11 I have referred some class diagram to actually create visitor.

#include <iostream>

using namespace std; //know should not be used, but for simplicity

class Book;
class Pen;
class Computer;

class ICartVisitor
{
public:
    virtual int Visit(Book &) = 0;
    virtual int Visit(Pen &) = 0;
    virtual int Visit(Computer &) = 0;
};

class IElement
{
public:

    virtual int accept (ICartVisitor& cartVisitor) = 0;
    virtual int getPrice () = 0;

};

class Book : public IElement
{

public:

    int accept (ICartVisitor& cartVisitor)
    {
        return cartVisitor.Visit(*this);
    }

    int getPrice ()
    {
        return 100;
    }
};

class Pen : public IElement
{
public:

    int accept (ICartVisitor& cartVisitor)
    {
        return cartVisitor.Visit(*this);

    }

    int getPrice ()
    {
        return 5;
    }
};

class Computer : public IElement
{
public:

    int accept (ICartVisitor& cartVisitor)
    {
        return cartVisitor.Visit(*this);

    }

    int getPrice ()
    {
        return 10000;
    }
};

class CartVisitor : public ICartVisitor
{
public:

    //inlining all the functions for simplicity
    int Visit(Book & book)
    {
        return book.getPrice();
    }

    int Visit(Pen & pen)
    {
        return pen.getPrice();

    }

    int Visit(Computer & computer)
    {
        return computer.getPrice();

    }
};

int main(int argc, char* argv[])
{
    int total = 0;

    ICartVisitor *cartVisitor = new CartVisitor();

    total += cartVisitor->Visit(*(new Computer()));
    total += cartVisitor->Visit(*(new Book()));
    total += cartVisitor->Visit(*(new Pen()));

    cout<<total;

    return 0;
}

First: you leak memory. Terribly. Never use new unless you know what you are doing. Use smart pointers, or in this case just objects on the stack.

The visitor pattern is meant for separating algorithms from an object structure. There is no object structure / composition / hierarchy in your code, so there is no use for the visitor pattern.

Following your example, there should be a Cart class with an accept method, that iteratively calls accept for all elements in the cart.

Also the visitor pattern is supposed to prepare for unknown algorithms operating on data structures that should not know about the algorithms. So it does not make sense to use a return value here. If you want to accumulate a prize here, you would have to do it in the state of the visitor.

Also: Use consistent spacing between type& variable etc. Use consistent capitalization of methods (visit). Avoid redundant newlines.


Simplified error handling when reading from stdin and parsing to a type I've recently started learning Rust, and I've managed to whip up this function, which reads and parses a thing from stdin. I don't like it though, and I feel it could be further simplified. What do you think?

fn read_thing<T: str::FromStr>() -> Result<T, &'static str> {
    let mut buffer = String::new();
    match io::stdin().read_line(&mut buffer) {
        Ok(_)  => match buffer.trim().parse() {
            Ok(n)  => Ok(n),
            Err(_) => Err("Failed parsing input.")
        },
        Err(_) => Err("Failed reading from stdin."),
    }
}

Here's a few possibilities. They all rely on the fact that Result can be mapped over, both in the success and failure cases.

Additionally, I personally detest having trait bounds on generics inside the generic declaration. I almost always move them to a where clause for readability.

fn read_thing<T>() -> Result<T, &'static str>
    where T: str::FromStr
{
    let mut buffer = String::new();
    try!(io::stdin().read_line(&mut buffer).map_err(|_| "Failed reading from stdin."));
    buffer.trim().parse().map_err(|_| "Failed parsing input.")

This is the most direct modification to your existing code. It transforms the errors into the desired strings and uses the try! macro to return immediately on the first error.

use std::io::BufRead;

fn read_thing<T>() -> Result<T, &'static str>
    where T: str::FromStr
{
    let stdin = io::stdin();
    let v = stdin.lock().lines().next()
        .ok_or("No data to read")
        .and_then(|s| s.map_err(|_| "Failed reading from stdin"))
        .and_then(|s| s.trim().parse().map_err(|_| "Failed parsing input"));
    v
}

This was an attempt to chain it all together. It almost works nicely, but introduces a new error condition - what if stdin is fine, there's just nothing to read? Add to that the lifetime shenanigans that involve locking the standard in, and this isn't the nicest either.

fn one_line_from_stdin() -> io::Result<String> {
    let mut buffer = String::new();
    io::stdin().read_line(&mut buffer).map(|_| buffer)
}

fn read_thing2<T>() -> Result<T, &'static str>
    where T: str::FromStr
{
    one_line_from_stdin()
        .map_err(|_| "Failed reading from stdin")
        .and_then(|s| s.trim().parse().map_err(|_| "Failed parsing input"))
}

Continuing on a quest to chain it together, I extracted a method that just returns a single string. It's probably not very efficient, but reads OK.

fn read_thing2<T>() -> Result<T, &'static str>
    where T: str::FromStr
{
    let mut buffer = String::new();
    io::stdin().read_line(&mut buffer).map(|_| buffer)
        .map_err(|_| "Failed reading from stdin")
        .and_then(|s| s.trim().parse().map_err(|_| "Failed parsing input"))

Same idea as the previous code, this just inlines the input method.

use std::error::Error;

fn read_thing<T>() -> Result<T, Box<Error>>
    where T: str::FromStr,
          T::Err: Error + 'static,
{
    let mut buffer = String::new();
    try!(io::stdin().read_line(&mut buffer));
    let v = try!(buffer.trim().parse());
    Ok(v)
}

This is what I'd probably write to start with. I find the messages from most errors to be fairly readable and understandable, but I'm not showing them to an end user most of the time. I'd just use a little elegance baked into the try! macro and the From implementation of Box<Error> to pass back the original errors, abstracted out to the generic concept of an error. More information can be found in The Rust Programming Language.

If I cared more about custom messages or efficiency, I'd upgrade to a custom error type. I've also recently discovered the quick_error crate which provides a macro to make custom types easier, but haven't had a chance to try it out yet.


Another Brainfuck interpreter in Haskell I came up with the following Brainfuck interpreter in Haskell after thinking about how to represent the program and the memory functionally using zippers to represent current location. This works since in Brainfuck, there is no way to reference more than one memory location away from your present location at one time.

import Control.Monad
import System.Environment

data ZL a = EM | ZL [a] a [a]

fromList []           = EM
fromList (x:xs)       = ZL [] x xs
zprv EM               = EM
zprv (ZL [] x rs)     = EM
zprv (ZL (l:ls) x rs) = ZL ls l (x:rs)
znxt EM               = EM
znxt (ZL ls x [])     = EM
znxt (ZL ls x (r:rs)) = ZL (x:ls) r rs
zmap (ZL ls x rs) f   = ZL ls (f x) rs

run code = go (fromList (repeat 0)) (fromList code)
  where
    go m EM                          = return m
    go m              i@(ZL _ '>' _) = go (znxt m) (znxt i)
    go m              i@(ZL _ '<' _) = go (zprv m) (znxt i)
    go m              i@(ZL _ '+' _) = go (zmap m (\x -> x + 1)) (znxt i)
    go m              i@(ZL _ '-' _) = go (zmap m (\x -> x - 1)) (znxt i)
    go m@(ZL ml x mr) i@(ZL _ '[' _) = go m (if x /= 0 then znxt i else skip znxt 0 i)
    go m@(ZL ml x mr) i@(ZL _ ']' _) = go m (if x == 0 then znxt i else skip zprv 0 i)
    go m@(ZL  _ x  _) i@(ZL _ '.' _) = putChar (toEnum x) >> go m (znxt i)
    go m@(ZL ml _ mr) i@(ZL _ ',' _) = getChar >>= \x -> go (ZL ml (fromEnum x) mr) (znxt i)
    go m@(ZL ml x mr) i@(ZL _   _ _) = go m (znxt i) -- Ignore
    skip _  1 i@(ZL _ ']' _)         = znxt i
    skip _ (-1) i@(ZL _ '[' _)       = znxt i
    skip d  n i@(ZL l '[' r)         = skip d (n+1) (d i)
    skip d  n i@(ZL l ']' r)         = skip d (n-1) (d i)
    skip d  n i@(ZL l   _ r)         = skip d n (d i)

main = do
  getArgs >>= \args -> case args of
    []    -> putStrLn "Usage: bf <program.bf>"
    (x:_) -> readFile x >>= run >> return ()

I find this very difficult to read without any logical line breaks or function types given. I.e., you should leave a blank line between definitions for different functions, and every top-level definition should be given a type signature.

fromList :: [a] -> ZL a
fromList []     = EM
fromList (x:xs) = ZL [] x xs

zprv :: ZL a -> ZL a
zprv EM               = EM
zprv (ZL []     x rs) = EM
zprv (ZL (l:ls) x rs) = ZL ls l (x:rs)

-- &c...

zmap is a partial function, and it isn't acting much like a mapping as it only modifies the current element in your given definition. Maybe zapp is a better name? (Or poke, or prod, or...)

zapp :: (a -> a) -> ZL a -> ZL a
zapp _ EM           = EM             -- Or, raise a descriptive `error`
zapp f (ZL ls x rs) = ZL ls (f x) rs

The definition of run is extremely long and not very semantic, rewrite it to be the composition of smaller, more compositional, more meaningful functions.

zinc :: (Num a) => ZL a -> ZL a
zinc = zapp (+1)

zdec :: (Num a) => ZL a -> ZL a
zdec = zapp (subtract 1)

zjmp :: (ZL a -> ZL a) -> Int -> ZL a -> ZL a
zjmp _   1  i@(ZL _ ']' _) = znxt i
zjmp _ (-1) i@(ZL _ '[' _) = znxt i
zjmp d   n  i@(ZL l '[' r) = zjmp d (n+1) (d i)
zjmp d   n  i@(ZL l ']' r) = zjmp d (n-1) (d i)
zjmp d   n  i@(ZL l   _ r) = zjmp d n     (d i)

interpret :: ZL Int -> ZL Char -> IO ()
interpret _                 EM          = return ()
interpret m              i@(ZL _ '>' _) = interpret (znxt m) (znxt i)
interpret m              i@(ZL _ '<' _) = interpret (zprv m) (znxt i)
interpret m              i@(ZL _ '+' _) = interpret (zinc m) (znxt i)
interpret m              i@(ZL _ '-' _) = interpret (zdec m) (znxt i)
interpret m@(ZL _  x _ ) i@(ZL _ '[' _) = interpret m (if x /= 0 then znxt i else zjmp znxt 0 i)
interpret m@(ZL _  x _ ) i@(ZL _ ']' _) = interpret m (if x == 0 then znxt i else zjmp zprv 0 i)
interpret m@(ZL _  x _ ) i@(ZL _ '.' _) = putChar (toEnum x) >> interpret m (znxt i)
interpret m@(ZL ml _ mr) i@(ZL _ ',' _) = getChar >>= \x -> interpret (ZL ml (fromEnum x) mr) (znxt i)
interpret m              i@(ZL _ _   _) = interpret m (znxt i) -- Comment `Char`

run :: String -> IO ()
run program = interpret tape (fromList program)
    where
        tape :: ZL Int
        tape = fromList (repeat 0)

Your usage of do is redundant in main. You can either just drop the word do, or write it more idiomatically as—

main :: IO ()
main = do
          args <- getArgs
          case args of
              [x] -> do
                        program <- readFile x
                        run program
              _   -> usage
    where
        usage = putStrLn "Usage: bf FILE"


Finding root of strictly monotonic function by bisection I need to find a root of strictly monotonic function f (only raising or only lowering). There is only a one root of a function.
If given range from a to b doesn't contain root (fabs(f(a)*f(b) is not lower or equal than 0), we need to change range, since the assumption is that there is a one root, so we need to move on x-axis to left or right and check again, if f(a).
If it is, then (f)*f(b) (e.g (-3)*2=-6, and that means there is a root between a and b).

I've made an solution and I would like to improve performance in terms of changing ranges (obviously adding +2 is not good for huge functions) vs halving ranges further, when finding f(c).

What would be the best strategy?
Also will there be any other problem to look on?
What can I do to avoid user epsilon to be smaller than DBL_EPSILON?
Any ideas to improve my code?

#include <iostream>
#include <cmath>

double f(double x)
{
    //double y = std::pow(2, x) - 6;
    double y = x - 10000;
    return y;
}

bool isRaising(double a, double b)
{
    if (f(a) > f(b)) return false;
    return true;
}

bool areBothNumbersPositive(double a, double b)
{
    if (f(a) > 0 && f(b) > 0) return true;
    return false;
}

void moveLeft(double &a, double &b)
{
    b = a;
    a = a - 2;
}

void moveRight(double& a, double& b)
{
    a = b;
    b = b + 2;
}

void changeRanges(double &a, double &b)
{
    if (!isRaising(a, b))
    {
        if (areBothNumbersPositive(a, b))
        {
            moveRight(a, b);
        }
        else
        {
            moveLeft(a, b);
        }
    }
    else
    {
        if (areBothNumbersPositive(a, b))
        {
            moveLeft(a, b);
        }
        else
        {
            moveRight(a, b);
        }
    }
}

double findRoot(double a, double b, double epsilon)
{
    if (epsilon < DBL_EPSILON) epsilon = DBL_EPSILON;
    if (a == b) throw std::exception("Range must be higher than zero!");

    double c;
    if (std::fabs(f(a)) < epsilon) return a;
    if (std::fabs(f(b)) < epsilon) return b;

    while (f(a) * f(b) > 0)
    {
        changeRanges(a, b);
    }
    if (std::fabs(f(a)) < epsilon) return a;
    if (std::fabs(f(b)) < epsilon) return b;
    do
    {
        c = (a + b) / 2;
        if (f(a) * f(c) > 0) a = c;
        else b = c;
    } while (!(std::fabs(f(c)) < epsilon));
    return c;
}

int main()
{
    //f(a) * f(b) > 0
    std::cout << findRoot(-100,  -300,   0.00000000001) << std::endl;
    std::cout << findRoot(100, 300, 0.00000000001) << std::endl;
    std::cout << findRoot(-300, -100,  0.00000000001) << std::endl;
    std::cout << findRoot(300, 100,  0.00000000001) << std::endl;

    //f(a) * f(b) <= 0
    std::cout << findRoot(-100, 300, 0.00000000001) << std::endl;
    std::cout << findRoot(100, -300, 0.00000000001) << std::endl;
    std::cout << findRoot(300, -100, 0.00000000001) << std::endl;
    std::cout << findRoot(-300, 100, 0.00000000001) << std::endl;
}

1. Use established terms to make your code more readable to others

In this case, use "increasing" instead of "raising". In such a simple context it is quite obvious what you mean by "raising", but you would be surprised how much effort and how many misunderstandings you can save by using existing, well-known terms.

2. Give clear, unambiguous names to functions

When I see a function areBothNumbersPositive(double a, double b), I assume it returns a > 0 && b > 0. A more suitable name would be areBothFuncValuesPositive(double a, double b), even though it's a bit longer.

3. Don't return true or false based on the value of a boolean expression, return the expression directly

bool isIncreasing(double a, double b)
{
    return f(a) < f(b);
}

bool areBothFuncValuesPositive(double a, double b)
{
    return f(a) > 0 && f(b) > 0;
}

4. Duplicate code in chageRanges can be reduced

void changeRanges(double &a, double &b)
{
    if (isIncreasing(a, b) == areBothFuncValuesPositive(a, b))
    {
        moveLeft(a, b);
    }
    else
    {
        moveRight(a, b);
    }
}

Although I must admit, this seems a bit confusing. Maybe it would be a good idea to define a function which would figure out whether to go left or right, and then use this function in the if. This might be an overkill in this simple case, but is quite commonly used with more complex boolean expressions.

bool shouldMoveLeft(double a, double b)
{
    return (isIncreasing(a, b) && areBothFuncValuesPositive(a, b))
         ||(isDecreasing(a, b) && areBothFuncValuesNegative(a, b));
}

void changeRanges(double &a, double &b)
{
    if (shouldMoveLeft(a, b))
    {
        moveLeft(a, b);
    }
    else
    {
        moveRight(a, b);
    }
}

5. Use functions where appropriate

Functions serve two main purposes: 1) code reuse; 2) conceptual abstraction. Even if you don't reuse them, they can make code more readable. For example, when you calculate f(a) * f(b) > 0, what you really want to know is whether the function has the same sign at both values, and neither f(a) nor f(b) are zero.

bool areBothFuncValuesNonzeroSameSign(double a, double b)
{
    return f(a) * f(b) > 0;
}

An even better idea would be to introduce the concept of a sign, and then build upon it (I copied the sign function from this SO answer).

template <typename T> int sign(T val) {
    return (T(0) < val) - (val < T(0));
}

bool areBothNumbersSameSign(double x, double y)
{
    return sign(x) == sign(y);
}

bool areBothNumbersNonzero(double x, double y)
{
    return x != 0 && y != 0;
}

bool areBothFuncValuesNonzeroSameSign(double a, double b)
{
    const double
        fa = f(a),
        fb = f(b);

    return areBothNumbersNonzero(fa, fb) && areBothNumbersSameSign(fa, fb);
}

This is not as succinct and elegantly clever as your original code. But imagine it wasn't a single line, but an entire function / class / project written in such a clever way. Then you would likely have a bit of trouble understanding it when you come back to it after some time. It would be even more trouble for someone else who has never seen your code before and now has to understand it, because you don't work at the company anymore and they inherited your code, or because they are extending your project, using your library, etc. By keeping your code simple and "stupid", you make it more readable and accessible to more people, and therefore, more maintainable.

Another two important aspects of software, besides readability and maintainability, are composability and reusability. Like readability and maintainability, these two are related. The functions sign, areBothNumbersSameSign and areBothNumbersNonzero are quite generic, so they are easily combined together (composed). This improves their reusability. Make your functions as generic as possible, so they can be reused later in different contexts, and composed to form larger abstractions. This includes the use of generic types (templates), as in the sign function. Making the function closely tailored for one specific use case prevents it from being used in other cases. You can observe that the function areBothFuncValuesNonzeroSameSign can only be used in the context of the function f, since it depends on it. We can decompose its functionality into two logically distinct parts: 1) calculating the values f(a), f(b); and 2) checking if the computed values are both non-zero and have the same sign.

bool areBothNumbersNonzeroSameSign(double x, double y)
{
    return areBothNumbersNonzero(x, y) && areBothNumbersSameSign(x, y);
}

bool areBothFuncValuesNonzeroSameSign(double a, double b)
{
    return areBothNumbersNonzeroSameSign(f(a), f(b));
}

Again, this is just a simple example. But as the code grows bigger and the functionality gets more complex, such refactoring is necessary, although very often neglected.

6. Changing range in moveLeft and moveRight

As you pointed out, it is not ideal to use a constant (2) when adjusting the range. The first naive idea that would come to my mind is to simply shift the range, without changing its size.

void moveLeft(double &a, double &b)
{
    const double diff = b - a;
    b = a;
    a = a - diff;
}

void moveRight(double& a, double& b)
{
    const double diff = b - a;
    a = b;
    b = b + diff;
}

But, as you said, this is not very efficient. What you can do is increase the size of the range exponentially. I think (but I'm not sure), that this would converge more quickly on average.

void moveLeft(double &a, double &b)
{
    const double diff = b - a;
    b = a;
    a = a - diff*2;
}

void moveRight(double& a, double& b)
{
    const double diff = b - a;
    a = b;
    b = b + diff*2;
}

Note that I have not tested any of this, so I can't guarantee it works.


Inputting and sorting three integers The problem to the question is:

  Write a program that takes 3 integers as an input from the user using input dialog messages and sorts the three numbers.

I just want to know: is there any major differences between my code and the book's code in terms of efficiency or anything specific that I could have done better in my code? I am slowly learning Java and am trying to pick up the best techniques as I go along trying to learn this language. Also, I really don't understand the logic behind the book's answer. It seems as if they are only comparing two numbers, but how can they guess what the third number will be in the sort?

Note: I am following my textbook and the newest thing we have learned are if and else statements along with switch statements. I have not learned any looping and I am still in the basics. I am following Introduction To Java Programming 9th edition Daniel Liang and this is from the Chapter 3 exercises.

My solution:

package Chapter3Exercises;
import javax.swing.JOptionPane;

public class SortThreeIntegers
{
    public static void main(String[] args)
    {
        //Prompt user to enter three integers

        String stringNum1 = JOptionPane.showInputDialog(null, "Please enter 1st Integer: ");
        String stringNum2 = JOptionPane.showInputDialog(null, "Please enter 2nd Integer: ");
        String stringNum3 = JOptionPane.showInputDialog(null, "Please enter 3rd Integer: ");

        int num1 = Integer.parseInt(stringNum1);
        int num2 = Integer.parseInt(stringNum2);
        int num3 = Integer.parseInt(stringNum3);

        if ((num1 > num2 && num1 > num3))
        {
            if(num2 > num3)
            {
                System.out.print(num3 + " " + num2 + " " + num1);
            }
            else
                System.out.print(num2 + " " + num3 + " " + num1);
        }
        else if ((num2 > num1 && num2 > num3))
        {
            if(num1 > num3)
            {
                System.out.print(num3 + " " + num1 + " " + num1);
            }
            else
                {
                System.out.print(num1 + " " + num3 + " " + num2);
                }
        }
        else if ((num3 > num1 && num3 > num2))
        {
            if(num1 > num2)
            {
                System.out.print(num2 + " " + num1 + " " + num3);
            }
            else
                System.out.print(num1 + " " + num2 + " " + num3);
        }
        else
        {
            System.out.println("ERROR!");
        }

    }

}

Book's solution:

public class Exercise03_08 {
  public static void main(String[] args) {
    java.util.Scanner input = new java.util.Scanner(System.in);

    // Enter three numbers
    System.out.print("Enter three integers: ");
    int number1 = input.nextInt();
    int number2 = input.nextInt();
    int number3 = input.nextInt();

    if (number1 > number2) {
      int temp = number1;
      number1 = number2;
      number2 = temp;
    }

    if (number2 > number3) {
      int temp = number2;
      number2 = number3;
      number3 = temp;
    }

    if (number1 > number2) {
      int temp = number1;
      number1 = number2;
      number2 = temp;
    }

    System.out.println("The sorted numbers are "
      + number1 + " " + number2 + " " + number3);
  }
}

The books solution, is definitely cleaner, and the biggest difference I can see is the books solution is actually sorting the variables, rather than just changing the display order.

In terms of efficiency, displaying stuff to the console is more efficient than popping up message boxes. There's less conditional logic in the books solution and a lot less code duplication. Other than that, there is very little difference in the sorting operation, the books maybe slightly more efficient due to less conditional checks.

Overall though, in my opinion the book provides the better solution, because it has virtually no duplicated code, conditionals are a single level deep (much easier to read), actually sorts the numbers in memory (if this program were to do more, your variables would already be sorted for you next time you need them, as opposed to having to duplicate the conditional logic, and overall it's much easier to understand what it's doing and what's going on.


Remove words from an exclude list in each row of a pandas.Series I am trying to remove all occurrences of the words I collect in the list exclude from every text note. This is my approach.

import operator,re

exclude=[]
for i in range(len(freq_select)):
  if freq_select[i][1]<=25:
    exclude.append(freq_select[i][0])

def remove_nonfrequent(note):
  print(remove_nonfrequent.i)
  sentences=[]
  for sent in note.split('.'):
    words=sent.split(' ')
    #print("Before:",len(words))
    for word in words:
      for j in exclude:
        if j==word:
          #print(j,word)
          words.remove(word)
    #print("After:",len(words))
    sent=' '.join(words)
    sentences.append(sent)
  note='. '.join(sentences)
  remove_nonfrequent.i+=1
  return '. '.join([i for i in note.split('.') if i!=''])

remove_nonfrequent.i=0
jdf.NOTES=jdf.NOTES.apply(remove_nonfrequent)

I was initially looping over all the notes in pandas series. But now I am using apply() and I can say performance increased little bit. But it still takes a very long time.

Each note in the pandas.Series is of variable length. However, an average note can contain somewhere between 3000-6000 words. One note even has 13000 words.

I would like to get some help in this.

You currently have to loop over all exclude words to check if it is equal to the current word. If your exclude was a set, you could just do if j in exclude. This change alone should speed up your code a lot (how much depends on how large your list of exclude words actually is).

In addition, you could simplify getting the exclude words:

from collections import Counter
form itertools import takewhile

THRESHOLD = 25

words = Counter(whatever_generates_your_words())
exclude = set(t[0] for t in takewhile(lambda t: t[1] <= THRESHOLD,
                                      reversed(words.most_common())))

This uses the fact that collections.Counter.most_common is sorted by frequency, so reversing it starts with the least common words. itertools.takewhile stops taking when the condition is no longer true, so you don't need to go through all words in the Counter.

For the filtering you should probably use some list comprehensions. All three of the following functions are doing the same thing:

from itertools import filterfalse

def remove_nonfrequent(note):
    sentences = []
    for sentence in note.split('. '):
        sentence_ = []
        for word in sentence:
            if word not in exclude:
                sentence_.append(word)
        if sentence:
            sentences.append(" ".join(sentence))
    return ". ".join(sentences)

def remove_nonfrequent(note):
    return ". ".join(" ".join(filterfalse(exclude.__contains__,
                              sentence.split(" ")))
                     for sentence in note.split('. '))
                     if sentence)

def remove_nonfrequent(note):
    return ". ".join(" ".join(word for word in sentence.split(" ")
                              if word not in exclude)
                     for sentence in note.split('. ') if sentence)

I personally prefer the last one, as it is the most readable one to me.

Note that Python has an official style-guide, PEP8, which recommends using 4 spaces as indentation and surrounding operators with spaces.

Since you are using this function with pandas later, a different approach might actually be faster. There is a pd.Series.replace method that can take a dictionary of replacements. You can use it like this:

from itertools import repeat

replacements = dict(zip((fr'\b{word}\b' for word in exclude), repeat("")))
df.NOTES.replace(replacements, regex=True, inplace=True)
df.NOTES.replace({r' +': ' ', r' +\.': '.'}, regex=True, inplace=True)

The first replace does all the word replacements (using \b to ensure only complete words are replaced), while the latter one fixes multiple spaces and spaces before a period.


TCP server in Android to continuously listen to an RFID reader I have created a TCP server thread which I want to keep running and listening to the IOT device (an RFID reader).
I have only one client.

Please review my code and provide your feedback. Are there any edge cases that would make this TCP server fail?

import android.util.Log;
import java.io.DataInputStream;
import java.io.DataOutputStream;
import java.io.IOException;
import java.net.ServerSocket;
import java.net.Socket;

public class TcpServerThread extends Thread {

    private static final String TAG = "TcpServerThread";

    private boolean run_ = true;
    DataInputStream stream;
    ServerSocket serverSocket;
    Socket clientSocket;

    public TcpServerThread() {
        super();
        start(); //start the thread
    }

    @Override
    public void run() {

        byte[] inBytes = new byte[200];
        int mPort = 1025;

        //creating a socket to listen on a given port
        Log.d(TAG, "PORT OPENED SUCESSFULLY in " + mPort);
        try {
            serverSocket = new ServerSocket(mPort);

        } catch (IOException e) {
            Log.d(TAG, "run: [exception occured in creating the socket]" + e.getLocalizedMessage());
        }

        while (run_) {

            Log.d(TAG, "run() :started");

            try {
                while (serverSocket != null) {
                    Log.d(TAG, "run() : running ");
                    try {
                        //accepting the incoming socket connection request
                        long started = System.currentTimeMillis();
                        clientSocket = serverSocket.accept();
                        clientSocket.setKeepAlive(true);

                        if (clientSocket != null) {
                            long established = (System.currentTimeMillis() - started);
                            Log.d(TAG, "[Connection made at]:" + clientSocket.getRemoteSocketAddress() + "in mills :" + established);

                            DataInputStream stream = new DataInputStream(clientSocket.getInputStream());
                            DataOutputStream outToServer = new DataOutputStream(clientSocket.getOutputStream());

                            while ((input = stream.read(inBytes)) != -1) {
                                Log.d(TAG, "Size read is " + input);
                                Log.d(TAG, "Data is " + (inBytes));
                                outToServer.writeByte(10);

                            }

                        }

                    } catch (IOException e) {
                        Log.d(TAG, "IOexception occured: " + e.getLocalizedMessage());
                        continue;

                    } catch (Exception e) {
                        Log.d(TAG, "EXCEPTION occured: " + e.getLocalizedMessage());
                        continue;
                    }

                }

            } catch (Exception e) {
                //we need to watch out for IllegalStateException cases
                Log.d(TAG, "[Exception occured] :" + e.getLocalizedMessage());

                continue;

            }

        }
        try {
            throw new RuntimeException(TAG + " Crashed!!");
        } catch (RuntimeException e) {
                      Log.d(TAG, "RuntimeException: " + e.getLocalizedMessage());

        }
    }

    public void abort() {
        run_ = false;
        closeServer();
        interrupt();
    }

    public void closeServer() {

        try {
            serverSocket.close();
            clientSocket.close();
            stream.close();
        } catch (Exception e) {
            Log.d(TAG, "closeServer: " + e.getLocalizedMessage());
        } finally {
            start();
        }
    }

You should be implementing Runnable, not extending Thread. You are not creating a new generally-useful extension of thread that can be used as an API class by multiple clients. You just want a normal thread to run some code.

Classes not designed to be extended should be marked final. Variables not designed to be reassigned should be marked final.

None of stream, serverSocket, and clientSocket should be visible outside your class. Only expose methods when necessary. Only expose member variables when really, really necessary.

In idiomatic Java, underscores are only used in constants. In other cases, they should not be used.

Prefer primitives to objects where it's reasonable to do so. There's no reason to give loop_count the overhead of being an object.

loop_count is never read and can be removed.

a is a poor variable name. Variable names should clearly indicate the value they're holding. serverPort would be highly preferable. This change would make your comment noise, and it could be removed.

The two log messages could be joined. Also, you're logging the port as being open before it's open.

In idiomatic java, ALL_CAPS are reserved for constants. Other variables should begin with a lowercase letter, even if they are acronyms. ip is preferable to IP.

If you have an error opening the socket, your code busy-loops forever inside the while (run) block until something interrupts it. If the server can't be started, the run method should return, since your code isn't trying to recover from that failure. This will also let you remove the while (serverSocket != null) line, since it will never be null.

When creating the server socket, you're consuming the stack trace. You should always log stack trace. It would also be nice if your message logged the ip and port that could not be opened.

If this code is interrupted or an exception occurs, your open resources will not be closed. Prefer the use of try-with-resources blocks to ensure resources are closed.

Logging the start of every time through the loop is going to result in a lot of meaningless log messages.

The first try block inside the while (run) block does nothing and can be removed.

You dereference clientSocket immediately before you check if it's null. If it could ever be null (it can't), you'd get a NullPointerException before the null check.

Variables should be declared to minimize their scope where possible. This makes code more readable and allows for more aggressive reclamation of memory.

10 is a magic number in your code. Declare a constant whose name explains what the logical meaning of 10 is and use that in your code, so a reader understands the intent, not just that a 10 is going over the wire.

It is a bad practice to catch Exception. You are inadvertently handling any checked or unchecked exceptions that may be added in the future. If you don't know what to do with a specific exception, let it percolate up the stack. Likewise, catching RuntimeException is generally ill advised.

You have a try block that's always throwing an exception. That should obviously be removed.

The stream instance variable is never used, but then closed in the closeServer method. You create stream instances inside run and then don't close them.

The proper term is "millis", not "mills".

It would be nice to extract some of your code into a separate method so run looks less like a giant >.

With proper use of try-with-resources blocks, the closeServer method is not required.

abort should not be firing interrupt(). Include checks inside your run method to see if the thread has been interrupted, and halt gracefully if it has.

If you were to make all these changes, your code might look more like:

public class TcpServer implements Runnable {

    private static final String TAG = "TcpServerThread";
    private static final byte RECEIVED_MESSAGE_PART = 10;

    private boolean running = true;

    public TcpServer() {
        super();
    }

    @Override
    public void run() {
        final int port = 1025;
        final String ip = MiscUtilities.getIpAccess();

        try (final ServerSocket serverSocket = new ServerSocket(port)) {
            Log.d(TAG, "Server started, accepting connections to " + ip + ":" + port);

            while (running && !Thread.currentThread().isInterrupted()) {
                final long started = System.currentTimeMillis();
                try (final Socket clientSocket = serverSocket.accept()) {
                    clientSocket.setKeepAlive(true);

                    final long established = (System.currentTimeMillis() - started);
                    Log.d(TAG, "[Connection made at]:" + clientSocket.getRemoteSocketAddress() + " in mills :" + established);

                    this.handleData(clientSocket);

                } catch (final IOException e) {
                    Log.d(TAG, "Failure handling inbound connection to " + ip + ":" + port, e);
                    continue;
                }
            }
        } catch (final IOException e) {
            Log.d(TAG, "Unable to open socket " + ip + ":" + port, e);
            return;
        }
    }

    public void abort() {
        this.running = false;
    }

    private void handleData(final Socket clientSocket) throws IOException {
        try (final DataInputStream stream = new DataInputStream(clientSocket.getInputStream());
                final DataOutputStream outToServer = new DataOutputStream(clientSocket.getOutputStream())) {

            final byte[] inBytes = new byte[200];
            int input = 0;
            while ((input = stream.read(inBytes)) != -1) {
                Log.d(TAG, "Size read in bytes: " + input);
                Log.d(TAG, "Data is: " + MiscUtilities.bytesToHex(inBytes));

                outToServer.writeByte(RECEIVED_MESSAGE_PART);
            }
        }

    }

}


Knuth's Word Wrap Algorithm in Haskell I put together a Haskell function for Knuth's word wrap algorithm. A simplified version of the algorithm is described here.

I've already been told that there are certain aspects of my style that are not very agreeable (e.g. lack of comments and type declarations). However, I'd like to get some opinions on where I might be able to improve the performance of the code itself without resorting to agressive, structure breaking optimization techniques.

import Data.Array
import Data.List
import Data.Ord

knuth m xs = map (concat . intersperse " ") $ split ixs 1 ws
  where
    (_:ixs)           = reverse $ snd (c ! lls)
    ws                = words xs
    ls                = map length ws
    lls               = length ls

    split [] _ ws     = [ws]
    split (x:xs) n ws = let (a, b) = splitAt (x-n) ws in a : split xs x b

    c                 = listArray (0, lls) $ map go [0..lls]
      where
        go 0          = (0, [])
        go j          = minimumBy (comparing fst) $ map (path j) [1..j]
        path j i      = let (x, p) = c ! (i-1) in (lc ! (i, j) + x, i:p)

    lc                = listArray ((1, 1), (lls, lls)) $ concat $ go ls
      where
        go            = map (costs . extras) . take lls . tails
        extras        = lpad lls 0 . map (m -) . zipWith (+) [0..] . scanl1 (+)
        lpad m x      = reverse . take m . (++ repeat x) . reverse
        costs []      = []
        costs (e:es)
          | e < 0     = (1/0) : costs es
          | null es   = [0]
          | otherwise = exp (fromIntegral e) : costs es

The code can be exercised like the following examples:

t1 = knuth 6 "aaa bb cc ddddd"
t2 = knuth 37 "You cant trust code that you did not create yourself. (Especially code from companies that employ people like me.) No amount of source-level verification or scrutiny will protect you from untrusted code."

Lack of comments, type information, and good naming makes this code much harder to read than it should be.

lls is not descriptive, and the s at the end makes it sound like a list of something, when it's just a number.  Since n is not used for anything else, I'd go with that.

The names c and lc are horrible, these arrays are central to the algorithm, and their definitions take up most of the function, but there is nothing to tell us what they are in the program.  The linked article says that the c array represents cumulative costs, and lc[i][j] is the cost of a line containing words i through j, so I would suggest the names cumulativeCost and lineCost.  However, the c in your program also adds the p array from the article, so cumulativeCost doesn't quite cover it.  cumulativeCostAndPath is a bit long, but not too bad.

Since we're only ever interested in either the cost or the path from cumulativeCostAndPath (though they're calculated together), I'd suggest getpath = snd and cumCost = fst as ways of being clearer about what you mean (you could also separate the pieces, but that would be more work).

With these name changes, the first confusing line of the program, (_:ixs) = reverse $ snd (c ! lls) becomes (_:ixs) = reverse $ getpath (cumulativeCostAndPath ! n).

path j i = let (x, p) = c ! (i-1) in (lc ! (i, j) + x, i:p) is also hard to read, mostly because of naming.  It uses 6 names, of which lc is the only one with more than one character, and only i and j have obvious meanings.  I'm going to call x costSoFar and p pathSoFar.  I'm also going to change the let into a where and break it into 2 lines.

path j i = (lineCost ! (i, j) + costSoFar, i:pathSoFar)
           where (costSoFar, pathSoFar) = cumulativeCostAndPath ! (i-1)

It is not obvious what extras is trying to do, even with the article as a reference.  It would be much easier to follow with a comment explaining the formula for extras[i][j] in the article, what extras is meant to represent, and mentioning that the scanl1 call in extras and the tails call in go interact to create the last term in extras.

The costs function could use a comment explaining what each of the 3 possibilities means.  If extras were extracted to be its own array, it would allow costs to be rewritten more cleanly, as well as potentially simplifying the definition of lc.  Before figuring out the details of what's going on, that deep pipeline of functions was hard to reason about.

You spend a lot of effort making lists which have to match up to arrays.  This is harder to follow than defining each array element with a function (which is possible for these arrays), and it sets you up for off-by-one errors if you're not careful.

I can see two possible ways to speed it up.  You use exp in the definition of costs, which is more expensive than the 2 multiplications required for cubing suggested in the article.  Also, the calculation of extras doesn't stop after it's impossible.  Since after finding that words i through j won't fit, all of the rest of them won't fit either, you could avoid calculating everything after the first negative number.  If you have 500 words to wrap and an 80 column limit, figuring out whether you can fit words 1 through 200 on a line is a waste of time.


Powerset (all subsets of a set) in Java I am trying to get a powerset (all subsets of a set) in Java. So my logic for that is:

A given set is one subset, so add so it as it is to result.
Remove each element of set from given set and we will get all the combinations for 1 element less. Add these results to final result.
Recursively find subsets of all sets from step 2 and add to result.

I tried searching for a similar solution on Stack Overflow but did not get one. Is this a clean way to implement it?

import java.util.*;

public class PowerSet {
    public static <E> Set<Set<E>> findPowerSet(Set<E> set){
        Set<Set<E>> ret = new HashSet<Set<E>>();
        ret.add(set);
        if(set.isEmpty()){
            return ret;
        }
        Iterator<E> it = set.iterator();
        while(it.hasNext()){
            Set<E> tmp = new HashSet<E>(set);   //create a copy of current set
            tmp.remove(it.next());              //remove current element from copy set
            ret.add(tmp);                       //add the remaining set to result
            ret.addAll(findPowerSet(tmp));      //recursively find subsets of copy set
        }
        return ret;
    }

    public static void main(String[] args) {
        Set<Character> set = new HashSet<Character>();
        set.add('a');set.add('b');set.add('c');
        System.out.println("Input set");        printSet(set);
        System.out.println("\nsub sets");
        findPowerSet(set).stream().forEach(PowerSet::printSet);
    }

    public static <E> void printSet(Set<E> set){
        StringBuilder sb = new StringBuilder(set.size()==0 ? "{}\n" :"{");
        Iterator<E> it = set.iterator();
        while(it.hasNext()){
            sb.append(it.next().toString())
            .append(it.hasNext()? ", " : "}\n");
        }
        System.out.print(sb.toString());
    }
}

if(set.isEmpty()){
            return ret;
        }

This is unnecessary. If the set is empty, the iterator will return false to the first hasNext(), and you'll go straight to the final return ret;.

Other than that, it seems like a simple, obviously correct, approach.

However, it is quite inefficient.

        Iterator<E> it = set.iterator();
        while(it.hasNext()){
            Set<E> tmp = new HashSet<E>(set);   //create a copy of current set
            tmp.remove(it.next());              //remove current element from copy set
            ret.add(tmp);                       //add the remaining set to result
            ret.addAll(findPowerSet(tmp));      //recursively find subsets of copy set
        }
        return ret;
    }

Suppose that we call it with a set of \$n\$ elements. The outermost call produces a set of \$2^n\$ subsets by taking the union of \$n\$ subresults, each of \$2^{n-1}\$ elements. So the outermost call discards as duplicates \$(n-2)2^{n-1}\$ subsets. But, of course, each of those recursive calls discarded \$(n-3)2^{n-2}\$ subsets from their subresults, etc.

This is why the typical simple implementation is to build up from the empty set.

An advanced implementation, which isn't always better but can reduce copying in some use cases, is to write a custom iterator which uses a Gray code to add or remove precisely one element in each call to next().


Interpret YYYYMMDD as the nth day of the year I am provided a bunch of dates in the format (YYYYMMDD) such as:

date='20170503'

My goal is to convert that date into the number of days in the given year. For example, it would convert:

20170130 to 30
20170201 to 32
20151231 to 365

Here is my solution:

import pandas as pd
my_date = pd.to_datetime(date, format='%Y%m%d')
my_ordinal = 736329 - 365*(2017 - my_date.year)
print(datetime.toordinal(my_date) - my_ordinal)

Is there a better way to do this without making it much more complex? Note that pandas is already in the program, so there is no extra burden in using it.

As you already use pandas, you are right that there is no overhead on using Timestamps (the kind of objects returned by pd.to_datetime). But, as said in the documentation (help(pd.Timestamp)):

  Help on class Timestamp in module pandas.tslib:

  class Timestamp(_Timestamp)
 |  TimeStamp is the pandas equivalent of python's Datetime
 |  and is interchangable with it in most cases. It's the type used
 |  for the entries that make up a DatetimeIndex, and other timeseries
 |  oriented data structures in pandas.

So you may choose pd.Timestamp or Python's datetime.datetime interchangeably.

The simplest way to count days from January 1st of the same year is to build both objects and then substract each other to get the count of days separating them. Take care of adding one day to actually count January 1st as well:

date = pd.to_datetime('20170201', format='%Y%m%d')
new_year_day = pd.Timestamp(year=date.year, month=1, day=1)
day_of_the_year = (date - new_year_day).days + 1

Now, having this piece of code hanging around is not really of any use, so you should wrap it into a function for better reusability:

import pandas as pd

def date_to_nth_day(date, format='%Y%m%d'):
    date = pd.to_datetime(date, format=format)
    new_year_day = pd.Timestamp(year=date.year, month=1, day=1)
    return (date - new_year_day).days + 1

if __name__ == '__main__':
    print(date_to_nth_day('20170201'))

Using Python's datetime module, the function would look like:

def date_to_nth_day(date, format='%Y%m%d'):
    date = datetime.datetime.strptime(date, format=format)
    new_year_day = datetime.datetime(year=date.year, month=1, day=1)
    return (date - new_year_day).days + 1


Connecting and getting mysql connection object I have written this code that connects to a mysql database. The function should be able to be used as a context manager - via with. It is working, but I am not sure if this is a great way of doing it.

conect.py

import os
import mysql.connector
from mysql.connector import errorcode
import contextlib

_CFG = {
  'user': os.getenv("MYSQL_USER"),
  'password': os.getenv("MYSQL_PASSWORD"),
  'host': os.getenv("MYSQL_SERVER"),
  'database': os.getenv("SENSOR_DB"),
  'raise_on_warnings': True
}

@contextlib.contextmanager
def _connect(db=None):
    if db:
        _CFG["database"] = db

    try:
        print("{usr} connecting to {db} on {host}.".format(
            usr=_CFG["user"],
            db=_CFG["database"],
            host=_CFG["host"]
            )
        )
        connection = mysql.connector.connect(**_CFG)
        yield connection
    except mysql.connector.Error as err:
        if err.errno == errorcode.ER_ACCESS_DENIED_ERROR:
            print("Something is wrong with your user name or password")
        elif err.errno == errorcode.ER_BAD_DB_ERROR:
            print("Database does not exist")
        else:
            print(err)
        yield None
    else:
        connection.commit()
    finally:
        connection.close()

Obvious (dumb) point - I'd suggest using logging instead of print
I would have some kind of builder for _CFG so I could choose if I use environment variables for configuration or something else
Instead of str.format, you may want to use fstring: f"{_CFG["user"]} connecting to {_CFG["database"]} on {_CFG["host"]}."
Instead of yield None you may want to reraise exception (from the docs, "If an exception is trapped merely in order to log it or to perform some action (rather than to suppress it entirely), the generator must reraise that exception"). Otherwise, your code will fail with a funny exception.


Codility voracious fish are moving along a river I am trying to solve the codility problem:

  N voracious fish are moving along a river. Calculate how many fish are alive.

Codility Problem

My code get only 50 percent of 100. Could you tell me what is wrong with my code?

  You are given two non-empty zero-indexed arrays A and B consisting of
  N integers. Arrays A and B represent N voracious fish in a river,
  ordered downstream along the flow of the river.

  The fish are numbered from 0 to N − 1. If P and Q are two fish and P <
  Q, then fish P is initially upstream of fish Q. Initially, each fish
  has a unique position.

  Fish number P is represented by A[P] and B[P]. Array A contains the
  sizes of the fish. All its elements are unique. Array B contains the
  directions of the fish. It contains only 0s and/or 1s, where:

  0 represents a fish flowing upstream, 1 represents a fish flowing
  downstream. If two fish move in opposite directions and there are no
  other (living) fish between them, they will eventually meet each
  other. Then only one fish can stay alive − the larger fish eats the
  smaller one. More precisely, we say that two fish P and Q meet each
  other when P < Q, B[P] = 1 and B[Q] = 0, and there are no living fish
  between them. After they meet:

  If A[P] > A[Q] then P eats Q, and P will still be flowing downstream.
  If A[Q] > A[P] then Q eats P, and Q will still be flowing upstream. We
  assume that all the fish are flowing at the same speed. That is, fish
  moving in the same direction never meet. The goal is to calculate the
  number of fish that will stay alive.

  For example, consider arrays A and B such that:

A[0] = 4    B[0] = 0
A[1] = 3    B[1] = 1
A[2] = 2    B[2] = 0
A[3] = 1    B[3] = 0
A[4] = 5    B[4] = 0

  Initially all the fish are alive and all except fish number 1 are
  moving upstream. Fish number 1 meets fish number 2 and eats it, then
  it meets fish number 3 and eats it too. Finally, it meets fish number
  4 and is eaten by it. The remaining two fish, number 0 and 4, never
  meet and therefore stay alive.

  Write a function:

class Solution { public int solution(int[] A, int[] B); }

  that, given two non-empty zero-indexed arrays A and B consisting of N
  integers, returns the number of fish that will stay alive.

  For example, given the arrays shown above, the function should return
  2, as explained above.

  Assume that:

  N is an integer within the range [1..100,000]; each element of array A
  is an integer within the range [0..1,000,000,000]; each element of
  array B is an integer that can have one of the following values: 0, 1;
  the elements of A are all distinct.

  Complexity:

  Expected worst-case time complexity is O(N); expected worst-case space
  complexity is O(N), beyond input storage (not counting the storage
  required for input arguments).

This is my algorithm:

public static int solution(int[] size, int[] direction)
{
    var stack = new Stack<int>();
    const int UPSTREAM = 0;
    const int DOWNSTREAM = 1;
    var updateStack = false;

    for (int i = 0; i < size.Length; i++)
    {
        if (stack.Count <= 0) //initial data
            stack.Push(i);
        else
        {
            var lastFish = stack.Peek();

            if (direction[i] == direction[lastFish] || direction[lastFish] == UPSTREAM && direction[i] == DOWNSTREAM)
            {
                stack.Push(i);
            }
            else
            {
                while (stack.Count > 0 && size[lastFish] < size[i] && direction[lastFish] == DOWNSTREAM && direction[i] == UPSTREAM)
                {
                    updateStack = true;
                    stack.Pop();
                    if (stack.Count > 0)
                        lastFish = stack.Peek();
                }
                if (updateStack)
                {
                    stack.Push(i);
                    updateStack = false;
                }
            }
        }

    }
    return stack.Count;
}

This is an example of a situation where good naming is your friend. Instead of parameters A and B, and magic numbers 0 and 1, and the rather long-winded expression stack.Peek(), I suggest you start by installing some useful names:

const int UPSTREAM = 0;
const int DOWNSTREAM = 1;

public static int solution(int[] size, int[] direction)
{
    var stack = new Stack<int>();
    var updateStack = false;
    for (int i = 0; i < size.Length; i++)
    {
        if (stack.Count <= 0) //initial data
            stack.Push(i);
        else
        {
            var lastFish = stack.Peek();

            if (direction[i] == direction[lastFish]
             || direction[i] != direction[lastFish]
                && direction[lastFish] == UPSTREAM
                && direction[i] == DOWNSTREAM)
            {
                stack.Push(i);
            }
            else
            {
                while (stack.Count > 0
                       && size[lastFish] < size[i]
                       && direction[lastFish] == DOWNSTREAM
                       && direction[i] == UPSTREAM)
                {
                    updateStack = true;
                    stack.Pop();
                    lastFish = stack.Peek();
                }
                if (updateStack)
                    stack.Push(i);
            }
        }

    }
    return stack.Count;
}

This shows me there is some confusion on your part. You are allowed to assume that a logical or (||) fails because it's false - there's no need to repeat the test in a false mode:

if (direction[i] == direction[lastFish]
 || direction[i] != direction[lastFish] && ...

Instead, just assume that failure implies failure, and go on:

if (direction[i] == direction[lastFish]
  || direction[lastFish] == UPSTREAM
     && direction[i] == DOWNSTREAM)

Furthermore, you can assume that since you know the directions are different, and since you know there are only two directions, testing one of them is enough:

if (direction[i] == direction[lastFish] || direction[i] == DOWNSTREAM)

That said, I notice that you initialize updateStack to false outside of your loop and never reset it to false anyplace else. This means that once you've set updateStack = true, it will stay true forever (inside you loop, anyway). That's probably the source of your error, because it should stop being true once you've updated the stack.


def parse_board(board):
    def map_black(spot):

This name suggests that the function is a variant on map. Whereas its actually intended to be used as a parameter on map

         return str(int(spot == 'B'))
    def map_white(spot):
         return str(int(spot == 'W'))

Both functions are very similiar, can you refactor?

    blackInit = '0b'
    whiteInit = '0b'

The python style guide suggest 'lowercase_with_underscores' for local variable names

    for line in board:
        squares = line.split('|')
        blackInit = blackInit + ''.join(map(map_black,squares))
        whiteInit = whiteInit + ''.join(map(map_white,squares))

Repeatedly adding strings is not very efficient python.

    return (blackInit,whiteInit)

Everything in your function is done twice. Once for 'B' and once for 'W'. I'd suggest you should have a function that takes 'B' or 'W' as parameter, and call that twice.

Here's how I'd tackle this:

def bitstring_from_board(board, letter):
    bits = []
    for line in board:
        bits.extend( spot == letter for spot in line.split('|') )
    return '0b' + ''.join(str(int(x)) for x in bits)

def parse_board(board):
    return bitstring_from_board(board, 'B'), bitstring_from_board(board, 'W')


Program to rotate an array I wrote this code where I have to rotate an array of 'n' elements by 'd' elements in the anti-clockwise direction.Can it be done in a better and simpler way?

 public class RotateArr{
     public static void rotate(int[] arr,int d,int n){

    System.out.println("Array before rotating : ");

    for(int l=0; l<n; l++){
        System.out.print(arr[l]+" ");
    }
        System.out.println();

    int temp = arr[0];

    for(int i=0; i<d; i++){
        for(int j=1; j<n; j++){
            arr[j-1] = arr[j];
            if(j==(n-1)){
                arr[n-1] = temp;
            }
        }
        temp = arr[0];
    }

    System.out.println("Array after rotating");
    for(int k=0; k<n; k++){
        System.out.print(arr[k]+" ");
    }
}
    public static void main(String[] args) {

    int[] arr = {1,2,3,4,5,6,7};

    int d = 3;

    int n = arr.length;

    rotate(arr,d,n);
      }
   }

Separation of concerns. Ideally, one method should do one thing. I would expect the rotate method to actually rotate the array. That's it. Your method also prints something to the standard output. There are two reasons why it's bad. Firstly, it's confusing. Secondly, it introduces more reasons for the method to change. For instance, if you need to change the format of the output or write it to the file, you have to change the rotate method. It's not convenient, is it?
The interface is quite strange. The n parameter of the rotate method is redundant (it's expected to be equal to the length of the array, isn't it?). It also makes the interface less clear (what if I pass a different value of n?).
The algorithm itself is not efficient. Shifting the array be one d times requires O(n * d) time. There are at least two ways to make it linear. The first one is pretty simple. You can copy the entire array to a buffer and that assign res[i] = buffer[(i + d) % buffer.length] in a loop. However, it requires additional memory. The second one uses a constant amount of space and O(n) operations. It goes like this: to cyclically shift an array by k positions to the left, one can reverse the [0, k - 1] range, then reverse the [k, n - 1] range and finally reverse the entire array.
Documenting your code is a good practice. It's not clear what the valid range of values for d is from your code itself. It's also not clear whether it performs a left or a right shift.
Your code formatting violates the Java coding conventions. The indentation is kind of messed up. The binary operators are not surrounded with whitespaces. The names of the variables are not meaningful (for example, what does d stand for?). That's why it's hard to read.


Quiz setting users grade I have made this code to determine the users grade for a quiz. However I've just been thinking that maybe I could shorten the code somehow? as its just a lot of if and elif statements.

if($_SESSION['numberQuestions'] == 10){
        if($_SESSION['score'] == 10){
            $grade = "A+";
        }
        elseif($_SESSION['score'] == 9){
            $grade = "A";
        }
        elseif($_SESSION['score'] == 8){
            $grade = "B";
        }
        elseif($_SESSION['score'] == 7){
            $grade = "C";
        }
        elseif($_SESSION['score'] == 6){
            $grade = "D";
        }
        elseif($_SESSION['score'] <= 5){
            $grade = "F";
        }
    }
    if($_SESSION['numberQuestions'] == 5){
        if($_SESSION['score'] == 5){
            $grade = "A+";
        }
        elseif($_SESSION['score'] == 4){
            $grade = "A";
        }
        elseif($_SESSION['score'] == 3){
            $grade = "B";
        }
        elseif($_SESSION['score'] == 2){
            $grade = "C";
        }
        elseif($_SESSION['score'] == 1){
            $grade = "D";
        }
        elseif($_SESSION['score'] == 0){
            $grade = "F";
        }
    }

You could use an array of grades:

$grades = Array("F","F","F","F","F","F","D","C","B","A","A+");
if($_SESSION['numberQuestions'] == 10){
    if ($_SESSION['score'] <= 10 and $_SESSION['score'] >= 0) {
        $grade = $grades[$_SESSION['score']];
    } else {
        $grade = 'Unknown';
    }
}

Use the same method for the second test.


Ruby Connected Components in a Graph I've written the following code to find the connected components of a graph in ruby, but it is quite slow since I don't use any sensible data structure for the disjoint sets, and hence find_set is probably \$O(n^2)\$ (don't know the complexity for enumerable.find in Ruby).

Can anybody help me improve it? I am relatively new to ruby, and so would appreciate any insights. This code is meant to solve the problem posed Here:

My solution, in plain English, is the find the number of connected components in the graph given, then either calculate the cost of building a library in every node or the cost of building a library in each connected component plus the cost of building the roads for the minimum spanning tree of each component.

The Code

#!/bin/ruby

require 'set'

def roadsAndLibraries(n, c_lib, c_road, cities)
  vertices = make_set((1..n).to_a)

  #find connected components
  cities.each do |a, b|
    first_set = find_set(vertices, a)
    second_set = find_set(vertices, b)
    if first_set != second_set
      vertices.add(first_set | second_set)
      vertices.delete(first_set)
      vertices.delete(second_set)
    end
  end

  #calculate the cost of building the edges required for a MST of each component
  roads_cost = c_road * vertices.to_a.map{|x| x.size - 1}.inject(:+)

  if c_road > c_lib
    return n * c_lib
  else
    return vertices.size * c_lib + roads_cost
  end

end

#takes an array of integers, and maps them into a set of singleton sets containing each node number
def make_set(arr_n)
  vertices = Set.new
  for i in 1..arr_n.size
    vertices.add([i])
  end
  vertices.map!{|x| x.to_set}
end

#finds set containing a given node value
def find_set(set, node)
  set.find{|x| x.include?(node)}
end

# q is number of queries, n is the number of vertices in the graph
q = gets.strip.to_i
for a0 in (0..q-1)
  n, m, c_lib, c_road = gets.strip.split(' ')
  n = n.to_i
  m = m.to_i
  c_lib = c_lib.to_i
  c_road = c_road.to_i
  cities = Array.new(m)
  for cities_i in (0..m-1)
    cities_t = gets.strip
    cities[cities_i] = cities_t.split(' ').map(&:to_i)
  end
  result = roadsAndLibraries(n, c_lib, c_road, cities)
  puts result
end

It Your solution being slow is not too much a matter of language here.
I will try highlighting ruby related things you could improve to make it more readable on one hand,
and algorithmic flaws on the other hand.

Ruby

Here I will point out some of the ruby code that could be improved. Note that this will not help
your solution to be much faster since it is mostly an algorithmic problem you have here.

Find Set

Your find_set method seems fine. It is a O(n) method. You have to go through every set to find
the one which includes the node you want. The #include? method of a set is constant in time, and
that is a good use of set here.

Make Set

Your make_set method could be made shorter and cleaner. For instance:

def make_set(n)
  Set.new((1..n).map { |i| Set.new([i]) })
end

Map + Inject

I would not recommand using #map and then #inject as it will create an intermediate array, and
because you can have all in the #inject, like:

vertices.inject(0) { |sum, set| set.size + 1 + sum }

And by the way, you can get rid of to_a since sets accept the #inject and #map methods too.

Method name

You should use snake case: roadsAndLibraries becomes roads_and_libraries.

Algorithmic enhancements

Unneeded computation

Itseems to me you don't need any computation if the road is more expensive than the library.
You can move this to the top of your roadsAndLibraries method.

return n * c_lib if c_road > c_lib

Sets lookup

The main issue with your solution here is probably the lookup you do for each edge. You parse the
whole set of vertices for each city. This is expensive. You could try having a constant lookup
here that would help getting the connected component of a vertice on O(1).

You can use 2 arrays here. One for storing the connected components, like you already do. But
instead of storing them in a set, you can store them in an array, so they have an associated index.
Let's call it components.

And then the second array can be the lookup, we will call it component_index. For each node, you
store the index of its connected component. That way, all find_set has to do is to take the
stored index: components[component_index[a]] for city a.

Your main loop could look like:

cities.each do |a, b|
  first_set = component_index[a]
  second_set = component_index[b]
  next if first_set == second_set
  merge_components(component_index, components, first_set, second_set)
end

Merging components

Where the algorithm will spend time is when we merge components together. Here you will have to
update component indexes. What you have to see here is that it will be performed much less often
than the lookup, especially in case of strongly connected components, where first_set and
second_set of the main loop will be equal.

To merge 2 components, I would put the nodes of the smallest component into the largest one, to
avoid updating too many indexes. And then I would update the indexed component so that it contains
all nodes.

def merge_components(component_index, components, first_set, second_set)
  to = first_set
  from = second_set
  to, from = from, to if components[to].size < components[from].size

  components[from].each do |i|
    component_index[i] = to
  end
  components[to].merge components[from]
end

In case memory is a problem, you probably don't need to store the sets themselves, but only their
size, since you know that they will never overlap, and that the size of the merged set is always
the sum of the sizes of the 2 sets. But by doing so you have to loop through all nodes when
updating components, which can be much longer.

Building Cost

Last thing to change: how we compute the cost of the solution. We will need the final connected
components. This can be extracted like:

final_component_index = component_index.uniq

Then the MST cost would be

  roads_cost = c_road * final_component_index.inject(0) { |sum, x| components[x].size - 1 + sum }

And the final cost

  final_component_index.size * c_lib + roads_cost


Codility Fish (queue) programming challenge Today I have attempted a Codility exercise "Fish" (https://app.codility.com/programmers/lessons/7-stacks_and_queues/fish/), and I have to say I found it to be:
1) Not necessarily "easy" as implied by the page, AND
2) Quite satisfying to figure out in the end.
Below the text of the challenge:

  You are given two non-empty arrays A and B consisting of N integers.
  Arrays A and B represent N voracious fish in a river, ordered
  downstream along the flow of the river.

  The fish are numbered from 0 to N − 1. If P and Q are two fish and P <
  Q, then fish P is initially upstream of fish Q. Initially, each fish
  has a unique position.

  Fish number P is represented by A[P] and B[P]. Array A contains the
  sizes of the fish. All its elements are unique. Array B contains the
  directions of the fish. It contains only 0s and/or 1s, where:

  0 represents a fish flowing upstream,
  1 represents a fish flowing downstream.

  If two fish move in opposite directions and there are no other
  (living) fish between them, they will eventually meet each other. Then
  only one fish can stay alive − the larger fish eats the smaller one.
  More precisely, we say that two fish P and Q meet each other when P <
  Q, B[P] = 1 and B[Q] = 0, and there are no living fish between them.
  After they meet:

  If A[P] > A[Q] then P eats Q, and P will still be flowing downstream,
  If A[Q] > A[P] then Q eats P, and Q will still be flowing upstream.

  We assume that all the fish are flowing at the same speed. That is,
  fish moving in the same direction never meet. The goal is to calculate
  the number of fish that will stay alive.

My solution is a bit playful, and I was wondering if anyone could have a look and let me know if this is a correct way of solving this problem. All comments will be appreciated.

import java.util.ArrayDeque;

// you can write to stdout for debugging purposes, e.g.
// System.out.println("this is a debug message");

class Solution {
    public int solution(int[] A, int[] B) {

        ArrayDeque<Fish> safetyPool = new ArrayDeque<Fish>();
        ArrayDeque<Fish> activePool = new ArrayDeque<Fish>();

        for (int i = 0; i < A.length; i++) {
            activePool.offer(Fish.cFish(A[i], B[i]));
        }

        while(!activePool.isEmpty()) {
            // System.out.println(safetyPool + " " + activePool);
            Fish safeFish = safetyPool.peekLast();
            Fish swimmingFish = activePool.peekFirst();

            if (safeFish != null) {
                if (safeFish.dir == 1) {
                    if (safeFish.dir != swimmingFish.dir) {
                        if (safeFish.att > swimmingFish.att) {
                            activePool.removeFirst();
                        } else {
                            safetyPool.removeLast();
                        }
                    } else {
                        safetyPool.offer(activePool.pollFirst());
                    }
                } else {
                    safetyPool.offer(activePool.pollFirst());
                }
            } else {
                safetyPool.offer(activePool.pollFirst());
            }
        }
        return safetyPool.size();
    }
}

class Fish {
    public int att;
    public int dir;

    public Fish(int att, int dir) {
        this.att = att;
        this.dir = dir;
    }

    public static Fish cFish(int att, int dir) {
        return new Fish(att, dir);
    }

    @Override
    public String toString() {
        return String.format("Fish(att: %d, dir: %d)", att, dir);
    }
}
```

I have some suggestions.

When declaring variable that has inheritance, uses the interface in the left declaration part; this will make the code easier to refactor.

Before

ArrayDeque<Fish> safetyPool = new ArrayDeque<Fish>();
ArrayDeque<Fish> activePool = new ArrayDeque<Fish>();

After

Deque<Fish> safetyPool = new ArrayDeque<Fish>();
Deque<Fish> activePool = new ArrayDeque<Fish>();

When using the diamond operators with variables, declared the type in the left side, the right one will become optional; since it's declared in the variable side.

Deque<Fish> safetyPool = new ArrayDeque<>();
Deque<Fish> activePool = new ArrayDeque<>();

To remove the Arrow Code in the while, I suggest that you read the blog post made by Jeff Atwood Flattening Arrow Code. In my opinion, it's the best way to get rid this kind of code.

The logic contained in the while can be refactored.

while (!activePool.isEmpty()) {
   Fish safeFish = safetyPool.peekLast();
   Fish swimmingFish = activePool.peekFirst();

   if (safeFish == null || safeFish.dir != 1 || safeFish.dir == swimmingFish.dir) {
      safetyPool.offer(activePool.pollFirst());
      continue;
   }

   if (safeFish.att > swimmingFish.att) {
      activePool.removeFirst();
   } else {
      safetyPool.removeLast();
   }
}


Find pairs in an integer array whose sum is equal to n (bonus: do it in linear time) I gave this one a shot, and the code works. Am I on the right track? How can the code be improved? Also, I am unaware of how to determine if this would be linear time or not. Could someone maybe break that determination down for me?

'''
Problem statement: Find pairs in an integer array whose sum is equal to n (bonus: do it in linear time)

@author: Anonymous3.1415
'''

def pair_sum_to_n(integer_list, n):
    pairs = []
    #checks for the occurence of halfs
    if not n % 2:
        if integer_list.count(n/2) > 1:
            pairs.append((n/2, n/2))
        integer_set = set(integer_list) - {n/2}
    #finds pairs of integers where x + (n - x) = n
    for x in integer_set:
        if (n - x) in integer_set:
            if (n - x, x) not in pairs:
                pairs.append((x, n - x))
    return pairs

integer_list = list(map(int, raw_input().strip().split(" ")))
pairs = pair_sum_to_n(integer_list, 10)
print(pairs)

The map by default returns a list. You do not need another wrapper around it.

Your current implementation is of \$ O(n^2) \$ complexity. The statement:

(n - x, x) not in pairs

is of \$ O(n) \$ time, which lies inside a loop with \$ O(n) \$.

To do this in linear time, generate a dict/hash where you store simply the \$ n - x \$ values in first iteration. In the second (and independent) iteration, simply append the pair $$ \left( min(x,\ n - x),\ max(x,\ n - x) \right) $$ for each x in your list, if the \$ O(1) \$ lookup in the dict is true.