Number to English word converter This is a simple program that I wrote. It takes a 64-bit unsigned int as input and returns its English name.

#include <iostream>
#include <string>
#include <cmath>
#include <stdint-gcc.h>
using namespace std;
int container[7];                       /// every element contains a number between 0 and 999
string conv0to19(uint64_t);   /// converts 1 to 19 to its name
string conv_dec(uint64_t);    /// converts 20,30...,90 to its name
string convert(uint64_t);       ///converts a number from of range uint64_t to its name
void fillContainer(uint64_t);
string convert1to999(uint64_t, int);

string name[]={"","thousand","million","billion","trillion","quadrillion","quintillion"};

string conv0to19(uint64_t a)
{
    switch (a)
    {
        case 0:return "";   break;
        case 1:return "one ";  break;
        case 2:return "two ";  break;
        case 3:return "three ";break;
        case 4:return "four "; break;
        case 5:return "five "; break;
        case 6:return "six ";  break;
        case 7:return "seven ";break;
        case 8:return "eight ";break;
        case 9:return "nine "; break;
        case 10:return "ten ";  break;
        case 11:return "eleven "; break;
        case 12:return "twelve "; break;
        case 13:return "thirteen ";break;
        case 14:return "fourteen "; break;
        case 15:return "fifteen "; break;
        case 16:return "sixteen "; break;
        case 17:return "seventeen ";break;
        case 18:return "eighteen ";break;
        case 19:return "nineteen ";break;
    }
}

string conv_dec(uint64_t a)
{
    switch(a)
    {
        case 0:return "";break;
        case 1:return "";break;
        case 2:return "twenty ";   break;
        case 3:return "thirty ";  break;
        case 4:return "forty ";  break;
        case 5:return "fifty "; break;
        case 6:return "sixty "; break;
        case 7:return "seventy "; break;
        case 8:return "eighty ";  break;
        case 9:return "ninety ";break;

    }
}

string convert1to999(uint64_t a,int i)
{
    if(a!=0)
    {
        if (a>0&&a<20)
            return conv0to19(a)+name[i];

        if(a>20&&a<100&&a%10!=0)
            return conv_dec(a/10)+conv0to19(a%10)+name[i];

        if(a>=20&&a&&a<100&&a%10==0)
            return conv_dec(a/10)+name[i];

        if(a>=100&&a<1000)
            return conv0to19(a/100)+"hundred "+conv_dec(fmod((a/10),10))+conv0to19(a%10)+name[i];
    }
    else return "";
}

void fillContainer(uint64_t a)
{
    for(int i=0;i<=6;i++)
    {
        container[i]=a%1000;
        a/=1000;
    }
}

string convert(uint64_t a)
{
    if(a!=0)
    {
        string output;
        fillContainer(a);

        for(int i=6;i>=0;i--)
            if(container[i]!=0)
                output=output+convert1to999(container[i],i)+" ";

        while(output[output.size()-1]==' ')         ///deletes the
        output.resize(output.size()-1);            ///white spaces on the end

        return output;
    }
    else
        return "zero";
}

int main()
{
    uint64_t a;
    cin>>a;
    cout<<convert(a);
}

Switched

Let's start with the obvious things we can improve. A return statement terminates the current function. Therefore, all your

case XY: return AB; break;

can be rewritten as

case XY: return AB;

However, you don't have any default cases in both switch statements. What happens if you use conv0to19(100)? Well, it's not obvious, because it's undefined behaviour.

For the sake of simplicity, let's use an assert for unhandled cases. But before we do that, let's have a closer look at your switch:

    case 0:return "";
    case 1:return "one ";
    case 2:return "two ";
      ...
    case 18:return "eighteen ";
    case 19:return "nineteen ";

Hm. You have a continuous range of integer values. So let's use that to our advantage:

std::string conv0to19(uint64_t a) {
    static char const * const numbers[20] =
      {"", "one", "two", "three", "four", "five", "six", "seven", "eight", "nine", "ten",
      "eleven", "twelve", "thirteen", "fourteen", "fifteen", "sixteen", "seventeen", "eighteen", "nineteen"
      };

    assert(0 <= a && a < 20);

    return numbers[a];
}

You can do the same for conv_dec. Note that I've removed the suffix space from the string. More on that later.

While we're at it and using an array, your name should:

