Automatic flowchart generator I wanted to see graphically what my programme was doing so I wrote this automatic flowchart generator:

class Object

    def log(previous_method)
        if not DEBUGGING
            return
        end
        repr = if self.class == Enumerator then self.to_a else self end
        if [Array, String].include?(repr.class) and repr.length > 100
            repr = repr.first(20) + [". . ."] + repr.last(20)
        end
        if previous_method.downcase == 'start'
            self.tap {|x| puts """
                   #{repr}
                   """}
        else
            self.tap {|x| puts """
                     |
                     |     #{previous_method}
                     |
                     V

                   #{repr}
                   """}
         end
     end
end

Some testcases:

class Integer
    def is_digits_pow_sum?(exp)
        self                           .log("start")
            .digits                    .log("Digits")
            .map{|x| x**exp}           .log("Each digit to the power of #{exp}")
            .sum                       .log("Sum")
            .equal?(self)              .log("Equals the start? (#{self})")
    end
end

441.is_digits_pow_sum?(3)

Generates the flowchart:

               441

                 |
                 |     Digits
                 |
                 V

               [4, 4, 1]

                 |
                 |     Each digit to the power of 3
                 |
                 V

               [64, 64, 1]

                 |
                 |     Sum
                 |
                 V

               129

                 |
                 |     Equals the start? (441)
                 |
                 V

               false

class String
    def alpha_value
        self                           .log("Start")
            .chars                     .log("Split char by char")
            .map(&:alphabet_position)  .log("Map by position in the alphabet")
            .sum                       .log("Sum")
    end
end

"hellopeople".alpha_value

Gives the flowchart:

               hellopeople

                 |
                 |     Split char by char
                 |
                 V

               ["h", "e", "l", "l", "o", "p", "e", "o", "p", "l", "e"]

                 |
                 |     Map by position in the alphabet
                 |
                 V

               [8, 5, 12, 12, 15, 16, 5, 15, 16, 12, 5]

                 |
                 |     Sum
                 |
                 V

               121

