Project Euler #14 in Java /*
 * The following iterative sequence is defined for the set of positive
 * integers:
 *
 * n -> n/2 (n is even) n -> 3n + 1 (n is odd)
 *
 * Using the rule above and starting with 13, we generate the following
 * sequence:
 *
 * 13 -> 40 -> 20 -> 10 -> 5 -> 16 -> 8 -> 4 -> 2 -> 1 It can be seen that
 * this sequence (starting at 13 and finishing at 1) contains 10 terms.
 * Although it has not been proved yet (Collatz Problem), it is thought that
 * all starting numbers finish at 1.
 *
 * Which starting number, under one million, produces the longest chain?
 *
 * NOTE: Once the chain starts the terms are allowed to go above one
 * million.
 */
public static int get14() {
    int max = 1;
    int ans = 1;
    // Map<Integer, Integer> t_cache = new HashMap<Integer, Integer>();
    for (int i = 2; i < 1000000; i++) {
        long n = i;
        int t = 0;
        do {
            // if (t_cache.containsKey(n)) {
            // t += t_cache.get(n);
            // break;
            // }
            ++t;
            if (n % 2 == 0) {
                n /= 2;
            } else {
                n = 3 * n + 1;
            }
            // t_cache.put(i, t);
        } while (n != 1);
        // System.out.println(i + "->" + t);
        if (t > max) {
            max = t;
            ans = i;
        }
    }
    return ans;
}

Using no cache I get:

  Answer is 837799, Time taken 22.08799153 seconds

Using cache I get:

  Answer is 837799, Time taken 90.002679307 seconds {really?}

I need improvements for this.

Cache implementation had bugs

Actually your cache implementation would have helped except that you didn't quite write it correctly.

The t_cache.put() is in the wrong spot.  It should be outside the while loop since you don't know t until the loop finishes.
This function t_cache.containsKey(n) is actually returning false always.  You need to cast n to an int like this: t_cache.containsKey((int) n).  Otherwise since n is a long, it doesn't actually look up the right key.  I believe it is attempting to look up a Long key instead.

With this fixed up code, it runs 3.7x faster than without the cache:

public static int get14() {
    int max = 1;
    int ans = 1;
    Map<Integer, Integer> t_cache = new HashMap<Integer, Integer>();

    t_cache.put(1, 0);
    for (int i = 2; i < 1000000; i++) {
        long n = i;
        int t = 0;
        do {
            ++t;
            if (t_cache.containsKey((int)n)) {
                t += t_cache.get((int) n);
                break;
            }
            if (n % 2 == 0) {
                n /= 2;
            } else {
                n = 3 * n + 1;
            }
        } while (n != 1);
        t_cache.put(i, t);
        // System.out.println(i + "->" + t);
        if (t > max) {
            max = t;
            ans = i;
        }
    }
    return ans;
}

How to get even faster

Maaartinus already pointed out how to use bit operators to improve your speed.  Using the bit operators in addition to the HashMap cache improves the speed to 4.5x the original (from 3.7x, or a 22% speedup).  But this is still slower than just using the simple loop in maaartinus's answer (5.3x faster than original).

The problem is that the cache implementation can still be improved.

You don't need to call t_cache.containsKey() at all.  Since you are proceeding in order, the check if (n < i) will do the same thing, since you know that at any i, all the results below i have been found and cached.
You only need to check the cache in the "even" case, where n is decreasing.  In the "odd" case, n will increase so there is no chance that the cache will be hit.
You can use an array instead of a HashMap.  You are filling in the cache sequentially, so there is no reason for hashing.

So here is the final version with all of the improvements:

public static int get14() {
    int max = 1;
    int ans = 1;
    int [] t_cache = new int[1000000];

    for (int i = 2; i < 1000000; i++) {
        long n = i;
        int t = 0;
        do {
            ++t;
            if ((n & 1) == 0) {
                n >>= 1;
                if (n < i) {
                    t += t_cache[(int)n];
                    break;
                }
            } else {
                n = 3 * n + 1;
            }
        } while (true);
        t_cache[i] = t;
        if (t > max) {
            max = t;
            ans = i;
        }
    }
    return ans;
}

And here is the speed of each implementation:

Implementation                 Time    Speedup from original
---------------------------    ----    ---------------------
Original (uncached)            3.33           1.0x
HashMap                        0.90           3.7x
HashMap + bit ops              0.74           4.5x
maaartinus (bit ops)           0.63           5.3x
JS1 (Array cache + bit ops)    0.10          33.3x


Listener pattern implementation In my project, I am doing asynchronous processes in almost every classes. To explain my problem, I have created a sample:

public interface TestListener {
  void onResponse(Response result);
  void onError(Exception e);
}

public class AsyncTask {
  TestListener testListener;

  public AsyncTask(TestListener testListener, Request request)
  {

  }

  public void excute()
  {
    try {
      // process request
      Response response = new Response();
      testListener.onResponse(response);
    } catch (Exception e) {
      testListener.onError(e);
    }
  }
}

public class Test1 implements TestListener {

  public void doStuff()
  {
    Request request = new Request();
    // add some stuff to request;
    AsyncTask task = new AsyncTask(this, request);
    task.excute();
  }

  @Override
  public void onResponse(Response result)
  {
    // process response

  }

  @Override
  public void onError(Exception e)
  {
    // process error

  }
}

public class Test2 implements TestListener {

  void someStuff()
  {
    Request request = new Request();
    // add some stuff to request;
    AsyncTask task = new AsyncTask(this, request);
    task.excute();
  }

  @Override
  public void onResponse(Response result)
  {
    // process response

  }

  @Override
  public void onError(Exception e)
  {
    // process error

  }
}

This doesn't look good to me. In every class I have onResponse and onError. What I want to do is just create a helper class that will have onResponse and onError and it will return me the response.

public class Test1{
  public void doStuff(){
    AsyncHelper helper = AsyncHelper.getInstance();
    Response response = helper.test1Method(request);
  }
}

I have two questions:

Am I thinking correctly to remove onResponse code from every class and collect in one class.
How should I create an AsyncHelper class?

Naming the classes and the design (at least as far as I can see from the code you pasted) suggests that you want to use the observer pattern. The main idea of this pattern is to use listeners which implement common listener interface and register them for the specific object which you want to make 'observable'. Then in the observable object you can call notifyAllListeners() method or notifyAllListenersOnError() or some similar which will do nothing else than for listener from registered listeners call listener.notify() (or listener.onError() etc.).

Methods notify(), onError() etc have to be declared in the common listener interface.

What I mean is described here.

The main idea is:

Create observable object.
Create as many listeners which implements common listener interface.
Register all created listeners for the observable object.
In the observable object call notifyAll code.

I also created a short article which explains the idea.


GSON parsing - nested classes or not Today I was trying GSON after a while and went to the YQL console to run this query:

select * from yahoo.finance.xchange where pair in ("EURUSD","GBPUSD","INRUSD","JPYUSD")

and get back this JSON:

{
 "query": {
  "count": 4,
  "created": "2014-12-11T18:41:23Z",
  "lang": "en-US",
  "results": {
   "rate": [
    {
     "id": "EURUSD",
     "Name": "EUR to USD",
     "Rate": "1.238",
     "Date": "12/11/2014",
     "Time": "1:41pm",
     "Ask": "1.2382",
     "Bid": "1.2378"
    },
    {
     "id": "GBPUSD",
     "Name": "GBP to USD",
     "Rate": "1.5705",
     "Date": "12/11/2014",
     "Time": "1:41pm",
     "Ask": "1.5708",
     "Bid": "1.5703"
    },
    {
     "id": "INRUSD",
     "Name": "INR to USD",
     "Rate": "0.016",
     "Date": "12/11/2014",
     "Time": "1:40pm",
     "Ask": "0.016",
     "Bid": "0.016"
    },
    {
     "id": "JPYUSD",
     "Name": "JPY to USD",
     "Rate": "0.0084",
     "Date": "12/11/2014",
     "Time": "1:41pm",
     "Ask": "0.0084",
     "Bid": "0.0084"
    }
   ]
  }
 }
}

To parse it I wrote this code which works fine:

Wrapper.java

public class Wrapper {
    private Query query;

    public Query getQuery() { return query; }
}

Query.java

public class Query {
    private int count;
    private Date created;
    private String lang;
    private Result results;

    public int getCount() { return count; }
    public Date getCreated() { return created; }
    public String getLang() { return lang; }
    public Result getResults() { return results; }

}

Result.java

public class Result {
    private Rate[] rate;

    public Rate[] getRate() { return rate; }
}

Rate.java

public class Rate {
    private String id;
    private String Name;
    private Double Rate;
    private String Date;
    private String Time;
    private Double Ask;
    private Double Bid;

    public String getId() { return id; }
    public String getName() { return Name; }
    public Double getRate() { return Rate; }
    public String getDate() { return Date; }
    public String getTime() { return Time; }
    public Double getAsk() { return Ask; }
    public Double getBid() { return Bid; }
}

I created separate classes and then in a main class with a main method created the Gson object and did this:

Gson gson = new GsonBuilder().create();
Wrapper wrapper = gson.fromJson(jsonString, Wrapper.class);

But what differences would there be if I had the above classes as nested classes in the Wrapper.java class? Or is creating them separately and then putting them in a package more preferable?

What are the differences in terms of speed, efficiency, design, or anything else anyone could advise on?

But what differences would there be if I had the above classes as
  nested classes in the Wrapper.java class? Or is creating them
  separately and then putting them in a package more preferable?

It depends on your information hiding principles.
In general, it's good to hide as much information as possible.
The fewer classes you expose,
the fewer classes you have to support,
and therefore the more freedom you have to make changes later.

So, if these classes don't need be visible by users of your library,
then they can be nested inside Wrapper.
Wrapper too, looks like something that shouldn't be exposed.

On the other hand: (from Effective Java by Joshua Bloch)

  A nested class should exist only to serve its enclosing class. If a nested class would be useful in some other context, then it should be a top-level class.

  What are the differences in terms of speed, efficiency, design, or
  anything else anyone could advise on?

Speed, nothing. Efficiency, nothing.
The difference is in API design.
Everything that's public is a liability for you.
Once other developers start using it,
you'll have to pay attention to the changes you make,
in case you might break compatibility.


Z-Algorithm for pattern matching in strings I was trying to refactor the following Python code (keeping the same time-complexity) which is an implementation of Z-Algorithm for pattern matching in strings.

def compute_z(string):
   n = len(string)
   z = [n]

   L = R = 0
   for i in xrange(1,n):
        if (i > R):
            L = R = i
            while (R < n and string[R-L] == string[R]): R+=1
            z.append(R-L)
            R -= 1
        else:
            k = i-L
            if (z[k] < R-i): z.append(z[k])
            else:
                L = i
                while (R < n and string[R-L] == string[R]): R += 1
                z.append(R-L)
                R -= 1

I was thinking of using lambda but it returns expressions and I need to store the values in z array. So I could not think of a way of using lambda.

The max I could refactor was this:

def g(L,R,string):
    L=i
    while (R < len(string) and string[R-L] == string[R]): R += 1
    return L,R

def compute_z(string):
   n = len(string)
   z = [n]

   L = R = 0
   for i in xrange(1,n):
        if (i > R):
            R = i
            L,R = g(L,R,string)
            z.append(R-L+1)
        else:
            k = i-L
            if (z[k] < R-i): z.append(z[k])
            else:
                L,R = g(L,R,string)
                z.append(R-L+1)

