Determining primality by removing select composits Calculating many remainders for each prime and composite consumes more time.

I use a factoring function to find factors of single values.

When I use a factoring function to find lists of primes, I use a wheel.

A wheel exploits composite values punctuating an otherwise prime sequence.

The primary advantage of a wheel is reducing significantly, the candidate list and so processing.

My wheel eliminates 2, 3, 5 and 7 multiples from the a list. For the longest time, I used only elimination of 2s, 3s and 5s because the wheel list is only 8 values and simple to construct but there are just to many 7s.

Take the list of primes 11,13,17,19,23,29,31,39 and add 30 to each of the previous 8 values for subsequent values.

Take the first 55 from the list and remove the 7 multiples. The deltas will be this wheel.

What should be faster is subtraction of one list from another where the calc is compare and once only for each.

wl = [2,4,2,4,6,2,6,4,2,4,6,6,2,6,4,2,6,4,6,8,4,2,4,2,4,8,6,4,6,2,4,6,2,6,6,4,2,4,6,2,6,4,2,4,2,10,2,10]
-- the so-called wheel: the recurring pattern in a composite punctuated list
n7sl = scanl (+) 11 $ cycle wl -- infinite

-- short list of composits with no 2's, 3's 5's or 7's; 77%+ reduced
n7s = take 150 $ n7sl

-- diagonalize and limit the factor's composite list
lls i y = drop i.take y $ n7sl

-- substract the 1st list from the 2nd, infinite list
rms [] _ = [] -- stop when first list is exhausted
rms n@(x:xs) p@(y:ys)
    | x==y = rms xs ys
    | x>y  = y:rms n ys
    | y>x  = rms xs p

-- generate the composite list
comps [] _ _ _ _  =[]
comps (n:ns) [] y i b = comps ns (lls (i+1) y) y (i+1) b
comps k@(n:ns) (r:rs) y i b
      | m>b  =comps ns (lls (i+1) y) y (i+1) b
      | m<=b =m:comps k rs y i b
   where m = n*r

-- the result of `comps` is not just a diagonalization but 2, top & irregular bottom
-- `comps` is maybe necessary to stop when the limit of a list is reached
-- otherwise, I was using a list comprehension which is way less complicated

-- `comps` has to many parameters so this to reduce it to 1
-- it will be subtracted the fixed length wheel numbers and the lazy wheel
comp1 n =sort $ comps n7s (lls 0 n) n 0 (11*(last.take n $ n7sl))
-- put everything together to generate primes

last $ rms (comp1 5000) n7sl

240641

(0.12 secs, 65,219,592 bytes)

last $ rms (comp1 10000) n7sl

481249

(0.24 secs, 143,743,944 bytes)

last $ rms (comp1 15000) n7sl

721891

(0.37 secs, 220,099,904 bytes)

Question is this better that factoring? Is there a better way to generate multiples without multiplying in comps or list comprehensions?

Any prime list I generate excludes 2,3,5 & 7 because they are misbehaved, irregular and to well known.

You should use more Data.List functions - explicit recursion is hard to read.

comp1 y = sort [ m
  | (i,n) <- zip [0..149] n7sl
  , m <- takeWhile ((<= 11 * last (take y n7sl))) $ map (n*) $ drop i $ take n n7sl
  ]

Edit: Not sure whether my version fails to be equal to your original one, but here's a further refactoring of the one in your comment to get rid of some number manipulation. The takeWhile reflects the short-circuiting behavior of the original, explicitly recursive implementation, which uses that bs is ascending.

t = sort [ m | bs@(a:_) <- tails n7s,
  m <- takeWhile (<= 11*last n7s) $ map (a*) bs]


Use Python to determine the repeating pattern in a string (no leftovers) As a part of a challenge, I was asked to write a function to determine if there was repetition in a string (if no repetition, output is 1), and if a substring is repeated throughout the string (for example if the string is "abcabc," "abc" is repeated twice 2, if the string is "bcabcabc," the repetition is 0 since we start from the first string (no leftovers)).

My code works and I have submitted it to the challenge, but I am curious if there are ways to improve this code:

def answer(s):
     length = (len(s))
     range_list = (range(1, len(s)+1))
     list_char = list(s)
     divisors = []
     divisors = [x for x in range_list if (length)% (x) == 0]
     max_pieces = []
     for i in range (len(divisors)):
         size = divisors[i]
         split = [list_char[i:i+size] for i in range(0, length, size)]
         if(all(x == split[0] for x in split)):
             max_pieces = int(length/size)
             break
     return max_pieces

length = (len(s))

Unnecessary ( )’s.

 range_list = (range(1, len(s)+1))

Again, unnecessary ( )’s.  Does not actually create a list, just a range object, so a better name is in order.   And you can remove this entire statement (see below).

 list_char = list(s)

You don’t need to create a list of characters to solve this problem.  (More later.)

 divisors = []

Unnecessary statement.  Made obsolete by very next statement:

 divisors = [x for x in range_list if (length)% (x) == 0]

Unnecessary ( )’s ... twice.  Could replace range_list with range(1, len(s)+1), and remove above range_list = .....

 max_pieces = []

Misleading and useless.  You store a single integer in this variable later, not a list.

 for i in range (len(divisors)):
     size = divisors[i]

A more pythonic way would be:

 for size in divisors:

I might use divisor instead of size, when looping over a list of divisors.

     split = [list_char[i:i+size] for i in range(0, length, size)]

Confusing.   i is already used in outer loop.  List comprehension’s i is (fortunately) a separate variable ... but give it a different name ... maybe j.

Instead of list_char[i:i+size], you could simply use s[i:i+size].  This would return a substring instead of a sublist of characters, which would be just as effective.

     if(all(x == split[0] for x in split)):

The if statement does not need the outer ( )’s.

         max_pieces = int(length/size)

More pythonic would be max_pieces = length // size

         break
 return max_pieces

break could be replaced with just return max_pieces.  And the current return statement replaced with an error message or return 1 catch-all.


Statistical queries on a list of customers I have been given a list of simple Customer objects to work with. This would be an example of a customer list:

customers = [
    Customer(active=True, age=38, gender='F'),
    Customer(active=False, age=18, gender='M'),
    None,
    Customer(active=False, gender='F'),
    None,
    Customer(age=64),
    Customer(active=True, age=23),
    None,
    Customer(age=23, gender='M'),
    None,
]

The list contains None values for customers that have been deleted and active, gender and age are not required properties of Customer and so they can be un-set.

To find the number of active customers:

def number_of_active_customers(customers):
    # Assumption: A customer is only active if active=True is set
    return len([customer for customer in customers if customer is not None and customer.active is True])

To find the number of inactive customers:

def number_of_inactive_customers(customers):
    # Assumption: Deleted customers also count as inactive
    return len([customer for customer in customers if customer is None or customer.active in (False, None)])

To find out if the list of customers has any None values:

def list_has_none_values(customers):
    # any() won't work here since a list of None's always evaluates to False. We don't need to know the length
    # of the list so we can simply loop over each element and return True when we encounter the first None value
    for customer in customers:
        if customer is None:
            return True
    return False

To find out if the list has any customers with active=None:

def list_has_active_equals_none_customers(customers):
    # any() works here because we can stop processing at the first True value in the list
    return any([customer for customer in customers if customer is not None and customer.active is None])

To find the average age of male customers:

def average_age_of_male_customers(customers):
    try:
        # Round average to 1 digit
        return round(
            mean(
                (customer.age for customer in customers if customer is not None
                 and customer.gender == 'M' and isinstance(customer.age, int))
            ),
            1)
    except StatisticsError:
        # Return 0 when there are no male customers
        return 0

My code should be as pythonic and efficient as possible. I'm fairly sure I got it, I'm just second-guessing myself a little. This is not a school assignment and I am allowed to ask for help.

If you need to do more data analysis on this afterwards, then this might be a good time to learn about pandas which has data frames, allowing you to modify whole records at a time.

First, read in your list into a data frame:

import pandas as pd

df = pd.DataFrame([[customer.active, customer.age, customer.gender]
                   for customer in filter(None, customers)],
                  columns=["active", "age", "gender"])

Here the filter(None, it) filters out all None values (and all falsey values, but an object that exists is by default truthy, so unless you overwrote Customer.__eq__ or Customer.__bool__, it should be fine).

Now we have a data frame like this:

print(df)
#   active   age gender
# 0   True  38.0      F
# 1  False  18.0      M
# 2  False   NaN      F
# 3   None  64.0   None
# 4   True  23.0   None
# 5   None  23.0      M

To make handling of the left over None values easier, we replace them with numpy.nan:

import numpy as np

df = df.replace([None], np.nan)
print(df)
#   active   age gender
# 0   True  38.0      F
# 1  False   NaN      M
# 2  False   NaN      F
# 3    NaN  64.0    NaN
# 4   True  23.0    NaN
# 5    NaN  23.0      M

Now to get to your tasks:

Number of active customers:

active_users = df.active.sum()
# 2

Number of inactive customers:

deleted_users = sum(customer is None for customer in customers)
inactive_users = len(df) + deleted_users - active_users
# 8

Any customer deleted:

deleted_users > 0
# True

Any customer with active=None:

df.active.isna().any()
# True

Average of male customers (NaNs are automatically ignored):

df[df.gender == "M"].age.mean()
# 20.5

If there are no people of that gender, it will return a numpy.nan instead of 0, though:

df[df.gender == "X"].age.mean()
# nan

From there on you can also do more fancy stuff:

Average age per gender:

df.groupby("gender").age.mean()
# gender
# F    38.0
# M    20.5
# Name: age, dtype: float64

