Slicing a big NumPy array I have a function that takes a four-dimensional NumPy array M and, for each value i of its second index, takes all of M without the i-th "column", evaluates the product over all other columns, and stacks the results so that the array returned has the same shape as M.

In other words, what I want to evaluate is:

$$R_{ijkl} = \prod_{j^l \ne j} M_{ij^{l}kl}$$

My main concern is performance: M's shape is usually something similar to (16,8,1000,255) and this function call takes up the great majority of my program's execution time.

import numpy as np

def sliceAndMultiply(M):
    # create masks
    masks = [ range(i) + range(i+1, M.shape[1]) for i in range(M.shape[1]) ]
    # evaluate products over masks and stack them
    return np.stack([ np.prod(M[:,m,:,:], axis=1) for m in masks ], axis=1)

M = np.random.rand(16,8,1000,255)
R = sliceAndMultiply(M)

Another variant I tried is:

def sliceAndMultiply(M):
    return np.stack([ np.prod(np.delete(M, j, axis=1), axis=1) \
                      for j in range(M.shape[1]) ], axis=1)

but the performance of these two functions is basically the same.

A one liner for your problem:

def slice_and_multiply(arr):
    return np.product(arr, axis=1, keepdims=True) / M

This multiplies all of the "columns" first, then divides by each "column" to get the result of multiplying all but that column, which may be problematic depending on your data due to potential overflows or precision losses. And it breaks completely if there are any zeros in the input array.

If you can get rid of the division (this is a typical interview question, by the way), then all of those problems dissappear. And you can do it with relative ease, by cleverly storing accumulated products from both ends of the array. then multiplying them together:

def slice_and_multiply_bis(arr):
    ret = np.ones_like(arr)
    np.cumprod(arr[:, :-1], axis=1, out=ret[:, 1:])
    ret[:, :-1] *= np.cumprod(arr[:, :0:-1], axis=1)[:, ::-1]
    return ret

And of course:

>>> M = np.random.rand(16, 8, 1000, 255)
>>> np.allclose(slice_and_multiply(M), slice_and_multiply_bis(M))
True

That last code is a little too clever for its own good, so it should it should be heavily commented.

By my timings, for your target size, slice_and_multiply is 5x faster than your implementation, and slice_and_multiply_bis about 2-3x faster.


Can this architecture of base + derived classes be coded more efficient? I've got some architecture in my code similar to this. It is about the Construct method in the base class in the example below. This method contains the logic for constructing a building, which is the same in every building. Yet different buildings may want to do things differently, like a different roof. So I've used events to do this. Is this a valid architecture? I know it works, but is there a better way? I want to leave the logic of constructing any building in the base class, and not override it to just build another roof and copying 66% of its code.
Remember this is an example. In the code I had to create, it not uses 3 functions in the Construct base method as the logic flow, but over a dozen.

Base class

public abstract class Building
{
    /// <summary>
    /// use this event in the derived class to lay another foundation.
    /// </summary>
    public event EventHandler OnFoundation;
    /// <summary>
    /// Use this event in the derived class to build different walls.
    /// </summary>
    public event EventHandler OnWalls;
    /// <summary>
    /// Use this event in the derived class to build a different roof.
    /// </summary>
    public event EventHandler OnRoof;

    /// <summary>
    /// The number of floors a building as determines how high it will be.
    /// </summary>
    public readonly int NumberOfFloors;

    /// <summary>
    /// Constructor.
    /// </summary>
    /// <param name="argNumberOfFloors">The number of floors a building as determines how high it will be.</param>
    protected Building(int argNumberOfFloors)
    {
        NumberOfFloors = argNumberOfFloors;
    }

    public void Construct()
    {
        //Lay the foundation.
        if (OnFoundation != null)
        {
            OnFoundation(this, new EventArgs());
        }
        else
        {
            Foundation();
        }
        //Build the walls
        if (OnWalls != null)
        {
            OnWalls(this, new EventArgs());
        }
        else
        {
            Walls();
        }
        //Build the roof
        if (OnRoof != null)
        {
            OnRoof(this, new EventArgs());
        }
        else
        {
            Roof();
        }
    }

    public void Foundation()
    {
        //Lay foundation
    }

    public void Walls()
    {
        for (int i = 0; i < NumberOfFloors; i++)
        {
            //Build walls
        }
    }

    public void Roof()
    {
        //Build roof
    }
}

Implementations

  Villa

public class Villa : Building
{
    public Villa() : base(2)
    {
        base.OnRoof += Villa_OnRoof;
    }

    /// <summary>
    /// A villa gets another roof, the rest stays the same
    /// </summary>
    void Villa_OnRoof(object sender, EventArgs e)
    {
        //Build another roof than the base class would.
    }
}

  SkyScraper

public class SkyScraper : Building
{
    public SkyScraper() : base(60)
    {
        base.OnFoundation += SkyScraper_OnFoundation;
    }

    void SkyScraper_OnFoundation(object sender, EventArgs e)
    {
        //A skycraper needs a better foundation.
    }
}

This works but is very unusual. The standard way of doing this would actually be virtual methods

public abstract class building {
   ...
   protected virtual CreateWalls() {
      //create 4 walls by default
   }
   protected virtual CreateFoundation() {
     //straightforward foundation by default
   }

   public void Construct() {
      CreateWalls();
      CreateFoundation();
   }
}

public class SkyScraper : Building {
   public override CreateFoundation() {
      //create deep foundation in bedrock
   }
}

While this is the simplest and is certainly acceptable architecture it is always a good idea to also consider Composition over Inheritance

So instead you might do something like

public abstract class Building {
   protected Building(WallConstruction walls, FoundationConstruction foundations, ...) {
      walls.ConstructFor(this);
      foundations.ConstructFor(this);
   }
}

or even more generic

public abstract class Building {
   protected Building(IBuildingComponent[] components) {
      components.ForEach(x => x.ConstructFor(this));
   }
}

a couple more things to consider.

Does it make sense for a Building to be in an unconstructed state? Should this instead be something like BuildingBuilder (or just Construction) and have a Build method that produces a building?
The whole (EventArgs, object) pattern for events really hasn't made much sense since we got good delegate signatures in .Net 3.5. Just make the signature for the event exactly what you want it to be and use a Action or Func<> type.
When using events, instantiate the event to an empty one

   public event Action foo = delegate {};

that way the delegate will never be empty so there will be no need for a null check.


Rails and Redis: how should I handle validation here? I'm new to rails.  I am using Redis instead of something backed w/ ActiveRecord.  I need to validate the presence of location, categories, start_date, and end_date.  I then need to check that start_date and end_date are valid dates, that start_date comes before end_date.  And that location matches a regex [A-Za-z_].  And that categories.length > 0.  Since the start_date and end_date parameters in my model's setters are Date objects, should I check for valid dates and convert them in my controller.  Then have my model's setters take care of the rest of the validation?

I just don't know where to put the validations:  in my model or controller?

Model:

class MyThingie
  def self.set_x(location, categories, start_date, end_date, value)
    updates = {}
    for date in (start_date .. end_date)
      # ...
    end

    $redis.mset(*updates.flatten)
  end

  def self.set_y(location, categories, default)
    updates = {}
    for category in categories
      # ...
    end

    $redis.mset(*updates.flatten)
  end

  def self.set_z(location, categories, start_date, end_date, block)
    if block
      updates = {}
      for date in (start_date .. end_date)
        # ...
      end

      $redis.mset(*updates.flatten)
    else
      deletes = []
      for date in (start_date .. end_date)
        # ...
      end

      $redis.del(*deletes)
    end
  end
end

Controller:

class MyThingieController < ApplicationController
  # ...

  def create
    begin
      method = params[:method]
      location = params[:location]
      categories = params[:categories]
      s_start_date = params[:start_date]
      s_end_date = params[:end_date]

      if method == "normal"
        value = params[:value]

        start_date = Date.strptime(s_start_date, "%m/%d/%Y")
        end_date = Date.strptime(s_end_date, "%m/%d/%Y")

        MyThingie.set_x(location, categories, start_date, end_date, value)
      elsif method == "default"
        default = params[:default]

        MyThingie.set_y(location, categories, default)
      elsif method == "block"
        block = params[:block]

        start_date = Date.strptime(s_start_date, "%m/%d/%Y")
        end_date = Date.strptime(s_end_date, "%m/%d/%Y")

        MyThingie.set_z(location, categories, start_date, end_date, block)
      else
        raise "Invalid form submit"
      end
    rescue Exception => e
      errors = [e.message]
      respond_to do |format|
        format.json do
          json = Jsonify::Builder.new
          json.errors errors
          a = json.compile!
          render :status => 400, :json => a
        end
      end
    else
      respond_to do |format|
        format.json do
          json = Jsonify::Builder.new
          json.msg "Update successful."
          a = json.compile!
          render :json => a
        end
      end
    end
  end

Here the keys are like

mythingie:location:date = value

mythingie:location:default = default

mythingie:location:date:z = "true"

In your model.

You can include ActiveModel::Validations directly in your model without relying on ActiveRecord for persistence.

See ActiveModel::Validations & the date_validator gem

A quick example more relevant to you;

class MyThingie

  include ActiveModel::Validations

  validates :start_date, :presence => true
  validates :end_date,   :presence => true

  # Check out the date_validator gem, it allows things like
  validates_date_of :end_date, :after => :start_date

  validates_format_of :location, :with => /[A-Za-z]/

  # ...
end


Validations in filter class For my web application I have written AuthenticationFilter which validates each request to check against CSRF attacks, Browser Checks, Session Validity etc. Instead of putting multiple if else blocks I thought it can be implemented as following.

Please let me know if I can improve this.

public interface IValidator {

    public void validate(Context context) throws ValidatorException;

    public void handleFailure(Context context) throws Exception;

}

public class CSRFValidator implements IValidator, CWSConstants {

    private final Logger LOGGER = LoggerFactory.getLogger(CSRFValidator.class);

    @Override
    public void validate(Context context) throws ValidatorException {
        //some logic to validate request if it contains CSRF token
    }

    @Override
    public void handleFailure(Context context) throws Exception {
        context.getHttpServletResponse().sendError(HttpServletResponse.SC_NON_AUTHORITATIVE_INFORMATION);
    }
}

public class BrowserCheckValidator implements IValidator, CWSConstants {

    private final Logger LOGGER = LoggerFactory.getLogger(CSRFValidator.class);

    @Override
    public void validate(Context context) throws ValidatorException {
        //if request is not coming from suported browser then throw exception
    }

    @Override
    public void handleFailure(Context context) throws Exception {
        context.getHttpServletResponse().sendError(HttpServletResponse.SC_NON_AUTHORITATIVE_INFORMATION);
    }
}

public class AuthenticationFilter implements Filter{
    private List<IValidator> requestValidators = new ArrayList<>();