Could it be any further refactored?

NOTE: Z-Algorithm is defined as z(i) = number of characters in the prefix of s(i) that matches the prefix of S.

For example, a string S = 'abaab'. The substrings of S are

s(0) = 'abaab' = S
s(1) = 'baab'
s(2) = 'aab'
s(3) = 'ab'
s(4) = 'b'

The z values are respectively

z(0) = 5 = length(S)
z(1) = 0
z(2) = 1
z(3) = 2
z(4) = 0

For more details, see this video.

Bugs

In your original compute_z(), you fail to return z.
In your refactored solution, g() has an undefined variable i.
The correct z-values for 'abaab' are [5, 0, 1, 2, 0], not [6, 0, 1, 2, 0].

Questions containing non-working code would normally be closed on Code Review, but since @jonrsharpe has already posted an answer, I'll let it slide.  Since the refactored solution is unreviewable in its current state, I'll examine only the original compute_z().

Reverse engineering

I was, admittedly, unfamiliar with the Z Algorithm.  I stared at compute_z(), befuddled by the cryptically named variables i, k, L, R, and by the purpose of the each if- and else-branch.  Frankly, my experience was closer to reverse-engineering than reading the code.

I came to the realization that I could strip the code down to the following and still get the same result.

def naive_z(string):
    n = len(string)
    z = [n]

    for i in xrange(1, n):
        L = R = i
        while (R < n and string[R - L] == string[R]): R += 1
        z.append(R - L)
    return z

I therefore surmised that the rest of the code served as some kind of optimization, in particular, if (z[k] < R-i): z.append(z[k]).

I also wondered about the R -= 1 statements.  I figured out that there was a loop invariant: at the bottom of the for-loop, string[L : R + 1] was the longest common prefix.

At that point, I decided to give up and start educating myself about the Z Algorithm. ☺

In the end, I decided that the algorithm is complex enough that it wouldn't be reasonable to assume that the underlying principles would be self-evident just by reading the code.  So, I learned something new today.

Observations

Your implementation appears to be a direct translation of the C code at http://codeforces.com/blog/entry/3107.

Superficial criticisms first: you have a few PEP 8 violations.  Notably, the first level of indentation is only 3 spaces, which in Python is a big deal.  The variables L and R violate capitalization conventions, though arguably L is more readable than l.  You also have a few C-isms, such as parentheses around the if-conditions.

Rather than appending one element at a time to z, you should pre-allocate an n-element list at the beginning.  Otherwise, the array might need to be reallocated as it grows.

You miss out on some optimizations due to a botched inequality.  if (z[k] < R-i) should be if z[i] <= R - i.

The R -= 1 statements are awkward, and should be eliminated.  In languages where strings are indexed starting from 0, it would be more idiomatic to use inclusive-exclusive bounds.  In other words, R should point just past the end of the match, such that string[L:R] is the longest matching prefix.

The concern that prompted you to write this question is probably the repetition of the while loops.  It is indeed possible to restructure the code such that the loop only appears once.

def compute_z(string):
    n = len(string)
    z = [0] * n

    zbox_l, zbox_r, z[0] = 0, 0, n
    for i in xrange(1, n):
        if i < zbox_r:              # i is within a zbox
            k = i - zbox_l
            if z[k] < zbox_r - i:
                z[i] = z[k]         # Full optimization
                continue
            zbox_l = i              # Partial optimization
        else:
            zbox_l = zbox_r = i     # Match from scratch
        while zbox_r < n and string[zbox_r - zbox_l] == string[zbox_r]:
            zbox_r += 1
        z[i] = zbox_r - zbox_l
    return z


Generic implementation of SiftUp/Down operations on binary heap Binary heap construction and maintenance boil down to the two basic operations: sift up and sift down. Following are generic template implementations of the two operations. Any comments are welcome. I'm preparing for an interview, and I hope my code will appeal to the interviewer :-)

/// Compile with `-std=c++14`.
#include <cassert>
#include <algorithm>
#include <array>
#include <functional>
#include <iostream>
#include <iterator>
#include <utility>

template <class RandIt, class Less = std::less<void>>
void SiftDown(RandIt first, RandIt last, Less&& less = Less{}) {
  auto n = static_cast<unsigned>(last - first);
  if (n < 2) return;
  auto child = 1u;
  if (2 < n && less(first[1], first[2])) ++child;
  if (less(first[0], first[child])) {
    typename std::iterator_traits<RandIt>::value_type val =
      std::move(first[0]);
    first[0] = std::move(first[child]);
    while (true) {
      first += child;
      n = static_cast<unsigned>(last - first);
      if (n < 2) break;
      child = 1u;
      if (2 < n && less(first[1], first[2])) ++child;
      if (! less(val, first[child])) break;
      first[0] = std::move(first[child]);
    }
    first[0] = std::move(val);
  }
}

template <class RandIt, class Less = std::less<void>>
void SiftUp(RandIt first, RandIt last, Less&& less = Less{}) {
  auto n = static_cast<unsigned>(last - first);
  if (n < 2) return;
  auto parent = n / 2 - 1;
  if (less(first[parent], last[-1])) {
    typename std::iterator_traits<RandIt>::value_type val =
      std::move(last[-1]);
    last[-1] = std::move(first[parent]);
    while (true) {
      last = first + parent + 1;
      n = static_cast<unsigned>(last - first);
      if (n < 2) break;
      parent = n / 2 - 1;
      if (! less(first[parent], val)) break;
      last[-1] = std::move(first[parent]);
    }
    last[-1] = std::move(val);
  }
}

int main() {
  int arr[5] = {2, 1, 5, 4, 3};
  auto it = arr;
  while (true) {
    SiftUp(arr, it);
    if (it == arr + 5) break;
    ++it;
  }
  for (auto i = 0; i < 5; ++i) {
    std::cout << *arr << ' ';
    std::iter_swap(arr, --it);
    SiftDown(arr, it);
  }
}

Don't specify the array size in int arr[5] = {2, 1, 5, 4, 3};. int arr[] = {2, 1, 5, 4, 3}; will cause less maintenance issues.
Don't use magic numbers. All other occurrences of 5 should be replaced with either sizeof arr / sizeof *arr or much better turn arr into an std::array and use arr.size().
I don't like the combination of while (true) with a conditional break. Not sure how to get around it though.
I don't understand what the static_cast<unsigned> are for. Casts are not very appealing.
You use auto for basically everything including unsigned ints, but then you explicitly name the type typename std::iterator_traits<RandIt>::value_type when auto would suffice.
I would appreciate some comments. I don't actually know what SiftUp and SiftDown do and why they are not hidden in some heap implementation. I suppose returning when n < 2 is because you are heapifying the array and an array of less than 2 elements it automatically a heap, but I'm not sure.
The code in main is essentially a demo how to use the code. That is good. You could flesh that out a bit though. I made a move only type to check if you screwed up move semantics somewhere. You probably did too. Leave the check in. Also add an assert to make sure the resulting array is 5 4 3 2 1. (that is the correct result, right?). Therefore, when people mess with optimize the code they will be notified when they screw up. Add a case with an empty array, 1 element array and an array with a bunch of equal elements to show you thought about edge cases.
SiftUp and SiftDown could probably use some asserts to check if invariants hold. This also documents that there are invariants and what they are and you are told when you break them.
I would add some line breaks and even parenthesis to if statements. That is a style/habit thing through. If you can get your hands on code written by the interviewer's company try to mimic their style.
last in SiftDown should be a const & to avoid an unnecessary copy. Same goes or first in SiftDown, but then you need to pass std::begin(arr) instead of just arr.

After having criticized a lot let me say this: I would probably hire you. You show a good understanding of C++ and should be easy to shape into a good software developer. Good luck!


C# approach to prevent null references in strings My insight here is that it's usually not useful to distinguish between null and empty strings. Sometimes it is, but usually not. Usually I like to assign my strings as empty if they would have been null.

var lastName = Request.Params["lastName"] ?? ""

Thus I had the idea of a struct which would wrap a real string, internally coalesce it to "", prevent all manner of null references, provide "less dangerous" versions of common string operations, yet still be compatible with strings.

This is an incomplete listing--I just wanted to get feedback on the idea. Please let me know why this is awesome, mediocre, or just plain terrible!

//with normal string
public void WriteName(string name)
{
    if(name != null && name.Length > 30)
    { name = name.Substring(0,30); }

    Console.WriteLine(name);
}

//with safestring
public void WriteName(SafeString name)
{
    Console.WriteLine(name.ReduceTo(30));
}

Below is the SafeString struct:

/// <summary>
/// Struct wrapper for a string, to prevent null references when you
///  don't care to distinguish between `null` and empty strings.
/// Has implicit conversions to & from string
/// </summary>
public struct SafeString : IEquatable<SafeString>, IEquatable<string>
{
    /*
      The wrapped string value.
      This will be properly initialized only if the constructor is called.
      It's possible to create a struct with `default(SafeString)`
        or by calling the parameterless ctor, `new SafeString()`
      C#6 will allow us to define a parameterless ctor,
        but there is still the issue of `default(SafeString)` which does not call any ctor.
      Thus when we wish to use the value of `_theString`,
        we need to either to check for null or funnel access through a helper
    */
    private string _theString;

    public SafeString(string s)
    {
        _theString = s ?? "";
    }

    public override string ToString()
    {
        //perform initialization in case this was created without using the ctor
        return _theString ?? (_theString = "");
    }

    public int Length { get { return ToString().Length; } }

    //safe substring method -- so you don't get an error when asking for a substring
    //   longer than the original
    public SafeString SubString(int start, int length)
    {
        if (start < 0)
        {
            start = 0;
        }
        if (length <= 0)
        {
            return "";
        }

        var over = Math.Max((start + length) - Length, 0);
        length = length - over;

        return ToString().Substring(start, length);
    }

    //shorthand for SubString(0,length), for when you wish to cut a string down to size
    public SafeString ReduceTo(int length)
    {
        if (length > Length)
        {
            return this;
        }
        return SubString(0, length);
    }

    public bool IsEmpty { get { return string.IsNullOrEmpty(_theString); } }
    public bool IsWhitespace { get { return string.IsNullOrWhiteSpace(_theString); } }

    public static implicit operator string(SafeString ss)
    {
        return ss.ToString();
    }

    public static implicit operator SafeString(string s)
    {
        return new SafeString(s);
    }

    public override int GetHashCode()
    {
        return ToString().GetHashCode();
    }

    public override bool Equals(object obj)
    {
        if (obj is SafeString)
        {
            return Equals((SafeString) obj);
        }
        if (obj is string)
        {
            return Equals((string) obj);
        }

        return false;
    }

    public bool Equals(SafeString other)
    {
        return ToString() == other.ToString();
    }

    public bool Equals(string other)
    {
        if (other == null)
        {
            return ToString() == ""; //we are never 'null'
        }
        return other == ToString();
    }
}

System.String is a class, a reference type. Wrapping it in a value type makes for a pretty confusing start: it's like the opposite of boxing, where a value type is wrapped with a reference type. It's ...awkward.

Value types should be immutable; _theString doesn't immediately strike me as a readonly field:

private string _theString;