Using this makes your code both more efficient to write (since most things you could want to do are already implemented) as well as faster to run (since the methods are usually implemented in C, instead of running in Python. So if you have lots of customers, this will certainly be better.


Combinatorial Geometry Algorithm (synthetic) I created an algorithm to solve in synthetic geometry. This problem states that these points are a scalar (real) number. The goal is to first make the possible triangles from the line segments. Then sum up the points within the triangles. For this question, ordering does not matter within the triangle.
I would like to know if there are any improvements possible on the algorithm. For example, if there is a Las Vegas Algorithm for it.
Also, I would like to know if I should make classes or is this even less maintainable than using built-in data structures.

"""

Triangle sums module

module fn
make_triangles(the_lines: [(float, float)]) -> [(float, float, float)]
total_triangles(the_triangles: [(float, float, float)]) -> [float]
user_output(the_sums: [float]) -> None

"""

"""
Problem

Given:
    Our points are made of a single float
    These points are made into tuples within a list
Find:
    the triangles within the points
    The sum of the points on the triangle

Note: the triangles have no distinct ordering in the points
"""

def make_triangles(the_lines: [(float, float)]) -> [(float, float, float)]:
    """
    :param the_lines: the lines to make triangles from
    :return: the triangles
    time: O(len(the_lines) * len(the_lines)
    space: O(len(the_lines) * len(the_lines))
    """
    the_triangles: [(float, float, float)] = []
    for pair_a in the_lines:
        for pair_b in the_lines:
            if pair_a == pair_b:
                continue
            if pair_a[0] == pair_b[0]:
                the_triangles.append((pair_a[0], pair_a[1], pair_b[1]))
            elif pair_a[0] == pair_b[1]:
                the_triangles.append((pair_a[0], pair_a[1], pair_b[0]))
    return the_triangles

def total_triangles(the_triangles: [(float, float, float)]) -> [float]:
    """
    :param the_triangles: the triangles of points to add up
    :return: a list of the totals
    time: O(len(the_triangles))
    space: O(len(the_triangles))
    """
    vec: [float] = [0.0] * len(the_triangles)
    for row in range(len(vec)):
        vec[row] = the_triangles[row][0] + the_triangles[row][1] + the_triangles[row][2]
    return vec

def user_output(the_sums: [float]) -> None:
    """
    :param the_sums: the calculated sums
    :return: None
    Output: std output
    """
    row = 0
    for number in the_sums:
        if row % 3 == 0:
            print('', end='\n')
        print(str(number), end=" ")
        row = row + 1
    print('\n', end='')

def main() -> None:
    """
    Entry point for script
    :return: None
    Input: None
    Output: std output
    """
    the_lines = [(0.0, 1.0), (1.0, 2.0), (2.0, 3.0)]
    the_triangles = make_triangles(the_lines)
    the_sums = total_triangles(the_triangles)
    user_output(the_sums)

if __name__ == "__main__":
    main()

 ```

Here's my attempt at trimming it down. Note that there's no real reason to prefix variables with the_. Also list comprehensions can help with making code more concise, removing the need for total_triangles() entirely.
def make_triangles(lines: [(float, float)]) -> [(float, float, float)]:
    """
    :param lines: the lines to make triangles from
    :return: the triangles
    time: O(len(lines) * len(lines)
    space: O(len(lines) * len(lines))
    """
    triangles: [(float, float, float)] = []
    for ax, ay in lines:
        for bx, by in lines:
            if ax == bx and ay != by:
                triangles.append((ax, ay, by))
            if ax == by and ax != bx:
                triangles.append((ax, ay, bx))
    return triangles

def print_sums(sums: [float]) -> None:
    """
    :param the_sums: the calculated sums
    :return: None
    Output: std output
    """
    row = 0
    for number in sums:
        if row % 3 == 0:
            print('', end='\n')
        print(str(number), end=" ")
        row = row + 1
    print('\n', end='')

def main() -> None:
    """
    Entry point for script
    :return: None
    Input: None
    Output: std output
    """
    lines = [(0.0, 1.0), (1.0, 2.0), (2.0, 3.0)]
    triangles = make_triangles(lines)
    sums = [ sum(triangle) for triangle in triangles ]
    print_sums(sums)

if __name__ == "__main__":
    main()


User model with validation, delegates, and carrierwave upload mounter I've refactored this model about 50 times according to codeclimate, trying to get the code smell down. With all of the associations, I feel like this is as bare bones as I can get it while still keeping the necessary associations. I wanted to throw it up on here and see what people thought or if anyone had any suggestions on what I could do differently. It's pretty basic, has some custom validators for email and passwords, delegates, carrierwave upload mounter and some concerns included.

class User < ActiveRecord::Base
  self.table_name = "users_standard"

  include Scopeable::Clientable, Searchable, Auditable
  attr_accessor :password, :password_confirmation

  belongs_to :role
  has_many :discussion_posts, dependent: :destroy
  has_many :passtokens
  has_many :attendees
  has_many :authorized_ipads
  has_many :bookjobs
  with_options foreign_key: "user_id" do |user|
    user.has_and_belongs_to_many :questions,    join_table: "questions_users"
    user.has_and_belongs_to_many :chatposts,    join_table: "chatusers"
    user.has_and_belongs_to_many :permissions,  join_table: "permissions_users"
    user.has_and_belongs_to_many :committees,   join_table: "committees_users"
    user.has_and_belongs_to_many :bookfolders,  join_table: "bookfolders_users"
    user.has_and_belongs_to_many :efolders,     join_table: "efolders_users"
    user.has_and_belongs_to_many :surveys,      join_table: "surveys_users"
    user.has_and_belongs_to_many :tasks,        join_table: "tasks_users"
  end

  with_options uniqueness: { scope: :client_id } do |user|
    user.validates :email, email: true, allow_blank: true
    user.validates :companyemail, email: true, allow_blank: true
    user.validates :login, presence: true
  end
  validates :firstname, :lastname, presence: true
  validates :password, presence: true, confirmation: { message: "does not match" }, password: true, allow_blank: true

  before_save do |u|
    if u.password.present?
      self.password_hash = SecurePasswordService.new(self.password).secure
      self.p2            = true
    end
  end

  accepts_nested_attributes_for :permissions, allow_destroy: true

  delegate :title, :hostname, :forge_email_sender, :email_from, :email_new_account, :profile_update_email, :remove_email_footer, :disclaimer, :disclaimer_expiration_period, :password_change_days, :ftlogin, :user_lockout, to: :client, prefix: true

  mount_uploader :photo, UserPhotoUploader

  def self.active
    where('active = 1 and (system = 0 or system is null) and global_spawn != 1').order('lastname asc')
  end

  def self.private_directory_list(user)
    joins(:committees).where('committees.id in (?)', user.committees.map(&:id))
  end

  def endpoint_safe
    { user: self.attributes.with_indifferent_access.except!(:client_id) }
  end

  def is_committee_member?(committee)
    (committee.is_a?(Committee) && self.committees.include?(committee)) or (!committee.is_a?(Committee) && committee.map{ |c| self.committees.include?(c) })
  end
end

A partial (an really short) answer:

for User.active method you have the following:

system = 0 or system is null

I think you should change the condition order to

system is null or system = 0

This because when (and depending of dbms) the database compare null with a value (system = 0 could be null = 0) it can invalidate the condition.


Filling TextBoxes from a WPF DataGrid I have a very basic method at the moment that fills text boxes with data relevant to the selected company. I load the data into a DataGrid using this method;

CompanyDataService

namespace SdcDatabase.Controller
{
    class CompanyDataService
    {
        public DataTable GetCompanyList()
        {
            string constr = ConfigurationManager.ConnectionStrings["dbfString"].ConnectionString;
            using (var dbfCon = new OleDbConnection(constr))
            {
                dbfCon.Open();
                using (var myQuery = dbfCon.CreateCommand())
                {
                    myQuery.CommandText = "SELECT cm_name, cm_addr1, cm_addr2, cm_town, cm_county, cm_pcode, cm_tel1, cm_tel2, cm_fax, cm_email FROM compns ORDER BY cm_name";
                    var dTable = new DataTable();
                    var dataAdapter = new OleDbDataAdapter(myQuery);
                    dataAdapter.Fill(dTable);
                    return dTable;
                }
            }
        }
    }
}

After the data is inside the DataGrid I then load the data into the textboxes based upon the company selected. I do this using this method;

FillCompanyListTextBoxes

        private void FillCompanyListTextBoxes(object sender, SelectionChangedEventArgs e)
    {
        DataRowView dv = (DataRowView)dataGrid.SelectedItem;
        if (dv != null)
        {
            FillCompanyDetailsTextBoxes();
            compDetailsLabel.Content = "Details for: " + dv.Row.ItemArray[0].ToString();
            compNameTextBox.Text = dv.Row.ItemArray[0].ToString(); //Company Name
            compAddr1TextBox.Text = dv.Row.ItemArray[1].ToString(); //Company Addr1
            compAddr2TextBox.Text = dv.Row.ItemArray[2].ToString(); //Company Addr2
            compTownTextBox.Text = dv.Row.ItemArray[3].ToString(); //Company Town
            compCountyTextBox.Text = dv.Row.ItemArray[4].ToString(); //Company County
            compPcodeTextBox.Text = dv.Row.ItemArray[5].ToString(); //Company Post Code
            compTelTextBox.Text = dv.Row.ItemArray[6].ToString(); //Company Telephone
            compAltTelTextBox.Text = dv.Row.ItemArray[7].ToString(); //Company Alt Tel
            compFaxTextBox.Text = dv.Row.ItemArray[8].ToString(); //Company Fax
            compEmailTextBox.Text = dv.Row.ItemArray[9].ToString(); //Company Email
        }
    }

FillCompanyDetailsTextBoxes is simply a method that loads that data into textboxes in the next tabitem, allowing the user update the companies details if necessary.

I know the second method isn't great. It works and it doesn't look overly disturbing but many problems would be encountered if for some reason the column numbers changed and then all the text boxes would have a domino effect and all be in the wrong place.

Basically my question is this, is there a better way?

Like I've said in an answer to a previous question of yours: implement MVVM. Do not work in the code-behind, instead bind from the XAML in your View to its ViewModel.

But more importantly: first you need to stop using DataTables and start using well-defined objects. Your code already shows that currently you need to add comments to remember that for instance the sixth item of an ItemArray that's attached to your row contains the Company Telephone. Now what if you simply had a List<Company> where each Company had a property called Telephone -- wouldn't that be far easier and less error-prone?

Note that your code still contains the same bad practices we warned about in your two previous questions, e.g. OleDbDataAdapter is IDisposable so it needs to be inside a using. You need to apply our remarks not just to the small bit of code you offer for review, but you need to apply those lessons to all of your code.


User defined literal for std::integral_constant I created a user defined literal _c to convert an "integer" literal into an std::integral_constant. Basically, the goal is to allow users to write std::integral_constant instances without the usual boilerplate. Here is the implementation:

#include <type_traits>

template<typename T, typename U>
constexpr auto pow_helper(T acc, T value, U times)
    -> T
{
    return (times > 1) ?
        pow_helper(acc*value, value, times-1) :
            acc;
}

// Compile-time pow function, only works with
// an unsigned integer exponent
template<typename T, typename U>
constexpr auto pow(T value, U exponent)
    -> T
{
    return (exponent == 0) ? 1 :
        (exponent > 0) ? pow_helper(value, value, exponent) :
            1 / pow_helper(value, value, -exponent);
}

// Structure to parse an integer literal
template<typename Integral, char C, char... Digits>
struct parse
{
    static constexpr Integral value =
        parse<Integral, C>::value * pow(10u, sizeof...(Digits))
        + parse<Integral, Digits...>::value;
};

// Specialization of parse to parse a single
// decimal digit
template<typename Integral, char C>
struct parse<Integral, C>
{
    static_assert(C >= '0' && C <= '9',
                  "only characters in range 0..9 are accepted");

    static constexpr Integral value = C - '0';
};

// User defined literal for std::integral_constant
template<char... Digits>
constexpr auto operator"" _c()
    -> std::integral_constant<int, parse<int, Digits...>::value>
{
    return {};
}

With this literal, wirting 42_c generates an instance of std::integral_constant<int, 42>. Here is a small working example:

int main()
{
    std::cout << 45_c << '\n';   // prints 45
    std::cout << -23_c << '\n';  // prints -23

    static_assert(std::is_same<decltype(58_c), std::integral_constant<int, 58>>::value, "");
}

To generate other integral constants, I plan to add the user defined literals _cl, _cll, _cu, _cul and _cull whoe implementation is exactly the same, only the resulting type differs.

Is there a way to improve this code and/or make it cleaner or more idiomatic? Have I missed some potential flaws?

Usage of "fat" templates

I would try to avoid using full-blown class templates when there's an elegant alternative solution with alias templates or constexpr functions. Note that I didn't measure this, so take it with a grain of salt: lightweight constexpr functions and alias template can be faster to instantiate. Using constexpr functions might reduce the number of instantiations to a minimum of 1 (the literal operator template) or two (the constexpr function template with a single template type parameter).

Algorithm

By changing the order of operations, you can get rid of the pow function (template) entirely: Pass the current result to the next step, then multiply and add. The shifting (in base 10) will be done on the fly.

Sketch:

constexpr int combine(int p)
{
    return p;
}

template<class... TT>
constexpr int combine(int val, int p0, TT... pp)
{
    return combine(val*10 + p0, pp...);
}

constexpr int parse(char C)
{
    return (C >= '0' && C <= '9')
           ? C - '0'
           : throw std::out_of_range("only decimal digits are allowed");
}

template<char... Digits>
constexpr auto operator"" _c()
-> std::integral_constant<int, combine(0, parse(Digits)...)>
{
    return {};
}

Literals in other bases

Your parse function currently rejects hexadecimal, octal, and in C++1y binary literals, as well as C++1y's digit separators. Supporting the bases < 10 is fairly simple because you only need to change the "shifting" factor. I.e. multiply by 8 or 2 instead of 10. For base 16, you also need to add some code to the character -> digit conversion. IIRC, it's not guaranteed that the letters are contiguous in the basic execution character set (as opposed to the digits), so this could be rather painful w/o a switch or lookup-table. Also, upper/lower case.

Automatically growing literal type

Normal C++ literals automatically adjust their types according to their value. If they don't fit into an int, they'll try long, long long etc. Maybe one of the user-defined literals should mimic that behaviour for convenience.

Operators

Unfortunately, the StdLib doesn't provide operators for std::integral_constant. Therefore, -23_c will not be a std::integral_constant<int, -23> but rather an int (via the implicit conversion operator). I find that surprising.

Consider using a custom type, possibly derived from / convertible to std::integral_constant and provide (metaprogramming) operators for this type.

Side remark:

lit<45> is much easier to implement:

template<std::uintmax_t N>
using lit = std::integral_constant<std::uintmax_t, N>;


Codility Frog Jump - Count minimal number of jumps from position X to Y Here is a question I tried from the Codility train website:

  A small frog wants to get to the other side of the road. The frog is
  currently located at position X and wants to get to a position greater
  than or equal to Y. The small frog always jumps a fixed distance, D.
  Count the minimal number of jumps that the small frog must perform to
  reach its target.

  Write a function:

class Solution { public int solution(int X, int Y, int D); }

  that, given three integers X, Y and D, returns the minimal number of
  jumps from position X to a position equal to or greater than Y.

  For example, given:

  X = 10   Y = 85   D = 30

  the function should return 3, because the frog will be positioned as
  follows: after the first jump, at position 10 + 30 = 40 after the
  second jump, at position 10 + 30 + 30 = 70 after the third jump, at
  position 10 + 30 + 30 + 30 = 100

Assume that:
X, Y and D are integers within the range [1..1,000,000,000];
X ≤ Y.
Complexity:
expected worst-case time complexity is O(1);
expected worst-case space complexity is O(1).

This is the solution I gave which fetched me 50% and time complexity of O(Y-X). Can anyone please suggest a better solution?

class Solution {
//X=start, Y=end, D=distance for code clarity
public int solution(int start, int end, int distance) {

// write your code in Java SE 7
int progress = start;
int count=0;
while(progress<end) {
progress=progress+distance;
count++;
}
return count;
}
}

You don't need a loop for this, there is a mathematical solution:

If y - x is divisible by d, then it takes (y - x) / d jumps
If y - x is not divisible by d, then it takes (y - x) / d + 1 jumps

In other words:

if ((y - x) % d == 0) {
    return (y - x) / d;
}
return (y - x) / d + 1;

Or the somewhat less readable but more compact:

return (y - x) / d + ((y - x) % d == 0 ? 0 : 1);


Returning items which are different from their preceding items from a list I have a list of tuples (key, value1, value2, ...). I simplify the tuple here so it is only of length 2. Keys contain duplicated figures.

xs = zip([1,2,2,3,3,3,4,1,1,2,2], range(11)) # simple to input

[(1, 0),
 (2, 1),
 (2, 2),
 (3, 3),
 (3, 4),
 (3, 5),
 (4, 6),
 (1, 7),
 (1, 8),
 (2, 9),
 (2, 10)]

Now, I would like to build a function, which takes the list as input, and return a list or generator, which is a subset of the original list.

It must captures all items where the keys are changed, and include the first and last items of the list if they are not already there.

f1(xs) = [(1, 0), (2, 1), (3, 3), (4, 6), (1, 7), (2, 9), (2, 10)]
f1([]) = []

The following is my code, it works, but I don't really like it:

xs = zip([1,2,2,3,3,3,4,1,1,2,2], range(11))

def f1(xs):
    if not xs:
        return
    last_a = None # I wish I don't have to use None here.
    is_yield = False
    for a, b in xs:
        if a != last_a:
            last_a = a
            is_yield = True
            yield (a, b)
        else:
            is_yield = False
    if not is_yield:
        yield (a, b) # Ugly...in C# a, b is not defined here.

print list(f1(xs))
print list(f1([]))

What's a better (functional or nonfunctional) way of doing this?

The itertools library has a handy function for this problem called groupby:

def f1(xs):
    for _, group_iter in itertools.groupby(xs, key = lambda pair: pair[0]):
        group = list(group_iter)
        yield group[0]

That'll get us the pairs for which the first element changed from the previous one. Now we just need to make sure we're yield-ing the last element. Which will only NOT be yielded if the last group has size more than 1, hence:

def f1(xs):
    for _, group_iter in itertools.groupby(xs, key = lambda pair: pair[0]):
        group = list(group_iter)
        yield group[0]

    # make sure we yield xs[-1]
    if len(group) > 1:
        yield group[-1]


Laravel controller for form validation skinny I'm trying to make controllers skinny as possible. I use repositories for accessing the database and I use Services to do other stuff. In this case, I use it to insert a new post to the database. For validation I use Jeffery Way's validation package for Laravel.

PostController.php

<?php

use Laracasts\Validation\FormValidationException;

use Dnianas\Post\PostCreationService;
use Dnianas\Post\PostRepository;

use Dnianas\Forms\PostForm;

class PostController extends BaseController
{

    /*
     * Dnianas\Services\PostCreationService
     */
    protected $post;

    /*
    * The post repository
     */
    protected $posts;

    /**
     * @param PostForm $postForm
     * @param PostCreationService $post
     * @param PostRepository $postRepo
     */
    public function __construct(PostForm $postForm, PostCreationService $post, PostRepository $postRepo)
    {
        $this->posts = $postRepo;
        $this->postForm = $postForm;
        $this->post = $post;
    }

    public function index()
    {
        $this->beforeFilter('auth');
    }

    /**
     * @return \Illuminate\Http\JsonResponse
     */
    public function create()
    {
        // Get the input
        $input = Input::all();

        // Validate it
        try {
            $this->postForm->validate($input);
        } catch(FormValidationException $e) {
            return Response::json([
                'success' => 'false',
                'message' => 'You didn\'t enter anything, Post cannot be empty.'
            ]);
        }

        // Insert it to the database
        $post = $this->post->create($input, Auth::user()->id);

        // Get the html content from the view
        $html = View::make('posts.post', ['post_id' => $post->id])->render();

        // Return a message along with the html content
        return Response::json([
            'success' => 'true',
            'message' => 'Your post has been successfuly posted!',
            'post_html' => $html
        ]);

    }

PostForm.php

<?php namespace Dnianas\Forms;

use Laracasts\Validation\FormValidator;

class PostForm extends FormValidator
{
    /**
     * Validation rules for when the user creates a post
     * @var array
     */
    protected $rules = [
        'post_content' => 'required'
    ];

}

PostCreationService.php

namespace Dnianas\Post;

class PostCreationService
{

    /**
     * @param $input
     * @param $user_id
     * @return \Post
     */
    public function create($input, $user_id)
    {
        // Validation passed to we insert it to our database
        $post = new \Post;
        $post->post_content = $input['post_content'];
        $post->posted_date  = \Carbon::now();
        $post->user_id      = $user_id;
        $post->save();

        return $post;
    }
}

Is this controller skinny enough? Can it get any skinnier? If so, then how?

Check out Dwight Watson's Validating Trait. I like to use it with:

protected $throwValidationExceptions = true;

Validation of Eloquent models occurs automatically according to the rules defined on the model and if it fails, a ValidatingException is thrown that you can catch further up the application stack. For example, I might put the following exception handler somewhere like app/global/start.php

App::error(function (\Watson\Validating\ValidationException $exception)
{
    Log::error($exception);
    // I prefer 'success' => 'false' to be expressed in the response code
    return Response::json([
        'message' => $exception->getErrors()->first(),
    ], 422); // Unprocessable entity
});

Them my controllers end up looking more like this:

public function create()
{
    $input = Input::all();
    // Validation occurs automatically during object creation
    $post = $this->post->create($input, Auth::user()->id);
    $html = View::make('posts.post', ['post_id' => $post->id])->render();

    // I prefer "success" to be expressed by the 200 OK response code
    return Response::json([
        'message' => 'Your post has been successfully posted!',
        'post_html' => $html
    ]); // not specifying a response code implies 200 OK

}

But it's really just a matter of personal preference at this point. Your code looks good to me.

tl;dr +1 would pull & merge


'Team Split' problem Below is my solution for this 'Team Split' problem.

In summary, the problem is to find relative strength of 2 teams by finding the difference between their total strength (individual strengths can be found using a quadratic expression of form ax2+bx+c).

The solution in Java manages to solve the problem, but I am regularly getting the Time Limit Exceeded issue.

Can anyone offer advice on how to reduce the complexity of this code?

import java.util.*;
import java.io.*;

public class Main {
    public static void main (String args[]) throws IOException{
        try{
            BufferedReader sc = new BufferedReader(new InputStreamReader(System.in));
            int not=Integer.parseInt(sc.readLine());
            int a,b,c,d;
            for(int i=0;i<not;i++){
                String s[]=sc.readLine().trim().split(" ");
                int r=Integer.parseInt(s[0]);
                a=Integer.parseInt(s[1]);
                b=Integer.parseInt(s[2]);
                c=Integer.parseInt(s[3]);
                d=Integer.parseInt(s[4]);
                long S[]=new long [r];
                S[0]=d;
                for(int k=1;k<r;k++){
                    S[k]=((a*S[k-1]*S[k-1])+(b*S[k-1])+c)%1000000;
                    //System.out.println(S[k]);
                }
                Arrays.sort(S);
                long so1=0;
                long so2=0;
                long t;
                for(int p=r-1,q=r-2;p>=0;p=p-2,q=q-2){
                    if(q>=-1){
                        so1=so1+S[p];
                        if(q>=0){
                            so2=so2+S[q];
                        }
                    }
                }
                // System.out.println(so1);
                // System.out.println(so2);
                t=so1-so2;
                if(t<0){
                    t=(-1)*t;
                }
                System.out.println(t);
                //sc.close();
            }
        }catch(NumberFormatException afe){
            System.out.println(afe);
        }
    }
}

Rewafadhabihlgdity

(If you can't tell, that is supposed to say 'readability'). Leaves some things to be desired.

Variable names: All your variable names are three characters or less. This makes it very hard to read your code. Renaming r to playerCount is a start, and S can be called strengths. so1 and so2 should also be renamed, along with t, p and q. Make variables self-documenting by giving them a reasonable name.
Spacing: Dontyoufinditabithardtoreadtext/codethatdoesntcontainanyspacesandappropriatepunctuation? (I'm exagerating just to prove a point). Even though it is a bit easier to read lack of code spacing than lack of word-spacing in written text, I find a line like this hard to read:

S[k]=((a*S[k-1]*S[k-1])+(b*S[k-1])+c)%1000000;

For this particular line, consider rewriting it to this:

int previous = strengths[index - 1];
strengths[index] = (a * previous * previous + b * previous + c) % 1000000;

By using better variable names, adding space, extracting a variable, and removing unnecessary parenthesis that line becomes a lot more readable.

The lack of good variable names and spacing especially made this part of the code really hard for me to understand what you are doing and why:

for(int p=r-1,q=r-2;p>=0;p=p-2,q=q-2){
     if(q>=-1){
         so1=so1+S[p];
         if(q>=0){
             so2=so2+S[q];
         }
     }
}

Speed

Unable to reproduce / Unclear what the issues are. It seems to me that your code is quite fast. Even the input 10000000 5 6 7 8 calculated with reasonable speed IMO.

Extensibility

For testing purposes, to ensure both correct results and fast enough results, it would be useful to extract a method to do the actual calculations, and then your outermost loop could be:

        for (int i = 0; i < count; i++) {
            String s[] = sc.readLine().trim().split(" ");
            int playerCount = Integer.parseInt(s[0]);
            a = Integer.parseInt(s[1]);
            b = Integer.parseInt(s[2]);
            c = Integer.parseInt(s[3]);
            d = Integer.parseInt(s[4]);
            System.out.println(determineTeamDifferences(playerCount, a, b, c, d));
        }


A program that expands a list of maps and chooses a random element This is the essence of chapter 3 of "Clojure for the Brave and True".
This program is about completing the list asym-hobbit-body-parts by adding a given number of elements called "0-<name>" "1-<name>" and so on for every element in asym-hobbit-body-parts that starts with "left" and subsequently choosing a random body part to hit.
This contains code from the book which I rewrote from memory after finishing chapter 3. (And I mixed it with what I learned on the official homepage)

I am very curious what a clean version of this would look like.

(ns clojure-noob.core
  (:gen-class)
  (:require [clojure.string :as str] ))

(def asym-hobbit-body-parts [{:name "head" :size 3}
                             {:name "left-eye" :size 1}
                             {:name "left-ear" :size 1}
                             {:name "mouth" :size 1}
                             {:name "nose" :size 1}
                             {:name "neck" :size 2}
                             {:name "left-shoulder" :size 3}
                             {:name "left-upper-arm" :size 3}
                             {:name "chest" :size 10}
                             {:name "back" :size 10}
                             {:name "left-forearm" :size 3}
                             {:name "abdomen" :size 6}
                             {:name "left-kidney" :size 1}
                             {:name "left-hand" :size 2}
                             {:name "left-knee" :size 2}
                             {:name "left-thigh" :size 4}
                             {:name "left-lower-leg" :size 3}
                             {:name "left-achilles" :size 1}
                             {:name "left-foot" :size 2}])

(defn make-sym-parts [asym-set num]
  (reduce (fn [sink, {:keys [name size] :as body-part}]
           (if (str/starts-with? name "left-")
             (into sink (apply vector body-part
                         (for [i (range num)]
                           {:name (str/replace name #"^left" (str i))
                            :size size})))
             (conj sink body-part)))
           []
           asym-set))

(defn rand-part [parts]
  (let [size-sum (reduce + (map :size parts))
        thresh (rand size-sum)]
    (loop [[current & remaining] parts
           sum (:size current)]
      (if (> sum thresh)
        (:name current)
        (recur remaining (+ sum (:size (first remaining))))))))

(defn -main
  [& arg]
  (println (rand-part (make-sym-parts asym-hobbit-body-parts 3))))

You actually already fixed up the main thing that I was going to comment on: your previous use of def over let. Just to emphasize on why it's important to choose one over the other though:

(defn f []
  (def my-var 5))

(f)

(println my-var)
5  ; It leaked the state of the function and polluted the namespace

Beyond that, this isn't bad code. As I mentioned on SO, I'm a little rusty, so hopefully someone else will go a little more in-depth. I'll mention what I see here though:

This might be a little over-engineering, but I don't like how the asym-hobbit-body-parts definition is repeating the keys :name and :size over and over. This allows typos to slip in, and makes more work for you if you want to add more parts. I'd make a "constructor" that automatically creates the maps from the data:

(defn new-part [part-name part-size]
  {:name part-name, :size part-size})

(def asym-hobbit-body-parts [(new-part "head" 3)
                             (new-part "left-eye" 1)
                             (new-part "left-ear" 1)])

I like this because now there's a function that definitively says what a "part" is defined as, and if you ever want to change what keywords you use, you only need to change new-part now.

This still has duplication though, and could be reduced down with a helper that auto-wraps vector "tuples":

(defn new-parts [& name-size-pairs]
  (mapv #(apply new-part %) name-size-pairs))
  ; or (mapv (partial apply new-part) name-size-pairs))  ; Partially apply "apply"

(def asym-hobbit-body-parts3 (new-parts
                               ["head" 3]
                               ["left-eye" 1]
                               ["left-ear" 1]))

Or, if you want to get rid of the need for the wrapping vectors, you can use partition to cut a list of attributes into 2s:

(defn new-parts [& name-size-pairs]
  (->> name-size-pairs
       (partition 2)  ; Cut the list into pairs of 2
       (mapv #(apply new-part %))))

(def asym-hobbit-body-parts3 (new-parts
                               "head" 3,
                               "left-eye" 1,
                               "left-ear" 1))

It really depends on how you want it to read.

In case you aren't familiar with them, I highly recommend practicing use of -> and ->>. They seem a little complicated at first, but once you understand them, they have the potential to make your code much neater. They allow your code to read more easily as a series of transformations.

I think the reduction in make-sym-parts is too big. When I have a call to reduce with a large reducing function like that, I try to move it out into its own standalone function. That gives that code a name to make it clearer what it's doing, and makes the call to reduce easier to understand. I decided just to make it a local function using letfn. You could also make it a global function using defn; but I don't think that's necessary.

(defn make-sym-parts [asym-set num]
  (letfn [(transform-lefts [sink {:keys [name] :as body-part}]
            (if (str/starts-with? name "left-")
              (into sink (apply vector body-part
                                (for [i (range num)]
                                  (update body-part :name
                                          #(str/replace % #"^left" (str i))))))

              (conj sink body-part)))]

    (reduce transform-lefts [] asym-set)))

I also altered what you're doing in the for comprehension. You had a literal {:name ..., :size size} in there. This isn't good practice though. Pretend in the future you decide to add a new attribute to body parts. Maybe :quality or something. What happens if you forget to update make-sym-parts? Because you're reconstructing the map from scratch, the new body part will be discarded! update is like assoc, but it "updates" a previous value using a function instead of overwriting it like assoc does. The previous value is passed into the function, and the new value becomes whatever the function returns.

This could still be improved a bit by making use of mapcat. Right now, you're using reduce instead of map because in some cases, you need to add multiple entries per existing element. mapcat (basically short for "map then concatenate") can help here. It will automatically flatten the resulting list. This means you can get rid of the sink accumulator:

(defn make-sym-parts [asym-set num]
  (letfn [(transform-lefts [{:keys [name] :as body-part}]
            (if (str/starts-with? name "left-")
              [body-part
               (for [i (range num)]
                 (update body-part :name
                         #(str/replace % #"^left" (str i))))]

              [body-part]))]  ; Note how this requires a wrapping so it can be flattened

    (mapcat transform-lefts asym-set)))  ; Lazy!

The major drawback here is I wouldn't be surprised if this is slower. That likely isn't an issue though.

I was going to suggest you change rand-part to use reduce (I had a whole thing written up and everything!), then I realized that it requires knowledge of the next part in the list. It is still possible to use reduce, but it would require doing something messy like partitioning the list of parts into pairs, then reducing over the pairs. I think what you have here is likely neater.

I will mention though, just in case you didn't know, reduce allows for an early exit using reduced. If you didn't need to use remaining in the loop, that would allow your loop here to be written using reduce.

The only real two noteworthy things in rand-part are

I would use :keys here to deconstruct current. You're using explicit key-access a lot which adds some bulk.
(reduce + can also be written as (apply +. If you look at the definition of +, it and most other binary operators have a var-arg overload to delegates to a reduction. It's not a big deal either way. From what I've seen, (apply + is generally regarded as more idiomatic, but not by much.

Something like:

(defn rand-part [parts]
  (let [size-sum (apply + (map :size parts))
        thresh (rand size-sum)]
    (loop [[{:keys [name size]} & remaining] parts
           sum size]
      (if (> sum thresh)
        name
        (recur remaining
               (+ sum (:size (first remaining))))))))

Good luck and welcome to Clojure!


Is there another way / proper way of  parsing this JSON array ? I have a JSON object which holds some json_array like this: (with org.codehaus.jettison.json.*)

json array:[{"C1":["S1","S2"]},{"C2":["S1","S2"]},{"C3":["S1","S2"]},{"C4":["S1","S2"]}]

I have to get two list,one for keys,another for the elements of each key.I have done like this:

//sample: clusterIdList:[C1, C2, C3, C4] and serviceIdList:[S1, S2, S1, S2, S1, S2, S1, S2]

    //get jsonarray by key = serviceCode
    JSONArray array = incident.getJSONArray("serviceCode");

    List<String> clusterIdList = new ArrayList<String>();
    List<String> serviceIdList = new ArrayList<String>();
    String key="";

    for (int i = 0,length=array.length(); i < length; i++) {
        JSONObject b = array.getJSONObject(i);
        key = b.names().getString(0);
        clusterIdList.add(key);
        JSONArray sublist = b.getJSONArray(key);
        for (int j = 0,sublistLength = sublist.length(); j < sublistLength; j++) {
            serviceIdList.add(sublist.getString(j));
        }
    }

Is this a proper way to do this? or can it be improved? I am trying to improve my code quality and readability.

This can be argued, but I think you can write directly i < array.length(); without noticeable slowdown (if length() does the right thing and is just a getter of a field). Makes the loop slightly more readable, IMHO. Or at least add spaces: int i = 0, length = array.length();

I had a glance at the Jettison site, and couldn't find a JavaDoc page. But if JSONArray implements the Iterable interface, you can use the foreach syntax, even more readable.

I agree with the comment of RoToRa, too.

Otherwise, code looks OK.


Verb conjugator for French I had written with very few PHP skills a conjugator script.
It was not good, so I decided to start it from scratch with a help from a friend. I'm using the code formatter from eclipse.

Now I need your reviews and improvement tips.

I know all my long arrays with the all verbs and many more long arrays aren't a good way,but I have no clue how to put them all in a database and how then to use them.

Now a specific question:

word_stem.php In this file, the script gets the correct word_stem to building in conjugate.php function in the line:

Compiling a conjugated verb

$conjugated_verb = word_stem($infinitiveVerb, $person, $tense, $mood) . ending($person, $tense, $mood, $endingwith, $exceptionmodel, $infinitiveVerb);

They are many exception rules for many verb groups.
That's why I had to writte somethings long and complicated if conditions to find the needed changed mood-tense-person combinations, which should use the correct word_stem.

Excerpt 1 from word_stem()

if (in_array($exceptionmodel->getValue(), [
    ExceptionModel::Eler_Ele,
    ExceptionModel::Eter_Ete
]) && (($mood->getValue() === Mood::Indicatif && $tense->getValue() === Tense::Present && in_array($person->getValue(), [
    Person::FirstPersonSingular,
    Person::SecondPersonSingular,
    Person::ThirdPersonSingular,
    Person::ThirdPersonPlural
]) || $tense->getValue() === Tense::Futur) || ($mood->getValue() === Mood::Subjonctif && $tense->getValue() === Tense::Present && in_array($person->getValue(), [
    Person::FirstPersonSingular,
    Person::SecondPersonSingular,
    Person::ThirdPersonSingular,
    Person::ThirdPersonPlural
])) || ($mood->getValue() === Mood::Conditionnel && $tense->getValue() === Tense::Present) || ($mood->getValue() === Mood::Imperatif && $tense->getValue() === Tense::Present && $person->getValue() === Person::SecondPersonSingular))) {
    $word_stem = substr_replace($word_stem, 'è', - 2, 1);
}

Excerpt 2

if ($exceptionmodel->getValue() === ExceptionModel::VALOIR && (($mood->getValue() === Mood::Indicatif && $tense->getValue() === Tense::Present && in_array($person->getValue(), [
    Person::FirstPersonSingular,
    Person::SecondPersonSingular,
    Person::ThirdPersonSingular
]) || $tense->getValue() === Tense::Futur) || ($mood->getValue() === Mood::Conditionnel && $tense->getValue() === Tense::Present) || (($mood->getValue() === Mood::Imperatif && $tense->getValue() === Tense::Present && $person->getValue() === Person::SecondPersonSingular)))) {
    $word_stem = word_stem_length($infinitiveVerb, 4) . 'u';
}
if ($exceptionmodel->getValue() === ExceptionModel::VALOIR && ($mood->getValue() === Mood::Subjonctif && $tense->getValue() === Tense::Present && in_array($person->getValue(), [
    Person::FirstPersonSingular,
    Person::SecondPersonSingular,
    Person::ThirdPersonSingular,
    Person::ThirdPersonPlural
]))) {
    $word_stem = word_stem_length($infinitiveVerb, 4) . 'ill';
}

Sometimes I need in one if condition all persons from a tense in a mood and sometimes only some parts froma tense in a mood.

So looks for example a exception array for one Person in one time and mood.

$endings[Mood::Indicatif][Tense::Present][Person::FirstPersonSingular]

Feel free, to have a look at the other files and give me please tips.
You can also become a constributor in this project.

I will use the script for my german speaking learning website.

Wrap conditions to be visible

The first order of business when having long if conditions is to wrap lines at each and every && or ||, and in general to wrap the lines
so that you are able to see the conditions without scrolling. If you have to scroll, you'll loose context straight away. This will render your code into the following:

if ($exceptionmodel->getValue() === ExceptionModel::VALOIR
    && (  ($mood->getValue() === Mood::Indicatif
           && $tense->getValue() === Tense::Present
           && in_array($person->getValue(),
                     [ Person::FirstPersonSingular,
                       Person::SecondPersonSingular,
                       Person::ThirdPersonSingular
                     ]))
        || $tense->getValue() === Tense::Futur
        || ($mood->getValue() === Mood::Conditionnel
            && $tense->getValue() === Tense::Present)
        || ($mood->getValue() === Mood::Imperatif
            && $tense->getValue() === Tense::Present
            && $person->getValue() === Person::SecondPersonSingular
           )
       )
   )
{
    $word_stem = word_stem_length($infinitiveVerb, 4) . 'u';
}

if ($exceptionmodel->getValue() === ExceptionModel::VALOIR
   && ($mood->getValue() === Mood::Subjonctif
       && $tense->getValue() === Tense::Present
       && in_array($person->getValue(),
                   [
                     Person::FirstPersonSingular,
                     Person::SecondPersonSingular,
                     Person::ThirdPersonSingular,
                     Person::ThirdPersonPlural
                   ])
      )
   )
{
    $word_stem = word_stem_length($infinitiveVerb, 4) . 'ill';
}

In addition to making them all visible, I've indented the different blocks as best I can. Possible bug: Do you have an parenthesis issue in the first if block after the in_array() where there seem to be missing an parenthesis? I've changed it here, but you need to reiterate it, to verify its correctness. In general if you wrap and get alternating && vs || you need to take care and verify correctness regarding ordering and grouping of conditionals.

Also note how I moved the starting brace, {, to the next line to keep it aligned with both the if and the ending brace, }. This also gives a much needed vertical space to break between conditions and the statement to be executed.

I've put the ending parenthesis, ), on a separate line as well, this comes somewhat down to personal taste, so do that if that pleases you. Or pack them together on the previous line. Just be consistent, and when dealing with long lines like in this block, try avoid having more than one condition on any line.

Precompute condition parts

Another good option can be to precompute the parts used in the condition. This has at least the following advantages:

Shorten the condition statement in general
Make it read more easily, as you clearly indicate the test condition
Avoid computing values more than once, i.e. $tense->getValue()
Simplify grouping and ordering of multiple conditions
Reuse of condition part in following if statements

Doing this you can end up with the following:

// Precompute condition parts
$exceptionIsValoir = $exceptionmodel->getValue() === ExceptionModel::VALOIR;

$moodVal =  $mood->getValue();
$moodIsIndicatif = $moodVal == Mood::Indicatif;
$moodIsConditionnel = $moodVal === Mood::Conditionnel;
$moodIsSubjonctif =  $moodVal === Mood::Subjonctif;

$tenseVal = $tense->getValue();
$tenseIsPresent = $tenseVal=== Tense::Present;
$tenseIsFutur = $tenseVal === Tense::Futur;

$personVal = $person->getValue();
$personIs_2S = $personVal == Person::SecondPersonSingular;
$personIs_1S_2S_3S = in_array($personVal, [
    Person::FirstPersonSingular,
    Person::SecondPersonSingular,
    Person::ThirdPersonSingular
  ]);
$personIs_1S_2S_3S_3P = in_array($personVal, [
    Person::FirstPersonSingular,
    Person::SecondPersonSingular,
    Person::ThirdPersonSingular,
    Person::ThirdPersonPlural
  ]);

// The actual validations
if ($exceptionIsValoir
    && (   ($moodIsIndicatif && $tenseIsPresent && $personIs_1S_2S_3S)
        || $tenseIsFutur
        || ($moodIsConditionnel && $tenseIsPresent)
        || ($moodIsImperatif && $tenseIsPresent && $personIs_2S)))
{
    $word_stem = word_stem_length($infinitiveVerb, 4) . 'u';
}

if ($exceptionIsValoir
    && ($moodIsSubjonctif && $tenseIsPresent && $personIs_1S_2S_3S_3P))
{
    $word_stem = word_stem_length($infinitiveVerb, 4) . 'ill';
}

Early returns to reduce nesting levels

The concept of early returns to reduce nesting levels, is that if you have a structure like the following with no else block on the first few levels:

function something() {
    if first_level_condition {
        if second_level_condition {
            if third_level_condition and something {
                do something useful
            } else {
                do something else
            }
        }  // Note missing else block on second level
     }  // Note missing else block on first level
}

Then you can simplify this to:

function something() {
    if not first_level_condition
       or not second_level_condition {
        return
    }

    if third_level_condition and something {
        do something useful
    } else {
        do something else
    }
}

Now you've moved your code of interest, that is level three, out two indentation levels, which gives you some more room to write code in.

At first glance of your example, it seemed like you could do this in your code as the two original cases both had the common element of $exceptionmodel->getValue() === ExceptionModel::VALOIR, which could have been negated, and caused an early return. However in your added excerpt, and in the original code many more variants exists making this not so useful in your context.

It is still a useful pattern to be aware of, when doing multiple if statements and nesting of these.

(Optionally) Build condition groups

Another option which I've used sparingly is to actually precompute the entire groups of condition statements, using constructions like the following:

// Precompute condition parts
... as before ...

// Build condition group
$c = ($moodIsIndicatif && $tenseIsPresent && $personIs_1P_2P_3P)
$c = $c || $tenseIsFutur
$c = $c || ($moodIsConditionnel && $tenseIsPresent)
$c = $c || ($moodIsImperatif && $tenseIsPresent && $personIs_2S)

if ($exceptionIsValoir && $c)
{
   $word_stem = word_stem_length($infinitiveVerb, 4) . 'u';
}
...

This construct can be useful in some circumstances, but in your case I believe I would stop at precomputing the condition part using good condensed variable names, and do an early return if possible.

Addendum 1: Automating generation of condition variables

You ask if one can automate stuff like $exceptionIsValoir or $tenseIsPresent, and as I'm a bit rusty I php I posted my version as a question here: Dynamic variables in Php from enum. But it is possible to make dynamic variables in PHP, and one way of doing it is:

$exceptionVal = $exceptionmodel->getValue();
foreach(ExceptionModel::getConstants() as $constName => $constValue) {
    ${'exceptionIs' . $constName} = $exceptionVal === $constValue;
}

$tenseVal = $tense->getValue();
foreach (Tense::getConstants() as $constName => $constValue) {
    ${'tenseIs' . $constName} = $tenseVal === $constValue;
}

This would allow you to use $exceptionIsALLOIR or $tenseIsPresent. You could optionally change the first one to use $exception_is_ALLOIR by changing the constant string prefix.

With regards to the use of in_array() within the if conditions, I'm no personal fan of it. This is partly because it is lengthy and kind of confusing as you prefix the enums with full class name. I would rather precompute these manually, like in $personIs_1S_2S_3S, as this can shorten the text compared to the rather lengthy version of $personIsFirstPersonSingular || $personIsSecondPersonSingular || $personIsThirdPersonSingular.

Regarding precomputing combinations of ExceptionModel, I would do it on a case by case basis. Possibly I there would use the or'ed combination as I don't see a neat way of shortening the names as is easily done for the Person enum.

Addendum 2: Reconsider general concept of word_stem()

When following your links to the original code I see that word_stem() is a rather long function consisting of above 40 disconnected if statements similar to the two above (or four in modified post), spanning over 300 lines. That is a heavy function, and raise some concerns:

Can it be simplified by using other functions?
Can it be simplified by connecting together the if's, so that you don't need to execute all of them each and every time?
How do you test such a massive beast if it is correct at all?
If you match on one of the if's, does that exclude all others? In other words, can you do return $word_stem directly instead of at end?

Without looking into the functions too much, I would consider if you should make functions like word_stem_Exer_Exe and word_stem_xER and word_stem_ENVOYER, and let the code in word_stem() be:

if ($exceptionIs_ELER_ELE_or_ETER_ETE) {
    $word_stem =  word_stem_Exer_Exe($person, $tense, $mood);
}
if ($exceptionIsCER or $exceptionIsGER or $exceptionIsE_Akut_CER
    or $exceptionIsE_Akut_GER)
{
    $word_stem =  word_stem_xER($person, $tense, $mood);
}
if ($exceptionIsENVOYER) {
    $word_stem = word_stem_ENVOYER($person, $tense, $mood);
}

Or possibly use if ... else if ... else if which would stop evaluating the if sequences at first hit. In other words, this would require that only one of the 40++ if's matches. Using return directly would also eliminate evaluating further if's effectively.

Using functions like word_stem_xxxx() would also ease the testing some, as you could specific test that exception group. With these functions you would then move the person, tense and mood tests inside of the function, and then they could possibly be clearer in general even without that much shortening. You could then possibly only use the first tip of $tenseVal = $tense->getValue() to shorten them sufficiently.

Disclaimer: I don't know the entire context, and haven't thoroughly read the accompanying code, so consider this carefully before reimplementing into sub-functions or else-if's as that might have side effects not known to me. Then again, you might have a lot of side effects already in the massive beast that word_stem() is.

Addendum 3: Automagical is<ENUM_VALUE>() methods

An alternative approach to creating the temporary variables, is to use reflection and automatically define test function for equality to any given enum value.

If extending the base Enum class, or the ExtensionModel class with the following function:

function __call($func, $param) {
    $func_prefix = substr($func, 0, 2);
    $func_const = substr($func, 2);
    if ($func_prefix == "is") {
        $reflection = new ReflectionClass(get_class($this));
        return $this->getValue() === $reflection->getConstant($func_const);
    }
}

Then it is legal to do stuff like in the following test function:

function myFunction(ExceptionModel $exceptionModel, Tense $tense) {

    if ($exceptionModel->isALLER()
        && $tense->isPresent() ) {
        ... do something ...
    }

given that both ExceptionModel and Tense inherits from the Enum class. In other words, now you can do $enumobject->is<ENUM_VALUE>() for any enum value inheriting from Enum.

  PS: After posting my original answer you added two more excerpts. I've not included those in my analysis, but the concept will be the same only with a few more lines added.


Allowing a robot to drive around Background:

I'm a member of my high school's robotics club (and in charge of the
programming team).  The robot is going to be written in C++, but I'm mostly
a Python coder and want to make sure I don't have any fundamental gaps in my
knowledge.

In preparation for the start of the competition in just a few days, I've
quickly written up the bare minimum required for a robot to drive around
(included below).  I was hoping to get feedback and criticism at this stage
to nip any problems/misconceptions I have before they get serious.

As previously stated, I'm unfamiliar with C++ so don't know many of its best
practices/idioms/etc.  What I'm looking for is a bit vague, but in essence,
how can I make my code more professional/better?  Am I making any errors?

Any feedback would be welcome.

There are three files:

MainRobot.h
MainRobot.cpp
typedefs.h

MainRobot.h

#ifndef MAINROBOT_H_
#define MAINROBOT_H_

// 3rd-party modules
#include "WPILib.h"

// Modules created by us
#include "typedefs.h"

class MainRobot : public SimpleRobot
{
public:
    // Required methods (for robot to run)
    MainRobot(void);
    void Autonomous(void);
    void OperatorControl(void);

    // Objects (provided by WPILib.h)
    RobotDrive robotDrive;
    Joystick *leftStick;
    Joystick *rightStick;
    Timer timer;

    // Motor ports (hardware)
    static const UINT32 kLeftFrontMotor = kPWMPort_1;
    static const UINT32 kRightFrontMotor = kPWMPort_2;
    static const UINT32 kLeftBackMotor = kPWMPort_3;
    static const UINT32 kRightBackMotor = kPWMPort_4;

    // Joystick ports (hardware)
    static const UINT32 kRightJoystickPort = kUSBPort_1;
    static const UINT32 kLeftJoystickPort = kUSBPort_2;

    // Delay to prevent spamming motors with input
    static const float kDelayValue = 0.01;
};

#endif /*MAINROBOT_H_*/

MainRobot.cpp

#include "MainRobot.h"

/**
 * The constructor.
 *
 * Initializes the motors, drivetrain, joysticks, safety devices, etc.
 */
MainRobot::MainRobot(void):
    robotDrive(kLeftFrontMotor,
               kRightFrontMotor,
               kLeftBackMotor,
               kRightBackMotor)
{
    MainRobot::GetWatchdog().SetExpiration(0.1);    // In seconds

    MainRobot::leftStick = new Joystick(kLeftJoystickPort);
    MainRobot::rightStick = new Joystick(kRightJoystickPort);

    return;
}

/**
 * Initializes the autonomous period when called
 * (robot performs a task without human intervention)
 *
 * Currently left blank.
 */
void MainRobot::Autonomous(void) {
    while(MainRobot::IsAutonomous()) {
        MainRobot::GetWatchdog().Feed();
        Wait(kDelayValue);
        // Do nothing.
    }
    return;
}

/**
 * Initializes the teleoperated period when called.
 * (robot is driven by humans using two joysticks)
 */
void MainRobot::OperatorControl(void) {
    MainRobot::GetWatchdog().SetEnabled(true);

    MainRobot::timer.Reset();
    MainRobot::timer.Start();

    while(MainRobot::IsOperatorControl()) {
        MainRobot::robotDrive.TankDrive(MainRobot::leftStick,
                                        MainRobot::rightStick);
        MainRobot::GetWatchdog().Feed();
        Wait(kDelayValue);
    }
    return;
}

typedefs.h

#ifndef TYPE_DEFS_H_
#define TYPE_DEFS_H_

// The available ports for any motors
typedef enum {
    kPWMPort_1 = 1,
    kPWMPort_2 = 2,
    kPWMPort_3 = 3,
    kPWMPort_4 = 4,
    kPWMPort_5 = 5,
    kPWMPort_6 = 6,
    kPWMPort_7 = 7,
    kPWMPort_8 = 8,
    kPWMPort_9 = 9,
    kPWMPort_10 = 10
} PWMPorts;

// The available ports for USB drives (for joysticks, etc)
typedef enum {
    kUSBPort_1 = 1,
    kUSBPort_2 = 2,
    kUSBPort_3 = 3,
    kUSBPort_4 = 4
} USBPorts;

#endif

Do you plan to replace the Joystick object?

Joystick *leftStick;
Joystick *rightStick;

If not then declare them as

Joystick  leftStick;
Joystick  rightStick;

If you are going to replace them then you should be using smart pointers rather than a RAW pointer (if you want to do it manually then learn the rule of three) But I would go with smart pointers (but that is a second option to normal objects).

If you must have pointer use smart pointers:

std::unique_ptr<Joystick>  leftStick;
std::unique_ptr<Joystick>  rightStick;

There is no need declare these as static (unless you want to get the address or something) so just use enum:

// Motor ports (hardware)
static const UINT32 kLeftFrontMotor = kPWMPort_1;
static const UINT32 kRightFrontMotor = kPWMPort_2;
static const UINT32 kLeftBackMotor = kPWMPort_3;
static const UINT32 kRightBackMotor = kPWMPort_4;

Prefer:

enum { kLeftFrontMotor = kPWMPort_1, kRightFrontMotor = kPWMPort_2, kLeftBackMotor = kPWMPort_3, kRightBackMotor = kPWMPort_4};

No need to explicitly return from a constructor:

MainRobot::MainRobot(void)
{
    ...

    return;  // No need
}

There is nothing wrong with prefixing functions with the class name but personally I would not.

while(MainRobot::IsAutonomous()) {
    MainRobot::GetWatchdog().Feed();
    Wait(kDelayValue);
    // Do nothing.
}

Easier to write:

while(IsAutonomous())
{
    GetWatchdog().Feed();
    Wait(kDelayValue);
    // Do nothing.
}

The file name "typedefs.h" is relatively common. You may want to avoid it.


Utility to cache Google Maps I wrote a little wrapper for Google Static Maps API so that I do not pull down from Google Maps if I have already retrieved the address. The below code works, but I wrote it in a quick fashion - not really paying attention to the best practices of golang.

Several questions I have about this code:
 - How can I improve the v := url.Values{} section?
 - Is there a better method to the for i := 0; i < 30; i++ loop?

package main

import (
    "crypto/md5"
    "encoding/hex"
    "errors"
    "fmt"
    "io/ioutil"
    "net/http"
    "net/url"
    "os"
)

var apiKey = os.Getenv("API_KEY")

// loadMap generates a new map or returns a cached one
func loadMap(address string, title string) ([]byte, error) {
    mapHost := "http://maps.googleapis.com/maps/api/staticmap"

    v := url.Values{}
    v.Add("center", address)
    v.Add("zoom", "15")
    v.Add("scale", "2")
    v.Add("size", "400x350")
    v.Add("markers", address)
    v.Add("sensor", "false")
    v.Add("key", apiKey)
    mapURL := mapHost + "?" + v.Encode()

    filename := title + ".png"
    for i := 0; i < 30; i++ {
        body, err := ioutil.ReadFile(filename)
        if err == nil {
            return body, nil
        }

        if i == 0 {
            fmt.Println("Getting map for " + address + " and saving it to " + filename)
            fmt.Println("Map URL: " + mapURL)
            httpClientGetMap, _ := http.Get(mapURL)
            body, _ := ioutil.ReadAll(httpClientGetMap.Body)
            ioutil.WriteFile(filename, []byte(body), 0600)
        }

        i++
    }

    return nil, errors.New("Failed to get an image")
}

// viewHandler returns the map image
func viewHandler(w http.ResponseWriter, r *http.Request) {

    address := r.URL.Query().Get("address")

    md5 := md5.Sum([]byte(address))
    title := hex.EncodeToString(md5[:])
    p, err := loadMap(address, title)

    if err != nil {
        fmt.Println(err)
        w.WriteHeader(http.StatusGatewayTimeout)
        return
    }

    w.Header().Set("content-type", "image/png")
    w.Write(p)
}

func main() {
    http.HandleFunc("/getmap", viewHandler)

    http.ListenAndServe(":8080", nil)
}

Improve the for loop

The loadMap method is really hard to read, and is not verry efficient:

Currently, if a file doesn't exists, here are the steps followed by the method:

try to read the content of a file to see if it exists
send a request to google map, and write the response to a file
read the content of the new file

but we could merge just return the response instead of reading the file twice !
Also, it would be better to have a map for keeping records of saved map.
To do this, we create a server that implement the ServerHTTP method:

  type server struct {
    mux        *http.ServeMux
    fetchedMap map[string]string
  }

  func (s *server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
      s.mux.ServeHTTP(w, r)
  }

Now, let's rewritte the loadMap method:

  func (s *server) loadMap(address string, title string) ([]byte, error) {

    if filename, ok := s.fetchedMap[title]; ok {
        return ioutil.ReadFile(filename)
    }
    mapHost := "http://maps.googleapis.com/maps/api/staticmap"

    v := url.Values{}
    v.Add("center", address)
    v.Add("zoom", "15")
    v.Add("scale", "2")
    v.Add("size", "400x350")
    v.Add("markers", address)
    v.Add("sensor", "false")
    v.Add("key", apiKey)
    mapURL := mapHost + "?" + v.Encode()

    fmt.Println("Getting map for " + address + " and saving it to " + filename)
    fmt.Println("Map URL: " + mapURL)
    httpClientGetMap, _ := http.Get(mapURL)
    body, _ := ioutil.ReadAll(httpClientGetMap.Body)
    ioutil.WriteFile(filename, []byte(body), 0600)
    return body, nil
  }

The steps are now:

check if requested address has already been fetched, and if yes, return file content
if no fetch map from google map and write it to a file
return the map

General advices

use constant for mapHost and other constant parameters
use fmt.Printf("%v", str) instead of fmt.Println(" " + str) because it's easier to read
Always check the error ! Silently discarding errors is really dangerous
avoid useless conversion: body is already a slice of byte

Here is the new version of the code:

package main

import (
    "crypto/md5"
    "encoding/hex"
    "fmt"
    "io/ioutil"
    "net/http"
    "net/url"
    "os"
)

var apiKey = os.Getenv("API_KEY")

const (
    baseURL = "http://maps.googleapis.com/maps/api/staticmap"
    zoom    = "15"
    scale   = "2"
    size    = "400x350"
)

func (s *server) loadMap(address string, title string) ([]byte, error) {

    if filename, ok := s.fetchedMap[title]; ok {
        return ioutil.ReadFile(filename)
    }
    v := url.Values{
        "center":  {address},
        "zoom":    {zoom},
        "scale":   {scale},
        "size":    {size},
        "markers": {address},
        "sensor":  {"false"},
        "key":     {apiKey},
    }
    mapURL := baseURL + "?" + v.Encode()
    filename := title + ".png"

    fmt.Printf("Getting map for adress %s and saving it to %s\nMap URL: %s\n", address, filename, mapURL)

    httpClientGetMap, err := http.Get(mapURL)
    if err != nil {
        return nil, err
    }
    body, err := ioutil.ReadAll(httpClientGetMap.Body)
    if err != nil {
        return nil, err
    }
    err = ioutil.WriteFile(filename, body, 0600)
    if err != nil {
        return nil, err
    }
    s.fetchedMap[title] = filename
    return body, nil
}

func (s *server) viewHandler(w http.ResponseWriter, r *http.Request) {

    address := r.URL.Query().Get("address")

    checksum := md5.Sum([]byte(address))
    title := hex.EncodeToString(checksum[:])
    mapByte, err := s.loadMap(address, title)
    if err != nil {
        fmt.Println(err)
        w.WriteHeader(http.StatusGatewayTimeout)
        return
    }
    w.Header().Set("content-type", "image/png")
    w.Write(mapByte)
}

type server struct {
    mux        *http.ServeMux
    fetchedMap map[string]string
}

func (s *server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
    s.mux.ServeHTTP(w, r)
}

func main() {

    s := &server{
        mux:        http.DefaultServeMux,
        fetchedMap: map[string]string{},
    }
    s.mux.HandleFunc("/getmap", s.viewHandler)
    http.ListenAndServe(":8080", s)
}


Basic calculator in Java I'm new to programming and just made a basic calculator on my own.

Can anyone see if there is any room for improvement in the code, like stuff i should do different.

package calculator;

import java.util.Scanner;

public class Main {

static Scanner userInput = new Scanner(System.in);

public static void main(String[] args) {
    float numOne;
    float numTwo;
    float answer = 0;
    String option;
    boolean loop = true;

    while(loop==true){
    System.out.print("Addition, Subtraction, Multiplication or Division? (WRITE ONLY IN LOWERCASE!) \n Type:");
    option = userInput.next();
    if(option.equals("addition")){
        System.out.print("Enter your first number: ");
        numOne = userInput.nextFloat();
        System.out.print("Enter your second number: ");
        numTwo = userInput.nextFloat();
        answer = addition(numOne, numTwo);
        getAnswer(answer);
    }else if(option.equals("subtraction")){
        System.out.print("Enter your first number: ");
        numOne = userInput.nextFloat();
        System.out.print("Enter your second number: ");
        numTwo = userInput.nextFloat();
        answer = subtraction(numOne, numTwo);
        getAnswer(answer);
    }else if(option.equals("multiplication")){
        System.out.print("Enter your first number: ");
        numOne = userInput.nextFloat();
        System.out.print("Enter your second number: ");
        numTwo = userInput.nextFloat();
        answer = multiplication(numOne, numTwo);
        getAnswer(answer);
    }else if(option.equals("division")){
        System.out.print("Enter your first number: ");
        numOne = userInput.nextFloat();
        System.out.print("Enter your second number: ");
        numTwo = userInput.nextFloat();
        answer = division(numOne, numTwo);
        getAnswer(answer);
    }else{
        System.out.println("Error! unvalid option chosen!");
    }
    System.out.println("Do you whish to quit? (if so, do /quit but if not, type /continue)\ntype: ");
    option = userInput.next();

    if(option.equals("/quit")){
        loop = false;
    }

    }

    }

static float addition(float numX, float numY){
    float finalNum;
    finalNum = numX + numY;
    return finalNum;
}

static float subtraction(float numOne, float numTwo){
    float finalNum;
    finalNum = numOne - numTwo;
    return finalNum;
}

static float multiplication(float numOne, float numTwo){
    float finalNum;
    finalNum = numOne * numTwo;
    return finalNum;
}

static float division(float numOne, float numTwo){
    float finalNum;
    finalNum = numOne / numTwo;
    return finalNum;
}

static void getAnswer(float finalAnswer){
    System.out.println("The answer is " + finalAnswer);
}

}

You really don't need:

    System.out.print("Enter your first number: ");
    numOne = userInput.nextFloat();
    System.out.print("Enter your second number: ");
    numTwo = userInput.nextFloat();

four times. Just get the function first and then the two numbers independent of which function was chosen.

Also, do you really need to write functions for simple mathematical operations?

Finally, you don't need getAnswer(answer); four times either.


Scrolling elements into view On scroll, I am adding a class (current) to particular elements.

The code looks like it can be made better. For example, it seems like I am reusing the same things over and over again (i.e. the nth-child(1), nth-child(2), etc.).

I appreciate any and all comments and advice.

 function isScrolledIntoView(elem) {
    if (elem.length > 0) {
      var docViewTop = $(window).scrollTop();
      var docViewBottom = docViewTop + $(window).height();
      var elemTop = $(elem).offset().top;
      return ((elemTop <= docViewBottom) && (elemTop >= docViewTop));
    }
  }

   $(document).scroll(function () { // Measure scroll To Top
      // @TODO Fix it so that there is only 1 if block
      if (isScrolledIntoView($('.section .main-content:nth-child(1)')) === true) {
        $('.events-widget-menu li').removeClass('current');
        $('.events-widget-menu li:nth-child(1)').addClass('current');
      }

      if (isScrolledIntoView($('.section .main-content:nth-child(2)')) === true) {
        $('.events-widget-menu li').removeClass('current');
        $('.events-widget-menu li:nth-child(2)').addClass('current');
      }

      if (isScrolledIntoView($('.section .main-content:nth-child(3)')) === true) {
        $('.events-widget-menu li').removeClass('current');
        $('.events-widget-menu li:nth-child(3)').addClass('current');
      }

      if (isScrolledIntoView($('.section .main-content:nth-child(4)')) === true) {
        $('.events-widget-menu li').removeClass('current');
        $('.events-widget-menu li:nth-child(4)').addClass('current');
      }
    });

I don't understand why :nth-child selector is even in play here.  It seems all you are wanting to do is add a class to an li with same index within its parent as what the content's index within it's parent is.

I might suggest something along these lines to achieve that:

// Cache jQuery collections somewhere in scope that `scroll()` inherits.
// This prevents you from having to continually re-query the DOM every time
// there is a scroll event.
var $mainContent = $('.section .main-content');
var $eventLi = $('.events-widget-menu li');

$(document).scroll(function () {
    $eventLi.removeClass('current');
    // find content elements in view
    var $currentElements = $mainContent.filter(function(index) {
        return isScrolledIntoView(this);
    });
    // apply class to LI with corresponding index
    $currentElements.each(function() {
        var index = $mainContent.index(this);
        $eventLi.eq(index).addClass('current');
    });
}

This solution is not hard-coded to X number of elements.


Top 10 editors partitioned by year I wanted to get some stats on who has done the most edits on Code Review, so I wrote the following query on SE Data Explorer. All improvement suggestions are welcome.

Note that I used a cursor to partition top users by year because at first there was a fundamental flaw in my logic trying to get the top users overall and then grouping them by years, which gave incorrect results.

The execution plan doesn't look too bad, considering the cursor only iterates a total of 6 times (2011-2016), but if it can be improved without degrading readability too much, I'd like to know.

/*
  Aggregate the top 10 users with the most post edits
  for each year, along with relevant statistics.
*/

/* Filter for Edit events: */
declare @EditPostHistoryTypes table (Id int);
insert into @EditPostHistoryTypes
    select Id
    from PostHistoryTypes
    where Name like 'Edit%';

/* All edits on record: */
declare @CountAllEdits int = (
    select count(Id)
    from PostHistory
    where
      PostHistoryTypeId in (select Id from @EditPostHistoryTypes)
      and PostId in (
          select Id from Posts
          where DeletionDate is null
      )
);
/* Hold top editors per year for main query: */
declare @TopEditorsPerYear table (
    [Year] int,
    UserId int,
    CountByYear int,
    TotalCount int
);
/* Get all years with edit activity: */
declare _years cursor for
    select distinct datepart(year, CreationDate)
    from PostHistory
    where PostHistoryTypeId in (
        select Id
        from PostHistoryTypes
        where Name like 'Edit%'
    );
declare @currentYear int;
/* Cursor to split top editors into yearly partitions: */
open _years;
fetch next from _years into @currentYear;
while @@fetch_status = 0
begin;
    insert into @TopEditorsPerYear ([Year], UserId, CountByYear, TotalCount)
        select top 10
            datepart(year, ph.CreationDate) as [Year],
            ph.UserId,
            count(ph.Id),
            (select count(Id) from PostHistory as phInline where phInline.UserId = ph.UserId)
        from PostHistory as ph
        where PostHistoryTypeId in (
            select Id
            from PostHistoryTypes
            where Name like 'Edit%'
        )
        and datepart(year, CreationDate) = @currentYear
        and UserId is not null
        group by
            datepart(year, CreationDate),
            UserId
        order by count(Id) desc;
    fetch next from _years into @currentYear;
end;
/* Cleanup cursor: */
close _years;
deallocate _years;

/* Main query: */
select
    [Year Rank] = row_number() over (
        partition by tops.[Year]
        order by tops.CountByYear desc
    ),
    [Year] = tops.[Year],
    [User Link] = hist.UserId,
    [Annual Total] = tops.CountByYear,
    [% Year/User] = cast((cast(tops.CountByYear as decimal(10,2)) / tops.TotalCount) * 100 as decimal(5,2)),
    [% Year/Site] = cast((cast(tops.CountByYear as decimal(10,2)) / @CountAllEdits) * 100 as decimal(5,2)),
    [% Total/Site] = cast((cast(tops.TotalCount as decimal(10,2)) / @CountAllEdits) * 100 as decimal(5,2)),
    [Grand Total] = tops.TotalCount
from
    PostHistory as hist
    join @TopEditorsPerYear as tops on hist.UserId = tops.UserId
where
    PostHistoryTypeId in (select Id from @EditPostHistoryTypes)
    and PostId in (select Id from Posts where DeletionDate is null)
group by
    hist.UserId,
    tops.[Year],
    tops.CountByYear,
    tops.TotalCount
order by
    [Year Rank] asc,
    [Year] asc;

This can be simplified down to a single query :-)

You calculate COUNTs using different WHERE-conditions and aggregation levels, but the base tables are always the same:

CountByYear: Edits per user & year
TotalCount: All actions of a user
CountAllEdits: All Edits of all users (including UserId NULL)

In a case like this you can utilize conditional counts (COUNT(CASE...)) and Windowed Aggregate Functions (SUM() OVER).

To be able to calculate both Edits only and All actions I use a LEFT JOIN PostHistoryTypes ON Name like 'Edit%' which returns NULL for non-edit-actions. This enables both counts using:

count(PostHistory.PostHistoryTypeId) = all actions
count(PostHistoryTypes.id)           = edits only

Aggregation starts with the lowest level, GROUP BY UserId, YEAR. And because OVER is processed after  aggregation you get the higher aggregation levels using:

sum(count(...)) over (partition by UserId) = -- count by user
sum(count(...)) over ()                    = -- count all

This results in this base query:

   select
      [Year Rank] = row_number()
                    over (partition by datepart(year, CreationDate)
                                         -- (excluding NULL UserId)
                          order by count(case when UserId is not null then PHT.Id end) desc),
      [Year] = datepart(year, CreationDate),
      UserId,
      CountByYear = count(pht.id), -- Edits per user & year
      TotalCount = sum(count(UserId)) over (partition by UserId), -- All actions of a user
      CountAllEdits = sum(count(pht.Id)) over () -- All Edits (including NULL UserId)
   from
       PostHistory as ph
   left join PostHistoryTypes as pht
     on ph.PostHistoryTypeId = pht.Id
    and pht.Name like 'Edit%'
   where PostId in (select Id from Posts where DeletionDate is null)
   group by
       [UserId],
       datepart(year, CreationDate)

And then you just have to filter for the top 10 and do the percentage calculations:

/*
  Aggregate the top 10 users with the most post edits
  for each year, along with relevant statistics.
*/

with cte as
 (
   select
      [Year Rank] = row_number()
                    over (partition by datepart(year, CreationDate)
                                         -- (excluding NULL UserId)
                          order by count(case when UserId is not null then PHT.Id end) desc),
      [Year] = datepart(year, CreationDate),
      UserId,
      CountByYear = count(pht.id), -- Edits per user & year
      TotalCount = sum(count(UserId)) over (partition by UserId), -- All actions of a user
      CountAllEdits = sum(count(pht.Id)) over () -- All Edits (including NULL UserId)
   from
       PostHistory as ph
   left join PostHistoryTypes as pht
     on ph.PostHistoryTypeId = pht.Id
    and pht.Name like 'Edit%'
   where PostId in (select Id from Posts where DeletionDate is null)
   group by
       [UserId],
       datepart(year, CreationDate)
 )
select
   [Year Rank],
   [Year],
   [User Link] = UserId,

   -- number of edits by user & year
   -- [Edits by User & Year]?
   [Annual Total] = CountByYear,

   -- % of edits vs. all actions by user & year
   [% Year/User]  = cast(100.00 * CountByYear / TotalCount    as decimal(5,2)),

   -- % of edits by user & year vs. all edits
   [% Year/Site]  = cast(100.00 * CountByYear / CountAllEdits as decimal(5,2)),

   -- % of actions by user vs. all edits
   [% Total/Site] = cast(100.00 * TotalCount  / CountAllEdits as decimal(5,2)),

   -- number of all actions by user
   [Grand Total] = TotalCount

from cte
where [Year Rank] <= 10
  and CountByYear > 0
order by
    [Year Rank] asc,
    [Year] asc;
;

I noticed you didn't use exactly the same WHERE-conditions all the time, in your TotalCount calculation you didn't check for DeletionDate is null, but in fact there's no NULL at all in that column (thus it could be removed from the query).

It was hard for me to infer the actual meaning from the column names.
And the calculation of [% Total/Site] doesn't make sense for me, you compare all actions of a user to all edit-actions.

First I would expect indicators like:

edit count of this user (per year & globally)
% edits of this user vs all edits (per year & globally)

And additionally maybe:

all actions count of this user (per year & globally)
% edits vs all actions of this user (per year & globally)


Count number of matching nodes in a Binary tree Problem

Count the number of matching nodes in the subtree rooted at some node n.

Solution

from collections import namedtuple

Node = namedtuple('Node', 'key, left, right')

def build_tree(inorder, preorder):
    """Builds a Binary Tree from given inorder and preorder traversal.
    Steps:
    1. Take the first element from the preorder list.
    2. Get the index of the element from inorder list.
    3. Set the element as root.
    4. Take all elements left to root from inorder list.
    5. Take all elements right to root from inorder list.
    6. Calculate preorder list for left and right subtree respectively.
    7. Repeat from step 1 for each subtree.
    """
    if not preorder:
        return
    if len(preorder) == 1:
        return Node(preorder, None, None)
    root = preorder[0]
    root_index    = inorder.find(root)
    inorder_left  = inorder[:root_index]
    inorder_right = inorder[root_index+1:]
    preorder_left = preorder[1:len(inorder_left)+1]
    preorder_right = preorder[root_index+1:]
    return Node(
        root,
        build_tree(inorder_left, preorder_left),
        build_tree(inorder_right, preorder_right))

def match_count(root, key1, key2):
    """Counts the number of node that matches key1 or key2 in the subtree."""
    if root is None:
        return 0
    count_left = match_count(root.left, key1, key2)
    count_right = match_count(root.right, key1, key2)
    if root.key == key1 or root.key == key2:
        return count_left + count_right + 1
    return count_left + count_right

root = build_tree('DBEACF', 'ABDECF')

import unittest

class TestMatchCount(unittest.TestCase):
    def setUp(self):
        """
                A
               / \
              B   C
             /\    \
            D  E    F
        """
        root = build_tree('DBEACF', 'ABDECF')

    def test_noNodesFound(self):
        self.assertEqual(match_count(root, 'Z', 'Q'), 0)

    def test_nodesOnSameSide(self):
        self.assertEqual(match_count(root, 'D', 'E'), 2)

    def test_nodesOnDiffSide(self):
        self.assertEqual(match_count(root, 'D', 'F'), 2)

    def test_oneNodeIsRoot(self):
        self.assertEqual(match_count(root, 'D', 'A'), 2)

if __name__ == '__main__':
  unittest.main()

Time complexity

Since we check each node at least once hence it should be \$O(n)\$, where n is the number of nodes in the Binary Tree.

Note

while unit testing I felt that I am not able to express intent clearly for my test case unlike Jasmine where we can write proper description in it(...) format.

I'll leave the comments on build_tree() as they stand in my other answer, and focus on your match_count() method:

Bad parameters for method? – It feels a little constructed, and special, to test for two key values. Why not one, or three, or an arbitrary number? Or a range?
Not optimal method name? – To me, it sounds a little strange with match_count. I just think of safety matches... I think I would prefer just count() (or count_matching()). Do note that when using count() we kind of hide/reimplement the default count() of Python. If your binary tree was within a class (as in my test cases) that could work nicely, but outside of classes it should probable be avoided and you should rather use the count_matching or match_count name.
Consider use of local variables – I don't oppose to using local variables like count_left and count, but in this rather simple case you could actually do:

def count(node, key1, key2):
    """Counts the number of node that matches key1 or key2 in the subtree."""
    if node is None:
        return 0

    return ( (node.key == key1 or node.key == key2)
             + match_count(node.left, key1, key2)
             + match_count(node.right, key1, key2) )

Use predicates to generalize method

If you look at the accepted answer of "Most pythonic way of counting matching elements in something iterable", they suggest some neat ways of handling your issue at hand. In fact your match_count() can be replaced with the itertools.quantify recipe.

def quantify(seq, pred=None):
    "Count how many times the predicate is true in the sequence"
    return sum(imap(pred, seq))

You could either use this directly, and reuse any of the order traversals, like in:

def count(node, pred=None):
    """Count how many times the predicate is true in binary tree."""
    return sum(imap(pred, inorder(node)))

Another version would be to use the predicate similar to the first count() version above:

def count(node, pred=None):
    """Count how many times the predicate is true in binary tree."""
    if node is None:
        return 0

    return (not pred or pred(node.key)) + count(node.left, pred) + count(node.right, pred)

Have added a not pred in front of the test to allow for using this method with default parameter of None in which case it ends up counting all elements in binary tree.

This general approach opens up for counting whatever you feel like, but you need to call it slightly different. Here are some examples:

# Two variants to test against two different key values
# Note that the lambda function will receive the node.key value
count(root, lambda x: x == 'D' or x=='E')
count(root, lambda x: x in 'DE')

# or use a designated method (accepting
def is_vowel(char):
    return char in 'AEIOU'
count(root, is_vowel)

Complexity considerations

Time complexity of any of these implementations (including your original one) will be \$O(n)\$. Slightly different memory complexity, but neither algorithm will use much extra memory (with the exception when using a list implementation of inorder() which duplicates the binary tree into a list thusly requiring more memory).