have a better name (especially not name, since it's a collection of several ones)
be const, so that you don't change it by accident:

const std::string numberNames[] = {"", "thousand", "million", "billion", "trillion", "quadrillion", "quintillion"};

By the way, did you notice that I've added whitespace to separate the arguments? Remember code is written for humans (and for machines). If you want to remove a word later, it should be easy to do so, and whitespace makes it a lot easier to mark text with Ctrl+Shift (or whatever your operating system/editor uses).

Back to the beginning

int container[7];                       /// every element contains a number between 0 and 999

That is a code smell. It's a global variable, which isn't constant. And the description is, well, no really helpful. Just remove it.

Also, the 7 seems like a magic number. Why is it 7? Why not 10?

Either way, you should include cstdint instead of stdint-gcc.h, or — if your compiler is too old — stdint.h. Also, I recommend you to typedef your used number type, e.g.:

typedef uint64_t number_type;

That way you can replace the type easily, if you want to support even larger numbers later.

And using namespace std… well, it's considered bad practice for large programs, especially in headers, but you're writing only a single file here. It's up to you, but I wouldn't use it.

Using logic to reduce if-complexity

Your convert1to999 is too complex. If we didn't enter the following block:

    if (a>0&&a<20)
        return conv0to19(a)+name[i];

we're guaranteed that a is larger then 20. Why? Because a isn't 0. Since it is a non-negative number, it must be then greater than zero, so a>0 holds, so a < 20 must not hold. We don't need to repeat that in the next condition:

    if(a>20&&a<100&&a%10!=0)
       ^^^^

And again, your code is neigh unreadable, since you don't use any whitespace. What is easier to read:

a>=20&&a&&a<100&&a%10==0

or

a >= 20 && a && a < 100 && a % 10 == 0

I guess you now see the superfluous a in the condition, which will always be true at that point.

Next, this function should do one job. It should get a number from 0-999 and return its name, not add the suffix. So let's get rid of that and fix the if complexity. And while we're at it, let's add another function for 0-99:

std::string conv0to99(number_type a)
{
    assert(0 <= a && a < 100);

    if(a == 0) {
        return "";
    } else if (a <= 20) {
        return conv0to19(a);
    } else {
        return conv_dec(a / 10) + "-" + conv0to19(a % 10);
    }
}

std::string convert1to999(number_type a)
{
    assert(0 <= a && a < 1000);

    if(a == 0) {
        return "";
    } else if (a <= 100) {
        return conv0to99(a);
    } else {
        return conv0to19(a/100) + " hundred " + conv0to99(a % 100);
    }
}

Both functions are rather simple and easy to check. The hurdles of the <=20 are now (almost) entirely hidden in conv0to99, which is still easy enough to understand quickly.

As I said, get rid of fillContainer. You use it only in convert, so let's only have it there. Also, handle the easy case first:

std::string convert(uint64_t a)
{
    if(a == 0) {
        return "zero";
    }

    std::string output;
    int container[7];

    for(int i = 0; i < 7; ++i) {
        container[i] = a % 1000;
        a = a / 1000;
    }

    for(int i = 6; i>=0; i--) {
        if(container[i] != 0) {
            // we add the name now here instead of in convert1to999
            output += convert1to999(container[i]) + " " + name[i] + " ";
        }
    }

    // if your compiler doesn't have `output.back()`, use your old check
    while(output.back() == ' ') {
        output.pop_back();
    }

    return output;
}

You can also get rid of container if you build the number from the back.

Last words

Make sure to format your code. Either use an editor which supports automatic formatting, or use a program that formats your code, especially if you want to share your code.


A Python decorator for an "observable property", that notifies observers when the data is changed I have classes that have some data, and whenever this data changes, it should notify observers of that data change. There also should be a method to add new observers.
My first approach was to use a property with a custom setter. This means that whenever the = operator was used on the data it would call the custom setter, which would notify the observers.
Like so:
# (1)
from typing import Callable

class Foo:

    def __init__(self):
        self.__bar = 0
        self.__bar_observers = []

    @property
    def bar(self):
        return self.__bar

    @bar.setter
    def bar(self, value):
        self.__bar = value
        #notify observers
        for obs in self.__bar_observers:
            obs(value)

    def add_bar_observer(self, observer : Callable[[int], None]):
        self.__bar_observers.append(observer)

f = Foo()
f.add_bar_observer(lambda newval : print(f'new bar val {newval}'))
f.bar = 8

But this is a lot of boilerplate and is many lines of code just for one property. If Foo had 10 observable properties instead of just one, this class would be quite large.
It would be good if this pattern could be created automatically with a decorator.
Here is my attempt at that:
# (2)
def observable_property(initial_value):

    class _observable_property:

        def __init__(self, fget):
            pass

        # We don't know if __set__, __get__, or add_observer will be called first, so we need to call this at the start of all 3.
        def __init(self, obj):
            #On first time called, create the data
            if not hasattr(obj, f'__{self.name}'):
                setattr(obj, f'__{self.name}', initial_value)
            if not hasattr(obj, f'__{self.name}_observers'):
                setattr(obj, f'__{self.name}_observers', [])

        def __get__(self, obj, objtype=None):
            self.__init(obj)
            return getattr(obj, f'__{self.name}')

        def __set__(self, obj, value):
            self.__init(obj)
            setattr(obj, f'__{self.name}', value)
            #Notify all of the observers
            for obs in getattr(obj, f'__{self.name}_observers'):
                obs(value)

        def __set_name__(self, owner: type, name : str):
            self.name = name
            #Add a function to the owner to add new observers
            def add_observer(obj, observer):
                self.__init(obj)
                getattr(obj, f'__{name}_observers').append(observer)

            setattr(owner, f'add_{name}_observer', add_observer)

    return _observable_property

And then a test:
# (3)
Meters = NewType('Meters', float)
Kelvin = NewType('Kelvin', float)

class Axis:

    @observable_property(initial_value=Meters(0.0))
    def position(self):
        pass

    @observable_property(initial_value=Kelvin(0.0))
    def temperature(self):
        pass

a = Axis()
a.add_position_observer(lambda newval : print(f'New position for a: {newval}'))
a.add_temperature_observer(lambda newval : print(f'New temperature for a: {newval}'))
a.position = Meters(5.0)
a.temperature = Kelvin(300.0)

This works, but two downsides I can see are that the add_{}_observer functions do not appear in an IDE IntelliSense autocomplete, and it also loses its type hints.
What do you think? Would you use this in production code?
I would also prefer it if the initial_value was set in the constructor of the class, instead of as a decorator parameter, and then the function to be decorated returns that, as is the case with the standard @property decorator.
i.e
# (4)
class Axis:

    def __init__(self):
        self.__position=Meters(0.0)

    @observable_property
    def position(self):
        return self.__position

But I cannot do this as I can't mutate the self.__position from the __set__ descriptor. Is there any way to get the style of # (4), with the functionality of # (3)?
Thanks.

Private name mangling
The reason why you "can't mutate the self.__position from the __set__ descriptor" is the leading double underscore.  From 6.2.1. Identifiers (Names):

Private name mangling: When an identifier that textually occurs in a class definition begins with two or more underscore characters and does not end in two or more underscores, it is considered a private name of that class. Private names are transformed to a longer form before code is generated for them. The transformation inserts the class name, with leading underscores removed and a single underscore inserted, in front of the name. For example, the identifier __spam occurring in a class named Ham will be transformed to _Ham__spam. This transformation is independent of the syntactical context in which the identifier is used. If the transformed name is extremely long (longer than 255 characters), implementation defined truncation may happen. If the class name consists only of underscores, no transformation is done.

In the constructor, self.__position = 0.0 is rewritten to be self._Axis__position = 0.0.  When you are in the descriptor's __set__ method, getattr(obj, '__position') does not exists, because the name is wrong.
If you used a single leading underscore, instead of a double leading underscore, no name mangling will take place, and attribute can be referenced with the same name in both the main class and the descriptors.  This means you can set the initial values in the constructor, as desired.
Boilerplate
    @observable_property(initial_value=Meters(0.0))
    def position(self):
        pass

The def, (self): and pass parts of this are also boilerplate.  If you tried to use actual code, instead of the pass statement, the code would never be executed, because the descriptor's __get__ method is used instead.
The above code is approximately:
    position = observable_property(initial_value=Meters(0.0))(lambda self: pass)

If you removed the unused fget argument from the constructor, or removed the _observable_property constructor entirely, this could be reduced to:
    position = observable_property(initial_value=Meters(0.0))()

Since you don't need (or want) the initial value in the observable property descriptor, you want it in the class constructor, and the previous section showed why it wasn't working and how to fix it, we can remove the initial_value argument entirely, and with it the def observable_property wrapper around the class _observable_property.
Reworked code:
from typing import NewType

class ObservableProperty:

    def __set_name__(self, owner: type, name: str) -> None:
        def add_observer(obj, observer):
            if not hasattr(obj, self.observers_name):
                setattr(obj, self.observers_name, [])
            getattr(obj, self.observers_name).append(observer)

        self.private_name = f'_{name}'
        self.observers_name = f'_{name}_observers'
        setattr(owner, f'add_{name}_observer', add_observer)

    def __get__(self, obj, objtype=None):
        if obj is None:
            return self
        return getattr(obj, self.private_name)

    def __set__(self, obj, value):
        setattr(obj, self.private_name, value)
        for observer in getattr(obj, self.observers_name, []):
            observer(value)

Meters = NewType('Meters', float)
Kelvin = NewType('Kelvin', float)

class Axis:

    def __init__(self):
        self._position = Meters(0.0)
        self._temperature = Kelvin(273.0)

    position = ObservableProperty()
    temperature = ObservableProperty()

a = Axis()
print(a.position)     # Initial values from the constructor
print(a.temperature)
a.add_position_observer(lambda newval : print(f'New position for a: {newval}'))
a.add_temperature_observer(lambda newval : print(f'New temperature for a: {newval}'))
a.position = Meters(5.0)
a.temperature = Kelvin(300.0)

Composition
The ObservableProperty descriptor takes away all control of the attribute from the class.  The attribute cannot be calculated.  For instance, an observable attribute celsius could not read self.kelvin + 273, or write self.kelvin = value - 273 because it has no control over the get/set functionality.
What if you made an @observable decorator, which just added the observer capability, which could be used like:
class Axis:

    def __init__(self):
        self._position = 0.0

    @property
    def position(self) -> Meters:
        return self._position

    @observable
    @position.setter
    def position(self, value: Meters) -> None:
        self._position = value

Then, you could have validation in the setter (such as position can't be negative).
And if you wanted, you could create a SimpleProperty, which had the trivial getter & setter, and compose that into ObservableSimpleProperty and so on.


Computing the distribution of a Jonckheere-Terpstra statistic I have the following C++ function which I'd like to speed up, if possible. This program will have many different users, so parallelization is not really an option.

The C++ function is wrapped by C code and then called from R, using:

R CMD SHLIB -o jonckterp.so wrapper.cpp inside.cpp

from the command line, then:

dyn.load("jonckterp.so")

from within R.

inside.cpp

#include <stdio.h>
#include <vector>
#include <iostream>
#include <R.h>
#include <Rmath.h>

extern "C" void jonckterp(int *x,int *c, int *ngrp, double *comb);

int *finalcountdown;
static int count;

static int *tabInit(int num){
  int *w;
  w = (int *) R_alloc(num, sizeof(int));
  memset(w, '\0', num * sizeof(int));
  return(w);
}

static int mannwhit(std::vector<int> c, std::vector<int> sizes){
  int Ustat=0;
  int start=0;
  size_t Ngroups=sizes.size();
   for(int i=0; i<Ngroups-1;i++){
    for(int j=start;j<(start+sizes[i]);j++){
      for(int k=start+sizes[i];k<c.size();k++){
    if(c[j]<c[k])
      Ustat+=1;
      }
    }
    start+=sizes[i];
  }
   return(Ustat);
    }

void allocate_to_groups_impl( std::vector<char>& usage, std::vector<int>& order, const           std::vector<int>& cumsum, int group, int place, int min )
{
std::vector<int> store(order.size());
std::vector<int> sizes(cumsum.size());
sizes[0]=cumsum[0];
for(int i=1;i<cumsum.size();i++)
sizes[i]=(cumsum[i]-cumsum[i-1]);
    if (place == cumsum[group]) {
    group++; min = 0;
    if (group == cumsum.size()) {
      int ij=0;
      for( std::vector<int>::iterator it = order.begin(); it != order.end(); ++it ){
store[ij]=*it;
    ij++;
  }
  finalcountdown[count]=mannwhit(store,sizes);
  count++;
  return;
    }
  }

  for( int v = min, max = usage.size() + place - cumsum[group]; v <= max; ++v ) {
    if (!usage[v]) {
    order[place] = v;
    usage[v] = 1;
    allocate_to_groups_impl(usage, order, cumsum, group, place+1, v+1);
  usage[v] = 0;
    }
  }
}

void allocate_to_groups( int *c, int Ngroups )
{
  size_t sum_of_c = 0;
  std::vector<int> cumsum_of_c;
  for( int* it = c; it < c + Ngroups; ++it ){
    cumsum_of_c.push_back(sum_of_c += *it);
  }
  std::vector<int> order(sum_of_c);
  std::vector<char> usage(sum_of_c);
  allocate_to_groups_impl(usage, order, cumsum_of_c, 0, 0, 0);
}

void jonckterp(int *x,int *c, int *ngrp, double *numcomb){
  count=0;
  float p=0;
  int d[*ngrp];
  for(int i=0; i< *ngrp; i++)
    d[i]=c[i];
  finalcountdown =tabInit(*numcomb);
  allocate_to_groups(d,*ngrp);
  for(int test=0;test<*numcomb;test++){
  if(finalcountdown[test]>=*x){
    p+=1;
}
}
*x=p;
}

wrapper.cpp

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
extern "C" {
void jonckterp(int *x,int *c, int *ngrp, double *numcomb);
}

jonckterp.R

early.fn<-function(stat,group.sizes){
numcomb<-factorial(sum(group.sizes))/prod(factorial(group.sizes))
.C("jonckterp",p=as.integer(stat),as.integer(group.sizes),as.integer(length(group.sizes)),as.double(numcomb))$p/numcomb
}

I didn't read in it's entirity, but change this line

static int mannwhit(std::vector<int> c, std::vector<int> sizes)

to

static int mannwhit(const std::vector<int>& c,
                    const std::vector<int>& sizes)

This won't speed up your code but you should use

#include <cstdio> //instead of stdio.h

and memset will need to be prefixed as std::memset.

Also ensure you compiler is compiling at 02 and with out debug flags.


Maximum sum of non-consecutive elements of an array You are given a list of houses on a street, with the numeric value
  representing the amount of money stashed away in each house.

  Write a method to find the maximum amount of money you can steal. You
  cannot steal from any two adjacent houses, otherwise you will be
  caught! Use Dynamic Programming!

  Examples:

  robbery([60, 50, 20, 32, 20, 50]) → 142
  robbery([40, 25, 25, 30]) → 70

I would like to ask for a review of the following solution.

def robbery(amounts):
    amounts = iter(amounts)
    prev, total = 0, next(amounts, 0)
    for x in amounts: prev, total = total, max(total, prev + x)
    return total

Here's my code on Gist:
https://gist.github.com/Jeffchiucp/6dbd481ce4bc208e6da68bc8e920bf3f

The algorithm is conceptually fine: it's simple and it works.

I don't like the use of iter() and next(), though.  When you write for x in amounts, that will already treat amounts as an iterator.  The only benefit you get from an explicit amount = iter(amounts), then, is the ability to write next(amounts, 0).  And the only reason for writing next(amounts, 0) is to handle the special case where the input is an empty list.  You could just as well handle that by pretending that there is always another leading house with 0 money.

def robbery(amounts):
    prev, total = 0, 0
    for amount in amounts:
        prev, total = total, max(total, prev + amount)
    return total

The for loop would be better written with the conventional indentation.


Calculator that can add/subtract/divide/multiply two inputs Here I am making a calculator for a robotics assignment. I need the program to be able to take two inputs and convert to float. It also needs to display previous calculations when exiting and also stop when trying to divide by 0. It works I just want to make sure its clean and im not using any bad practices.
operators = ["+", "-", "*", "/"]
answers = []
def run_calculator():
    def instructions():
        try: chosen_operator = int(input("Choose an Operator:\n1) Addition\n2) Subtraction\n3) Multiplication\n4) Division\n\n5) Exit\nInput (1/2/3/4/5):"))
        except:
            print('Invalid Input: Choose A Valid Operation')
            return
        if chosen_operator > 4 or chosen_operator < 1:
            if chosen_operator == 5: return
            print('Invalid Input: Choose a Valid Operation')
            return
        operator = operators[chosen_operator - 1]
        try: firstnum = float(input('First Number:'))
        except:
            print('Invalid Input: Not a valid number')
            return
        try: secondnum = float(input('Second Number:'))
        except:
            print('Invalid Input: Not a valid number')
            return
        return operator, firstnum, secondnum
    while (True):
        try: operator, firstnum, secondnum = instructions()
        except: break
        if operator == "/" and secondnum == 0.0:
            print("Cannot divide by 0")
            break
        calc_function = float.__add__ if operator == "+" else float.__sub__ if operator == "-" else float.__mul__ if operator == "*" else float.__truediv__
        answers.append(f"{firstnum} {operator} {secondnum} = {calc_function(firstnum, secondnum)}")
        restart = input((f"{firstnum} {operator} {secondnum} = {calc_function(firstnum, secondnum)}\nWould you like to do another calculation? (Y/N)"))
        if restart.lower() == 'y':
            continue
        else:
            print("Results:")
            for x in range(len(answers)):
                print(f"{answers[x]}")
            break
run_calculator()

Immutable sequences
operators should be a tuple (i.e. ('+', ...)), since you don't change it.
Nested functions
instructions should be moved to the global level - you shouldn't need a closure, and should be passing information between instructions and run_calculator with parameters and return values, not closure variables.
Index-based selection
It's a little awkward to ask that the user select an operator based on numerical index. Make operators a dictionary instead, and have the user input the operator character. Of course, this is only possible if the parameters of your homework assignment permit.
Never except:
Seriously. except ValueError: instead. Otherwise, you'll catch exceptions you don't want to, including user-break Ctrl+C.
Parens
Python doesn't need parens here:
while (True):

so delete them.
Logic inversion
    if restart.lower() == 'y':
        continue
    else:
        print("Results:")
        for x in range(len(answers)):
            print(f"{answers[x]}")
        break

is more easily expressed as
    if restart.lower() != 'y':
        print("Results:")
        for x in range(len(answers)):
            print(f"{answers[x]}")
        break


Preventing XSS attacks in a Spring MVC application controller One of the URL in my application is vulnerable to XSS attack, so I am handling it in the below way.

I created a util class:

import java.util.regex.Pattern;

public class HtmlUtils {
    public final static String tagStart = "\\<\\w+((\\s+\\w+(\\s*\\=\\s*(?:\".*?\"|'.*?'|[^'\"\\>\\s]+))?)+\\s*|\\s*)\\>";

    public final static String tagEnd = "\\</\\w+\\>";

    public final static String tagSelfClosing = "\\<\\w+((\\s+\\w+(\\s*\\=\\s*(?:\".*?\"|'.*?'|[^'\"\\>\\s]+))?)+\\s*|\\s*)/\\>";

    public final static String htmlEntity = "&[a-zA-Z][a-zA-Z0-9]+;";

    public final static Pattern htmlPattern = Pattern.compile("(" + tagStart + ".*" + tagEnd + ")|(" + tagSelfClosing
            + ")|(" + htmlEntity + ")", Pattern.DOTALL);

    public static boolean isHtml(String s) {
        boolean ret = false;
        if (s != null) {
            ret = htmlPattern.matcher(s).find();
        }
        return ret;
    }
}

And in the controller, I am throwing an exception if the requested value has an HTML script:

def isValidRequest(request){
    def htmlRequest = false
    def params = request.getParameterNames();

    while(params.hasMoreElements()){
      String paramName = (String)params.nextElement();
      def value = request.getParameter(paramName);
       htmlRequest = HtmlUtils.isHtml(value);
         if(BooleanUtils.isTrue(htmlRequest))
          break;
      }

  if(BooleanUtils.isTrue(htmlRequest))
      throw new AccessDeniedException("Access Denied");
}

Does this look good or is there a better way to handle the XSS attack in the controller?

Don't use RegEx to prevent XSS attacks!
Why?

Regex is not a tool that can be used to correctly parse HTML.

There are much better ways to prevent XSS attacks.

An example, using your code, modified to use Spring HtmlUtils.
Please note that I changed names s to input and ret to isHtml, as these names indicate what the variable is intended for, rather than just what kind it is.
import org.springframework.web.util.HtmlUtils;

public class HtmlUtils {
    /**
     * Verify if a string contains any HTML characters by comparing its
     * HTML-escaped version with the original.
     * @param String input  the input String
     * @return boolean  True if the String contains HTML characters
     */
    public static boolean isHtml(String input) {
        boolean isHtml = false;
        if (input != null) {
            if (!input.equals(HtmlUtils.htmlEscape(input))) {
                isHtml = true;
            }
        }
        return isHtml;
    }
}

I'm sure there are other things to say about your code, but this is very important.
There is much more to truly preventing XSS attacks, depending on how critical your use case is. This page has a tremendous amount of useful information you may want to consider: XSS (Cross Site Scripting) Prevention Cheat Sheet


Encoding and assigning the result to a label I have to do some encoding and then assign the result to a label. Is there a more optimal (or concise) way to write this?

// encoding fix
NSString *correctStringTitle = [NSString stringWithCString: [[item objectForKey:@"main_tag"] cStringUsingEncoding:NSISOLatin1StringEncoding]
                                                  encoding: NSUTF8StringEncoding];

cell.titleLabel.text = [correctStringTitle capitalizedString];

Here is a marginally more concise solution:

cell.titleLabel.text = [NSString stringWithCString:[[item[@"main_tag"] cStringUsingEncoding:NSISOLatin1StringEncoding] encoding:NSUTF8StringEncoding].capitalizedString;

If I had to do this more than once I would consider hiding this complexity in an NSString category like:

@interface NSString(Utils)
    -(NSString)stringWithLatinEncoding;
@end

@implementation NSString(Utils)

-(NSString)stringWithLatinEncoding
{
     return [NSString stringWithCString:[self cStringUsingEncoding:NSISOLatin1StringEncoding] encoding:NSUTF8StringEncoding];
}

@end

This will simplify your code to:

cell.titleLabel.text = ((NSString*)item[@"main_tag"]).stringWithLatinEncoding.capitalizedString;


A heart.lisp I wrote with my 6 year old daughter Is it possible to improve this program somehow? It will probably be more convenient to suggest edits as forks of this Gist.

(defpackage heart
  (:use :cl))
(in-package :heart)

;; Usage:
;; sbcl -l heart.lisp -e '(uiop:quit 0)'
;;

(defun print-line (&rest args)
  (loop
     :for type :in args :by #'cddr
     :for num :in (cdr args) :by #'cddr
     :do (loop :repeat num
            :do (princ (ecase type
                         (:s " ")
                         (:h "❤")))))
  (format t "~%"))

(defun print-lines (data)
  ;; ensure there is a new line
  (format t "~&")

  (dolist (line data)
    (apply #'print-line line)))

(defvar *data*
  '((:s 8 :h 8 :s 8 :h 8)
    (:s 3 :h 15 :s 4 :h 15)
    (:s 1 :h 18 :s 2 :h 18)
    (:s 0 :h 40)
    (:s 0 :h 40)
    (:s 0 :h 40)
    (:s 1 :h 38)
    (:s 2 :h 36)
    (:s 4 :h 32)
    (:s 6 :h 28)
    (:s 8 :h 24)
    (:s 11 :h 18)
    (:s 13 :h 14)
    (:s 15 :h 10)
    (:s 17 :h 6)
    (:s 19 :h 2)))

(print-lines *data*)

That's a great start! Looks like a lot fun.

I would do only a few things slightly different. Some of them a real improvements and some are style preferences of an old Lisper... ;-)

First function: PRINT-LINE

(defun print-line (&rest args)
  (loop
     :for type :in args :by #'cddr
     :for num :in (cdr args) :by #'cddr
     :do (loop :repeat num
            :do (princ (ecase type
                         (:s " ")
                         (:h "❤")))))
  (format t "~%"))

There are two real improvements:

don't use a &rest arg, pass the list directly
the outer LOOP can be written slightly simpler with destructuring

New version:

(defun print-line (line)
  (loop for (type num) on line by #'cddr
        do (loop repeat num
                 do (write-char (ecase type
                                  (:s #\space)
                                  (:h #\❤)))))
  (terpri))

The second function: PRINT-LINES

(defun print-lines (data)
  ;; ensure there is a new line
  (format t "~&")

  (dolist (line data)
    (apply #'print-line line)))

There is one real improvement:

don't use apply, just pass the list directly

New version:

(defun print-lines (data)
  (terpri)
  (dolist (line data)
    (print-line line)))

The data variable

(defvar *data*
  '((:s 8 :h 8 :s 8 :h 8)
    ...
    (:s 19 :h 2)))

It helps to add a documentation string, or to give it a useful name.

(defvar *heart*
  '((:s 8 :h 8 :s 8 :h 8)
    ...
    (:s 19 :h 2)))

or

(defvar *data*
  '((:s 8 :h 8 :s 8 :h 8)
    ...
    (:s 19 :h 2))
  "The description of a heart.")


JavaScript Etch-A-Sketch implementation from The Odin Project I'm working through the curriculum at The Odin Project. I recently completed the Etch-A-Sketch project and am looking for some help/feedback regarding my code. Specifically, I feel that the functions in my JavaScript contain too much repeat code. But, I'm having trouble understanding how to refactor the functions without breaking them.

Github HTML Preview

Full project code

const gridContainer = document.getElementById('gridContainer');

    function createGrid(size){
        let totalSquares = size * size;
        for (let i = 0; i < totalSquares; i++){
            const newSquare = document.createElement('div');
            newSquare.classList.add('newSquare');
            gridContainer.appendChild(newSquare);
            newSquare.addEventListener('mouseover', function(e){
            newSquare.style.backgroundColor = 'black';
            });

        }
        document.documentElement.style.setProperty("--rowNum", size);
        document.documentElement.style.setProperty("--colNum", size);
    }

    function pencilGrid(size){
        let totalSquares = size * size;
        for (let i = 0; i < totalSquares; i++){
            const newSquare = document.createElement('div');
            newSquare.classList.add('newSquare');
            gridContainer.appendChild(newSquare);
            newSquare.addEventListener('mouseover', function(e){
            newSquare.style.backgroundColor = 'black';
            newSquare.style.opacity -= '-0.1';
            });

        }
        document.documentElement.style.setProperty("--rowNum", size);
        document.documentElement.style.setProperty("--colNum", size);
    }

    function colorfulGrid(size){
        let totalSquares = size * size;
        for (let i = 0; i < totalSquares; i++){
            const newSquare = document.createElement('div');
            newSquare.classList.add('newSquare');
            gridContainer.appendChild(newSquare);
            newSquare.addEventListener('mouseover', function(e){
                let randomColor = Math.floor(Math.random()*16777215).toString(16);
            newSquare.style.backgroundColor = randomColor;
            newSquare.style.border = '0px';
            });

        }
        document.documentElement.style.setProperty("--rowNum", size);
        document.documentElement.style.setProperty("--colNum", size);
    }

    function clearGrid(){
        while (gridContainer.firstChild){
            gridContainer.removeChild(gridContainer.firstChild);
        }
    }

    createGrid(16);

    const newGridButton = document.getElementById('newGridButton');
    newGridButton.addEventListener('click', function(e){
        clearGrid();
        let newSize = prompt('New Grid Size:');
        createGrid(newSize);

    });

    const pencilButton = document.getElementById('pencilButton');
    pencilButton.addEventListener('click', function(e){
        clearGrid();
        let newSize = prompt('New Grid Size')
        pencilGrid(newSize);
    });

    const colorfulButton = document.getElementById('colorfulButton');
    colorfulButton.addEventListener('click', function(e){
        clearGrid();
        let newSize = prompt('New Grid Size')
        colorfulGrid(newSize);
    });

Bug
There is a small chance  (about 1 in 16) that the random color function will generate an bad color as you don't check for leading zeros. See code below for an alternative.
DRYing out code (Don't Repeat Yourself)
Yes there is way to much repeated code.
Repeated code is not only a pain to create, it is a source of bugs when it comes time to make changes.
Differences as arguments.
To reduce repetition we use functions to wrap up code that has only slight differences, passing the differences as arguments.
For example you have the 3 button event handlers with 3 sections of almost identical code.

const newGridButton = document.getElementById('newGridButton');
newGridButton.addEventListener('click', function(e){
    clearGrid();
    let newSize = prompt('New Grid Size:');
    createGrid(newSize);
});

const pencilButton = document.getElementById('pencilButton');
pencilButton.addEventListener('click', function(e){
    clearGrid();
    let newSize = prompt('New Grid Size')
    pencilGrid(newSize);
});

const colorfulButton = document.getElementById('colorfulButton');
colorfulButton.addEventListener('click', function(e){
    clearGrid();
    let newSize = prompt('New Grid Size')
    colorfulGrid(newSize);
});

The only difference in each is the Id of the button and the function called at the end. Thus we can simply wrap one of those sections in a function passing the Id and the setup function as arguments.
function setUpButton(buttonId, createGrid) {
    const button = document.getElementById(buttonId);
    button.addEventListener('click', function(e){
        clearGrid();
        let newSize = prompt('New Grid Size')
        createGrid(newSize);
    });
}

Reduce code noise
Shorten it little by removing code noise,

The prompt can move into the function call.
The clearGrid can move to the createGrid function
We can use direct DOM reference for the button element when we call the function, so we don't need document.getElementById
Use an arrow function for the event, and we don't need to event argument.

Thus we get the whole thing done in 6 lines.
function setUpButton(button, createGrid) {
    button.addEventListener('click', () => createGrid(prompt('New Grid Size')))
}
setUpButton(newGridButton, createGrid);
setUpButton(pencilButton, pencilGrid);
setUpButton(colorfulButton, colorfulGrid);

18 lines down to 6.
Using names to reference functions
Looking at the create grid function the only difference is the mouse event, all the rest is identical. Create the mouse event functions separately. We need a reference to the newSquare, that can be found in the event as event.target
To make accessing the functions easier we can add them to an object and use their names in the create grid function.
const draw = {
    colorful(e) {
        e.target.style.background = (Math.random()*0xFFFFFF|0).toString(16).padStart(7,"#000000");
        e.target.style.border = '0px';
    },
    pencil(e) {
        e.target.style.background = 'black';
        e.target.style.opacity += 0.1;
    },
    create(e) { e.target.style.background = 'black' }
}

and modify the create grid function to take the name of the draw function as well as the size. Also move the clear grid function into this function.
The end result
Putting it all together we get it all done in about half the code.
const drawing = {
    colorful(e) {
        e.target.style.background = (Math.random()*0xFFFFFF|0).toString(16).padStart(7,"#000000");
        e.target.style.border = "0px";
    },
    pencil(e) {
        e.target.style.backgroundColor = 'black';
        e.target.style.opacity += 0.1;
    },
    black(e) { e.target.style.backgroundColor = 'black' },
};
setUpButton(newGridButton, "black");
setUpButton(pencilButton, "pencil"");
setUpButton(colorfulButton, "colorful");
createGrid("black");

function setUpButton(button, name) {
    button.addEventListener('click', () => createGrid(name, prompt('New Grid Size')))
}

function createGrid(name, size = 16){
    var count = size ** 2;
    while (gridContainer.firstChild) { gridContainer.removeChild(gridContainer.firstChild) }
    while (count--) {
        const cell = Object.assign(document.createElement("div"), {className : "newSquare"});
        gridContainer.appendChild(cell);
        cell.addEventListener("mouseover", drawing[name]);
    }
    ["--rowNum","--colNum"].forEach(p => document.documentElement.style.setProperty(p, size));
}

I used direct element reference for gridContainer, newGridButton, colorfulButton, and pencilButton so you must ensure that they are unique ids on the page.


99 Haskell problems, problem 18 The problem is here, the solutions are here. I don't see a solution like mine (maybe the 4th), it seems to work though.

-- ---------------------------
-- 18 (**) Extract a slice from a list (idx start from 1)

slice _      _ 0 = []
slice (x:xs) 1 k = x : slice xs 1 (k-1)
slice (x:xs) i k = slice xs (i-1) (k-1)

I tried testing it with QuickCheck against the drop/take solution, but it gives up because of the very restrictive conditions (1 <= i <= k <= len xs) and I don't want to get deeper in testing right now.

Is this good Haskell code? Am I forgetting any edge case? (...should I suggest it on the solutions wiki?)

Yes you forgot to cover some cases.

Your compiler should be able to detect one of them. Here is what ghc -Wall says:

slice.hs:2:1: Warning:
    Pattern match(es) are non-exhaustive
    In an equation for `slice':
        Patterns not matched: [] _ (GHC.Types.I# #x) with #x `notElem` [0#]

slice.hs:4:8: Warning: Defined but not used: `x'

This version fixes those two warnings:

slice []     _ _ = []
slice _      _ 0 = []
slice (x:xs) 1 k = x : slice xs 1 (k-1)
slice (_:xs) i k = slice xs (i-1) (k-1)

But it is still incorrect, because there is another case that the compiler doesn't warn you about: a negative k value. Your code will return the entire list instead of an empty list.

Consequently, if you use a negative k on an infinite list you get an infinite loop:

> print $ slice [1..] 1 (-1)
[1,2,3,4,5,6,7,8,9,...


Could someone check the Minimax Algorithm correctly implemented? I'm working on AI tic-tac-toe that computer make a move and I wanted to know that I correctly implemented the minimax algorithm. Please someone check my code to identify a problem or any kind of bug. Any help would be greatly appreciated.

I wanted to know that my code is correctly running the minimax algorithm according to this image.

test-cases-for-tic-tac-toe

from math import inf as infinity

def winner(game, turn):
    win_game = [
        [game[0][0], game[0][1], game[0][2]],
        [game[1][0], game[1][1], game[1][2]],
        [game[2][0], game[2][1], game[2][2]],
        [game[0][0], game[1][0], game[2][0]],
        [game[0][1], game[1][1], game[2][1]],
        [game[0][2], game[1][2], game[2][2]],
        [game[0][0], game[1][1], game[2][2]],
        [game[2][0], game[1][1], game[0][2]],
    ]
    if [turn, turn, turn] in win_game:
        return True
    else:
        return False

def draw(game):
    """ Check whether the board is full """
    counter = 0
    for i in range(3):
        for j in range(3):
            if game[i][j] == "X" or game[i][j] == "O":
                counter+=1
    return counter == 9

def game_over(game, turn):
    return winner(game, turn) or draw(game)

def available_moves(game):
    cell = []
    for i in range(3):
        for j in range(3):
            if not(game[i][j] == "X" or game[i][j] == "O"):
                cell.append([i, j])
    return cell

def minimax(game, turn):
    if winner(game, "X"):
        return 1
    elif winner(game, "O"):
        return -1
    elif draw(game):
        return 0

    moves = available_moves(game)
    print(moves)

    if turn == "X":
        value = -infinity
        for move in moves:
            tmp = game[:][:]
            tmp[move[0]][move[1]] = "X"
            value = max(value, minimax(game, "O"))
            tmp[move[0]][move[1]] = None
            x, y = move[0], move[1]
    else:
        value = infinity
        for move in moves:
            tmp = game[:][:]
            tmp[move[0]][move[1]] = "O"
            value = min(value, minimax(game, "X"))
            tmp[move[0]][move[1]] = None
            x, y = move[0], move[1]
    return value

if __name__ == '__main__':

    game = [
        ["O", None, None],
        ["O", "X", "X"],
        [None, "O", "X"]]

    print(minimax(game, "X"))

tmp = game[:][:] is not doing what you think it is doing.  game[:] returns a slice of all the rows of game (so a list of columns).  To that, you apply the slice operator [:], which takes the slice of the row list (of columns), and returns a row list (of columns).  In short, game[:] is the same as game[:][:], which is the same as game[:][:][:][:][:].

In particular, tmp is list which contains references to the same column lists as game, so tmp[row][col] accesses exactly the same entry as game[row][col].  You have not created a temporary copy of game; you have created a copy of references to the same column list objects.

As such, you can remove tmp, and just set game[row][col] directly, and set it back to None like you are doing via tmp.

The code x, y = move[0], move[1] seems to be useless, since x and y are never used.

The minimax() function returns the best possible outcome for that player, but doesn’t return the move that corresponds to that outcome.  “You have a guaranteed winning move, but I’m not going to tell you what it is.”

You might want to return the move that corresponds to that outcome.


Converting Roman numerals to decimal Could someone please point out the error(s) in the given code?  It was downvoted on Stack Overflow without any explanation, but it seems to be working fine for me:

int value(char roman)
{
   switch(roman)
   {
        case 'I':return 1;
        case 'V':return 5;
        case 'X':return 10;
        case 'L':return 50;
        case 'C':return 100;
        case 'D':return 500;
        case 'M':return 1000;
   }
}

int getdec(const string& input)
{
  int sum=0; char prev='%';
  for(int i=(input.length()-1); i>=0; i--)
  {
    if(value(input[i])<sum && (input[i]!=prev))
    {       sum -= value(input[i]);
            prev = input[i];
    }
    else
    {
            sum += value(input[i]);
            prev = input[i];
    }
  }
  return sum;
}

This was the output received from the code:

  I = 1 II = 2 III = 3 IV = 4 V = 5 VI = 6 VII = 7 VIII = 8 IX = 9 X = 10 XI = 11 XII = 12 XIII = 13 XIV = 14 XV = 15 XVI = 16 XVII = 17 XVIII = 18 XIX = 19 XX = 20 XXI = 21 XXII = 22 XXIII = 23 XXIV = 24 XXV = 25 XXVI = 26 XXVII = 27 XXVIII = 28 XXIX = 29 XXX = 30 XXXI = 31 XXXII = 32 XXXIII = 33 XXXIV = 34 XXXV = 35 XXXVI = 36 XXXVII = 37 XXXVIII = 38 XXXIX = 39 XL = 40 MMMMCMXCIX = 4999 CM = 900 XC = 90

If you have only valid Roman numbers you could use a simpler algorithms.  (i.e IIV is invalid )

  The symbols "I", "X", "C", and "M" can be repeated three times in succession, but no more. (They may appear more than three times if
  they appear non-sequentially, such as XXXIX.) "V", "L", and "D" can
  never be repeated. A common exception to this is the use of IIII on
  clocks; see below.
  "I" can be subtracted from "V" and "X" only. "X" can be subtracted from "L" and "C" only. "C" can be subtracted from "D" and "M" only.
  "V", "L", and "D" can never be subtracted
  Only one small-value symbol may be subtracted from any large-value symbol.

Just (string-)replace IV by IIII, IX by VIIII and so on. Afterwards you just have to sum the numbers from left to right.


Expression evaluators, with possible short circuit evaluation I'm writing a project, and the part I'm doing now is getting bloated real fast. How can I remove this nested if statements, but still have the same behaviour?

It would be nice the the resulting code was succinct, and extra local flags weren't required. This might not look so bad now, but I'm planning on writing more methods, up to T16, you can imagine how bad these nested if statements will get.

using ContainerExpressions.Containers;
using System;

namespace ContainerExpressions.Expressions.Core
{
    internal static class Compose
    {
        public static Response<T> Evaluate<T>(Func<Response<T>> func) => func();

        public static Response<TResult> Evaluate<T1, TResult>(Func<Response<T1>> func1, Func<T1, Response<TResult>> funcResult)
        {
            var response = new Response<TResult>();

            var result = func1();
            if (result)
            {
                response = response.WithValue(funcResult(result));
            }

            return response;
        }

        public static Response<TResult> Evaluate<T1, T2, TResult>(Func<Response<T1>> func1, Func<T1, Response<T2>> func2, Func<T2, Response<TResult>> funcResult)
        {
            var response = new Response<TResult>();

            var result1 = func1();
            if (result1)
            {
                var result2 = func2(result1);
                if (result2)
                {
                    response = response.WithValue(funcResult(result2));
                }
            }

            return response;
        }

        public static Response<TResult> Evaluate<T1, T2, T3, TResult>(Func<Response<T1>> func1, Func<T1, Response<T2>> func2, Func<T2, Response<T3>> func3, Func<T3, Response<TResult>> funcResult)
        {
            var response = new Response<TResult>();

            var result1 = func1();
            if (result1)
            {
                var result2 = func2(result1);
                if (result2)
                {
                    var result3 = func3(result2);
                    if (result3)
                    {
                        response = response.WithValue(funcResult(result3));
                    }
                }
            }

            return response;
        }
    }
}

If your code will always behave as the parts shown, you can refactor it to early-out returns. I show your Evaluate<T1, T2, T3, TResult>(...) as an example:

public static Response<TResult> Evaluate<T1, T2, T3, TResult>(Func<Response<T1>> func1, Func<T1, Response<T2>> func2, Func<T2, Response<T3>> func3, Func<T3, Response<TResult>> funcResult)
{
    var response = new Response<TResult>();

    if (!func1()) return response;
    if (!func2(true)) return response;
    if (!func3(true)) return response;
    return response.WithValue(funcResult(result3));
}

So you get rid of the nesting if statements. And you don't need the local flag variables since you know that the value is true.


Fetching BasicInfo, BillingInfo, and DocumentInfo objects The repetition is super obvious here. How can I rewrite this code? I need one base method in which I pass a Django model, to call objects.filter, but I can't figure out how to do it.

from profiles.models import BasicInfo, BillingInfo, DocumentInfo

__author__ = 'vol'

class BasicInfoService:
    def __init__(self):
        pass

    @staticmethod
    def get_by_profile_id(profile_id, is_versioned):
        if is_versioned:
            return BasicInfo.objects.filter(profile_id=profile_id).order_by('-version')
        return BasicInfo.objects.filter(profile_id=profile_id).order_by('-version')[:1][0]

class BillingInfoService:
    def __init__(self):
        pass

    @staticmethod
    def get_by_profile_id(profile_id, is_versioned):
        if is_versioned:
            return BillingInfo.objects.filter(profile_id=profile_id).order_by('-version')
        return BillingInfo.objects.filter(profile_id=profile_id).order_by('-version')[:1][0]

class DocumentInfoService:
    def __init__(self):
        pass

    @staticmethod
    def get_by_profile_id(profile_id, is_versioned):
        if is_versioned:
            return DocumentInfo.objects.filter(profile_id=profile_id).order_by('-version')
        return DocumentInfo.objects.filter(profile_id=profile_id).order_by('-version')[:1][0]

The template method pattern seems useful here:

Create a base class with a get_by_profile_id method that calls a get_queryset method to get the at appropriate QuerySet object to work with
The base class doesn't implement get_queryset, it's the job of the subclasses. This is the template method

Something along the lines of this in the base class:

def get_by_profile_id(self, profile_id, is_versioned):
    queryset = self.get_queryset().filter(profile_id=profile_id).order_by('-version')
    if is_versioned:
        return queryset
    return queryset[:1][0]

And in the subclass for BasicInfo:

def get_queryset(self):
    return BasicInfo.objects

Notice that I eliminated another duplication in the following and ordering logic. Note that Djangi will not execute the query until elements are accessed in the query set, so when I initialize queryset before the if condition, that doesn't hit the database.


Solve binomial coefficient without lookup I want to solve for n in the expression (binomial coefficient):
\$y=\frac{n^2+n}2\$
let y = (Math.pow(n, 2) + n) / 2;

I know the \$y\$ value, but I want to compute \$n\$.

Here is what I have so far, but I would want to know if there was a way to compute the value rather than look it up. In this example, I have to lookup the max value in a reversed array that is constrained by a length of 10.
Edit: Here is another avenue I took (still not an expression):
// Iterative
const toBase = n => {
  if (n < 1) return 0;
  let value = n, delta = 0;
  while (value > 0) value -= delta++;
  return delta - 1;
};

// Recursive
const toBase = (n, delta = 0) =>
  n === 0 && delta === 0
    ? 0
    : n < 1
      ? delta - 1
      : toBase(n - delta, delta + 1);

const MAX_BASES = 10;
const range = n => new Array(MAX_BASES).fill(0);
const binomialCoefficient = n => (Math.pow(n, 2) + n) / 2;
const baseMax = range(MAX_BASES).map((v, i) => binomialCoefficient(i));
const toBase = n => baseMax.indexOf([...baseMax].reverse().find(max => n > max)) + 1;

const main = () => {
  const inputs = getInputs().trim().split(/\n/)
    .map(l => l.trim().split(/\s+/g).map(v => parseInt(v.trim(), 10)))
    .filter(([v]) => !isNaN(v));
  inputs.forEach(input => validate(...input));
};

const validate = (n, expected) => {
  const actual = toBase(n);
  if (actual !== expected) {
    throw new Error(`${actual} !== ${expected} for n=${n}`);
  }
  console.log(`${n}: ${expected} === ${actual} | valid!`);
};

const getInputs = () => `
 0 0

 1 1

 2 2
 3 2

 4 3
 5 3
 6 3

 7 4
 8 4
 9 4
10 4

11 5
12 5
13 5
14 5
15 5

16 6
17 6
18 6
19 6
20 6
21 6

22 7
23 7
24 7
25 7
26 7
27 7
28 7

29 8
30 8
31 8
32 8
33 8
34 8
35 8
36 8
`;

main();
.as-console-wrapper { top: 0; max-height: 100% !important; }
14

14 - 1 =  13
13 - 2 =  11
11 - 3 =   8
 8 - 4 =   4
 4 - 5 =  -1

Background: I noticed a pattern which looked very similar to a factorial expression, but with addition rather than multiplication. After some research, I stumbled upon the following question which lead me in the right direction:

"Factorial, but with addition" - Mathematics Stack Exchange

A review of the first 5 lines
Only looking at the code related to the problem and ignoring the testing code as it looks haphazardly put together.
Thus

const MAX_BASES = 10;
const range = n => new Array(MAX_BASES).fill(0);
const binomialCoefficient = n => (Math.pow(n, 2) + n) / 2;
const baseMax = range(MAX_BASES).map((v, i) => binomialCoefficient(i));
const toBase = n => baseMax.indexOf([...baseMax].reverse().find(max => n > max)) + 1;

Minor point
You should use the operator ** (Exponentiation) rather than the math function Math.pow . Both ** and Math.pow use the same pow function under the hood so there is no performance difference, however ** is cleaner.
The bad part
The only thing that is bad in your code, even assuming you are forced to lookup the values, is that every time you call toBase you recreate the array, reverse it, and then search it.
To get the correct value subtract the index from the arrays length. There is an edge case when the input is 0. That can be handled with a ternary.
You only need to reverse the array once.
 const baseMax = range(MAX_BASES).map((v, i) => binomialCoefficient(i)).reverse();

Subtract the index from the arrays length to get the result you are after. The edge case n = 0 can be handled with a ternary.
 const toBase = n => !n ? 0 : baseMax.length - baseMax.indexOf(baseMax.find(max => n > max));

Rather than use Array.find you can use Array.findIndex This will save you having to search the array twice, once to find the value n <= max then again to find the index with Array.indexOf
Thus you get
 const toBase = n => !n ? 0 : baseMax.length - baseMax.findIndex(max => n > max);

And you don't need to reverse the array which makes it even simpler
 const baseMax = range(MAX_BASES).map((v, i) => binomialCoefficient(i));
 const toBase = n => !n ? 0 : baseMax.findIndex(max => n <= max);

A functional solution.
The function (n ** 2 + n) / 2 is a quadratic \$\frac{1}2n^2 + \frac{1}2n + 0 = y\$
You can solve the quadratic when \$y=0\$ using \$\begin{array}{*{20}c} {x = \frac{{ - b \pm \sqrt {b^2 - 4ac} }}{{2a}}} \\ \end{array}\$
However as we change \$n\$ the solution we are after is no longer at \$y = 0\$
If we move the parabola down so that \$y\$ is at 0 for the value \$n\$ we are solving for we can then solve for any value of \$n\$
The quadratic to solve is \$\frac{1}2x^2 + \frac{1}2x - n = 0\$
\$ {x=\frac{{-\frac{1}2+\sqrt{\frac{1}4-4*\frac{1}2*-n} }}{{2*\frac{1}2}}}\$
which is
\$x=-\frac{1}2+\sqrt{\frac{1}4+2n} \$
Now we get \$x\$ for \$n\$ and round it up when \$x\$ is a fraction.
The rewrite
An thus the 5 lines become one line.
const toBase = n => Math.ceil(-0.5 + (0.25 + 2 * n) ** 0.5);


Finding unique pairs in lottery tickets I was trying to solve one of the hackerrank practice questions. Could anyone please let me know how can I make this code much effective, in terms of time and space complexity?

  The Super Bowl Lottery is about to commence, and there are several
  lottery tickets being sold, and each ticket is identified with a
  ticket ID. In one of the many winning scenarios in the Superbowl
  lottery, a winning pair of tickets is:

  Concatenation of the two ticket IDs in the pair, in any order,
  contains each digit from 0 to 9 at least once.

  For example, if there are 2 distinct tickets with ticket ID 12930455 and 56789, (129300455, 56789) is a winning pair.

  NOTE: The ticket IDs can be concantenated in any order. Digits in the
  ticket ID can occur in any order.

  Your task is to find the number of winning pairs of distinct tickets,
  such that concatenation of their ticket IDs (in any order) makes for a
  winning scenario. Complete the function winningLotteryTicket which
  takes a string array of ticket IDs as input, and return the number of
  winning pairs.

  Input Format

  The first line contains n denoting the total number of lottery tickets
  in the Super Bowl. Each of the next n lines contains a string, where
  string on a ith line denotes the ticket id of the ith ticket.

  Constraints

  [1 ≤ pretty much everything  input ≤ 10⁶]
  Each ticket id consists of digits from [0, 9]

  Output Format

  Print the number of pairs in a new line.

  Sample Input

5
129300455
5559948277
012334556
56789
123456879

  Sample Output

5

import java.io.*;
import java.util.*;
import java.text.*;
import java.math.*;
import java.util.regex.*;

public class Solution {
    static Long winningLotteryTicket(String[] tickets) {
        int count = 0;
        for(int i = 0 ; i < tickets.length-1 ; i++) {
            for(int j = i+1 ; j < tickets.length ;j++ ){
               if(!(tickets[i]).equals(tickets[j]) && getStatus(tickets[i],tickets[j])){
                   count++;
               }
            }
        }
        return Long.valueOf(count);
    }

    public static boolean getStatus(String a, String b){
        String c = a+b;
        if(c.length() < 10){
            return false;
        }
        Set<Character> charSet = new HashSet<>();
        char[] arr = c.toCharArray();
        for(int i = 0; i < arr.length; i++){
            charSet.add(arr[i]);
        }

        if (charSet.size() == 10){
            return true;
        } else {
            return false;
        }
    }

    public static void main(String[] args) {
        Scanner in = new Scanner(System.in);
        int n = in.nextInt();
        String[] tickets = new String[n];
        for(int tickets_i = 0; tickets_i < n; tickets_i++){
            tickets[tickets_i] = in.next();
        }
        Long result = winningLotteryTicket(tickets);
        System.out.println(result);
        in.close();
    }
}

try-with-resources

Since Java 7, you can use try-with-resources for safe and efficient handling of the underlying I/O resource.

return boolean

if (condition) {
    return true;
} else {
    return false;
}

This kind of code can be simplified as return condition.

Method names

Your naming can be better refined to reflect what they are doing. For example, getStatus can be renamed as hasUniqueNumerals, following the standard is/has prefix for methods returning a boolean. winningLotteryTicket can be renamed as countWinningPairs.

for-each loop

Your loop on c.toCharArray() can also be written as:

for (char current : c.toCharArray()) {
    charSet.add(current);
}

What's nice

You checked if the concatenation of the two inputs will give you 10 or more digits, returning false first if not.
You declared charSet as a Set rather than a HashSet and relied on generic type inference.


Returning a list of objects Please review the following code. Methods getFirsts and getSeconds, both of which are private, return a list of objects which implement CommonInterface. Is this a good or bad design?

@Override
public final List<? extends CommonInterface> getObjects(final CommonEnum type) {
    if (type == null) {
        return new ArrayList<CommonInterface>();
    }
    switch (type) {
        case FIRST:
            return getFirsts();
        case SECOND:
            return getSeconds();
        default:
            return null;
    }
}

It's hard to say anything since it seems rather a pseudo-code. Anyway, two notes which you might find useful:

I guess you could replace the switch-case structure with polymorphism. Two useful reading:

Refactoring: Improving the Design of Existing Code by Martin Fowler: Replacing the Conditional Logic on Price Code with Polymorphism
Replace Conditional with Polymorphism

Are you sure that returning null in the default is fine? I'd consider returning an empty list (as it returns when type == null) or throwing an exception (IllegalStateException, for example) if it's a programming error. (See: The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies)


Design To Handle Multiple Similar Model Class I am doing a web app for an event company whereby customer can access the website for a self-help instant quotation. Customer will choose the type of event, fill a form and expect an instant quotation.

Can anyone give me some tips how can I improve my design? Below are my current design thoughts.

Quotation class:

public class Quotation{

    // Customer specific details
    private String customerName;
    private String customerEmail;
    private String customerMobile;

    // Event specific details
    private WeddingEvent weddingEvent;
    private RetreatEvent retreatEvent;
    // ... another 4 or 5 more event classes

    // getters and setters
    // ...
}

Event Classes:

public class BaseEvent{
    private int attendance;
    private String venue;
}

public class WeddingEvent extends BaseEvent{
    private boolean needWeddingPlanner;
    // getters and setters
    // ...
}

public class RetreatEvent extends BaseEvent{
    private boolean needBreakfast;
    private boolean needLunch;
    private boolean needDinner;

    // getters and setters
    // ...
}

I would suggest the following potential improvements.

Change the Quotation class to the following

public class Quotation
{
    private Customer customer;

    private List<BaseEvent> events = new List<BaseEvent>();

    // Getters and setters to access the two entities above.
}

So now you have divided you elements into specific entities like Quotation, Customer and Events.

Adding more details to a Customer or adding a new Event type, don't require change to the Quotation class, but only changes to the specific entity.

On the side, you might also find it beneficial to look into a simple visitor pattern for the events and provide algorithms to etc. calculate total price, date collisions, number of participants etc. etc.

Enjoy.


Design/reiterate or code reuse for linked list This is what my class hierarchy looks like. I have an abstract superclass, AbstractLinkedMyList, that contains common operations for both sorted and unsorted linked list.

Here's the declaration for it and the method I have a question about:

public abstract class AbstractLinkedMyList<Type> implements MyList<Type> {
    protected ListNode<Type> front;
     protected ListNode<Type> back;

public void insertAtIndex(int index, Type value) {
        checkOkToInsertAtIndex(index);
        if (size == 0) {
            ListNode<Type> newNode = new ListNode<Type>(value);
             front = back = newNode;
         }
        else {
            if (index == 0) {
                   ListNode<Type> newNode = new ListNode<Type>(value, front);
             front = newNode;
            }
           else {
                  ListNode<Type> current = nodeAt(index - 1);
                  ListNode<Type> newNode = new ListNode<Type>(value, current.next);
                 current.next = newNode;
                 if (index == size) {
                     back = newNode;
                 }
            }
       }
   size++;
    }
     protected ListNode<Type> nodeAt(int index) {
         ListNode<Type> current = front;
         for (int i = 1; i <= index; i++) {
                current = current.next;
          }
         return current;
    }
  protected static class ListNode<Type> {
        public Type data;
        public ListNode<Type> next;
        public ListNode(Type data) {
             this(data, null);
        }
        public ListNode(Type data, ListNode<Type> next) {
             this.data = data;
             this.next = next;
        }
     }
    .....
}

Here is my declaration for the sorted linked list:

public class LinkedListSorted<E extends Comparable<E>> extends AbstractLinkedMyList<E>

My question is about the insertAtIndex method. I chose to put the insertAtIndex method in the abstract superclass because this would be how you insert something at an index, no matter what the subclass is (sorted vs unsorted). I realized that in the sorted linked list class, I will have to extend this method to impose restrictions(to not break sorted property of the list).

This is my insertAtIndex method (what I have so far) in the sorted linked list:

public class LinkedListSorted<E extends Comparable<E>> extends AbstractLinkedMyList<E>
@Override
public void insertAtIndex(int index, E value) {
    if(index == 0 && front != null && value.compareTo(front.data) > 0) {
        throwSortException();
    }
    if(index == size && value.compareTo(back.data) < 0) {
        throwSortException();
    }
    if(index > 0 && index < size) {
        ListNode<E> before = nodeAt(index - 1);
        if(value.compareTo(before.data) < 0 || value.compareTo(before.next.data) > 0) {
            throwSortException();
        }
    }
    super.insertAtIndex(index, value);
}

My logic here is here are all the cases where you cannot insert (would break the sorted property of the list). If you can make it past all these cases, I will allow insertAtIndex, whose implementation is in the abstract superclass (explained is this decision above).

Would this be good code design? One drawback I saw right away was that my check logic involved a lot of iteration. You would need to this iteration again if you actually wanted to insert. But then if you put the insert logic in the check case, you would be rewriting code (not taking advantage of code reuse). How would you weight the two? Is what I have acceptable?

No, it would not be good design. That method signature says "you can insert an element at any point in the list" and then the body says "but only if it's the place I want you to put it." This is why the Collections API doesn't have a sorted list, but rather allows lists to be sorted. Sorted and Unsorted lists share some contract, but insertion is fundamentally different for them. Unless your assignment requires it, I would not put them in the same inheritance hierarchy, and I'd be leery of putting them under the same interface (contract) unless it didn't include addition of elements.


Lambda to obtain a new merged object The requirement is to iterate over a list of Foos, and when 2 Foos have the same id merge their list of inputs together and obtain a new object.

I have achieved this with 2 streams and a private method to do this:

import java.util.HashSet;
import java.util.Set;

class Foo {
    private String id;
    private Set<String> inputs = new HashSet<>();

    String getId() {
        return id;
    }

    void setId(String id) {

        this.id = id;
    }

    Set<String> getInputs() {
        return inputs;

    }
}

and the GetMatchingFoos class:

import static java.util.stream.Collectors.groupingBy;
import static java.util.stream.Collectors.toList;

import java.util.List;
import java.util.Map;

public class GetMatchingFoos {

    public List<Foo> call(List<Foo> matchedFoos) {
        Map<String, List<Foo>> idToMatchedFooMap = matchedFoos.stream()
                .collect(groupingBy(Foo::getId, toList()));

        List<Foo> mergedAndMatchedFoos = idToMatchedFooMap.values().stream()
                .map(this::mergeFoos)
                .collect(toList());

        return mergedAndMatchedFoos;
    }

    private Foo mergeFoos(List<Foo> matchedFoos) {
        Foo result = new Foo();
        for(Foo matchedFoo : matchedFoos) {
            result.setId(matchedFoo.getId());
            result.getInputs().addAll(matchedFoo.getInputs());
        }
        return result;
    }

}

Can I obtain the same result by using only a single lambda operation that is more elegant than the above? I have looked into reducing but it was not very trivial.

To determine the result you're after, it is necessary to group by the id and hold a temporary map of the grouping operation.

You can make it a bit simpler, and not have a second Stream pipeline, by using the collectingAndThen built-in collector, which gives the ability to pass a finisher operation to the result of another collector.

Collection<Foo> result =
    matchedFoos.stream()
               .collect(groupingBy(Foo::getId, collectingAndThen(toList(), this::mergeFoos)))
               .values();

This code still collects all the foos having the same id into a list, but the use of collectingAndThen enables us to directly pass that list to mergeFoos and return the wanted Foo. Then, it is possible to retrieve the values by invoking values().

This will return a Collection<Foo> as values() is defined to return that. If you really want a List, you can convert it easily to an ArrayList for example, with:

List<Foo> mergedAndMatchedFoos = new ArrayList<>(result);

Note that the current code stores every foo having the same id in a list. This is a bit inefficient since, for the end result, we're interested in a single Foo where the inputs are all input of the foos in the list. So, instead, we can use the toMap(keyMapper, valueMapper, mergeFunction) collector and directly build the intermediate Map<String, Foo> instead a Map<String, List<Foo>>.

The key mapper returns the id of the foo; the value mapper returns a new Foo from a given one, and the merge function adds all inputs from one foo to the other.

Map<String, Foo> result =
    matchedFoos.stream()
       .collect(Collectors.toMap(
          Foo::getId,
          Foo::new,
          (foo1, foo2) -> { foo1.getInputs().addAll(foo2.getInputs()); return foo1; }
       ));

List<Foo> mergedAndMatchedFoos = new ArrayList<>(result.values());

This assumes that we have a copy-constructor Foo(Foo foo) constructing a Foo from another Foo (which is what the Foo::new method-reference refers to). The mutation of foo1 inside the merge function is safe to do since we're working on the new instance, constructed with the copy constructor.


Basic HTML structure for three columns of articles I'm new to HTML and programming in general and I want to make sure my code is organized and that I'm using the best practices while I go along.

So, I made this example layout I would like to convert to HTML5:

And this is the HTML5 code I created:

Title

Subhead

Item 1

Lorem ipsum...

Item 2

Lorem ipsum...

Item 3

Lorem ipsum...

Now to the questions:

Is there a difference between using div instead of sections or articles? I mean, not only in the organization realm but also in optimization realm? Is there a general best practice convention nowadays about this specific topic? Because I feel that using sections and articles makes the code easier to read (and also makes the CSS file cleaner).
Should I have a header tag inside each article? I heard this is best practice but I feel like this would make the code harder to read and at the same time I feel like it makes the code more organized so I'm kind of confused what to do.
About code readability: Should I keep a line break between different items like sections? In a small HTML file I'm aware that like breaks wouldn't be a big deal but in a big HTML file you could have like 200 empty lines making the code way bigger than it has to be. What do people usually do about this?

HTML is what's known as a markup language, which means the actual meaning of the elements (whether you choose div or sections) is up to the code to interpret.

Browsers have their own CSS files they apply to every page, so which tag you choose to use for an element doesn't really matter. Except for types like img and video that have special purpose in the browser

That said, using tag names that better suit the purpose increases the readability of your code and improves maintainability for future development.

Standards like HTML5 add more style rules for elements (as well adding more special purpose types like img and a bunch more stuff).

There's not an optimization tradeoff as the CSS engine doesn't treat rules on div different than rules on sections.

  Should I have a header tag inside each article?

Yeah, that's exactly what it's for! Mozilla Developer Network says that the header element is for representing heading elements, specifically even quoting usage designed for a wrapped section's header.

  I heard this is best practice but I feel like this would make the code harder to read

While code readability is arguably a personal thing, I always felt that this kind of layout:

Item

Text

Was a lot harder to understand than something like:

  Item
  <content>
    <photo>

    </photo>
    <description>

Text

    </description>
  </content>

Especially seeing as you can now easily see which closing tag is which.

  Should I keep a line break between different items like sections?

Again, code readability is up to you, do whatever is easier for you to understand.

  in a big HTML file you could have like 200 empty lines making the code way bigger than it has to be. What do people usually do about this?

If you've got that many elements, it might time to use a library (like jQuery, Ember, Angular or React) to manage your data and make the HTML layouts for you.

However for just a normal HTML file, line breaks aren't necessarily bad. Extra lines don't bloat the files, so you don't need to worry too much about the files being bloated.

Like I said before, code readability is an intensely personal thing. While I (and any other reviewers) can take a look at code at tell you how readable it is, you're the one developing it, and it's your creation.


in-nest-sequence: sequence generator in Racket in-nest-sequence is a sequence generator that takes a function and an initial value, and the return value of invoking the function on the current value is used as the subsequent value. For example, (in-nest-sequence add1 0) returns the sequence (0 1 2 3 4 ...).

Recently, soegaard challenged me to write a define-sequence-syntax version of in-nest-sequence, that is, a macro-based version. I decided to give it a try:

#lang racket
(require (for-syntax unstable/syntax))
(provide (rename-out [*in-nest-sequence in-nest-sequence]))

(define in-nest-sequence
  (case-lambda
    [(func init)
     (make-do-sequence
      (thunk (values identity func init #f #f #f)))]
    [(func . inits)
     (make-do-sequence
      (thunk (values (curry apply values)
                     (lambda (args)
                       (call-with-values (thunk (apply func args)) list))
                     inits #f #f #f)))]))

(define-sequence-syntax *in-nest-sequence
  (lambda () #'in-nest-sequence)
  (lambda (stx)
    (syntax-case stx ()
      [[(x ...) (_ func init ...)]
       (unless (= (syntax-length #'(x ...)) (syntax-length #'(init ...)))
         (raise-syntax-error 'in-nest-sequence
                             (format "~a values required" (syntax-length #'(x ...)))
                             stx #'(init ...)))
       (with-syntax ([for-arity (syntax-length #'(init ...))]
                     [(value ...) (generate-temporaries #'(init ...))]
                     [(y ...) (generate-temporaries #'(init ...))])
         #'[(x ...) (:do-in ([(f) func])
                            (unless (procedure-arity-includes? f for-arity)
                              (raise-arity-error f (procedure-arity f) init ...))
                            ([value init] ...)
                            #t
                            ([(x ...) (values value ...)]
                             [(y ...) (f value ...)])
                            #t
                            #t
                            (y ...))])])))

I'm a complete noob at using define-sequence-syntax, so I appreciate any and all style, performance, and/or general feedback about my code. (See the original post for some usage examples.)

For cases where the the user follows the syntax and the function returns and receives the correct number of values, this works fine.

It is a good principle to catch errors as early as possible.

Consider this example:

(in-iterate add1)

Here the user supplies a function add1 that has arity 1, but supplies no arguments. This will eventually lead to an "result arity mismatch;" error, when add1. This error can be caught earlier in using procedure-arity to check that enough init values are supplied. The error message presented to the user can be "expected 1 initial value, but received 0".

In the *in-iterate consider: [(x ...) (_ func init ...)] .
Here the number of bound identifiers (the xs)  must be the same as the number of init expressions. If they are not, a raise-syntax-error with stx as the source of the error can be used.

An example that should be a syntax-error (and not a runtime error):

(define (add1/2 x y) (values (+ x 1) (+ y 2)))
(for/list ([n     10]
           [(x y) (in-iterate add1/2 0)])
  (list x y))

The name of the construct is a bit off. In in-list and in-sequence we have the pattern in-noun, so that suggest in-iteration might be better. However iteration is a bit vague. Maybe one in-nested-sequence or in-nest-sequence based on the name used in Mathematica? http://reference.wolfram.com/language/ref/NestList.html


For a given balance and an annual interest rate, calculate the minimum fixed monthly payment to pay off debt in a year Interest is compounded monthly. Payment doesn't  vary from month to month.

Version 1

balance = 999999
annualInterestRate = 0.18
mRate = annualInterestRate/12
high = (((mRate+1)**12)*balance)/12
low = balance/12
guessed = False

def balanceLeft(balance,mRate,minPayment):
    monthsLeft = 12
    while monthsLeft > 0:
        unpaidBalance =  balance - minPayment
        interest = mRate * unpaidBalance
        balance = unpaidBalance
        balance += interest
        monthsLeft -= 1
    return balance

while guessed == False:
    minPayment = (high + low) / 2
    if round(balanceLeft(balance,mRate,minPayment),2) < 0:
        high = minPayment
    elif round(balanceLeft(balance,mRate,minPayment),2)> 0:
        low = minPayment
    else:
        if abs(round(balanceLeft(balance,mRate,minPayment),2) - 0) < 0.01:
            guessed = True

print('Lowest Payment: ',end='')
print(round(minPayment,2))

Version 2

annualInterestRate = 0.18
rate = annualInterestRate / 12
monthsLeftr = 12
xCoefficent = 1 + rate
ConstantTerm = 1 + rate
while monthsLeftr > 1:
    xCoefficent = (xCoefficent + 1) * ConstantTerm
    monthsLeftr -= 1

balance = 999999
monthsLeft = 12
while monthsLeft > 0:
    balance = balance * ConstantTerm
    monthsLeft -= 1
minPayment = balance / xCoefficent

print('Lowest Payment: ', end="")

I would like a comparative and overall review, please!

This is a problem that can be solved exactly with a little math, so there should be no need to program a search.

Suppose that we borrow initial capital \$c\$ at a monthly interest rate \$r\$, paying it off in \$n\$ months with a monthly payment \$p\$. (Here \$r\$ is expressed as a multiplier, for example if the monthly percentage rate is \$2\%\$ then \$r = 1.02\$.)

In this scenario the outstanding balance is $$ \eqalign{
cr - p &= cr^1 - p(1) & \quad\text{after 1 month;} \\
(cr - p)r - p &= cr^2 - p(1 + r) & \quad\text{after 2 months;} \\
((cr - p)r - p)r - p &= cr^3 - p(1 + r + r^2) & \quad\text{after 3 months;}}$$ and so after \$n\$ months the balance is $$
cr^n - p(1 + r + r^2 + \dots + r^{n - 1}) = cr^n - p{r^n - 1\over r-1}. $$ If this pays off the debt exactly then $$ cr^n - p{r^n - 1\over r-1} = 0 $$ and so $$ p = {cr^n(r-1) \over r^n - 1}.$$

Since payments typically have to be made in integer numbers of pennies (or whatever the minimum unit of account is) then you'll want to round \$p\$ up to the nearest penny.


Loops to merge documents and renumber pages I have a for loop implementation for merging documents and page re-numbering. But since I am modifying the collection while iterating, I suspect that the code might break. As foreach doesn't allow collection modification, I have implemented it with for.

Is there any better way to implement/optimize this part of code? Also, is it possible to reduce these 3 loops to a single one?

// Merge Document Sequence
for (int k = 0; k < response.documents.Count; k++)
{
    string presentDocument = string.Empty;
    string laterDocument = string.Empty;

    presentDocument = response.documents[k].docType;
    if ((response.documents.Count - 1) != k)
        laterDocument = response.documents[k + 1].docType;

    if ((laterDocument == presentDocument))
    {
        response.documents[k].pages.AddRange(response.documents[k + 1].pages);
        response.documents.RemoveAt(k + 1);
        k = k - 1;
    }
}

// Page Re-numbering
for (int i = 0; i < response.documents.Count; i++)
{
    for (int j = 0; j < response.documents[i].pages.Count; j++)
        response.documents[i].pages[j].pageNo= j + 1;
}

One way to make it easier to follow is to construct your merged documents result separately from your input (above code is not tested).

Construct a dictionary to easily get the document to append to, based on type. It also ensures a fast operation. For each document from the input, append its pages in the dictionary:

// type => merged document mapping
var mergedDocsDict = new Dictionary<String, Document>();

response.documents.ForEach(doc =>
    {
       if (!mergedDocDict.ContainsKey(doc.docType))
          mergedDocDict.Add(doc.docType, new Document());

       mergedDocDict[doc.docType].pages.AddRange(doc.pages);
    };

Finally renumbering all the pages:

mergedDocDict.Keys.ForEach(key =>
   {
       var doc = mergedDocDict[key];
       int pageCount = doc.pages.Count;
       for (int i = 0; i < pageCount; i ++)
          doc.pages[i].pageNo = i + 1;
   };

You result can be obtain from the dictionary:

mergedDocDict.Values.ToList();

This solution should be easier to understand, but uses extra memory for the result.

This can be overcome, by reusing the same documents:

response.documents.ForEach(doc =>
    {
       if (!mergedDocDict.ContainsKey(doc.docType))
          mergedDocDict.Add(doc.docType, doc);
       else
          mergedDocDict[doc.docType].pages.AddRange(doc.pages);
    };

Also, page renumbering can be done in the first loop, obtaining a single foreach:

response.documents.ForEach(doc =>
    {
       if (!mergedDocDict.ContainsKey(doc.docType))
          mergedDocDict.Add(doc.docType, doc);
       else
       {
          int existingCount = mergedDocDict[doc.docType].pages.Count;
          mergedDocDict[doc.docType].pages.AddRange(doc.pages);
          int newCount = doc.pages.Count + existingCount;
          for (int i = existingCount; i < newCount; i ++)
            mergedDocDict[doc.docType].pages[i].pageNo = i + 1;
       }
    };

Complexity should be O(number_of_docs * number_of_pages) as all dictionary operations are done in O(1).

If your documents were copied from some other data structure to your response, I think it is better to construct the dictionary from that data structure and obtain respose.documents from the merged data. Otherwise, you have to use a for loop to remove documents already merged (somewhat more convoluted than foreach version).


A view for Python list slices As you probably know, Python slices like arr[1:3] etc. do NOT provide a view onto list and instead creates a copy of the list, what can be not optimal performance if array is big and you make for example arr[1:] (copies all exept first)
the idea of a view to an array is like :

class ylist_view():

    def __init__(self,lst, begin, end):

        self.list = lst
        self.len = end - begin
        self.begin = begin
        self.end = end

    def __getitem__(self, i):
        if i<0: i += self.len
        return self.list[self.begin + i]

    def __setitem__(self, i,value):
        if i<0: i += self.len
        self.list[self.begin + i] = value

     def __str__(self):
            return  '[' + ', '.join( str(self.list[i]) for i in range(self.begin, self.end)) + ']'

class inbox():

    def __init__(self, value):
        self.value = value

    def __str__(self):
        return '<' + str(self.value) + '>'

    def __repr__(self):
        return '<' + str(self.value) + '>'

    a =   [ inbox(x) for x in  [1,2,3,4,5]]

    b = a [1:3]

    b[0] = 20 # creates a copy

    print('list a is unchanged:', a)
    print('b = a [1:3] is a copy of a :', b)

    c = ylist_view(a,1,3)

    c[0] = 20

    print('ylist_view c is a view to a:', a)

Inbox class is a dummy class for illustration.
As you can see, b = a [1:3] is a copy, whereas c = ylist_view(a,1,3) references the original array.
Does it make sense to use this approach in a real Python project or there is some built-in in Python to do that?

If you are dealing with number lists, then array.array has a compact memory representation and implements the buffer protocol which allows memoryview’s to be created, which directly support views like you are creating.

For lists which can hold other things (tuples, dictionaries, lambdas, ...), Python has no built in support, and your view class can be appropriate.

PEP8 guidelines

Your class name should begin with a capital letter.  I’d suggest ListView as an option.  I don’t know what the ‘y’ is intended to mean.

There should be one space after every comma.  There shouldn’t be any spaces between a variable name and the [ character (b = a[1:3]).

Private members (self.begin, etc) should begin with an underscore (self._begin, etc).

Use a pylint, pyflakes, ... to ensure PEP8 compliance.

You could implement __repr__ in terms of __str__:

    def __repr__(self):
        return str(self)

Extension: Your list view could support a view slice with a step size other than 1.

You don’t protect against indexing beyond the length of your view. i >= self.len.


Tree structure with support for inorder and preorder traversal I am learning Tree Data Structures and came up with this code for implementing basic Tree Node using class and performing various Tree based operations like Traversals and others. The code works perfectly but the structure is bothering me a lot, such as this line of code here:

print(tree.size(tree))

How can I improve the code structure?

class Node():
    def __init__(self, data):
        self.left = None
        self.right = None
        self.data = data

    def __str__(self):
        return self.data

    def inorder(self, node):
        if node is not None:
            self.inorder(node.left)
            print(node.data)
            self.inorder(node.right)

    def preorder(self, node):
        if node is not None:
            print(node.data)
            self.preorder(node.left)
            self.preorder(node.right)

    def size(self, node):
        if node is None:
            return 0
        return self.size(node.left) + 1 + self.size(node.right)

tree = Node(1)
tree.left = Node(2)
tree.right = Node(3)
tree.left.left = Node(4)
tree.left.right = Node(5)

print(tree.size(tree))

Your instinct to question the appropriateness of tree.size(tree) is right.  You should be able to write tree.size() instead.  The problem is that you've written the methods to take a node parameter, when in fact self is a Node.

Therefore, you should define

def size(self):
    return 1 + (self.left.size()  if self.left  is not None else 0) \
             + (self.right.size() if self.right is not None else 0)

so that you can write tree.size().

The __str__ method isn't right: there's nothing that says that data is a string.  (In fact, in your example, they are integers.)  A more reasonable implementation would be

def __str__(self):
    return str(self.data)

It would be better to avoid hard-coding the print() calls in inorder and preorder, so that they can be used to do more than just printing.  In Python, it would be better to yield each datum, so that inorder() and preorder() act as generators.  (Note that this uses yield from, introduced in Python 3.3.)

def inorder(self):
    if self.left is not None:
        yield from self.left.inorder()
    yield self.data
    if self.right is not None:
        yield from self.right.inorder()

… to be called like

for data in tree.inorder():
    print(data)

You would then have the flexibility to write something like sum(data for data in tree.inorder()) to add all of the nodes.


Simulation for repeatedly taking gloves until a matching pair is found I created the following code that simulates a person taking a random glove from a drawer until he/she gets a matching pair of gloves:

LinkedList<String> glovesList = new LinkedList<String>();
        LinkedList<String> matchList = new LinkedList<String>();
        Random rand = new Random();
        String[] glovesAvailable = {"red left", "red left", "red left", "red left", "red left",
                                    "red right", "red right", "red right", "red right", "red right",
                                    "yellow left", "yellow left", "yellow left", "yellow left",
                                    "yellow right", "yellow right", "yellow right", "yellow right",
                                    "green left", "green left", "green right", "green right"};

        // adding the gloves inside the list
        for(int i = 0; i < glovesAvailable.length; i++) {
            glovesList.add(glovesAvailable[i]);
        }

        // taking out the gloves from the drawer
        while(glovesList.size() != 0) {
            int n = rand.nextInt(glovesList.size());
            System.out.println("Removed a " + glovesList.get(n));
            matchList.add(glovesList.remove(n));
            if((matchList.contains("red left") && matchList.contains("red right")) ||
               (matchList.contains("yellow left") && matchList.contains("yellow right")) ||
               (matchList.contains("green left") && matchList.contains("green right"))) {
                System.out.println("Pair found!");
                break;
            }
        }

        // rechecking list if there are no more items
        System.out.println("Iterations till a matching pair is found: " + matchList.size());
        System.out.println("List of gloves taken out: " + matchList);
        System.out.println("Remaining gloves: " + glovesList.size());

The code works, but I am interested in making it more efficient. How can this code be improved?

Make the glove types an enum

public static enum GLOVE {
  RED_LEFT, RED_RIGHT,
  YELLOW_LEFT, YELLOW_RIGHT,
  GREEN_LEFT, GREEN_RIGHT
}

this has several benefits:

with the current String solution, you can have a typo and you will not detect this until run time.  With an enum, the compiler checks values for you
the matchList.contains... if statement can be replaced by iteration on enum values
checking equality on enum values is faster then String comparison

if you want an even better solution, you should make a Glove class with two attributes, color of type enum and hand of type enum.  then, implement equals() and hash() methods so you can put Glove instances in various Collections. The benefit if this solution is that it allows for future modification to Glove specifications (like, adding colors, adding material attribute, etc)


Locating empty field groups for saving data The aim of the code below is to see if selected fields are empty so data can be saved in that field. Since I'm making a library system, I wanted to record the basic info on one record (all the books they have taken out so the ID's of them as well as author and title) but this proves to be quite tricky and quite long to code.

I was wondering whether there's an easier way to code this instead of the very long if loop. It would also help if I could use the same solution (obviously tweaked) when displaying their loans on the form as well.

procedure Save;
{ procedure to locate which field groups are empty so product and loan informati
 - on can be saved }
begin
  if LoanRecords.ProductID1 ='' then
  begin
    LoanRecords.ProductID1 := ProductRecords.UniqueId;
    LoanRecords.ProductTitle1 := ProductRecords.ProductTitle;
    LoanRecords.ProductAuthor1 := ProductRecords.ProductAuthor;
  end
  else if LoanRecords.ProductID2 = '' then
  begin
    LoanRecords.ProductID2 := ProductRecords.UniqueId;
    LoanRecords.ProductTitle2 := ProductRecords.ProductTitle;
    LoanRecords.ProductAuthor2 := ProductRecords.ProductAuthor;
  end
  else if LoanRecords.ProductID3 = '' then
  begin
    LoanRecords.ProductID3 := ProductRecords.UniqueId;
    LoanRecords.ProductTitle3 := ProductRecords.ProductTitle;
    LoanRecords.ProductAuthor3 := ProductRecords.ProductAuthor;
  end
  else if LoanRecords.ProductID4 ='' then
  begin
    LoanRecords.ProductID4 := ProductRecords.UniqueId;
    LoanRecords.ProductTitle4 := ProductRecords.ProductTitle;
    LoanRecords.ProductAuthor4 := ProductRecords.ProductAuthor;
  end
  else if LoanRecords.ProductID5 ='' then
  begin
    LoanRecords.ProductID5 := ProductRecords.UniqueId;
    LoanRecords.ProductTitle5 := ProductRecords.ProductTitle;
    LoanRecords.ProductAuthor5 := ProductRecords.ProductAuthor;
  end
  else if LoanRecords.ProductID6='' then
  begin
    LoanRecords.ProductID6 := ProductRecords.UniqueId;
    LoanRecords.ProductTitle6 := ProductRecords.ProductTitle;
    LoanRecords.ProductAuthor6 := ProductRecords.ProductAuthor;
  end
end;

This is the record structure-

  TLoan = record
    LoanUniqueId: integer; { of loan record }
    DateDue: TDateTime; { date when products are due to be returned }
    MemberUniqueID: integer; { from MemberDetails File }
    MemberName: string[50]; { from MemberDetails File }
    MemberSecondName: string[50]; { from MemberDetails File }
    AmountBorrowed: integer; { amount of items currently on loan }
    { Product 1 }
    ProductID1: integer; { product ID }
    ProductTitle1: string[80]; { product title }
    ProductAuthor1: string[80]; { product author }
    Return1: boolean;
    { has book been returned- true/false- set to false until book is returned }
    { Product 2 }
    ProductID2: integer;
    ProductTitle2: string[80];
    ProductAuthor2: string[80];
    Return2: boolean;
    { Product 3 }
    ProductID3: integer;
    ProductTitle3: string[80];
    ProductAuthor3: string[80];
    Return3: boolean;
    { Product 4 }
    ProductID4: integer;
    ProductTitle4: string[80];
    ProductAuthor4: string[80];
    Return4: boolean;
    { Product 5 }
    ProductID5: integer;
    ProductTitle5: string[80];
    ProductAuthor5: string[80];
    Return5: boolean;
    { Product 6 }
    ProductID6: integer;
    ProductTitle6: string[80];
    ProductAuthor6: string[80];
    Return6: boolean;
  end; { of TLoan- loan details }

Technically, there is no such thing as if-loops. Your code is just a bunch of if-else statements (sorry, this was the language police speaking)

What your code actually needs, is actually a real loop. Use an array or a list of a record, TLoanProduct and loop through it. Your current approach allows a maximum of six products in one loan, it involves a lot of code duplication which in itself is a code smell, and it could be a bit weird if the values for Product 3 were set but not Product 1.

My Delphi skills are a bit rusty, but consider changing TLoan into an object or a class instead of a pure record. That way you could have the Save method as part of the TLoan object/class.

This is some code for how I think it should look. It's been a long while since I worked with Delphi so this might not be 100% correct code, but it should get you started in the right direction.

TLoanProduct = record
    ProductID: integer;
    ProductTitle: string[80];
    ProductAuthor: string[80];
    Returned: boolean;
end;
TLoan = record
    LoanUniqueId: integer; { of loan record }
    DateDue: TDateTime; { date when products are due to be returned }
    MemberUniqueID: integer; { from MemberDetails File }
    MemberName: string[50]; { from MemberDetails File }
    MemberSecondName: string[50]; { from MemberDetails File }
    AmountBorrowed: integer; { amount of items currently on loan }
    LoanProducts: array [0..5] of TLoanProduct;
end;

...

procedure Save;
{ procedure to locate which field groups are empty so product and loan informati
 - on can be saved }
var i: integer;
begin
  for i := 0 to High(LoanRecords.LoanProducts) do
  if LoanRecords.LoanProducts[i].ProductID = '' then
  begin
    LoanRecords.LoanProducts[i].ProductID := ProductRecords.UniqueId;
    LoanRecords.LoanProducts[i].ProductTitle := ProductRecords.ProductTitle;
    LoanRecords.LoanProducts[i].ProductAuthor := ProductRecords.ProductAuthor;
    Exit; // Exit the method so that this is only applied for one element
  end;
end;