    public void doFilter(ServletRequest request, ServletResponse response, FilterChain fc)
            throws IOException, ServletException {
                            Context context = new Context(httpRequest, httpResponse, session);
        for (IValidator validator : requestValidators) {
            try {
                LOGGER.debug("Validating " + validator.getClass().getName());
                validator.validate(context);
            } catch (ValidatorException e) {
                LOGGER.error("Error while validating " + validator.getClass().getName(), e);
                validator.handleFailure(context);
                return;
            }
        }
        fc.doFilter(request, response);
    }

    @Override
    public void init(FilterConfig filterConfig) throws ServletException {
        LOGGER.debug("Initializing validators");
        requestValidators.add(new CSRFValidator());
    }

}
public class ValidatorException extends Exception {

    private static final long serialVersionUID = 8990146478624920065L;

    public ValidatorException() {
        super();
    }

    public ValidatorException(String message, Throwable cause, boolean enableSuppression, boolean writableStackTrace) {
        super(message, cause, enableSuppression, writableStackTrace);
    }

    public ValidatorException(String message, Throwable cause) {
        super(message, cause);
    }

    public ValidatorException(String message) {
        super(message);
    }

    public ValidatorException(Throwable cause) {
        super(cause);
    }
}

public final class Context {

    private HttpServletRequest httpServletRequest;
    private HttpServletResponse httpServletResponse;
    private HttpSession httpSession;

    public Context(HttpServletRequest httpServletRequest, HttpServletResponse httpServletResponse,
            HttpSession httpSession) {
        this.httpServletRequest = httpServletRequest;
        this.httpServletResponse = httpServletResponse;
        this.httpSession = httpSession;
    }

    public HttpServletRequest getHttpServletRequest() {
        return httpServletRequest;
    }

    public HttpServletResponse getHttpServletResponse() {
        return httpServletResponse;
    }

    public HttpSession getHttpSession() {
        return httpSession;
    }

}

Instead of putting multiple if else blocks I thought it can be implemented as following.

Looks like the Strategy Pattern with polymorphic dispatch.
A sound plan!

Make fields final when possible

The private fields of Context can all be final.
It's good to make them final so you won't change them by accident.
final fields are easier to read, as you know they will only ever have the same value.

Avoid throwing Exception

IValidator.handleFailure is defined with throws Exception.
It's recommended to avoid using the generic Exception class,
it's better to use specific exceptions.
Code using this interface is forced to catch Exception,
which creates the risk that other exception types may be caught by mistake.

Implement just what you need

Do you need all the constructors in ValidatorException?
In the posted code, there isn't a single use case using of any of those.
I suggest to implement only the functionality you actually need,
and avoid to implement in advance something "you might need someday".

Conventions

In Java it's not a convention to prefix interface names with a capital I.
I suggest to rename IValidator to Validator.

The constant interface antipattern

You haven't shared the code of CWSConstants,
but it sounds like it might simply contain constants,
and your classes implement this interface only to have easy access to those constants.

This is considered a bad practice. Interfaces are intended for contracts of behaviors, using them for easy access of constants is an abuse of the language. The alternative is to put the constants in a class instead of an interface. You may use static imports to avoid polluting the code with the name of the class everywhere.


Find out the second highest in array I want to find the first and second highest number in an array. I could come up with one solution, but I want to know the optimum solution to do the same. Can someone help me with some alternative solution to this?

int main()
{

    int arr[10] = {0,1,2,13,4,5,9,8,11,6};
    int first = arr[0];
    int second = arr[0];
    int i;

    for(i=0;i<10;i++)
    {
        if(first < arr[i])
        {
            second = first;
            first = arr[i];
        }
        else if(second < arr[i])
        {
            second = arr[i];
        }

    }

    printf("First = %d\n", first);
    printf("Second = %d\n", second);

    return 0;
}

Output:

First = 13
Second = 11

There is a problem in your code. Assuming that input is [3, 2, 1], the program will work like this.

Set first and second as 3.
Iterate through elements noticing that nothing is larger than 3.
Claim that 3 is the second number.

To fix this, you can do something like this.

if (arr[0] < arr[1]) {
    second = arr[0];
    first = arr[1];
}
else {
    second = arr[1];
    first = arr[0];
}

for (i = 2; i < elems; i++) {
    /* Your code */
}

Also, your program doesn't work well when NaN is involved in first position. This probably doesn't really matter (currently this handles integers, not double floating point numbers), but this may be still relevant for you, as it would require some special code to handle NaN.


Terminal Typing Game A few weeks ago, I developed a terminal game that increases your typing speed. The game is all about typing, providing difficulty levels from easy to hard. I feel like this terminal game won't become as popular as my others, so I need tips on how I could improve this code to make it shorter, easier to run, and more entertaining.
Here is the code developed by me, PYWPM:
import time
import random

logo = '''
  _____    __          _______  __  __
 |  __ \   \ \        / /  __ \|  \/  |
 | |__) |   \ \  /\  / /| |__) | \  / |
 |  ___/ | | \ \/  \/ / |  ___/| |\/| |
 | |   | |_| |\  /\  /  | |    | |  | |
 |_|    \__, | \/  \/   |_|    |_|  |_|
         __/ |
        |___/
'''
print(logo)
print(" ")
difficulty = input("Enter difficulty level (easy/hard): ")
if difficulty == "easy":
  openPlz = open('easywordbank.txt','r')
  readPlz = openPlz.read()
  wordBank = readPlz.split()
elif difficulty == "hard":
  openPlz = open('hardwordbank.txt','r')
  readPlz = openPlz.read()
  wordBank = readPlz.split()

open2 = open('highscore.txt','r+')
open2lst = open2.readlines()

stat = True
strike = 0
score = 0

def gameMain(wordBank):
    #Primary game loop. Returns a lst:
    #lst[0] = added points, lst[1] = added strikes
    lst = [0,0]
    start = time.time()
    wordQuiz = wordBank[random.randint(0,(len(wordBank)-1))]
    wordType = input('Enter the word, '+ wordQuiz + ' : ')
    if wordType == wordQuiz and time.time()-start < 3:
        lst[0] += 1
    elif time.time()-start >= 3:
        print('STRIKE! Too Slow! ')
        lst[1] += 1
    else:
        print('STRIKE! Watch your spelling. Be careful with strikes!')
        lst[1] += 1
    return lst

def highScore(name,score,highScoreLst,zFile):
    for line in highScoreLst:
        if score >= int(line[-3:-1]):
            highScoreLst.insert(highScoreLst.index(line),name+'-'+str(score)+'\n')
            highScoreLst.pop()
            zFile.seek(0,0)
            zFile.writelines(highScoreLst)
            break

def rsg():
    print('Ready?')
    time.sleep(1)
    print('Set?')
    time.sleep(1)
    print('Go!')
    time.sleep(1)

name = input('Enter a username for this session: ')
print("Type the word then press enter in under 3 seconds!")
time.sleep(2)
rsg()

#MainState

while stat == True:
    lst = gameMain(wordBank)
    score += lst[0]
    strike += lst[1]
    if strike == 3:
        time.sleep(.5)
        print('Game Over! The game has ended..!\n')
        time.sleep(2)
        print('Your Typing & Accuracy Score: ' + str(score))
        highScore(name,score,open2lst,open2)
        time.sleep(2)
        break
print('\nHighscores for PyWPM:')
time.sleep(2)
for line in open2lst:
    print(line, end='')
    time.sleep(1.5)
time.sleep(5)

openPlz.close()
open2.close()

Yes, this game includes a word bank that randomizes words.
The high scores aren't global.

How could I make this better?

I made some changes to your code, here's the description:

Indentation correction;
Removal of possible spaces in user input;
Change of While stat ==. True for While stat. It's the same thing;
PEP8 (and good practices) use 1 blank space after a comma and 2 line breaks before and after the functions;
You can also simplify the import of packages by specifying exactly what you are going to use: I did it in the code below;
Finally, the documentation always provides for an empty line break at the end of the code.

from time import time, sleep
from random import randint

logo = '''
  _____    __          _______  __  __
 |  __ \   \ \        / /  __ \|  \/  |
 | |__) |   \ \  /\  / /| |__) | \  / |
 |  ___/ | | \ \/  \/ / |  ___/| |\/| |
 | |   | |_| |\  /\  /  | |    | |  | |
 |_|    \__, | \/  \/   |_|    |_|  |_|
         __/ |
        |___/
'''
print(logo)
print(" ")
difficulty = input("Enter difficulty level (easy/hard): ").strip
if difficulty == "easy":
    openPlz = open('easywordbank.txt', 'r')
    readPlz = openPlz.read()
    wordBank = readPlz.split()
elif difficulty == "hard":
    openPlz = open('hardwordbank.txt', 'r')
    readPlz = openPlz.read()
    wordBank = readPlz.split()

open2 = open('highscore.txt', 'r+')
open2lst = open2.readlines()

stat = True
strike = 0
score = 0

def gameMain(wordBank):
    """
    Primary game loop. Returns a lst:
    #lst[0] = added points, lst[1] = added strikes
    :param wordBank:
    :return:
    """
    lst = [0, 0]
    start = time()
    wordQuiz = wordBank[randint(0,(len(wordBank)-1))]
    wordType = input('Enter the word, '+ wordQuiz + ' : ')
    if wordType == wordQuiz and time()-start < 3:
        lst[0] += 1
    elif time()-start >= 3:
        print('STRIKE! Too Slow! ')
        lst[1] += 1
    else:
        print('STRIKE! Watch your spelling. Be careful with strikes!')
        lst[1] += 1
    return lst

def highScore(name, score, highScoreLst, zFile):
    for line in highScoreLst:
        if score >= int(line[-3:-1]):
            highScoreLst.insert(highScoreLst.index(line), name+'-'+str(score)+'\n')
            highScoreLst.pop()
            zFile.seek(0, 0)
            zFile.writelines(highScoreLst)
            break

def rsg():
    print('Ready?')
    sleep(1)
    print('Set?')
    sleep(1)
    print('Go!')
    sleep(1)

name = input('Enter a username for this session: ')
print("Type the word then press enter in under 3 seconds!")
sleep(2)
rsg()

# MainState
while stat:
    lst = gameMain(wordBank)
    score += lst[0]
    strike += lst[1]
    if strike == 3:
        sleep(.5)
        print('Game Over! The game has ended..!\n')
        sleep(2)
        print('Your Typing & Accuracy Score: ' + str(score))
        highScore(name, score, open2lst, open2)
        sleep(2)
        break
print('\nHighscores for PyWPM:')
sleep(2)
for line in open2lst:
    print(line, end='')
    sleep(1.5)
sleep(5)

openPlz.close()
open2.close()


Unwrapping optionals for max of a 2D array in swift I'm working in swift and I've got a custom class Array2D to handle fixed-width, two-dimensional arrays of integers. What I'm trying to do is write a simple, ideally-elegant function max() -> Int? to behave the same way as for a 1-D array: return nil if the array is empty, or else the maximum value in the array. I make no promises about the array contents being positive.