You're a Python native, aren't you? I can tell from the """ multiline strings and the four-space indents and really unidiomatic whitespacing and if/then/else/end instead of ternary and if not instead of unless and use of blocks instead of statements and wow this is really unidiomatic ;-;

Each tip assumes that you've already applied the last.

Also, I'm writing this review from the perspective of Ruby 2.2.2 -- some methods that existed when you wrote this don't exist any more, and some methods that do what you want better have been created. I suggest looking through the Ruby docs for the updated methods, since there's no way I caught everything.

Ruby indents are two spaces, not four.
You may have noticed a bug -- even though you seem to be specifying a single multiline string, it's printing out a bunch of newlines between the beginning and the end. That's because """ [stuff] """ is interpreted not as a single string but as three: "", " [stuff] ", and "". Then, it outputs each of those on a new line, which results in a bunch of extra spacing. It also doesn't eliminate the indentation, so it ends up being moved really far to the right, too.

To fix both issues, you can use a heredoc:

else
  self.tap {|x| puts <<-END.gsub(/^\s*\||#@.*$/, '')
                  |
                  |    |
                  |    |    #{previous_method}
                  |    |
                  |    V
                  |
                  | #{repr}
                END}
end

In multiline blocks, use do/end instead of {/}:

else
  self.tap do |x|
    puts <<-END.gsub(/^\s*\||#@.*$/, '')
      |
      |    |
      |    |    #{previous_method}
      |    |
      |    V
      |
      | #{repr}
    END
  end
end

Actually, why are you using self.tap at all? Just do the puts, then return self after:

else
  puts <<-END.gsub(/^\s*\||#@.*$/, '')
    |
    |    |
    |    |    #{previous_method}
    |    |
    |    V
    |
    | #{repr}
  END
end
self

In Ruby, we have the ternary operator (condition ? a : b). Use it instead of the one-line if/then/else/end construction:

repr = (self.class == Enumerator) ? self.to_a : self

But even that's unnecessary -- firstly, to_a returns an Array, not an Enumerable. Secondly, calling to_a on an Array will return the unmodified array, so you can just write:

repr = self.to_a

Of course, this is a bad idea. What happens when someone tries to use your .log with an IO? Should the entire contents of the IO be put into an array? That seems really stupid to me. Personally, I would instead have

repr = self.to_s

Since, after all, you'll be printing it, and it's far better to let the designers of an object decide how it ought to be printed, rather than trying to wrest control from the developer using the library.
You know, repr is really a crappy name. What does it mean? You can feel free to have long variable names in Ruby; just make sure they're in snake_case.
What is this?

if [Array, String].include?(repr.class) and repr.length > 100
  repr = repr.first(20) + [". . ."] + repr.last(20)
end

You already forced repr to be a String (and in your code, it was already an Array). Why are you checking that again? Plus, if it's a String, then it'll fail when you try to call the nonexistent first on it! Besides, Array and String aren't the only classes with a .length method. This is how I'd do it (keeping in mind that in #6 I converted it to a String):

repr = repr.length > 100 ? "#{repr[0..20]}. . .#{repr[-20..-1]}" : repr

For more information about String#[], see the docs. I'd also recommend pulling this out into a method -- the method name is up to you, but I'd personally call it shorten or trim_extra_chars or something like that.
previous_method really isn't a very good name. It's not necessarily method that came before, after all -- it's an operation that led to the current state. It could well have been something that isn't a method! I'd call it previous_operation.
What happens when someone wants to log a method called start? For example, say they have an object representing a task that's built over several chained method calls, then started with start, which returns the result of the task. They'll try to .log('start'), but it'll instead give them this weird thing that they didn't expect! I'd recommend splitting it into two methods -- then, instead of an if/else and a default value, you can have:

def log_start
    #etc.
end

and

def log(previous_operation)
    #etc.
end

One bonus of this (combined with #8) is that you can totally get rid of repr.
Now, there are a couple of other things: Very rarely do people want to log stuff to the console (i.e. where puts goes). I'd recommend adding support for outputting to other streams, though there are a couple of ways to do it. One of them, my personal favorite, involves a lot of metaprogramming and probably isn't worth it. Another is just to add a parameter output=$stdin and replace all calls to puts with output.puts to log_start and log.
Lastly, I did some changes in the actual output, just because it looks nicer IMO. That's entirely up to you.

Now, here's your code with all of those changes applied:

class String
  def shorten
    self.length > 100 ? "#{self[0..20]}. . .#{self[-20..-1]}" : self
  end
end

class Object
  def log_start
    puts <<-END.gsub(/^\s*\||#@.*$/, '')
      |  #{self.to_s.shorten}
    END
    self
  end

  def log(previous_method)
      puts <<-END.gsub(/^\s*\||#@.*$/, '')
        |   |
        |   |    #{previous_method}
        |   V
        |  #{self.to_s.shorten}
      END
    self
  end
end

Note: I didn't apply tip #11 because there are a bunch of ways to do it and I felt it would be better if I left it up to you how you want to do that.

To test it (Adapted to Ruby 2.2.2 but works the same):

class Integer
  def is_digits_pow_sum?(exp)
    self                           .log_start
        .to_s.chars.map(&:to_i)    .log("Get the digits")
        .map{|x| x**exp}           .log("Each digit to the power of #{exp}")
        .inject(&:+)               .log("Sum the digits")
        .equal?(self)              .log("Equals the start? (#{self})")
  end
end

441.is_digits_pow_sum?(3)

which prints

  441
   |
   |    Get the digits
   V
  [4, 4, 1]
   |
   |    Each digit to the power of 3
   V
  [64, 64, 1]
   |
   |    Sum the digits
   V
  129
   |
   |    Equals the start? (441)
   V
  false


How to refactor this groupby alike method but with index? I have got such 2d jagged array (row is 1st dimension, column is 2nd dimension, bottom to up, left to right)

0   0   b   b   b   c
0   0   h   g   g   c
0   0   h   a   a   c
0   0   f   f   d   d
0   0   i   j   e   e
0   0   i   j   0   0

I would like to get a 2d jagged array summarize the positions of consecutive elements for each row, and ignore all 0 or single elements (assume that things like x x x y x x would not occur, so there won't be 2 blocks of same chars in a row)

the ideal outcome would be

{(b,3,5,2)}  // char, length, rowID, columnID
{(g,2,4,3)}
{(a,2,3,3)}
{(f,2,2,2), (d,2,2,4)}
{(e,2,1,4)}
{}

this is my code, i feel it is not that readable, so I appreciate any suggestion in C# or F# (Linq extension method is welcomed as well)

   Tuple<char, int, int, int>[][] GetHorizontalBricks()
    {
        List<Tuple<char, int, int, int>[]> ret = new List<Tuple<char, int, int, int>[]>();

        for (int rowID = 0; rowID < rowNum; rowID++)
        {
            List<Tuple<char, int, int, int>> rowBricks = new List<Tuple<char, int, int, int>>();
            var row = myBoard[rowID];

            int recentCount = 1;
            int columnID = 0;
            while (columnID < columnNum)
            {
                char recent = row[columnID];
                int next = columnID + 1;
                for (; next < columnNum; next++)
                {
                    if (row[next] == recent)
                    {
                        recentCount++;
                    }
                    else
                    {
                        break;
                    }
                }

                if (recentCount > 1 && recent != '0')
                {
                    rowBricks.Add(Tuple.Create(recent, recentCount, rowID, columnID));

                }
                columnID = next;
                recentCount = 1;
            }

            ret.Add(rowBricks.ToArray());
        }
        return ret.ToArray();
    }

First of all get rid of these tuples. After one week you will forget what each int represents. Define simple struct/class for your brick.

Second thing, small performance suggestion for big row count - explicit initial list size:

... = new List<Tuple<char, int, int, int>[]>(myBoard.Length);//assuming myBoard is an array

without it, list would be recreated and rewritten every 4 new elements.

Below is mine implementation with linq extensions. Probably it is not faster. I don't know if it is more readable, but at least line count is smaller :)

public struct Brick
{
    public char Symbol { get; set; }
    public int Length { get; set; }
    public int RowID { get; set; }
    public int ColumnID { get; set; }

    public override string ToString()
    {
        return string.Format("{0} {1} {2} {3}", Symbol, Length, RowID, ColumnID);
    }
}

public Brick[][] GetHorizontalBricks(char[][] board)
{
    return
        board
            .Reverse() //bottom up row order
            .Select((row,rowIndex) =>
                row
                    .Select((c,i) => new { c = c, i = i })
                    .GroupBy(symbol => symbol.c) //assumption that we will never see 2 groups of same nonzero character in single row
                    .Where(symbol => symbol.Key != 0 && symbol.Count() > 1)
                    .Select(brick =>
                        new Brick
                        {
                            Symbol = brick.Key,
                            RowID = rowIndex,
                            ColumnID = brick.First().i,
                            Length = brick.Count()
                        })
                    .ToArray()
            )
            .Reverse() //reverse again to show result in form like you provided
            .ToArray();
}

and Console app test:

static void Main(string[] args)
{
    char[][] board = new char[][]
    {
        new char[] { (char)0, (char)0, 'b' ,'b','b','c'},
        new char[] { (char)0, (char)0, 'h' ,'g','g','c'},
        new char[] { (char)0, (char)0, 'h' ,'a','a','c'},
        new char[] { (char)0, (char)0, 'f' ,'f','d','d'},
        new char[] { (char)0, (char)0, 'i' ,'j','e','e'},
        new char[] { (char)0, (char)0, 'i' ,'j',(char)0,(char)0}
    };

    foreach (var row in new Program().GetHorizontalBricks(board))
    {
        Console.WriteLine("Row:");
        foreach (var brick in row)
            Console.WriteLine(brick);
    }

    Console.ReadLine();
}


Drawing a hollow rectangle of rows and column provided by the console application This question was put forward by my senior to draw a hollow rectangle in C#, and with the condition that the rows and column would be specified by the user. I was stuck at first, but tried to code and after several hours I realized how it would be done, and then I accomplished the task.

I would like to share my code, as the beginners may get help out of it, and the experts can also look and propose, if they know any other alternative solution which would be composed of less code.

class Program
{
    static void Main(string[] args)
    {
        int arg1;
        int arg2;
        arg1 =Convert.ToInt16(Console.ReadLine());
        Console.ReadLine();
        arg2 = Convert.ToInt16(Console.ReadLine());
       // Console.WriteLine(arg1 + " " + arg2);
        for (int row = 1; row <= arg1; row++)
        {

            if (row == 1 || row == arg1)
            {
                for (int col = 1; col <= arg2; col++)
                {
                    Console.Write("*");
                }
                Console.WriteLine();
            }

            if (row < arg1 && row < arg1-1)
            {
                Console.Write("*");
                for (int i = 0; i < arg2-2; i++)
                {
                    Console.Write(" ");
                }
                Console.Write("*");
                    Console.WriteLine();
            }

        }
        Console.ReadKey();
       // Console.ReadKey();
    }
}

static void DrawLine(int w, char ends, char mids)
    {
        Console.Write(ends);
        for (int i = 1 ; i < w-1 ; ++i)
            Console.Write(mids);
        Console.WriteLine(ends);
    }

    static void DrawBox(int w, int h)
    {
        DrawLine(w, '*', '*');
        for (int i = 1; i < h-1; ++i)
            DrawLine(w, '*', ' ');
        DrawLine(w, '*', '*');
    }

    static void Main()
    {
        DrawBox(10, 10);
    }


Implementation of Asynchronous Cache I have been attempting to implement a cache which caches the results of an asynchronous method, with the restriction that I only want that method to run once for any particular item in the cache. Many other implementations I have seen allow the method to run multiple times. Although only the first returned value is actually stored, if this is a long running method, this is not desirable for me.

Storing Task<T> (instead of T) in the cache, as recommended by most implementers, is a good idea, but I still could not work out how to prevent the method that fetches the value (the valueFactory) from running more than once for any given item. The solution I came up with was to use TaskCompletionSource instead of Task for the items in the cache.

This is what I came up with:

public class AsyncCache<TKey, TValue>
  {
    private readonly Func<TKey, Task<TValue>> valueFactory;

    private readonly ConcurrentDictionary<TKey, TaskCompletionSource<TValue>> completionSourceCache =
      new ConcurrentDictionary<TKey, TaskCompletionSource<TValue>>();

    public AsyncCache(Func<TKey, Task<TValue>> valueFactory)
    {
      this.valueFactory = valueFactory;
    }

    public async Task<TValue> GetItem(TKey key)
    {
      if (!completionSourceCache.TryAdd(key, new TaskCompletionSource<TValue>()))
      {
        return await completionSourceCache[key].Task;
      }

      TaskCompletionSource<TValue> taskSource = completionSourceCache[key];

      try
      {
        var result = await valueFactory(key);
        taskSource.SetResult(result);
      }
      catch (Exception e)
      {
        taskSource.SetException(e);
      }

      return await taskSource.Task;
    }
  }

The idea was that a key is associated with a TaskCompletionSource. The first time a particular item is requested, the TaskCompletionSource will be added to the dictionary for that key. Any subsequent (potentially reentrant) callers would be returned the Task associated with the TaskCompletionSource.

The first caller would then go on to actually call the method, and once the value was determined, this would be associated with the TaskCompletionSource, and anyone waiting on that Task would be able to view the result.

This is my first use of TaskCompletionSource, so please tell me if you think I've missed anything, or done anything wrong.

Note: I am aware of the ParallelExtensionsExtras AsyncCache here: https://blogs.msdn.microsoft.com/pfxteam/2010/04/23/parallelextensionsextras-tour-12-asynccache/

but I only found this after I'd written the above! This is the only other implementation I've found with the semantics described, but since it uses Lazy, it does not await the result of the value factory. I'm not sure if this makes any difference though, i.e. does this mean it is not truly asynchronous, are new threads created, etc.

The way your code is today this should work fine.  But if you ever decide to add a RemoveItem method you can have issues.  Since you are checking if the key exist with TryAdd then accessing assuming the key is there.  This works because nothing can remove a value from the ConcurrentDictionary as it stands.  But if you add the ability to remove an item from the ConcurrentDictionary then calling a TryAdd then assuming it's there is dangerous since the RemoveItem could have been processed between those statements.   This is why the ConcurrentDictionary has a GetOrAdd method to handle that situation for you.

With using a TaskCompletionSource you would struggle to know if you should call the factory method or not from the GetOrAdd since you wouldn't know if your thread was the one to add it.  This is where the Lazy<> version shines as it doesn't matter who added the Lazy<Task<TValue>> both threads can await it since it was passed in the valuefactory.

To me it would be a natural path that RemoveItem might be added later as most caching has a way to invalidate the cache items, for example if the Task threw an Exception maybe I want to remove it from the cache to try again.  Someone coming after to add a RemoveItem might not catch the subtle bug they introduced in the AddItem that was working.

In a nutshell it will work today but might not be as future proof as you want it to be.


Could this be considered a valid mergesort? I tried to do a mergesort function for an assignment without importing merge from heapq:

def mergesort(a):
    if len(a) < 2:
        return a
    return mymerge(mergesort(a[len(a) // 2:]), mergesort(a[:len(a) // 2]))

def mymerge(list1, list2):
    for i in range(len(list1)):
        for x in range(len(list2)):
            if list1[0] < list2[x]:
                list2.insert(x, list1[0])
                list1.pop(0)
                break
            if list1[0] > list2[len(list2) - 1]:
                list2.append(list1[0])
                list1.pop(0)
                break
    return list2

Could this be considered a valid mergesort or does my function have flaws? Are there ways to improve the mymerge function?

No, it is not a merge sort. It never accounts for the fact that list1 is already sorted. Instead it performs len(list1) insertions, every time traversing list2 from the very beginning.

Each call to mymerge is quadratic with respect to the length of the list). At each recursion level \$r\$ there are \$2^r\$ fragments of length \$\dfrac{n}{2^r}\$, so each recursion level will take \$O\left(r\left(\dfrac{n}{2^r}\right)^2\right)\$ time, and summing up for \$r\$ from \$1\$ to \$\log{n}\$ yields the total complexity \$O\left( n^2 \log{n}\right)\$.

The expected runtime of merge sort shall be \$O(n\log{n})\$ (or \$O(n \log^2{n})\$ if done in place).


is it a good strlcat() implementation? I'd like to know if i respect strlcat(3) behiour with my implementation, cause i'm not really sure to understand strlcat particular behaviour

int my_strlcat(char *dest, char *src, int size)
{
  int i;
  int src_len;
  int dest_len;

  i = 0;
  dest_len = strlen(dest);
  src_len = strlen(src);
  if (size <= 0)
   return dest_len;
  while (dest_len < size)
  {
    dest[dest_len] = src[i];
    dest_len++;
    i++;
  }
  dest[dest_len] = '\0';
  return (strlen(dest));
}

src_len is not used..
the strlen in the return statement is not needed.. you know the length from dest_len.

IMHO very weird implementation :)


Discovering words from letters in Clojure (Letterpress Solver) I created a Letterpress solver that will take a string of letters and return a list of valid words that can be constructer with the provided letters. Not all letters need to be used in each word.

This works by reading all words into a list named dictionary, transforming that list of words into a map, letter-permutations, keyed on ordered letters (as a list of characters) with values of vectors containing anagrams of the key letters.

Then, calling find-words with a string of letters and a lower bound for word length, will find all distinct combinations of those letters with lengths from the lower bound to the size of the input string.

The combinations are then map'd over letter-permutations to pull out all valid words that can be made.

(def dictionary
  (clojure.string/split-lines (slurp "words")))

(def letter-permutations
  (group-by sort dictionary))

(defn get-combinations
  [letters lower-bound upper-bound]
  (mapcat #(distinct (combo/combinations letters %)) (range lower-bound (inc upper-bound))))

(defn find-words
  [letters lower-bound]
  (mapcat letter-permutations (get-combinations (sort letters) lower-bound (count letters))))

This works extremely well for letter Strings up to 22 characters in length. But with 23 characters and upwards there is a drastic increase in the amount of time taken, when the response will come in bursts of words with long pauses (garbage collection?) between each batch.

My testing for the above stats has been with the string "otxtzlsunowayzoyiwqyocetl" (which may of may not be my current board :)), but I've experienced the same same behaviour occurring for other strings as well.

I'm happy to hear feedback on all of the code, but particularly for insights to the cause of the bottlenecks and suggestions of how I could improve this.

Full source code can be seen on Github

The (distinct (combo/combinations letters %)) looks mighty troublesome to me. I don't know what the complexity function is for combinations offhand, but I'm going to guess it's got a factorial in it. Using distinct around it means that you will hold that entire set of combinations in memory. That's one of those things that works well for small numbers, then falls off a cliff.

I also think this is one of those kinds of problems that can benefit significantly from memoization if structured properly. (Although memoization creates a cache in memory, so you might want to build your own with a customized cache to avoid blowing the heap. This is pretty easy with core.memoize.)


Short square matrix class in C++ using an array I'm a novice at C++ in comparison with other languages, so as an exercise I wrote a square Matrix class that doesn't use STL.

#ifndef MATRIX_H
#define MATRIX_H
#include <initializer_list>
#include <algorithm>

class Matrix {
public:
  Matrix(std::size_t sz) : elements{new int[sz * sz]}, size{sz} {}
  Matrix(std::size_t sz, std::initializer_list<int> elem) : Matrix(sz) {
    std::copy(elem.begin(), elem.end(), elements);
  }
  ~Matrix() { delete[] elements; }

  /** Get the matrix element at row i and column j. */
  int get(std::size_t i, std::size_t j) const {
    return elements[convert(i, j)];
  }

  /** Set the value of the matrix at row i and column j and return the old value. */
  int set(std::size_t i, std::size_t j, int new_value) {
    std::size_t idx = convert(i, j);
    int old_value = elements[idx];
    elements[idx] = new_value;
    return old_value;
  }

  std::size_t width() const {
    return size;
  }

private:
  int *elements;
  std::size_t size;

  inline std::size_t convert(std::size_t i, std::size_t j) const {
    if (i >= size || j >= size) {
      throw std::exception("Matrix indices out of bounds");
    }
    std::size_t idx = i * size + j;
    return idx;
  }
};

#endif

Critiques are welcome!  In particular, I'd be happy to get advice on the following:

Is this an appropriate usage of exception handling?  Is there a better option than just tossing up a std::exception if the matrix indices are out of bounds?
Do the constructors and destructors make sense?
Any general comments on writing in idiomatic C++ style.

(1) The exception is a good way of handling such errors, but it may be a good idea to provide a non-throwing, less safe accessors. For example, std::vector::at() will throw an std::out_of_range if requested index is not valid, but std::vector::operator[] will silently accept all values. That's because, if individual elements are accessed very frequently (some complicated math algorithm), all those ifs may impact the performance. So, as it goes for the exceptions, two suggestions:

throw std::out_of_range instead of plain std::exception to indicate what kind of error just happened,
do not throw in every accessor (you only have one, which leads to point 4, see below).

(2) Yes, they do. But, the basic question is: do you really need to be able to specify size of a matrix at runtime? Perhaps it would be better to use compile-time constant size:

template <size_t Size>
class Matrix
{
private:
  int elements[Size*Size];
  //...
};

Then, you would use it this way:

Matrix<4> mat4;

In this version, no destructor would be necessary and no memory allocation required. Because of that, this version also can be faster.

(3) "Idiomatic C++ style" has become a very fragile term recently (IMHO). But, you could probably provide support for accessing elements of your matrix using operator[] (see point 4).

(4) It would be nice if users of your Matrix could write simply:

Matrix<4> mat; // Or: Matrix mat {4}, in dynamically-allocated version
mat[3][3] = 1; // set elements in row 3, column 3 (note, that indices are from 0 to 3)

Or, equivalently, in simpler version:

mat[15] = 1;

(*) Simple version only requires the presence of operator[]:

template <size_t Size>
class Matrix
{
    int& operator[](size_t index)
    {
        return elements[index];
    }

    const int& operator[](size_t index) const
    {
        return elements[index];
    }
};

Note, that two version are provided, so you can do this:

Matrix<4> mat;
mat[5] = 1;

but not this:

const Matrix<4> constMat;
constMat[2] = 3; // compilation error: cannot assign to const reference

(**) The more elegant, but also more difficult version (nested [][] syntax), requires, that operator[] of the matrix must return an object (or an array) that also supports [] notation.

The easier way to achieve this would be to to change the T* elements to T** elements (or, in templated version, elements[Size*Size] to elements[Size][Size]). Note, that in this case you do not need the convert() function, since you can use the indices explicitly.

You could also declare simple Vector struct (I will limit myself here to templated version):

template <size_t Size>
struct Vector
{
private:
  int elements[Size];

public:
  int& operator[](size_t index)
  {
    return elements[index];
  }

  const int& operator[](size_t index) const
  {
    return elements[index];
  }
};

And then implement your Matrix using Vector:

template <size_t Size>
class Matrix
{
private:
  Vector<Size> columns[Size];

public:
  Vector<Size>& operator[](size_t index)
  {
    return columns[index];
  }

  const Vector<Size>& operator[](size_t index) const
  {
    return columns[index];
  }
};

(1.1) Now, after we covered the topic of operator[], we can return to the exceptions once more. To aid those unsafe operator[]s, we can add more safe, explicit at() function:

template <size_t Size>
class Matrix
{
private:
  Vector<Size> columns[Size];

public:
  Vector<Size>& at(size_t index)
  {
    if (index >= Size) throw std::out_of_range("Invalid index!");
    return columns[index];
  }

  const Vector<Size>& at(size_t index) const
  {
    if (index >= Size) throw std::out_of_range("Invalid index!");
    return columns[index];
  }
};


Turn On/Off Functions to speed things up I have these two macros that are like my Yin & Yang. They turn off and on functions in excel so my macros calculate quicker.

I call on in the beginning and one at the end.

Is there anything I can do or add to these to make them better?

Private Sub TurnOffFunctions()

    Application.ScreenUpdating = False
    Application.DisplayStatusBar = False
    Application.Calculation = xlCalculationManual
    Application.EnableEvents = False

End Sub

Private Sub TurnOnFunctions()

    Application.ScreenUpdating = True
    Application.DisplayStatusBar = True
    Application.Calculation = xlCalculationAutomatic
    Application.EnableEvents = True

End Sub

You should save the initial state of the Application.* variables before you mess with them, then reset them when you're done. Not all of your users will have the same settings and it's rude to assume they do.

A simple way to do it would be to implement a class like this:

'@Folder("Classes")
Option Explicit

Private Type ApplicationSettings
  ScreenUpdating As Boolean
  DisplayStatusBar As Boolean
  CalculationMethod As Excel.XlCalculation
  EnableEvents As Boolean
End Type

Private this As ApplicationSettings

Public Property Get ScreenUpdating() As Boolean
  ScreenUpdating = this.ScreenUpdating
End Property

Public Property Get DisplayStatusBar() As Boolean
  DisplayStatusBar = this.DisplayStatusBar
End Property

Public Property Get CalculationMethod() As Excel.XlCalculation
  CalculationMethod = this.CalculationMethod
End Property

Public Function DisableExcelDisplayUpdates()

  SetScreenUpdating False
  SetDisplayStatusBar False
  SetCalculationMethod Excel.xlCalculationManual
  SetEnableEvents False

End Function

Public Function EnableExcelDisplayUpdates()

  Application.ScreenUpdating = this.ScreenUpdating
  Application.DisplayStatusBar = this.DisplayStatusBar
  Application.Calculation = this.CalculationMethod
  Application.EnableEvents = this.EnableEvents

End Function

Private Function SetScreenUpdating(ByVal value As Boolean)
  this.ScreenUpdating = Application.ScreenUpdating
  Application.ScreenUpdating = value
End Function

Private Function SetDisplayStatusBar(ByVal value As Boolean)
  this.DisplayStatusBar = Application.DisplayStatusBar
  Application.DisplayStatusBar = value
End Function

Private Function SetCalculationMethod(ByVal value As Excel.XlCalculation)
  this.CalculationMethod = Application.Calculation
  Application.Calculation = value
End Function

Private Function SetEnableEvents(ByVal value As Boolean)
  this.EnableEvents = Application.EnableEvents
  Application.EnableEvents = value
End Function

Then test it out like this:

Option Explicit
`@Folder("Tests")

Public Sub testIt()

  Dim ExcelValues As Class1
  Set ExcelValues = New Class1

  Debug.Print "Before "
  Debug.Print "ScreenUpdating: " & Application.ScreenUpdating
  Debug.Print "DisplayStatusBar: " & Application.DisplayStatusBar
  Debug.Print "Calculation: " & Application.Calculation
  Debug.Print "EnableEvents: " & Application.EnableEvents

  ExcelValues.DisableExcelDisplayUpdates

  Debug.Print "During "
  Debug.Print "ScreenUpdating: " & Application.ScreenUpdating
  Debug.Print "DisplayStatusBar: " & Application.DisplayStatusBar
  Debug.Print "Calculation: " & Application.Calculation
  Debug.Print "EnableEvents: " & Application.EnableEvents

  MsgBox "Do your long running process here"

  ExcelValues.EnableExcelDisplayUpdates

  Debug.Print "After "
  Debug.Print "ScreenUpdating: " & Application.ScreenUpdating
  Debug.Print "DisplayStatusBar: " & Application.DisplayStatusBar
  Debug.Print "Calculation: " & Application.Calculation
  Debug.Print "EnableEvents: " & Application.EnableEvents

End Sub

*Note that the '@Folder("<something>") annotation is a feature of Rubberduck which is a great tool for helping to improve your VBA code. I'm an avid user and occasional contributor to the OSS project.


Read line and combine duplicate entries based on one of the fields The following Java code reads lines from an input text file and will output the entry that has the highest number in field-2 to an output file:

INPUT:

  William J. Clinton 6 Q1124
  42nd President of the United States 6 Q1124
  Bill Clinton,6,Q1124
  Bill Clinton,14,Q9890
  Bill Clinton,5,Q9880
  William Jefferson Clinton,6,Q1124

OUTPUT:

  William J. Clinton,Q1124  42nd President of the United
  States,Q1124  Bill Clinton,Q9890 (this Q-ID selected because
  field-2 was highest number "14")
  William Jefferson Clinton,Q1124

Could you offer suggestions on making this more elegant?

/*
 * This class uses a method to combine duplicate Wiki keys into one and
 * selecting the Qid of the one with largest link count
 */

public static void main(String[] args) {

    if (args.length < 2) {
        usage();
        System.exit(0);
    }

    //
    try {
        // Create output file
        BufferedWriter out = Files.newBufferedWriter(Paths.get(args[1]),
                StandardCharsets.UTF_8);

        // read input file
        BufferedReader br = Files.newBufferedReader(Paths.get(args[0]),
                StandardCharsets.UTF_8);
        String key = "";
        int links = 0;
        String qID = "";

        String next, line = br.readLine();
        for (boolean first = true, last = (line == null); !last; first = false, line = next) {
            last = ((next = br.readLine()) == null);

            String[] entry = line.split(",");

            if (first) {
                // only assign current values
                key = entry[0];
                links = new Integer(entry[1]);
                qID = entry[2];

            } else if (last) {

                //check if key is a duplicate
                if(key.equals(entry[0])){
                    //use the Qid from the highest links
                    int temp = new Integer(entry[1]);
                    if(links < temp){
                        //write entry to output higher links
                        out.write(key + "," + entry[2] + "\n");
                    }else{
                        //write entry to output using current
                        out.write(key + "," + qID + "\n");
                    }

                }else{
                    //write last entry to output
                    out.write(key + "," + qID + "\n");
                    out.write(entry[0] + "," + entry[2] + "\n");
                }

            } else {

                //check if key is a duplicate
                if(key.equalsIgnoreCase(entry[0])){
                    //use the Qid from the highest links
                    int temp = new Integer(entry[1]);
                    if(links < temp){
                        links = temp;
                        qID = entry[2];
                    }

                }else{
                    //write entry to output
                    out.write(key + "," + qID + "\n");

                    //reassign current values
                    key = entry[0];
                    links = new Integer(entry[1]);
                    qID = entry[2];
                }

            }

        }

        // Close the output stream
        out.close();

    } catch (Exception e) {// Catch exception if any
        System.err.println("Error: " + e.getMessage());
    }

}

private static void usage() {
    System.err.println("Usage: CombineKeys InputFile OutputFile");
}

I'm far from a Java expert, but a few things jumped out at me

You forgot to close the reader (br).

Do not close the streams inside of the try. What happens if an exception gets thrown before the close commands are reached? You need a finally block. Or, better yet, if you're using Java 7 or above, use a try-with-resources block.

If you're only error handling is to just print out the message, I'd be tempted to go ahead and let the exception bubble up and kill the program. Unless you're just trying to go for a more simple error, in which case hiding the stack trace could be nice.

I would just let key be null the first time around and check for that instead of using first.

I might try to restructure the for conditions. They're very confusing in the current form. Perhaps loop as long as you successfully read a line, and then handle the last logic outside of the for loop. That way you don't have to have so much going on inside of the loop.

On a minor note, you should try to create the reader before the writer. That way you don't create an output file if you can't open the input one.

The repetition in the last and else blocks should be refactored out into a common case. Based on that they do the exact same thing, I'm not actually sure why last get's special treatment.

I would use Integer.parseInt instead of using unboxing on new Integer(str).

temp is a bad variable name. Name it something descriptive of what it actually is.

Don't bother commenting really obvious things ("Catch exception if any" for example).

Sometimes you use spaces like if (...) and sometimes you don't. Pick one and stick with it (by which I really mean always use spaces :p).

All in all, my Java is a bit rusty, so take this with a grain of salt, but, I would do something like this:

public static void main(String[] args) {
    if (args.length < 2) {
        System.err.println("Usage: CombineKeys InputFile OutputFile");
        System.exit(1);
    }

    BufferedWriter out = null;
    BufferedReader br = null;

    try {
        out = Files.newBufferedWriter(Paths.get(args[1]), StandardCharsets.UTF_8);

        br = Files.newBufferedReader(Paths.get(args[0]), StandardCharsets.UTF_8);

        String key = null;
        int links = 0;
        String qID = null;

        while ((line = br.readLine()) != null) {
            String[] entry = line.split(",");

            if (entry[0].equalsIgnoreCase(key)) {
                int tmpLinks = Integer.parseInt(entry[1]);
                if (links < tmpLinks){
                    links = tmpLinks;
                    qID = entry[2];
                }
            } else {
                if (key != null) {
                    out.write(key + "," + qID + "\n");
                }
                key = entry[0];
                links = Integer.parseInt(entry[1]);
                qID = entry[2];
            }
        }
        if (key != null) {
            out.write(key + "," + qID + "\n");
        }
    } catch (Exception e) {
        System.err.println("Error: " + e.getMessage());
    } finally {
        if (br != null) {
            br.close();
        }
        if (out != null) {
            out.close();
        }
    }
}

I don't really like the repetition of the outputting, but I can't figure out a way around that. Also, due to the nesting level, I would probably pull the actual processing into a new method that takes in the two already opened streams.


Use different contructors of disposable class I have a very simple helper class to validate credentials. Sometimes the domain has a value, sometimes not. Depending on this, a different constructor for PrincipalContext must be used. PrincipalContext is disposable, so I am using using. It looks ugly, and recommendations?

public class LDAPManager : ILDAPManager
{
    public bool ValidCredentials(string username, string password, string domain)
    {
        using (var context = string.IsNullOrWhiteSpace(domain) ? new PrincipalContext(ContextType.Domain) : new PrincipalContext(ContextType.Domain, domain))
        {
            return context.ValidateCredentials(username, password);
        }
    }
}

As the constructor of PrincipalContext which only takes a ContextType as parameter internally calls this constructor

public PrincipalContext(ContextType contextType, string name, string container,
      ContextOptions options, string userName, string password){}

by passing null for the name I would change your method to

public class LDAPManager : ILDAPManager
{
    public bool ValidCredentials(string username, string password, string domain)
    {
        if (string.IsNullOrWhiteSpace(domain)) {
            domain = null;
        }
        using (var context = new PrincipalContext(ContextType.Domain, domain))
        {
            return context.ValidateCredentials(username, password);
        }
    }
}

based on the valid comment of Lightness Races in Orbit it can be that this implementation will change sometime. So be careful.

After getting this comment, I also noticed some naming issue with this method. Based on the naming guidlines, you should use verbs or verb phrases to name your methods.
So the method would be better named ValidateCredentials().


Java pagination algorithm I made a pagination algorithm but in my opinion it is too complicated and it is quite difficult to be understood. This algorithm should show 5 pages where in the middle is the current page. It should also show two pages before and two pages after the selected page (if these pages exist).

So, I just want to generate the array which is underlined in the above picture.

Here is my code:

public class Pagination {

    private int totalPages;
    private int selectedPage;
    private int offset;

    public Pagination() {
    totalPages = 6;
    selectedPage = 4;
    offset = 5;
    }

    public static void main(String[] args) {
    Pagination pagination = new Pagination();

    System.out.println(pagination.buildPaginationConditions().toString());
    }

    private List<Integer> buildPaginationConditions() {

    if (selectedPage <= offset / 2) {
        return doShowFirstPages();
    }
    return buildPagination();
    }

    private List<Integer> doShowFirstPages() {

    List<Integer> pagination = new LinkedList<Integer>();

    for (int i = 1; i <= offset; i++) {
        if (totalPages >= i) {
        pagination.add(i);
        }
    }
    return pagination;
    }

    private List<Integer> buildPagination() {

    List<Integer> pagination = new LinkedList<Integer>();
    int delta = offset / 2; // How many pages to left/right
    int paginationMiddle = (int) Math.ceil((double) offset / 2);

    for (int i = 1; i <= offset; i++) {

        if (i < paginationMiddle) {
        pagination.add(selectedPage - delta);
        delta--;
        continue;
        }
        if (paginationMiddle == i) {
        pagination.add(selectedPage);
        delta = 1;
        continue;
        }
        if (totalPages >= selectedPage + delta) {
        pagination.add(selectedPage + delta);
        delta++;
        continue;
        }

        if (selectedPage >= totalPages - 1) {

        //If it is the last or the penultimate page shift the pages to the right woth 1/2 positions

        int noOfGapsInPagination = offset - pagination.size();
        int temp = new Integer(offset);
        for (int j = 0; j < noOfGapsInPagination; j++) {
            pagination.add(j, totalPages - (--temp));
        }
        break;
        }
    }

    return pagination;
    }
}

The algorithm works but I want to write that for loop from buildPagination() much more simpler because it looks too complicated now .

I know that I can to extract those pices of code from if conditions in smaller methods but it is not going to simplify the code to much. Do you have any suggestions?

the most simple algorithm would be to calculate the start and end indices

    // set start index relative to selected
    int start = selectedPage - (offset / 2);
    // adjust for first pages
    start = Math.max(start, 1);
    // set end index relative to start
    int end = start + offset - 1;
    // adjust start and end for last pages
    if (end > totalPages) {
        end = totalPages;
        start = end - offset + 1;
    }

    return IntStream.rangeClosed(start, end).boxed()
        .collect(Collectors.toList());

as for code review:

method signatures correctly return interface List. however, there are two places where you initialize the list with a concrete implementation. now imagine you would decide that ArrayList is better suited. you need to remember to modify two places.
while on the subject: don't use LinkedList. it has an extremely narrow use case where it is preferable over ArrayList. this is not one of them. Especially if you initialize the ArrayList with initial size.
use Java 8 collection stream (if you've learned it) it is more efficient and concise than for loop
The code does not use java standard indentation. code inside if block should be indented inward.


Zig-zag function - coded solution Here is my coded solution to the LeetCode zig-zag problem -

  The string "PAYPALISHIRING" is written in a zigzag pattern on a given
  number of rows like this (you may want to display this pattern in a
  fixed font for better legibility) -

P   A   H   N
A P L S I I G
Y   I   R

  And then read line by line: "PAHNAPLSIIGYIR"

  Write the code that will take a string and make this conversion given
  a number of rows.

I know the final line is a bit messy, and I could unpack it into a two-line for loop, but I really like single-line for loops.

def zigzag() -> list:
    """Returns the zig-zag word of a given input as a list."""
    array = []
    for _ in range(num_rows):
        array.append([])
    increasing = 1
    array_index = 0
    word_index = 0
    while word_index < len(word):
        if increasing:
            array[array_index].append(word[word_index])
            array_index += 1
            word_index += 1
        else:
            array[array_index].append(word[word_index])
            array_index -= 1
            word_index += 1
        if array_index == -1 and increasing == 0:
            increasing = 1
            array_index = 1
        if array_index == num_rows and increasing == 1:
            increasing = 0
            array_index = num_rows - 2
    return array

if __name__ == "__main__":
    word = "PAYPALISHIRING"
    num_rows = 3
    print("".join(line for array in zigzag() for line in array))

PEP 8 implementation is great, now to focus on being concise -

While you have started writing immaculate programs, as in here and your current question, I believe you should start making your programs more concise. This decreases memory space and sometimes, makes your program faster. Also, I'm glad to see you've started implementing the if __name__ == '__main__' guard.

Here's one approach you could use to make your program really concise and fast -

def zigzag(word: str, num_rows: int) -> str:

    ans = [''] * num_rows
    x = 0
    direction = 1
    for i in word:
        ans[x] += i
        if 0 <= x + direction < num_rows:
             x += direction
        else:
             direction *= -1
             x += direction
    return ''.join(ans)

if __name__ == "__main__":
    print(zigzag("PAYPALISHIRING", 3))

Here, I believe that it would be better if you don't declare word and num_rows before calling the function. This -

print(zigzag("PAYPALISHIRING", 3))

looks much shorter (and better) than -

word = "PAYPALISHIRING"
num_rows = 3
print("".join(line for array in zigzag() for line in array))

Also, (hey, I'm kind of like a beginner, so please correct me if I'm wrong) I feel that you should perform the .join() function, or any other function really, in the principal function, as I have done above -

# rest of the principal function
return ''.join(ans)

Just use the if __name__ == __main__ guard to (simply) call the principal function and execute the entire program (you shouldn't really perform any other function(s) in it except calling the principal function to execute it) -

if __name__ == "__main__":
    print(zigzag("PAYPALISHIRING", 3))

Also, I would rename increasing to direction as it suits this task better ("the value of "direction" changes only when we have moved up to the topmost row or moved down to the bottommost row" (taken from Leetcode). If you're wondering what *= means, it's just another way of doing x = x * 5, which is specified here.

Also, you could make -

array = []
for _ in range(num_rows):
    array.append([])

num_rows = 3
print(array)
# [[], [], []]

more concise by just simply initializing it -

array = [[] for _ in range(num_rows)]  # Thanks to Maarten Fabré

num_rows = 3
print(array)
# [[], [], []]

See how concise you can get by doing this? This removes the need to use .append(), which, in this case, is unnecessary (this will also take up less memory space).

  I know the final line is a bit messy, and I could unpack it into a
  two-line for loop but I really like single-line for loops.

Nah, don't worry. As far as I know,

print("".join(line for array in zigzag() for line in array))

is so much more concise than unpacking it into a two-line for loop. I'm glad you like single-line for loops :)

Here are the times taken for each function -

Your function -

%timeit "".join(line for array in zigzag() for line in array)
8.34 µs ± 834 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

My function -

%timeit zigzag("PAYPALISHIRING", 3)
4.19 µs ± 134 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

As you can see above, the time taken for execution has been approximately halved, which also shows that making your function concise, down to what I have done in my function, can make your program much faster.

Here is the Leetcode result for my program (if need be) -

Hope this helps!


strstr implementation with python and sets Please review my strstr implementation in terms of time/space efficiency and overall readability. I am preparing for a coding assessment coming up as I am looking to pivot my career from Physics to Computer Science. This is an exercise from the interview practice questions on CodeSignal. My implementation successfully passes all 14 tests but I get the following error when I submit my answer:

Tests passed: 21/27. Execution time limit exceeded on test 22: Program
exceeded the execution time limit. Make sure that it completes
execution in a few seconds for any possible input.

"""strstr().

