Tail Recursion for Sum - Elixir I'm new to both Elixir and tail recursion.

defmodule MyInteger do
  defp sum_upto(x, accumulator) when accumulator <= 1 do
    accumulator
  end

  defp sum_upto(x, accumulator) when accumulator > 1 do
    sum_upto(x, accumulator-1) + accumulator
  end

  def sum_upto(x) when x <= 1 do
    x
  end

  def sum_upto(x) do
    sum_upto(x, x-1) + x
  end
end

Is the above an optimal way to calculate the sum up to x?

Overview

I see that there are some good comments and I thought I'd just explain in a full answer block -- I'm not on here too often so I apologize for the tardiness of the response.  Bear with me as I first show you what I might really do then demonstrate the answer to your tail recursive question using the same terms with which you posed your question.

What I Would Do

First, I think you are asking how to add up the numbers 1,2,3,...,x.

I would actually use the Enum.reduce function:

1..20 |> Enum.reduce(0, fn(x,acc) -> acc + x end)

or using Elixir capture syntax which is more concise and obtuse:

1..20 |> Enum.reduce(0, &(&1 + &2))

The Main Question

Now to the crux of your actual question, how would I do this without using Enum and using tail recursion.  First note that the last entry in the function call will be the function itself.  There must be no other add, sub, multiply, -- operation of any kind before or after the function call.  You can change the parameters but don't do anything outside call itself.

Here is a simple tail recursive routine that adds numbers up:

defmodule MyInteger do
  def sum_to_x(0, acc) do
    acc
  end

  # initial function call
  def sum_to_x(x, acc) do
    sum_to_x(x - 1, acc + x)
  end
end

IO.puts MyInteger.sum_to_x(20, 0)

I wrote this counting down to make it like your example.  When you first call MyInteger.sum_to_x(20, 0) the second form of the sum_to_x function is used.  We pass 0 as the initial value of the accumulator. The last line in that function calls itself and nothing else (tail recursive).  That is when the function finally "returns" there are no other ops to do.  This function will call itself counting x down until x is zero.  When is is zero, we match the first form of the function since we have a "0" in the first parameter.  This form just returns the accumulated result --

uDude


Square root calculation in Scheme (SICP Exercise 1.7) I have done exercise 1.7 in SICP (calculate square root precision when change in guesses is under a certain value), but I am calling the change-in-precision function twice in each iteration, which disturbs me. I wonder if there is a better way to implement this.

(define (average . ns) (/ (apply + ns) (length ns)))

(define (change-in-precision guess x)
  ( - (- guess (average guess (/ x guess)))))

(define (sqrt guess x)
  (if (< (abs (change-in-precision guess x)) (/ 0.00000001 guess))
  (+ guess (change-in-precision guess x))
  (sqrt (+ guess (change-in-precision guess x)) x)))

In change-in-precision, you can avoid the negation by swapping the operands for the subtraction.

In sqrt, if the guess is already close enough, why not just return guess?

To eliminate the repeated call to change-in-precision, use a let to define a variable delta.

The two branches of the if should be indented.


Rails: Setting a transient attribute on a set of objects from one model based on information from a junction model I have a user model, a task model, and a junction model user_task that saves the data joining the two:

class Task < ActiveRecord::Base
  attr_accessor :completed_on
  has_many :user_tasks

class UserTask < ActiveRecord::Base
  attr_accessible :task_id, :user_id, :completed_on
  belongs_to :user
  belongs_to :task

class User < ActiveRecord::Base
  has_many :user_tasks, :dependent => :destroy
  has_many :tasks, :through => :user_tasks

In order to query the tasks and show if they're complete or not when a user is logged in, I have a transient attribute, (I think that's the correct terminology) an attr_accessor on the task model :completed_on. Right now I'm looping through the tasks as follows in order to get this attribute filled out.

This just thrown in a controller as a quick hack to provide what I need.

  @tasks.each do |task|
    if current_user.tasks.include? task
      task.completed_on = current_user.user_tasks.find_by_task_id(task.id).completed_on
    end
  end

I need something that, given an array of tasks and a user_id, loops through the tasks and fills that variable with the completed_on date if the user has finished that task. The goal for this is that when responding to JSON, I have overriden as_json on the task model to merge the completed_on attribute into the JSON hash:

h.merge({:completed_on => completed_on})

Possible ideas to get this done more properly:

Create a function on the tasks model that takes a user ID, joins the
user_tasks table to the tasks table, with a where clause to specify
the user. This idea is actually half baked and I'm not sure where to
go from here.
Pretty much put the code above into the user model and    change
current_user to self. I'm not sure what to name it though,
completed_tasks doesn't fit since not all of them would be completed.
fill_completed_on may work.
Utilize a decorator somehow? Is it worth    it just for this single
attribute? If so, how would I structure it?
Something like Task.where(conditions) then Task.where(conditions).completed_by(user) then setting the completed_on on the latter and merging the two results.

It feels like I'm doing this the a bad way, and could be leveraging ActiveRelation/scopes somehow.

I also think there is a better way to do include completed_on in when needed and not have to override the task model's as_json to get it to appear.

The problem is that the data to render does not match the modelling. You call it "task", but if it includes a completed_on it's not a task, it's something more of a user_task. I definitely wouldn't use an accessor to accomplish this, that's messing with Task, this model should know nothing about completion dates of a particular user.

Even if you call it "tasks" in the frontend, conceptually in your app you should be rendering a collection of user_tasks. Something to start with (make sure, using includes or whatever, that user_task_by doesn't hit the DB repeatedly):

user_tasks = Task.where(...).includes(:users_tasks).map do |task|
  task.user_task_by(user) || task.user_tasks.new
end

Now you'd override UserTask#as_json to build a hash with the mixed structure (getting some attributes from task and completed_on from user_task).

Was this any help?


Matrix input in N*N form in Python I am using the below code in Python to input a N*N matrix in its original form:

n = int(input())
l=[]
for i in range(n):
    x=input().split()
    c=[]
    for j in range(n):
        c.append(int(x[j]))
    l.append(c)

print(l)

Here's the output:

2 #input n

1 3 #input row

1 5. # input another row

[[1, 3], [1, 5]] # final matrix

How can this code be improved? Is there a better way to take a matrix as input?

Since the number of rows is to be equal to the number of columns, you can simply get the number of columns from the first input and stop taking input when the user enters the same number of rows:

l = []
while not l or len(l) < len(l[0]):
    l.append(list(map(int, input().split())))

so that given an input of:

2 3 4
1 2 3
5 6 7

l would become:

[[2, 3, 4], [1, 2, 3], [5, 6, 7]]


Evaluation of e^x with series expansion My code calculates \$e^x\$ with series expansion. Is there any way to make it shorter and cleaner?

#lang racket
; evaluate e^x with series expansion, use first 10 terms.

(define (exp x)
  (letrec ([f1 (lambda (n cx cn)
                 ;(writeln (list n cx cn))
                 (if (>= n 10)
                     0
                     (+ (/ cx cn) (f1 (+ n 1) (* cx x) (* cn (+ n 1))))))])
    (+ 1 (f1 1 x 1))
    )
  )

(letrec ([N (read)]
         [rd (lambda (n)
               (unless (zero? n)
                       (define x (read))
                       (writeln (exp x)) (rd (- n 1))))])
  (rd N))

General Programming

Using built the built in exponent function from racket/base and referencing Euler's Number from math/base would probably reduce the likelihood of bugs; increase speed; and allow programming at a higher level of abstraction.

An alternative to read is the GUI function get-text-from-user in racket/gui/base.

Example Code
#lang racket

(require (only-in math/base
                  euler.0)
         (only-in racket/gui/base
                  get-text-from-user))

(define (get-power-of-e)
  (define x (string->number
             (get-text-from-user "Power of e"
                                 "provide exponent")))
  (expt euler.0 x))


Rails Calendar Generation Method I have this helper method that generates an HTML calendar based on the current day:

module PageHelper
  def calendar(date)
    cal = "

"
    days = %w(Sun Mon Tue Wed Thu Fri Sat)
    days.each do |day|
      cal += "#{day}	"
    end
    cal+= '

'
    #end of table head
    first_day = date.at_beginning_of_month.strftime('%w').to_i
    last_day = date.at_end_of_month.strftime('%d').to_i
    last_day_week = date.at_end_of_month.strftime('%w').to_i
    current_day = date.strftime("%d").to_i
    prev_month_day = (date - 1.month).at_end_of_month.strftime('%d').to_i
    # print first row
    first_day.times do |day|
      num = prev_month_day - first_day + day + 1
      cal += "#{num}	"
    end
    (7-first_day).times do |day|
      if day == current_day -1
        cal += "#{day+1}	"
      else
        cal += "#{day+1}	"
      end
    end
    # print remaining rows
    cal += '
'
    (7-first_day).upto(last_day-1).each_slice(7) do |slice|
      cal += '
'
      slice.each do |day|
        if day == current_day -1
          cal += "#{day+1}</day>"
        else
          cal += "#{day+1}	"
        end
      end
      cal += '
' unless slice.last == last_day -1
    end
    # get remaining number of cells
    rem_cells = 6 - last_day_week
    if rem_cells && rem_cells != 6
      rem_cells.times do |day|
        cal += "#{day+1}	"
      end
    end
    cal += '

'
  end
end

The method is called like this:

<%=  calendar(Date.today).html_safe %>

Here is an example calendar:

However, I think that my code is not very DRY and the method is unnecessarily longer than what it should be.

I agree that it is too long. I find a good place to start is to try breaking it up into methods that are short and each do a single thing. While I'm doing this I usually see opportunities to make things DRY.

My code might look like:

content_tag(:table, ...) do
  header_row + week_rows
end

def week_rows
  num_rows = (first_day + last_day) % 7
  rows = num_rows.times.map { |i| week_row(i) }
  content_tag(:tbody) do rows.join('')
end

A couple of things I noticed:

Instead of hard-coding html like "<table class='table-condensed table-bordered table-striped'><thead><tr>" use the Rails content_tag helper
Instead of date.at_end_of_month.strftime('%w').to_i use the wday method. There is also a day method that you can use instead of %d.
Have your method return a string that is already html_safe


Python implementation of Conway's Game of Life For learning purpose I've started creating my implementation of Conway's Game of Life. I've used numpy to store big array, contating dead and alive cells, then I've apllied Conway's rules, to create mechanics of cells life. To manage grid, and graphics I used pygame module. After many reviews, and rewriting code in many ways, I can't find out what's wrong with it, so I decided to ask you. For example I've tried to make a glider, (as code shows), but he dies after 3 loop cycles. I'd be appreciate for help and tips. Can you help me find out why the cells aren't reproducing? Thanks in advance. Code:

import pygame
import numpy as np

BLACK = (0, 0, 0)
GREEN = (0, 255, 0)
N = 195
WIDTH = 10
HEIGHT = 10

grid = np.zeros(shape=(N, N), dtype=np.int32)
glider = np.array([[0, 0, 1],
                       [1, 0, 1],
                       [0, 1, 1]])
grid[3:6, 3:6] = glider

pygame.init()

WINDOW_SIZE = [980, 980]
screen = pygame.display.set_mode(WINDOW_SIZE)

pygame.display.set_caption("GAME OF LIFE")

done = False

clock = pygame.time.Clock()

while not done:
    for event in pygame.event.get():
        if event.type == pygame.QUIT:
            done = True
    for row in range(N):
        for column in range(N):
            color = BLACK
            if grid[row][column] == 1:
                color = GREEN
            pygame.draw.rect(screen, color,
                             [WIDTH * column,
                              HEIGHT * row,
                              WIDTH,
                              HEIGHT])

    newGrid = grid.copy()
    for i in range(N):
        for j in range(N):
            total = grid[(i-1) % N:(i+1) % N, (j-1) % N:(j+1) % N].sum() - grid[i, j]
            if grid[i, j] == 1:
                if(total < 2) or (total > 3):
                    newGrid[i, j] = 0
                else:
                    if total == 3:
                        newGrid[i, j] = 1
    grid = newGrid

    clock.tick(60)
    pygame.display.flip()

pygame.quit()

I think you have a couple of subtle bugs in the way you implement Conway's rules.
See my comments in the code for details:

for i in range(N):
    for j in range(N):
        # I changed the range of the extent of the sum, note +2 rather than +1 !
        total = grid[(i-1) % N:(i+2) % N, (j-1) % N:(j+2) % N].sum() - grid[i, j]
        if grid[i, j] == 1:
            if(total < 2) or (total > 3):
                newGrid[i, j] = 0
            # allow for survival in case of total = 2 or 3
            else:
                newGrid[i, j] = 1
        # allow for reproduction if cell is empty
        else:
            if total == 3:
                newGrid[i, j] = 1

With these edits the glider should glide :)