And _theString is a very, very bad name - I would have simply gone with _value, like this:

private readonly string _value;

Calling ToString shouldn't have side-effects - here calling ToString is mutating the instance, changing the reference stored in _theString:

public override string ToString()
{
    //perform initialization in case this was created without using the ctor
    return _theString ?? (_theString = "");
}

There is little to no gain in doing this, and it makes your value type violate a fundamental rule: value types should be immutable, no ifs, no buts. I'd change it to this:

public override string ToString()
{
    return _value ?? string.Empty;
}

The only way client code can retrieve the encapsulated value, is by calling ToString - this is practical and useful, but not exactly instinctive. I would expose a getter:

public string Value { get { return ToString(); } }

The Equals(string) implementation relies on implementation details of another method:

return ToString() == "" //we are never 'null'

I don't see a need for an explanatory comment when using "normal" string methods:

return ToString().IsNullOrEmpty();

You have hard-coded "" in a bunch of places - I find string.Empty conveys "" in a more unambiguous way, and I would replace all occurrences of "" with string.Empty.

I think your struct is violating the Single Responsibility Principle, with methods like ReduceTo - I think these members belong as extension methods, extending the String type itself.

Also when writing code at that level, you should make sure every public member has proper XML comments, so that client code consuming it has a bit of documentation in IntelliSense.


Printing a table of letter frequencies using Python What is the recommended (standard, cleaner, more elegant, etc.) method in Python to get a string with all the alphabet letters?

Here's three versions of code to generate a dictionary with keys as letters and 0 as all values. The aim is to parse a text and count how often a letter appears.

from string import ascii_lowercase
frequency = {letter: 0 for letter in ascii_lowercase}

A downside of this method is that the constant shows up in the pydoc output for the module in which it is imported.

frequency = {chr(letter): 0 for letter in range(ord('a'), ord('z')+1)}

frequency = {letter: 0 for letter in 'abcdefghijklmnopqrstuvwxyz'}

This is the snippet of code where it is used:

frequency = {chr(letter): 0 for letter in range(ord('a'), ord('z')+1)}

# The 'text' variable is taken from somewhere else, we define it here for example's sake
text = 'StackExchange Code Review'
for c in text.lower():
    if c in frequency.keys():
        frequency[c] += 1
        total += 1

output = OrderedDict(sorted(frequency.items(), key = lambda x: x[0]))
for l in output.keys():
    print(l + ':' + '{:>8}'.format(output[l]) + '{:9.2f}'.format(100 * output[l] / (total if total > 0 else 1)) + ' %')

The printing portion should be simplified:

The use of OrderedDict is superfluous.  A list of pairs would suffice.
I'd prefer to stick to the same iteration variable, when it is used to refer to the same thing.  Choose either c or l.
You are not using str.format() effectively.
(total if total > 0 else 1) could just be (total or 1).

for c, n in sorted(frequency.items()):
    print('{}:{:>8}{:9.2f}%'.format(c, n, 100 * n / (total or 1)))

As for the counting process itself, you could write c in frequency instead of c in frequency.keys().  But, really, the task is so naturally suited to a collections.Counter that it would be silly not to use it.

from collections import Counter as _Counter
from string import ascii_lowercase as _ascii_lowercase

text = 'StackExchange Code Review'
frequency = _Counter({c: 0 for c in _ascii_lowercase})
frequency.update(c for c in text.lower() if c in frequency)

total = sum(frequency.values())
print('\n'.join(
    '{}:{:>8}{:9.2f}%'.format(c, n, 100 * n / (total or 1))
    for c, n in sorted(frequency.items())
))

I think that the call to sorted() should be optional as of Python 3.6.


Allocating memory and releasing the same I was going through a code snippet...

try
{
    ...
    // Memory is allocated here...
    NmsMessage* pNmsMessage = new NmsMessage(NMS_MODEM_DIAL_NUMBER);
    ...
    {
        // pNmsMessage is passed to another thread.
        pThis->m_pDeviceThread->PostThreadMessage(WM_NMS_MESSAGE, 0,pNmsMessage);
    }
    return NOERROR;
}
catch(...)
{
    ...
}

In the PostThreadMessage, memory pointed by pNmsMessage is getting released.

My question:

Is this the right way to handle memory in case of threaded environment?
If it is not threaded environment, is it better to release the memory before returning from the function and also in the catch block(I don't want to use smart pointers)?

I could see passing the pointer to another thread, and the other thread deleting the object.  (Gives me the willies, but it at least makes sense.)  But if PostThreadMessage itself is doing the deletion, that feels wrong.  I'm used to having a delete for every new, and PostThreadMessage is currently breaking that all to hell.  Now i have to go rooting around just to figure out who owns an object, so i don't accidentally delete it twice and open up a wormhole or something.

If the pointee doesn't need to outlive the call to PostThreadMessage, then you should be able to put it in a local variable and pass a pointer to the local.  Of course, that also means PostThreadMessage needs to keep its paws off the delete button.  :P

try {
    ...
    NmsMessage nmsMessage(NMS_MODEM_DIAL_NUMBER);
    ...
    {
        pThis->m_pDeviceThread->PostThreadMessage(WM_NMS_MESSAGE, 0, &nmsMessage);
    }
    return NOERROR;
}
catch(...) {
    ...
}

nmsMessage, having "automatic storage duration", will be deleted when control leaves the block.  Ownership and intent are clearer, you don't need to fart around with new and delete, and C++ cleans up for you automatically whether you return normally or throw an exception.

If PostThreadMessage wants the message info to outlive this code, seems to me that it should be making its own copy.  But it kinda sounds like it's already doing that.

(Note, most of this answer assumes that you can change PostThreadMessage.  If you can't -- for example, if it's part of some API you're stuck using -- then the only guarantees you have are the ones the API makes.  If it doesn't promise that pNmsMessage is either deleted or not in each case that may occur, then you have a pretty serious problem; any dynamically allocated object must be deleted, but deleting it twice can cause major problems.)


Construct A String Using StringBuilder from a List using elements index I am a java beginner and want to build a string from elements in a list using its elements index, the code runs fine yet i want to learn if there are other ways to improve upon it.Below is my sample code and I'm welcomed to suggestions.

String buildObject(List<Integer> obj){
    StringBuilder name = new StringBuilder();
   for(int i = 0; i < obj.size(); i ++){
       int m = obj.size() - 1;//will be nine
       if(i == 0 && i < 2){
           name.append(obj.get(0))
                   .append(" & ")
                   .append(obj.get(1));
       }else if (i >= 2 && i != m) {
           name.append(", ");//between second and before last value
            name.append(obj.get(i));
       }else if(i == m){//last value
           name.append(" and ")
                   .append(obj.get(m));
       }

   }
  return name.toString();
   }

As far as your code is concerned, it would be more efficient to apply the strings from the first 2 and the last elements outside the loop.  This will eliminate 3 elements from the loop:

String buildObject(List<Integer> obj){
   StringBuilder name = new StringBuilder();
   name.append(obj.get(0))
       .append(" & ")
       .append(obj.get(1));
   int limit = obj.size() - 1;
   for(int i = 2; i < limit; i++){
       name.append(", ");
           .append(obj.get(i));
   }
   name.append(" and ")
       .append(obj.get(limit));
   return name.toString();
}


Displaying days of the week for all of 2015 This program is supposed to determine the day of the week for 2015. Could someone tell me how I could shorten my code?

#include <stdio.h>
#include <conio.h>

void findDay(int , int);

int main(void)
{
int jan, feb, mar, apr, may, jun, jul, aug, sep, oct, nov, dec;

for(jan=1; jan<=31; ++jan)
{
    printf(" 01/%d/2015 is", jan);
    findDay(jan, 0);
}
for(feb=1; feb<=28; ++feb)
{
    printf("02/%d/2015 is", feb);
    findDay(feb, 3);
}
for(mar=1; mar<=31; ++mar)
{
    printf("03/%d/2015 is", mar);
    findDay(mar, 3);
}
for(apr=1; apr<=30; ++apr)
{
    printf("04/%d/2015 is", apr);
    findDay(apr, 6);
}
for(may=1; may<=31; ++may)
{
    printf("05/%d/2015 is", may);
    findDay(may, 1);
}
for(jun=1; jun<=30; ++jun)
{
    printf("06/%d/2015 is", jun);
    findDay(jun, 4);
}
for(jul=1; jul<=31; ++jul)
{
    printf("07/%d/2015 is", jul);
    findDay(jul, 6);
}
for(aug=1; aug<=31; ++aug)
{
    printf("08/%d/2015 is", aug);
    findDay(aug, 2);
}
for(sep=1; sep<=30; ++sep)
{
    printf("09/%d/2015 is", sep);
    findDay(sep, 5);
}
for(oct=1; oct<=31; ++oct)
{
    printf("10/%d/2015 is", oct);
    findDay(oct, 0);
}
for(nov=1; nov<=30; ++nov)
{
    printf("11/%d/2015 is", nov);
    findDay(nov, 3);
}
for(dec=1; dec<=31; ++dec)
{
    printf("12/%d/2015 is", dec);
    findDay(dec, 5);
}

getch();
return 0;
}
void findDay(int dow, int num)
{
int x;
x=(6+15+3+dow+num)%7;//a formula to calculate the day of the week

switch (x)
{
case 0:
printf(" Sunday\n");
break;
case 1:
printf(" Monday\n");
break;
case 2:
printf(" Tuesday\n");
break;
case 3:
printf(" Wednesday\n");
break;
case 4:
printf(" Thursday\n");
break;
case 5:
printf(" Friday\n");
break;
case 6:
printf(" Saturday\n");
break;
default:
printf("Invalid Input");

}

}

Critique

The way to generalize your for-loops is to determine what varies — namely, the number of days in each month and the day of the week that each month starts on.  Then, you want to extract the magic numbers into arrays so that your code is data-directed.

The second argument to findDay() does not make obvious sense.  Considering that January 2015 starts on a Thursday, why would you call findDay(jan, 0)?  I would expect something like findDay(jan, 4), give or take 1 depending on your numbering convention.  Better yet, make your numbering convention clearer by defining an enum.

Similarly, findDay() would be better implemented using a lookup table instead of a switch.

findDay() is misnamed and poorly designed.  It doesn't just return the result: it also prints it (with a leading space and a trailing newline!).  Printing the result limits your ability to ever reuse that function for anything else in the future.  (Normally, returning a string from a C function could be a tricky proposition.  In this case, you'll be returning constant strings, which is fine.)

Suggested solution

#include <stdio.h>

typedef enum {
    SUN, MON, TUE, WED, THU, FRI, SAT
} Weekday;

static const char *DAY_NAMES[] = {
    "Sunday",
    "Monday",
    "Tuesday",
    "Wednesday",
    "Thursday",
    "Friday",
    "Saturday",
};

static const int MONTH_LENGTHS_NON_LEAP_YEAR[] = {
    31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31
};

const char *dayOfWeekIn2015(int month, int dayOfMonth) {
    static const Weekday DAY0[] = {
        WED,    // 2014-12-31
        SAT,    // 2015-01-31
        SAT,    // 2015-02-28
        TUE,    // 2015-03-31
        THU,    // 2015-04-30
        SUN,    // 2015-05-31
        TUE,    // 2015-06-30
        FRI,    // 2015-07-31
        MON,    // 2015-08-31
        WED,    // 2015-09-30
        SAT,    // 2015-10-31
        MON     // 2015-11-30
    };
    return DAY_NAMES[(DAY0[month - 1] + dayOfMonth) % 7];
}