Given two strings, text and pattern, return an integer indicating the index
in the text of the first occurrence of the pattern.
"""

def strstr(text, pattern):
    """Needle in haystack O(|text|) time? and O(?) space.

    Return an integer indicating the first occurrence of pattern in the text,
    or -1 if the pattern is not part of the text.
    """
    lookup = set([pattern])
    len_text = len(text)
    len_pattern = len(pattern)
    for index in range(len_text-len_pattern+1):
        if text[index:index+len_pattern] in lookup:
            return index
    return -1

def tests():
    """Sample Tests."""
    samples = [("CodesignalIsAwesome", "IA", -1),
               ("CodesignalIsAwesome", "IsA", 10),
               ("a", "a", 0),
               ("a", "A", -1),
               ("sst", "st", 1),
               ("lrnkbldxguzgcseccinlizyogwqzlifxcthdgmanjztlt", "an", 38)]
    for text, pattern, answer in samples:
        if strstr(text, pattern) != answer:
            print(f"text: {text}, pattern: {pattern}, aswer: {answer}")

if __name__ == "__main__":
    tests()

Edit: Avoid using built-in functions to solve this challenge. Implement them yourself, since this is what you would be asked to do during a real interview.

As someone trying to pivot from physics to Computer Science, I think this solution is good, many CS graduates might not come up with a better solution first up.
Let's get some optimizations unrelated to cyclomatic complexity out of the way.
You added the pattern to a set. The lookup is constant O(1), I don't see a need to do that and you can save on a high constant.
Let's take a closer look at
    for index in range(len_text-len_pattern+1):
        if text[index:index+len_pattern] in lookup:

specifically text[index:index+len_pattern], this creates a copy of the string for the range index:len_pattern.  This is an implementation detail of Python, one can just as easily imagine an optimization that does not create a copy.
This question and its response might help explain this better.
You might be able to optimize away the object creation, again this unrelated to cyclomatic complexity.
premature optimization is evil - the intent above is more of a code review. Addressing you question below.

The runtime complexity of the loop is the equivalent of creating a copy of the pattern times the len of text
if m is len(text) and n is len(pattern) the for loop is O(n*m) in every iteration.
Typically in questions like these, there is an insight that is not always obvious that drives the solution resulting in a lower cyclomatic complexity. In this instance it is that the algorithm can be implemented in a single pass O(n) by keeping track of indexes across both the text and the pattern
def strstr(text, pattern):

    t = 0
    len_text = len(text)
    len_pattern = len(pattern)
    i = 0
    for i in range(len_text + 1):
        if (t == len_pattern or i == len_text):
            break
        if (text[i] == pattern[t]):
            t += 1
        elif (text[i] == pattern[0]):
            t = 1
        else:
            t = 0

    if (t < len_pattern):
        return -1
    else:
        return i - t

def tests():
    """Sample Tests."""
    samples = [("CodesignalIsAwesome", "IA", -1),
               ("CodesignalIsAwesome", "IsA", 10),
               ("a", "a", 0),
               ("a", "A", -1),
               ("sst", "st", 1),
               ("lrnkbldxguzgcseccinlizyogwqzlifxcthdgmanjztlt", "an", 38),
               ("abcd" * 300000 + "abcde", "abcde", 1200000),]
    for text, pattern, answer in samples:
        actual = strstr(text, pattern)
        if actual != answer:
            print(f"FAILED : pattern: {pattern}, actual: {actual}, expected: {answer}")
        else:
            print(f"SUCCESS: pattern: {pattern}, actual: {actual} matched expected")

I added an additional text case to demonstrate the runtime, testing this with time module in python, the clock time of the implementation is a few milli seconds compared to the clock time of the O(n*m) algorithm which on my machine is aboue 21 seconds.


Implementation of many-to-many relation Tag Table I have belowe tables, for Map string[] Tags from ViewModel to Model I'm using Below codes :

  public class Tag:BaseEntity
{
    public string Name { get; set; }
    public virtual ICollection<Blog.BlogPost> BlogPosts { get; set; }
    public virtual ICollection<News.NewsItem> NewsItems { get; set; }

}
 public class BlogPost : BaseContent, IAuditable
    {
    public virtual ICollection<Tag> Tags { get; set; }
}

public class NewsItem : BaseContent, IAuditable
    {
    public virtual ICollection<Tag> Tags { get; set; }

    public NewsItem()
    {
        Tags = new List<Tag>();
    }
}

In newsService I have two methods Insert ,Update to Add and Update NewsItem :

     public void Insert(AddNewsItemViewModel newsItemViewModel)
    {
        if (newsItemViewModel == null)
            throw new ArgumentNullException("newsItem");

        var model = Mapper.Map<NewsItem>(newsItemViewModel);

        if(newsItemViewModel.Tags.Any()) // Tags is `string[]`
        {
            var listOfActualTags = _tagService.GetTagsByName(newsItemViewModel.Tags); // return List<Tag>
            var listOfActualTagNames = listOfActualTags.Select(x => x.Name.ToLower()).ToList();

            foreach (var tag in newsItemViewModel.Tags)
            {
                if (!listOfActualTagNames.Contains(tag.ToLowerInvariant().Trim()))
                {
                    Tag tagModel = new Tag { Name=tag};
                    _tagService.Insert(tagModel);
                    model.Tags.Add(tagModel);
                }
            }

            model.MetaKeywords =string.Join(",",newsItemViewModel.Tags);
        }
        _uow.MarkAsAdded(model);
    }

and for Update  Method I have this Code :

        public void Update(EditNewsItemViewModel newsItemViewModel)
    {
        if (newsItemViewModel == null)
            throw new ArgumentNullException("newsItem");
        var model = GetById(newsItemViewModel.Id);
        model = Mapper.Map(newsItemViewModel, model);
        if (model.Tags != null && model.Tags.Any())
            model.Tags.Clear();

        var listOfActualTags = _tagService.GetTagsByName(newsItemViewModel.Tags); // return List<Tag>
        var listOfActualTagNames = listOfActualTags.Select(x => x.Name.ToLower()).ToList();

        foreach (var tag in newsItemViewModel.Tags)
        {
            Tag tagModel = new Tag { Name = tag };
            if (!listOfActualTagNames.Contains(tag.ToLowerInvariant().Trim()))
            {

                _tagService.Insert(tagModel);
                model.Tags.Add(tagModel);
            }
            else
                model.Tags.Add(tagModel);
        }

        model.MetaKeywords = string.Join(",", newsItemViewModel.Tags);

        _uow.MarkAsChanged(model);
    }

For BlogPost I have to repeat Above Code (for tag part) fro that , Is there a way to Optimize this ?

Your Insert method has a bug - if a tag exists in a database, you won't add it to your model.Tags collection. Interesting enough, your Update method doesn't have that problem. After you fix that bug, you're left with almost identical part of the code that could also be reused by BlogPost logic.

Because it should be reused, it would be best to place it in your TagService and, after a few improvements, it would look like this:

public void AddTagsToList(ICollection<Tag> tagList, string[] tagNames)
{
  var listOfActualTags = GetTagsByName(tagNames);

  foreach (var tagName in tagNames)
  {
    var tag = listOfActualTags.FirstOrDefault(t => t.Name == tagName);

    if (tag == null)
    {
      tag = new Tag { Name = tagName };
      Insert(tag);
    }

    tagList.Add(tag);
  }
}

Your Insert method would become:

public void Insert(AddNewsItemViewModel newsItemViewModel)
{
  if (newsItemViewModel == null)
    throw new ArgumentNullException("newsItem");

  var model = Mapper.Map<NewsItem>(newsItemViewModel);

  if(newsItemViewModel.Tags.Any())
  {
    _tagService.AddTagsToList(model.Tags, newsItemViewModel.Tags);

    model.MetaKeywords = string.Join(",", newsItemViewModel.Tags);
  }

  _uow.MarkAsAdded(model);
}

And Update method would become:

public void Update(EditNewsItemViewModel newsItemViewModel)
{
  if (newsItemViewModel == null)
      throw new ArgumentNullException("newsItem");
  var model = GetById(newsItemViewModel.Id);
  model = Mapper.Map(newsItemViewModel, model);
  if (model.Tags != null && model.Tags.Any())
    model.Tags.Clear();

  if(newsItemViewModel.Tags.Any())
  {
    _tagService.AddTagsToList(model.Tags, newsItemViewModel.Tags);

    model.MetaKeywords = string.Join(",", newsItemViewModel.Tags);
  }

  _uow.MarkAsChanged(model);
}

As I said, you can reuse _tagService.AddTagsToList method for BlogPost as well by providing the Tags property as a first parameter and a list of tag names as a second parameter.


How can I make this StringToCollections method more generic? I am new to C# and I have big functions that receives the following arguments:

(Package inPackage, string materialIDs, string functionIDs, string companyIDs)

Asides from the first argument, which is an object of type Package, all the other arguments are strings of IDs.

So, for example, a valid input for this function would be: myPackage, "1,2,3", ",2,3,4", ",45,2".

A package is an object that contains a list of materials, a list of functions and a list of companies.

The objective of this function, is to look up in the DB all the objects that contain those IDs, and add them to myPackage.

To achieve this objective, this is what I have created:

private Package StringToCollections(Package inPackage, string materialIDs, string functionIDs, string companyIDs)
        {
            //Check Materials Data - Case WITHOUT FORMAT PROTECTION
            var materialList = new List<Material>();
            var materialStringArray = materialIDs.Split(',');
            var materialIdsArray = materialStringArray.Where(x => x != "").Select(int.Parse);
            materialList.AddRange(DB.Materials.Where(x => materialIdsArray.Contains(x.MaterialId)));
            inPackage.GATE_Material = materialList;

            //Check Employee_Functions Data - HAS FORMAT PROTECTION ... KINDOFF?
            List<Function> functionsList = new List<Function>();
            string[] functionStringArray = functionIDs.Split(',');
            List<int> functionIdsArray = new List<int>();
            for (int i = 0; i < functionStringArray.Length; i++)
            {
                try
                {
                    functionIdsArray.Add(int.Parse(functionStringArray[i]));
                }
                catch (FormatException)
                {
                    //not a number, ignore - IS THIS A GOOD IDEA!?
                }
            }
            functionsList.AddRange(DB.Function.Where(x => functionIdsArray.Contains(x.FunctionId)));
            inPackage.Function = functionsList;

            //Check Company Data - ANOTHER WAY OF FORMAT PROTECTION ... KINDOFF?
            if (!companyIDs.IsNullOrWhiteSpace())
            {
                var companiesList = new List<Company>();
                var companyStringArray = companyIDs.Split(',');
                var companyIdsArray = companyStringArray.Where(x => x != "").Select(int.Parse);
                try
                {
                    companiesList.AddRange(DB.Companies.Where(x => companyIdsArray.Contains(x.ID)));
                    inPackage.Companies = companiesList;
                }
                catch (FormatException)
                {
                    //do nothing. If the string has an incorrect format, we add nothing and continue.
                }
            }

            return inPackage;
        }

I honestly believe this code is a mess and was hoping that it could be fixed by using C# generics like <T>, but I am not sure if this is possible due to the differences in getting each fields ID and Name.

Is it possible to optimize this function and make it more generic with protection from format exception? if so, how?

Consistency:

Don't mixup using var and declaring your variables explicitly. Choose one of the two, preferably var.

Combining methods:

Use method chaining to shorten your code. There's no need to create a variable for every step your code does. This:

var materialStringArray = materialIDs.Split(',');
var materialIdsArray = materialStringArray.Where(x => x != "").Select(int.Parse);

can be changed to:

var materialIdsArray = materialIDs.Split(',').Where(x => x != "").Select(int.Parse);

Generics:

You cannot make one magic generic method to do this. Why? Simply because you do not know the type ahead of time. What you can do however is to create separate method for getting the valid ids from the string:

public IEnumerable<int> GetValidIDs(string ids)
{
    foreach (var id in ids.Split(','))
    {
        int x;
        if (Int32.TryParse(id, out x))
        {
            yield return x;
        }
    }
}

private Package StringToCollections(Package inPackage, string materialIDs, string functionIDs, string companyIDs)
{
    var validMaterialIDs = GetValidIDs(materialIDs);
    var materials = DB.Materials.Where(x => validMaterialIDs.Contains(x.MaterialId));

    var validFunctionIDs = GetValidIDs(functionIDs);
    var functions = DB.Function.Where(x => validFunctionIDs.Contains(x.FunctionId)));

    var validCompanyIDs = GetValidIDs(functionIDs);
    var companies = DB.Companies.Where(x => validCompanyIDs.Contains(x.FunctionId)));

    inPackage.GATE_material = materials;
    inPackage.Function = functions;
    inPackage.Companies = companies;

    return inPackage;
}

Now your code looks much cleaner and is easier to read and maintain, not only by you but also other people.

You can also assign the database call directly to the according property of inPackage, for example:

var validMaterialIDs = GetValidIDs(materialIDs);
inPackage.GATE_material = DB.Materials.Where(x => validMaterialIDs.Contains(x.MaterialId));


Hangman mini-game on python I'm a 16 year old who has picked up coding recently using the codecademy course on Python. I completed the last of the lessons yesterday and decided to start on a simple Hangman game. As this is my first free-form project, it took me around 7h to finish it (a number i'm not proud of, i admit).

Here is the main body:

import string
import Hidden_word

try_again = 'y'

while try_again == 'y':
    word = Hidden_word.HiddenWord()
    hidden_word = word.hide_word()
    shown_word = word.show_word()
    chances_left = int(len(shown_word) / 2 + 1)
    misses = 0
    used_letters = []

    print("Here is the hidden word:", ' '.join(hidden_word))
    print("You have %d tries" % chances_left)

    while chances_left > 0:
        letter = input("Choose a letter: ").lower()
        while True:
            if not letter in string.ascii_lowercase or letter == '':
                letter = input("Please provide a valid answer: ").lower()
            elif letter in used_letters:
                letter = input("You've already used this letter, try another one: ").lower()
            else:
                used_letters.append(letter)
                break

        if letter in shown_word:
            print("Bingo!")
            for x in range(len(shown_word)):
                if shown_word[x] == letter:
                    hidden_word[x] = letter
            print(' '.join(hidden_word))
        else:
            chances_left -= 1
            if chances_left > 0:
                print ("Whoops, there is no '%s' in this word, try again! (%d chance(s) left)" % (letter, chances_left))
                print(' '.join(hidden_word))
                misses += 1

        if ''.join(hidden_word) == shown_word:
            if misses == 0:
                print("Congratulations! You did it without missing a single time!")
            else:
                print("Congratulations! You did it! But you've missed %d time(s)" % misses)
            break
        elif chances_left == 0:
            print("There's no '%s' in this word. You've got no tries left" % letter)
            print("The word was:", shown_word)

    try_again = input("Would you like to try again ? (y/n)").lower()

    while not try_again.lower() in ('y', 'n'):
        try_again = input("Please provide a valid answer: ")
print("Good bye!")

and here is the hidden_word class:

import linecache
import random

def number_lines ():
    with open("Words.txt", 'r') as file:
        for i, l in enumerate(file):
            pass
        return i + 1

class HiddenWord(object):
    hidden_word = []

    def __init__(self):
        self.shown_word = linecache.getline("Words.txt", random.randint(1, number_lines()))[:-1]

    def show_word (self):
        return self.shown_word

    def hide_word(self):
        self.hidden_word = []
        for x in self.shown_word:
            if x == ' ':
                self.hidden_word.append(' ')
            else:
                self.hidden_word.append('*')
        return self.hidden_word

I made sure to check all the possible cases, and it works perfectly without a hitch.

First, I'll mark a couple of things:

Parsing variables to strings.

You should just choose a form to parse variables into strings and stick with it.

print("Here is the hidden word:", ' '.join(hidden_word))
print("You have %d tries" % chances_left)

You're using .join and %d, %s to print variables. I suggest you to just use format (like I've done in the sample code), because you don't have to worry about parsing data. It allows you to be cleaner and has powerful options. Plus, % formatting is deprecated as far as I know.

The .join function is useful when you're using it to create a new string from a couple of different strings. Else, use format.

Classes and objects.

In your code, you're using a class with "only" a constructor.

The method show_word is kind of useless in Python, because you can access it directly, no need for a method to receive the element.
The method hide_word is used only one time. It should be called from the constructor and use it to create hidden_word.

The main.py code is just functional programming. You should use methods to make it cleaner and easier to read. Plus, you're just using your class to pass the elements from it to the while loop in the main, instead of using it as a class.

My final suggestion is that you look for Object Oriented Programming tutorials and examples, since you're using it very poorly and badly. Don't worry, you'll improve over time :)
Take a look to PEP8 style guide for Python too, it will help you to write cleaner and more readable.

I leave you this code as a suggestion, try to look for everything you don't understand.

import linecache
import random

class HiddenWord():

    def __init__(self, word):
        """Gets a word and turns it into a list. Then hides it as another list in hidden_word"""
        self.word = list(word.upper())
        self.hidden_word = []
        for letter in self.word:
            if letter == " ":
                self.hidden_word.append(" ")
            else:
                self.hidden_word.append('*')

    def check_letter(self, letter):
        """Checks if the letter received is in the word. If it is,
           it will make it visible in the hidden word, and will return true.
           If it isn't, it will just return false."""
        letter = letter.capitalize()
        if letter in self.word:
            for index in range(len(self.hidden_word)):
                if(self.word[index] == letter):
                    self.hidden_word[index] = letter

            return True

        return False

