Project Euler #50 Consecutive prime sum I'm having trouble optimising the project euler #50 exercise, it runs for around 30-35 seconds, which is terrible performance.

  The prime 41, can be written as the sum of six consecutive primes:

  41 = 2 + 3 + 5 + 7 + 11 + 13
  This is the longest sum of consecutive primes that adds to a prime below one-hundred.

  The longest sum of consecutive primes below one-thousand that adds to a prime, contains 21 terms, and is equal to 953.

  Which prime, below one-million, can be written as the sum of the most consecutive primes?

The catch here is that it's not necessary to start from 2, the sum of consecutive primes which add to 953 are starting from 7.

Here's my code :

    static void Main(string[] args)
    {
        int max = 0;
        int maxCount = 1;
        List<int> primes = new List<int>();
        Stopwatch sw = Stopwatch.StartNew();
        bool[] allNumbers = SetPrimes(1000000);
        for (int i = 0; i < allNumbers.Length; i++)
        {
            if (allNumbers[i])
            {
                primes.Add(i);
            }
        }
        foreach (int prime in primes)
        {
            int startingIndex = 0;
            while (primes[startingIndex] < prime/maxCount)
            {
                int n = prime;
                int j = startingIndex;
                int sum = 0;
                int count = 0;
                while (n > 0)
                {
                    sum += primes[j];
                    n -= primes[j];
                    j++;
                    count++;
                }
                if (sum == prime)
                {
                    if (count > maxCount)
                    {
                        maxCount = count;
                        max = prime;
                    }
                }
                startingIndex++;
            }
        }
        sw.Stop();
        Console.WriteLine(max);
        Console.WriteLine($"Time to calculate : {sw.ElapsedMilliseconds}");
        Console.ReadKey();
    }

    private static bool[] SetPrimes(int max)
    {
        bool[] localPrimes = new bool[max + 1];
        for (int i = 2; i <= max; i++)
        {
            localPrimes[i] = true;
        }
        for (int i = 2; i <= Math.Sqrt(max); i++)
        {
            if (localPrimes[i])
            {
                for (int j = i * i; j <= max; j += i)
                {
                    localPrimes[j] = false;
                }
            }
        }
        return localPrimes;
    }

Even though your prime number generator is not the bottleneck, you should not do this:

for (int i = 2; i <= Math.Sqrt(max); i++)
                     ^^^^^^^^^

Do not calculate the same square root in every loop iteration. Sqrt is an expensive enough operation that you don't want to call unnecessarily. Calculate it only once, store that in a variable and use that in the loop.

I couldn't completely analyze your algorithm, but I ran through the first few steps with the debugger. It looks like you're doing a lot of unnecessary work.
For each prime, you start by looking for a sum of length 1, then length 2, then 3, etc. Even if you have already found a sum of length 100, you always start at 0 again. I'm guessing that's where your bottleneck is.
You want to find the longest anyway, so why not start at the maximum length and shorten it as you go? You can stop as soon as you find one. (there are a lot more possible sums of length 2 than of length 500)

My algorithm works like this:
We have a sum of primes: p(1), p(2), p(3), ... p(n-1), p(n)
See if this sum is a prime also (by doing a binary search on the prime numbers)
If it's not, check the next sum of the same length by substracting p(1) and adding p(n+1). Keep doing this until we find a prime or until the sum becomes greater than 1000000.
Then, shorten the length by 1 by subtracting the last prime, so we get the sum p(1),p(2),p(3)...p(n-1) and check each sum of this length, etc.
(This code is a few years old and might still be a little sloppy)

public int Solve050() {
    const int Limit = 1000000;
    int[] primes = WhateverPrimeGenerator.PrimesUpTo(Limit).ToArray();
    int sum = 0, length = 0;
    //Find the maximum possible length by adding up primes, while sum < Limit
    while (sum < Limit) {
        int newSum = sum + primes[length];
        if (newSum >= Limit) break;
        sum = newSum;
        length++;
    }

    int answer = 0;
    for (; length > 1; length--) {
        answer = FindPrime(primes, Limit, sum, length - 1);
        if (answer > 0) break;
        sum -= primes[length - 1];
    }
    return answer;
}

 //Tries to find a prime of sum-length defined by lastIndex
private static int FindPrime(int[] primes, int maxSum, int sum, int lastIndex)
{
    int result = 0;
    int index = lastIndex + 1;
    for (int firstIndex = 0; lastIndex < primes.Length && sum <= maxSum; firstIndex++, lastIndex++) {
        index = Array.BinarySearch(primes, index, primes.Length - index, sum);
        if (index > 0) result = primes[index]; //Prime found
        if (index < 0) index = ~index;
        sum = sum - primes[firstIndex] + primes[lastIndex + 1];
    }
    return result;
}


Listing books by priority #region GetBookListByPriority
private static List<BCBook> GetBookListByPriority(List<BCBook> listBcBook)
{
    List<BCBook> newList = new List<BCBook>();
    try
    {
        List<BCBook> listNonPriorityBcBooks = new List<BCBook>();
        List<BCBook> listPriorityBcBooks = new List<BCBook>();

        foreach (BCBook bcBook in listBcBook)
        {
            BookHistoryDto historyDto = new BookHistoryFacade().FindHistoryByBookIdByPriority(bcBook.id.ToString(CultureInfo.CurrentCulture));
            if (historyDto != null && historyDto.HistoryId > 0)
            {
                bcBook.Priority = historyDto.Priority;
                listPriorityBcBooks.Add(bcBook);
            }
            else
            {
                listNonPriorityBcBooks.Add(bcBook);
            }
        }
        int count = 0;
        int prio = 0;
        foreach (BCBook bcBook in listNonPriorityBcBooks)
        {
            for (int j = count + 1; j <= 5; j++)
            {
                List<BCBook> listBook = new List<BCBook>();
                listBook = ListBookPriority(listPriorityBcBooks, j, newList);
                count++;
                if (listBook.Count() > 0)
                {
                    prio++;
                }
                else
                {
                    break;
                }
                foreach (BCBook bc in listBook)
                {
                    newList.Add(bc);
                }
            }
            newList.Add(bcBook);
        }
    }
    catch (Exception ex)
    {
        ErrorLogger.WriteErrorToLog(ex);
    }

    return newList;
}
#endregion

#region ListBookPriority
private static List<BCBook> ListBookPriority(List<BCBook> list, int priority, List<BCBook> newList)
{

    List<BCBook> listBook = new List<BCBook>();
    try
    {
        foreach (BCBook vid in list)
        {
            if (!IsExists(newList, vid.id.ToString(CultureInfo.CurrentCulture)))
            {
                if (vid.Priority == priority)
                {
                    listBook.Add(vid);
                }
            }
        }
    }
    catch (Exception ex)
    {
        ErrorLogger.WriteErrorToLog(ex);
    }
    return listBook;
}
#endregion

I would get rid of foreach and use for. I have found (and read about) a huge performance difference between the 2 (EDIT, but not always, see below).

Example (untested code):

int c  = listBcBook.Count;

for (int i = 0; i < c; i++)
{
    BCBook bcBook = listBcBook[i];

    BookHistoryDto historyDto = new BookHistoryFacade().FindHistoryByBookIdByPriority(bcBook.id.ToString(CultureInfo.CurrentCulture));
    if (historyDto != null && historyDto.HistoryId > 0)
    {
        bcBook.Priority = historyDto.Priority;
        listPriorityBcBooks.Add(bcBook);
    }
    else
    {
        listNonPriorityBcBooks.Add(bcBook);
    }
}

EDIT: I just read that the performance difference isn't always there because in some cases, the compiler will optimize your foreach loops into for loops. Here is a good read over on SO: https://stackoverflow.com/questions/1124753/for-vs-foreach-loop-in-c.

In your case, because you're using a List, directly using for should be faster.


Finding nth lowest value without any Math max or min methods So I decided to make a more efficient solution to a Google interview question where they ask you to either find the lowest or second lowest value in an array without using the max or min methods. I believe it is more efficient because you can use my method to find any nth lowest value in the array so it solves for all cases.

Please let me know if there is anything I can do to improve this solution or if there is anything I am missing:

/*Algorithm:
-first sort the array from ascending order
-Then iterate over the array with a loop
-if(array[i + nth lowest input you want])
-return the output

function nThOrderStatistic(array, number):
 -if you want to find the lowest number sub in 0 for number
 -for every nth lowest number you would sub in n - 1 for the number */

function nThOrderStatistic(array, number) {
  array.sort(function(a,b){return a -b});
  for(i = 0; i < array.length; i++)
  {
    if(array[i + number] < array[i + (number + 1)])
  {
     return array[i + number];
    }
  }
}

nThOrderStatistic([6,7,43,2,95], 2);

//sample output for 3rd lowest value is: 7 //

I decided to make a more efficient solution to a Google interview question

The purpose of such interview questions is to get signal as to whether you understand asymptotic efficiency or not; your answer indicates you do not. It will also be looking for signal as to whether you understand the asymptotic performance characteristics of various data structures.

sort is O(n lg n) time complexity in the typical case, but max and min can be implemented in O(n) time complexity; whether you can recognize that or not, and write the code accordingly, is the test.

  I believe it is more efficient because you can use my method to find any nth lowest value in the array so it solves for all cases.

That's not the sort of "efficiency" they're looking for. The question they're asking is not "can you solve a more general problem, and therefore be more efficient in terms of lines of code written per problem solved?"  The question they are asking is "can you find the algorithm that makes more efficient use of machine resources?".

If you wanted to use this solution in a Google-style interview and impress the interviewers, what you need to do is to make the argument that your sorting solution is more efficient in terms of amortized costs.

Exercise: under what circumstances is the amortized cost of the "least n" query on an arbitrary unsorted array lower if you spend the time sorting the array once?

  Please let me know if there is anything I can do to improve this solution

Google-style interviewers will be looking primarily for signal as to whether you can write correct code, and provide test cases and logical arguments that demonstrate its correctness. Summing up:

You haven't found a correct solution.
You haven't made any argument for correctness.
You have provided a single test case.
You haven't found the asymptotically efficient solution.

This would be an easy "no hire" for a Google interviewer.

So, this gives you some things to concentrate on:

Solve the problem that was posed; if you feel the need to solve a more general problem, say why.
Have a good working understanding of asymptotic performance; I do not expect candidates to be able to rattle off the Master Theorem (I have had a grand total of one candidate in 20 years do so, and they did it wrong) but I do expect candidates to be able to tell me if an algorithm is sublinear, linear, superlinear, quadratic, exponential, and so on.
Follow good coding practices: don't mutate your inputs, for example.
Create a correct solution.
Make a cogent, clear, correct argument that explains why your solution is correct.
Provide test cases that convince the interviewer that you've thought about ways your algorithm could go wrong.