int main(void) {
    for (int month = 1; month <= 12; month++) {
        for (int dayOfMonth = 1; dayOfMonth <= MONTH_LENGTHS_NON_LEAP_YEAR[month - 1]; dayOfMonth++) {
            printf("%02d/%02d/2015 is a %s\n",
                   month, dayOfMonth, dayOfWeekIn2015(month, dayOfMonth));
        }
    }
}


Implementation of Observable with multiple callback function signatures I have been playing around with ways of implementing a generic class for supporting the observer pattern. I have already asked to review a basic approach here. Now I have come up with an Observable class that supports parametrizing with an arbitrary number of function signatures, in order for the subject class to register and notify observers interested in different kinds of events that require different kind of information to be transmitted.

Here is the code:

observable.h:

#include <functional>
#include <vector>
#include <tuple>

template<typename ... Funcs>
class Observable
{
public:
    using ObserverList = std::tuple<std::vector<std::function<Funcs>>...>;

    template<unsigned int eventidx, typename Callable>
    void AttachObserver(Callable c)
    {
        std::get<eventidx>(observers).push_back(c);
    }

    template<unsigned int eventidx, typename ... T>
    void NotifyObserversOfEvent(T ... args)
    {
        for(auto f: std::get<eventidx>(observers))
        {
            f(std::forward<T>(args)...);
        }
    }

private:
    ObserverList observers;
};

Sample Test code

main.cpp:

#include <iostream>
#include <functional>
#include <vector>

#include "observable.h"

class A : public Observable<
                            void(void),
                            void(int,std::string),
                            void(int,int),
                            >
{
public:
    enum Event : unsigned
    {
        Event0,
        Event1,
        Event2
    };

private:
    int a;
    std::string s;
};

class B
{
public:
    void EventZeroHappened()
    {
        std::cout << "B has observed event zero\n";
    }

    void EventOneHappened(int a, std::string s)
    {
        std::cout << "B has observed event one with params: " << a << " and " << "\"" << s << "\"" <<'\n';
    }

    void EventTwoHappened(int a, int b)
    {
        std::cout << "B has observed event two with params: " << a << " and " << b <<'\n';
    }
};

int main()
{
    A a;
    B b;

    a.AttachObserver<A::Event::Event0>([&b](){b.EventZeroHappened();});
    a.AttachObserver<A::Event::Event1>([&b](int i, std::string s){b.EventOneHappened(i,s);});
    a.AttachObserver<A::Event::Event2>([&b](int i, int j){b.EventTwoHappened(i,j);});

    a.NotifyObserversOfEvent<A::Event::Event0>();
    a.NotifyObserversOfEvent<A::Event::Event1>(37,"Hello There");
    a.NotifyObserversOfEvent<A::Event::Event2>(182,150);
}

Basically the idea I wanted to implement is that the user of the observable class, simply writes in the template parameter list, the function signatures of the callback it supports. Instead of forcing the observer to implement an abstract interface and derive from it, which pollutes the observer code and creates an unnecessary dependency. The only draw back is that in order to select which event the observer wants to subscribe to (and which event the subject wants to transmit) it has to be done via integral constant, which is made a bit nicer in the example with an enum. I'm still searching for a better way to do this part.

The reason behind that is that, even in the old way of implementing observer, where you hard-code in a *.h file a pure abstract class and tell other classes that want to observe: "please overload this virtual functions if you want to observe me". And the meaning of each of the events you are going to notify is encoded in the names of the functions. I thought this example was a more general and flexible way of transmitting the same information. Plus being a bit easier to modify to add new events.

As always, I would welcome any comments, corrections criticisms and suggestions.

There is a way to get rid of the integral constant dispatch without forcing the user to inherit from an observable class, but it's a bit complex.

The idea is to encapsulate runtime polymorphism inside the Observable class. The pattern looks like:

class PolymorphicBehavior {
    class Concept {
        // here are the virtual functions
    };
    template <typename ConcreteType>
    class Object {
        // here are concrete implementations of the functions
        // The class encapsulates type information
        private:
        ConcreteType value;
    };
    public:
    template <typename ConcreteType>
    PolymorphicBehavior(const ConcreteType& ct) : ptr(new Object<ConcreteType>(ct)) {}
    private:
    Concept* ptr;
};

In the case of Observable, this pattern allows you to erase the function signature (concrete type information is stored in the inner object). There's still another type erasure to perform though, because we'll need to pass arguments to the encapsulated functions. I used std::any over a tuple to create a unified interface.

Here's the full code:

#include <iostream>
#include <vector>
#include <memory>
#include <tuple>
#include <any>
#include <functional>

//deducing the tuple type from the function type
// e.g: std::function<void(float, double)> => std::tuple<float, double>
template <typename T>
struct tuple_desc;
template <typename R, typename... Args>
struct tuple_desc<std::function<R(Args...)>> {
    using type = std::tuple<Args...>;
};

// deducing the function signature from the callable
template <typename Callable>
using function_class = decltype(std::function(std::declval<Callable>()));

class Observable {

    class Observable_model { // type neutral hook
        public:
        virtual void notify(unsigned event_number, std::any& argument_wrapper) = 0;
        virtual ~Observable_model() = default;
    };

    template <typename Callback>
    class Observable_object : public Observable_model { // stores concrete type information
        public:
        Observable_object(unsigned event_class, const Callback& cbk) : event_class(event_class), callback(cbk) {}

        void notify(unsigned event_number, std::any& argument_wrapper) override {
            using Arg_tuple = typename tuple_desc<decltype(this->callback)>::type; // so tuple<int, float> for [](int a, float b) { /*...*/ }
            if (event_number == event_class) { // dynamic dispatch
                std::apply(callback, std::any_cast<Arg_tuple> (argument_wrapper));
            }
        }
        private:
        unsigned event_class;
        function_class<Callback> callback;
    };

    public:
    template <typename Callback>
    void attach(unsigned event_class, Callback&& callback) {
        auto new_obj = std::make_unique<Observable_object<Callback>>(event_class, callback);
        observations.push_back(std::move(new_obj));
    }
    template <typename... Args>
    void notify(unsigned event_number, Args&&... args) {
        std::tuple<Args...> arg_tuple(std::forward<Args>(args)...);
        std::any argument_wrapper(arg_tuple); // argument type erasure
        for (auto& obs : observations) obs->notify(event_number, argument_wrapper);
    }

    private:
    std::vector<std::unique_ptr<Observable_model>> observations;

};

// testing the interface;

enum { WILDLIFE = 0, MACHINES = 1 };

struct Buzzer {
    void go(int h) { std::cout << "drrrr... drrrr... it's " << h << " a.m, time to wake up!\n"; }
};
struct Coffee_machine {
    void make_coffee(int h) { if (h == 7) std::cout << "time to make coffee!\n"; else std::cout << "no coffee yet baby!\n"; }
};

struct Rooster {
    void sing() { std::cout << "Cocorico!\n"; }
};
struct Vampire {
    void bury() { std::cout << "Bye! I'll be in my grave!\n"; }
};

struct Sun : public Observable {
    void rise() {
        std::cout << "Waouh, the sun rises!\n";
        notify(WILDLIFE);
        notify(MACHINES, 6);
    }
};

int main() {
    Sun sun;
    Buzzer buzzer;
    Rooster rooster;
    Vampire vampire;
    Coffee_machine coffee_machine;
    sun.attach(WILDLIFE, [&rooster] {rooster.sing();});
    sun.attach(WILDLIFE, [&vampire] {vampire.bury();});
    sun.attach(MACHINES, [&buzzer](int h) {buzzer.go(h);});
    sun.attach(MACHINES, [&coffee_machine](int h) {coffee_machine.make_coffee(h);});
    sun.rise();
}


Search/match algorithm between 2 vectors I have 2 vectors of Facebook-like pages which are represented by an ID from 2 different users. I want to verify the number of liked pages that are similar between these 2 users.

This is my algorithm that I am using right now, which is basically a search into all rows and columns of a matrix:

for(int i = 0; i < 100; i++) {
 val = [user_like objectAtIndex:i];
 id like_user = [[val allValues] objectAtIndex:0];
 NSLog(@"like user %d is %@",i, like_user);

 for (int j = 0; j < 100; j++) {

 val2 = [friend_like objectAtIndex:j];
 id like_friend = [[val2 allValues] objectAtIndex:2];
 NSLog(@"like friend %d is %@",j, like_friend);

 if (like_user == like_friend) {
 //do something
 }

 }

 }

How can I optimize this search/match algorithm?

Some problems with this code I want to address before I even get into optimization of speed.

First of all, there's absolutely no indentation, which makes this very hard to read.

Second of all, your variable names are both lacking, and not in keeping with Objective-C standard conventions.  Variable names like val and val2 aren't really descriptive enough.  A variable name like like_user may be descriptive enough (I don't know, it's borderline), but Objective-C standards suggest that the variable should instead be named likeUser.  We prefer camel casing to underscoring.

Next, you're using a for loop where you should be using a forin (I'll get to that later), but we're using the loop iterator as an index into an array... which is fine-ish... but you're concerning yourself with the array's length in the for's condition.  You're risking an index out of bounds exception.

But we really don't want to use a for loop anyway.  Any time we need to do something with each object in an Objective-C collection, we should use a forin loop anytime we can.  forin loops run faster than regular for loops--they're processed in batches.

But as 200_success points out, in this specific instance, we don't need to iterate through the indexes.

You stated you just want to count the number of common liked pages.

So, just as 200_success suggested:

NSSet *userLikeSet = [NSSet setWithArray:userLike];
NSMutableSet *commonLikeSet = [NSMutableSet setWithArray:friendLike];
NSSet *commonLikes = [commonLikeSet intersectSet:userLikeSet];

NSInteger commonLikeCount = [commonLikes count];


Better way to display list of categories I need to display a list of category links, like so:

Category 1, Category 2, Category 3

I've got it working already but the code seems pretty repetitive and a bit of a mess, I was wondering if there was a better way of doing it.

This is what I've got so far:

for (var i = 0; i < data['categories'].length; i++) {
    var comma = document.createTextNode(', ');
    var link = document.createElement('a');
        link.setAttribute('href', '#journal__category--' + data['categories'][i]['url_title'] );
        link.setAttribute('class', 'js--page__link');
        link.innerHTML = data['categories'][i]['category_name'];

        document.getElementById('js--journal__categories').appendChild(link);
        if( (i + 1) != data['categories'].length ) {
            document.getElementById('js--journal__categories').appendChild(comma);
        }
}

Here's how I might try to clean things up:

refer to object properties without bracket notation where possible
e.g. data.categories.length
create a loop variable for the current category, which allows you to reference it nicely without data.categories[i]

for (var i = 0; i < data.categories.length; i++) {
    var category = data.categories[i];
}

don't look up the container on every iteration, move that outside the loop:

var container = document.getElementById('js--journal__categories');
for (...loop...) {
    ...create your node...
    container.appendChild(link);
}

same for creating the comma node, create it outside the loop.
you don't need to wrap i + 1 in parenthesis, and this is just my preference, but you should be consistent with spaces. You could also use < instead of != which is more consistent with the for loop. (These are fairly minor nits)