class Game():
    def number_lines(self):
        """Gets the number of lines that the txt has"""
        num_lines = sum(1 for line in open('Words.txt'))
        return num_lines

    def printInfo(self):
        """Shows by console tries left, the hidden word and the used letters"""
        print("Tries left: {}".format(self.chances_left))
        print("Hidden word: {}".format(self.hw.hidden_word))
        print("Used letters: {}".format(self.used_letters))

    def __init__(self):
        """Gets a word from the .txt and initialises the hidenword class"""
        word = linecache.getline("Words.txt", random.randint(1, self.number_lines()))[:-1]
        self.hw = HiddenWord(word)
        self.chances_left = int(len(self.hw.word) / 2 + 1)
        self.used_letters = []
        self.misses = 0

    def gameFinish(self):
        """Check if the player has won"""
        if (self.hw.hidden_word == self.hw.word):
            return True

        return False

    def play(self):
        """Main game"""
        while(self.chances_left > 0 and not self.gameFinish()):
            self.printInfo()
            letter = ""
            while(not letter or letter in self.used_letters):
                letter = input("Choose a letter: ")

            self.used_letters.append(letter)

            if(self.hw.check_letter(letter)):
                print("Bingo! That letter is in the word.")
            else:
                print("That letter is not in the word.")
                self.misses += 1
                self.chances_left -= 1

            if(self.gameFinish()):
                if(self.misses == 0):
                    print("Congratulations! You did it without missing a single time!")
                else:
                    print("Congratulations! You did it! But you've missed {} time(s)".format(self.misses))
                break

        print("You lost. The word was {}.".format(self.hw.word))