Permuting a typelist Given a typelist, return a typelist of all the permutations of it. For example:

using Types = typelist<char, int, double>;
using Permutations = typelist<
    typelist<char, int, double>,
    typelist<char, double, int>,
    typelist<int, char, double>,
    typelist<int, double, char>,
    typelist<double, char, int>,
    typelist<double, int, char>
    >;

static_assert(std::is_same<permute_t<Types>, Permutations>{}, "!");

There are two different ways of writing metafunction "overloads": partial specialization or function overloading. I'm still unsure of which is "better", and in this solution I ended up using both, which strikes me as questionable.

The algorithm here is: the permutations of a list is the concatenation of (each element in the list prepended to all the permutations of the list with that element removed).

template <class...> struct typelist { };
template <class T> struct tag { using type=T; };

// remove first instance of type from a typelist
template <class T, class... Ts, class... Rs>
auto remove_first(tag<T>, typelist<T, Ts...>, typelist<Rs...>)
    -> tag<typelist<Rs..., Ts...>>;

template <class T, class F, class... Ts, class... Rs>
auto remove_first(tag<T>, typelist<F, Ts...>, typelist<Rs...>)
    -> decltype(remove_first(tag<T>{}, typelist<Ts...>{}, typelist<Rs..., F>{}));

template <class T, class TL>
using remove_first_t = typename decltype(remove_first(tag<T>{}, TL{}, typelist<>{}))::type;

// concatenate lots of typelists into one typelist
template <class TL>
auto concat(TL ) -> tag<TL>;

template <class... A, class... B, class... Cs>
auto concat(typelist<A...>, typelist<B...>, Cs... cs)
    -> decltype(concat(typelist<A..., B...>{}, cs...));

template <class... Ts>
using concat_t = typename decltype(concat(Ts{}...))::type;

// prepend a type onto a typelist of typelists
template <class T, class... TLs>
auto prepend(tag<T>, typelist<TLs...>)
    -> tag<typelist<concat_t<typelist<T>, TLs>...>>;

template <class T, class TL>
using prepend_t = typename decltype(prepend(tag<T>{}, TL{}))::type;

// get all the permutations of a typelist
template <class T, class=void>
struct permute : tag<typelist<T>> { };

template <class T>
using permute_t = typename permute<T>::type;

template <class... Ts>
struct permute<typelist<Ts...>, std::enable_if_t<(sizeof...(Ts)>1)>>
    : tag<
        concat_t<
            prepend_t<
                Ts,
                permute_t<remove_first_t<Ts, typelist<Ts...>>>
            >...
        >
    >
{ };

You might have some bug in the algorithm.

"testdata"

struct A {};
struct B {};
struct C {};
struct D {};

using Types = typelist<A, B, C, D>;
using Permutations = typelist<
    typelist<A,B,C,D>,
    typelist<A,B,D,C>,
    typelist<A,C,B,D>,
    typelist<A,C,D,B>,
    typelist<A,D,B,C>,
    typelist<A,D,C,B>,
    typelist<B,A,D,C>,
    typelist<B,A,C,D>,
    typelist<B,C,D,A>,
    typelist<B,C,A,D>,
    typelist<B,D,C,A>,
    typelist<B,D,A,C>,
    typelist<C,A,B,D>,
    typelist<C,A,D,B>,
    typelist<C,B,A,D>,
    typelist<C,B,D,A>,
    typelist<C,D,A,B>,
    typelist<C,D,B,A>,
    typelist<D,A,C,B>,
    typelist<D,A,B,C>,
    typelist<D,B,C,A>,
    typelist<D,B,A,C>,
    typelist<D,C,B,A>,
    typelist<D,C,A,B>
>;
static_assert(std::is_same<permute_t<Types>, Permutations>{}, "!");

results in:

prog.cpp:90:1: error: static assertion failed: !
 static_assert(std::is_same<permute_t<Types>, Permutations>{}, "!");
 ^


Recursive Regular Expressions I want to first search for a specific regular expression.
If it is not found then I would like to search for another regular expression
If that one is also not found then I would like to search for a third
(And so on...)

Whenever a matching regular expression is found, I want to return a String array containing:

The text before the match
The match itself
The text after the match

I've only come up with this horribly if-else nesting way of doing it. There's gotta be a better (more extensible and more clean) way but I can't come up with how...

Here is the code, it is currently only containing two regular expressions to search for and it's getting nasty already:

private static String[] findNumbers(String string) {
    Matcher match = Pattern.compile("\\d+[\\:\\.\\,]\\d+").matcher(string);
    if (match.find()) {
        String found = string.substring(match.start(), match.end());
        return new String[]{ string.substring(0, match.start()),
            found, string.substring(match.end()) };
    }
    else {
        match = Pattern.compile("\\d{3,}").matcher(string);
        if (match.find()) {
            String found = string.substring(match.start(), match.end());
            return new String[]{ string.substring(0, match.start()),
                found, string.substring(match.end()) };
        }
        else {
            // Another if-else could be added here
            return new String[]{ string, "", "" };
        }
    }
}

@Test
public void test() {
    assertArrayEquals(new String[]{ "ABC ", "2000", " DEF" }, findNumbers("ABC 2000 DEF"));
    assertArrayEquals(new String[]{ "42 ABC ", "2000", " DEF" }, findNumbers("42 ABC 2000 DEF"));
    assertArrayEquals(new String[]{ "42 ABC ", "18:47", " DEF" }, findNumbers("42 ABC 18:47 DEF"));
}

I want to put priority to one regular expression before trying the next, that's why I have written this code.

Nesting is not necessary, you can iterate instead...

But first:

naming the input String string can be confusing. Of course it's a string, what is it used for though?
there is no need to escape the values inside the [...] construct. the pattern: "\\d+[:.,]\\d+" is just fine, and more readable.
you should be pre-compiling and then reusing the patterns, instead of re-compiling them every time.

Consider the following:

private static final Pattern[] patternorder = {
        Pattern.compile("\\d+[:.,]\\d+"),
        Pattern.compile("\\d{3,}")
};

private static String[] findNumbers(String input) {
    for (Pattern pat : patternorder) {
        Matcher match = pat.matcher(input);
        if (match.find()) {
            return new String[] {
                input.substring(0, match.start()),
                input.substring(match.start(), match.end()),
                input.substring(match.end()),
            };
        }
    }
    return new String[]{input, "", ""};
}


remove elements in one list according to the conditions on corresponding elements of another list I have two lists that have the same length, lstA, lstB, I want to remove elements that are not good based on some conditions, but at the meantime, I also want to remove the elements in lstB that have the same position with the unwanted one in lstA. like below:

lstA = ['good','good2', '']
lstB = [1,2,3]

I want '' in lstA and 3 in lstB removed:

results:
lstA = ['good','good2']
lstB = [1,2]

Currently, I am doing this:

lstB = [lstB[i] for i in range(len(lstB)) if lstA[i] != '']
lstA = [st for st in lstA if st != '']

Want a more elegant and pythonic way of doing this

Rather than two comprehensions, it'd be simpler to just use one for loop. Make new_list_a and new_list_b start empty, and if the item is good add to both at the same time. This is simpler to understand, as you see clearly that both are being filtered by the same thing.

new_list_a = []
new_list_b = []
for a, b in zip(list_a, list_b):
    if a != '':
        new_list_a.append(a)
        new_list_b.append(b)

However, it looks like you'd be better served if you only create one list. This means you can have a tuple of (a, b). And so could implement something like:

[(a, b) for a, b in zip(list_a, list_b) if a != '']


Get every information about the Reservation I'm developing a plugin for Revit (a software to make 3D buildings).

The goal is simple to understand. When there is an intersection between a Wall and a Duct I create an object called Reservation at this location. I need to extract the Curve of the Ducts and the Faces of the Walls in order to calculate this intersection.

My algorithm is working fine and fast with a small building (3 Ducts, 10 Walls and 8 intersections) But when I want to launch it on a real project (around 10 000 Ducts) the code is way too slow due to many ForEach loops. Here is the sample which cause the issue:

foreach (Duct d in ducts)
            {
                Curve ductCurve = FindDuctCurve(d);
                curves.Add(ductCurve);

                foreach (Wall w in walls)
                {
                    wallFaces = FindWallFace(w);
                    foreach (Curve c in curves)
                    {
                        foreach (Face f in wallFaces)
                        {
                            foreach (KeyValuePair<XYZ, Wall> pair in FindInterWalls(c, f, walls))
                            {
                                Reservation.Res res = new Reservation.Res();
                                res.RoundCenter = new XYZ(Math.Round(pair.Key.X), Math.Round(pair.Key.Y), Math.Round(pair.Key.Z));
                                res.WallWidth = pair.Value.Width;
                                bool containsItemX = resList.Any(itemX => itemX.RoundCenter.DistanceTo(res.RoundCenter) < res.WallWidth + 1);

                                if (containsItemX == false)
                                {
                                    res.AssociatedWall = pair.Value;
                                    res.Radius = 1;
                                    res.AssociatedDuct = d;

                                    res.Center = pair.Key;
                                    resList.Add(res);
                                    model.Reservations.Add(new Reservation { ResList = resList });
                                }
                            }
                        }
                    }
                }
            }

The custom methods I'm using also contain loops. I wonder if a LINQ would be faster but I don't really know how to use it.

In a nutshell i want to get all the information about Reservations without loosing so much time stuck in so many foreach loops.

Here is my Reservation Class :

public sealed class Reservation
{

    public List<Res> ResList { get; set; }

    public class Res
    {
        public XYZ Center { get; set; }
        public XYZ RoundCenter { get; set; }
        public Duct AssociatedDuct { get; set; }
        public Wall AssociatedWall { get; set; }
        public double WallWidth { get; set; }
        public int Radius { get; set; }
    }

    public Reservation()
    {
        ResList = new List<Res>();

    }

}