For the sake of simplicity, I'm writing these functions as freestanding rather than parts of a class here.

Broadly speaking, I'm trying to do this:

func max(array: [[Int]]) -> Int? {
  set up a variable for maximum value maxSoFar
  for row in array {
    if row.max() > maxSoFar { maxSoFar = row.max() }
  }
  return maxSoFar
}

What I'm struggling with is avoiding lots of nested optional handling: I have to handle the Int? returned by max for each row of the array, and it looks like I'll need maxSoFar to be an Int? as well, since I don't want to initialize it to zero since all the entries in the array could be negative.

So, a few options:

func max1(array: [[Int]]) -> Int? {
  if array.count == 0 || array[0].count == 0 { return nil }
  var maxval: Int = array[0][0]
  for row in array {
    if let rowmax = row.max() {
      if rowmax > maxval { maxval = rowmax }
    }
  }
  return maxval
}

func max2(array: [[Int]]) -> Int? {
   var maxval: Int?
   for row in array {
     guard let rowmax = row.max() else {
       return nil
     }
     guard let temp = maxval else {
       maxval = rowmax
       continue
     }
     if rowmax > temp {
        maxval = rowmax
     }
    }
    return maxval

Thoughts? Alternatives? I'd be delighted to learn a better way to handle this.

Your first method does not work correctly if the first row is empty:

print(max1(array: [[], [1, 2], [-3]])) // nil

This is difficult to salvage while avoiding an optional for the current maxval.

Your second method returns nil if any row is empty:

print(max2(array: [[1, 2], [], [3, 4]])) // nil

guard is a useful language feature to avoid the “optional pyramid of doom,” or to “early exit” in exceptional cases. But in your max2() function it makes the program flow difficult to understand (and introduced an error, as we saw above). With plain old if-let-else it would look like this:

func max2(array: [[Int]]) -> Int? {
    var maxval: Int?
    for row in array {
        if let rowmax = row.max() {
            if let temp = maxval {
                if rowmax > temp { maxval = rowmax }
            } else {
                maxval = rowmax
            }
        }
    }
    return maxval
}

But actually the (explicit) handling of all the special cases of an empty array or empty rows can be avoided completely!

The Sequence.joined() can be applied to the nested array, it returns a (lazily evaluated) sequence of all concatenated rows.

This simplifies the implementation to

func max2D(array: [[Int]]) -> Int? {
    return array.joined().max()
}

Or more generic, for nested arrays of comparable elements:

func max2D<T>(array: [[T]]) -> T? where T: Comparable {
    return array.joined().max()
}

You may also consider if it is worth defining a function for that purpose, if you also can call

let myMax = my2DArray.joined.max()

directly.


Get attribute values with LINQ to XML I'm pulling the values from the following XML based on the attribute name.

<DataRow>
  <DataItem name="SYMBOL">MSFT-US</DataItem>
  <DataItem name="NAME">Microsoft Corporation</DataItem>
  <DataItem name="CUSIP">59491812311</DataItem>
  <DataItem name="SEDOL">2588CFA</DataItem>
  <DataItem name="CURRENCY">USD</DataItem>
  <DataItem name="DATE">2018-10-05</DataItem>
  <DataItem name="PRICE">100.25</DataItem>
</DataRow>

Here's my working code to select the values into a list:

IEnumerable<XElement> xDoc = XDocument.Parse(xmlString).Descendants("DataRow");
var query = from item in xDoc
        let symbol = (item.Elements("DataItem")
            .Where(i => (string)i.Attribute("name") == "SYMBOL").Select(i => i.Value)).FirstOrDefault()
        let compName = (item.Elements("DataItem")
            .Where(i => (string)i.Attribute("name") == "NAME").Select(i => i.Value)).FirstOrDefault()
        let cusip = (item.Elements("DataItem")
            .Where(i => (string)i.Attribute("name") == "CUSIP").Select(i => i.Value)).FirstOrDefault()
        let sedol = (item.Elements("DataItem")
            .Where(i => (string)i.Attribute("name") == "SEDOL").Select(i => i.Value)).FirstOrDefault()
        select new {
            Symbol = symbol,
            CompName = compName,
            Cusip = cusip,
            Sedol = sedol
        };

I feel that this can be made cleaner.  How can it be done?

If you don't want create a full-blown xml serializer (I wouldn't if it was a simple as that) all you can do is to just tide this up. This is, add a helper variable for dataItems and create a helper extension for finding elements based on the Name attribute. Then you can remove all the other intermediate viariables and directly assign the values to the properties because this is pretty short right now.

public static string ValueWhereName(this IEnumerable<XElement> sournce, string name)
{
    return sournce.SingleOrDefault(i => i.Attribute("name").Value.Equals(name))?.Value;
}

and use it like this:

var query =
    from item in dataRows
    let dataItems = item.Elements("DataItem")
    select new
    {
        Symbol = dataItems.ValueWhereName("SYMBOL"),
        CompName = dataItems.ValueWhereName("NAME"),
        Cusip = dataItems.ValueWhereName("CUSIP"),
        Sedol = dataItems.ValueWhereName("SEDOL")
    };

If you had a dedicated class for the result you could make it even more robust by using nameof(YourClass.Symbol). But then the properties would have to have the same names as the attributes or you would need to decorate them with your own attributes and use some reflection... You would also need to modify the Equals(name, StringComparison.OrdinalIgnoreCase) call to ignore-case sind the properties obviously wouldn't match the xml attributes casewise.

Another option could be the package Json.NET as it's also able to parse xml but I'm not sure how it could handle the attributes.


Defining a simple function-calling operator I have been writing Swift code for a while and have read a lot of tutorials about how functional paradigms can be applied to Swift.

I defined a simple operator:

infix operator >>> {associativity left}
func >>><A,B>(a: A?, f: A -> B?) -> B? {
    if let a = a {
        return f(a)
    } else {
        return .None
    }
}

Now for instance I am gonna populate a UITableViewCell instance from Core Data entity:

func tableView(tableView: UITableView, cellForRowAtIndexPath indexPath: NSIndexPath) -> UITableViewCell {

    func getWalks(dog: Dog) -> NSOrderedSet? {
        return dog.walks
    }

    func getWalkAtIndex(walks: NSOrderedSet) -> Walk? {
        return walks[indexPath.row] as? Walk
    }

    let cell = tableView.dequeueReusableCellWithIdentifier("Cell", forIndexPath: indexPath) as UITableViewCell

    func setUpCell(walk: Walk) {
        let dateFormatter = NSDateFormatter()
        dateFormatter.dateStyle = .ShortStyle
        dateFormatter.timeStyle = .MediumStyle

        cell.textLabel.text = dateFormatter.stringFromDate(walk.date)
    }

    self.currentDog >>> getWalks >>> getWalkAtIndex >>> setUpCell

    return cell
}

I am creating a function for every step, though right now I am not good at separating steps but I kind of like this style of coding. What is your opinion about this?

This coding style is confusing.

Let's unravel the line of code in question:

self.currentDog >>> getWalks >>> getWalkAtIndex >>> setUpCell

What's happening here?  To make sense of this, we essentially have to read this backwards.

What are we doing?  We're setting up a cell.

With what information are we setting up the cell?  The walk at index.  (What index?  Well, the function has a bad name...)

What collection are we looking through for this index?  The one returned by "get walks".

And what does that return?  self.currentDog.walks

If we were to code this more normally, it'd look more like this:

let cell = tableView.dequeueReusableCellWithIdentifier("Cell",
    forIndexPath: indexPath) as UITableViewCell

let currentWalk = self.currentDog.walks[indexPath.row];

let dateFormatter = NSDateFormatter()
dateFormatter.dateStyle = .ShortStyle
dateFormatter.timeStyle = .MediumStyle

cell.textLabel.text = dateFormatter.stringFromDate(currentWalk.date)

This is fewer lines of code.

This is going to be less confusing to your average Swift programmer.  Swift programmers aren't very likely to be functional programming types.

Ultimately, the biggest problem here is that the operator you've created relies on the left hand argument being the argument for a function on the right hand side.  Which means in order to use this, we have to invent lots of very specific functions that are very specific toward the objects we have.

For example, the getWalks function is only usable on Dog objects and it simply returns the walk array.

Why would I ever want to do dog >>> getWalks when I can just do dog.walks?

And truly, should this operator be different somehow?  The dog isn't getting the walks.  We're applying the getWalks function to the dog.  dog <<< getWalks makes more sense.  The arrows point to the object we're getting walks of.  However, I actually like getWalks >>> dog better.  This reads better if we say "of" in place of the arrows.

"Get walks of dog"

And it looks more like a function we'd write.

getWalks(dog)

And that feels more like functional programming.

But the problem is when we start inventing functions that aren't reusable and end up extending the lines of code we write just for the sake of using an operator.  Moreover, these are functions that don't even take in the necessary number of arguments.

Isn't indexPath.row a value that should be passed into getWalkAtIndex?  The easiest, clearest thing to do would just treat the returned walks array as what it is... an array... and get the object at index the same way you would anyway... but if you insist on a function, then the function should be more like this:

getWalk(walks: NSOrderedSet index atIndex:NSInteger) -> Walk? {
    return walks[index] as Walk?
}

And we should call it as such:

getWalk(walks atIndex:indexPath.row)

We shouldn't require an NSIndexPath when we're only going to use the .row property of it.  And we really shouldn't be reaching outside of the function at what is effectively a global variable as far as this function is concerned.

Some of the same can be said for the setUpCell function.  Although truly, this should be a function.  We don't need a function to set a cell's text label's text property.

The function should be to format the date string.  And it should simply take an NSDate object and return a string.

So we'll have:

cell.textLabel.text = formatDate(walk.date)

At the end of the day though, Swift is an object oriented language.  If you're writing Swift, you're using all of the Apple libraries, frameworks, etc.  And that means you're using a lot of stuff that was designed for OOP.  And unless you intend to write a function sister to match your pattern for all of these existing methods for all these classes, then any code you write using this operator is going to be a hodge-podged mix of styles that will be confusing to people expecting OOP.


Print out N by N Spiral Matrix in javascript I got this question during my practice interview.
Matrices - N Spiral Matrix
Prompt
Given an integer N, output an N x N spiral matrix with integers 1 through N.
Examples:
Input:  3

Output: [[1, 2, 3],
         [8, 9, 4],
         [7, 6, 5]]

Input: 1

Output: [[1]]

My solution is below:
function spiralMatrix(n) {
  const matrix = [];
  let counter = 1,
    rowMin = 0,
    rowMax = n - 1,
    colMin = 0,
    colMax = n - 1;

  for (let i = 0; i < n; i++) {
    matrix.push(new Array(n).fill(0));
  }

  while (rowMin <= rowMax && colMin <= colMax) {
    for (let col = colMin; col <= colMax; col++) {
      matrix[rowMin][col] = counter++;
    }
    rowMin++;
    for (let row = rowMin; row <= rowMax; row++) {
      matrix[row][colMax] = counter++;
    }
    colMax--;
    for (let col = colMax; col >= colMin; col--) {
      matrix[rowMax][col] = counter++;
    }
    rowMax--;
    for (let row = rowMax; row >= rowMin; row--) {
      matrix[row][colMin] = counter++;
    }
    colMin++;
  }

  return matrix;
}