if __name__ == '__main__':
    """Main loop"""
    try_again = 'y'
    while try_again == 'y':
        game = Game()
        game.play()

        try_again = input("Would you like to try again ? (y/n)").lower()

        while not try_again.lower() in ('y', 'n'):
            try_again = input("Please provide a valid answer: ")

    print("See you!")


Google Gson - adding arrays to a List I want to make sure if it's good. Maybe there is another better solution.

List<String[]> list = new ArrayList<String[]>();
    for (int i = 0; i < 3; i++) {
        String[] arr = { String.valueOf(i) };
        list.add(arr);
    }

    String json = new Gson().toJson(list);
    System.out.println(json);

Expected output:

[
[
"0"
],
[
"1"
],
[
"2"
]
]

If you are able to use Java 8 you can use streams:

List<String[]> list = IntStream.range(0, 3)
    .mapToObj(i -> new String[] { String.valueOf(i) })
    .collect(Collectors.toList());

Let's break it down:

IntStream.range(0, 3) gives you the values 0, 1 and 2
.mapToObj(i -> new String[] { String.valueOf(i) }) converts them to a string and puts them into a String[]
.collect(Collectors.toList()) collects the stream into a List<String[]>


Performance of speech enhancement code for Android app I wrote a speech enhancement code for an Android App. The algorithm runs on 256 size frames of voice samples. On my PC the code runs per about 5ms per frame, while on my Nexsus 5 it more like 50ms per frame, making the speech enhancement on a 30sec long recording run for over two minutes. On my PC, enhancing the whole audio file takes under a minute. I am looking to improve run time anywhere I can. I am attaching the code that runs per frame which is where I see biggest increase in run time.