A Curve is a right in the center of a Duct (each Duct contains one Curve). And a Face is a side of a Wall (each Wall contains 6 Faces)

I hope I have understood this now.

Instead of iterating over all the Duct items and adding each iteration the related Curve to curves you should create another class like

public class DuctCurev
{
    public Duct TheDuct {get; private set; }
    public Curve TheCurve {get; private set; }
    public DuctCurve(Duct duct, Curve curve)
    {
        TheDuct = duct;
        TheCurve = curve;
    }
}

now we iterate once over all of the Duct's and find the related Curve which we will add to a List<DuctCurve> like so

List<DuctCurve> ductCurves = new List<DuctCurve>();
foreach (Duct d in ducts)
{
    ductCurves.Add(d, FindDuctCurve(d));
}

then we need to adjust the remaining code to use the ductCurves and use the ! operator instead of using containsItemX == false like so

foreach (Wall w in walls)
{
    wallFaces = FindWallFace(w);
    foreach (DuctCurve dc in ductCurves)
    {
        foreach (Face f in wallFaces)
        {
            foreach (KeyValuePair<XYZ, Wall> pair in FindInterWalls(dc.Curve, f, walls))
            {
                Reservation.Res res = new Reservation.Res();
                res.RoundCenter = new XYZ(Math.Round(pair.Key.X), Math.Round(pair.Key.Y), Math.Round(pair.Key.Z));
                res.WallWidth = pair.Value.Width;
                bool containsItemX = resList.Any(itemX => itemX.RoundCenter.DistanceTo(res.RoundCenter) < res.WallWidth + 1);

                if (!containsItemX)
                {
                    res.AssociatedWall = pair.Value;
                    res.Radius = 1;
                    res.AssociatedDuct = dc.Duct;

                    res.Center = pair.Key;
                    resList.Add(res);
                    model.Reservations.Add(new Reservation { ResList = resList });
                }
            }
        }
    }
}


Robot Name - Exercise Robot Name

  Manage robot factory settings.

  When robots come off the factory floor, they have no name.

  The first time you boot them up, a random name is generated in the format
  of two uppercase letters followed by three digits, such as RX837 or BC811.

  Every once in a while we need to reset a robot to its factory settings,
  which means that their name gets wiped. The next time you ask, it will
  respond with a new random name.

  The names must be random: they should not follow a predictable sequence.
  Random names means a risk of collisions. Your solution must ensure that
  every existing robot has a unique name.

That was the instructions given to me.  The only constraints is the tests.  From my previous post I'm trying to improve on my naming conventions, DRY code, access modifiers, and keeping it readable.  Any advice would help me a lot since I'm learning C# and teaching myself.  Thank you for your help especially to those that follow my progress and help me.

using System;
using System.Collections.Generic;
using System.Linq;

public class Robot
{
    private static Random _random;
    private string _name;
    private static readonly HashSet<string> _nameList = new HashSet<string>();
    public string Name => _name;

    public Robot()
    {
        _random = new Random();
        _name = GenerateRandomName();
    }

    public void Reset()
    {
        _nameList.Remove(_name);
        _name = GenerateRandomName();
    }

    private static string GenerateRandomLetters() => new string(Enumerable.Repeat("ABCDEFGHIJKLMNOPQRSTUVWXYZ", 2)
      .Select(s => s[_random.Next(s.Length)]).ToArray());

    private static string GenerateRandomName()
    {
        string name;

        do
        {
            name = $"{GenerateRandomLetters()}{_random.Next(10)}{_random.Next(10)}{_random.Next(10)}";
        }
        while (_nameList.Contains(name));

        _nameList.Add(name);
        return name;
    }
}

using System.Collections.Generic;
using Xunit;

public class RobotNameTests
{
    private readonly Robot robot = new Robot();

    [Fact]
    public void Robot_has_a_name() => Assert.Matches(@"^[A-Z]{2}\d{3}$", robot.Name);

    [Fact]
    public void Name_is_the_same_each_time() => Assert.Equal(robot.Name, robot.Name);

    [Fact]
    public void Different_robots_have_different_names() => Assert.NotEqual(new Robot().Name, robot.Name);

    [Fact]
    public void Can_reset_the_name()
    {
        var originalName = robot.Name;
        robot.Reset();
        Assert.NotEqual(originalName, robot.Name);
    }

    [Fact]
    public void After_reset_the_name_is_valid()
    {
        robot.Reset();
        Assert.Matches(@"^[A-Z]{2}\d{3}$", robot.Name);
    }

    [Fact]
    public void Robot_names_are_unique()
    {
        var names = new HashSet<string>();
        for (int i = 0; i < 10_000; i++) {
            var robot = new Robot();
            Assert.True(names.Add(robot.Name));
        }
    }
}

The test are immutable.  They cannot be altered at all.  I did not write these test.  I have to work within the test.  I would be interested in learning on how to improve on them out of curiosity at best.

We can create 676000 robots with unique names, because we have 26 possibilities for each 'letter position' and 10 possibilities for 'number position' in our name. So calculation is 26 * 26 * 10 * 10 * 10 = 676000.

Why am I mentioning that? Because that number is not so big and maybe it's worth to consider pregenerating all possible values, keep them in pool and get/return when necessary. That's another approach. I don't want to say that it's better. It will be more memory consuming solution, but robot 'initialization' will be faster. Also it'll be easier to apply fix for point 6. Anyway, you should always get what suits your requirements.

End of digression - back to your code!

Why I can't see namespaces both in tests and in class? You copied it that way?
I understand why you set up Random as static, but it's not thread safe the way you implemented it. Reference: [JON SKEET ARTICLE]
Static readonly HashSet is very dangerous, because it's instance is shared between all threads and Hashset.Remove method could cause strange problems/exceptions in multithreaded environment. Of course no one is saying that in requirements, but I think that case should never be ignored. ConcurrentDictionary should be your choice in here, because it ensures thread safety. [MSDN]
Letters in alphabet will never change, so you can keep it as a const field. Thanks to that you'll avoid allocation string for each time somebody tries to generate random letters.
In case for readability I have nothing to add it's pretty clear what's happening here.
Last, but most important, comment from my side. A little challenge for you :) what will happen with your code when you create 676000 robots and try to create one more? Hint: you can use code from Robot_names_are_unique() test method, but increase upper limit of iterations to 676000.

Hope this helps!


Simple INI file parser in Haskell I've written a rudimentary parser for INI files:

{-# LANGUAGE OverloadedStrings   #-}

import qualified Data.Map           as M
import           Data.Maybe         (fromMaybe)
import qualified Data.Text          as T

type Ini = M.Map T.Text Section

data Section =
  Section
    { name       :: T.Text
    , properties :: M.Map T.Text T.Text
    }
  deriving (Show)

main :: IO ()
main = parseIni iniFilePath >>= \ini -> putStrLn $ "Parsed INI: " ++ show ini
  where
    iniFilePath = "/home/me/test.ini"

parseIni :: FilePath -> IO Ini
parseIni iniFilePath = parseToIni . T.pack <$> readFile iniFilePath

parseToIni :: T.Text -> Ini
parseToIni stringToParse =
  -- We return the parsed Ini, not the helper values
  firstOfTriple $
  foldr
    (\line (ini, currentSectionMaybe, lineIndex) ->
        -- We're at a new section start or the end of the file → add the previous section
        -- to the parsed Ini value and create a new section
      if isSectionHeader line || lineIndex >= length lines - 1
        then let updatedIni = addSection ini currentSectionMaybe
              in (updatedIni, Just $ Section (getSectionName line) M.empty, 1 + lineIndex)
        else (ini, updateSection currentSectionMaybe line, 1 + lineIndex))
    (M.empty, Nothing, 0) $
  -- Since foldr is right associative we would process the lines starting with the last one, that's
  -- why we reverse the list of lines
  reverse lines
  where
    lines :: [T.Text]
    lines = T.splitOn "\n" stringToParse

firstOfTriple :: (a, b, c) -> a
firstOfTriple (x, _, _) = x

parseProperty :: T.Text -> Maybe (T.Text, T.Text)
parseProperty line =
  case T.splitOn "=" line of
    [name, value] -> Just (T.strip name, T.strip value)
    _             -> Nothing

updateSection :: Maybe Section -> T.Text -> Maybe Section
updateSection sectionMaybe line = fmap updateSection' sectionMaybe
  where
    updateSection' :: Section -> Section
    updateSection' section =
      -- Add the property to the section if the property can be parsed.
      -- Otherwise, leave the section as it were
      maybe
        section
        (\(propName, value) -> Section (name section) (M.insert propName value (properties section)))
        (parseProperty line)

getSectionName :: T.Text -> T.Text
getSectionName line = fromMaybe line headerWithoutBracketsMaybe
  where
    headerWithoutBracketsMaybe = T.stripPrefix "[" line >>= T.stripSuffix "]"

isSectionHeader :: T.Text -> Bool
isSectionHeader line = T.isPrefixOf "[" strippedLine && T.isSuffixOf "]" strippedLine
  where
    strippedLine = T.strip line

addSection :: Ini -> Maybe Section -> Ini
addSection ini sectionMaybe = maybe ini (\section -> M.insert (name section) section ini) sectionMaybe

I'd love to get feedback on how to simplify the code and/or make it more readable.

Things I'm aware of and ok with at the moment:

The parser doesn't support comments
addSection could be eta-reduced
I don't use a parsing library like Parsec
I don't use lenses

I know you said you were fine with not using a parser combinator library like parsec, but I thought you might like to see the how the same thing might look using one, so I wrote an Attoparsec based parser for your data types:

{-# LANGUAGE LambdaCase        #-}
{-# LANGUAGE OverloadedStrings #-}

module IniParser where

import           Control.Monad               ( void )
import           Data.Attoparsec.Text        ( Parser
                                             , char
                                             , endOfInput
                                             , endOfLine
                                             , many'
                                             , many1
                                             , notInClass
                                             , parseOnly
                                             , satisfy
                                             , space
                                             )
import           Data.Map.Strict             ( Map )
import qualified Data.Map.Strict      as Map
import           Data.Text                   ( Text
                                             , pack
                                             )
import           System.Environment          ( getArgs )

type Ini = Map Text Section

data Section = Section
  { name       :: Text
  , properties :: Map Text Text
  } deriving (Show)

main :: IO ()
main = do
  [path] <- getArgs
  parseIniFile path >>= \case
    Right ini -> putStrLn $ "Parsed INI: " ++ show ini
    Left err  -> putStrLn $ "ERROR parsing ini: " ++ err

parseIniFile :: FilePath -> IO (Either String Ini)
parseIniFile iniFilePath = parseIni . pack <$> readFile iniFilePath

parseIni :: Text -> Either String Ini
parseIni = parseOnly ini

ini :: Parser Ini
ini = do
  defaultSection <- lexeme (Section "" <$> (Map.fromList <$> many' property))
  namedSections  <- lexeme (many' section)
  void $ endOfInput
  let allSections | null (properties defaultSection) = namedSections
                  | otherwise = defaultSection:namedSections
  pure . Map.fromList . map (\section -> (name section, section))
    $ allSections

section :: Parser Section
section = Section <$> sectionName <*> (Map.fromList <$> many' (lexeme property))

sectionName :: Parser Text
sectionName = char '[' *> sectionNameChars <* char ']' <* endOfLine

sectionNameChars :: Parser Text
sectionNameChars = pack <$> many' (satisfy $ notInClass "]\r\n")

property :: Parser (Text, Text)
property = (,) <$> propertyName <*> (lexeme (char '=') *> propertyValue)

propertyName :: Parser Text
propertyName = pack <$> many' (satisfy $ notInClass "=\r\n\t ")

propertyValue :: Parser Text
propertyValue = pack <$> many' (satisfy $ notInClass "\r\n")

lexeme :: Parser a -> Parser a
lexeme p = whitespace *> p <* whitespace

whitespace :: Parser String
whitespace = many' space

I think the main strength of the approach is self pretty self-evident.  It eliminates all the multi-line lambdas, the entire foldr, etc. which (at least IMHO) really obscures the essence of what the code is expressing.

Additionally I've restricted the use of qualified imports to a single one usage where I think it makes the code more readable, though your taste may vary.

You can see the whole stack based project here if you're interested.


Curious fractions in functional Ruby Problem 33 in Project Euler asks:

  The fraction 49/98 is a curious fraction, as an inexperienced
  mathematician in attempting to simplify it may incorrectly believe
  that 49/98 = 4/8, which is correct, is obtained by canceling the 9s.

  We shall consider fractions like, 30/50 = 3/5, to be trivial examples.

  There are exactly four non-trivial examples of this type of fraction,
  less than one in value, and containing two digits in the numerator and
  denominator.

  If the product of these four fractions is given in its lowest common
  terms, find the value of the denominator.

This is the code I came up with, which solves the problem pretty handily. But, I'm concerned that this is messy and unreadable, even with the comments - and I simply hate using magic numbers/indices littered throughout the code. Also, I do not want a procedural ("C" style) solution - I'm learning how to write idiomatic (read "functional") programming with Ruby, and I'd like to learn how to write cleaner functional code.

a = 10.upto(99).to_a.map(&:to_s)  # Create array of integers 10 - 99
puts a.product(a)                 # Form cartesion product [a,b] for all 2 digit numbers a,b
      .reject{|x| x.any? {|y| y.chars.include? '0'}}     # Numbers inlcuding '0' are the trivial matches
      .reject{|x| x.first == x.last ||                   # Reject pairs where a = b
                  x.first.to_f/x.last.to_f > 1  ||       # Reject fractions greater than 1
                  (x.first.chars & x.last.chars).size != 1} # Reject pairs in which no digits are common
      .map{|x| [x.first.to_f / x.last.to_f, x.first.chars, x.last.chars, x.first.chars & x.last.chars]} # Ex: [0.5, ["4", "9"], ["9", "8"], ["9"]]
      .map{|x| [x.first, x[1] - x.last, x[2] - x.last, x[1], x[2]]}   # Ex: [0.5, ["4"], ["8"],  ["4", "9"], ["9", "8"]]
      .select{|x| x.length == 5 && x.first == x[1].join.to_f/x[2].join.to_f} # Select only the tuples which match the condition
      .map{|x| Rational(x[-2].join.to_i, x[-1].join.to_i)}  # Convert each such tuple to rational numbers
      .reduce(:*)

Array multiple assignment
You pass arrays around, and you find yourself calling for x[1] and x.last - this is very unreadable, and brittle.
Ruby allows you to implicitely assign elements of the array to parameters in your block according to its location, so you could write the same "maps" like this:

  .map do|numerator, denominator|
     [numerator.to_f / denominator.to_f, numerator.chars, denominator.chars,
      numerator.chars & denominator.chars]
  end.map do |result, numerator_digits, denominator_digits, mutual_digit|
    [result, numerator_digits - mutual_digit, denominator_digits - mutual_digit,
     numerator_digits, denominator_digits]
  end.select do|result, new_numerator_digits, new_denominator_digits|
    result == new_numerator_digits.join.to_f/new_denominator_digits.join.to_f
  end.map do |_, _, _, numerator_digits, denominator_digits|
    Rational(numerator_digits.join.to_i, denominator_digits.join.to_i)
  end.reduce(:*)

Note that unneeded array elements at the end of the array are truncated, so you don't have to write them. For the unneeded elements at the beginning of the array I've used the _ idiom which tells the method that I expect these parameters, but I won't need them.

If it deserves a comment - it deserves a method
Instead of commenting your code, consider refactoring it into methods. Each method will do one thing, and, given the proper name, will be descriptive when used, and easily read in itself:

def trivial?(pair)
  pair.any? {|y| y.chars.include? '0'}
end

def identical?(numerator, denominator)
  numerator == denominator
end

def larger_than_one?(numerator, denominator)
  numerator.to_i > numerator.to_i
end

def has_common_digits?(numerator, denominator)
  (numerator.chars & numerator.chars).size > 0
end

a = 10.upto(99).to_a.map(&:to_s)
puts a.product(a)
      .reject(&method(:trivial?))
      .reject(&method(:identical?))
      .reject(&method(:larger_than_one?))
      .select(&method(:has_common_digits?))

Make your steps logical
The last few steps you take look like they are held out with gum, and that you made some acrobatics to stay one-line and "idiomatic". It is better to make your methods do something you can explain, than simply adding and removing temp-variables:

def reduced_fraction(numerator, denominator)
  mutual_digit = numerator.chars & denominator.chars
  (numerator.chars - mutual_digit).join.to_f / (denominator.chars - mutual_digit).join.to_f
end

a = 10.upto(99).to_a.map(&:to_s)
puts a.product(a)
      .reject(&method(:trivial?))
      .reject(&method(:identical?))
      .reject(&method(:larger_than_one?))
      .select(&method(:has_common_digits?))
      .select do |numerator, denominator|
        numerator.to_f / denominator.to_f == reduced_fraction(numerator, denominator)
      end.map { |fraction| Rational(*fraction) }
      .reduce(:*)

Update
Yes, it seems that using &method(:something) in map and multiple assignment are mutually exclusive... :(

I can suggest a few strategies where you don't have to fall back to using pair.first, pair.last:

Use multiple assignment in the first line of your method:

def identical?(pair)
  numerator, denominator = pair
  ...
end

Use a hash to hold your values, and use keyword arguments in your method

def identical?(numerator: nil, denominator: nil)
  numerator == denominator
end

a = 10.upto(99).to_a.map(&:to_s)
puts a.product(a)
      .reject(&method(:trivial?))
      .map { |numerator, denominator| {numerator: numerator, denominator: denominator } }
      .reject(&method(:identical?))


Creating a dictionary with a path name and SHA-256 hash The purpose is just to create a dictionary with a path name, along with the file's SHA-256 hash.

I'm very new to Python, and have a feeling that there's a much better way to implement the following code. It works, I'd just like to clean it up a bit.
    fnamelst = [r'C:\file1.txt',
                r'C:\file2.txt']

[fname.replace('\\', '\\\\') for fname in fnamelst]

diction = [{fname: hashlib.sha256(open(fname, 'rb').read()).digest()} for fname in fnamelst]

for iter in range(0,len(fnamelst)):
    dic2 = {fnamelst[iter]: hashlib.sha256(open(fnamelst[iter], 'rb').read()).digest()}

In your code, you show two ways to create your dictionary, but both are somewhat flawed:

diction will be a list of dictionaries, each holding just one entry
dic2 will get overwritten in each iteration of the loop, and will finally hold only one dictionary, again with only one element.

Instead, try this:

diction = dict([ (fname, hashlib.sha256(open(fname, 'rb').read()).digest())
                 for fname in fnamelst ])

Or shorter, for Python 2.7 or later:

diction = { fname: hashlib.sha256(open(fname, 'rb').read()).digest()
                 for fname in fnamelst }

The first one uses a list comprehension to create a list of key-value tuples, and then uses the dict function to create a single dictionary from that list. The second one does the same, using a dict comprehension.


Detecting a combination of characters from input Write a program that reads input up to # and reports the number of times that
  the sequence ei occurs.

Is this a decent code?

#include <stdio.h>

int main(void)

{
    int index = 0, combinationTimes = 0, total = 0;
    char userInput;
    char wordChar[index];

    printf("please enter your input:\n");

    while ((userInput = getchar()) != '#')
    {
        if (userInput == '\n')
            continue;

        wordChar[index] = userInput;
        index++;
        total++;
    }

    for (index = 1; index < total; index++)
    {
        if (wordChar[index] == 'i')
        {
            if (wordChar[--index] == 'e')
            {
            combinationTimes++;
            ++index;
            }
        }
    }

    printf("number of combination is: %d", combinationTimes);

    return 0;
}

Here are some inputs on command line which break your code:

dan@albatross  $ gcc -Wall f.c -o f
dan@albatross  $ ./f
please enter your input:
bfiqwb23b r9pu3h2ru23r
9aisdbfuiasdf
adsf#asdf
#
#
#
    adsfadsfasdf
34324!
^C

I assume your program is supposed to terminate at the first #, but notice here that it does not. I have to kill the program to stop it. The shows there is a problem with one of your loops. By seeing the manipulation of the index variable in the second loop, I think it's the problem.

Code Review

First: you do not need to have the line index++ inside the loop. Use the single variable total instead. This is a minor thing.

Instead of having a nested if-statement, use an and conditional. And stop decreasing the index variable - that's what causing all those problems. Try this:

for (index = 1; index < total; index++)
    {
        if (wordChar[index] == 'i' && wordChar[index - 1] == 'e')
            combinationTimes++;
    }

However, that is still inefficient code, because you should be processing the input as it is given, not processing it after the last hash. Notice that you are doing length_of_input work at the end, when you could be doing constant work at each step, which is computationally less noticeable to a user. In addition, the array has length_of_input memory overhead, which seems unnecessary here. Consider writing something like this instead:

#define FALSE 0
#define TRUE 1

int main(void)
{
    int combinationTimes = 0;
    char userInput;
    short int last_e = FALSE;

    printf("please enter your input:\n");

    while ((userInput = getchar()) != '#')
    {
        if (userInput == 'e')
        {
            last_e = TRUE;
        }
        else if (userInput == 'i' && last_e)
        {
            combinationTimes++;
            last_e = FALSE;
        }
        else
        {
            //this is important, otherwise combinations like 'ite' are counted
            last_e = FALSE;
        }
    }

    printf("Number of combos: %d\n", combinationTimes);
    return 0;
}

Here are some key features:

I removed your array, so I have no significant memory overhead. Each char is processed and discarded, one at a time
I remove your variables total and index
I process the event you care about inside the first for loop, as you read the character
I create boolean variables with compiler flags

This code also has a lot of room for improvement, but that's left as an exercise...


Optimizing digit sum and product with GMP I have a bottleneck in my program at the point where I calculate the digit sum and product of a number. I am using GMP because I have to work with numbers that follow \$1e8 < n < 1e80\$.

To calculate the sum/product of a number, I initially used strings, thinking the division/modulo method would result in an improvement later on.

However, when I later implemented this it turned out that this did not give me a performance improvement.

mpz_class num = 1120390317;
mpz_class sum = 0;
mpz_class pro = 1;

while(num != 0) {
    mpz_class rem = (num % 10);
    sum += rem;
    pro *= rem;
    num = num / 10;
}

Instead of:

mpz_class num = 1120390317;
mpz_class sum = 0;
mpz_class pro = 1;

const std::string strNum = num.get_str();
for (auto c : strNum) {
    int res = c - '0';
    sum += res;
    pro *= res;
}

In the actual program the values of num are actually in the before mentioned range. But they provide results about equal to this test case.

The test case and the program shows me (with or without optimizations) using the first method is 3x as slow. How can I improve the modulo version to run faster than the string version? Or should I use the string version?  If so, can that be optimized?

There are a few ways to speed up your division.  First, note that because rem is declared within the while loop, it's both constructed and destroyed every loop iteration.  That's not really necessary, so you could instead declare it outside the loop and simply assign a new value within the loop each time.  Also by changing the line from num = num / 10 to num /= 10, you avoid creating and destroying yet another temporary.  Applying both of those saves a little time, but it's still slower than the string method because you're essentially performing the division twice (once for num % 10 and again for num / 10).

The key is to do only a single division per loop, but you'll have to resort to using the underlying C function since it's not exposed to the C++ interface. Fortunately, it's simple to do so:

unsigned long digit;
while(num != 0) {
    digit = mpz_tdiv_q_ui(num.get_mpz_t(), num.get_mpz_t(), 10);
    sum += digit;
    pro *= digit;
}

On my machine this is slightly faster than the string method, and probably uses a little less memory.

Update:

Because the speed of that code relative to the string method varies based on what compiler optimizations are used, it's not necessarily always faster.  This version uses low-level GMP calls for more speed:

__mpz_struct *q = num.get_mpz_t();
while(*(q->_mp_d) != 0) {
    auto digit = mpn_divrem_1 (q->_mp_d, (mp_size_t) 0,
            q->_mp_d, q->_mp_size, (mp_limb_t) 10);
    sum += digit;
    pro *= digit;
}

This makes a couple of assumptions that are important.  First, it assumes that num > 0 and it dives deep into the internal structure of __mpz_struct so it will not be very durable to changes in GMP, should they decide to change the underlying representation of numbers.  However, with this code, I find that it's a bit faster on my machine than the string method, both with and without -O3 optimizations.

Also, note that within your range of integers, the sum of digits will always fit within a long int which will save time, but could equally be applied to the string method.


Storing arbitrary form data in SQL as an XML blob I am currently storing form data in an XML format:

<cfsavecontent variable="myFormData">
<cfoutput>

  <cfloop list="#form.fieldnames#" index="item">
    <cfloop list="#form[item]#" index="eachItem">

#xmlformat(item)# <var>#xmlformat(eachItem)#</var>
    </cfloop>
  </cfloop>

<cfoutput>
</cfsavecontent>

Then do a single insert:

<cfquery>
   INSERT INTO table (formData)
   VALUES (<cfqueryparam value="#myFormData#" cfsqltype="cf_sql_varchar">)
</cfquery>

When I pull the data out, I can:

<cfquery name="qryData">
   SELECT formData
   FROM table
   WHERE ID = <cfqueryparam value="#ID#" cfsqltype="cf_sql_integer">
</cfquery>

<cfoutput query="qryData">#formData#</cfoutput>

OR

<cfquery name="qryData">
   SELECT li.value('(b/text())[1]', 'varchar(50)') AS Item,
      li.value('(var/text())[1]', 'varchar(50)')   AS Value
   FROM table
   CROSS APPLY XmlData.nodes('/ul/li') AS ul(li)
   WHERE ID = <cfqueryparam value="#ID#" cfsqltype="cf_sql_integer">
</cfquery>

<cfoutput query="qryData">#Item# #Value#</cfoutput>

Some developers have concerns about storing the data in this format. Is this the best way to store arbitrary form data?

Well, it depends what you want to do with this data. If you ever want to query individual field names or debug an issue with a field you will have a hard time extracting this kind of information out of an XML blob.

Potential pitfall: According to the coldfusion docs, CF_SQL_VARCHAR maps to varchar columns on MSSQL. You need to make sure that the column is varchar(max) as XML can get pretty verbose and any character limit might get hit.

On how to better store the data this is hard to say. From your example it looks like a form has fields and each field could have multiple values. This looks like a classic 1:n relationship to me:

Form table:

FormID | Name | Description | ...

Field Name table:

FieldID | FormID | Name

Field Value table:

ValueID | FieldID | Value


Determine Collatz sequence length for given starting values I am trying to get into the habit of solving programming puzzles in my spare time but am stuck on the first one (which I know can be for a large number of reasons, hence my posting for help). Shown below is my code that is meant to solve the 3n+1 Challenge:

from sys import stdin

def main():
    curr_Line=stdin.readline()
    while curr_Line!=None:
        curr_Line=curr_Line.split()
        min_num=int(curr_Line[0])
        max_num=int(curr_Line[1])
        maxCycleLength =0
        for curr_num in range(min_num,max_num+1):
            currCycleLength =1
            while curr_num!=1:
                currCycleLength +=1
                if curr_num%2==0:
                    curr_num=curr_num/2
                else:
                    curr_num=3*curr_num+1
            maxCycleLength=max(currCycleLength,maxCycleLength)
        print(min_num,max_num,maxCycleLength,sep=" ")
        curr_Line=stdin.readline()
    return
main()

When I run this, the UVA online judge says that I've exceeded the 3 second time limit.

I've tried the following:

changing the the while loop to for line in stdin:

Any suggestions would be much appreciated.

You need functions.
The first one you can make is the collatz_conjecture.
First off I'd say make it recursive.
If you ignore the currCycleLength you should get:

def collatz_conjecture(curr_num):
    if curr_num == 1:
        return 1
    if curr_num % 2 == 0:
        curr_num = curr_num / 2
    else:
        curr_num = 3 * curr_num + 1
    collatz_conjecture(curr_num)

The best way to make this better is to use functools.lru_cache(None).
This is a wrapper, but it'll give you speed for just adding one line! (This is what Zack was talking about)
You also should add the depth too, this is just adding one onto the previous depth. The if's are actually pretty ugly, so you can change it to the turnery operator.

@functools.lru_cache(None)
def collatz_conjecture(curr_num):
    if curr_num == 1:
        return 1
    return collatz_conjecture(3 * curr_num + 1 if curr_num % 2 else curr_num / 2) + 1

Finally I don't like that if n == 1. I'd reimplement lru_cache and supply it with that at the beggining.

def memoize(cache=None):
    if cache is None:
        cache = {}
    def wrapper(fn):
        def inner(*args):
            try:
                return cache[args]
            except KeyError:
                r = fn(*args)
                cache[args] = r
                return r
        return inner
    return wrapper

@memoize({(1,): 1})
def collatz_conjecture(n):
    return collatz_conjecture(3 * n + 1 if n % 2 else n / 2) + 1

As for the rest of your code, I'd use input with map, I'd use a comprehension passed to max and I'd also add if __name__ == '__main__':

This should get you something like:

def main():
    while True:
        user_input = input()
        if not user_input:
            break
        start, end = map(int, user_input.split()[:2])
        print(start, end, max(collatz_conjecture(i) for i in range(start, end + 1)))

if __name__ == '__main__':
    main()


Sorting a collection Related to, but not exactly a follow up of this question. After fixing some issues discovered in the last review, I added a little more functionality to the Enumerable class. The problem is, I've never sorted before. I tried (and failed)to implement a few of the standard algorithms before coming up with this. It's not very efficient. It has to check to see if the collection IsSorted and just keeps looping until it is.

Is there a way to make this more efficient without using a more advanced algorithm?
What would be a simple to understand algorithm that is more efficient than this?

I realize that I could simplify some logic if I created some interfaces, but I would like to work with "built in" collections without wrapping everything in a class that implements an interface.

Bonus points to anyone who can tell me what algorithm I ended up using. I just kept testing until it worked.

Public Function Sort(collectionObject As Collection) As Collection

    Dim item As Variant
    Dim innerItem As Variant

    Dim i As Long
    Dim j As Long
    Dim index As Long

    Do Until IsSorted(collectionObject)
        For i = 1 To collectionObject.Count
            index = i

            If IsObject(collectionObject(i)) Then
                Set item = collectionObject(i)
            Else
                item = collectionObject(i)
            End If

            For j = i To collectionObject.Count
                If IsObject(collectionObject(j)) Then
                    Set innerItem = collectionObject(j)
                Else
                    innerItem = collectionObject(j)
                End If

                If item > innerItem Then
                    collectionObject.Add item, After:=j
                    collectionObject.Remove index
                    index = j
                End If
            Next j
        Next i
    Loop
End Function

Private Function IsSorted(collectionObject As Collection) As Boolean

    Dim item As Variant
    Dim previous As Variant

    For Each item In collectionObject
        If item < previous Then
            IsSorted = False
            Exit Function
        End If

        If IsObject(item) Then
            Set previous = item
        Else
            previous = item
        End If
    Next item

    IsSorted = True
End Function

FEAR

This is a red flag.

Do Until IsSorted(collectionObject)

    ' Sorting algorithm

Loop

Your algorithm should return a sorted sequence and should not need to be checked.  If it fails the check you should rewrite your sorting algorithm and not simply try to do it again.

Abstracting Methods

This bit of code is used often enough to merit it's own sub routine

Sub AssignUnknown(ByRef dest As Variant, ByRef src As Variant)

    If IsObject(src) Then
        Set dest = src

    Else
        dest = src

    End If

End Sub

Now your code looks so much better!  It looks like a bubble sort but

For i = 1 To collectionObject.count
    index = i

    AssignUnknown item, collectionObject(i)

    For j = i To collectionObject.count

        AssignUnknown innerItem, collectionObject(j)

        If item > innerItem Then

            collectionObject.Add item, After:=j
            collectionObject.Remove index
            index = j

        End If

    Next j

Next i

Algorithm

it doesn't work, hence you needing to loop until it is sorted.  You are only a few steps away from true bubble sort though, which is IMO simpler.

    For i = collectionObject.count To 2 Step -1

        ' hasSwapped = False

        For j = 1 To i - 1

            If collectionObject(j) > collectionObject(j + 1) Then

                collectionObject.Add collectionObject(j), After:=j + 1
                collectionObject.Remove j
                ' hasSwapped = true

            End If

        Next j

        ' If Not hasSwapped Then goto sorted ' Exit For

    Next i

sorted:

In short, instead of bubbling up the same object each time, the BubbleSort drops the current item for the next one when it finds a larger item.  This means after each inner loop the item at position i is in the correct place, which is why the outer loop is counting down not up.

hasSwapped uncommented allows for early exit if the sequence is already sorted.  For more information on the bubble sort, wikipedia has a great article.  I also found this site with some cool animations while trying to figure out your sorting algo.

Also note that AssignUnknown is no longer needed.

Error Handling

As we discussed in the comments, the comparison operators will not be defined for objects that do not have a default property.  It can't be fixed, but we can raise a more descriptive error

    On Error Goto no_default_property

    ' bubble sort

sorted:
Exit Function
no_default_property:

    If Err.Number = 438 Then ' preferably use the vb constant that I don't know
        Err.Clear
        Err.Raise 438, "Sort", "An item in the collection does not have a default property"

    End If

End Function

After Thoughts

Consider writing CollectionToArray and ArrayToCollection functions so you don't need to duplicate sorting methods for Collections and Arrays.  Also consider a Sorted function that returns a sorted copy.

Sub Sorted(collec As Collection) As Collection

    Set Sorted = collec
    Sort Sorted

End Sub

I was tempted to insist you use a Swap function like this.

Sub Swap(ByRef a As Variant, ByRef b As Variant)

    Dim t as Variant
    t = a
    a = b
    b = t

End Sub

However, you are supporting objects in your collection, you will need to make that routine SwapUnknown that uses AssignUnknown.  You could avoid calling IsObject(a) twice but I prefer the simpler solution.

Sub SwapUnkown(ByRef a As Variant, ByRef b As Variant)

    Dim t as Variant
    AssignUnknown t, a
    AssignUnknown a, b
    AssignUnknown b, t

End Sub

I didn't include this because:

Your current swap method could be faster depending on how Collection is implemented
Abstracting your current method seems pointless.
It doesn't require AssignUnkown, which cuts out some ugly code.


The conversion from UTF-16 to UTF-8 I have created a function that converts from UTF-16 to UTF-8.
This function converts from UTF-16 to codepoint firstly,  then from codepoint to UTF-8.
void ToUTF8(char16_t *str) {
    while (*str) {
        unsigned int codepoint = 0x0;

        //-------(1) UTF-16 to codepoint -------

        if (*str <= 0xD7FF) {
            codepoint = *str;
            str++;
        } else if (*str <= 0xDBFF) {
            unsigned short highSurrogate = (*str - 0xD800) * 0x400;
            unsigned short lowSurrogate = *(str+1) - 0xDC00;
            codepoint = (lowSurrogate | highSurrogate) + 0x10000;
            str += 2;
        }

        //-------(2) Codepoint to UTF-8 -------

        if (codepoint <= 0x007F) {
            unsigned char hex[2] = { 0 };
            hex[0] = (char)codepoint;
            hex[1] = 0;
            cout << std::hex << std::uppercase << "(1Byte) " << (unsigned short)hex[0] << endl;
        } else if (codepoint <= 0x07FF) {
            unsigned char hex[3] = { 0 };
            hex[0] = ((codepoint >> 6) & 0x1F) | 0xC0;
            hex[1] = (codepoint & 0x3F) | 0x80;
            hex[2] = 0;
            cout << std::hex << std::uppercase << "(2Bytes) " << (unsigned short)hex[0] << "-" << (unsigned short)hex[1] << endl;
        } else if (codepoint <= 0xFFFF) {
            unsigned char hex[4] = { 0 };
            hex[0] = ((codepoint >> 12) & 0x0F) | 0xE0;
            hex[1] = ((codepoint >> 6) & 0x3F) | 0x80;
            hex[2] = ((codepoint) & 0x3F) | 0x80;
            hex[3] = 0;
            cout << std::hex << std::uppercase << "(3Bytes) " << (unsigned short)hex[0] << "-" << (unsigned short)hex[1] << "-" << (unsigned short)hex[2] << endl;
        } else if (codepoint <= 0x10FFFF) {
            unsigned char hex[5] = { 0 };
            hex[0] = ((codepoint >> 18) & 0x07) | 0xF0;
            hex[1] = ((codepoint >> 12) & 0x3F) | 0x80;
            hex[2] = ((codepoint >> 6) & 0x3F) | 0x80;
            hex[3] = ((codepoint) & 0x3F) | 0x80;
            hex[4] = 0;
            cout << std::hex << std::uppercase << "(4Bytes) " << (unsigned short)hex[0] << "-" << (unsigned short)hex[1] << "-" << (unsigned short)hex[2] << "-" << (unsigned short)hex[3] << endl;
        }
    }
}

Also, you can compile and test the code from here
What do you think about that function in terms of performance, and ease?

First a special problem: Unicode 0 is a terminator char in strings in C/C++.
Modified UTF (i.e. UTF-8) deals with this by also doing the encoding for what officially should be one byte 0. As decoding poses no problem. You might consider this. For instance simply requiring modified UTF-16 on input as you check the terminator.
    if (codepoint <= 0x007F && codepoint != 0) {

Now the actual review:

(Optional) To cope with modified (*str) UTF-16, generating modified UTF-8:
  if (codepoint <= 0x007F && codepoint != 0) {

You are now giving the result to cout using an extra nul byte as terminator.

There should be a byte output stream. An output stream parameter could be appended to per byte, without intermediate arrays; and only at the end might need a NUL byte. If str occupies N UTF bytes, then the result will at most need 2N UTF bytes. _N UTF-16 bytes ~ to N/2 code points ~ max 2N UTF-8 (N/2 * 4-byte sequences)._
That terminator should be added outside the loop.

Creating arrays is superfluous, immediately return the single bytes. (This would be the case for delivering the result somewhat like cout << ((codepoint >> 6) & 0x1F) | 0xC0)

You nicely validate the input for illegal UTF-16 chars above the max. In java one would throw an exception, you just discard the char.

(A matter of taste) Maybe consider an API with a string length as input parameter instead of relying on a NUL terminator. If the area of application is file based, that
would even be more natural.


Searching for duplicates without any collection or sorting I have written code which is easy to understand, but may not be efficient but will solve the purpose.

public static void main(String a[]){
        int ar[] ={1,2,3,4};
        boolean flag=false;
        int count=0;
        while(count<ar.length){
            for(int i=0;i<ar.length-1;i++){
                if(ar[count]==ar[i+1] && count !=i+1){
                    flag=true;
                }
        }
            count++;
        }
        if(flag){

        System.out.println("Duplicate Exists in Array!");
        }else{
            System.out.println("Duplicates Doesn't Exists in Array!");
        }

    }

As you may be aware, your solution takes O(n2) time.  Sorting can bring that down to O(n log n).  Hashing can produce an O(n) solution.  Of course, for a four-element array, performance isn't an issue at all.

Please take care to indent your code consistently.

This kind of loop structure…

int count = 0;
while (count < ar.length) {
    …
    count++;
}

… is more easily recognizable when written as a for-loop:

for (int count = 0; count < ar.length; count++) {
    …
}

Once you do that, you'll see nested for-loop, which is easier to understand:

for (int count = 0; count < ar.length; count++) {
    for (int i = 0; i < ar.length - 1; i++) {
        if (ar[count] == ar[i + 1] && cont != i + 1) {
            flag = true;
        }
    }
}

But you are inspecting almost every pair of elements twice.  Typically, the inner loop limits are written to avoid half the work:

for (int i = 1; i < ar.length; i++) {
    for (int j = 0; j < i; j++) {
        if (ar[i] == ar[j]) {
            // Duplicate found!
            …
        }
    }
}

Variables named flag (or that act as a flag) are usually a bad idea, because they express your intentions indirectly.  In the case of duplicate element detection, once you've found a single duplicate, you're all done.  There should be a break, or better yet, a return:

public static boolean duplicateExists(int[] ar) {
    for (int i = 1; i < ar.length; i++) {
        for (int j = 0; j < i; j++) {
            if (ar[i] == ar[j]) {
                return true;
            }
        }
    }
    return false;
}

public static void main(String[] args) {
    int[] ar = …;
    System.out.println(duplicateExists(ar) ? "Duplicate exists in array!"
                                           : "No duplicates in array!");
}


HTML cleaner in JavaScript Please review code quality and give me shortcuts or optimization tips as well as corrections.

<!DOCTYPE html>

<meta charset="utf-8">
<title>Clean HTML Code</title>

<textarea id="code" cols="80" rows="15" autofocus></textarea>

<input id="comments" type="checkbox" checked> Remove All HTML Comments

<input id="head" type="checkbox"> Remove Head Tags

<input id="style" type="checkbox"> Remove Style Tags

<input id="script" type="checkbox"> Remove All Script Tags (also removes NoScript Tags)

<button id="btn">Clean Up HTML</button>

First of all, you can't parse HTML with regex. RegEx is totally the wrong person for the job. Your code there assumes that the input HTML is properly formatted. If given broken HTML, you code won't even stand a chance. I suggest you use a parser instead. Creating one isn't trivial, so you might want to look for one out there.

Now, I see you use $ and I thought it was jQuery until I saw the code define it. Avoid using $. Name your function verbosely instead. That way, you don't confuse people to thinking it is jQuery when it isn't.

Now, speaking of jQuery, you could theoretically use jQuery to strip out HTML since it has a DOM parser and handy DOM functions:

// Get the HTML
var html = $('#code').val();
var DOM = $(html);

// Collect selectors to remove
var toRemove = [];
if(head.checked) toRemove.push('head');
if(style.checked) toRemove.push('style');
if(script.checked) toRemove.push('script');
if(form.checked) toRemove.push('form');

DOM.remove(toRemove.join(','));

if(comments.checked) DOM = DOM.filter('*');

var cleanHTML = DOM.html();

Just a theory. Dunno how it would really perform.


Reversing the elements of an array Any way to make this more concise/efficient?

// Write a loop that reverses the elements of an array.
#include <iostream>
#include <iomanip>

const int SIZE = 10;

void reverse(int iArrayRef1 []);
void display(int iArrayRef2 []);

int main()
{
    int iArray[SIZE] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };

    // Display in the original order.
    std::cout << "Forwards:\n";
    display(iArray);

    // Reverse the array.
    reverse(iArray);

    std::cout << std::endl;

    // Display in reversed order.
    std::cout << "Backwards:\n";
    display(iArray);
    return 0;
}

void reverse(int iArrayRef1 [])
{
    int temp = 0;
    int* indexPtrBeg = iArrayRef1;
    int* indexPtrEnd = iArrayRef1 += (SIZE-1);

    for (int index = 0; index < (SIZE/2); index++)
    {
        temp = *indexPtrEnd;
        *indexPtrEnd = *indexPtrBeg;
        *indexPtrBeg = temp;
        indexPtrEnd--;
        indexPtrBeg++;
    }
}

void display(int iArrayRef2 [])
{
    for (int index = 0; index < SIZE; index++)
    {
        std::cout << std::setw(2) << iArrayRef2[index] << " ";
    }
    std::cout << std::endl;
}

For an implementation without the STL, this is pretty good.  There's no risky memory-management going on with the raw pointers, and your sorting isn't needlessly slow (compared to bubble sort).  I'll base my review on the STL anyway (for future reference), along with some other general tips.

SIZE should be of type std::size_t.  It's preferred to use an unsigned integer type for sizes, plus you'll have a larger range (no negative values are part of it).
If you have C++11, prefer std::array (or any other STL container) over C-style arrays.  This also makes it needless to pass the array size since STL containers already know their own.

Your array initialization, for instance, would look like this:

std::array<int, SIZE> iArray = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };

If you use the above array or another STL container, the loop counters in reverse() and display() should be of type std::size_type.  This will ensure:

the supported size type for this container is used
the loop will be able to access any number of elements

Also, as @thelamb mentioned, pre-increment (++index) is preferred because it will avoid extra copies.  This is especially important for user-defined types (such as std::array), while post-increment is still okay with native types (such as int).
Prefer iterators to raw pointers in C++:

// with C++11

auto begin = std::begin(container);
auto end   = std::end(container);

// without C++11

std::ContainerType::iterator begin = std::begin(container);
std::ContainerType::iterator end   = std::end(container);

You could compare your reverse algorithm to the idiomatic STL method with regards to efficiency (if you were to perform a speed test on them):

std::reverse(container.begin(), container.end());

I'd avoid the extra newlines in display().  The function's purpose is solely to display the array, plus you'll be forced to have the newline with each call.  Two things for this:

add the newlines before and/or after the function call instead
use \n instead of std::endl (the latter also flushes the buffer, which takes longer)

So, instead of this:

std::cout << std::endl;

std::cout << "Backwards:\n";
display(iArray);

you could have this:

std::cout << "\nBackwards:\n"; // another '\n' at the front
display(iArray);
std::cout << "\n";             // newline here instead of in display()

For comparison, here's an idiomatic alternative using templates (any element type is supported).  It uses an std::vector instead of an array, saving you size constraints.  It also displays the vector in forward and in reverse using an iterator, instead of mutating it for the reverse.

#include <algorithm> // std::copy, std::reverse_copy
#include <iostream>
#include <iterator>  // std::ostream_iterator
#include <vector>

template <typename T>
void displayForward(std::vector<T> const& vec)
{
    std::copy(vec.cbegin(), vec.cend(),
        std::ostream_iterator<T>(std::cout, " "));
}

template <typename T>
void displayReverse(std::vector<T> const& vec)
{
    std::reverse_copy(vec.cbegin(), vec.cend(),
        std::ostream_iterator<T>(std::cout, " "));
}

int main()
{
    std::vector<int> vec = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };

    std::cout << "Forward:\n";
    displayForward(vec);

    std::cout << "\nBackwards:\n";
    displayReverse(vec);
}


Creating a Basic Success Function for Http I got sick of every time I make an Http request validating that the response was a success. I did a lot of:

if response.class == Net::HTTPOK

or

if response.is_a? Net::HTTPSuccess

I wanted something a little prettier so I came up with the following. I don't write a lot of class extensions like this so I wanted some input.

require 'net/http'
class Net::HTTPResponse
  def success?
    return false if ! self.is_a? Net::HTTPSuccess
    return true
  end
end

Now once the file is included I can say:

if response.success?

Though your solution would work, I would have gone for something like this:

class Net::HTTPResponse
  def success?
    false
  end
end

class Net::HTTPSuccess
  def success?
    true
  end
end

If you do keep your solution, you could simplify it to:

class Net::HTTPResponse
  def success?
    self.is_a? Net::HTTPSuccess
  end
end


OAuth Provider token generation I'm currently creating an OAuth provider in Java using Jersey. To the best of my knowledge Jersey does not provide a method to create oauth tokens so I'm creating my own.

For those unfamiliar with OAuth, the tokens will be used in a somewhat similar fashion to public/private keys to sign and verify all requests to the server.

A String is formed using a token issued by the server (me) and then encrypted with that token secret (which only the server and the application know). The signature is then sent to the server and verified.

Each token must be:

non-sequential
non-guessable
unique (the tokens will be stored in a database so uniqueness can be verified)

This is the code I'm thinking of using to generate the keys:

public String generateToken() {
    SecureRandom secureRandom = SecureRandom.getInstance("SHA1PRNG");
    MessageDigest digest = MessageDigest.getInstance("SHA-256");
    secureRandom.setSeed(secureRandom.generateSeed(128));
    return new String(digest.digest((secureRandom.nextLong() + "").getBytes()));
}

I'm generating a Long using Java's SecureRandom with SHA-1-PRNG.
Using a 128 bit seed again generated by SecureRandom.

I'm then using SHA-256 to hash the resulting Long to get a 32 character Unicode String as the token.

Is anyone able to see any issues with this style of token generation?
If multiple tokens were requested in a row, is there a chance of predicting the next one?
I assume that 32 characters is more than enough for this kind of request signing.

Each token must be;

  non-sequential
  non-guessable
  unique

Without reading into any of the OAuth specifics, if the above is the only criteria to which you must adhere, then I would suggest what you're doing is quite a huge effort to achieve what's already been done with GUID (Globally Unique Identifier).

Java has an implementation of this, a class named UUID:

  ...that represents an immutable
  universally unique identifier (UUID).
  A UUID represents a 128-bit value.

Conveniently, a GUID is also a 32 character string.

Some code I found to utilise this using Java:

UUID uuid = UUID.randomUUID();
String randomUUIDString = uuid.toString();

Note that I'm not really qualified to be an authority on this where Java is concerned, though the topic I'm concerning myself with here is very transferable, you will need to determine whether A) a GUID satisfies all criteria of an OAuth token, and B) that the Java implementation works as the rest of the world expects - I can't vouch for that.


Factory-like pattern implementation I have a hierarchy of types in my program and need to be able to create them dynamically at runtime. The solution I've come up with below is not the most elegant solution. Are there ways of improving it?

Specifically would it be possible to have the method getBuilderFor(...) not need to instantiate the type it's trying to create in order to get a builder for that type?

public class Main {

    public interface E {    }

    public class EImpl implements E { }

    public interface C<V> extends E {

        interface Builder<T> { T build(); }
    }

    public abstract class CImpl<T> extends EImpl implements C<T> {

        public abstract class BuilderImpl<V extends C<?>> implements Builder<V>{  }
    }

    public interface B extends C<String> { }

    public class BImpl extends CImpl<String> implements B {

        public class BuilderImpl extends CImpl<B>.BuilderImpl<B> {
            @Override public B build() { return new BImpl(); }
        }
    }

    public interface D extends C<Double> { }

    public class DImpl extends CImpl<Double> implements D {

        public class BuilderImpl extends CImpl<D>.BuilderImpl<D> {
            @Override public D build() { return new DImpl(); }
        }
    }

    public class Factory {

        public <R extends C<?>, K extends C.Builder<R>> K getBuilderFor(final Class<R> type) {

            if (BImpl.class.equals(type)) {
                // I would ideally like to just have return BImpl.BuilderImpl() here - is it possible?
                return (K) new BImpl().new BuilderImpl();
            } else if (DImpl.class.equals(type)) {
                // I would ideally like to just have return DImpl.BuilderImpl() here - is it possible?
                return (K) new DImpl().new BuilderImpl();
            }
            return null;
        }
    }
}

Why not use a map of types (CImpl concrete instances) that are instantiated upon Factory initialization? The values of the map would be instances of BImpl, DImpl, etc, and the respective keys would be the types. The function getBuilderFor() would just do a map lookup and on the value returned from the map, call BuilderImpl() and return the result.

This approach would allow to avoid the if/else block. Additionally, it would make it easier and more elegant to add more types in the future.


Spell a given number in English I wrote a program that will take a number from 0 to 999,999,999,999 and spell out that number in English.

How could I improve the readability of my code?

class Say
  UNITS ={
    1=> "one",
    2=> "two",
    3=> "three",
    4=> "four",
    5=> "five",
    6=> "six",
    7=> "seven",
    8=> "eight",
    9=> "nine"
  }
  TEENS ={
    11=> "eleven",
    12=> "twelve",
    13=> "thirteen",
    14=> "fourteen",
    15=> "fifteen",
    16=> "sixteen",
    17=> "seventeen",
    18=> "eighteen",
    19=> "nineteen"
  }
  TENS ={
    1=> "ten",
    2=> "twenty",
    3=> "thirty",
    4=> "forty",
    5=> "fifty",
    6=> "sixty",
    7=> "seventy",
    8=> "eighty",
    9=> "ninety"
  }
  NUMBER_SCALE = {
    1=>"hundred",
    2=>"thousand",
    3=>"million",
    4=>"billion",
    5=>"trillion"
  }

  def initialize(number)
    @number = number.to_s
  end

  def in_english
    raise ArgumentError if @number.to_i < 0 or @number.to_i > 999_999_999_999
    return "zero" if @number == "0"
    ret_val =""
    chunks = chunks_of_thousand(@number)
    chunks.each.with_index do |n,i|
      if n.length > 2
        ret_val << " " unless ret_val.empty?
        ret_val << "#{UNITS[n[0].to_i]} #{NUMBER_SCALE[1]}"
        two_digit_number = n[1]+n[2]
        if two_digit_number.to_i > 0
          ret_val << " " unless ret_val.empty?
          ret_val << "#{less_than_100(two_digit_number)}"
        end
      else
        if n.to_i > 0
          ret_val << " " unless ret_val.empty?
          ret_val << less_than_100(n)
        end
      end
      ret_val << " #{NUMBER_SCALE[chunks.length-i]}" if chunks.length-i > 1 && n.to_i > 0
    end
    #system `say #{ret_val}`
    ret_val
  end

  private
  def chunks_of_thousand(a)
    ret_val =[]
    n = a.to_i.to_s.length
    num_chunks= n/3
    rem = n % 3
    if rem > 0 then
      ret_val << a.to_i.to_s.split("")[0..rem-1].join().to_i.to_s
      start = rem
    else
      start = 0
    end
    0.upto(num_chunks-1) do |i|
      ret_val << a.to_i.to_s[start..3*(i+1)+(rem-1)].to_i.to_s
      start= 3*(i+1)+rem
    end
    ret_val
  end

  def less_than_100(n)
    return UNITS[n.to_i] if n.length == 1
    return TENS[n[0].to_i] if n.length == 2 && n[1] == "0"
    return TEENS[n.to_i] if n.length == 2 && n[0] == "1"
    return "#{TENS[n[0].to_i]}-#{UNITS[n[1].to_i]}" if n.length == 2
    raise "Number length is greater then 2"
  end
end

It certainly seems like you're working too hard here. Some long methods, a lot of to_i and to_s (and back to_i) and so on. A line like this:

ret_val << a.to_i.to_s[start..3*(i+1)+(rem-1)].to_i.to_s

is, well, horrifying. Sorry.

Quick review

As ckuhn203 pointed out in the comments, this might be worth monkey patching into Integer. But if you choose not to, instantiating an object for it is a bit much. The entire thing is state-less, so it could just as well be a class method or module function.
You make heavy use of the ret_val name. Besides being unnecessarily shortened, it's also completely opaque. Ok, it's the return value, but what is it? It's sort of like naming a method method - it doesn't really explain much.
You make a lot of use of unless some_string.empty?. A simpler solution would be to (conditionally) push strings to an array, and then call join at the end. No need for "manual" string concatenation.
And, as mentioned, there's a lot of number-to-string-to-number stuff going, a lot of slicing and dicing, and tricky loops with complex ranges like start..3*(i+1)+(rem-1). As a rule of thumb, you very, very rarely need to do index arithmetic in Ruby, so if you find yourself doing that, take a step back and reconsider.

In terms of readability, I pretty much just gave up, to be honest. I mean, I get the gist of what your code is doing in one place or another, but I just don't want to read through it in detail and make heads and tails of all the slicing and looping.

It doesn't have to be this way, though. While it's tempting to approach this as slicing up a string, what we're dealing with is just a number, so a little arithmetic works too; you basically want to work with the remainders of dividing by 1000, 100, and 10.

Alternative approach

Your friends for this task are Numeric#divmod and multiple assignments (aka array destructuring). For instance, to split the input number into "thousands-chunks":

chunks = []
while number > 0
  number, chunk = number.divmod(1000)
  chunks << chunk
end

or, if you want to be a little obtuse about it:

chunks = []
number, chunks[chunks.count] = number.divmod(1000) while number > 0

That basically replaces your chunks_of_thousand method. Note that the chunks array is "backwards", e.g. if number = 1_123_456_089 you'll get:

[89, 456, 123, 1]

Now that we've got chunks to work on, we can again use divmod to our advantage. However, it'd be nice to reorganize those lookup tables a little. I'd do something like this, combining the first 20 numbers into one hash (or just an array)

ZERO_TO_TWENTY = {
  0 => nil,
  1 => "one",
  # ... snip ...
  19 => "nineteen"
}.freeze

The TENS can remain as-is for now (although it's recommended to call freeze on objects that are declared as constants), and the NUMBER_SCALE we'll change later.

Now, to convert each 0-999 chunk to English, we can do this (it's not super pretty though - I've included some further alternatives later on):

strings = chunks.map do |number|
  string = []

  # get the hundreds
  hundreds, remainder = number.divmod(100)
  string << "#{ZERO_TO_TWENTY[hundreds]} hundred" if hundreds > 0

  # get the tens and units (with a special case for numbers below 20)
  tens, units = remainder < 20 ? [nil, remainder] : remainder.divmod(10)
  string << [ TENS[tens], ZERO_TO_TWENTY[units] ].compact.join("-")

  string.join(" ")
end

Yes, I have hard-coded the string "hundred" in there, which is less than ideal. I wouldn't take much to move it to a constant of course. However, there's no sense in sticking it in the NUMBER_SCALE hash like your current code does, but where it doesn't belong.

Now, to match those strings to their "scale", we have Array#zip. Note, however, that for this to work, we have to redefine your NUMBER_SCALE a bit:

NUMBER_SCALE = [
  nil,
  "thousand",
  "million",
  "billion",
  "trillion"
].freeze

After that, we can do this bit of chaining magic:

strings.zip(NUMBER_SCALE).reverse.flatten.compact.join(" ")

which'll give us the result. E.g.:

in_english(1_123_456_089)
#=> "one billion one hundred twenty-three million four hundred fifty-six thousand eighty-nine"

All together now

All we need is the input checks from your code and we've got:

# constants declared as above

def in_english(number)
  number = number.to_i

  raise RangeError if number < 0 or number > 999_999_999_999
  return "zero" if number.zero?

  chunks = []
  number, chunks[chunks.count] = number.divmod(1000) while number > 0

  strings = chunks.map do |number|
    string = []
    hundreds, remainder = number.divmod(100)
    string << "#{ZERO_TO_TWENTY[hundreds]} hundred" if hundreds > 0

    tens, units = remainder < 20 ? [nil, remainder] : remainder.divmod(10)
    string << [ TENS[tens], ZERO_TO_TWENTY[units] ].compact.join("-")

    string.join(" ")
  end

  strings.zip(NUMBER_SCALE).reverse.flatten.compact.join(" ")
end

It's not brilliant, but I'd say it's at least better. If nothing else, there's just a lot less code there.

You'll note I've just made a single method here, not a class. You can package it however you want of course (e.g., as ckuhn203 suggested, as a monkey patch on integer) - but I'll leave that as an exercise to the reader. If you make a class, it'd of course be beneficial to break the various parts of the logic into separate (private) methods.

Also of note is that I'm raising a RangeError, as that's the most appropriate error to use when a numeric value is, well, out of range. It would be very nice to append a custom error message to that error while you're at it, to let the user know what the range actually is.

And I'm using zero? instead of == 0, as that's more idiomatic.

I'm a little on the fence about the number.to_i line. I think I'd prefer to assume that a number has been passed in, and if not then it's the user's problem (since the code will just raise a NoMethodError at some point). Garbage In, Garbage Out, basically. I've left it in to match your current code, but I would probably leave it out otherwise.

You can try it here if you'd like.

Further alternatives

Here's a different approach, using a recursive lambda

def in_english(number)
  number = number.to_i
  raise RangeError if number < 0 or number > 999_999_999_999
  return "zero" if number.zero?

  convert = -> (number) do
    case
    when number.zero?
      nil

    when number < 20
      ZERO_TO_TWENTY[number]

    when number < 100
      tens, units = number.divmod(10)
      [ TENS[tens], convert[units] ].compact.join("-")

    when number < 1000
      hundreds, number = number.divmod(100)
      [ "#{ZERO_TO_TWENTY[hundreds]} hundred", convert[number] ].compact.join(" ")

    else
      chunks = []
      number, chunks[chunks.count] = number.divmod(1000) while number > 0
      chunks.map(&convert).zip(NUMBER_SCALE).reverse.flatten.compact.join(" ")
    end
  end

  convert[number]
end

I like this a little better myself. The lambda could of course be a (private) method instead, which would make sense in a class. The only thing that's keeping this from simply being a single, recursive method is the "zero" vs nil return values.

You could handle that with just an extra method argument, however. It might in fact be useful to the user - not just as a hack to make the code work. It'd let someone pass in, say, "none" instead of zero which could be nice is some situations.

Doing so, we get a single, recursive method:

def in_english(number, zero_string = "zero")
  number = number.to_i
  raise RangeError if number < 0 or number > 999_999_999_999

  case
  when number.zero?
    zero_string

  when number < 20
    ZERO_TO_TWENTY[number]

  when number < 100
    tens, units = number.divmod(10)
    [ TENS[tens], in_english(units, nil) ].compact.join("-")

  when number < 1000
    hundreds, number = number.divmod(100)
    [ "#{ZERO_TO_TWENTY[hundreds]} hundred", in_english(number, nil) ].compact.join(" ")

  else
    chunks = []
    number, chunks[chunks.count] = number.divmod(1000) while number > 0
    chunks.map { |n| in_english(n, nil) }.zip(NUMBER_SCALE).reverse.flatten.compact.join(" ")
  end
end

It's still quite long for a Ruby method, but I like it all the same.


A simple game that keeps track of the number of questions asked This is just a simple counter, i was able to code it in two different ways but i want to know which way is more efficient and which way is more readable.
and which set of code is more "correct" in broad-manner of speaking.
and if there is another entirely different set of code that does the same function ?

Game Rules

  This is a simple game , at first it asks the user if they want to go
  right or left .  if the user picks left ; the program ends and it
  displays the "You Win!" message. however if the user picks anything
  else other than left ( for example : right ) the program will start a
  counter 'z' and it will display a different message for each time the
  user chooses the wrong direction or input. i have set it to 4
  different messages and then one last message that keeps playing on
  repeat until the user picks the correct direction.

however i feel like that there must be better code to do this.
can you please help?

the first set of code :

#This is a simple game where if you pick left you win,
#but if you pick right you keep going through 4 different levels of
#'you lose' messages and then a last one keeps repeating in a loop.

z = 0
n = input ( "Go left or right? \n" )
while n not in ["left", "Left"]:
    if z == 0 :
        print ("You lose * 0 ")
        z = z+1
        n = input ( "Go left or right? \n" )
    elif z == 1 :
        print ("You lose * 1 ")
        z = z+1
        n = input ( "Go left or right? \n" )
    elif z == 2 :
        print ("You lose * 2 ")
        z = z+1
        n = input ( "Go left or right? \n" )
    elif z == 3 :
        print ("You lose * 3 ")
        z = " "
        n = input ( "Go left or right? \n" )
    else :
        while n not in ["left", "Left" ] :
            print ( "You Lose * Infinite" )
            n = input ( "Go left or right? \n" )
print ( "\nYou Win!" )

Second set of code :

#This is a simple game where if you pick left you win,
#but if you pick right you keep going through 4 diffrent levels of
#'you lose' messegses and then a last one keeps repeating on a loop.

v = 0
z = 0
u = 0
x = 0
n = 0

while x == 0 :
    n = input ("Go Left or Right?")
    while n not in ["left", "Left"] and u == 0 :
        if   z == 0:
            print ( " You lose * 0 ")
        elif z == 1:
            print ( " You lose * 1 ")
        elif z == 2:
            print ( " You lose * 2 ")
        elif z == 3:
            print ( " You lose * 3 ")
        else :
            print ( " You lose * Infinite " )
        u = " "
    while z < 4 and v == 0 :
        z = z + 1
        v = " "
    u = 0
    v = 0
    if n == "left" or n == "Left":
        x=" "

print ( " You Win! ")

final set of code :

user = input ("Go left or right?")
counter = 0

while user not in ["left", "Left"] :
    if counter == 0 :
        print ("You lose * 0")
    elif counter == 1 :
        print ("You lose * 1")
    elif counter == 2 :
        print ("You lose * 2")
    elif counter == 3 :
        print ("You lose * 3")
    else :
        print ("You lose * Infinite")
    counter = counter + 1
    user = input ("Go left or right?")

print ("You Win!")

You're doing some weird things with the scripts. I consider the first to be better, since especially in the second one, you're doing a lot of things with variables which we have better ways to do. I'll point those out separately - lets first have a look at your first script.

Your first script

Code Duplication

If you find yourself typing the same thing a lot, you're probably doing something wrong. Examples of this are incrementing z and printing the message.

Variable Names

Variables should have short but meaningful names. That means that if you find yourself using variables like z or n, you're probably doing something wrong somewhere.

Loops and termination

We have a really nice command named break in python. It will terminate the inner-most loop. Using this will obsolete all nested loops your currently have.

I'll keep using while loops here, but you might also want to try the following instead:

from itertools import count

for iteration in count(1):
    # do stuff...
    if we_are_done():
        break

this function is basically an infinity list - it's doing exactly the same as:

iteration = 0
while True:
    iteration += 1
    # do stuff....
    if we_are_done():
        break

Which we'll be doing all the time here.

Keeping that all in mind....

iteration = 0
while True:
    command = input("Go Left or Right?")
    if "left" in command.lower():  # Transforms the input to all lowercase. This makes it case-insensitive.
        # We also use "in" as membership test, so that whitespace is ignored, and even an command like
        # "go left" will be considered valid.
        break  # Terminates the loop.
    iteration += 1
    print(f"You lose * {iteration if iteration < 4 else 'infinity'}")
print("\nYou Win!")

This will do the same as your first script. In the loop, we get the input. We decide if we have won yet, and if we do, we break out. Then we increment the iteration variable. Then we print how often we've moved wrong before, or if it's more than 3 times, we instead print we lost * infinity.

If you're confused by the way we put our variable in that string, you should read up on f-strings. If you're confused by how we print that number, read up on ternary expressions. A good python programmer will know both. If the python version allows it, (s)he'll probably also use both.

Now lets dissect your second function...

I'll ignore everything I've already said something about regarding the first script.

Variable instantiation - don't do it unless you have to

v = 0
z = 0
u = 0
x = 0
n = 0

If you find yourself writing something like this, you should ask yourself if you really need all those. Generally, you'll only instantiate variables in python when you need them, and leave them undefined as long as you don't. IF, and this is a big if, you really need them, you'd write it like this:

v = z = u = x = n = 0

How not to loop

Your outer loop should of course be either with count() or while True:. But the first inner loop is just obsolete alltogether. It cannot run more than once, since there's an u == 0 requirement for it to run, and at the end of the loop you unconditionally set it to " ", which is a totally different something. Your second inner loop does the exact same thing, but with v.

For both of these, you should at the least change the loops to if statements. However, if you also do the following paragraph, you can just remove them and unindent your code instead.

Do it just once if you can.

You check for n not in ["left", "Left"] twice. Of course, the first time is to guard against the printing, and the second is to break out of your loop. If you just break at the top, you'll have avoided having to do it twice.