Rust Brainfuck interpreter I took the code from kostyas benchmarks for the Rust Brainfuck interpreter and tried to optimize it. There is also a discussion on Reddit about the poor performance of Rust in the Benchmark.

Before my improvements the code needed 16.81s to complete the benchmark and used 6.2Mb of memory, after that the code only needs 4.89s to run but the memory consumption is nearly unchanged.

How can I improve the code and maybe reduce memory usage?

The file with my changes is also on Github.

use std::fs::File;
use std::path::Path;
use std::io::prelude::*;
use std::vec::Vec;
use std::io;
use std::env;
use std::collections::BTreeMap;

struct Tape {
  pos: usize,
  tape: Vec<isize>
}

impl Tape {
  fn new() -> Tape { Tape { pos: 0, tape: vec![0] } }
  fn get(&self) -> isize { self.tape[self.pos] }
  fn getc(&self) -> u8 { self.get() as u8 }
  fn inc(&mut self) { self.tape[self.pos] += 1; }
  fn dec(&mut self) { self.tape[self.pos] -= 1; }
  fn advance(&mut self) { self.pos += 1; if self.tape.len() <= self.pos { self.tape.push(0) } }
  fn devance(&mut self) { if self.pos > 0 { self.pos -= 1; } }
}

struct Program {
  code: Vec<u8>,
  bracket_map: BTreeMap<usize, usize>
}

impl Program {
  fn new(content: Vec<u8>) -> Program {
    let mut code = Vec::new();
    let mut bracket_map = BTreeMap::new();
    let mut leftstack = Vec::new();

    for (pc, b) in content.iter().filter(|&&x| x == b'+' || x == b'-' || x == b'.' || x == b','
            || x == b'<' || x == b'>' || x == b'[' || x == b']').map(|&x| x).enumerate() {
      if b == b'[' {
        leftstack.push(pc);
      } else if b == b']' {
        if let Some(left) = leftstack.pop() {
          bracket_map.insert(left, pc);
          bracket_map.insert(pc, left);
        }
      }
      code.push(b);
    }
    Program{ code: code, bracket_map: bracket_map }
  }

  fn run(&self) {
    let mut pc: usize = 0;
    let mut tape = Tape::new();
    let mut stdout = io::stdout();

    while pc < self.code.len() {
      match self.code[pc] {
        b'+' => tape.inc(),
        b'-' => tape.dec(),
        b'>' => tape.advance(),
        b'<' => tape.devance(),
        b'[' => { if tape.get() == 0 { pc = self.bracket_map[&pc]; } },
        b']' => { if tape.get() != 0 { pc = self.bracket_map[&pc]; } },
        b'.' => { stdout.write(&[tape.getc()]).unwrap(); stdout.flush().unwrap() },
        _ => unreachable!()
      }
      pc += 1;
    }
  }
}

fn main() {
  let mut buf = Vec::new();
  {
    let arg1 = env::args().nth(1).unwrap();
    let path = Path::new(&arg1);
    let mut file = File::open(&path).unwrap();
    file.read_to_end(&mut buf).unwrap();
  }
  Program::new(buf).run();
}

More information: The program is compiled with rustc -C opt-level=3 brainfuck/brainfuck.rs -o brainfuck_rs, the memory usage is determined by a ruby script. C++ performs a lot better regading memory usage it only needs 1.6Mb.

Brainfuck rogram used for benchmark:

 Benchmark brainf*ck program
>++[<+++++++++++++>-]<[[>+>+<<-]>[<+>-]++++++++
[>++++++++<-]>.[-]<<>++++++++++[>++++++++++[>++
++++++++[>++++++++++[>++++++++++[>++++++++++[>+
+++++++++[-]<-]<-]<-]<-]<-]<-]<-]++++++++++.

I am using Rust nightly rust-nightly-bin-1.2.0_2015.06.06-1 on Arch Linux running on 4 Intel(R) Core(TM) i5-2450M CPU @ 2.50GHz.


Javascript parsing performance I am using some string which contains an html. As follows

var pagerHtml = 'No.Of Records<select class="rmselect" id="records" ><option value="10">10</option><option value="20">20</option><option value="40">40</option><option value="80">80</option><option value="100">100</option></select>';

Another is with " and escape sequence \" for " in the html as follows

var pagerHtml = "No.Of Records<select class=\"rmselect\" id=\"records\" ><option value=\"10\">10</option><option value=\"20\">20</option><option value=\"40\">40</option><option value=\"80\">80</option><option value=\"100\">100</option></select>";

And I am writing this html to a div using jquery

Which one gives the better performance while parsing?

Is there any performance difference on parsing escape sequence?

According to this jsperf test, there is no consistent difference between parsing a string literal with and without escape sequences.

There are differences depending on what browser you use, but they are quite small, and Chrome even seems to parse strings with escape sequences faster than strings without.


Insertion sort implementation in Python I came up with a small optimization while practice coding insertion sort in Python. It's mostly about not accessing the input array too much and letting the hardware do element shifts instead of swapping manually. Could someone comment on my coding style as well as the optimization idea?

Original Version

# this version of insertion sort is half as fast as selection sort on my machine
def runslow(input):

    for i in range(1,len(input)):

        j = i

        # list being accessed twice in this comparison
        while j > 0 and input[j] < input[j-1]:

            # unnecessary writes creating slowdown
            input[j-1], input[j] = input[j], input[j-1]

            j -= 1

Improved Version

# the optimized insertion sort
def run(input):

    for i in range(1,len(input)):

        j = i

        # extract the value being compared
        saved = input[i]

        # now list isn't being accessed twice unnecessarily
        while(j > 0 and input[j-1] > saved):

            # don't actually perform the intermediate swaps,
            # just let the cursor settle into the final position
            # input[j - 1], input[j] = input[j], input[j - 1]
            j-=1

        # now insert it and let the hardware do the actual shifting of the values in the list
        input.insert(j,input.pop(i))

    return input

TL;DR

Be as specific as you can:

When naming functions,
And when setting up loops.

Details:

Here's what I've noticed in your code, line-by-line:

def run(input)

Avoid common names for your functions. While it's not always possible to prevent having the same name as functions from other modules (there are methods to deal with that), naming a function run is asking for trouble. For example, a commonly used command in Python test-suites is also run.

Your function name should match as closely as possible the specific goal of the function. e.g. maybe call it insertion_sort.

for i in range(1,len(input)):

Try to use the most specific iterators possible.