if (i + 1 < data.categories.length)

Why are you using double dashes and underscores in your element IDs?
Comments are helpful to explain what your for loop and if statement is doing, since it's probably easier/quicker to understand the comment than figure out the for loop.
Maybe I'm getting overkill at this point, but a way to write really descriptive, easy to read code is to break it into functions whose name makes sense:

function createCategoryElement(name, url_title) {
    ...create your anchor element here...
    ...set its attributes and content, etc...
    return element;
}

I ended up with something like this:

var data = {
  categories: [
    {name: 'one', url_title: 'oneUrl'},
    {name: 'two', url_title: 'twoUrl'}
  ],
};

var container = document.getElementById('container');
var comma = document.createTextNode(', ');

function createCategoryElement(name, url) {
  var urlBase = '#journal-category-';
  var cssClass = 'js-page-link';

  var el = document.createElement('a');
  el.setAttribute('href', urlBase + url);
  el.setAttribute('class', cssClass);
  el.innerHTML = name;
  return el;
}

// Create HTML elements for each category and append them to the DOM.

for (var i = 0; i < data.categories.length; i++) {
  var category = data.categories[i];
  var categoryElement = createCategoryElement(category.name, category.url_title);
  container.appendChild(categoryElement);

  // Join categories with a comma
  if (i + 1 < data.categories.length) {
    container.appendChild(comma);
  }
}

        for (...loop over categories...) {
            ...
            var categoryElement = createCategoryElement(category.name, category.url_title);
            container.appendChild(categoryElement);

            if (i + 1 < data.categories.length) {
                containenr.appendChild(commaElement);
            }
        }


Regex - Input Checker Class - Lambda with two lists I have an input checker class with some RegExes, and I have converted some of the loops to lambdas. Am I doing it right? They seem to work fine, but I'm new to Lambdas. Does this offer a performance benefit over the commented implementation? Should I try implementing the first one as a lambda, and if so, how? Thanks in advance!
public static class DataChecker
{
    public static bool IsGameInputDataValid(string[] dataFile)
    {
        if (dataFile.Length < 5)
        {
            return false;
        }

        var regexList = new List<Regex>();
        regexList.Add(new Regex("^\\d\\s\\d$"));
        regexList.Add(new Regex("^(\\d,\\d )*(\\d,\\d)$"));
        regexList.Add(new Regex("^\\d\\s\\d$"));
        regexList.Add(new Regex("^\\d \\d [NESW]{1}$"));
        regexList.Add(new Regex("^[RLM]{1}( [RLM]{1})*$"));

        //cant figure this one out as lambda... but should it even be converted for performance reasons?
        for (int i = 0; i < 4; i++)
        {
            if (!regexList[i].IsMatch(dataFile[i]))
            {
                return false;
            }
        }

        var listOfMoves = dataFile.Select(x => x).Skip(4).ToList();
        //instead of:
        // for (int i = 4; i<countOfLinesInInputFile; i++)
        // {
        //     listOfMoves.Add(lines[i]);
        // }

        var areaAnyMoveSequencesInvalid =
            listOfMoves.Select(x => regexList[4].IsMatch(x)).ToList()
            .Any(y => y == false);

        if (areaAnyMoveSequencesInvalid)
        {
            return false;
        }

        // instead of:
        // foreach (var line in listOfMoves)
        // {
        //     if (!regexList[4].IsMatch(line))
        //     {
        //         return false;
        //     }
        // }

        return true;
    }
}

edit: Originally I wanted the question to only focus on the lambdas, but I got a lot of responses for the RegExes as well. Therefore I'll add the missing parts to make sense of that. It's a turtle-moving game in 2-dimensional x,y coordinates map.
Sample data file:
5 4
1,1 1,3 3,3
4 2
0 1 N
R M L M M
R M M M
M R M M M M
M R M M
M M M

Rules:
The first line should define the board size
• The second line should contain a list of mines (i.e. list of co-ordinates separated by a space)
• The third line of the file should contain the exit point.
• The fourth line of the file should contain the starting position of the turtle.
• The fifth line to the end of the file should contain a series of moves.
Where:
R: Rotate right
L: Rotate left
M: Move
N,E,S,W: cardinal directions
I've updated the original with d+ instead of d, to allow for multiple digit coordinates:
        regexList.Add(new Regex("^\\d+\\s\\d+$"));
        regexList.Add(new Regex("^(\\d+,\\d+ )*(\\d+,\\d+)$|^$"));
        regexList.Add(new Regex("^\\d+\\s\\d+$"));
        regexList.Add(new Regex("^\\d+ \\d+ [NESW]{1}$"));
        regexList.Add(new Regex("^[RLM]{1}( [RLM]{1})*$"));

I would tackle this problem by not adding the last regex to the list but to have this as a separate regex. Using a little "trick" by using Enumerable.Range() you can replace the for with a linq expression like so
if (Enumerable.Range(0, 4).Any(i => !regexList[i].IsMatch(dataFile[i]))) { return false; }

After the loop you have this line
var listOfMoves = dataFile.Select(x => x).Skip(4).ToList();

in which you neither need the call to Select() nor to ToList().
In the next line
var areaAnyMoveSequencesInvalid =
            listOfMoves.Select(x => regexList[4].IsMatch(x)).ToList()
            .Any(y => y == false);

A simple call to Any() would be sufficient but you could just return the value of a call to All() instead like shown below.
If the method in question is called quite often you should consider to use compiled regex. They take a little bit more time to create them but executing them will become faster. If you take this way you should initialize the regexes in the static constructor.
Summing this up will lead to
public static class DataChecker
{
    static DataChecker()
    {
        regexList = new List<Regex>();
        regexList.Add(new Regex("^\\d\\s\\d$", RegexOptions.Compiled));
        regexList.Add(new Regex("^(\\d,\\d )*(\\d,\\d)$", RegexOptions.Compiled));
        regexList.Add(new Regex("^\\d\\s\\d$", RegexOptions.Compiled));
        regexList.Add(new Regex("^\\d \\d [NESW]{1}$", RegexOptions.Compiled));

        lastRegex = new Regex("^[RLM]{1}( [RLM]{1})*$", RegexOptions.Compiled);
    }
    private static readonly List<Regex> regexList;
    private static readonly Regex lastRegex;
    public static bool IsGameInputDataValid(string[] dataFile)
    {
        if (dataFile.Length < 5) { return false; }

        if (Enumerable.Range(0, 4).Any(i => !regexList[i].IsMatch(dataFile[i]))) { return false; }

        return dataFile.Skip(4).All(x => lastRegex.IsMatch(x));
    }
}

Without knowing what the input looks like I can't come up with a better name for the last regex. In addition this isn't tested but should work like a charme.


The 100 chair survivor challenge I came across the following challenge. Here is my implementation in Python 3.7

  A room has 100 chairs numbered from 1-100. On the first round of the game the FIRST individual is eliminated. On the second round, the THIRD individual is eliminated. On every successive round the number of people skipped increases by one (0, 1, 2, 3, 4...). So after the third person, the next person asked to leave is the sixth (skip 2 people - i.e: 4 and 5).

  This game continues until only 1 person remains.

  Which chair is left by the end?

I actually don't know what the true answer is, but my code outputs 31.

I think this is the fastest solution and at worst has an O(N^2) time complexity

n = 100
skip = 0
players = [x for x in range (1, n+1)]
pointer = 0
while len(players) > 1:
    pointer += skip
    while pointer >= len(players):
        pointer = pointer - len(players)
    players.pop(pointer)
    skip += 1
print(players[0])

The loop

while pointer >= len(players):
    pointer = pointer - len(players)

is a long way to say pointer %= len(players).
You'd be in a better position factoring the computations into a function,

def survivor(n):
    skip = 0
    players = [x for x in range (1, n+1)]
    pointer = 0
    while len(players) > 1:
        pointer += skip
        while pointer >= len(players):
            pointer = pointer - len(players)
        players.pop(pointer)
        skip += 1
    return players[0]

and adding an if __name__ == '__main__' clause. This way it is easy to generate first few results.

TL;DR

I did so, generated first 20 numbers,

    1 2 2 2 4 5 4 8 8 7 11 8 13 4 11 12 8 12 2

and search this sequence in OEIS.

Surprisingly there was a match. A closer inspection demonstrated that the problem is a Josephus problem with eliminating every n-th person. Try to prove, or at least convince yourself, that it is indeed the case.

Further reading revealed that there is a linear solution.


Tight loop, string manipulation and calculations I'm working on some precompilation operations for a world compiler. Currently to identify flags placed by the level designer I need recognize when a specific entity exists at specific coordinates from the flag's origin. I need to generate a list of offset coordinates from the origin(and include the origin) in string form. That is 7 coordinates. this operation is done for every flag in the world, so it is preferred to be efficient. It is not an operation that takes place during gameplay, so I don't have to worry too much.

My current implementation works correctly, and quickly, however I still would like to get a review on what I have written, and hear any advice to better this operation.

string origin = "0 0 0";
double[] originParts = origin.Split(' ').Select(s => double.Parse(s)).ToArray();
List<string> origins = new List<string> { origin };
for (int i = 0; i < 3; i++)
    for (int j = -48; j <= 48; j += 96)
        origins.Add(string.Format("{0} {1} {2}",
            originParts[0] + (i == 0 ? j : 0),
            originParts[1] + (i == 1 ? j : 0),
            originParts[2] + (i == 2 ? j : 0)));

This code is effectively calculating a set of offsets from the origin and adding them in turn to get the final co-ordinates.  However, you're calculating the offsets each time for every flag and the question implies that this is done many times. Better to calculate them up-front, then iterate through them, applying the co-ordinates of the flag to each:

// Done once at initialization...
int[,] offsets = new int[6, 3] { { -48,   0,   0 },
                                 {  48,   0,   0 },
                                 {   0, -48,   0 },
                                 {   0,  48,   0 },
                                 {   0,   0, -48 },
                                 {   0,   0,  48 } }; ;

// Done for each flag in the game world...
string origin = "0 0 0";
double[] originParts = origin.Split(' ').Select(s => double.Parse(s)).ToArray();
List<string> origins = new List<string> { origin };

for(int idx = 0; idx < offsets.GetLength(0); idx++)
{
    origins.Add(String.Format("{0} {1} {2}",
        originParts[0] + offsets[idx, 0],
        originParts[1] + offsets[idx, 1],
        originParts[2] + offsets[idx, 2]));
}

You could, of course, build the array of offsets programmatically instead of hardcoding them like I did.

This gives you slightly more lines of code, but it reduces the number of calculations done on each loop, and separates the calculation of the offsets from the calculation of the final co-ordinate, which may make future modifications simpler (e.g. if the designer wants more co-ordinates added to the list, or co-ordinates should be read from a configuration file, and so on).


Vue.js Stoplight app - Dynamically changing classes on elements I recently started delving into Vue.js, and decided to try my hand at some custom class/style bindings, so I made a small app that's supposed to cycle through each "light" of a stoplight (red, yellow, green, red, etc.).

My Vue instance has a data property count, which is initially set to 0, and I also have a setInterval callout that is to increments the count every one second.