for(int j = 0; j < Consts.NUM_OF_CLUSTERS; j++)
{
    tmp_log_h=0;
    for(int k = 0; k < Consts.SIZE_OF_SEMPELS_IN_CHUNCK; k++)
    {

        f_X = (1/FastMath.sqrt(2*FastMath.PI*sigmaSqr.get(j).get(k)))*FastMath.exp(-(FastMath.pow(NoisySignal[k] - mue.get(j).get(k),2))/(2*sigmaSqr.get(j).get(k)));// equation (3)
        F_X=(0.500 + 0.5*Erf.erf((NoisySignal[k] - mue.get(j).get(k))/(FastMath.sqrt(2*sigmaSqr.get(j).get(k)))));// equation (6)

        if(j==0)
        {
            g_pdf_Y[k]=1/FastMath.sqrt(2*FastMath.PI*Noise_sigmaSqr.get(k))*FastMath.exp(-(FastMath.pow(NoisySignal[k] - Noise_mu.get(k),2))/(2*Noise_sigmaSqr.get(k)));// equation (4)
            G_cdf_Y[k]=(0.500 + 0.5*Erf.erf((NoisySignal[k] - Noise_mu.get(k))/(FastMath.sqrt(2*Noise_sigmaSqr.get(k)))));// equation (5)
        }

        tmp_R_samples = (f_X/F_X);// equation (12.5)

        if(j==0)
        {
            R_noise[k]=g_pdf_Y[k]/G_cdf_Y[k]; // equation (12.5)
        }

        Roe[j][k] = 1/(1 + (R_noise[k]/tmp_R_samples));
        X_hat[j][k] = NoisySignal[k]*Roe[j][k]+ (mue.get(j).get(k))*(1 - Roe[j][k]) - (sigmaSqr.get(j).get(k))*((f_X*R_noise[k])/(f_X + F_X*R_noise[k]));
        tmp_h_first=(f_X*G_cdf_Y[k] + F_X*g_pdf_Y[k]);// equation (7.5)
        tmp_log_h += FastMath.log(tmp_h_first); // equation (7.5)
    }

    Log_h_second[j]=tmp_log_h;
    Log_h_final_vecotr[j]=FastMath.log(c.get(j)) + Log_h_second[j];
}