The general construction for i in range(... gets used a lot where it doesn't have to be. Any time you see it, there is usually a better way iterate, whether using a list comprehension, using Itertools, or other more specific methods. In your case, replacing for i in range( with for i in input results helps a lot (as in my example below).

The next two lines j = 1 and saved = input[i] are only there to convert i from the non-specific `for I in range (...' to the values you actually want. They can be elimited by changing how the loop is set up.

    while(j > 0 and input[j-1] > saved):

The same goes for your while loop. You will never need a i+=1 (or j-=1) to increment a loop in Python. It's always best to think of what information the inside of the loop needs, and set up an iterator, list comprehension, or for loop that immediately provides that information.

Here is a modified version of your code, to show what I mean about being specific with loops:

def insertion_sort(input):
    for i,n in enumerate(input):
        ins_pos = 0               # default location to insert n
        for j,m in reversed([r for r in enumerate(input[:i]])):
            if m > n:
                ins_pos = j + 1   # insert n one position after first larger number found
                break
        input.insert(ins_pos, input.pop(i))
    return input

Once inside the loop, you only need:

i and j, the indices of the values to pop and insert.
n and m, the values to compare.

Once you know what the inside of the loop needs, set up the loop statements (e.g. In my example, for i,n in enumerate(input), and for m in reversed(input[i])) so they return only what is needed. This eliminates the extra lines in your code. Also, after this modification, the purpose of each loop is more clear.

    input.insert(j,input.pop(i))

PEP8 specifies spaces between arguments in function calls.

And a note on the last line:

Try not to modify a list that is the source of your iterator.

In general, it's a bad idea to modify the same list that is being iterated over. Your example only works because the pop and insert functions get applied to the parts of the input list that have already been read through by the iterator. If you need to do it in future, be aware of why this works, and how other examples of the same practice might go wrong. I've assumed that part of your spec is to modify the list in place, so I've left it how it is.

Performance:

Nice work with eliminating the extra slice steps. Re-writing the list with each step in the inner while loop is indeed more expensive than just comparing values and moving on. However, both pop, and insert within a list are both \$\mathcal{O} (n)\$, not \$\mathcal{O} (1)\$, sad but true. There may be a small speed-up of a constant factor, doing away with the slices, but both your versions of insertion sort are ultimately limited by the time complexity of the algorithm itself, which is \$\mathcal{O} (n^2)\$, where n is the length of the list.

This line:

    saved = input[i]

Won't change anything as, in Python, this sets up saved as a pointer to the value in input[i], rather than making a copy and storing it in saved. You could explicitly force a copy, but this would probably be self-defeating, as reading an index from an array is no more expensive than accessing any other int.


python : Collatz Sequence I am a beginner in python. I found this problem in the book Automate the Boring Stuff with Python. How can I improve this program using latest version of python.

def generateCollatzSequence(number):
    print(str(number) + " ")

    while number != 1:
        if number % 2 == 0:
            number = number / 2
            print(str(number) + " ")

        elif number % 2 == 1:
            number = (3 * number) + 1
            print(str(number) + " ")

#print("Enter number: ")
inputNumber = int(input("Enter number: "))
generateCollatzSequence(inputNumber)

Your function is called generate but it doesn't return anything. Instead display would be a better name.
It would be better if you had two functions one to generate the values, one to display them.
Function names should be lower_snake_case to be idiomatic.
+ " " is not needed, as print makes the value go to the next line anyway.
str(number) also isn't needed as print will do this automatically for you.

Thanks @Toby Speight:

You can move the print to be after both the if and the else.

def generate_collatz_sequence(number):
    output = [number]
    while number != 1:
        if number % 2 == 0:
            number = number / 2
        elif number % 2 == 1:
            number = (3 * number) + 1
        output.append(number)

def display_collatz_sequence(values):
    for number in values:
        print(number)

If you want this to still display values in real time, then you can make generate_collatz_sequence a generator function.

def generate_collatz_sequence(number):
    yield number
    while number != 1:
        if number % 2 == 0:
            number = number / 2
        elif number % 2 == 1:
            number = (3 * number) + 1
        yield number


PHP session_set_saver_handler with session timeout I have developed a class that utilises the session_set_saver_handler function so I can store sessions within my DB. The class works just as I would like.

However, my only concern is the way I have approached the session timeout.

Current the _read() function code looks like:

/**
     * Read session function
     * @access public
     * @return the 'data' record providinf the PDO statement executed correctly. Otherwise, return false.
     */
    public function _read($id)
    {

        $timeout = time() - $this->accessTime;
        $locked  = false;

        $this->database->query('SELECT updatedTime, data FROM sessions WHERE session = :id AND locked = :locked');
        $this->database->bind(':id', $id);
        $this->database->bind(':locked', $locked);

        if($this->database->execute())
        {
            if($this->database->rowCount() > 0)
            {
                $row = $this->database->singleResult();

                if($row['updatedTime'] < $timeout)
                {

                    //Set the location of the user.
                    $url = "http://" . $_SERVER['SERVER_NAME'] . $_SERVER['REQUEST_URI'];

                    if($url != $this->redirectUrl)
                    {
                        header('Location: ' . $this->redirectUrl);
                        return;
                    }

                    return '';
                }

                return $row['data'];
            }
        }

        return '';
    }

When I originally create the script I hard coded the redirect URL. The problem was logout.php (the file the user is redirected to) contains the session class. Meaning I have a constant loop. So I approached it by implementing the following:

if($row['updatedTime'] < $timeout)
                {

                    //Set the location of the user.
                    $url = "http://" . $_SERVER['SERVER_NAME'] . $_SERVER['REQUEST_URI'];

                    if($url != $this->redirectUrl)
                    {
                        header('Location: ' . $this->redirectUrl);
                        return;
                    }

                    return '';
                }

This seems to me more of a 'hack' than intelligent code.

Did I approach this correct (probably not)? If not, what would be a more intelligent work around?

Since the code for logout.php is not provided, I'll just make assumptions. Your code is pretty clean but I think it could be improved a bit.

Miscellaneous

Both locked and timeout are defined but only used once. Since they are local variables, I consider that they could be removed and just replaced by their values.

Still concerning the timeout, I prefer working with proper date and time types provided by MySQL rather than working with timestamps stored as integers. The main reason is that there are a lot of functions at your disposition, like for example NOW() and TIMESTAMPDIFF(), which could be used to rewrite your query easily

SELECT TIMESTAMPDIFF(MINUTE, NOW(), updatedTime ) AS minutesSinceLastActivity, data FROM sessions WHERE session = :id AND locked = :locked

Once you have that, your condition is pretty simple : if($row['minutesSinceLastActivity'] > $this->accessTime) . Which leads me to another point, the naming of accessTime is pretty bad, I would define it as something like minutesInactivityBeforeLogout.

One last thing that I'm a bit unsure about (I know it works for C, don't remember for PHP), you could regroup the two conditions if($this->database->execute()) and if($this->database->rowCount() > 0) into one with the and operator. Since you'll use an and, if the execution of the query fails, execution the rest of the code in the if condition is unnecessary since it cannot change the result of the condition. Once again, this is something I know works in C, not quite sure about PHP.

How it works

You said you had a constant redirection loop with a previous version of your code. The main reason I could fin for that is that if you detect a timeout, you don't do anything on your MySQL table, which means you'll always end up in the same condition and doing always the same thing.

That's why you should keep your structure and query as it is (you could put the timeout condition in the query, but doing so, you would be unable to differentiate the case where the timeout is reached from the case where there is no record for the current id). The only thing that you have to do is for example to delete the record if the timeout condition is reached.


Write ListItemCollection item to a file I am using below code to write list item to a file. I want only 2 fields from the list. Is there any direct way without looping through all the items to do that? Here serversToshutDown is a SharePoint ListItemCollection.

using (System.IO.StreamWriter file = new System.IO.StreamWriter("F:\test\Test.csv"))
{
    foreach (ListItem item in serversToshutDown)
    {
        context.Load(item);
        context.ExecuteQuery();
        String server = item["Server"].ToString();
        String service = item["Service"].ToString();
        Console.WriteLine(server + "," + service);
        file.WriteLine(server + "," + service);
    }
}

First of all "F:\test\Test.csv" should be escaped or a verbatim string:

new StreamWriter("F:\\test\\Test.csv")

Or:

new StreamWriter(@"F:\test\Test.csv")

Also you should not hardcode paths in your code but I suppose this is only because of posting here (but you'd better show your actual code cleaned from sensitive information).

StreamWriter defaults to UTF-8 encoding. It may be what you need but I usually like to make it explicit to anyone reading my code:

new StreamWriter(path, false, Encoding.UTF8)

In my opinion main issue here is that this function is doing too many things: loading required data, writing to output and logging to console.

If output file size is not too big you may start with this (type names are fictional and some functions need a better name, I don't have enough context to pick a proper one).

Let's first define a class for your server data, it's also responsible for loading from context. If your scenario will become more complex you may want to introduce a separate Loader class and keep Server as a POCO object.

Note: I don't know about SharePoint ListItem object (see also BCdotWEB's answer), if you already have everything you need in ListItem then you may drop Server object.

sealed class Server
{
    public Server(Context context, ListItem item)
    {
        context.Load(server);
        context.ExecuteQuery();

        Name = Convert.ToString(item["Server"]);
        Service = Convert.ToString(item["Service"]);
    }

    public string Name
    {
        get;
        private set;
    }

    public string Service
    {
        get;
        private set;
    }

    public override string ToString()
    {
        return $"{Name}, {Service}";
    }
}

Now you can simplify your function moving different responsibilities to different functions. Your public function has to implement current behavior but must be easy to extend/change (for example if logging isn't for console, if output file is not specified by its path and so on):

public static void WriteServersToShutDown(string path, Context context,
    IEnumerable<ListItem> serversToShutdown)
{
    if (context == null)
        throw new ArgumentNullException("context");

    var content = ConvertToString(LoadData(context, serversToShutdown));

    Log(content);
    WriteFile(path, content);
}

To load data is the basic reusable block of your code (for example if you also need to perform some action, not just to dump a log):

private static IEnumerable<Server> LoadData(Context context,
    IEnumerable<ListItem> serversToShutdown)
{
    return serversToShutdown.Select(x => new Server(context, x));
}

You also need a function to convert IEnumerable<Server> list into a string with one line for each entry. This is the function you will edit if output format will be changed. In this example I rely on Server.ToString() implementation but it's often a bad idea, for this kind of tasks I usually override ToString() for debugging/logging purposes and I move formatting responsibility to a separate IFormatProvider object.

private string ConvertToString(IEnumerable<Server> servers)
{
    return String.Join(Environment.NewLine, servers);
}

Few helper functions:

private static void Log(string text)
{
    Console.WriteLine(text);
}

private static void WriteFile(string path, string content)
{
    File.WriteAllText(path, content, Encoding.UTF8);
}

Everything may be rewritten as:

var content = String.Join(Environment.NewLine,
    serversToShutdown.Select(x => new Server(context, x)));

Console.WriteLine(content);
File.WriteAllText(path, content, Encoding.UTF8);

Then why so many functions? Because each function performs exactly one task and if you need to change anything you will need to change only one function. This may not be obvious in this simple example but it's important for bigger programs. Also note that smaller functions may be reused and you keep all the code in sync:

foreach (var server in LoadData(context, serversToShutdown))
    ForceShutdown(server);

If you need to introduce a factory class (above mentioned loader) you need to change only LoadData() function. Do you need to change output file encoding or to add some error handling? Just go to WriteFile() and all callers will be updated. You change log to something else instead of console? Pick Log() function. Above everything else there is readability: to understand what WriteServersToShutDown() does you need to inspect just three lines of self-explicative code (after you renamed those functions with proper names from your domain).


Euler Method Harmonic Oscillator I have written very simple Python code to solve the simple harmonic oscillator using Euler method, but I am not sure if the program is correct or not. I would be very grateful if anyone can look at my code and suggest further improvements since I am very new to this programming thing.

import matplotlib.pyplot as plt
v=0.0   #initial velocity
h=0.01  #time step
x=5.0   #initial position

t=0.0
ta,xa=[],[]

while t<1.0:
    ta.append(t)
    xa.append(x)

    v=v-(10.0/1.0)*x*h    #k=10.0, m=1.0
    x=x+v*h
    t=t+h
plt.figure()
plt.plot(ta,xa,'--')
plt.xlabel('$t(s)$')
plt.ylabel('$x(m)$')
plt.show()

The code is quite monolithic: calculation and presentation are separate concerns, and while I appreciate the context given by showing the presentation, it would be better to make clear that they're separate concerns by separating them with whitespace and commenting briefly.

Also on structure: separate constants (h) from variables (v, x, t).

    v=v-(10.0/1.0)*x*h    #k=10.0, m=1.0
    x=x+v*h
    t=t+h

Two things: firstly, what are k and m? It seems that they should be constants with comments explaining their physical significance.

Secondly, this isn't Euler's method. Euler's method is \$\vec{x}_{n+1} = \vec{x}_n + hf(t_n, \vec{x}_n)\$ where \$f = \frac{d\vec{x}}{dt}\$. Here \$\vec{x} = (v, x)\$ and \$f(t, v, x) = (\frac{dv}{dt}, \frac{dx}{dt}) = (-\frac{k}{m}x, v)\$. In other words, to be Euler's method you should update x according to the previous value of v, not the updated value of v. Since you're using Python, you can take advantage of simultaneous assignment:

    v,x=v-(k/m)*x*h,x+v*h
    t=t+h

(As it happens your buggy implementation works better than Euler's method, but if it was intended to implement Euler's method then it's still technically buggy).


Contact information validation The scripts below, I've obtained through tutorials and edited the best I know how. It all works - but I'd like to place them in one, external script. If at all possible, I'd appreciate some help in cleaning up the code too. I've a feeling some of it is redundant.

With the client-side validation script, I have "Name," "Email," "Phone," and "Message" set as input text values in the HTML. If they submit the form with those default values still in place, it gives an error. Also gives one when email is not valid or if phone is not actually a phone number. BUT the phone number is not required. So, the default value of "Phone" could pass through if nothing at all is entered there.

<script type="text/javascript">
$(document).ready(function() {
    $('nav a[href^="#"]').bind('click.smoothscroll',function (e) {
        e.preventDefault();
        var target = this.hash,
        $target = $(target);
        $('html, body').stop().animate({
            'scrollTop': $target.offset().top
        }, 900, 'swing', function () {
            window.location.hash = target;
        });
    });
    $('a[href=#top]').click(function(){
        $('html, body').animate({scrollTop:0}, 'slow');
        return false;
    });
    $(function() {
        $("nav select").change(function() {
            window.location = $(this).find("option:selected").val();
        });
    });
</script>
<script>
$(document).ready(function(){
    // Place ID's of all required fields here.
    required = ["name", "email", "message"];
    // If using an ID other than #email or #error then replace it here
    email = $("#email");
    phone = $("#tel");
    errornotice = $("#error");
    // The text to show up within a field when it is incorrect
    emptyerror = "Required field.";
    emailerror = "Invalid email.";
    nameerror = "Name";
    phoneerror = "Phone";
    messageerror = "Message";
    onlynumber = "Invalid phone.";

    $("#contact").submit(function(){
        //Validate required fields
        for (i=0;i<required.length;i++) {
            var input = $('#'+required[i]);
            if ((input.val() == "") || (input.val() == emptyerror) || (input.val() == nameerror) || (input.val() == messageerror)) {
                input.addClass("needsfilled");
                input.val(emptyerror);
                errornotice.fadeIn(750);
            } else {
                input.removeClass("needsfilled");
            }
        }
        // Validate the e-mail.
        if (!/^([a-zA-Z0-9_\.\-])+\@(([a-zA-Z0-9\-])+\.)+([a-zA-Z0-9]{2,4})+$/.test(email.val())) {
            email.addClass("needsfilled");
            email.val(emailerror);
        }

            if(phone.val() != phoneerror) {
        if (!/\(?([0-9]{3})\)?([ .-]?)([0-9]{3})\2([0-9]{4})/.test(phone.val())) {
                phone.addClass("needsfilled");
                phone.val(onlynumber);
    }
}

        //if any inputs on the page have the class 'needsfilled' the form will not submit
        if ($(":input").hasClass("needsfilled")) {
            return false;
        } else {
            errornotice.hide();
            return true;
        }
    });

    // Clears any fields in the form when the user clicks on them
    $(":input").focus(function(){
       if ($(this).hasClass("needsfilled") ) {
            $(this).val("");
            $(this).removeClass("needsfilled");
       }
    });
});
</script>
<script>
$(function(){
        $('input, textarea').each(function(){
            var txtval = $(this).val();
            $(this).focus(function(){
                $(this).val('')
            });
            $(this).blur(function(){
                if($(this).val() == ""){
                    $(this).val(txtval);
                }
            });
        });
    });
</script>

basically just cleaned up some redundant brackets, tags, added a comment and modified existing comments, fixed indentations etc. Then i ran it through JSLint(http://www.jslint.com/), enjoy :)!

$(document).ready(function() {
    $('nav a[href^="#"]').bind('click.smoothscroll',function (e) {
        e.preventDefault();
        var target = this.hash,
        $target = $(target);
        $('html, body').stop().animate({
            'scrollTop': $target.offset().top
        }, 900, 'swing', function () {
            window.location.hash = target;
        });
    });
    $('a[href=#top]').click(function(){
        $('html, body').animate({scrollTop:0}, 'slow');
        return false;
    });
    $(function() {
        $("nav select").change(function() {
            window.location = $(this).find("option:selected").val();
        });
    });

    //place ID's of all required fields here.
    required = ["name", "email", "message"];
    //if using an ID other than #email or #error then replace it here
    email = $("#email");
    phone = $("#tel");
    errornotice = $("#error");
    //the text to show up within a field when it is incorrect
    emptyerror = "Required field.";
    emailerror = "Invalid email.";
    nameerror = "Name";
    phoneerror = "Phone";
    messageerror = "Message";
    onlynumber = "Invalid phone.";

    $("#contact").submit(function(){
        //validate required fields
        for (i = 0; i < required.length; i++) {
            var input = $('#' + required[i]);
            if (input.val() == "" || input.val() == emptyerror || input.val() == nameerror || input.val() == messageerror) {
                input.addClass("needsfilled");
                input.val(emptyerror);
                errornotice.fadeIn(750);
            } else {
                input.removeClass("needsfilled");
            }
        }
        //validate the e-mail.
        if (!/^([a-zA-Z0-9_\.\-])+\@(([a-zA-Z0-9\-])+\.)+([a-zA-Z0-9]{2,4})+$/.test(email.val())) {
            email.addClass("needsfilled");
            email.val(emailerror);
        }

        //validate phone
        if(phone.val() != phoneerror) {
            if (!/\(?([0-9]{3})\)?([ .-]?)([0-9]{3})\2([0-9]{4})/.test(phone.val())) {
                phone.addClass("needsfilled");
                phone.val(onlynumber);
            }
        }

        //if any inputs on the page have the class 'needsfilled' the form will not submit
        if ($(":input").hasClass("needsfilled")) {
            return false;
        } else {
            errornotice.hide();
            return true;
        }
    });

    //clears any fields in the form when the user clicks on them
    $(":input").focus(function(){
       if ($(this).hasClass("needsfilled") ) {
            $(this).val("");
            $(this).removeClass("needsfilled");
       }
    });

    $(function(){
        $('input, textarea').each(function(){
            var txtval = $(this).val();
            $(this).focus(function(){
                $(this).val('')
            });
            $(this).blur(function(){
                if($(this).val() == ""){
                    $(this).val(txtval);
                }
            });
        });
    });
});


Split string at index field length or just before that if there is white space I had to write a function to split a string that goes into a field if it(the string) were too long for the field, as it would disappear and be unreadable.

How's the code lookin' ?

function splitText(instr, fieldSize) //instr = string to split into two lines, fieldSize = the width(in characters)of the field where the string will be showing
{
    if(instr.length>fieldSize) // Check if String requires splitting
    {
        //If Yes split
        var firstLine = instr.substring(0,fieldSize);
        var secondLine = instr.substring(fieldSize);
        //Check some arbitrary number of characters towards the end if there is whiye space(5) Obviously too much would not look too good on teh webSite
        var firstLeft = firstLine.substring(0,13);
        var firstRight = firstLine.substring(13);
        if(firstRight.indexOf(' ')>-1)
            {
                //If There is white space, break the line at it
               instr = firstLeft+firstRight.substr(0,firstRight.indexOf(' '))+" \n"+firstRight.substr(firstRight.indexOf(' ')+1);
            }
        else
            //else just split the string, according to the field size
            instr = instr.substring(0,fieldSize)+"\n "+instr.substring(fieldSize);
        }

    return instr;
}

Your indentation is a bit off here, with the true case of the if being indented one extra level. The else case is missing its brackets, which is probably the cause of confusion.

    var firstRight = firstLine.substring(13);
    if(firstRight.indexOf(' ')>-1)
        {
            //If There is white space, break the line at it
           instr = firstLeft+firstRight.substr(0,firstRight.indexOf(' '))+" \n"+firstRight.substr(firstRight.indexOf(' ')+1);
        }
    else
        //else just split the string, according to the field size
        instr = instr.substring(0,fieldSize)+"\n "+instr.substring(fieldSize);
    }

Aside from that, you call firstRight.indexOf(' ') three times, and you'd be better off placing this in a separate variable.


Messy Date Formatting There can only be better ways to do this?

create strings for  today's directory
strDate is yesterday's date in format ddmmyyyy (e.g. 08122016)
DateStr is yesterday's date in format yymmdd (e.g 161208)

strDate = '{d.year}{d.month:02}{d.day:02}'.format(d=datetime.datetime.now()- timedelta(1))
strDate = str(strDate)

day = calendar.day_name[datetime.datetime.today().weekday()-1]
day = day[:3]

week  = datetime.datetime.today().isocalendar()[1]

fpath = strDate + "_" + str(day)  + "_" +  str(week) + ".zip"

DateStr = '{d.day:02}{d.month:02}{d.year}'.format(d=datetime.datetime.now()- timedelta(1))
DateStr = str(DateStr[:4]+DateStr[6:])

Formatting dates

Simpler way to produce your required the formats is using the strftime symbols, for example:

# yesterday's date in format ddmmyyyy (e.g. 08122016)
strDate = '{:%d%m%Y}'.format(d - timedelta(1))

# yesterday's date in format yymmdd (e.g 161208)
DateStr = '{:%y%m%d}'.format(d - timedelta(1))

Formattings dates to use in filenames

I would strongly discourage using any of these formats in filenames.
Consider adopting the yyyymmdd format instead.
The good thing about this format is that the alphabetic ordering is the same as chronological ordering,
which is often very practical.

String conversions

The format function of a string returns a string.
So the second line here is unnecessary:

strDate = '{d.year}{d.month:02}{d.day:02}'.format(d=datetime.datetime.now()- timedelta(1))
strDate = str(strDate)

String concatenation

String concatenation like this is not recommended:

fpath = strDate + "_" + str(day)  + "_" +  str(week) + ".zip"

Better use the format function:

fpath = '{}_{}_{}.zip'.format(strDate, day, week)

Other Python conventions

The strDate and DateStr variable names go against the recommended naming convention.
I suggest to read PEP8, it explains this and other recommendations to follow.


All possible values that will match a regular expression This is a similar question to this, but I am looking for the set of all possible values that will match a regular expression pattern.

To avoid an infinite set of possible values, I am willing to restrict the regular expression pattern to a subset of the regular expression language.

Here's the approach I took (Python code):

def generate_possible_strings(pattern):
    '''
    input: 'K0[2468]'
    output: ['K02', 'K04', 'K06', 'K08']

    generates a list of possible strings that would match pattern
    ie, any value X such that re.search(pattern, X) is a match
    '''
    query = re.compile(pattern, re.IGNORECASE)
    fill_in = string.uppercase + string.digits + '_'

    # Build a re for a language subset that is supported by reverse_search
    bracket = r'\[[^\]]*\]' #finds [A-Z], [0-5], [02468]
    symbol = r'\\.' #finds \w, \d
    expression = '|'.join((bracket,symbol)) #search query

    tokens = re.split(expression, pattern)
    for c in product(fill_in, repeat=len(tokens)-1):
        candidate = ''.join(roundrobin(tokens, c)) #roundrobin recipe from itertools documentation
        if query.match(candidate):
            yield candidate

Supported subset of regular expressions language

Supports [] set of characters ([A-Z], [0-5], etc)
Supports escaped special characters (\w, \d, \D, etc)

Basically what this does is locate all parts of a regular expression that could match a single character ([A-Z] or [0-5] or [02468] or \w or \d), then for all of the valid replacement characters A-Z0-9_ test to see if the replacement matches the regular expression.

This algorithm is slow for regular expressions with many fields or if fill_in is not restricted to just A-Z0-9_, but at least it guarantees finding every possible string that will match a regular expression in finite time (if the solution set is finite).

Is there a faster approach to solving this problem, or an approach that supports a larger percentage of the standard regular expression language?

A major inefficiency in your solution is that you try every fill_in character as a replacement for any character class in the pattern. Instead, you could use the character class to select matching characters from fill_in and only loop over those.

>>> pattern = 'K0[2468]'
>>> re.findall(expression, pattern)
['[2468]']
>>> re.findall('[2468]', fill_in)
['2', '4', '6', '8']

For a more complete existing solution, you may want to look into these:

Regular expression parsing in Python
invRegex.py in Pyparsing examples


Sort data from a text file in descending order using quick sort I have a text file containing the following data.

ascon1 201707011 John 77.5 11.5 11.5 11.5
ascon1 201707012 Grld 70.0 11.5 11.5 11.5
ascon1 201707013 Josh 79.5 11.5 11.5 11.5
ascon1 201707014 Jess 67.5 11.5 11.5 11.5
ascon1 201707015 Jack 97.5 11.5 11.5 11.5

I need the data to look like this.

ascon1 201707015 Jack 97.5 11.5 11.5 11.5
ascon1 201707013 Josh 79.5 11.5 11.5 11.5
ascon1 201707011 John 77.5 11.5 11.5 11.5
ascon1 201707012 Grld 70.0 11.5 11.5 11.5
ascon1 201707014 Jess 67.5 11.5 11.5 11.5

so here is my code. The above part is working using the inbuilt sorted function from python. but in ascending order instead of decending order.

with open('input.txt') as f:
    lines = [line.split(' ') for line in f]
output = open("input(sorted).txt", 'w')

for line in sorted(lines, key=itemgetter(3)):
    output.write(' '.join(line))

output.close()

I want to use the quick-sort function to solve this problem. this is my function.

def qsort(d_val):
    if len(d_val) <= 1: return d_val
    return qsort([lt for lt in d_val[1:] if lt < d_val[0]]) + d_val[0:1] + \
           qsort([ge for ge in d_val[1:] if ge >= d_val[0]])

tl; dr

Use sorted(lines, key=itemgetter(3), reverse=True) instead of your custom function. This is both faster, more efficient, easier to read and easily understood by anyone.

Style

Your variable names doesn't convey much meaning. I don't get what the d in d_val can mean (data ?). In general, abreviations put unnecessary mental burden on the reader so avoid them. Do not hesitate to use more intermediate variable also as you can both:

cache reused values;
name your object and thus document your algorithm a bit more.

My take on this would be:

def quick_sort(values):
    if len(values) < 2:
        return values

    pivot, *values = values  # Python 3 syntax, use `pivot, values = values[0], values[1:]` on Python 2
    lower = [x for x in values if x < pivot]
    upper = [x for x in values if x >= pivot]
    return quick_sort(lower) + [pivot] + quick_sort(upper)

Performances

Each time you call qsort, you’re building 5 new lists: two for the intermediate values and one for the returned value plus 2 for each d_val[1:]. This quintuple the amount of memory needed by quick_sort call. Luckily for you, these are shallow copies which means inner lists are not copied, only their reference count increases.

Speed

Creating all these lists means a lot of allocations and/or copies happening under the hood, which slows down the whole thing. A simple sort of 2500 values timed using the following code:

from operator import itemgetter
import timeit

def qsort(d_val):
    if len(d_val) <= 1:
        return d_val
    return qsort([lt for lt in d_val[1:] if lt < d_val[0]]) + d_val[0:1] + \
           qsort([ge for ge in d_val[1:] if ge >= d_val[0]])

def qsort_test_case(l):
    return qsort(l)

def sorted_test_case(l):
    return sorted(l, key=itemgetter(3), reverse=True)

def generate_data():
    data = '''ascon1 201707011 John 77.5 11.5 11.5 11.5
ascon1 201707012 Grld 70.0 11.5 11.5 11.5
ascon1 201707013 Josh 79.5 11.5 11.5 11.5
ascon1 201707014 Jess 67.5 11.5 11.5 11.5
ascon1 201707015 Jack 97.5 11.5 11.5 11.5'''
    data = '\n'.join(data for _ in range(500))
    for line in data.splitlines():
        name, date, n, d1, d2, d3, d4 = line.split()
        yield name, date, n, float(d1), float(d2), float(d3), float(d4)

def main():
    print(timeit.timeit('test(l)', setup='from __main__ import generate_data, qsort_test_case as test; l = list(generate_data())', number=100))
    print(timeit.timeit('test(l)', setup='from __main__ import generate_data, sorted_test_case as test; l = list(generate_data())', number=100))

if __name__ == '__main__':
    main()

Yields a runtime of 18.58127073900141 seconds for your quick sort versus 0.03511061900098866 seconds for sorted. 530 times slower, I guess quick sort is not the right name there.

Alternate approach

Instead of creating intermediate lists, you could have an helper function that sort in-place but accept start and stop indices as arguments; and the recursive call will only change these values. This means you'll have to wrap the first call in a function that will perform a copy of the original list, call this helper to sort the copy in-place, and return the copy; leaving the original list untouched.

The advantage of this approach is that you can now use any iterable as input, not necessarily lists:

def quick_sort(iterable):
    result = list(iterable)
    _quick_sort(result, 0, len(result) - 1)
    return result

def _quick_sort(values, begin, end):
    if end <= begin:
        return
    # TODO: implement quick sort using swaps

Genericity

Your qsort function does not sort the array the same way than the sorted call in your original code. This is due to the fact that each item will sorted according to its first element then second if it compares equal (then third…) in your implementation (implicit rule for list comparisons); whereas you are using itemgetter(3) in the sorted call to compare only the fourth element.

Adding this kind of genericity to your quick sort will cost you time, impair readability and understanding of the code and can become error-prone at some point. Whereas when using sorted you don't have to worry about this kind of details that comes for free.


Remotely collect server data using Net::OpenSSH Perl I am newbie in Perl programming and currently trying to use Net::OpenSSH module in my code, my new code as below which the task is to run multiple command in remote server,

Code as below::

foreach $s (@servers) {
my $ssh = Net::OpenSSH->new("$username\@$s", timeout=>30);
$ssh->error and die "Unable to connect: " . $ssh->error;
print "Connected to $s\n";

my $fh = $ssh->capture("df -k /home/ | tail -1") or die "Unable to run command\n";
my @df_arr = split(/\s+/, $fh);
print "$s:  Disk space /home/ = $df_arr[3] \n";

my $fh1 = $ssh->capture("svmon -G -O unit=GB | grep memory") or die "Unable to run command\n";
my @sv_arr = split(/\s+/, $fh1);
print "$s:  Free memory = $sv_arr[3] \n\n";

close $fh;
undef $ssh;
}

This code is not so nice since I plan to simplify and reduce a line as many as possible.

Does there are any techniques or methods that I can use to simplify this code?

Looks good to me. Kudos for the "... or die" idiom. More informative output would be "unable to run df on $s" or "unable to run svmon on $s".

You don't have to do tail or grep on the far end, as a line of perl could do that. But it is a perfectly clean and sensible approach, which I advocate keeping.

If you wind up with many such monitoring commands, you may find it convenient to move the post-processing into perl commands that can be refactored out as utility functions.


Calculating potential Chess moves I started a Chess in Python for fun. I learned Python in my free time but I've been doing it for a while. The code works but I just want to know if there's anything non-pythonic in my code. The functions calculate all potentially possible moves (ignoring game state).

from itertools import product
from itertools import chain
from functools import wraps
from math import atan2

_angle_to_direction = {90: "up", -90: "down", 180: "right", 0: "left", -135: "right_down", 135: "right_up",
                       45: "left_up", -45: "left_down"}

def move(f):
    @wraps(f)
    def wrapper(*args, **kwargs):
        x, y = args
        moves = f(x, y)
        if (x, y) in moves: moves.remove((x, y))
        return moves

    return wrapper

def check_range(x:tuple) -> bool:
    return x[0] >= 0 and x[1] >= 0 and x[0] < 8 and x[1] < 8

@move
def _knight(x:int, y:int) -> set:
    moves = chain(product([x - 1, x + 1], [y - 2, y + 2]), product([x - 2, x + 2], [y - 1, y + 1]))
    moves = {(x, y) for x, y in moves if check_range((x, y))}
    return moves

@move
def _rook(x:int, y:int) -> set:
    return {(x, i) for i in range(0, 8)}.union({(i, y) for i in range(0, 8)})

@move
def _bishop(x:int, y:int) -> set:
    possible = lambda k: [(x + k, y + k), (x + k, y - k), (x - k, y + k), (x - k, y - k)]
    return {j for i in range(1, 8) for j in possible(i) if check_range(j)}

def direction(start:tuple, end:tuple) -> str:
    delta_a = start[1] - end[1]
    delta_b = start[0] - end[0]
    return _angle_to_direction[int(atan2(delta_a, delta_b) * 180 / 3.14)]

The implementation of your move decorator is a little schizophrenic. On the one hand, it carefully allows any and all arguments to be passed to it by declaring itself def wrapper(*args, **kwargs). However it then requires exactly two positional arguments which it then passes on to the wrapped function. Keyword arguments are accepted but ignored and dropped. I would suggest changing this to be all at one end or all at the other end of this spectrum. Given the annotations elsewhere, I would expect you to prefer changing the function to def wrapper(x, y) (or maybe to def wrapper(x, y, **kwargs) but then also passing **kwargs to f).

It might make sense to incorporate check_range into the wrapper as well, simplifying most of the move generators. It might also make sense to skip the decorator and instead call the filter in your move generators: return filter_move((x, y), moves).

As a side note, it would be interesting to see if there are any performance advantages to either the current approach of checking for (x,y) and conditionally removing it or to unconditionally doing a set difference: return f(x, y) - {(x, y)}.

The rest of your code is fairly straightforward, if perhaps a bit too clever. Clever is typically bad for other readers, or for debugging misbehavior. The two things that stand out to me is the name of your lambda possible (which seems more like a diagonal_moves_of_distance or simply diagonal), and the approach used for direction. Given the limited number of possible move deltas, bringing in trigonometry to handle it seems like overkill; I'd almost expect a simple if/else tree instead. As is, it will have problems classifying a knight's move.


Integers from scratch in Haskell Inspired by this article about natural numbers from first principles in swift I implemented integers from scratch in Haskell.

Besides obviously being extremely inefficient, is this code idiomatic Haskell?

import Data.List
import GHC.Real

data ℤ = Pred ℤ | Zero | Succ ℤ

toNat = toEnum :: Int -> ℤ
fromNat = fromEnum :: ℤ -> Int

toInteger' :: ℤ -> Integer
toInteger' Zero = 0
toInteger' (Succ n) = toInteger' n + 1
toInteger' (Pred n) = toInteger' n - 1

instance Show ℤ where

  show = ((++) "Nat: ") . show . toInteger'

data SimpleNat = MinusOne | One deriving (Eq, Ord, Show)

toList :: ℤ -> [SimpleNat]
toList Zero = []
toList (Succ n) = One : toList n
toList (Pred n) = MinusOne : toList n

fromList :: [SimpleNat] -> ℤ
fromList [] = Zero
fromList (One:xs) = Succ (fromList xs)
fromList (MinusOne:xs) = Pred (fromList xs)

normaliseList :: [SimpleNat] -> [SimpleNat]
normaliseList xs = normaliseSorted $ sort xs
                   where normaliseSorted xs = map fst filtered
                         filtered = filter (uncurry (==)) $ zip xs (reverse xs)

normalise :: ℤ -> ℤ
normalise = fromList . normaliseList . toList

instance Enum ℤ where

  succ (Pred n) = n
  succ n = Succ n

  pred (Succ n) = n
  pred n = Pred n

  toEnum n | n < 0 = Pred $ toEnum (n+1)
           | n > 0 = Succ $ toEnum (n-1)
           | otherwise = Zero

  fromEnum Zero = 0
  fromEnum (Succ n) = fromEnum n + 1
  fromEnum (Pred n) = fromEnum n - 1

instance Num ℤ where

  Zero + n = n
  n + Zero = n
  (Succ n) + m = Succ (n + m)
  (Pred n) + m = Pred (n + m)

  abs n = abs' $ normalise n
          where abs' (Pred n) = Succ (abs' n)
                abs' n = n

  signum n = signum' $ normalise n
             where signum' Zero = Zero
                   signum' (Succ n) = Succ Zero
                   signum' (Pred n) = Pred Zero

  negate n = negate' $ normalise n
             where negate' Zero = Zero
                   negate' (Succ n) = Pred (negate' n)
                   negate' (Pred n) = Succ (negate' n)

  fromInteger n | n < 0 = Pred $ fromInteger (n+1)
                | n > 0 = Succ $ fromInteger (n-1)
                | otherwise = Zero

  Zero * _ = Zero
  _ * Zero = Zero
  (Succ n) * m = n*m + m
  (Pred n) * m = n*m - m

instance Eq ℤ where

  n == m = normalise n `eq` normalise m
           where Zero `eq` Zero = True
                 (Succ n) `eq` (Succ m) = n `eq` m
                 (Pred n) `eq` (Pred m) = n `eq` m
                 _ `eq` _ = False

instance Ord ℤ where

  a `compare` b = normalise a `comp` normalise b
                  where Zero `comp` Zero = EQ
                        Zero `comp` (Succ _) = LT
                        Zero `comp` (Pred _) = GT
                        (Succ _) `comp` Zero = GT
                        (Succ _) `comp` (Pred _) = GT
                        (Pred _) `comp` Zero = LT
                        (Pred _) `comp` (Succ _) = LT
                        (Succ n) `comp` (Succ m) = n `comp` m
                        (Pred n) `comp` (Pred m) = n `comp` m

instance Real ℤ where

  toRational n = toInteger' n % 1

instance Integral ℤ where

  toInteger = toInteger'

  quotRem _ Zero = error "divide by zero"
  quotRem Zero _ = (Zero, Zero)
  quotRem n d | n == d = (Succ Zero,0)
              | n < d = (Zero, n)
              | otherwise = (Succ (fst foo), snd foo)
                          where foo = (n-d) `quotRem` d

This is some good looking code, there are just a few things that throw me. The first is that you're defining the integers but occasionally referring to them as "Nat"s. The natural numbers are non-negative integers, don't confuse the two.

toNat = toEnum :: Int -> ℤ
fromNat = fromEnum :: ℤ -> Int

This is a strange construction, give the type signature before the definition of the function and GHC will figure out which version of toEnum and fromEnum to use. Anything else is unusual and unnecessary.

Your Show instance should really just punt to the instance for Integers. Again, naturals numbers aren't integers so your "Nat: " tag is incorrect, it doesn't really add anything, and it makes writing a Read instance more difficult. Also there's no need to section infix functions by writing them prefix style. Thus:

instance Show ℤ where
    show = show . toInteger

instance Read ℤ where
    read = fromInteger . read

Your normalization function is more complex than it needs to be, but consider what having to normalize says about the representation you picked. Here's one version that doesn't change anything about your data types.

normalise = fromList . uncurry tailOfLonger . partition isOne . toList
    where
          isOne :: SimpleNat -> Bool
          isOne One = True
          isOne _   = False

          tailOfLonger :: [a] -> [a] -> [a]
          tailOfLonger    []     ys  = ys
          tailOfLonger    xs     []  = xs
          tailOfLonger (_:xs) (_:ys) = tailOfLonger xs ys

You have some unnecessary pattern match cases, such as n + Zero = n in the Num instance declaration. There's nothing wrong operationally with that case of course, but it's superfluous and the beauty of the inductive construction of the integers encourages me at least to be ruthless with flensing redundant code.

Here is an alternate construction that could obviate the need for all of the expensive normalization your version incurs. Writing the Num instance is pretty fun.

data Nat = Zero | Succ Nat

data Z = Negative Nat | NonNegative Nat


Tower of Hanoi solver Requesting code review, best practices, optimizations for this solution to the Tower of Hanoi problem.

final class PlatePair {

    private final String source;
    private final String destination;

    public PlatePair (String source, String destination) {
        this.source = source;
        this.destination = destination;
    }

    public String getSource() {
        return source;
    }

    public String getDestination() {
        return destination;
    }
}

public final class ToweOfHanoi {

    private ToweOfHanoi () { }

    /**
     * Solves the problem of tower of hanoi
     *
     * @param n     The number of plates
     * @return      The Set of sequential steps to take to solve the hanoi problem.
     */
    public static Set<PlatePair> solve(int n) {
        if (n <= 0) throw new IllegalArgumentException("The number of plates " + n  + " should be greater than zero.");

        final Set<PlatePair> platePair = new LinkedHashSet<PlatePair>();
        moveTowers(n, "A", "B", "C", platePair);
        return platePair;
    }

    public static void moveTowers(int n, String from, String using, String to, Set<PlatePair> platePair) {
        assert platePair != null;

        if (n == 0) return;

        moveTowers(n - 1, from, to, using, platePair);
        platePair.add(new PlatePair(from, to));
        moveTowers(n - 1, using, from, to, platePair);
    }

    public static void main(String[] args) {
        for (PlatePair getHanoi :  solve(3)) {
            System.out.println(getHanoi.getSource() + " : " + getHanoi.getDestination());
        }
    }
}

While your solution appears to work, and I cannot see any bugs, there are a number of best-practices that you are not employing in your solution. It is alsmost as if your code works by coincidence, rather than by design. There are a number of things that look like desperate attempts to make it work... and, like duct-tape, they work, but they are not pretty.

General

You have not described your algorithm in any detail. You have two recursive calls in your recursive method, and you have no documentation as to why. Recursion, in general, is relatively complex to understand, and can be unintuitive. You should help the person reading your code to understand.... and you do nothing. I had to debug the code to watch it happen to understand why you do things the way you do. This is not fair.

Naming conventions

Your Set<PlatePair> is called platePair. At minimum the variable name should be a plural, because, as it stands, it makes sense that platePair is a single PlatePair, and not a Set<PlatePair>. My preference would be 'moves' which indicates what the data represents

Oh, your class is called ToweOfHanoi, but should contain an 'r' as in TowerOfHanoi

Collections

Speaking of Set<PlatePair> ... really? Your actual implementation of the Set<PlatePair> is a LinkedHashSet. The LinkedHashSet will iterate the results in the same order they were inserted, which is why this solution actually works, but, you do not expose that property of the LinkedHashSet to the calling functions. There is nothing stopping a user from calling:

Set<PlatePair> result = new HashSet<>();
moveTowers(4, "A", "B", "C", result);

and this would produce crap results.

Frankly, you should not be using a set at all. You should be using a List.... because that is what it is, a List of moves to make.

This is your duct tape LinkedHashSet is a desperate solution to trying to bring order to a data collection that is designed to maintain unique contents (a Set) rather than an ordered collection of non-unique data (a List). Your moves are not unique... you often move a tile from tower A to tower C.... but, you use the the HashSet with unique move instances (instead of the neater move-reuse mechanism).

A more immediate solution is to change the moveTower from being a public method to being a private method, which you should do anyway. There is no need for it to be public. Making it private would also affect the assert in the method.... you assert that the platePair instance is not null, but you do not check the from/using/to instances. That is careless. If you are going to assert, then do it comprehensively. In this case, if the method is changed to be private, I would simply remove the asserts, and move on.... you do trust your own code, right?

PlatePair

Which comes down to the next problem, The PlatePair class. This class should have a number of differences. Firstly, the values should not be Strings, they should be an ENUM, one member for each tower.

public enum Tower {
    A, B, C;
}

public class PlatePair {
    private final Tower source, destination;

    public PlatePair(Tower source, Tower destination) {
        this.source = source;
        this.destination = destination;
    }

    public Tower getSource() {...}
    public Tower getDestination() {...}

}

I would also strongly encourage you to avoid creating so many PlatePair instances.... there are only 6 possible moves that can be made... each Tower can only be the source/destination of 2 possible moves. You should only have 6 PlatePair instances, and they should be immutable, and have good hashCode() and equals() methods.

Also, I don't like the class name PlatePair. It is not a pair of Plates, it is an ordered pair of Towers... a source, and a destination. I would rename it something like Transfer.

Conclusion

The code works, and gets the right answers. It does not read very well, and there are a few gotcha's in the code which mean you have to dig in to the code and understand the 'tricks' you are playing. The documentation/comments are really poor, and this code would fail any formal review in any sane business.

I have taken your code, and restructured it using the ideas I have shown above. This is closer to something that I would consider readable. Note, I have not changed the algorithm at all:

import java.util.ArrayList;
import java.util.EnumMap;
import java.util.List;

enum Tower {
    A, B, C;

    static {

        // make each tower know what moves are possible from that tower.
        // tower A can have a transfer to towers B, and C.
        // tower B can have a transfer to towers A, and C.
        // tower C can have a transfer to towers A, and B.
        // these 2 transfers per tower are available through the getMove(destination)
        // method for each tower.
        A.setupTransfers(B, C);
        B.setupTransfers(A, C);
        C.setupTransfers(A, B);

    }

    private EnumMap<Tower, Transfer> destinations = null;

    private void setupTransfers(Tower one, Tower two) {
        destinations = new EnumMap<Tower, Transfer>(Tower.class);
        destinations.put(one, new Transfer(this, one));
        destinations.put(two, new Transfer(this, two));
    }

    public Transfer getMove(Tower destination) {
        return destinations.get(destination);
    }
}

final class Transfer {

    private final Tower source;
    private final Tower destination;

    public Transfer(Tower source, Tower destination) {
        this.source = source;
        this.destination = destination;
    }

    public Tower getSource() {
        return source;
    }

    public Tower getDestination() {
        return destination;
    }

    @Override
    public int hashCode() {
        return (31 * source.hashCode()) ^ destination.hashCode();
    }

    @Override
    public boolean equals(Object obj) {
        if (obj == this) {
            return true;
        }
        if (obj instanceof Transfer) {
            return ((Transfer) obj).source == source
                    && ((Transfer) obj).destination == destination;
        }
        return false;
    }
}

public final class TowerOfHanoi {

    private TowerOfHanoi() {
    }

    /**
     * Solves the Tower of Hanoi problem
     *
     * @param n
     *        The number of plates on the starting tower
     * @return The sequential steps required to solve the problem.
     */
    public static List<Transfer> solve(int n) {
        if (n <= 0)
            throw new IllegalArgumentException("The number of plates " + n
                    + " should be greater than zero.");

        final List<Transfer> moves = new ArrayList<Transfer>();
        moveTowers(n, Tower.A, Tower.B, Tower.C, moves);
        return moves;
    }

    private static void moveTowers(int n, Tower from, Tower using, Tower to,
            List<Transfer> moves) {

        if (n == 0) {
            return;
        }

        // algorithm is:
        // we want to move the 'bottom' tile from the 'from' tower to the 'to'
        // tower.
        // we move everything on top of it to the 'using' tower
        // then we move the tile to the to tower,
        // then we move the things from the using tower back on top of the tile
        // we moved.

        // temporarily move everything on top to the using tower.
        moveTowers(n - 1, from, to, using, moves);
        // move the actual tile.
        moves.add(from.getMove(to));
        // move the temp stuff back on to the tile we just moved.
        moveTowers(n - 1, using, from, to, moves);
    }

    public static void main(String[] args) {
        for (final Transfer getHanoi : solve(4)) {
            System.out.println(getHanoi.getSource() + " : "
                    + getHanoi.getDestination());
        }
    }
}


DiagnosticAnalyzer for Roslyn that guards against catch-all exception clauses Dabbling around with Roslyn and made a small analyzer just now. This one will show a warning in Visual Studio when you have a try-catch statement that only has a catch(Exception e).

I realize the working code (AnalyzeNode) is rather small, but I'm looking for feedback on best-practices (insofar there are already best practices established) and general remarks on scenarios that I might have overlooked.

I have also been looking for a way to unit test this, but haven't come up with a good solution yet. Is there an elegant way to test these analyzers instead of looking over them by hand? Or perhaps an API that exposes some crude methods which I could provide a wrapper for?

Analyzer

[DiagnosticAnalyzer]
[ExportDiagnosticAnalyzer(DiagnosticId, LanguageNames.CSharp)]
class SingleGeneralExceptionAnalyzer : ISyntaxNodeAnalyzer<SyntaxKind>
{
    private const string DiagnosticId = "SingleGeneralException";
    private const string Description = "Verifies whether a try-catch block does not contain just a single Exception clause.";
    private const string MessageFormat = "A catch-all clause has been used.";
    private const string Category = "Exceptions";
    private static readonly DiagnosticDescriptor Rule = new DiagnosticDescriptor(DiagnosticId, Description, MessageFormat, Category, DiagnosticSeverity.Warning);

    public ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics
    {
        get
        {
            return ImmutableArray.Create(Rule);
        }
    }

    public ImmutableArray<SyntaxKind> SyntaxKindsOfInterest
    {
        get
        {
            return ImmutableArray.Create(SyntaxKind.CatchClause);
        }
    }

    public void AnalyzeNode(SyntaxNode node, SemanticModel semanticModel, Action<Diagnostic> addDiagnostic, CancellationToken cancellationToken)
    {
        var clause = node as CatchClauseSyntax;
        var exceptionType = clause.Declaration.Type;
        var identifier = semanticModel.GetSymbolInfo(exceptionType);
        var isGeneralException = identifier.Symbol.Name == typeof(Exception).Name;
        var hasMultipleClauses = clause.Parent.ChildNodes().OfType<CatchClauseSyntax>().ToList().Count > 1;

        // Less-specific exceptions can't be caught after a more general exception, or a compile error occurs
        // Therefore we don't need to look at the order of the clauses

        if (isGeneralException && !hasMultipleClauses)
        {
            addDiagnostic(Diagnostic.Create(Rule, clause.Declaration.GetLocation()));
        }
    }
}

Testclass

class SingleExceptionClauseAnalyzer
{
    void SingleException_ShouldCause_Warning()
    {
        try
        {
            int x = Int32.Parse("5");
        }
        catch (Exception e)
        {
            int x = 8;
        }
    }

    void MultipleExceptions_ShouldNotCause_Warning()
    {
        try
        {
            int x = Int32.Parse("5");
        }
        catch (FormatException e)
        {
            int x = 7;
        }
        catch (Exception e)
        {
            int x = 8;
        }
    }

    void SingleException_WithFullyQualifiedName_ShouldCause_Warning()
    {
        try
        {
            int x = Int32.Parse("5");
        }
        catch (System.Exception e)
        {
            int x = 8;
        }
    }

    void MultipleExceptions_WithFullyQualifiedName_ShouldNotCause_Warning()
    {
        try
        {
            int x = Int32.Parse("5");
        }
        catch (System.FormatException e)
        {
            int x = 7;
        }
        catch (System.Exception e)
        {
            int x = 8;
        }
    }
}

I've never coded a diagnostics analyzer, so I don't know if that's a possibility, but I think these:

private const string DiagnosticId = "SingleGeneralException";
private const string Description = "Verifies whether a try-catch block does not contain just a single Exception clause.";
private const string MessageFormat = "A catch-all clause has been used.";
private const string Category = "Exceptions";

Would be better off defined in a .resx file, so you can localize it - not everyone runs an English IDE, I'd try to have the the messages be shown in the same language as the stack traces.

In these declarations:

    var clause = node as CatchClauseSyntax;
    var exceptionType = clause.Declaration.Type;
    var identifier = semanticModel.GetSymbolInfo(exceptionType);
    var isGeneralException = identifier.Symbol.Name == typeof(Exception).Name;
    var hasMultipleClauses = clause.Parent.ChildNodes().OfType<CatchClauseSyntax>().ToList().Count > 1;

I think the relationships between the variables would be more obvious with some vertical whitespace:

    var clause = node as CatchClauseSyntax;

    var exceptionType = clause.Declaration.Type;
    var hasMultipleClauses = clause.Parent.ChildNodes()
                                          .OfType<CatchClauseSyntax>()
                                          .ToList().Count > 1;

    var identifier = semanticModel.GetSymbolInfo(exceptionType);
    var isGeneralException = identifier.Symbol.Name == typeof(Exception).Name;

I believe there's a possible execution path where the clause would be null (because of the as cast), in which case the next line would throw an easily avoidable NullReferenceException:

    var clause = node as CatchClauseSyntax;
    if (clause == null) return;


Determining the existence of string Intersections Problem:

  Given 2 strings, consider all the substrings within them
  of length len. Len will be 1 or more.
  Returns true if there are any such substrings which appear in both strings.
  Compute this in linear time using a HashSet.

Solution:

import java.util.HashSet;

public class Standford3 {
    public static void main(String[] args) {
        System.out.println("Test 1: " +
            (false  == stringIntersect("blahblah", "foralheh", 3))
        );
        System.out.println("Test 2: " +
            (true  == stringIntersect("checking", "deck", 2))
        );
        System.out.println("Test 3: " +
            (false  == stringIntersect("derping", "slurp", 3))
        );
        System.out.println("Test 4: " +
            (false  == stringIntersect("foo", "bar", 1))
        );
        System.out.println("Test 5: " +
            (true  == stringIntersect("nowai", "55&dcsnow", 3))
        );
    }

    public static boolean stringIntersect(String a, String b, int len) {
        if (a.length() == 0 || b.length() == 0) { return false; }

        HashSet<String> alpha = permutateString(a, len);
        HashSet<String> beta = permutateString(b, len);

        for (String s : alpha) {
            if (beta.contains(s)) { return true; }
        }

        return false;
    }

    public static HashSet<String> permutateString(String str, int i) {
        if (i > str.length()) {
            throw new IllegalArgumentException(
                "Substring length cannot be larger than provided string"
            );
        }

        HashSet<String> set = new HashSet<>();
        int count = i;

        for (int j = 0; j < str.length(); j++ ) {
            if (count > str.length()) { break; }

            set.add(str.substring(j, count));
            count++;
        }

        return set;
    }
}

Are these tests sufficient? Unlike the two challenges that preceded it, the tests are mine; is there anything I should be testing for that I missed?
I hope this isn't outside of codereview territory, but I'm wondering if something more was meant by "compute in linear time" or does this adequately encompass that requirement?
Although the use of HashSet was explicitly cited, I'm wondering if there are performance advantages in using another built in class like LinkedHashSet?

Some simple suggestions for refactoring on your current code (without going into the implementation)

Use Set over HashSet

Your original question says "Compute this in linear time using a HashSet.", but that usually doesn't mean you need to use HashSet as the return type. Set<String> result = new HashSet<>() works better in the sense that callers of your method will not need to know the underlying implementation. Also, you'll be free to change it to LinkedHashSet without changing the method signature.

Variable names

Two points here.

i is usually used in for loops, so do consider using a better name in the method signature, e.g. permutateString(String str, int length).

Your count isn't actually counting anything, but to keep track of the last exclusive index for substring-ing (I think I just made a new word up!).  For people who don't use String.substring() often, that line of code will instead read like you are creating sub-strings of increasing lengths.

Looping

for (int j = 0; j < str.length(); j++ ) {
    if (count > str.length()) { break; }
    ...
}

This can be better expressed as:

for (int j = 0; j <= str.length() - length; j++ ) {
    result.add(str.substring(j, j + length));
}

Illustration:

1234567890
123
 234
  345
   456
    567
     678
      789
       890

So, to extract 3-character sub-strings from a 10-character string, we need to loop from i = 0 to i = 7, or i <= string.length() - length for the latter.
Unit testing

As mentioned elsewhere, please consider using unit testing frameworks like JUnit or TestNG. :)


Ruby on Rails text editor I'm currently building a text editor using contenteditable and RoR back end. As the editor is WYSIWYG and trying to follow DRY, I used only one file to 3 actions: Show, Edit and New.

Edit and new will pretty much be the same, however, show won't be editable and won't show the Subtitle if it's empty. I was doing it all through JavaScript, but it didn't feel right to me, so I decided to make it using rails.

This is the final result. 0 Javascript, all rails. It is working now, but it's not DRY. There are lots of repeated conditions.

How can this code be more DRY and human-readable?

%section#text-editor-container
  %article#new-text
    %section#text-header.new-text-container.row{style: ("background: url('#{@text.text_images.last.photo.url}') center center no-repeat transparent; background-size: cover;" unless @text.text_images.empty?)}
      - if (current_page?(edit_text_url(@text)) || current_page?(new_text_url))
        .picture-icon.hidden-xs.col-sm-1.col-md-1.col-lg-1
          = render 'direct_upload', callback: text_cover_url
      %h1#title.col-xs-12.col-sm-11.col-md-11.col-lg-11.placeholdify{class: ('editable' if current_page?(edit_text_url(@text)) || current_page?(new_text_url)), contenteditable: (true if current_page?(edit_text_url(@text)) || current_page?(new_text_url)), data: {placeholder: t(:title)}}= raw(@text.title) unless @text.title.nil?
      - if (((@text.subtitle.nil? || @text.subtitle.empty?) && (current_page?(edit_text_url(@text)) || current_page?(new_text_url))) || !@text.subtitle.empty?)
        %h2#subtitle.col-xs-12.col-sm-11.col-md-11.col-lg-11.placeholdify{class: ('editable' if current_page?(edit_text_url(@text)) || current_page?(new_text_url)), contenteditable: (true if current_page?(edit_text_url(@text)) || current_page?(new_text_url)), data: {placeholder: t(:sub_title)}}= raw(@text.subtitle) unless @text.subtitle.nil?
    %section#text-commands-bar.new-text-container
      -if @text.is_draft
        %span.pull-left= t(:text_still_draft)
      %ul.pull-right.options
        - if (current_page?(new_text_url) || current_page?(edit_text_url(@text)))
          %li.drop_down_menu#menu_categoria.text-center
            = select("text", "category_id", Category.all.collect { |c| [t(c.locale_key.to_sym), c.id]}, {include_blank: t(:select_cat)})
          %li.drop_down_menu#menu_lingua.text-center
            = select("text", "language_id", Language.all.collect { |l| [l.name, l.id] }, {include_blank: t(:select_lan)})
          %li
            %button#publish= t(:publish)
          %li.drop_down_menu#menu_salvar.text-center
            = link_to t(:save_draft), @text, class: 'btn'
        - elsif (!current_page?(new_text_url) && !current_page?(edit_text_url(@text))) && (!@current_user.nil? && @current_user.id == @text.user_id)
          %li
            = link_to '', edit_text_path(@text), class: 'btn icon-editor edit-icon'
          %li
            = link_to '', text_path(@text), method: :delete , data: {confirm: t(:exclusion_confirm)},class: 'btn icon-editor delete-icon'

    %section#text-body.new-text-container
      %p#text.placeholdify{class: ('editable' if current_page?(edit_text_url(@text)) || current_page?(new_text_url)),contenteditable: (true if current_page?(edit_text_url(@text)) || current_page?(new_text_url)), data: {placeholder: t(:new_text)}}= @text.description.html_safe unless @text.description.nil?
= hidden_field_tag 'my_text', @text.id

There's two things I usually consider in this kind of situation:

Use custom helpers and partials

The idea here is to break down your view in small components, and have one helper to build each component. Most of the time those helpers will just dispatch and render an appropriate partial :

def wysiwyg_title(text_object)
  if text_object.persisted?
    render 'components/wysiwyg/title/persisted', text: text_object
  else
    render 'components/wysiwyg/title/new_record', text: text_object
  end
end

sometimes you can also DRY it up a little by using 'peacemeal' helpers that only affect parts of a tag :

def wysiwyg_container_classes(text_object)
  classes = ['wysiwyg_container']
  classes << 'wysiwyg_container--draft' unless text_object.persisted?
  classes
end

(use it like that:)

= content_tag :div, class: wysiwyg_container_classes(@text)

and so on. You get the spirit.

Use presenters

Another option is to use the presenter pattern. The Draper gem, for instance, makes it easy.

The idea here is that instead of passing your model directly to the view, you decorate it with the appropriate presenter. You'll have for instance one presenter for the editable view, and one for the static view. Each presenter will have methods like title, container, etc. that know how to render a component of your view, format the values, etc. your actual view will simply be a layout that arranges the components.

As presenters are objects, you are free to use inheritance / composition / delegation to share some logic, which often helps DRYing the code while keeping it clear and readable.


301 redirect using .htaccess I wonder if the following 301 redirect is the best way for my situation. I used this post on SO to give me the following 301 redirect:

RewriteRule locations\.php$ compare-biomass.php [R=301]

What we've done is changed a filename from locations.php to compare-biomass.php.

The new URL should be:

  www.mydomain.com/compare-biomass.php?variable1=1&variable2=2&variable3=3.

The above SO post mentioned that the old query string gets appended to the new filename, which is exactly what I want. However, I found that I was being redirected so that SERVER["DOCUMENT_ROOT"] was being appended to the domain, so the URL was:

  www.mydomain.com/home/mydomain/public_html/compare-biomass.php?variable1=1&variable2=2&variable3=3

I worked out that I had to include the following in my .htaccess file

RewriteBase /

However, now that I've had a problem and this is the very first time I'm doing anything related to Apache, can you please tell me if the .htaccess file I'm using is optimal for me:

Options +FollowSymLinks
RewriteEngine on
RewriteCond %{HTTP_HOST} ^mydomain.com [NC]
RewriteRule ^(.*)$ http://www.mydomain.com/$1 [R=301,L]
RewriteBase /
RewriteRule locations\.php$ compare-biomass.php [R=301]

Since the RewriteBase directive applies to the entire .htaccess file, it should not be buried between two RewriteRules.

You don't need capturing parentheses for the RewriteCond.

Options +FollowSymLinks

RewriteEngine on
RewriteBase /

RewriteCond %{HTTP_HOST} ^example.com [NC]
RewriteRule .* http://www.example.com/$0 [R=301,L]

RewriteRule ^locations\.php$ compare-biomass.php [R=301]


Printing a triangle of stars I've got a rather long function that will take the size of a triangle print it with stars and give the user the option of whether or not they want stars printed inside the triangle, and the orientation (point up or down).  I was hoping for some general refactoring methodologies I could apply.

Every permutation is possible for inverted/filled, inverted/not filled, etc.  What are some good general methods I can apply here to get some more code reuse?

void Triangle::Print(string direction, string fill)
{
    string starWithSpace = "* ";

    for (int i = leg1 - 1, j = 1; i >= 0; i--, j++, j < i)
    {
        if (direction == "vertical")
        {
            cout << string(i, ' ');

            if (fill == "yes")
            {
                int repeat = 0;
                while (repeat < j)
                {
                    cout<< starWithSpace;
                    repeat++;
                }
            }
            else if (fill == "no")
            {
                if (j * 2 - 3 < 0)
                {
                    cout << "*";
                }

                if ( j > 1 && i >= 1)
                {
                    cout << "*";
                    cout << string(((j * 2 - 3) < 0) ? 1 : j * 2 - 3, ' ');
                    cout << "*";

                }
                if (i == 0)
                {
                    int repeat = 0;
                    while (repeat < leg1)
                    {
                        cout << starWithSpace;
                        repeat++;
                    }
                }

            }
            cout << "\n";
        }
        else if (direction == "inverted")
        {
            //print bottom line
            if (j == 1)
            {
                int repeat = 0;
                while (repeat < leg1)
                {
                    cout << starWithSpace;
                    repeat++;
                }
            }
            cout << string(j - 1, ' ');
            if (j != 1 && fill == "yes")
            {
                int repeat = 0;
                while (j >1 && repeat < i +1)
                {
                    cout << starWithSpace;
                    repeat++;
                }
            }
            else if (fill == "no" && j!= 1)
            {
                int skipFactor = i * 2 - 1;
                cout << "*";
                cout << string(skipFactor > 0 ? skipFactor: 1, ' ');
                if (i != 0)
                {
                    cout << "*";
                }
            }
            cout << "\n";
        }
    }
    cout << "\n";
}

These comments apply to your question, not your answer.

Using strings as arguments seem unnecessary, it would be better to use bools or enums, that way if you one day change strings (e.g. another language) it does not affect the print function.

void Triangle::Print(Direction direction, bool fill)

There are magic numbers in your code 2,3 etc. try using constants for those
values with descriptive names.

The function is arguably a bit long and has quite a few if-statement levels, better to split into several functions that you can parameterize.

You may also want to present public functions to the user that do not have all the arguments then leave the original Print protected e.g.

PrintVertical(bool fill)
PrintInverted(bool fill);
...

that may make its use less confusing than keeping track of all possible argument combinations.

You may also want to include the stream to print to as an argument to Print, that way if you can alternatively print to a file instead of cout.


tic tac toe code in c++ with two players My main problems are making draw case and any player can choose a place which is chosen by another player here is the code :
#include <iostream>
using namespace std;

char board[3][3] = {{'1', '2', '3'}, {'4', '5', '6'}, {'7', '8', '9'}};
static int turnnumber = 1 ;
bool winner = false, flag = false ;
bool win(){
     if(board[0][0]==board[1][1]&&board[1][1]==board[2][2])
    winner = true ;
    if(board[0][2]==board[1][1]&&board[1][1]==board[2][0])
    winner = true ;
if(board[1][0]==board[1][1]&&board[1][1]==board[1][2])
    winner = true ;
if(board[0][0]==board[0][1]&&board[0][1]==board[0][2])
    winner = true ;
if(board[2][0]==board[2][1]&&board[2][1]==board[2][2])
    winner = true ;
if(board[0][0]==board[1][0]&&board[1][0]==board[2][0])
    winner = true ;
if(board[0][1]==board[1][1]&&board[1][1]==board[2][1])
    winner = true ;
if(board[0][2]==board[1][2]&&board[1][2]==board[2][2])
    winner = true ;

    if(winner==true&&turnnumber==1)
       cout << "player2 won \n\n" ;
    if(winner==true&&turnnumber==2)
       cout << "player1 won \n\n" ;

    return winner;
}

void view()
{
    for (int i = 0; i < 3; i++)
    {
        for (int x = 0; x < 3; x++)
        {
            cout << "[" << board[i][x] << "]  ";
        }
        cout << endl
             << "-------------" << endl;
    }
}

void players()
{

    char player1 = 'X', player2 = 'O';
    int number;
    cout << "\nplayer " << turnnumber << " it's your turn ";
    if(turnnumber==1)
       turnnumber++;
    else if(turnnumber==2)
       turnnumber--;
    char player;
    if(turnnumber==1)
       player=player2;
    if(turnnumber==2)
       player=player1;
    cin >> number ;

    switch(number){

    case 1:
        board[0][0] = player;
        break;
    case 2:
        board[0][1] = player;
        break;
    case 3:
        board[0][2] = player;
        break;
    case 4:
        board[1][0] = player;
        break;
    case 5:
        board[1][1] = player;
        break;
    case 6:
        board[1][2] = player;
        break;
    case 7:
        board[2][0] = player;
        break;
    case 8:
        board[2][1] = player;
        break;
    case 9:
        board[2][2] = player;
        break;
    default:
        cout << "\nwrong number\n";
        players();

    }
    system("cls");
    view();

    if(!win())
        players();
}

int main()
{
    view();
    players();
}

You can address both of the your problems using "struct".
By making something like this:
struct myBorad
{
    char board[3][3] = { {'1', '2', '3'}, {'4', '5', '6'}, {'7', '8', '9'} };
    bool bMyBoardStatus[9] = { false };
};
myBorad g_myBoard;

You then need to change each of the fields as "taken" by setting them to "true".
after each player play.
g_myBoard.bMyBoardStatus[0] = true;

Now that you have this in place you can add two ifs:

Check if the status of selected by player place is "taken" aka not false.
Check if all the array aka all nine of them are true and you didn't meet the win condition yet that means its draw.

there are more elegant solutions but this will help you get what you need, and this does not have to be in struct. i prefer this way.