Each "light" in the stoplight has a Bootstrap button class associated with it (btn-danger for red, btn-warning for yellow, and btn-success for green), and each class becomes "active" based on some modulus arithmetic against my Vue instance's count.

I'm wondering if perhaps I could be handling the change in these classes more efficiently, but am not sure what could be done.

Here is my current code:

var app = new Vue({
  el: '#app',
  data: {
    count: 0,
    styleObject: {
      display: 'block',
      width: '30px',
      margin: '0',
      borderRadius: '50px',
      border: '1px solid black'
    }
  }
});

setInterval(function() {
  app.count = app.count + 1;
}, 1000);
#light {
  background-color: yellow;
  display: inline-block;
  border: 2px solid black;
  margin: 10px;
  padding: 5px;
}
<link href="https://stackpath.bootstrapcdn.com/bootstrap/4.1.3/css/bootstrap.min.css" rel="stylesheet" />

    <input type='button' id='red' :class='["btn", (count % 3 === 0) && "btn-danger"]' :style='styleObject' />

    <input type='button' id='yellow' :class='["btn", (count % 3 === 1) && "btn-warning"]' :style='styleObject' />

    <input type='button' id='green' :class='["btn", (count % 3 === 2) && "btn-success"]' :style='styleObject' />

You could use the Object Syntax to specify the class bindings instead of the Array Syntax:

<input type='button' id='red' :class='{"btn": true, "btn-danger": (count % 3 === 0)}'  />

That "btn": true is okay but a little annoying. Luckily "it’s also possible to use the object syntax inside array syntax"1

<input type='button' id='red' :class='["btn", {"btn-danger": (count % 3 === 0)}]'  />

The documentation uses double quotes and I attempted to use those but it didn't appear to work - perhaps because btn-danger needs to be surrounded by quotes.

<input type='button' id='red' :class="[btn, {'btn-danger': (count % 3 === 0)}]" />

There doesn't appear to be anything dynamic about the styles in styleObject, so those can be moved out of the business logic and maintained with the other styles in the CSS section.

The interval function could be simplified using the increment operator:

setInterval(function() {
  app.count++;
}, 1000);

See the rewritten code that utilizes the advice above.

var app = new Vue({
  el: '#app',
  data: {
    count: 0
  }
});

setInterval(function() {
  app.count++;
}, 1000);
#light {
  background-color: yellow;
  display: inline-block;
  border: 2px solid black;
  margin: 10px;
  padding: 5px;
}

#light .btn {
  display: block;
  width: 30px;
  margin: 0;
  border-radius: 50px;
  border: 1px solid black;
}
<link href="https://stackpath.bootstrapcdn.com/bootstrap/4.1.3/css/bootstrap.min.css" rel="stylesheet" />

    <input type='button' id='red' :class='["btn", {"btn-danger": (count % 3 === 0)}]'  />

    <input type='button' id='yellow' :class='["btn", {"btn-warning": (count % 3 === 1)}]'  />

    <input type='button' id='green' :class='["btn", {"btn-success": (count % 3 === 2)}]'  />

1https://vuejs.org/v2/guide/class-and-style.html#Array-Syntax


Create user in users table and insert data into second table with one form in Laravel Relationships I'm trying  to create a user while adding data to a second table via one form:

users table:

Schema::create('users', function (Blueprint $table) {
        $table->id();
        $table->string('email')->unique();
        $table->timestamp('email_verified_at')->nullable();
        $table->string('password');
        $table->rememberToken();
        $table->timestamps();
    });

info table:

Schema::create('infos', function (Blueprint $table) {
        $table->id();
        $table->string('fname');
        $table->string('lname');
        $table->string('dateofbirth');
        $table->string('phonenumber')
        $table->timestamps();
    });

info_user table:

Schema::create('info_user', function (Blueprint $table) {
        $table->id();
        $table->unsignedBigInteger('user_id');
        $table->unsignedBigInteger('info_id');
        $table->foreign('user_id')->references('id')->on('users')->onUpdate('cascade')->onDelete('cascade');
        $table->foreign('info_id')->references('id')->on('infos')->onUpdate('cascade')->onDelete('cascade');
        $table->timestamps();
    });

User.php Model

public function infos() {
    return $this->belongsToMany(Info::class)->withTimestamps();;
}

Info.php Model

public function users() {
    return $this->belongsToMany(User::class)->withTimestamps();
}

And my attempt at InfoController

public function store(Info $info, User $user, Request $request)
{

        User::create([
            'email' => $request->email,
            'password' => Hash::make($request->password),
        ]);

        $data = new Info;
        $data->fname = $request->fname;
        $data->lname = $request->lname;
        $data->dateofbirth = $request->dateofbirth;
        $data->phonenumber = $request->phonenumber;

        $data->save();

    return redirect('/');
}

I'm trying to figure out how to register a user while adding info to the second table so they have a common info_user table.

I think I succeeded, but only if it is right and especially safe.

public function store(Info $info, User $user, Request $request)
{
        $user = User::create([
            'email' => $request->email,
            'password' => Hash::make($request->password),
        ]);

        $data = new Loan;

        $data->fname = $request->fname;
        $data->lname = $request->lname;
        $data->dateofbirth = $request->dateofbirth;
        $data->phonenumber = $request->phonenumber;

        $data->save();

        $user->infos()->attach($data);

    return redirect('/');
}

It's a correct way to do it like this.
Few suggestions:

Add validation before creating user. You don't want to fail on save for duplicate user email. For better UX if you can it's a good idea to do it even before form submit (with ajax request when email field is changed).

If your app has more user related things like user profile, user control panel I will suggest you to auto login user after form submit. Just before redirect() add Auth::login($user)


Pythagorean Triple Generator I'm just playing about with Go, and am trying to solve some project euler problems to get used to it.

I've created a generator function for Pythagorean triples, using a channel which returns a set of 3 integers for the sides, like so (HCF implements Euclid's algorithm for finding greatest common divisor or highest common factor in this part of the world):

package main
import "fmt"

func HCF(a, b int) int {
    if a % b == 0 {
        return b
    } else {
        return HCF(b, a%b)
    }
}

func PythagTriple() chan [3]int {

    ch := make(chan [3]int)

    go func() {
        for m := 2; ; m++ {
            for n := m%2 + 1; n < m; n += 2 {
                if HCF(m, n) == 1 {
                    var triple [3]int
                    triple[0] = m*m - n*n
                    triple[1] = 2 * m * n
                    triple[2] = m*m + n*n
                    ch <- triple
                }
            }
        }
    }()

    return ch
}

func main() {

    ch := PythagTriple()
    for i := 0; i < 10; i++ {
        fmt.Println(<- ch)
    }
}

Questions:

Do I need to do anything with the channel, or will it close and clean up when it goes out of scope?
Is there a better way of creating a quick array than how I've done it?
Is this idiomatic Go, or am I barking up the wrong tree entirely?  I've just started and am not sure if I'm doing it right!

Don't worry too much about efficiency of the algorithm; I'm not too concerned about speed at this point.

Not a Go expert but I think you should be able to define your array of values as you declare it :

triple := [...]int{m*m - n*n, 2 * m * n, m*m + n*n}

Source


Implementing HTTP data range representation class I need a class representing a parsed Range HTTP header value. I've done it like this:

namespace net {

namespace http {

    class DataRange {
    public:
        static const uint64_t UnspecifiedBytePos;
        static const DataRange Full;

        explicit DataRange(uint64_t firstBytePos, uint64_t lastBytePos);
        DataRange(const DataRange& other);

        uint64_t FirstBytePos() const;
        uint64_t LastBytePos() const;

        uint64_t Size() const;

        bool HasUnspecifiedBytePos() const;

        bool operator==(const DataRange& other) const;
        bool operator!=(const DataRange& other) const;

    private:
        uint64_t firstBytePos_;
        uint64_t lastBytePos_;
    };

    typedef std::list<DataRange> DataRanges;

    DataRange SpecifyDataRangeBytePos(const DataRange& dataRange, uint64_t dataSize);

} // namespace http

} // namespace net

I don't show the implementation here since it's quite simple. In this implementation, DataRange may have unspecified borders, representing Range headers like this one:

Range: -1234

To turn DataRange to one with specified borders, I've made a function SpecifyDataRangeBytePos:

DataRange SpecifyDataRangeBytePos(const DataRange& dataRange, uint64_t dataSize) {

    uint64_t specifiedFirstBytePos = dataRange.FirstBytePos();

    if ((dataRange.FirstBytePos() == DataRange::UnspecifiedBytePos) &&
            (dataRange.LastBytePos() != DataRange::UnspecifiedBytePos)) {

        uint64_t rangeUpperBorder = std::min(dataSize, dataRange.LastBytePos());
        specifiedFirstBytePos = dataSize - rangeUpperBorder;
    }

    if ((dataRange.FirstBytePos() == DataRange::UnspecifiedBytePos) &&
            (dataRange.LastBytePos() == DataRange::UnspecifiedBytePos)) {

        specifiedFirstBytePos = 0;
    }

    uint64_t specifiedLastBytePos = dataRange.LastBytePos();

    if ((dataRange.LastBytePos() != DataRange::UnspecifiedBytePos) &&
            (dataRange.FirstBytePos() == DataRange::UnspecifiedBytePos)) {

        specifiedLastBytePos = (dataSize > 0) ? (dataSize - 1) : dataSize;
    }

    if ((dataRange.LastBytePos() == DataRange::UnspecifiedBytePos) &&
            (dataRange.FirstBytePos() != DataRange::UnspecifiedBytePos)) {

        specifiedLastBytePos = (dataSize > 0) ? (dataSize - 1) : dataSize;
    }

    if ((dataRange.LastBytePos() == DataRange::UnspecifiedBytePos) &&
            (dataRange.FirstBytePos() == DataRange::UnspecifiedBytePos)) {

        specifiedLastBytePos = (dataSize > 0) ? (dataSize - 1) : dataSize;
    }

    DataRange result(specifiedFirstBytePos, specifiedLastBytePos);
    assert(!result.HasUnspecifiedBytePos());

    return result;
}

There are plenty of ifs, which seems very awkward, but I cannot do better. Any suggestions?

Starting from bottom. If you have too many ifs and you want to get rid of them then you should start to name things (as you already did for HasUnspecifiedBytePos()).

This expression dataRange.FirstBytePos() == DataRange::UnspecifiedBytePos is saying IsFirstBytePosSpecified (or HasFirstBytePos). Just add two simple functions to your DateRange class:

bool HasFirstBytePos() const {
    return FirstBytePos() == UnspecifiedBytePos;
}

Now you can rewrite your first if like this:

if (!dataRange.HasFirstBytePos() && dataRange.HasLastBytePos()) {
    uint64_t rangeUpperBorder = std::min(dataSize, dataRange.LastBytePos());
    specifiedFirstBytePos = dataSize - rangeUpperBorder;
}

It's still there but it's shorter and easier to understand at first sight. The same should be applied to all the other checks. Let's do a further step in this direction and refactor out code to calculate specifiedFirstBytePos into its own method (this may stay here or in DataRange class, I don't see how and where you use it).

uint64_t ResolveFirstBytePos(const DataRange& dataRange, uint64_t dataSize) const {
    if (dataRange.HasFirstBytePos())
        return dataRange.FirstBytePos();

    if (dataRange.HasLastBytePos()) {
        uint64_t rangeUpperBorder = std::min(dataSize, dataRange.LastBytePos());
        return dataSize - rangeUpperBorder;
    }

    return 0;
}