Double maxVi = MathHelpers.findMax(Log_h_final_vecotr);

double sumExp = 0.0;

for(int j = 0; j < Consts.NUM_OF_CLUSTERS; j++)
{
    sumExp +=(FastMath.exp(Log_h_final_vecotr[j] - maxVi));// equation (18)
    System.out.println(sumExp);
}

Log_h_final = maxVi + FastMath.log(sumExp);

for(int j = 0; j < Consts.NUM_OF_CLUSTERS; j++)
{
    Log_q[j]=FastMath.log(c.get(j)) + Log_h_second[j] - Log_h_final;
    q[j]=(FastMath.exp(Log_q[j]));// equation (10)
}

for(int k = 0; k < Consts.SIZE_OF_SEMPELS_IN_CHUNCK; k++)
{
    Double temp_X_estimate = 0.0;

    for(int j = 0; j < Consts.NUM_OF_CLUSTERS; j++)
    {
        temp_X_estimate += (X_hat[j][k]*q[j]);
    }

    X_estimate[k]=temp_X_estimate;// equation (9)
}

Performance

I cannot offer any mayor improvements, just some small things:

You should save results of a calculation or action instead of doing them over and over.

For example:

2*FastMath.PI: this is not dependent on the loop variables, just save it in a variable (same goes for 2*sigmaSqr.get(j).get(k) which you are doing three times for every loop; and NoisySignal[k] - mue.get(j).get(k) which you are doing twice). These duplicate calculations are quite hard to catch in your code. If you write the equations on paper, it becomes easier to spot.
mue.get(j).get(k): you are doing this three times. I don't know what mue is, but depending on the kind of list, it may be expensive (same goes for sigmaSqr.get(j).get(k))

You could also move the calculation of X_estimate inside the previous j loop:

for(int j = 0; j < Consts.NUM_OF_CLUSTERS; j++)
{
    Log_q[j]=FastMath.log(c.get(j)) + Log_h_second[j] - Log_h_final;
    q[j]=(FastMath.exp(Log_q[j]));// equation (10)

    Double temp_X_estimate = 0.0;
    for(int k = 0; k < Consts.SIZE_OF_SEMPELS_IN_CHUNCK; k++)
    {
        temp_X_estimate += (X_hat[j][k]*q[j]);
    }
    X_estimate[k]=temp_X_estimate;// equation (9)
}

It will save you running through the loop once, you can profile it to see if it is worth the loss in readability.

Readability

Code Structure

I would probably pull out the j==0 case in front of the loop. Keeping it in the loop is confusing and seems unnecessary.

Function calls do cause an overhead, but I would still consider extracting some of the code to separate functions (this would make profiling the code easier).

Additional variables also cause a slight overhead, but some of your expressions are quite long and hard to grasp. You might consider adding intermediate variables with an expressive name saving a temporary state of the calculation. An alternative would be to use newlines to present the expressions in a better way.

Variable Names

I don't think that your variable names are very expressive. I'm not that deep into the topic, but I would guess that at least some of them can be improved upon.

Also, just using case to differentiate names is quite confusing (f_X vs F_X).

Comments

I'm guessing that you have some kind of documentation to which your current comments refer. But without it, they are not that useful. It might be helpful to add this documentation (or a short excerpt) to the top.

Style

Please follow general style guidelines.

variable names start with a lowercase letter (this lets a reader easily see that it is a variable, not a class)
declare variables where they are used (tmp_log_h for example is declared outside the loop, even though it is not needed there)
use more spaces (for example, before and after ==, =, /, *, etc)
in Java, camelCase is usually used instead of underscores (you do this sometimes (eg maxVi or sumExp, but mostly you use underscores)

Misc

get rid of unnecessary brackets (for example (X_hat[j][k]*q[j]), tmp_R_samples = (f_X/F_X), or f_X = (...); especially in the last example, the expression already has enough parentheses without the extra once :) )


Substring function in C I am trying to implement substring in C. How efficient and standard is this code? I have a hard time understanding how C functions are usually meant to work, like how printf() returns an int or size_t.

size_t substring(char *destination, char const *string,  int start, int len) {
    // writes a substring to the destinaton starting at start index of string until end stepping by step
    // follows the same start at 0 off by one on the end format as Java substring method
    int substringLength = len;

    int stringLength = strlen(string);

    if (start > stringLength || len > stringLength || start < 0 || len < 0) {
        fputs("start and len must be 0 < start/len < length of string", stderr);
        return -1;
    }
    if (start >= len) {
        fputs("start must be start < len", stderr);
        return -1;
    }

    memcpy(destination, &string[start], substringLength);

    if (destination[stringLength] != '\x0') {
        destination[stringLength] = '\x0';
    }
    return substringLength;
}

Argument confusion
If you were trying to replicate the java substring function, you should have had a start and end argument.  As it is, you have a start and length argument, but you didn't handle the length argument correctly.
Bug 1
This line doesn't correctly check for the case of copying off the end of the source string:

if (start > stringLength || len > stringLength || start < 0 || len < 0) {

It needs to be:
if (start + len > stringLength || start < 0 || len < 0) {

Edit: If you want to check for integer overflow, you can also throw in || start + len < 0, and use size_t everywhere instead of int.
Bug 2
This check is unnecessary and can return an error incorrectly:

if (start >= len) {
    fputs("start must be start < len", stderr);
    return -1;
}

For example, if the string has 10 bytes, start is 8 and len is 1, this check will return an error.
Bug 3
When you terminate the destination string, you use the wrong index.  It should be len, not stringLength.  Also, I'm not sure why you do a check before you write to it:

if (destination[stringLength] != '\x0') {
    destination[stringLength] = '\x0';
}

should be:
destination[len] = '\0';

Also, '\0' is the standard way to write the null character.  I've never seen it written as '\x0' before.
Unnecessary local variable
The variable substringLength is unnecessary because it is always the same as len.  You can just use len instead.
Return value
The java substring function returns the string.  Your function returns the length but I don't think that this will be useful to the caller.  The reason you might want to return a string is so you can chain calls like this:
strcat(substring(buf, path, dir_start, dir_length), basename);


Coloured bash prompt including git status and previous exit code I wanted to create a short, informational and colourful prompt.

My prompt includes:

Exit status of last command (if not 0)
Distinctive changes when root
rsync-style user@host:pathname for copy-paste goodness
Git branch, index, modified, untracked and upstream information
Pretty colours

Example:

Is there a way to achieve a similar result with less lines of code?

In ~/.bashrc:

#
# Set the prompt #
#

# Select git info displayed, see /usr/lib/git-core/git-sh-prompt for more
export GIT_PS1_SHOWDIRTYSTATE=1           # '*'=unstaged, '+'=staged
export GIT_PS1_SHOWSTASHSTATE=1           # '$'=stashed
export GIT_PS1_SHOWUNTRACKEDFILES=1       # '%'=untracked
export GIT_PS1_SHOWUPSTREAM="verbose"     # 'u='=no difference, 'u+1'=ahead by 1 commit
export GIT_PS1_STATESEPARATOR=''          # No space between branch and index status
export GIT_PS1_DESCRIBE_STYLE="describe"  # detached HEAD style:
#  contains      relative to newer annotated tag (v1.6.3.2~35)
#  branch        relative to newer tag or branch (master~4)
#  describe      relative to older annotated tag (v1.6.3.1-13-gdd42c2f)
#  default       exactly eatching tag

# Check if we support colours
__colour_enabled() {
    local -i colors=$(tput colors 2>/dev/null)
    [[ $? -eq 0 ]] && [[ $colors -gt 2 ]]
}
unset __colourise_prompt && __colour_enabled && __colourise_prompt=1

__set_bash_prompt()
{
    local exit="$?" # Save the exit status of the last command

    # PS1 is made from $PreGitPS1 + <git-status> + $PostGitPS1
    local PreGitPS1="${debian_chroot:+($debian_chroot)}"
    local PostGitPS1=""

    if [[ $__colourise_prompt ]]; then
        export GIT_PS1_SHOWCOLORHINTS=1

        # Wrap the colour codes between \[ and \], so that
        # bash counts the correct number of characters for line wrapping:
        local Red='\[\e[0;31m\]'; local BRed='\[\e[1;31m\]'
        local Gre='\[\e[0;32m\]'; local BGre='\[\e[1;32m\]'
        local Yel='\[\e[0;33m\]'; local BYel='\[\e[1;33m\]'
        local Blu='\[\e[0;34m\]'; local BBlu='\[\e[1;34m\]'
        local Mag='\[\e[0;35m\]'; local BMag='\[\e[1;35m\]'
        local Cya='\[\e[0;36m\]'; local BCya='\[\e[1;36m\]'
        local Whi='\[\e[0;37m\]'; local BWhi='\[\e[1;37m\]'
        local None='\[\e[0m\]' # Return to default colour

        # No username and bright colour if root
        if [[ ${EUID} == 0 ]]; then
            PreGitPS1+="$BRed\h "
        else
            PreGitPS1+="$Red\u@\h$None:"
        fi

        PreGitPS1+="$Blu\w$None"
    else # No colour
        # Sets prompt like: ravi@boxy:~/prj/sample_app
        unset GIT_PS1_SHOWCOLORHINTS
        PreGitPS1="${debian_chroot:+($debian_chroot)}\u@\h:\w"
    fi

    # Now build the part after git's status

    # Highlight non-standard exit codes
    if [[ $exit != 0 ]]; then
        PostGitPS1="$Red[$exit]"
    fi

    # Change colour of prompt if root
    if [[ ${EUID} == 0 ]]; then
        PostGitPS1+="$BRed"'\$ '"$None"
    else
        PostGitPS1+="$Mag"'\$ '"$None"
    fi

    # Set PS1 from $PreGitPS1 + <git-status> + $PostGitPS1
    __git_ps1 "$PreGitPS1" "$PostGitPS1" '(%s)'

    # echo '$PS1='"$PS1" # debug
    # defaut Linux Mint 17.2 user prompt:
    # PS1='${debian_chroot:+($debian_chroot)}\[\033[01;32m\]\u@\h\[\033[01;34m\] \w\[\033[00m\] $(__git_ps1 "(%s)") \$ '
}

# This tells bash to reinterpret PS1 after every command, which we
# need because __git_ps1 will return different text and colors
PROMPT_COMMAND=__set_bash_prompt

[Note: the current code based upon feedback received is here]

Solid script, nicely done.

"less lines of code"

__colour_enabled is used only once,
when setting __colourise_prompt, which in turn is also used only once.
It seems an unnecessary indirection.
You could drop this line:

unset __colourise_prompt && __colour_enabled && __colourise_prompt=1

And change this condition:

if [[ $__colourise_prompt ]]; then

To this:

if __colour_enabled; then

Some color variables are defined but never used.
But I would keep them anyway,
in case you (or somebody else) want to adjust the colors later
(they are not exactly easy to remember).

To shorten the code, you could rewrite some of the if statements in cmd && ... || ... form, but I don't recommend that.
In its current form it's nicely readable and good that way.

Undefined variables

When you detect that colors are enabled, you set a bunch of variables, for example Red, BRed, Mag, None. If colors are not enabled, you don't clear these variables, but they may be used anyway later in the script.

Although it may seem unlikely that these variables are set outside this script, it can happen,
and that will be confusing and possibly annoying.
The clean solution is to make sure to set those variables to blank when colors are not enabled.

Condition without pattern matching

The == operator in [[ is useful for pattern matching.
In this example you don't need pattern matching,
so I suggest to replace it with =, which will do simple literal comparison.

if [[ ${EUID} == 0 ]]; then

Coding style

You mixed two styles of function declaration:

__colour_enabled() {
    # ...
}

__set_bash_prompt()
{
    # ...
}

It would be better to use consistent style (I prefer the first one).


Speed up solution to Project Euler problem 75 I wrote this code for Project Euler problem 75, which asks how many integers ≤ 1500000 exist, where the integer is a  perimeter length that can be divided into three integer-length sides of a right triangle in one unique way.

I was curious if anyone knew how to improve its speed. It runs fine, but I'm just looking to improve my own coding know-how.

from functools import reduce
import math

primes=set([2,3,5,7,11,13,17,19,23])
def isPrime(n):
    n=abs(n)
    if n in primes:
       return True
    if n<2:
       return False
    if n==2:
       return True
    if n%2==0:
       return False
    for x in range(3, int(n**0.5)+1, 2):
        if n % x == 0:
            return False
    primes.add(n)
    return True

def aFactors(n):
    if isPrime(n):
        return [1,n]
    return set(reduce(list.__add__,([i,n//i] for i in range(1,int(math.sqrt(n))+1) if n%i==0)))

count=0
number=12
while number<=1500000:
    p=number/2
    f=aFactors(p)
    triangles=[]
    pairs=[(i, int(p/i)) for i in f]
    add=triangles.append
    for i in pairs:
        mList=aFactors(i[0])
        pairsOfmc=[(k,int(i[0]/k)) for k in mList]
        for j in pairsOfmc:
            add((2*i[1]*i[0]-i[1]*i[1]*j[0],2*i[0]*i[1]-2*i[0]*j[1],2*i[0]*j[1]+i[1]*i[1]*j[0]-2*i[0]*i[1]))
    r=0
    while r<len(triangles):
        if any(triangles[r][i]<=0 for i in range(len(triangles[r]))):
            del triangles[r]
        else:
            l=list(triangles[r])
            l.sort()
            triangles[r]=tuple(l)
            r+=1
    trianglesFinal=list(set(triangles))
    for i in trianglesFinal:
        print(number, i)
    if len(trianglesFinal)==1:
        count+=1
    number+=2
print(count)

Please note that I am not looking for a different calculating method (I am sure there is one, but, for me, Project Euler is about finding your own methods.  Using yours would, to me, be cheating). However, any faster functions, ways to combine blocks of code, simplified tests, or the like (such as not checking ever other number, but every xth number, etc) would be greatly appreciated!

For the record, this is Project Euler problem 75.

1. Admonition

You write:

  Please note that I am not looking for a different calculating method (I am sure there is one, but, for me, Project Euler is about finding your own methods. Using yours would, to me, be cheating.)

Abandon this attitude! A key part of programming — just as important as coding — is developing a wide repertoire of algorithms and techniques that you can apply to problems you face. Although for your own intellectual satisfaction it's great to discover algorithms on your own, you should always go on to compare your solution to the best solutions discovered by others, so that you can improve your repertoire for the next problem.

In particular, when you want to speed up a program, it's counterproductive to say, "I don't want to implement a different algorithm, I just want to speed up the code I wrote". The biggest speedups come from finding better algorithms.

You say that your program "runs fine" but that doesn't seem to be true. Project Euler says:

  Each problem has been designed according to a "one-minute rule", which means that although it may take several hours to design a successful algorithm with more difficult problems, an efficient implementation will allow a solution to be obtained on a modestly powered computer in less than one minute.

I tried running your program, but after ten minutes it still had not produced an answer (and my laptop was running hot), so I killed it.

So in section 2 below I'll be using a smaller test size, 100,000 instead of 1,500,000, to keep the runtimes manageable.

2. Piecewise optimization

Your program is hard to test from the interactive interpreter because it has code at the top level. Better to put the main program inside a function so that you can call it from interpreter and then add an if __name__ == '__main__': section so that you can run it as a script if you want to.

I've used the function name problem75, and it takes an argument, which is the largest value of the length of wire in the problem. Now:

>>> from timeit import timeit
>>> timeit(lambda:problem75(10**5),number=1)
12 (3, 4, 5)
24 (6, 8, 10)
30 (5, 12, 13)
[... much output deleted ...]
33.87288845295552

The print statement near the end of the main loop is unnecessary. This saves about 3 seconds, bringing the time down to 31.0 seconds.
Your function aFactors makes an initial call to isPrime, taking \$ O(\sqrt n) \$, in order to avoid a loop that also takes \$ O(\sqrt n) \$. The test costs as much as it saves, so it's not worth it. Remove the call to isPrime and rewrite the function like like this:

def factors(n):
    """Return the set of factors of n."""
    result = set([1, n])
    add = result.add
    for i in range(2, int(math.sqrt(n)) + 1):
        if n % i == 0:
            add(i)
            add(n // i)
    return result

Here I've picked a better name for the function, written a docstring explaining what it does, and cached the value of result.add so that it does not need to be looked up each time arounnd the loop. This saves about 4 seconds; time now 27.7 seconds.
Since the only thing you do with the set of factors is to turn it into pairs which you then iterate over, why not generate the pairs, like this:

def product_pairs(n):
    """Generate pairs (i, j) such that i * j = n."""
    yield 1, n
    yield n, 1
    for i in range(2, int(math.sqrt(n)) + 1):
        if n % i == 0:
            j = n // i
            yield i, j
            yield j, i

and then process them like this:

triangles = []
append = triangles.append
for i in product_pairs(number // 2):
    for j in product_pairs(i[0]):
        append((2*i[1]*i[0]-i[1]*i[1]*j[0],2*i[0]*i[1]-2*i[0]*j[1],2*i[0]*j[1]+i[1]*i[1]*j[0]-2*i[0]*i[1]))

This avoids having to construct an intermediate list in memory. This saves about 3 seconds; time now 24.3 seconds.
Instead of writing for i in product_pairs(...): and then looking up i[0] and i[1], split up the pair into two variables when you assign it in the loop, like this:

for i, j in product_pairs(number // 2):
    for k, l in product_pairs(i):
        append((2*j*i - j*j*k, 2*i*j - 2*i*l, 2*i*l + j*j*k - 2*i*j))

This avoids the sequence lookups. This saves about 3 seconds; time now 21.2 seconds.
You construct a list of triangles, some of which have sides with negative or zero length. You then go through the list and del the triangles which are invalid. But del on a list is a potentially expensive operation: it has to copy the remainder of the list to keep it contiguous. See the TimeComplexity page on the Python wiki for the cost of basic operations on Python's built-in data structures, where you can see that "delete item" operation on a list takes \$O(n)\$.

So don't add these invalid triangles to the list in the first place. In fact, don't bother with the list at all, just construct the set of triangles directly:

triangles = set()
add = triangles.add
for i, j in product_pairs(number // 2):
    for k, l in product_pairs(i):
        triangle = 2*j*i - j*j*k, 2*i*j - 2*i*l, 2*i*l + j*j*k - 2*i*j
        if all(side > 0 for side in triangle):
            add(tuple(sorted(triangle)))

This saves about 6 seconds; time now 15.1 seconds.
You can avoid some repeated multiplications by naming the various products of i, j, k, and l:

for i, j in product_pairs(number // 2):
    for k, l in product_pairs(i):
        p, q, r = 2 * j * i, j * j * k, 2 * i * l
        triangle = p - q, p - r, r + q - p
        if all(side > 0 for side in triangle):
            add(tuple(sorted(triangle)))

This saves about 1 second; time now 13.9 seconds.

And I'm afraid that's as far as I got by applying piecewise speedups to your code. About 60% speedup in total. The revised program now gets the solution to the full Project Euler problem in about eight minutes on my laptop:

>>> timeit(lambda:main(1500000),number=1)
[... solution deleted ...]
467.3059961795807

Perhaps some other participant here can make more piecewise progress, but for dramatic speedups, you really need ...

3. A better algorithm

(Avert your eyes if you must.)

The basic insight is that instead of iterating over the perimeter and finding the Pythagorean triples with that perimeter, we can iterate over all Pythagorean triples in some more convenient order, and keep count of how many triangles are found at each perimeter length.

We can further speed things up by only generating the primitive Pythagorean triples, and then multiplying the primitive triples by successive numbers 1, 2, 3, ... to generate the remaining Pythagorean triples in the required range. In fact, we only need to multiply their perimeters.

Euclid’s formula can be used to generate all primitive Pythagorean triples. Given coprime positive integers \$m\$ and \$n\$, with \$m > n\$, and exactly one of \$m\$ and \$n\$ even, $$ \eqalign{ a &= m^2 − n^2 \\ b &= 2mn \\ c &= m^2 + n^2} $$ is a primitive Pythagorean triple.

The remaining subtlety is that Euclid’s formula doesn’t generate triples in order by length of the perimeter, so there needs to be some way to determine when to stop. My strategy in the code below is to iterate over ascending values of \$m\$. The perimeter of the triple is \$a + b + c = 2m^2 + 2mn\$, which is at least \$2m(m + 1)\$, since \$n ≥ 1\$. So \$2m(m + 1)\$ is a lower bound on the perimeter for which all triples have been generated so far. When this exceeds the limit, we can stop the search: there are no more triples to be found in the required range.

from collections import Counter
from fractions import gcd
from itertools import count

def coprime(m, n):
    """Return True iff m and n are coprime."""
    return gcd(m, n) == 1

def all_primitive_triples():
    """Generate all primitive Pythagorean triples, together with a lower
    bound on the perimeter for which all triples have been generated
    so far.

    """
    for m in count(1):
        for n in range(1, m):
            if (m + n) % 2 and coprime(m, n):
                a = m * m - n * n
                b = 2 * m * n
                yield a, b, m * m + n * n, 2 * m * (m + 1)

def problem75(limit=1500000):
    """Return the number of values of L <= limit such that there is
    exactly one integer-sided right-angled triangle with perimeter
    L.

    """
    triangles = Counter()
    for a, b, c, q in all_primitive_triples():
        if q > limit:
            break
        p = a + b + c
        for i in range(p, limit + 1, p):
            triangles[i] += 1
    return sum(n == 1 for n in triangles.values())

This solves the Project Euler problem in a more reasonable amount of time:

>>> timeit(problem75, number=1)
2.164217948913574