console.log(spiralMatrix(10));

This code is well written;

Comments

You have no comments whatsoever, fortunately the code reads well

JSHint.com

Your code passes all checks, well done


Hackerrank All Women's Codesprint 2019: Name the Product Link: https://www.hackerrank.com/contests/hackerrank-all-womens-codesprint-2019/challenges/name-the-product/

Problem

  You are contesting to name a new product in your company given the following conditions:

  You are given an array of \$n\$ different names, \$names\$, where \$names_i\$ denotes the \$i^{th}\$ name and all the names are of length \$5\$. The distance between any two names is the number of positions in which the characters in these names differ. For example, "bubby" and "bunny" differ in two positions.

  You have to choose a name such that the sum of differences of all names in \$names\$ with the chosen name is maximal. In order to win the contest, give the new product this chosen name.

  Note: If there are many such names chose the lexicographically largest one.

  Take for example, names = ["bubby", "bunny", "berry"], with length \$n = 3\$. Then, the name that you should choose is "zzzzz" as this name has no common character with any name in the names list and is also lexicographically the largest.

  Function Description

  Complete the productName function in the editor below. It should return the lexigraphically largest string of length  whose sum of differences with all the names is maximal.

  productName has the following parameter(s):

  names: array of \$n\$ names

  Input Format

  The first line contains an integer, \$n\$, denoting the number of elements in \$names\$.
  Each line \$i\$ of the \$n\$ subsequent lines (where \$0 \le i \le n\$) contains a string describing \$names_i\$.

  Constraints

  \$1 \le n \le 10^5\$
  All characters in the names are lowercase English alphabets.
  Each name is of length \$5\$.

  Output Format

  The output should contain the lexigraphically largest string of length  whose sum of differences with all the names is maximal.

  Sample Input 0

3
bubby
bunny
berry

  Sample Output 0

zzzzz

  Explanation 0

  Difference between \$names_0\$, bubby, and zzzzz is \$5\$.
  Difference between \$names_1\$, bunny, and zzzzz is \$5\$.
  Difference between \$names_2\$, berry, and zzzzz is \$5\$.
  So, total difference is 15, which is maximal.

  Sample Input 1

3
ready
stedy
zebra

  Sample Output 1

yzzzz

  Explanation 1

  Difference between \$names_0\$, ready, and yzzzz is \$5\$.
  Difference between \$names_1\$, stedy, and yzzzz is \$5\$.
  Difference between \$names_2\$, zebra, and yzzzz is \$5\$.
  So, total differce is 15, which is maximal.

I would strip the extraneous parts from my solution:

import math, os, random, re, sys
from collections import defaultdict as dd, Counter as count

alphabet = "abcdefghijklmnopqrstuvwxyz"
def productName(names):
    charmap = [dd(lambda: 0, count(name[i] for name in names)) for i in range(5)]
    return "".join(max(alphabet, key=lambda x: (-charmap[i][x], x)) for i in range(5))

I'm concerned with adhering to best practices and maximising performance.

Imports
import math, os, random, re, sys
from collections import defaultdict as dd, Counter as count

Seriously, don't do this. It may be useful to read PEP8 regarding imports again.

Imports should be on separate lines.

Why do Counter as count?  It impairs readability.
Especially in larger scripts, every Python dev knows Counter, but count could be a random variable and is easy to be overshadowed.

You don't use all of the imported modules; just remove them if you don't need them.

   alphabet = "abcdefghijklmnopqrstuvwxyz"

Here you missed something that is in the standard lib: from string import ascii_lowercase

Misc