Everything together:

DataRange SpecifyDataRangeBytePos(const DataRange& dataRange, uint64_t dataSize) {
    DataRange result(
        ResolveFirstBytePos(dataRange, dataSize),
        ResolveLastBytePos(dataRange, dataSize));

    assert(!result.HasUnspecifiedBytePos());

    return result;
}

Note that you may move assertion inside your constructor (I don't know if it's a valid condition then here I kept original code).

When you will read this code in future you won't need to deeply understand how it is doing things but in few lines you will understand what it is doing.

More things:

explicit on a constructor with two parameters is useless (no default value for lastBytePos), you may simply drop it.
I'd pick better names for FirstBytePos and LastBytePos. Without reading all your (unavailable) code I may just guess it's position (in bytes) within input data. If type is obvious (bytes) then you may simply use BeginInData and EndInData but I'd need more context to suggest anything meaningful. (BytePos comes from domain - its named like this directly in RFC - then it's a good name to use)
IMO HasUnspecifiedBytePos() expresses more an implementation detail (position, in bytes, is unspecified) than a behavior/state/property of your object. It should be renamed to something from domain, for example IsNull().
Your constructor has dataSize parameter but you pass LastBytePos() value. dataSize seems to indicate this parameter is an offset from firstBytePos. If it's not an offset then you should rename it, if it is then code is broken (when firstBytePos > 0)


Custom map annotations and clusters I registered a CustomMapAnnotationView and a ClusterView: the first one simply extends MKMarkerAnnotationView and overrides annotation this way:

override var annotation: MKAnnotation? {
        willSet {
        // CustomMapAnnotation is based on MKPointAnnotation
            if let c = value as? CustomMapAnnotation {
                clusteringIdentifier = "c"
                markerTintColor = UIColor.orange
                displayPriority = .defaultLow
            }
        }
    }

ClusterView is a MKAnnotationView that is used when annotations can be grouped.

 mapView.register(CustomMapAnnotationView.self, forAnnotationViewWithReuseIdentifier: MKMapViewDefaultAnnotationViewReuseIdentifier)
 mapView.register(ClusterView.self, forAnnotationViewWithReuseIdentifier: MKMapViewDefaultClusterAnnotationViewReuseIdentifier)

Now the code review part.

I use CustomMapAnnotationView and ClusterView this way:

func mapView(_ mapView: MKMapView, viewFor annotation: MKAnnotation) -> MKAnnotationView? {

        guard !(annotation is MKUserLocation) else {
            return nil
        }

        if(annotation is CustomMapAnnotation){

            let annotationID = "c"

            var annotationView: CustomMapAnnotationView?

            if let dequeuedAnnotationView = mapView.dequeueReusableAnnotationView(withIdentifier: annotationID) as? CustomMapAnnotationView {
                annotationView = dequeuedAnnotationView
                annotationView!.annotation = annotation
            }
            else
            {
                annotationView = CustomMapAnnotationView(annotation: annotation, reuseIdentifier: annotationID)
                annotationView?.rightCalloutAccessoryView = UIButton(type: .detailDisclosure)
            }

            if let annotationView = annotationView {
                annotationView.canShowCallout = true
            }

            return annotationView

        }else{

            let clusterID = "Cluster"
            var clusterView = mapView.dequeueReusableAnnotationView(withIdentifier: clusterID)
            if clusterView == nil {
                clusterView = ClusterView(annotation: annotation, reuseIdentifier: clusterID)
            } else {
                clusterView?.annotation = annotation
            }

            return clusterView
        }
        }

This code perfectly works or at least it does what I wanted it to do.
But I'm not sure this is correct way to perform this action inside the body of this function. Any suggestion is appreciated.

A couple of observations:

You have code that says:

var annotationView: CustomMapAnnotationView?

if let dequeuedAnnotationView = mapView.dequeueReusableAnnotationView(withIdentifier: annotationID) as? CustomMapAnnotationView {
    annotationView = dequeuedAnnotationView
    annotationView!.annotation = annotation
}
else
{
    annotationView = CustomMapAnnotationView(annotation: annotation, reuseIdentifier: annotationID)
    ...
}

In iOS 11 and later, the mapView.dequeueReusableAnnotationView(withIdentifier:for:) can reduce that to a single line:

let annotationView = mapView.dequeueReusableAnnotationView(withIdentifier: annotationID, for: annotation)

Your mapView(_:viewFor:) is configuring the annotation views. I’d move that configuration code into the MKAnnotationView implementations for better separation of responsibilities and eliminating cruft from your view controller. For example:

class CustomMapAnnotationView: MKMarkerAnnotationView {
    static let clusteringIdentifier = Bundle.main.bundleIdentifier! + ".c"

    override init(annotation: MKAnnotation?, reuseIdentifier: String?) {
        super.init(annotation: annotation, reuseIdentifier: reuseIdentifier)

        clusteringIdentifier = CustomMapAnnotationView.clusteringIdentifier
        markerTintColor = .orange
        displayPriority = .defaultLow
        collisionMode = .circle
        rightCalloutAccessoryView = UIButton(type: .detailDisclosure)
    }

    required init?(coder aDecoder: NSCoder) {
        fatalError("init(coder:) has not been implemented")
    }

    override var annotation: MKAnnotation? {
        willSet {
            clusteringIdentifier = CustomMapAnnotationView.clusteringIdentifier
        }
    }
}

And

class ClusterView: MKMarkerAnnotationView {
    override init(annotation: MKAnnotation?, reuseIdentifier: String?) {
        super.init(annotation: annotation, reuseIdentifier: reuseIdentifier)
        displayPriority = .required
        collisionMode = .circle
        markerTintColor = .purple
    }

    required init?(coder aDecoder: NSCoder) {
        fatalError("init(coder:) has not been implemented")
    }
}

This cleans up your mapView(_:viewFor:) and keeps the annotation view configuration code where it belongs.
Consider your registration of these classes:

mapView.register(CustomMapAnnotationView.self, forAnnotationViewWithReuseIdentifier: MKMapViewDefaultAnnotationViewReuseIdentifier)
mapView.register(ClusterView.self, forAnnotationViewWithReuseIdentifier: MKMapViewDefaultClusterAnnotationViewReuseIdentifier)

Note, you’re registering these identifiers, so your mapView(_:viewFor:) shouldn’t then use its own reuse identifiers of c and Cluster, respectively.
But the whole point of MKMapViewDefaultAnnotationViewReuseIdentifier, etc., in iOS 11 and later is so that you don’t have to implement mapView(_:viewFor:) at all. So you can comment out that whole routine (now that you’ve moved the annotation view configuration code to the annotation views, themselves, where it belongs).

That having been said, sometimes you need a mapView(_:viewFor:). For example, maybe you need to support iOS versions prior to 11. Or perhaps you have more than three types of annotation types (your main custom annotation, your cluster annotation, and MKUserLocation). But then you wouldn’t register MKMapViewDefaultAnnotationViewReuseIdentifier as the reuse identifier. You’d register your own reuse identifier.

If you do use your own reused identifier, I would have a static property in my annotation view with the preferred reuse identifier, rather than sprinkling hardcoded strings throughout my codebase. E.g.

class CustomMapAnnotationView: MKMarkerAnnotationView {
    static let preferredReuseIdentifier = Bundle.main.bundleIdentifier! + ".customMapAnnotationView”

    ...
}

And

mapView.register(CustomMapAnnotationView.self, forAnnotationViewWithReuseIdentifier: CustomMapAnnotationView.preferredReuseIdentifier)

And

extension ViewController: MKMapViewDelegate {
    func mapView(_ mapView: MKMapView, viewFor annotation: MKAnnotation) -> MKAnnotationView? {
        switch annotation {
        case let annotation as CustomMapAnnotation:
            return mapView.dequeueReusableAnnotationView(withIdentifier: CustomMapAnnotationView.preferredReuseIdentifier, for: annotation)

        ...
        }
    }
}

But that’s not needed in your case. You can remove mapView(_:viewFor:) entirely.


Listing files whose names match a pattern with a date My directory folderMarket has lots of files with the same name but are tagged with a date string at the end. The date tag can be formatted differently, e.g. "2018-07-25" or "25Jul18".  My helper function is tasked with extracting a path list matching each found file name against filename_list.  is there a better way to build a filename_list instead of brute force used below?

from datetime import datetime

strToday = "2018-07-25"
files_market = ['apples_DATE.xml', 'peaches_DATE.xml', 'cucumbers_DATE.xml', 'potatos_DATE.xml', 'tomates.DATE.csv']

def get_path_list(directory, base_filename_list, savedAsOf):

    strDate1 = savedAsOf
    filename_list1 = [n.replace('DATE', strDate1) for n in base_filename_list]

    strDate2 = datetime.strptime(savedAsOf, '%Y-%m-%d').strftime('%d%b%y')
    filename_list2 = [n.replace('DATE', strDate2) for n in base_filename_list]
    filename_list = filename_list1 + filename_list2

    path_list = []
    for file in os.listdir(directory):
        filename = os.fsdecode(file)
        if filename in filename_list:
            path_list.append(os.path.join(directory, filename))
            continue
    return path_list

print (len(get_path_list(folderMarket, files_market, strToday)))

Firstly some tips:

Your code uses a mixture of snake_case and camelCase, you should stick to a specific style. If you're going to write python, PEP8 states snake_case should be used as the expected style.
Your code lacks a if __name__ == "__main__": starting section. It's recommended to have this. As an example, if you don't have this, when you use tools like Sphinx to produce code documentation, it will actually run your code instead of just building the objects. Also, it's kind of expected :-)

As to the core questions you have, you can use pathlib's Path object to assist with the globbing of files and matching file parameters, as well as the string formatting format function to replace "placeholders" with a certain value. For instance:

from pathlib import Path

def get_path_list(directory, base_filename_list, saved_as_of):
    for template in base_filename_list:
        file_date = template.format(saved_as_of)
        print(f"looking for {file_date}")
        files = Path(directory).glob(file_date)
        for file in files:
            print(str(file))

if __name__ == "__main__":
    today = "2018-07-25"
    files_market = ['apples_{}.xml', 'peaches_{}.xml', 'cucumbers_{}.xml', 'potatoes_{}.xml', 'tomatoes.{}.csv']
    get_path_list(".", files_market, today)

The code above will get you 90% of the way there, but I'll leave that as an exercise for you to complete.

Good Luck!


Passing Context in CoffeeScript I'm trying to migrate from JavaScript to CoffeeScript. However I'm not sure about the best way to optimize the code generated by js2coffee.

Below is the original JavaScript source :

var async = require('async');

var context = {};
context.settings = require('./settings');

async.series([setupDb, setupApp, listen], ready);

function setupDb(callback)
{
  context.db = require('./db.js');
  context.db.init(context, callback);
}

function setupApp(callback)
{
  context.app = require('./app.js');
  context.app.init(context, callback);
}

// Ready to roll - start listening for connections
function listen(callback)
{
  context.app.listen(context.settings.http.port);
  callback(null);
}

function ready(err)
{
  if (err)
  {
    throw err;
  }
  console.log("Ready and listening at http://localhost:" + context.settings.http.port);
}

And below is the generated CoffeeScript :