This dd(lambda: 0, ... adds no value
Counter is already a dictionary with default value 0 just remove that part

Magic numbers
5 is a Magic number, you should define it as a CONSTANT:
  NAME_LENGTH = 5

The rest looks good!


Efficiently generating HTML CSS table using Java I am trying to generate an HTML table using Java. I have an object which I am iterating to make an HTML table.

StringBuilder sb = new StringBuilder();
sb.append("");
sb.append("");
sb.append("");
sb.append("
");
sb.append(" ClientName");
sb.append("	");
sb.append(" SyncCount");
sb.append("	");
sb.append(" SyncPercentile");
sb.append("	");
sb.append(" SyncAvg");
sb.append("	");
sb.append(" SyncMax");
sb.append("	");
sb.append(" AsyncCount");
sb.append("	");
sb.append(" AsyncPercentile");
sb.append("	");
sb.append(" AsyncAvg");
sb.append("	");

for (DataMetrics metrics : dataMetricsList) {
    sb.append("
");
    sb.append(" " + metrics.getName());
    sb.append("	");
    sb.append(" " + metrics.getSyncCall());
    sb.append("	");
    sb.append(" " + metrics.getSyncPercent());
    sb.append("	");
    sb.append(" " + metrics.getSyncAvg());
    sb.append("	");
    sb.append(" " + metrics.getSyncMax());
    sb.append("	");
    sb.append(" " + metrics.getAsyncCall());
    sb.append("	");
    sb.append(" " + metrics.getAsyncPercent());
    sb.append("	");
    sb.append(" " + metrics.getAsyncAvg());
    sb.append("	");
    sb.append("
");
}
sb.append("
");
sb.append("");
sb.append("");

System.out.println(sb.toString());

Somehow, the code looks pretty ugly to me in the way HTML and CSS are being used within StringBuilder. I am opting for code review to see whether we can improve anything here.  Is there any better way of writing this code?

How about extracting the style=""s into the head?

sb.append("");

You should write helper methods, that do the 'heavy lifting' for you:

void appendTag(StringBuilder sb, String tag, String contents) {
    sb.append('<').append(tag).append('>');
    sb.append(contents);
    sb.append("</").append(tag).append('>');
}
void appendDataCell(StringBuilder sb, String contents) {
    appendTag(sb, "td", contents);
}
void appendHeaderCell(StringBuilder sb, String contents) {
    appendTag(sb, "th", contents);
}

Please note, that if you are using StringBuilder you should not concatenate strings with +.  You want to get rid of string concatenation, not do it.

If you want to get even more expressive you should take a look at existing template engines. They are optimized and mostly easy to use.

Suggesting mustache:

If you are using mustache, you can write a template like this:

    <title>Table</title>

....

    {{#metrics}}

        {{getName}}
        ...

    {{/metrics}}

and then render it via e.g.

public static void main(String[] args) throws IOException {
    HashMap<String, Object> scopes = new HashMap<String, Object>();
    scopes.put("metrics", dataMetricsList);

    Writer writer = new OutputStreamWriter(System.out);
    MustacheFactory mf = new DefaultMustacheFactory();
    Mustache mustache = mf.compile(new StringReader(getTemplateCodeFromAbove()), "example");
    mustache.execute(writer, scopes);
    writer.flush();
}

It depends on your use case, of course. If all you ever want is to render a single table, you are fine with StringBuilder. But if you find yourself repeating this, get an external library for it.


Keeping object consistence at its creation Let's assume I have 2 objects Product and Volume. 1 Product must have at least 1 Volume which means: Product ----> 1...* Volume.

I want to keep the consistence of Product at its creation, in other words, I want to block the user to create products without any volumes.

I have a ready working code, but nonetheless I want to know your opinions guys.

Here is the running code: https://3v4l.org/t7JqO#v700

Here is my code:

<?php
declare(strict_types = 1);

namespace App
{
    class Product
    {
        private $id;
        private $name;
        private $volumes;

        /**
         * Product constructor.
         * @param int $id
         * @param string $name
         * @param array $volumes
         * @throws \InvalidArgumentException
         */
        public function __construct(int $id, string $name, array $volumes)
        {
            $this->id = $id;
            $this->name = $name;

            if (count($volumes) === 0) {
                throw new \InvalidArgumentException('Your product must have 1 product at least.');
            }

            $this->volumes = $this->prepareVolumes($volumes);
        }

        private function prepareVolumes(array $volumes)
        {
            foreach ($volumes as $volume) {
                $volume->setProduct($this);
            }
            return $volumes;
        }
    }

    class Volume
    {
        protected $id;
        protected $product;
        protected $description;

        public function __construct(string $description)
        {
            $this->description = $description;
        }

        public function setProduct(Product $product)
        {
            $this->product = $product;
        }
    }

    class ProductTest
    {
        public function __construct()
        {
            $volume1 = new Volume('part-1');
            $volume2 = new Volume('part-2');
            $volume3 = new Volume('part-3');

            $product = new Product(1, 'BED', [$volume1, $volume2, $volume3]);
            $invalidProduct = new Product(2, 'Table', []);
        }
    }

    return new ProductTest();
}

At first:

  Note: Strict typing is only defined for scalar type declarations, and as such, requires PHP 7.0.0 or later, as scalar type declarations
  were added in that version.

Secondly:
You are passing in an array of volumes $volumes to the __construct function when instantiating an object of class Product. But there is no confidence that each element of $volumes is an object of Volume class. Someone could put into $volumes array an arbitrary value.
I would recommend applying dependency with Volume class and pass in Product constructor only one object of Volume class (following the initial rule: "Product must have at least 1 Volume"). And then, if necessary, you will be able to add some additional 'volumes' to product object using an auxiliary method addVolume().
See the optimized version:

<?php
declare(strict_types = 1);

namespace App
{
    class Product
    {
        private $id;
        private $name;
        private $volumes = [];

        /**
         * Product constructor.
         * @param int $id
         * @param string $name
         * @param array $volumes
         */
        public function __construct(int $id, string $name, Volume $volume)
        {
            $this->id = $id;
            $this->name = $name;
            $volume->setProduct($this);
            $this->volumes[] = $volume;
        }

        public function addVolume(Volume $volume)
        {
            $volume->setProduct($this);
            $this->volumes[] = $volume;
        }
    }

    class Volume
    {
        protected $id;
        protected $product;
        protected $description;

        public function __construct(string $description)
        {
            $this->description = $description;
        }

        public function setProduct(Product $product)
        {
            $this->product = $product;
        }
    }

    class ProductTest
    {
        public function __construct()
        {
            $volume1 = new Volume('part-1');
            $volume2 = new Volume('part-2');
            $volume3 = new Volume('part-3');

            $product = new Product(1, 'BED', $volume1);
            $product->addVolume($volume2);
            $product->addVolume($volume3);

            $invalidProduct = new Product(2, 'Table', []);
        }
    }

    return new ProductTest();
}


Convert number from base-10 number system to any number system I wrote a small method for convert number from base-10 number system to any number system (code is not perfect but that is not the point).
My question is if I get right single responsibility principle. Is 2nd approach better than 1st or 'charReplace' functionality is too small for creating own method ?

    public static string convertNumber(BigInteger num, int baseNum)
    {
        string symbols = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
        if (baseNum < 2 || symbols.Length + 10 < baseNum)
        {
            throw new Exception;
        }

        var result = "";
        do
        {
            var partResult = (int)(num % baseNum);
            if (partResult >= 10)
            {
                result = result.Insert(0, symbols[partResult - 10].ToString());
            }
            else
            {
                result = result.Insert(0, partResult.ToString());
            }
            num /= baseNum;
        } while (num != 0);

        return result;
    }

vs.

    static readonly string Symbols = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";

    public static string convertNumber(BigInteger num, int baseNum)
    {
        if (baseNum < 2 || Symbols.Length + 10 < baseNum)
        {
            throw new Exception;
        }

        var result = "";
        do
        {
            var partResult = (int)(num % baseNum);
            result = result.Insert(0, charReplace(partResult));
            num /= baseNum;
        } while (num != 0);

        return result;
    }

    private static string charReplace(int number){
        if (number < 10){
            return number.ToString();
        }

        return Symbols[number - 10].ToString();
    }

I believe the 1st approach is good enough since it personally more readable.

Little suggestions

I'd prefer to store the whole alphabet in the symbols string, instead of only letters. This could simplify the logic.
You can calculate the resulting length of the output string and avoid string insertions.

Summarizing:

public static string ConvertNumber(BigInteger num, int baseNum)
{
    const string symbols = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
    if (baseNum < 2 || symbols.Length < baseNum)
    {
        throw new ArgumentOutOfRangeException(nameof(baseNum));
    }

    if (num < 0)
    {
        throw new ArgumentOutOfRangeException(nameof(num));
    }

    var resultLength = 1 + Math.Max((int)BigInteger.Log(num, baseNum), 0);
    var result = new char[resultLength];
    int index = resultLength - 1;
    do
    {
        result[index--] = symbols[(int)(num % baseNum)];
        num /= baseNum;
    } while (num != 0);

    return new string(result);
}

PS. It's recommended to name all classes and all methods in UpperCamelCase regardless of their publicity.


Ball Entity for a Entity/Component/System Soccer Game Here is a code snippet of a class that I be interested to refactor. I was in doubt on how to proceed with the process of instantiation. The original ask was here. After think about it and heard some opinions I end up refactoring and splitting the instantiation in several constructors, one public and all others private. what do you think of this approach?

I take this opportunity to ask you to evaluate other aspects of the code.

public class BallEntity extends UserEntity {
    public static final float SCALE_FACTOR = 0.78f;
    public static final String BALL = "ball";
    private Body ballBody;
    private Camera camera;
    private Sprite ball;
    private Fixture ballFixture;

    public BallEntity(TextureAtlas atlas, RubeSceneHelper rubeSceneHelper, Camera camera) {
        this(ScaledSprite.createUsingHeight(new Sprite(atlas.findRegion(BALL)), SCALE_FACTOR), rubeSceneHelper);
        this.camera = camera;
    }

    private BallEntity(ScaledSprite scaledSprite, RubeSceneHelper rubeSceneHelper) {
        this(scaledSprite.getSprite(), rubeSceneHelper.getBody(BALL), rubeSceneHelper);
    }

    private BallEntity(Sprite sprite, Body ballBody, RubeSceneHelper rubeSceneHelper) {
        this(rubeSceneHelper.getFixture(ballBody, BALL));
        this.ball = sprite;
        this.ballBody = ballBody;
    }

    private BallEntity(Fixture ballFixture) {
        this.ballFixture = ballFixture;
    }

    @Override
    public Array<Component> getComponents() {
        Array<Component> components = new Array<Component>();
        components.add(PositionComponent.newInstance());
        components.add(CameraFollowerComponent.newInstance(camera));
        components.add(SpriteComponent.newInstance(ball));
        components.add(BodyComponent.newInstance(ballBody));
        components.add(BallContextComponent.newInstance());
        return components;
    }

    @Override
    public void init(Entity entity) {
        BodyComponent bodyComponent = entity.getComponent(BodyComponent.class);
        bodyComponent.setPosition(Vector2.Zero);
        ballFixture.setUserData(new FixtureUserData(FixtureType.BALL, entity));
    }
}

public abstract class UserEntity {

    private final Entity entity;
    private boolean wasBuilt = false;
    private Array<Class<? extends Component>> componentClasses;

    protected UserEntity() {
        this.entity = new Entity();
    }

    public final Entity getEntity() {
        if (!wasBuilt) {
            final Array<Component> components = getComponents();
            componentClasses = new Array<Class<? extends Component>>();
            for (Component c : components) {
                entity.add(c);
                componentClasses.add(c.getClass());
            }
            init(entity);
        }
        return entity;
    }

    protected abstract Array<Component> getComponents();

    public Class<? extends Component>[] getComponentClasses() {
        if (!wasBuilt) {
            throw new IllegalArgumentException("It needs to build entity before use it.");
        }
        return componentClasses.toArray();
    }

    /* Override when need to init some components */
    public void init(Entity entity) {

    }

    public void addToEngine (Engine engine){
        engine.addEntity(entity);
    }

}

Looking at the class from the outside, this is how it gets created:

public BallEntity(TextureAtlas atlas, RubeSceneHelper rubeSceneHelper, Camera camera)

How the class actually gets created requires jumping between a sequence of private constructors... that isn't needed at all.

public BallEntity(TextureAtlas atlas, RubeSceneHelper rubeSceneHelper, Camera camera) {
    this.camera = camera;
    this.ballFixture = rubeSceneHelper.getFixture(ballBody, "ball");

    Sprite sprite = new Sprite(atlas.findRegion(BALL))
    this.ball = ScaledSprite.createUsingHeight(sprite, SCALE_FACTOR).getSprite();
    this.ballBody = rubeSceneHelper.getBody("ball");
}

Here I've introduced an intermediate Sprite object variable, only because all those chained calls wouldn't look right.

Isn't it easier to follow? :)

This seems pretty verbose:

public Array<Component> getComponents() {
    Array<Component> components = new Array<Component>();
    components.add(PositionComponent.newInstance());
    components.add(CameraFollowerComponent.newInstance(camera));
    components.add(SpriteComponent.newInstance(ball));
    components.add(BodyComponent.newInstance(ballBody));
    components.add(BallContextComponent.newInstance());
    return components;
}

I'm more into C#, but I'm pretty sure (make it 51%) you could do this instead (assuming Array<T> is intended to work like a T[]):

public Component[] getComponents() {
    return new Component[] {
        PositionComponent.newInstance(),
        CameraFollowerComponent.newInstance(camera),
        SpriteComponent.newInstance(ball),
        BodyComponent.newInstance(ballBody),
        BallContextComponent.newInstance()
    };
}


Can people understand my form validation code? I've just finished creating this form validation but I want to make it public for beginner 'contact us forms'.

I was wondering if I can have some peoples' input on if they can understand/read my JavaScript. Although I know my JS works, it's better to be safe than to have 20 people asking what this does.  It's just input feedback.

What I'm trying to achieve in my code is making fields required with JavaScript.

The following fields are REQUIRED:

First Name
Last Name
Email
Phone

Most of the functions such as alphaNumeric only allows numbers and onlyAlphameric only allows letters.

E.g for each function or what not, add a comment saying what it does.

For example, adding a comment:

This function does this and triggers that and so on.

JavaScript:

function elem(id) {
    return document.getElementById(id);
};

window.onload = function () {
    document.querySelector("#RadioGroup1_0").click();

    var form = document.getElementById('form');
    form.onsubmit = function (e) {
        var rules = [
        ['first-name', elem('first-name').value.length > 0],
        ['last-name', elem('last-name').value.length > 0],
        ['email', elem('email').value.length > 0 && /^([a-zA-Z0-9_\.\-])+\@(([a-zA-Z0-9\-])+\.)+([a-zA-Z0-9]{2,4})+$/.test(elem('email').value)],
        ['phone', elem('phone').value.length > 7 && elem('phone').value.length < 11 && /^(\+\d{1,2})?[\d ()-]+$/.test(elem('phone').value)]

    ];

    function myFunction()
{
document.getElementById("myForm").reset();
}

        function alpha(e) {
            var k;
            document.all ? k = e.keyCode : k = e.which;
            return ((k > 64 && k < 91) || (k > 96 && k < 123) || k == 8);
        }

        var valid = true;
        var firstFocus = null;
        for (var i = 0; i < rules.length; i++) {
            var parent = elem(rules[i][0]).parentNode;
            if (!rules[i][1]) {
                valid = false;
                parent.children[2].style.display = "inline";
                if (firstFocus == null) firstFocus = parent.children[1];
            } else {
                parent.children[2].style.display = "none";
            }

        }
          if (!valid) {
            firstFocus.focus();
            return false;
        }

        return true;
    };
};

function onlyAlphabets(e, t) {
    try {
        if (window.event) {
            var charCode = window.event.keyCode;
        } else if (e) {
            var charCode = e.which;
        } else {
            return true;
        }
        if ((charCode > 64 && charCode < 91) || (charCode > 96 && charCode < 123) || (charCode === 8)) return true;
        else return false;
    } catch (err) {
        alert(err.Description);
    }
}

var specialKeys = new Array();
specialKeys.push(8); //Backspace
function IsNumeric(e) {
    var keyCode = e.which ? e.which : e.keyCode
    var ret = ((keyCode >= 48 && keyCode <= 57) || specialKeys.indexOf(keyCode) != -1);
    //document.getElementById("phone").style.display = ret ? "none" : "inline";
    return ret;
}

Can people understand my code?

Its certainly not bad code - but I believe there are some things you can do which will improve the speed with which people like me can grasp what it is doing (and why).

Comments

Some people think well-written code needs no comments. I don't fall into that camp. Looking at your code I see the following:

Total number of comments is 2

Total number of helpful comments is 1

Total number of comments describing goal of sections of code is 0

Languages like Java have JavaDoc, which are comments in the code from which you can automatically produce documentation. If you've ever tried to re-use code that has no documentation you'll know how useful it is. If you've ever had to maintain documentation that is separate from code you'll know what a pain that is. Innaccurate documentation is worse than none.

So I believe it is best to put documentation into code. Ideally in a form from which documentation can be produced.

I like to treat every function/method I write as if it were one I would put into a library and re-use. To me this means it needs comments that document its purpose, interface and any quirks.

Magic Numbers

You have a lot of magic numbers, e.g. 48 and 57. These should be either

explained in comments
replaced by constants with meaningful names (e.g. ASCII_0, DIGIT_0 ...)
replaced by functions

The latter may be best, especially if you (or anyone else) want this code to work with character sets other than ASCII.

Meaningful names

 function myFunction()

I feel you may be underestimating the usefulness of meaningful names as an aid to other's understanding. I'd try to find a more helpful name that helps others understand the purpose of the the function and what it returns.

Writing for comprehension

You use the ternary operator. Often this is used for assignment. If you use if for assignment but in an unusual way you may momentarily confuse readers.

Unless you need conciseness, it can sometimes be clearer to use a conventional if-then-else form instead.

Wouldn't

document.all ? k = e.keyCode : k = e.which;

be clearer as

k = document.all ? e.keyCode : e.which;

Because the main purpose seems to be to assign a value to k. The earlier style is one I might use if the main point is to call a function and, as an afterthought, evaluate its's success or failure. I think in this case it moves the reader's focus to the wrong place.

Writing portable code and avoiding browser-sniffing

The above use of the ternary operator can probably be avoided entirely. I suspect you may be interested in jQuery and replace it with

keyPressed = e.which

Which I find easier to read and which can be understood more rapidly.

I would at least factor out browser-dependent and/or repeated stuff into my own functions/methods and/or carefully document in comments what it is doing and why it needs to be done.

Documenting purpose

  What I'm trying to achieve in my code is  ...

As I mentioned above, I believe it is very useful to have this described in comments in the code itself (others may disagree). From it's inclusion in your question you evidently believe this helps readers understand the following code. Therefore I'd put it into the code as a comment.


Member list reveals member information on click (#1) What I'm doing?

I'm creating a member-list where initially only the names are visible. Clicking the names reveals the member information. This is done with jQuery by adding/removing classes.

I left out the CSS with the transitions for this review.

HTML:

        member name

        member name

        member name

jQuery:

You're double-wrapping elements in jQuery. blContent and blCurrentContent are already jQuery objects, so doing $(blContent) isn't necessary. It's common practice to prefix jQuery collections with a dollar sign $.

Other than that your code looks fine, but you could use toggleClass and exclude the current sibling element from the previous collection:

var $blContent = $('.block-list__content').addClass('hidden');

$('.block-list__link').click(function(e) {
  e.preventDefault();
  var $content = $(this)
    .siblings('.block-list__content')
    .toggleClass('hidden visible');
  $blContent
    .not($content)
    .removeClass('visible')
    .addClass('hidden');
});

Your code: http://jsbin.com/duqeb/1/edit
Updated code: http://jsbin.com/geboq/4/edit


Is there a way to speed-up the following code? Is there a way to speed-up the following R code?

actionVec<-NULL
for(iii in 1:length(resultTable$V1)){
actionVec[iii]<- sum(browse$V2==resultTable$V1[iii] & browse$V4 == i & browse$V5 == ii)}

this is a loop to form a vector for each record (row) in resultTable, containing the number of entries that match the conditions in table browse

this script runs 36 minutes. I suspect that there is a way to make it run much faster. I have experienced a reduction from 15 min to 10 ms (!) on another script when avoiding loops but using vectors. But in this case I can`t find out how to make it.

EDIT1:
variables i and ii come from outer loop and may be two arbitrary integers for this loop.

EDIT2: more complete version

SESSION_NUMBER<-length(resultTable$V1)

for(i in 1:55){ for(ii in 1:21){
if(sum(browse$V4 == i & browse$V5 == ii) > treshold){
actionVec<-NULL
for(iii in 1:SESSION_NUMBER){
actionVec[iii]<- sum(browse$V2==resultTable$V1[iii] & browse$V4 == i & browse$V5 == ii)}
resultTable[,i+1] <- actionVec
}}}

Yes, there is:

Since i and ii and browse$V4 and browse$V5 are outside of the loop, you can precalculate this before going into the loop. That way you can save a small bit of calculating.

if (sum(browse$V4 == i & browse$V5 == ii) > treshold)

I dunno what you're doing here, but from what I can see, you're calling sum(1) or sum(0). Naturally, this produces 0 or 1. Then you're comparing it to treshold. Well, if treshold is below 1 but not below 0, then you can just remove the check, as well as the for loop, since browse doesn't change in the for loop!

If treshold is below 0, then the check is not needed. Thus, (please check if my syntax is correct)

SESSION_NUMBER<-length(resultTable$V1)

if(treshold < 1 && treshold >= 0){
    //check if browse$V4 and browse$V5 are in range
    if(browse$V4 >= 1 && browse$V4 <= 55 && browse$V5 >= 1 && browse$V5 <= 21){
        actionVec<-NULL
        for(iii in 1:SESSION_NUMBER){
            actionVec[iii]<- sum(browse$V2==resultTable$V1[iii])
        }
        resultTable[,browse$V4+1] <- actionVec
    }
} else if (treshold < 0){
    for(i in 1:55){
        for(ii in 1:21){
            actionVec<-NULL
            temp<-browse$V4 == i & browse$V5 == ii
            for(iii in 1:SESSION_NUMBER){
                actionVec[iii]<- sum(browse$V2==resultTable$V1[iii] & temp)
            }
            resultTable[,i+1] <- actionVec
        }
    }
}


Run-length encoding in Haskell I wrote a short Haskell script to compress and decompress via the use of run length encoding.

The concept is simple enough, n equal items x in a row will be replaced by (n, x), decompressing is just the reverse.

For example:

> runLengthCompress "foooo barrr"
[(1,'f'),(4,'o'),(1,' '),(1,'b'),(1,'a'),(3,'r')]

As you can see the four consecutive o have been substituted by (4,'o')

About the code itself I fear I have written too little points (arguments) to my functions, hindering readability but I am not sure, maybe the lack of points makes it more readable, not less.

Also I feel like my areInverses function is already built-in into Haskell and that I am rebuilding the wheel with it.

Without further ado, here is the code:

import Data.List
import Control.Monad
import Control.Arrow

runLengthExpand :: [(Int, a)] -> [a]
runLengthExpand = concat . map (uncurry replicate)

runLengthCompress :: Eq a => [a] -> [(Int, a)]
runLengthCompress = (map (length &&& head)) . group

areInverses :: Eq b => (a -> b) -> (b -> a) -> [b] -> Bool
areInverses f g = all ((==) =<< f . g)

examples :: [[Int]]
examples = [ [3, 3, 3, 5, 5, 7, 1], [1,1,1,0,0,0,5] ]

main :: IO()
main = do
  print $ runLengthCompress "foooo barrr"
  print $ areInverses runLengthExpand runLengthCompress examples

You want to import only the functions you're actually going to need:
import Data.List     (group)
import Control.Arrow ((&&&))

As Gurkenglas already said, use concatMap instead of concat . map. This leads to
runLengthExpand :: [(Int, a)] -> [a]
runLengthExpand = concatMap (uncurry replicate)

Your runLengthCompress is fine, although (&&&) might not be known to newcomers.
Now, areInverses is too obfuscated:

About the code itself I fear I have written too little points (arguments) to my functions, hindering readability but I am not sure, maybe the lack of points makes it more readable, not less.

Pointfree style isn't necessarily easier to read:

5 Problems with pointfree
Point-free style can (clearly) lead to Obfuscation when used unwisely.

We can split its functionality in multiple functions:
isInverseOn :: Eq a => (a -> b) -> (b -> a) -> a -> Bool
isInverseOn f g x = x == g (f x)

areInverses :: Eq a => (a -> b) -> (b -> a) -> [a] -> Bool
areInverses f g = all (isInverseOn f g)

This removes the need for =<< completely and is easier to read than your previous (==) =<< f . g.
However, that only checks whether g is an inverse of f, not whether f is the inverse of g. For example, g's codomain might not be the completely covered by f's domain. So a proper areInverses should also check isInverseOn g f for some appropriate values.
You can use QuickCheck to generate appropriate strings and lists of pairs, although you need to generate the latter by hand.


Caesar Cipher encrypt/decrypt tool At the moment I'm learning Python and I made this script to workout my skills.
Since it is one of my first programs I would love some feedback and tips, like more organization, less lines of code or more optimization, anything I could improve.

print "Do you want to encrypt or decrypt the message (e/d)"
action = raw_input("> ")
while action != "e" and action != "d":
    action = raw_input( "(e/d): ")

abc_l_e = "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz"
abc_u_e = "ABCDEFGHIJKLMNOPQRSTUVWXYZABCDEFGHIJKLMNOPQRSTUVWXYZ"
abc_l_d = "zyxwvutsrqponmlkjihgfedcbazyxwvutsrqponmlkjihgfedcba"
abc_u_d = "ZYXWVUTSRQPONMLKJIHGFEDCBAZYXWVUTSRQPONMLKJIHGFEDCBA"
chars = "!\"#$%&'()*+-'-./:;<=>?@[]^_{}|~1234567890"

if action == "e":
    print "\nEnter the message to encrypt"
    message = raw_input("> ")

    print "\nEnter the key"
    try:
        key = input("> ")
        while not (key >= 1 and key <= 26):
            print "Number not in range (1-26). Try again"
            key = input("> ")
    except:
        print "That's not a correct integer. Try again."
        exit()

    e_message = ""
    for char in message:
        if char in abc_l_e:
            e_message += abc_l_e[(abc_l_e.index(char) + key)]
        if char in abc_u_e:
            e_message += abc_u_e[(abc_u_e.index(char) + key)]
        elif char == " ":
            e_message += " "
        elif char in chars:
            e_message += char
    print "Your encrypted message is:\n", e_message

elif action == "d":
    print "\nEnter the message to decrypt"
    message = raw_input("> ")
    print "\nEnter the key (enter \"n\" if you don't know)"
    try:
        key = raw_input("> ")
        if key != "n":
            while not (int(key) >= 1 and int(key) <= 26):
                print "Number not in range (1-26). Try again"
                key = input("> ")
    except:
        print "That's not a correct integer nor \"n\". Try again."
        exit()

    d_message = ""
    if key != "n":
        key = int(key)
        for char in message:
            if char in abc_l_d:
                d_message += abc_l_d[(abc_l_d.index(char) + key)]
            elif char in abc_u_d:
                d_message += abc_u_d[(abc_u_d.index(char) + key)]
            elif char == " ":
                d_message += char
            elif char in chars:
                d_message += char
        print "Your decrypted message is:\n", d_message

    if key == "n":
        results = []
        for turn in range(1,26):
            d_message = ""
            for char in message:
                if char in abc_l_d:
                    d_message += abc_l_d[(abc_l_d.index(char) + turn)]
                elif char in abc_u_d:
                    d_message += abc_u_d[(abc_u_d.index(char) + turn)]
                elif char == " ":
                    d_message += char
                elif char in chars:
                    d_message += char
            results.append(d_message)

        print "\nYour decrypted message is between one of the following:"
        for result in results:
            print "%d." % (results.index(result) + 1), result

Rather than prompting for input, almost all shell scripts (including this) would be improved by accepting the static input (action and key) as arguments and then reading standard input to EOF to produce the output. For this you'll want a main method which just orchestrates argument parsing and passing the resulting values to the function or method which does the actual substitution. This makes it very easy to combine the script with other commands in a common way. argparse is very well suited for this.
Encrypting vs decrypting is simply a matter of shifting the characters left vs right. You can use this information to halve the code.
The brute force method should be implemented by running a single function 26 times.
In general, the code should be split into functions until there is no obvious duplication anywhere. This should massively simplify everything except the input handling, meaning that the interesting part of the script should end up being maybe three lines.
You should handle input one line at a time, so that even huge input results in small memory use.


BaseRepository for EF6 - updated Previous question: BaseRepository for EF6

I've update my code.  Is it correct now?  I've changed IEnumerable to IQueryable.

public interface IRepository<T> : IDisposable
{
    T Get(Expression<Func<T, bool>> predicate);
    IQueryable<T> GetWhere(Expression<Func<T, bool>> predicate = null);
    void Add(T entity);
    void Delete(T entity);
    void DeleteWhere(Expression<Func<T, bool>> predicate);
    void Update(T entity);
    void Save();
}

public class BaseRepository<T> : IRepository<T> where T : class
{
    private readonly DbContext _dbContext;

    public BaseRepository(DbContext dbContext)
    {
        _dbContext = dbContext;
    }

    public T Get(Expression<Func<T, bool>> predicate)
    {
        return GetWhere(predicate).FirstOrDefault();
    }

    public IQueryable<T> GetWhere(Expression<Func<T, bool>> predicate = null)
    {
        IQueryable<T> result = _dbContext.Set<T>();
        return (predicate == null) ? result : Queryable.Where<T>(result, predicate);
    }

    public void Add(T entity)
    {
        _dbContext.Entry(entity).State = EntityState.Added;
    }

    public void Delete(T entity)
    {
        _dbContext.Entry(entity).State = EntityState.Deleted;
    }

    public void DeleteWhere(Expression<Func<T, bool>> predicate)
    {
        IQueryable<T> entities = GetWhere(predicate);
        foreach (T entity in entities)
            Delete(entity);
    }

    public void Update(T entity)
    {
        _dbContext.Entry(entity).State = EntityState.Modified;
    }

    public void Save()
    {
        _dbContext.SaveChanges();
    }

    public async void SaveAsync()
    {
        await _dbContext.SaveChangesAsync();
    }

    public void Dispose()
    {
        if (_dbContext != null)
            _dbContext.Dispose();
    }
}

DISCLAIMER I don't believe in wrapping EF with a repository. Entity Framework is a unit-of-work and repository implementation.

  I've changed IEnumerable to IQueryable.

You've just destroyed the usefulness of your repository, and leaked Linq-to-Entities to your clients: it doesn't matter what predicate you give GetWhere, the client code can now just say GetWhere().Where(x => whatever);, and the client code now controls when (execution is deferred until the result is iterated) and how the query will run on the database server... and that defeats the purpose of a repository.

What you had before was much more robust.

One of the answerers of your other question said In general, use IQueryable over IEnumerable in your repository. - that did not mean to expose IQueryable.


Run control tasks asynchronously using TPL One of my classes regularly triggers one of two asynchronous operations: a "turn on" and a "turn off" that each take about a second. I'm using this method below to make sure that they don't run simultaneously. It doesn't seem very elegant to me. Is there some better way to do this? I'm using .NET 4.0, but I would like to know if there are any .NET 4.5 methods that make this easier.

private Task _changingControl;
private void RunControlTask(Task newTask)
{
    var cc = _changingControl;
    if (cc == null || cc.IsCompleted)
    {
        _changingControl = newTask;
        newTask.Start();
    }
    else
    {
        _changingControl = cc.ContinueWith(old =>
        {
            newTask.RunSynchronously();
            newTask.Dispose(); // only needed if we manually wait for it to finish
        });
    }
}

First, some notes about your code:

Using Tasks that are first created and later started usually doesn't make much sense. To represent some work to be done, just use a delegate.
Tasks almost never need to be Disposed.

Now, I think that what you want to do would be easiest to achieve using ActionBlock from TPL Dataflow. When you want to perform the operation, you would send a message to the block and the action in the block would execute it. ActionBlock already performs all synchronization necessary to make sure only one operation executes at a time.

ActionBlock<bool> _changeControlBlock = new ActionBlock<bool>(ChangeControl);

private void ChangeControl(bool on)
{
    if (on)
    {
        // turn on
    }
    else
    {
        // turn off
    }
}

public void TurnOn()
{
    _changeControlBlock.Post(true);
}

public void TurnOn()
{
    _changeControlBlock.Post(false);
}


Return a substring containing a certain pattern to VBA I've written in a C++ dll the following function to return to VBA a substring containing a certain pattern, given an input string, which contains that substring:

VARIANT _stdcall ReturnT(VARIANT FileName)
{

    wstring str(FileName.bstrVal, SysStringLen(FileName.bstrVal));
    wregex ToFind(L"_[tT]\\d{2,3}(_|.)");
    wsmatch TStr;

    regex_search(str, TStr, ToFind);
    wstring TVal = TStr.str(0).erase(0, 2);
    TVal.erase(TVal.end() - 1);

    assert(!TVal.empty());
    BSTR bs = SysAllocStringLen(TVal.data(), TVal.size());

    VARIANT TValue;
    TValue.vt = VT_BSTR;
    TValue.bstrVal = bs;

    return TValue;
}

This works flawlessly, but I suspect I am doing something more complicated than it really should be.

Notice that I'm using VARIANT to avoid interop problems ( BSTR as input would return kanjis, and in output would return a string with spaces between the characters)

String Operations

regex_search provides various overloads for various input types. You don't need to copy the data into a wstring just to pass it to regex_search.

Since the return string is only a substring of the original, you can simply construct the BSTR from part of the match value, avoiding the erase calls.

ATL

Microsoft's ATL library provides a lot of helper classes for dealing with COM. They can eliminate a lot of boilerplate code.

Regex Library

I haven't used VBA lately but I expect something as common as regular expressions to already be available. If not, I would probably expose COM classes wrapping regex functionality to VBA and keep the actual expressions in VBA closer to the rest of the business code.


Parsing messages with fixed-width fields into Employee objects I'm writing a application that reads from a queue of messages from a legacy mainframe system.

Some characteristics of message in the queue:

Message from the Q is always fixed length plain text : 64 char length
Each index or group of index indicates some meaning full data
The first 20 chars represent first name, next 20 surname, character following that represents gender, then next 8 chars denote date in yyyyMMdd format

I need to map these to Java objects. Here is the sample of what I'm doing.

import lombok.Data;
import lombok.NoArgsConstructor;
import lombok.extern.slf4j.Slf4j;

import java.time.LocalDate;
import java.time.format.DateTimeFormatter;
import java.util.Date;

@Slf4j
public class Solution {

    public static void main(String[] args){
        String input = "JOE                BLOGG                M19880101PX2018010199PNM";
        log.info("Input length 64 = ",input.length());
        log.info(empMapper(input).toString());
    }

    public static boolean boolMapper(char value){
        return (value == 'Y') ? true:false;
    }

    public static LocalDate dateMapper(String value){
        final String dateString = String.format("%s-%s-%s",value.substring(0,4),value.substring(4,6),value.substring(6));
        final DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd");
        return LocalDate.parse(dateString, formatter);
    }

    public static Emp empMapper(String input){
        final Emp emp = new Emp();
        emp.setFirstName(input.substring(0,19).trim());
        emp.setSurName(input.substring(19,39).trim());
        emp.setGender(input.charAt(40));
        emp.setDob(dateMapper(input.substring(41,49)));
        emp.setEmpId(input.substring(49,61));
        emp.setJobType(input.charAt(61));
        emp.setShiftNeeded(boolMapper(input.charAt(62)));
        emp.setEmpLevel(input.charAt(63));
        return emp;
    }

    @Data
    @NoArgsConstructor
    public static class Emp{
        private String firstName;
        private String surName;
        private char gender;
        private LocalDate dob;
        private String empId;
        private char jobType;
        private boolean shiftNeeded;
        private char empLevel;
    }
}

  Output: Emp(firstName=JOE, surName=BLOGG, gender=M, dob=1988-01-01,
  empId=PX2018010199, jobType=P, shiftNeeded=false, empLevel=M)

My question is there a better solution for doing this from a performance point of view

Why is everything static? It's not necessarily wrong, but it may make integration into a larger project more ugly.

EDIT: Some background to this: A static utility class makes other classes that depend on it difficult to use and test. It basically breaks the principal called Inversion of control.

Instead of having a second class hard-code the reference to such a static "employee factory" like this, it is given an instance of a "employee factory class" (e.g. as a constructor argument). That way the "employee factory" can easily be replaced without touching the second class.

This is especially interesting when you want to test that second class. In that case you don't need to worry about setting up the real "employee factory" and making sure that it works. Instead you can give the second class a so called mock class, that just simulates the workings of the "employee factory" and, for example, just returns hard-coded, predetermined employees.

The conditional operator (? :) in boolMapper is unnecessary:

public static boolean boolMapper(char value){
    return value == 'Y';
}

As @gervais.b says, formatting the date string to include the hyphens is unnecessary. You can parse the date directly with the pattern yyyyMMdd. Also you shouldn't create a new DateTimeFormatter instance in each method call. Instead use an instance stored in a static constant field:

private final static DateTimeFormatter EMPLOYEE_DATE_FORMATTER = DateTimeFormatter.ofPattern("yyyyMMdd");

public static LocalDate dateMapper(String value){
    return LocalDate.parse(value, EMPLOYEE_DATE_FORMATTER);
}

Don't unnecessarily abbreviate variable/class names. Use Employee instead of Emp, use dateOfBirth instead of dob, etc.

Considering you are converting dob and shiftNeeded to "proper" Java types, you should consider also consider converting gender, jobType and empLevel, which you currently store as a chars, into something more Java-esk, eg. enumerations.


A Ruby implementation of derangement (secret santa) algorithm A secret santa gift exchange is a game where each player is randomly assigned a person to whom they anonymously give a gift. The algorithm is referred to as Derangement.

For example, given the following players:

  Mohamad, Carolina, Sami, Tania, Ikram, José

A game may be:

  Mohamad gets Sami
  Carolina gets José
  Sami gets Tania
  Tania gets Mohamad
  Ikram gets Carolina
  José gets Ikram

The important rules are:

A person can not be assigned to themselves.
A person can only be assigned to another person once.
Assignment must be random.

Implementation

This is my object oriented implementation. I do not like my random_player_for implementation. It seems procedural and the use of temporary flags seems like a code smell.

How would you improve this implementation?

class Derangement
  attr_reader :game

  # for convenience
  # PLAYERS = %w[Mohamad Carolina Sami Tania Jose Ikram Rabii Ahmad Kazem]

  def initialize(players)
    @players = players
    @santas = players.dup
    @game = {}
  end

  def solve
    @santas.each do |santa|
      @game[santa] = random_player_for(santa)
    end
  end

private

  def random_player_for(santa)
    found = false
    until found do
      player = @players.sample
      unless player == santa
        found = true
        @players.delete(player)
      end
    end
    player
  end
end

I'm a bit unsure about whether the question is about a game of Secret Santa or if it's actually about derangement, specifically. Derangement requires that no element can appear in its original position. But that's irrelevant for a game of Secret Santa, since there's no ordering to speak of.

Edit: As tokland rightly points out in the comments derangement is of course related to the game. I wasn't thinking it through.

For derangement, Rosetta Code has a Ruby implementation you might be able to use or learn from.

For a game of Secret Santa, though, the specs are pretty much as you summed up in your 3 rules (although #3 "Assignment must be random" is a bit difficult to define/verify.) So here, things can be simplified; you need not worry about derangement "by itself" as long as the rules are obeyed. For instance, you might simply do:

players = %w[Mohamad Carolina Sami Tania Ikram Jose].shuffle
players << players.first # repeat the first player

assignments = players.each_cons(2).to_a

which will give you an array of santa/gift-receiver pairs such as:

[["Carolina", "Ikram"],
 ["Ikram", "Mohamad"],
 ["Mohamad", "Jose"],
 ["Jose", "Tania"],
 ["Tania", "Sami"],
 ["Sami", "Carolina"]]

Alternatively, you can get the "Santa targets" for several rounds by doing something like

players = %w[Mohamad Carolina Sami Tania Ikram Jose].shuffle
assignments = players.each_with_index.map do |santa, index|
  others = players.rotate(index+1)[0...-1]
  [santa, others]
end

Which will give you something like

[["Tania", ["Mohamad", "Sami", "Ikram", "Carolina", "Jose"]],
 ["Mohamad", ["Sami", "Ikram", "Carolina", "Jose", "Tania"]],
 ["Sami", ["Ikram", "Carolina", "Jose", "Tania", "Mohamad"]],
 ["Ikram", ["Carolina", "Jose", "Tania", "Mohamad", "Sami"]],
 ["Carolina", ["Jose", "Tania", "Mohamad", "Sami", "Ikram"]],
 ["Jose", ["Tania", "Mohamad", "Sami", "Ikram", "Carolina"]]]

There's a pattern of course (if you've given Ikram a gift, next you'll be giving Carolina a gift, then José, etc.). But if it's Secret Santa, the players shouldn't be able to figure that out anyway unless they cheat (and if they do, well, all bets are off).

You could of course use Array#combination or Array#permutation to achieve similar results. I highly encourage you to check out all of the built-in array methods, and those included from the Enumerable module. There's a lot of good stuff there.

As for a more OOP approach, I wouldn't make a class called "Derangement". Derangement is a method. It's an action, an operation, a means of achieving a certain result or state - not something that is itself stateful.
The simple solution, given your code, is to rename you class to "Game" or something along those lines.


Updating an auto-generated invoice (future) when current plan changes I've built-out a mechanism to go and find an auto-generated invoice and update it with new information when a plan is changed or if users are added to the plan for the current billing period. It will make it a lot easier than having to remember to change it a month, quarter, 6 months, or even a year from now. And this also reduces the burden on the finance department from having to go find the future invoice and manually updating it. I hope my comments in the code are clear.

I'm mainly looking for feedback on my code. It does work. I will need to write tests for this though.

Model

def new_amount(max_users, item)
  amount = ((max_users.to_i * item.amount) / item.quantity)
  # Returns the new adjusted amount
  case term_type
  when 'month'      then amount / 12
  when 'quarter'    then amount / 4
  when 'semiannual' then amount / 2
  when 'annual'     then amount / 1
  else                   amount
  end
end

Controller

  # Finds the upcoming / unpaid system invoice.
  manual_invoice = @subscription.manual_invoices.system.not_paid.latest.first

  # TODO: This is very hacky. If I use dup or clone on manual_invoice, and
  #   when change_plan is executed, the calculation runs, regardless if
  #   there were any updates. Also, chaining these scopes brings back all
  #   of the records when using .latest... so, I end up with an invoice
  #   where paid_at is nil.
  first_manual_invoice = @subscription.manual_invoices.system.paid.first

  # Guards changing of plan from blowing up if manual_invoice.nil?
  if manual_invoice
    # Iterate through the invoice's items.
    first_manual_invoice.items.each do |item|
      # Set new_amount by taking the new max_users quantity and multiply it
      #   by the previous amount. Then divide it by the previous max_users
      #   quantity. Finally, divide by payment frequency per term.
      new_amount = @subscription.new_amount(max_users, item)
      # Update the manual_invoice items with the calcuated new_amount and
      #   user quantity.
      manual_invoice.items.first.update_attributes(quantity: max_users,
                                                   amount: new_amount)
      # Update the manual_invoice.
      manual_invoice.update_attributes(amount_in_cents: new_amount,
                                       term_type: term_type,
                                       term_start_date: term_start_date,
                                       term_end_date: term_end_date,
                                       due_on: term_end_date)
    end
  end

Your code is very readable. Given the provided snippets of code, I won't focus this review on overall architecture (as I'm not sure what the entire system does). I'd like focus on two areas of this code: comments and method names.

When reading your comments I asked my self the following question (taken directly from Code Complete 2).

Do comments explain the code's intent or summarize what the code does, rather than just repeating the code?

# Returns the new adjusted amount
# Iterate through the invoice's items.
# Update the manual_invoice.

These comments repeat what your code does. Removing them would improve the readability and reduce the length of your code.

Next, the following comment and method invocation:

# Set new_amount by taking the new max_users quantity and multiply it
#   by the previous amount. Then divide it by the previous max_users
#   quantity. Finally, divide by payment frequency per term.
new_amount = @subscription.new_amount(max_users, item)

A 3 line comment before a method call such as this is a great opportunity to rename your method name to be more descriptive and possibly avoid a comment altogether. Here is an idea of a possible name:

adjusted_amount = @subscription.adjust_amount(max_users, item)

What other method names would better describe what this method does? Could you move this method to another Model?

Don't be afraid to use 3 or more words to describe a method if that's what it takes. Better to have a long method name instead of a 3-line comment.

I believe you could improve these two lines:

# Guards changing of plan from blowing up if manual_invoice.nil?
if manual_invoice

By replacing them with one line:

unless manual_invoice.nil?

Finally, inside the method new_amount I believe referencing model attributes with self would the improve the readability of your code by differentiating parameters from model attributes.

case self.term_type
# ...
end


Download and print reddit page which can be redirected to file I wanted to save the a reddit thread to a .org file. I noticed that it is easier to use pandoc to convert .md to .org (esp when it comes to list items).
My code prints the output to the screen which can be redirected to the required file.
Process is as follows.
python reddit_download.py -u <url_to_download> > myfile.md
pandoc -s myfile.md -o myfile.org

My script
reddit_config.py
class Config:
    client_id = 'myID'
    client_secret = 'mySecret'
    password = 'my_password'
    user_agent = 'my_user_agent'
    username = 'my_username'

reddit_download.py
import argparse
import praw
from reddit_config import Config

def reddit():
    red = praw.Reddit(client_id=Config.client_id,
                      client_secret=Config.client_secret,
                      user_agent=Config.user_agent,
                      username=Config.username,
                      password=Config.password)
    return red

def parse_args():
    parser = argparse.ArgumentParser()
    parser.add_argument('-u', '--url', required=True, help='URL required', dest='url')
    return parser

def get_full_page(reddit_instance, url):
    post = reddit_instance.submission(url=url)

    page = [{
        # content of post
        "id": post.id,
        "parent": post.id,
        "body": post.selftext,
        "author": post.author.name,
        "ups": post.ups,
        "title": post.title
    }]

    # all comments/replies
    list_of_comments = post.comments.list()
    list_of_comments_with_desired_attributes = [{
            "id": comment.id,
            "parent": comment.parent_id.replace('t1_', '').replace('t3_', ''),
            "body": comment.body,
            "author": comment.author.name,
            "ups": comment.ups
        }
        for comment in list_of_comments
    ]

    page += list_of_comments_with_desired_attributes

    return page

def lookup(loc):
    comment_lookup = dict()
    for comment in loc:
        comment_id = comment["id"]
        if comment_id != comment["parent"]:
            parent_id = comment["parent"]
            children = comment_lookup.get(parent_id)
            if not children:
                children = comment_lookup[parent_id] = list()
            children.append(comment.copy())
        else:
            parent = comment.copy()
    return parent, comment_lookup

def hierarchy(parent_element, comment_lookup):
    parents = comment_lookup.get(parent_element["id"], list())
    for parent in parents:
        hierarchy(parent, comment_lookup)
    if parents:
        parent_element['children'] = parents
    return parent_element

def org(post_dict, parent_id, visited=None, depth=1):
    if visited is None:
        visited = set()
    visited.add(parent_id)
    print('#'*depth, post_dict.get('ups'), post_dict.get('author'), end='\n\n')
    title = post_dict.get('title')
    if title is not None:
        print(post_dict.get('title', ''), end='\n\n')
    print(post_dict.get('body'), end='\n\n')
    children = post_dict.get('children')
    if children is not None:
        for child in children:
            if child.get('id') not in visited:
                org(child, child.get('id'), visited, depth+1)
    return ''

if __name__ == '__main__':
    parser = parse_args()
    args = parser.parse_args()
    reddit = reddit()
    page = get_full_page(reddit, args.url)
    parent, comment_lookup = lookup(page)
    post = hierarchy(parent, comment_lookup)
    post_id = post.get('id')
    org(post, post_id)

Is there a way to improve this code? I am confused how to remove the print statements and build a string to write to a file. Effectively I don't have to redirect the output in shell to a file.

Configuration
Your reddit_config.Config needs to be changed so that, rather than hard-coded strings in a Python module, these are externalized to environment variables, or a permissions-restricted configuration file.
List construction alternatives
There are some alternatives to your concatenate-and-return in get_full_page: you can either make this a generator function -
    yield {
        # content of post
        "id": post.id,
        "parent": post.id,
        "body": post.selftext,
        "author": post.author.name,
        "ups": post.ups,
        "title": post.title
    }
    # ...
    yield from (
        {
            "id": comment.id,
            "parent": comment.parent_id.replace('t1_', '').replace('t3_', ''),
            "body": comment.body,
            "author": comment.author.name,
            "ups": comment.ups
        }
        for comment in list_of_comments
    )

or you can star-expand -
return [
    {
        # content of post
        "id": post.id,
        "parent": post.id,
        "body": post.selftext,
        "author": post.author.name,
        "ups": post.ups,
        "title": post.title
    },
    *(
        {
            "id": comment.id,
            "parent": comment.parent_id.replace('t1_', '').replace('t3_', ''),
            "body": comment.body,
            "author": comment.author.name,
            "ups": comment.ups
        }
        for comment in list_of_comments
    ),
]

Truthy lists
Your
if not children

is too broad. This statement will pass if children == [], which it doesn't need to - instead, consider
if children is None:

or better yet, replace this whole mechanism with a defaultdict.
Unused return
org() returns '', which both has no meaning and is unused, so you can just delete that line.