setupDb = (callback) ->

  # Create our database object
  context.db = require("./db")

  # Set up the database connection, create context.db.posts object
  context.db.init context, callback
setupApp = (callback) ->

  # Create the Express app object and load our routes
  context.app = require("./app")
  context.app.init context, callback

# Ready to roll - start listening for connections
listen = (callback) ->
  context.app.listen context.settings.http.port
  callback null
ready = (err) ->
  throw err  if err
  console.log "Ready and listening at http://localhost:" + context.settings.http.port
async = require("async")
context = {}
context.settings = require("./settings")
async.series [setupDb, setupApp, listen], ready

Now, I'm not sure if the context variable is required in CoffeeScript. In the JavaScript source its function is to share the common set of setting across the various components of the application such as database and the settings. Will a proper use of the => operator in CoffeeScript help ? If yes how ?

The CoffeeScript looks fine to me. You'll still need the context var just as in plain JS, because you're not dealing with this. If you were, you could maybe use the fat arrow to preserve the this context, but even so it'd require some refactoring, and end up very different from the original JS.

CoffeeScript's fat arrow is basically meant to be used where you'd otherwise do the var that = this; trick in JavaScript. But since you're not doing such things in your JavaScript, there's no need or opportunity for it in the CoffeeScript either.


PHP Configuration class Created this little config class, any room for improvements? Think I have done an alright job but would like to know if any improovements could be made?

<?php
defined("SECURE") or exit('Please define SECURE keyword to continue.');
class config
{
    private $configValues;
    private $configFile;

    public function __construct()
    {
        $configValues = array();
    }

    public function loadConfiguration($configFile = "C:\Users\ashle\Miracle\config\config.ini")
    {
        if (!file_exists($configFile))
            loadDefault();

        $content = file($configFile);
        foreach ($content as $line)
            $this->setConfig(explode("=", $line)[0], explode("=", $line)[1]);
    }

    private function loadDefault()
    {
        /* DEFAULT CONFIG SETTINGS!!
           THIS SHOULD NOT BE THE CASE... */

        setConfig("database.host", "localhost");
        setConfig("database.username", "root");
        setConfig("database.password", "");
        setConfig("database.title", "mydb");
        setConfig("database.port", "3306");
    }

    private function setConfig($name, $value)
    {
        $this->configVlaues[$name] = $value;
    }

    public function getConfig($name)
    {
        return $this->configValues[$name];
    }
}
?>

Things that just pop up in my mind when seeing the code (more of the architecture and robustness rather than language constructs):

1) Harcoding path to ini file

The path might have a meaning in your development environment, but it will mean nothing in a test or production environment. I would set it in an included file and use it from there. Something like:

$defaultConfigFilePath = "C:\Users\ashle\Miracle\config\config.ini";

in loadConfiguration
    $configFile = !isset($configFile) ? $defaultConfigFilePath : $configFile;

Hopefully, they will introduce a coalesce operator soon to simply write:

$configFile = $configFile ?? $defaultConfigFilePath;

2) Error handling

loadConfiguration function does not handle any incorrect formatting. Any line which is not in the format name=value will not be imported correctly.

You can explode into an $lineArray and check if elements are defined and exactly two.

3) A more powerful config reader

This is a good exercise, but ini files have a more complex format that seems to be handled by parse_ini_file function.


Find common elements in a list of arrays I need to find the common elements present in all the given arrays. All the arrays are present in another array.

I have come up with this solution and it's working. I tried to remove the usage of indexOf, but I could not. Could someone help me optimize this?

var findCommonElements= function(arrs) {
    var resArr = [];
    for (var i = arrs[0].length - 1; i > 0; i--) {

        for (var j = arrs.length - 1; j > 0; j--) {
            if (arrs[j].indexOf(arrs[0][i]) == -1) {
                break;
            }
        }

        if (j === 0) {
            resArr.push(arrs[0][i]);
        }

    }
    return resArr;
}

Input Array of arrays:

var arrays  = [
    [1, 4, 6, 78, 8, 9, 124, 44],
    [44, 6, 9],
    [124, 44, 16, 9]
]

Output:

findCommonElements( arrays )
[44, 9]

In your code, you are using arrays to check whether the value is stored.  As you have noted, that operation is \$O(n)\$.  The solution to this is to use objects to check instead.  That operation is \$O(1)\$, meaning that the algorithm goes from \$O(n^2)\$ to \$O(n)\$.

However, there is a problem with storing using objects, namely that all keys need to be strings.  Because of this, we have to convert the strings back into integers after we are done.

Hence, our steps become:

Create a currentValues object.  Initialize it to contain the values of the first list.
Create a commonValues object, leave it empty.
For each of the arrays, except for the first:

Iterate through the array.  If currentValues contains the value, add it to commonValues
Set currentValues = commonValues, and reset commonValues to be an empty object again.

Finally, take the currentValues object, and convert its keys back into integers.

Some code that does the above (I'm no expert with Javascript, while the code works, there may be sub-optimal code here):

var arrays  = [
    [1, 4, 6, 78, 8, 9, 124, 44],
    [44, 6, 9],
    [124, 44, 16, 9]
];
function getCommonElements(arrays){//Assumes that we are dealing with an array of arrays of integers
  var currentValues = {};
  var commonValues = {};
  for (var i = arrays[0].length-1; i >=0; i--){//Iterating backwards for efficiency
    currentValues[arrays[0][i]] = 1; //Doesn't really matter what we set it to
  }
  for (var i = arrays.length-1; i>0; i--){
    var currentArray = arrays[i];
    for (var j = currentArray.length-1; j >=0; j--){
      if (currentArray[j] in currentValues){
        commonValues[currentArray[j]] = 1; //Once again, the `1` doesn't matter
      }
    }
    currentValues = commonValues;
    commonValues = {};
  }
  return Object.keys(currentValues).map(function(value){
    return parseInt(value);
  });
}
console.log(getCommonElements(arrays)); //Prints [9,44]


Echoing table contents I'm working on an exercise where I"m supposed to display all data contained in a table.  Here's the solution I can up with:

<?php
$result = mysql_query("SELECT * FROM contacts ORDER BY id DESC");
    while($user= mysql_fetch_array($result)) {
        echo $user['first'] . " " . $user['last'] . "
" . $user[phone] . "br />";
    }
?>

Here's the solution the tutorial gave:

<?php
$result=mysql_query("SELECT * FROM contacts ORDER BY id DESC");

$num=mysql_num_rows($result);

$i=0;
while ($i < $num) {

$first=mysql_result($result,$i,"first");
$last=mysql_result($result,$i,"last");
$phone=mysql_result($result,$i,"phone");

echo "$first $last
Phone: $phone

";

$i++;
}
?>

The database fields are:

id
first
last
phone

Can someone explain WHY one is better than the other. They both work as far as I can tell.

Yours is better.  Both are bad.
Don't use either of them
The mysql_* functions aren't a good way to learn how to interact with databases.  It is a shame that there are still tutorials teaching how to interact with databases using these functions.  Here is a snippet that explains more about why you shouldn't use those functions and what you can use instead:

Please, don't use mysql_* functions in new code. They are no longer maintained and the deprecation process has begun on it. See the red box? Learn about prepared statements instead, and use PDO or MySQLi - this article will help you decide which. If you choose PDO, here is a good tutorial.Snippet Source

Minor Comments
You missed quotes around phone.
In pure PHP don't use an end tag ?>.  It can cause output if you have blank lines after it causing headers to be sent prematurely (see this).


Generating Pythagorean triples below an upper bound I've got an assigment where I have to print all the Pythagorean triples smaller than a given n. Even though I could print them directly in pythagoreanTriple function, I decided to structure everything, as I want to go deply into programming structure.(my teacher does not teach us a lot, so I'll have to study on my own).

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

public class Main {

    public static void main(String[] args) {
        System.out.println("Starting program...");
        int n = readFromKeyboard();
        printResult(pythagoreanTriple(n));
    }

    private static int  readFromKeyboard(){
        Scanner keybInput = new Scanner(System.in);
        return keybInput.nextInt();
    }

    private static List<List<Integer>> pythagoreanTriple(int n){
        List<List<Integer>> pytTri = new ArrayList<List<Integer>>();
        int i,j,k;
        for (i = 3; i < n; i++)
            for (j = n; j > i; j--)
                for (k = n; k > j; k--)
                    if (i * i + j * j == k * k){
                        ArrayList<Integer> tempArray = new ArrayList<Integer>(); //we create a list of integers
                        tempArray.add(i);
                        tempArray.add(j);
                        tempArray.add(k);
                        pytTri.add(tempArray);
                    }
        return pytTri;
    }

    private static void printResult(List<List<Integer>> resultList){
        for (List<Integer> ls: resultList){
            for (Integer i: ls){
                System.out.print(i);
                System.out.print(" ");
            }
            System.out.println(";");
        }
    }

}

Knowing these:

I am curious about the following: How are the variable/function's name, should I use underscore _ for private variables etc...
I am not new to programming

(I know both C++ and Python so far), but I'm more interested in learning Java next.)

It's a good thing that there is a method to calculate the pythagorean triple up to a number, and then a second method to print the results.

About pythagoreanTriple

You can simply the pythagoreanTriple method in several aspects. First of all is performance. The current code does:

for (i = 3; i < n; i++)
    for (j = n; j > i; j--)
        for (k = n; k > j; k--)
            if (i * i + j * j == k * k) {

to check for triplets. This tries every possible combinations of i, j and k and checks if they form a triplet. However, you don't need to loop for every possible k: once you know i and j, you can deduce the only possible k for which (i,j,k) forms a triplet; it is exactly \$k = \sqrt{i^2+j^2}\$, when this result in an integer. Take care because such a calculation can lead to rounding issues, and there are good way to check if a number square root is an integer. I won't comment more on the performance, because if this is really a concern, there are other faster approaches than trying every possible combinations (see for example the Tree of primitive Pythagorean triples).

Second aspect, is the usage of built-in methods.

ArrayList<Integer> tempArray = new ArrayList<>();
tempArray.add(i);
tempArray.add(j);
tempArray.add(k);
pytTri.add(tempArray);

can be written more simply using the built-in Arrays.asList. You can directly have:

pytTri.add(Arrays.asList(i, j, k));

without creating any new ArrayList yourself.

Finally, even if it isn't strictly necessary, consider adding curly braces everywhere.

for (i = 3; i < n; i++)
    for (j = n; j > i; j--)

can quickly lead to problems if you want to add another statement in there, like storing the result of i * i as recommented by coderodde in their answer.

About readFromKeyboard

Currently, the code to retrieve the user input does not validate that only integers can be input. If you try to input a String like "a", an exception will be thrown; consider re-prompting for an input if that happens.

Also, this method creates a new Scanner. In the current code, it is called only once, but if later you want to read a second input from the user, it would be better to create it only once and reuse it. As such, consider creating the Scanner and passing it as parameter of readFromKeyboard.

This also has a direct impact: readFromKeyboard would not read from the keyboard anymore, it would read from a given Scanner, which makes it more reusable, since you could pass any instance of Scanner you want (for example, one reading from a file). And the method should renamed to something like readUpperBound, which makes it clear that its concern is not to read from the keyboard (which is the current technical implementation), but to fetch the upper bound to use in order to create the triplets (which is what it's really supposed to do).