Leetcode: divide-two-integers I was practicing divide-two-integers question from leetcode

Question: Given two integers dividend and divisor, divide two integers without using multiplication, division and mod operator.

Return the quotient after dividing dividend by divisor.

The integer division should truncate toward zero.

Question Link: https://leetcode.com/problems/divide-two-integers/

For this I wrote the following algorithm but clearly it isn't optimized with my time complexity coming out to be about  8248 ms. Can someone please help me in optimizing the code?

 /**
 * @param {number} dividend
 * @param {number} divisor
 * @return {number}
 */
/**
     * @param {number} dividend
     * @param {number} divisor
     * @return {number}
     */
var divide = function(dividend, divisor) {
   let negativeFlag = false
   let quiotentHolder = 0
   if (divisor === 0 || dividend === 0) return 0;
   if (dividend === -2147483648 && divisor === -1) return 2147483647;
  else if (dividend < 0 && divisor < 0) {
      dividend = -dividend
      divisor = -divisor
  } else if ((dividend < 0 && divisor > 0) || (dividend > 0 && divisor < 0)) {
        negativeFlag = true
        divisor < 0 ? divisor = -divisor :dividend = -dividend
    }
    while (divisor <= dividend) {
        dividend = dividend - divisor
        quiotentHolder += 1
    }
    if (negativeFlag) return -quiotentHolder
    else return quiotentHolder
}

Division by zero

Your implementation returns zero if you devide by zero. That is wrong. Division by zero is undefined.

if (divisor === 0 || dividend === 0) return 0;

On other hand, zero divided by nonzero is zero, that's ok. But zero divided by zero is still undefined. And so it should be checked first.

if (divisor === 0) throw "Division by zero";
if (dividend === 0) return 0;

Edge cases

For some reason you are checking combination -2147483648/-1 then you claim it is 2147483647, but that's wrong. Firstly, 2147483648 is representable in js number type. But if you consider it overflow, you should throw overflow error.

if (dividend === -2147483648 && divisor === -1) throw 'Overflow error';

Btw notice, that -2147483648/n would also overflow further in your code because you convert dividend and divisor to positives. And so maybe you should throw overflow error in this case as well no matter the divisor.

if (dividend === -2147483648) throw 'Overflow error';

Simplify conditions

There is unnecesarily complex condition:

(dividend < 0 && divisor > 0) || (dividend > 0 && divisor < 0)

assuming the first if was already executed, then the eleseif condition can be simplified as

dividend < 0 || divisor < 0

Or even better, split it in two elseifs, because you have a ternary that checks again.

if (dividend < 0 && divisor < 0) {
      dividend = -dividend
      divisor = -divisor
} else if (dividend < 0) {
        negativeFlag = true
        dividend = -dividend;
} else if (divisor < 0) {
        negativeFlag = true
        divisor = -divisor;
}

Notice that I omitted zeroes, because they have been excluded in the beginning.

Negative flag

This flag is unnecesary, you could instead have an integer variable with value of +1 or -1.

let quotientUnit = 1;
if (...) quotientUnit = -1;
let quiotentHolder = 0;
while (divisor <= dividend) {
     dividend -= divisor; //notice -=
     quiotentHolder += quotientUnit;
}
return quiotentHolder;

Performance

Just of curiosity, have you tried something like big/small. Is it fast? It seems to me the bigger the result of the division, the more time consuming your algorithm is... Not sure if there is a faster way though. Division without division is not something I usualy think of :) But I could suppose there is not, because that's why we have division optimized in our CPUs, where it can be fastened by paralel processing (by which i mean paralel nature of logic gates, not running multiple threads inside a CPU)

EDIT: You mentioned that your "time complexity is about 8248 ms". Firstly, time complexity is not a meassure of time. Time complexity is more like a meassure of relative time consumption based on the size of the input. Since in your case there is no "size of input", time complexity makes no sense with this algorithm. If you wanted to define something like time complexity for your algorithm, it would probably be something like O(dividend/divisor), but this is not really a time complexity, but I am still able to make sense of it...

But let's suppose you meant the actual run time. Not sure what input does the 8248 ms refer to. As I said, the speed is different for different inputs.


Wheel based unbounded sieve of Eratosthenes in Python Answering this SO question recently, I've developed the following code (based on a well-known Active Code recipe, as discussed here):

wheel = [2,4,2,4,6,2,6,4,2,4,6,6,2,6,4,2,6,4,6,8,4,2,4,2,
         4,8,6,4,6,2,4,6,2,6,6,4,2,4,6,2,6,4,2,4,2,10,2,10]
wsize = 48

def primes():
    yield from (2, 3, 5, 7)
    yield from wsieve()

def wsieve():       # wheel-sieve, by Will Ness. cf. ideone.com/WFv4f
    yield 11        # cf. https://stackoverflow.com/a/10733621/849891
    mults = {}      #     https://stackoverflow.com/a/19391111/849891
    ps = wsieve()
    p = next(ps)
    psq, c, i = p*p, 11, 0                   # 13 = 11 + wheel[0]
    cbase, ibase = 11, 0
    while True:
        c += wheel[i] ; i = (i+1) % wsize    # 17 = 13 + wheel[1]
        if c in mults:
            (j,pbase) = mults.pop(c)         # add(mults, NEXT c, j, pbase)
        elif c < psq:
            yield c ; continue
        else:          # (c==psq)
            while not cbase == p:
                cbase += wheel[ibase]
                ibase = (ibase+1) % wsize    # ibase - initial offset into wheel, for p
            j, pbase = ibase, p              # add(mults, NEXT c, ibase, p)
            p = next(ps) ; psq = p*p
        m = c + pbase*wheel[j] ; j = (j+1) % wsize    # mults(p) = map (*p)
        while m in mults:                             #              roll(wheel,ibase,p)
            m += pbase*wheel[j] ; j = (j+1) % wsize
        mults[m] = (j,pbase)

Comparing its performance on ideone with its odds-based predecessor reveals it runs about 1.5x faster. Empirical complexity seems okay (~ n1.1, for n primes produced).

Theoretically, it's supposed to be 1 / (2/3 * 4/5 * 6/7) = 2.1875x times faster, if I'm not mistaken (indeed, taking ratio of circumference over number of teeth, (210/48) / (2/1) = 2.1875).  But it is slower than that.

Is this just because of the increased code complexity? Is there a way to make it run faster, without changing it drastically? Improve it in some other sense, or make it prettier/more pythonic without loosing performance? Will appreciate a review.

In particular, I have the rolled wheel streams inlined, using different variables for different "objects". It seems all over the place; is there a way to encapsulate it into a bona fide object, without loosing performance? (last time when I used functions for some encapsulation, efficiency suffered).

I thought of two things that achieve a 27 % speedup and simplify wsieve a bit, at the expense of some precomputations.

Turning the wheel step by step from prime to prime seems wasteful. Instead of this code

while not cbase == p:
    cbase += wheel[ibase]
    ibase = (ibase+1) % wsize
j = ibase

you could do directly j = spoke_index[p % 210] having precomputed a suitable spoke_index dictionary with 48 entries. The variables cbase and ibase can be eliminated.
When a wheel is involved I find it hard to resist using itertools.cycle. Instead of this

i = 0
while True:
    c += wheel[i] ; i = (i+1) % wsize

you could do this

for step in cycle(wheel):
    c += step

The other uses of the wheel are more difficult to replace because there is no direct way to start the cycle at an arbitrary position. However, we can precompute all 48 rotations of wheels and put them in a dictionary for easy lookup.

My version of full solution. Instead of the index j I put a cyclic iterator in mults.

from itertools import cycle

CIRCUMFERENCE = 2*3*5*7
BASE_PRIMES = (2,3,5,7)
NEXT_PRIME = 11

def wheel(start):
    result = []
    i = start
    for j in range(i + 1, i + 1 + CIRCUMFERENCE):
        if all(j % k for k in BASE_PRIMES):
            result.append(j - i)
            i = j
    return result

def rotated_wheels():
    result = {}
    i = 1
    while i < CIRCUMFERENCE:
        result[i] = wheel(i)
        i = i + result[i][0]
    return result

def primes():
    yield from BASE_PRIMES
    yield from wsieve()

def wsieve(wheels=rotated_wheels()):
    yield NEXT_PRIME
    mults = {}
    ps = wsieve()
    p = next(ps)
    psq, c = p*p, p
    cwheel = cycle(wheels[c])
    for step in cwheel:
        c += step
        if c in mults:
            (mwheel, pbase) = mults.pop(c)
        elif c < psq:
            yield c
            continue
        else:          # (c==psq)
            mwheel = cycle(wheels[p % CIRCUMFERENCE])
            pbase = p
            p = next(ps) ; psq = p*p
        m = c
        for mstep in mwheel:
            m += pbase * mstep
            if m not in mults:
                break
        mults[m] = (mwheel, pbase)

Edited to use for instead of while.


WPF async await TcpClient/TcpListener sample I want to build a robust and highly scalable client server system. Here what I have so far(an echo server as my base of implementation)

My Server

    private void startServer_Click(object sender, RoutedEventArgs e)
    {
        if (anyIP.IsChecked == true)
        {
            listener = new TcpListener(IPAddress.Any, Int32.Parse(serverPort.Text));
            Logger.Info("Ip Address : " + IPAddress.Any + " Port : " + serverPort.Text);
        }
        else
        {
            listener = new TcpListener(IPAddress.Parse(serverIP.Text), Int32.Parse(serverPort.Text));
            Logger.Info("Ip Address : " + serverIP.Text + " Port : " + serverPort.Text);
        }
        try
        {
            listener.Start();
            Logger.Info("Listening");
            HandleConnectionAsync(listener, cts.Token);
        }
        //finally
        //{
            //cts.Cancel();
            //listener.Stop();
            //Logger.Info("Stop listening");
        //}

        //cts.Cancel();
    }

    async Task HandleConnectionAsync(TcpListener listener, CancellationToken ct)
    {
        while (!ct.IsCancellationRequested)
        {
            Logger.Info("Accepting client");
            //TcpClient client = await listener.AcceptTcpClientAsync();
            TcpClient client = await listener.AcceptTcpClientAsync();
            Logger.Info("Client accepted");
            EchoAsync(client, ct);
        }

    }

    async Task EchoAsync(TcpClient client, CancellationToken ct)
    {
        var buf = new byte[4096];
        var stream = client.GetStream();
        while (!ct.IsCancellationRequested)
        {
            var amountRead = await stream.ReadAsync(buf, 0, buf.Length, ct);
            Logger.Info("Receive " + stream.ToString());
            if (amountRead == 0) break; //end of stream.
            await stream.WriteAsync(buf, 0, amountRead, ct);
            Logger.Info("Echo to client");
        }
    }

    private void stopServer_Click(object sender, RoutedEventArgs e)
    {
        cts.Cancel();
        listener.Stop();
        Logger.Info("Stop listening");
    }

My Client

    private void connect_Click(object sender, System.Windows.RoutedEventArgs e)
    {
        IPAddress ipAddress;
        int port;

        //TODO Check if ip address is valid
        ipAddress = IPAddress.Parse(serverIP.Text);
        //TODO port range is 0-65000
        port = int.Parse(serverPort.Text);

        StartClient(ipAddress, port);
    }

    private static async void StartClient(IPAddress serverIpAddress, int port)
    {
        var client = new TcpClient();
        //can i try/catch to catch await exception?
        try
        {
            await client.ConnectAsync(serverIpAddress, port);
        }
        catch (Exception e)
        {
            Logger.Info(e);
        }
        Logger.Info("Connected to server");
        using (var networkStream = client.GetStream())
        using (var writer = new StreamWriter(networkStream))
        using (var reader = new StreamReader(networkStream))
        {
            writer.AutoFlush = true;
            for (int i = 0; i < 10; i++)
            {
                Logger.Info("Writing to server");
                await writer.WriteLineAsync(DateTime.Now.ToLongDateString());
                Logger.Info("Reading from server");
                var dataFromServer = await reader.ReadLineAsync();
                if (!string.IsNullOrEmpty(dataFromServer))
                {
                    Logger.Info(dataFromServer);
                }

            }
        }
        if (client != null)
        {
            client.Close();
            Logger.Info("Connection closed");
        }

    }

The general issue with your code is that your client and server implementations reside in your control's code-behind. It is always a good idea to separate business logic from UI.
This is copy-paste:

if (anyIP.IsChecked == true)
{
    listener = new TcpListener(IPAddress.Any, Int32.Parse(serverPort.Text));
    Logger.Info("Ip Address : " + IPAddress.Any + " Port : " + serverPort.Text);
}
else
{
    listener = new TcpListener(IPAddress.Parse(serverIP.Text), Int32.Parse(serverPort.Text));
    Logger.Info("Ip Address : " + serverIP.Text + " Port : " + serverPort.Text);
}

can be refactored to:

var ip = anyIP.IsChecked == true ? IPAddress.Any : IPAddress.Parse(serverIP.Text);
var port = Int32.Parse(serverPort.Text);
listener = new TcpListener(ip, port);
Logger.Info(String.Format("Ip Address : {0} Port : {1}", ip, port));

Its a good style to prefix fields with underscore, so they can be distinguished from local variables. So it should be _listener instead of listener.
You should shut down your server on exit.
Yes, as long, as you await the call you can catch exceptions.


Generic queries and handlers without reflection I'm trying to implement generic queries and handlers so that I can make requests with simple syntax, like this:

var query = new HelloQuery("hi");
var result = processor.Process(query);

This post explains a brilliant technique for accomplishing this, but it uses reflection and dynamic to resolve handlers for queries.  I came up with an alternative using generics.

The central idea is the IQuery:

public interface IQuery<TSelf, TResult>
    where TSelf : IQuery<TSelf, TResult>
{
}

One such implementation could be:

public class HelloQuery : IQuery<HelloQuery, string>
{
    public HelloQuery(string message)
    {
        this.message = message;
    }

    private readonly string message;
    public string Message
    {
        get { return this.message; }
    }
}

The TSelf type argument is the key difference between my technique and the other post's technique.  It allows us to avoid reflection in the HandlerResolver:

public interface IHandlerResolver
{
    IQueryHandler<TQuery, TResult> GetHandlerFor<TQuery, TResult>()
        where TQuery: IQuery<TQuery, TResult>;
}

public class HandlerResolver : IHandlerResolver
{
    private readonly IContainer container;

    public HandlerResolver(IContainer container)
    {
        this.container = container;
    }

    public IQueryHandler<TQuery, TResult> GetHandlerFor<TQuery, TResult>()
        where TQuery : IQuery<TQuery, TResult>
    {
        var handler = this.container.Get<IQueryHandler<TQuery, TResult>>();
        return handler;
    }
}

You would register handlers at the composition root.  Here's the interface for a handler, and an implementation for HelloQuery:

public interface IQueryHandler<TQuery, TResult>
    where TQuery : IQuery<TQuery, TResult>
{
    TResult Handle(TQuery request);
}

public class HelloQueryHandler : IQueryHandler<HelloQuery, string>
{
    public string Handle(HelloQuery request)
    {
        var response = string.Format("Well \"{0}\" yourself!", request.Message);
        return response;
    }
}

And finally, the actual query processor:

public interface IQueryProcessor
{
    TResult Process<TQuery, TResult>(IQuery<TQuery, TResult> request)
        where TQuery : IQuery<TQuery, TResult>;
}

public class QueryProcessor : IQueryProcessor
{
    private readonly IHandlerResolver handlerResolver;

    public QueryProcessor(IHandlerResolver handlerResolver)
    {
        this.handlerResolver = handlerResolver;
    }

    public TResult Process<TQuery, TResult>(IQuery<TQuery, TResult> request)
        where TQuery : IQuery<TQuery, TResult>
    {
        var handler = this.handlerResolver.GetHandlerFor<TQuery, TResult>();
        var response = handler.Handle((TQuery)request);
        return response;
    }
}

The interfaces would be simpler without the queries' TSelf type argument, but then the C# compiler can't infer the types, and you end up having to spell out the full Process() call like processor.Process<HelloQuery, string>(query).  I'm trying to avoid this ugliness.

My question is:  Is the TSelf type argument a code smell?  Is there a better alternative that doesn't require falling back on reflection?

One objection I could raise is that your generic type constraint doesn't guarantee what you think it might guarantee. Eg. you could have:

// TSelf is actually not T of Self
public class HelloQuery : IQuery<GoodbyeQuery, string>
{
}

// This one is fine
public class GoodbyeQuery: IQuery<GoodbyeQuery, string>
{
}

In other words, TSelf doesn't actually have to be the T of the class itself. This is not horrible, especially since you seem to only be using the interface as metadata, but it can still lead to confusing results during dependency injection if you've accidentally screwed up a class definition.

However, as long as you're not a masochist and don't declare nonsensical types like this, there is nothing wrong with this. This is an established design pattern known as the curiously recurring template pattern.


String search & replace in C I created a function to do that, and I want to know if there is a problem I'm overlooking about it ..

void replaceStr(buffer, haystack, needle, rep)
    char *buffer, *needle, *rep;
    const char *haystack;
{
    if (!buffer || !haystack || !needle || !rep)
        return;

    size_t rep_length = strlen(rep);
    long long diff = strlen(needle) - rep_length;
    memcpy(buffer, haystack, strlen(haystack) + 1);

    while ((buffer = strstr(buffer, needle))) {
        memcpy(buffer, rep, rep_length + 1); /* +1 for the null byte */
        if (diff > 0) {
            long long i;
            for (i = diff; i <= diff; ++i)
                buffer[i] = buffer[i+1];
            buffer[i] = 0;
        }
    }
}

Since this is about strings, I would rather use strcpy instead of memcpy. This has the benefit, that it saves the unnecessary strlen

strcpy(buffer, haystack);

You can declare the other parameters const too, e.g.

char *buffer;
const char *needle, *rep;

The for loop is a bit confusing, because it copies just one character at offset diff, no matter how large diff is. If you want to close a potential gap, you could again use strcpy

size_t needle_length = strlen(needle);
...
strcpy(buffer + rep_length, buffer + needle_length);

The other way round - if rep is larger than needle, you need to make space for the larger replacement string. Otherwise, you overwrite parts of haystack! (Unless this is what you intended, of course.)

You can avoid this, when you copy from haystack to buffer as you go, something like

size_t needle_length = strlen(needle);
char *found;
while ((found = strstr(haystack, needle))) {
    /* copy haystack part until needle */
    size_t n = found - haystack;
    strncpy(buffer, haystack, n);
    /* copy replacement string */
    strcpy(buffer + n, rep);
    /* adjust pointers */
    buffer += n + rep_length;
    haystack = found + needle_length;
}

/* copy remaining haystack */
strcpy(buffer, haystack);


First BDD/RSpec tests, would like some review regarding idioms, conventions and style This is my first attempt at BDD and RSpec. I'm more of a C# xUnit kind of guy.

I wrote this class while developing a 2D random tile map generator (just for fun). The project is still ongoing.

I would particularly like some review on:

Test granularity/scope in relation to BDD
Use of RSpec
Are these too much like unit tests?
Adherence to Ruby and RSpec idioms, conventions, coding style

I am also open to any other kind of comments and review.

require './map_factory'

describe MapFactory, "map creation" do
    before(:each) do
        @map_factory = MapFactory.new
    end

  it "should return a new Map instance" do
    map = @map_factory.make(20, 20)
    map.should be_an_instance_of(Map)
  end

  it "should be a map of the specified size" do
    map = @map_factory.make(20, 20)
    map.width.should equal(20)
    map.height.should equal(20)
    map.tiles.length.should equal(400)
  end

  it "should be an island" do
    map = @map_factory.make(20, 20)

    # an island should have water all around it
    for x in 0...map.width
        map.tile_at(x, 0).type.should eq(:water)
        map.tile_at(x, 19).type.should eq(:water)
    end

    for y in 0...map.height
        map.tile_at(0, y).type.should eq(:water)
        map.tile_at(19, y).type.should eq(:water)
    end
  end

  it "should have more than half the tiles be landmass" do
    map = @map_factory.make(40, 40)

    number_of_water_tiles = 0
    number_of_land_tiles = 0

    map.tiles.each do |tile|
        if tile.type == :water then
            number_of_water_tiles += 1
        else
            number_of_land_tiles += 1
        end
    end

    number_of_land_tiles.should be > number_of_water_tiles
  end
end

A small hint: the count method could clean up one of your test case, more or less as such:

it "should have more than half the tiles be landmass" do
    map = @map_factory.make(40, 40)
    number_of_water_tiles = map.tiles.count{|tile| tile.type == :water}
    number_of_land_tiles = map.tiles.size - number_of_water_tiles
    number_of_land_tiles.should be > (map.tiles.size / 2)
  end
end

Also your map could go in the before section too imho.


Javascript navigation effects I am attempting to build my own website, based off Google's Material Design principles, to better my design/UX skills.

I am currently working on effects for my navigation, I wondered what would be best to lower browser resource usage when viewing my web page.

  Quick Example:
  When scrolling, without wrapping certain property changes within an if, does the browser (jQuery) attempt to make the property changes although they have already been made?

var scrollPos = $(window).scrollTop();
$(window).scroll(function() {
    scrollPos = $(window).scrollTop();
    if(scrollPos >= 200) {
        if(!$('div.header').is('[style]')) { $('div.header').css('position','fixed'); };
    };
};

compared to...

var scrollPos = $(window).scrollTop();
$(window).scroll(function() {
    scrollPos = $(window).scrollTop();
    if(scrollPos >= 200) {
        $('div.header').is('[style]')) { $('div.header').css('position','fixed');
    };
};

Full Code Example:

//scroll listener
$(window).scroll(function() {
    scrollPos = $(window).scrollTop();
    if(scrollPos >= 200) {
        if(!$('div.header').is('[style]')) { $('div.header').css('position','fixed'); };
        if(!$('div.nav').is('[style]')) { $('div.nav').css('padding','0'); };
        $('div.nav > ul > li, div.nav > ul > ul > li')
            .not('.faded')
            .css('display','none')
            .fadeIn(250)
            .addClass('faded')
            .css('display','inline-block');
        if(!$('div.header div.title').is('[style]')) {
            $('div.header div.title')
                .css('display','none')
                .children('h1')
                .css('display','none');
        }
    } else {
        if($('div.header').is('[style]')) { $('div.header').removeAttr('style'); };
        if($('div.header div.title').is('[style]')) { $('div.header div.title').removeAttr('style'); };
        if($('div.nav').is('[style]')) { $('div.nav').removeAttr('style'); };
        if($('div.nav li').hasCLass('faded')) { $('div.nav li').removeClass('faded'); };
        if($('div.nav li.nav-title').is('[style]')) {
            $('div.nav li.nav-title').fadeOut(250, function() {
                $('div.header div.title h1').fadeIn(250);
            });
        };
    };
});

compared to...

//scroll listener
$(window).scroll(function() {
    scrollPos = $(window).scrollTop();
    if(scrollPos >= 200) {
        $('div.header').css('position','fixed');
        $('div.nav').css('padding','0');
        $('div.nav > ul > li, div.nav > ul > ul > li')
            .not('.faded')
            .css('display','none')
            .fadeIn(250)
            .addClass('faded')
            .css('display','inline-block');
        $('div.header div.title')
            .css('display','none')
            .children('h1')
            .css('display','none');
    } else {
        $('div.header').css('position','static');
        $('div.header div.title').css('display','block');
        $('div.nav').removeAttr('style');
        $('div.nav li').removeClass('faded');
        $('div.nav li.nav-title').fadeOut(250, function() {
            $('div.header div.title h1').fadeIn(250);
        });
    };
});

Does either one make a difference in browser resource usage? Or is one incorrect?

First of all, having scroll handlers is not the best idea. Usually it kills performance.
If you have no choise - use debounce-like function within - in 99% you do not need explict calls on every move.
Don't use $('smth') within your handler. Save them to variables somewhere before. Do not look for them in your DOM tree on every scroll event.


Login page code enhancement I am starting to learn ASP.NET, and I am designing a page for an admin to login and do some stuff. I have the username and password for the admin stored in Web.config (Is that a good practice?).

Second thing, I am checking if the user filled the username and password fields, then I am checking if the values for the admin and password exist in the web.config, then I am checking if these values equal each other, so I am having 3 nested if. Is that a good practice as well?

Here's my code:

if (!string.IsNullOrEmpty(txtLoginId.Text) && !string.IsNullOrEmpty(txtPassword.Text))
            {
                string configLoginID = WebConfigurationManager.AppSettings["AdminLoginID"];
                string configPassword = WebConfigurationManager.AppSettings["AdminPassword"];
                if (!string.IsNullOrEmpty(configLoginID) && !string.IsNullOrEmpty(configPassword))
                {
                    if (txtLoginId.Text == configLoginID && txtPassword.Text == configPassword)
                    {
                        Session["ShoppingAdmin"] = "ShoppingAdmin";
                        Response.Redirect("~/Admin/AddNewProducts.aspx");
                    }
                    else
                    {
                        lblAlert.Text = "Incorrect Username/Password";
                    }

                }
            }
            else
            {
                lblAlert.Text = "Please enter a usernamd and password";
            }

Last thing, checking empty fields using !string.IsNullOrEmpty for each field, is that the right thing to do?

I would like to write a clean code, thanks for helping me with these 3 cases.

I have the username and password for the admin stored in Web.config (Is that a good practice?).

No, it's not good practice to store the Admin login (or any login, for that matter) in the Web.config.

Depending on what ASP.NET model you're using, you have a few options. MVC has a built-in Roles provider which makes it simple to make certain pages/areas only accessible by  a specific role.

Web Forms is also pretty simple, though you have to roll your own access provisions. You can use the built-in Simple Membership if you like, which usually suits quite well once extended to fit your needs. (You can add properties to users pretty easily and such.)

You also aren't encrypting or hashing the password, which is a huge no-no. At the very least you should MD5 hash it, though for security I would recommend SHA1-salt hashing at the very weakest. Though, if you use the built-in Simple Membership for Web Forms or MVC, you'll get encryption and salting by default.

I didn't review the actual code as Heslacher did that just fine, but I do think you need to completely revisit the login system. ASP.NET has built-in providers for this, and they're super easy and, quite honestly, pretty fun to set up. (I don't know why no one has pointed this out in the several months this question has been here, or how it got on my radar, but hopefully you still get this advice.)


How can I make this SQL Bit scalar function more efficient? I'd like to make this SQL scalar function more efficient.  How can I do that?

CREATE FUNCTION [dbo].[fnFilterBySampleType]
(
    @context VARCHAR(10),
    @contextId INT,
    @sampleTypeId INT
)
RETURNS BIT
AS
BEGIN

    IF (
        @context = 'batch' AND @sampleTypeId = 247
        AND EXISTS(
            SELECT *
            FROM batch
            WHERE batchid = @contextId
            AND (batch.sampletypeid = @sampleTypeId OR batch.sampletypeid IS NULL)
        )
    ) RETURN 1

    IF (
        @context = 'batch' AND @sampleTypeId = 3301
        AND EXISTS(
            SELECT *
            FROM batch
            WHERE batchid = @contextId
            AND batch.sampletypeid = @sampleTypeId
        )
    ) RETURN 1

    IF (
        @context = 'batch' AND @sampleTypeId IS NULL
        AND EXISTS(
            SELECT *
            FROM batch
            WHERE batchid = @contextId
            AND batch.sampletypeid IS NULL
        )
    ) RETURN 1

    IF (
        @context = 'sample' AND @sampleTypeId = 247
        AND EXISTS(
            SELECT *
            FROM sample
            WHERE sampleid = @contextId
            AND ([sample].sampletypeid = @sampleTypeId OR [sample].sampletypeid IS NULL)
        )
    ) RETURN 1

    IF (
        @context = 'sample' AND @sampleTypeId = 3301
        AND EXISTS(
            SELECT *
            FROM sample
            WHERE sampleid = @contextId
            AND sample.sampletypeid = @sampleTypeId
        )
    ) RETURN 1

    IF (
        @context = 'sample' AND @sampleTypeId IS NULL
        AND EXISTS(
            SELECT *
            FROM sample
            WHERE sampleid = @contextId
            AND [sample].sampletypeid IS NULL
        )
    ) RETURN 1

    RETURN 0

END

I am rewriting my answer to be more clear...

the way you have it written the exists clause will be executed many times...

You should move the exists clauses inside the if block so that in the cases that the other parts don't match it will be forced to short circuit.  You can't depend on SQL server tsql statements to short circuit in the manner you expect.

So this will not execute the inner "IF exists" if the @context isn't 'batch' etc...

The way you have it written it may or may not execute the exists query.

IF (@context = 'batch' AND @sampleTypeId = 247)
begin

        if EXISTS(
            SELECT *
            FROM batch
            WHERE batchid = @contextId
            AND (batch.sampletypeid = @sampleTypeId OR batch.sampletypeid IS NULL)
        ) RETURN 1  ELSE RETURN 0

end

for more reading on this subject check out this articles.

http://beingmarkcohen.com/?p=62

http://www.sqlmag.com/article/tsql3/short-circuit

http://blogs.msdn.com/b/bartd/archive/2011/03/03/don-t-depend-on-expression-short-circuiting-in-t-sql-not-even-with-case.aspx


Conway's Game of Life - Conventional JavaScript? I'm using the Game of Life Kata to help me learn JavaScript. I've picked up the syntax through Codecademy tutorials, but my current skill level is Novice.

The example code has working functionality for initialising a World with live cells and asking it to 'tick'.

All suggestions for improvement are welcome. I'd especially like feedback on the following:

JavaScript idioms and conventions
Function and variable scoping
Making use of built-in functions and datatypes

myapp = {};

/**
 * World represents a 2 dimensional square matrix of cells.
 * @param dimension
 * @param liveCellCoordinates
 * @returns {myapp.World}
 */

myapp.World = function(dimension, liveCellCoordinatesArray) {
    this.dimension = dimension;
    this.liveCellCoordinates = liveCellCoordinatesArray;
};

/**
 *
 * @returns {Array} of live cells after tick.
 */
myapp.World.prototype.tick = function() {
    var nextGenerationOfLiveCells = [];

// for each cell in world
for(var y=0; y<this.dimension; y++) {
    for(var x=0; x<this.dimension; x++) {

        var liveNeighbours = this.getLiveNeighbourCount(x, y);
        if(this.isCellAlive(x, y)) {
            if(isSurvivor(liveNeighbours)) {
                nextGenerationOfLiveCells.push([x,y]);
            }
        }
        else {
            if(isBorn(liveNeighbours)) {
                nextGenerationOfLiveCells.push([x,y]);
            }
        }
    }
}

this.liveCellCoordinates = nextGenerationOfLiveCells;

return nextGenerationOfLiveCells;
};

myapp.World.prototype.getLiveNeighbourCount = function(x, y) {
var liveNeighbours = 0;

if(this.isCellAlive(x-1, y-1)) liveNeighbours++;
if(this.isCellAlive(x,   y-1)) liveNeighbours++;
if(this.isCellAlive(x+1, y-1)) liveNeighbours++;
if(this.isCellAlive(x-1, y)) liveNeighbours++;
if(this.isCellAlive(x+1, y)) liveNeighbours++;
if(this.isCellAlive(x-1, y+1)) liveNeighbours++;
if(this.isCellAlive(x,   y+1)) liveNeighbours++;
if(this.isCellAlive(x+1, y+1)) liveNeighbours++;

//  jstestdriver.console.log(">>> liveNeighbours of ", x, y, ": ", liveNeighbours);

return liveNeighbours;
};

myapp.World.prototype.isCellAlive = function(x, y) {
    for(var cell in this.liveCellCoordinates) {
        if(x === this.liveCellCoordinates[cell][0] && y === this.liveCellCoordinates[cell][1]) {
            return true;
        }
    }
    return false;
};

function isBorn(liveNeighbours) {
    return liveNeighbours === 3;
}

function isSurvivor(liveNeighbours) {
    return liveNeighbours === 2 || liveNeighbours === 3;
}

Looks quite good overall; not much to criticise. (I'm not looking at the overall structure or logic - just the style; the game of life can be written in so many ways)

Anyway, what I have is very trivial:

Missing indentation in the bodies of tick and getLiveNeighbourCount
Don't use a for...in loop for arrays (in isCellAlive); better and clearer to use a regular for-loop like you do elsewhere.
You ought to define myapp as window.myapp. Right now it's implicitly global, because it's missing a var declaration, but you might as well make that explicit (or perhaps it doesn't need to be global at all?)
Some more comments in the code itself would probably be nice.


Erlang parallel server using sockets The task was to build a simple parallel server in erlang that prints what it receives through a socket and alerts when the socket is closed.

-module(server).
-export([start/0]).

start() ->
    {ok, ListenSocket} = gen_tcp:listen(8211,[binary,{packet,4},{active, once}]),
    spawn(fun() -> accepter(ListenSocket) end),
  sleep().

sleep() ->
    receive
        cancel -> void
    end.

accepter(ListenSocket) ->
    {ok, Socket} = gen_tcp:accept(ListenSocket),
    spawn(fun() -> accepter(ListenSocket) end),
    Proc = spawn(fun() -> worker(Socket) end),
    gen_tcp:controlling_process(Socket, Proc).

worker(Socket) ->

    receive
        {tcp, Socket, Data} ->
            io:format("Received ~p",[Data]),
            inet:setopts(Socket, [{active, once}]),
            worker(Socket);
        {tcp_closed, Socket} ->
            io:format("Socket ~p closed",[Socket])
    end.

Is this the correct way of building such a thing?

Let's look at your solution function by function. First, start/0:

start() ->
    {ok, ListenSocket} = gen_tcp:listen(8211,[binary,{packet,4},{active, once}]),
    spawn(fun() -> accepter(ListenSocket) end),
    sleep().

First, port 8211 is hard-coded. You might instead want to export both a start/0 and a start/1, the latter taking a port number argument, with start/0 calling start/1 with 8211 as the default. You don't need {packet,4} unless your Erlang client is also using that option, or your non-Erlang client precedes each packet with a 4-byte integer in network order that indicates the packet length. You probably also want to use the {reuseaddr,true} option so that you can stop the application and quickly start it again on the same port. With all these changes, we wind up with this:

start() ->
    start(8211).
start(Port) ->
    {ok, ListenSocket} = gen_tcp:listen(Port, [binary,{active,once},{reuseaddr,true}]),
    spawn(fun() -> acceptor(ListenSocket) end),
    sleep().

(Note that technically you don't need the sleep/0 call, but I'm sure you put it there to make the program easier to work with interactively from an Erlang shell, so I'm leaving it.)

Next, we have sleep/0:

sleep() ->
    receive
        cancel -> void
    end.

I would return ok here, since that's extremely common in Erlang. Using the atom void is not.

Next, we have acceptor/1:

acceptor(ListenSocket) ->
    {ok, Socket} = gen_tcp:accept(ListenSocket),
    spawn(fun() -> acceptor(ListenSocket) end),
    Proc = spawn(fun() -> worker(Socket) end),
    gen_tcp:controlling_process(Socket, Proc).

The big problem here is the spawning of the worker process, and then handing control of the accepted socket over to it. There are two problems with this:

The accepted socket inherits {active,once} from the listen socket. But what happens if a message arrives after the worker is spawned but before control of the socket is given to it? The message is queued to the controlling process, which is the acceptor process, not the new worker process. The worker process misses that message, and since the acceptor never checks it, the message just gets lost.
Once the acceptor accepts a connection, it spawns a new acceptor. This is good. But it then spawns a worker, gives it the control of the socket, and then dies. This is pointless. Since the acceptor spawns a new acceptor, it can just continue and call worker/1 itself, and that way it can avoid the problem of changing the socket's controlling process, which means it won't miss any messages.

We can fix these problems by revising acceptor/1 like this:

acceptor(ListenSocket) ->
    {ok, Socket} = gen_tcp:accept(ListenSocket),
    spawn(fun() -> acceptor(ListenSocket) end),
    worker(Socket).

And finally, we have the original worker/1 function:

worker(Socket) ->
    receive
        {tcp, Socket, Data} ->
            io:format("Received ~p", [Data]),
            inet:setopts(Socket, [{active,once}]),
            worker(Socket);
        {tcp_closed, Socket} ->
            io:format("Socket ~p closed", [Socket])
    end.

This function looks just right, no changes needed.


Redis lock implementation Considering I'm not implementing distributed lock mechanism, is this code correct and clear?

class RedisLock
  class NotAcquired < StandardError; end

  def initialize(redis)
    @redis = redis
  end

  def lock(key, expiration_ms)
    val = SecureRandom.random_number(100000000000000000).to_s

    if @redis.set(key, val, nx: true, px: expiration_ms)
      yield
      unlock(key, val)
      true
    else
      false
    end
  end

  def lock!(*args, &block)
    unless lock(*args, &block)
      raise NotAcquired.new("Could not acquire lock with #{args}")
    end
  end

  def unlock(key, val)
    check_and_delete = <<-LUA
      if redis.call('get', KEYS[1]) == KEYS[2] then
        redis.call('del', KEYS[1])
      end
    LUA

    @redis.eval(check_and_delete, [key, val])
  end
end

RedisLock.new(Redis.current).lock!('key', 10000) { do_something }

Here's a stab at clarifying the intent without changing any of the functionality. I didn't try to run it or anything, so don't hold me to syntax.

# handles locking and unlocking redis for a particular operation
class RedisLock
  # thrown when a lock cannot be acquired
  class NotAcquired < StandardError; end

  # since the redis key and ms are used in pretty much every method,
  # might as well go ahead and pass them in the constructor.
  # set defaults here if there are defaults that make sense.
  # make a yardoc comment and describe what these all are.
  def initialize(redis: Redis.current, nx: true, redis_key:, expiration_ms:)
    @redis = redis
    @redis_key = redis_key
    @expiration_ms = expiration_ms
    @nx = nx
  end

  # call it with_lock instead of lock
  # to make it more apparent it accepts a block
  def with_lock!
    raise(NotAcquired, "Could not acquire lock with #{args}") unless lock
    yield
    unlock(redis_key, random_lock_number)
    true # do you really need to return true here?
  end

  def with_lock
    with_lock!
  rescue NotAcquired
    false
  end

  private

  # set instance variables in the initializer, but never call them directly.
  attr_reader :redis, :redis_key, :expiration_ms, :nx

  def lock
    redis.set(redis_key, random_lock_number, nx: nx, px: expiration_ms)
  end

  # number is a little decieving here when you call `.to_s` at the end.
  # can you use SecureRandom.uuid instead?
  def random_lock_number
    @random_lock_number ||= SecureRandom.random_number(100_000_000_000_000_000).to_s
  end

  # no need for this to be exposed publicly.
  # calling eval directly is usually a bad idea.
  def unlock
    redis.eval(redis_check_and_delete, [redis_key, random_lock_number])
  end

  def redis_check_and_delete
    <<-LUA
      if redis.call('get', KEYS[1]) == KEYS[2] then
        redis.call('del', KEYS[1])
      end
    LUA
  end
end

And to call it:

RedisLock.new(redis_key: 'key', expiration_ms: 10000).with_lock! { do_something }


Why is my C program for calculating Euler's constant of poor quality? I was trying to post this code to a Wikipedia article, but it was soon removed.  I then asked about the code in the page's talk section, and some other contributors said that is was "very poor"  and that "there are clearly going to be underflows all over the place."  What does that mean?  What is an "underflow"?

Could you please tell me more about my program's flaws and how it can be improved?  Please give me some constructive criticism.  Here is where I originally posted the code.

#include <stdio.h>

long factorial(int n) {
    long result = 1;
    for (int i = 1; i <= n; ++i)
    result *= i;
    return result;
}

int main ()
{
    double n=0;
    int i;
    for (i=0; i<=32; i++) {
        n=(1.0/factorial(i))+n;
    }
    printf("%.32f\n", n);
}

Here is the result of the program 2.71828182845904553488480814849027

If you're to implement something like this, you should first learn about how these things are done. I hope this doesn't sound too harsh. To explain it better, here is my variant of your code, with comparison to what C computes as e using expl(1):

#include <stdio.h>
#include <math.h>

int main ()
{
    long double n = 0, f = 1;
    int i;
    for (i = 28; i >= 1; i--) {
        f *= i;  // f = 28*27*...*i = 28! / (i-1)!
        n += f;  // n = 28 + 28*27 + ... + 28! / (i-1)!
    }  // n = 28! * (1/0! + 1/1! + ... + 1/28!), f = 28!
    n /= f;
    printf("%.64llf\n", n);
    printf("%.64llf\n", expl(1));
    printf("%llg\n", n - expl(1));
    printf("%d\n", n == expl(1));
}

Output:

2.7182818284590452354281681079939403389289509505033493041992187500
2.7182818284590452354281681079939403389289509505033493041992187500
0
1

There are two important changes to your code:

This code doesn't compute 1, 1*2, 1*2*3,... which is O(n^2), but computes 1*2*3*... in one pass (which is O(n)).
It starts from smaller numbers. Let us, for a moment, assume that your factorials are correct (see below). When you compute

1/1 + 1/2 + 1/6 + ... + 1/20!

and try to add it 1/21!, you are adding

1/21! = 1/51090942171709440000 = 2E-20,

to 2.something, which has no effect on the result (double holds about 16 significant digits). This effect is called underflow.

However, had you started with these numbers, i.e., if you computed 1/32!+1/31!+... they would all have some impact.

Notice that 32! needs 118 bits, i.e., 15 bytes. Your long type doesn't hold as much (the standard says at least 32 bits; it's not likely it will hold more than 64). If we add printf("%ld\n", factorial(i)); to your main for-loop, we see the factorials:

1
2
6
24
120
720
5040
40320
362880
3628800
39916800
479001600
6227020800
87178291200
1307674368000
20922789888000
355687428096000
6402373705728000
121645100408832000
2432902008176640000
-4249290049419214848
-1250660718674968576
8128291617894825984
-7835185981329244160
7034535277573963776
-1569523520172457984
-5483646897237262336
-5968160532966932480
-7055958792655077376
-8764578968847253504
4999213071378415616
-6045878379276664832

See the negatives? That's when your (long) integer grew too much and restarted from its lowest possible value. Read about overflows; it's important to understand them.

By the way, I'm getting the same result (on my computer) even if I replace long with long long (and apply the proper printf format %lld).

Computing this kind of stuff is complex, and takes quite a bit of knowledge about how the numbers are stored in the computer, as well as the numerical mathematics and the mathematical analysis. I don't consider myself an expert, so so there might be more to this problem than what I've wrote. However, my solution seems in accordance to what C computes with its expl function, on my 64bit machine, compiled with gcc 4.7.2 20120921.


Validating an IP address and returning the reason it's invalid I have a method that validates a user-written IP address in an Android application. If it's invalid, I need to know why and notify the user using a Toast.

I created an enum for this called Status that has a list of reasons for an IP address to be invalid and a value for when it's valid. The method isValid will return a Status value depending on whether or not it's invalid.

Android code

switch(IP.isValid(ip.getText().toString())) {
     case INVALID_NUMBER:
         Toast.makeText(MainActivity.this, "You have entered an invalid octect(s) value", Toast.LENGTH_SHORT).show();
         break;
     case INVALID_LENGTH:
         Toast.makeText(MainActivity.this, "You have entered an IP with an invalid length", Toast.LENGTH_SHORT).show();
         break;
     case NON_NUMERIC:
         Toast.makeText(MainActivity.this, "You have entered a non-numeric IP", Toast.LENGTH_SHORT).show();
         break;
}

ip is an EditText field.

Status enum

public enum Status {
    INVALID_NUMBER,
    INVALID_LENGTH,
    NON_NUMERIC,
    VALID
}

isValid() method

public static Status isValid(String ip) {
    String[] octets = ip.split("\\.");
    if(octets.length != 4) {
        return Status.INVALID_LENGTH;
    }

    for(String octet : octets) {
        if(StringUtils.isNumeric(octet)) {
            int val = Integer.parseInt(octet);
            if(val > 255 || val < 0) {
                return Status.INVALID_NUMBER;
            }
        } else {
            return Status.NON_NUMERIC;
        }
    }

    return Status.VALID;
}

StringUtils.isNumeric()

(StringUtils is a class I created, it's not from Apache)
https://stackoverflow.com/a/237204/4355066

public static boolean isNumeric(String str) {
    if (str == null) {
        return false;
    }
    int length = str.length();
    if (length == 0) {
        return false;
    }
    int i = 0;
    if (str.charAt(0) == '-') {
        if (length == 1) {
            return false;
        }
        i = 1;
    }
    for (; i < length; i++) {
        char c = str.charAt(i);
        if (c < '0' || c > '9') {
            return false;
        }
    }
    return true;
}

Is this the best way to this?

Your code has three major errors that all the previous reviewers failed to spot. If I were a lecturer, I'd show your code to the students as an example why they should always use libraries, even doing something seemingly simple as validating the IPv4 address.

It accepts 127.0.0.1..... as a valid IP address, which it is not. Pass -1 as a second argument to the split function.
Because of your faulty isNumeric function, the address 0.00123.0000034.12 is considered valid, too.
Similar to above, the address 127.-0.-0.1 is accepted too.

Consider using a library for validating the IP addresses

As described above, you can never be too careful. Besides that, we are starting to make the migration to the IPv6 standard, which is harder to validate.

So, if you can rely on a well tested library for IP validation that does not bring an additional huge dependency to your code, use that instead.

It is not resilient against crafted inputs

Depending on the source of the input, the validation function might pose a huge performance risk and cause OM (out of memory) errors.

See, even really short strings of just dots ".........x" create one String object per character. String object and a pointer overhead in Java is rather in the neighborhood of 44 bytes. Therefore, someone sending you a 10 MiB string is typically going to overflow the heap space.

Code locality (tie error messages to enum values)

Because error strings are tied to enum values, it is a good thing to specify them in the actual enum construct.

public enum IpValidation {
    INVALID_NUMBER("You have entered an invalid octect(s) value"),
    INVALID_LENGTH("You have entered an IP with an invalid length"),
    NON_NUMERIC("You have entered an IP with an invalid length"),
    VALID("This IP address is valid");

    private final String error;

    private IpValidation(final String error) {
        this.error = error;
    }

    public String getError() {
        return error;
    }
}

Give the enum a more meaningful name

Strings is just a bad name to give an enum. Pick something else.

Nits

Do put a space after the switch, if, and for keywords.
Using enums on Android might have performance consequences. Depending on the use case, you might want to avoid it.


Project Euler -problem 1 Find the sum of all the multiples of 3 or 5 below 1000.

https://projecteuler.net/problem=1
Is there any hack to do modulo operation in better way?

#include<iostream>

int main()
{
    unsigned int sum=0;

    for( int i = 0; i < 1000; ++i )
    {
        if( ( i % 3 == 0 ) || ( i % 5 == 0 )  ||
           ( ( i % 3 == 0 ) && ( i % 5 == 0 ) ) )
        {
            sum += i;
        }
    }
    std::cout<< sum <<"\n";
}

This is not a hack.

if( ( i % 3 == 0 ) || ( i % 5 == 0 )  ||
   ( ( i % 3 == 0 ) && ( i % 5 == 0 ) ) )

What you have here is essentially:

if( a || b || ( a && b ) )

The truth table for a || b is:

\begin{array} {|cc|c|}
\hline
a & b & a \lor b \\
\hline
0 & 0 & 0\\
0 & 1 & 1\\
1 & 0 & 1\\
1 & 1 & 1\\
\hline
\end{array}

Where \$0\$ indicates false and \$1\$ indicates true.

Therefore, the last part of your if is completely useless.

if( a || b || ( a && b ) )

is exactly the same as

if( a || b )

So your if statement can be:

if( ( i % 3 == 0 ) || ( i % 5 == 0 ) )

Arithmetic Sum

Project Euler 1 can be transformed into a Arithmetic sum problem.

Ask yourself these questions:

How many numbers that are multiples by 3 are there below 1000 ?
How many numbers that are multiples by 5 are there below 1000 ?
How many numbers that are multiples by both 3 and 5 (i.e. 15) are there below 1000 ?

Then use the values for these individual arithmetic sums to arrive at your final answer. This will give your code a complexity of \$O(1)\$ instead of \$O(n)\$

As this is Project Euler, I only want to give you a little push in the right direction, hope this helps.


Macro to run code once during the lifetime of the program I'm using this little macro a lot:

#define RUN_ONCE(runcode) \
{ \
    static bool code_ran = 0; \
    if(!code_ran){ \
        code_ran = 1; \
        runcode; \
    } \
}

I find it useful when I want to initialize stuff just once when I don't really care about performance (for example, if it's inside a render loop, it takes just 60 if's per sec).  In a random generator, I can do RUN_ONCE(init_random()), so I don't need to separately initialize it, which prevents me from crashing when I call the function without initializing it first.

I'd like to know if any of you use a similar system, and if there's better way of doing this. Or if I should stop using this method immediately...

It is not thread safe.  You can use Boost, or standard C++11 std::call_once.

Answer to comment:

Boost and C++11 are defining include library for launching threads and thread synchronization (locks, atomic variables…). The call_once function can either use those to ensure thread safety or use the thread lib of the OS (pthreads for *nix).

A very simplified (unefficent) implementation might be:

static std::mutex mutex;
static bool called = false;
{
   std::lock_guard<std::mutex> lock(mutex);
   if (!called) {
      f(); // <- User code
      called = true;
   }
}

It is not very efficient because the lock is always taken. An optimisation it to atomically check the once flag before taking the mutex:

static std::mutex mutex;
static std::atomic<bool> called = false;
{
   if (!called) {
     std::lock_guard<std::mutex> lock(mutex);
     if (!called) {
        f(); // <- User code
        called = true;
     }
   }
}

The implementation of pthread_once in glibc is interesting: it is much more complicated as it tries to behave correctly in presence of fork.

Boost/C++11 use a functor (function pointer or object with operator()): this way it can be implemented as a function and not a macro (you can use lambda in C++11 to avoid defining a separate function).


Getting a sum of added cells in an ultragridview The objective of this code is to add the first 3 cells of a column
  with the next 9 in another column and then to total out in 2
  currencyeditor control (which is like a textbox).

Here is a demonstration of what I am adding.

For the 1st control it's going to be the actual + adjusted (left square).
For the 2nd control value will be the actual + original (right square).

I've never added different rows of an ultragridrow before this.  It works right now but doesn't look neat or professional, and I really hate the saying "at least it works" .  I was just wondering if you could help me write this code neater or more efficiently?

private void doMath()
{
    decimal actualsum = 0m;
    decimal adjustedsum = 0m;
    decimal totalsum = 0m;

    for (int i = 0; i < 2; i++)
    {
        var row = dgvBudget.Rows[i];
        actualsum += Convert.ToDecimal(row.Cells["Actual_c"].Value);
    }
    for (int i = 3; i < 12; i++)
    {
        var row = dgvBudget.Rows[i];
        adjustedsum += Convert.ToDecimal(row.Cells["PrevFY"].Value);
    }
    totalsum = (adjustedsum + actualsum);
    nbrAdjustedTotal.Value = totalsum;
    adjustedsum = 0;
    actualsum = 0;
    totalsum = 0;

    for (int i = 0; i < 2; i++)
    {
        var row = dgvBudget.Rows[i];
        actualsum += Convert.ToDecimal(row.Cells["Actual_c"].Value);
    }
    for (int i = 3; i < 12; i++)
    {
        var row = dgvBudget.Rows[i];
        adjustedsum += Convert.ToDecimal(row.Cells["BudgetAmt"].Value);
    }
    totalsum = (adjustedsum + actualsum);
    nbrOriginalTotal.Value = totalsum;
}

Your Concerns

  I was just wondering if you could help me write this code neater or more efficiently?

Efficiency

You are looping through the data twice. Once to calculate adjustedsum from column["PrevFY"] and once for column["BudgetAmt"]. In both cases, actualsum gets calculated exactly the same way. It seems you so badly wanted to reuse variables that you forgot you could have just looped through the rows and store data in multiple variables.

decimal actualsum = 0m;
decimal adjustedTotal = 0m;
decimal originalTotal = 0m;

for (int i = 0; i < 2; i++)
{
    var row = dgvBudget.Rows[i];
    actualsum += Convert.ToDecimal(row.Cells["Actual_c"].Value);
}

for (int i = 3; i < 12; i++)
{
    var row = dgvBudget.Rows[i];
    adjustedTotal += Convert.ToDecimal(row.Cells["PrevFY"].Value);
    originalTotal += Convert.ToDecimal(row.Cells["BudgetAmt"].Value);
}

nbrAdjustedTotal.Value = adjustedTotal + actualsum;
nbrOriginalTotal.Value = originalTotal + actualsum;

Compactness (neatness)

You have recurring decimal parsing from data cells:

Convert.ToDecimal(row.Cells["Actual_c"].Value)

You could make a utility method for this. I'm assuming the class for the data row is called Row.

private static decimal ParseDecimal(Row row, string columnName)
{
    return Convert.ToDecimal(row.Cells[columnName].Value);
}

Further refactored:

decimal actualsum = 0m;
decimal adjustedTotal = 0m;
decimal originalTotal = 0m;

for (int i = 0; i < 2; i++)
{
    var row = dgvBudget.Rows[i];
    actualsum += ParseDecimal(row, "Actual_c");
}

for (int i = 3; i < 12; i++)
{
    var row = dgvBudget.Rows[i];
    adjustedTotal += ParseDecimal(row, "PrevFY");
    originalTotal += ParseDecimal(row, "BudgetAmt");
}

nbrAdjustedTotal.Value = adjustedTotal + actualsum;
nbrOriginalTotal.Value = originalTotal + actualsum;

Proposed Result

We could make the code even more compact by using LINQ's Skip, Take and Sum extension methods. I don't have access to the code, but I'm sure you could do something like:

var rows = dgvBudget.Rows.Take(3);
var actualsum = rows.Select(row => ParseDecimal(row, "Actual_c")).Sum();
var rows = dgvBudget.Rows.Skip(3).Take(9);
var adjustedTotal = rows.Select(row => ParseDecimal(row, "PrevFY")).Sum();
var originalTotal = rows.Select(row => ParseDecimal(row, "BudgetAmt")).Sum();

nbrAdjustedTotal.Value = adjustedTotal + actualsum;
nbrOriginalTotal.Value = originalTotal + actualsum;

This does come with a slight performance penalty, since you loop the first 3 items twice (once to skip). But it comes at a good increase in readability and compactness.


Versatile string validation I passed a technical test the other day, one part included a cheap string validation and I am wondering this can be further improved to be more versatile to requirement changes.

The requirements were something like that

  Create a Validate method, which accepts a string and returns true if it's valid and false if it's not.

  A string is valid if it satisfies the rules below:

  The string must be at least 6 characters long and not exceed 16 characters.
  The string must contain only letters, numbers and optionally one hyphen (-).
  The string must start with a letter, and must not end with a hyphen.
  For example, validate("Michelle Belle"); would return false because it contains a space.

My solution was like that:

public static class ComparableExtensions
{
    public static bool IsStrictlyLowerThan<TComparable>(this TComparable comparable, TComparable value)
        where TComparable : IComparable<TComparable>
    {
        return comparable.CompareTo(value) < 0;
    }

    public static bool IsStrictlyGreaterThan<TComparable>(this TComparable comparable, TComparable value)
        where TComparable : IComparable<TComparable>
    {
        return comparable.CompareTo(value) > 0;
    }

    public static bool IsStrictlyNotBetween<TComparable>(this TComparable comparable, TComparable lowerBound, TComparable upperBound)
        where TComparable : IComparable<TComparable>
    {
        if (lowerBound.IsStrictlyGreaterThan(upperBound))
        {
            throw new ArgumentOutOfRangeException(nameof(lowerBound) + nameof(upperBound));
        }

        return comparable.IsStrictlyLowerThan(lowerBound) || comparable.IsStrictlyGreaterThan(upperBound);
    }
}

public static class CharExtensions
{
    public static bool IsLetterOrDigit(this char c)
    {
        return char.IsLetterOrDigit(c);
    }

    public static bool IsLetter(this char c)
    {
        return char.IsLetter(c);
    }

    public static bool IsHyphen(this char c)
    {
        return c == '-';
    }
}

public class Test
{
    public static bool Validate(string str)
    {
        if (str.Length.IsStrictlyNotBetween(6, 16))
        {
            return false;
        }

        if (!str.First().IsLetter() || str.Last().IsHyphen())
        {
            return false;
        }

        var hyphenCount = 0;

        for (var i = 1; i < str.Length - 1; i++)
        {
            if (str[i].IsLetterOrDigit())
            {
                continue;
            }
            if (str[i].IsHyphen())
            {
                hyphenCount++;
                if (hyphenCount > 1)
                {
                    return false;
                }
            }
            else
            {
                return false;
            }
        }

        return true;
    }
}

I purposefully decided to not go with Regular Expressions to keep the logic readable and I am wondering if my code can be further refactored to incorporate new business rules.

There's a bug: you're not checking if the last character is a letter or digit, only that it isn't a hyphen, so this fails to reject "abcdef&". Denis' solution may be less efficient (2 iterations instead of 1), but with at most 16 characters that's not much of a concern, and it's both easier to read and it works correctly.
The first two rules read very nicely. I especially like that the business rules are translated to code almost 1:1, that will make updating easier. However, I do think those extension methods are over-engineered. str.Length < 6 || str.Length > 16 and !char.IsLetter(str.First()) || str.Last() == '-' is already quite readable, and that doesn't require extra code that needs to be understood and maintained.
You can use => syntax for methods with single-expression bodies.


Program that converts a number to a letter of the alphabet - follow-up This is a follow up from my previous post: Program that converts a number to a letter of the alphabet

This is meant to teach my little brother (who only knows how to make games in Geometry Dash) about C, and computers. This is an example I've written meant to be shown and explained to him.

#include <stdio.h>
#include <stdbool.h>

int main() {
  //A string is an array of characters
  const char * alphabet = "ABCDEFGHIJKLMNOPQRSTUVWSYZ";
  int temp,    letter   = 0;

  while( (1 <= letter && letter <= 26) == false){
    /*
     * if scanf fails, valadate input to avoid infinite loop
     * (the below while statement).
     * Uncomment the below printf and comment out the below
     * while statement to see what happens if you type
     * something like "asdef4grvgrthd" instead of a number.
     */
    if(scanf("%d", &letter) != 1){
      while((temp=getchar()) != EOF && temp != '\n');
      //printf("Fun loop :) ");
    }
  }

  printf("The number %i corresponds to the letter '%c'\n",
          letter, alphabet[letter-1]);
}

This is intended to help beginners understand C by being a slightly harder "Hello World" program.

int letter is not a letter. When printing you call it The number. Name it accordingly: int number.
The condition (1 <= letter && letter <= 26) == false is very hard to follow. As a general rule, avoid boolean constants in conditions. Rewriting it as:

!(1 <= letter && letter <= 26)

immediately calls for a deMorgan transformation into a much more readable form:

(letter < 1 || letter > 26)

It is also recommended to not rely on the operator precedence, which is very easy to get wrong. Use parenthesis instead:

((letter < 1) || (letter > 26))

Avoid raw loops. Each loop represent an important algorithm, and as such deserves a name. The goal of:

while((temp=getchar()) != EOF && temp != '\n');

is to discard the line. Factor it out into a function:

void discard_line()
{
    int temp;
    while((temp=getchar()) != EOF && temp != '\n') {
    }
}

As a perk benefit, temp is no longer cluttering the essential logic.

I also recommend to be a bit more explicit with the empty loops.
Avoid magic numbers. 26 is really strlen(alphabet). Better yet, declare the alphabet as an array, rather than a pointer, and use sizeof.


"Computer guesses my number" game In this exercise I have to do this:

  Modify the program so that no matter what number the user thinks of (1-100) the computer can guess it in 7 or less guesses.

I'm just a beginner in Java and I'd like to know what you think about my code:

public static void main(String[] args) {

    Scanner in = new Scanner(System.in);
    Random rand = new Random();
    int randNum = 0;
    int upperLimit = 100;
    int lowerLimit = 1;
    String myAnswer = "";

    do {
        randNum = rand.nextInt(upperLimit - lowerLimit + 1) + lowerLimit;
        System.out.println("I think it's " + randNum);
        myAnswer = in.nextLine();

        if(myAnswer.equals("tl")) {  //too low
            lowerLimit = randNum + 1;
        }
        else if(myAnswer.equals("th")) { // too high
            upperLimit = randNum - 1;
        }
    }while(!myAnswer.equals("y"));

    in.close();
    System.out.println("YAAAY! :D");
}

I think it's too slow, because sometimes it takes the computer more than 7 times to guess a number.

The guessing takes too long because picking a random number is not the best idea.
For example if the number to guess is 100,
and the computer is really really unlucky,
it might pick the numbers 1, 2, 3, 4, 5, 6, ... and take 100 guesses!

A better way is to halve the intervals.
That is, if you know the number must be between 1 and 100, then pick 50.
If that's too high, and so you know the number is between 1 and 50,
then pick 25. And so on.

So, changing one line could make a huge difference:

    // randNum = rand.nextInt(upperLimit - lowerLimit + 1) + lowerLimit;
    randNum = (upperLimit + lowerLimit + 1) / 2;

This is called a binary search.
As @cbojar pointed out in comments,
since binary search takes \$\log_2(n)\$ tests where \$n\$ is the total number of objects, and since \$\log_2(100) = 6.643856\$,
that means it's guaranteed that this algorithm will find the correct number in at most 7 steps.

The string literals "th", "tl", "y" that control the flow of the program are buried inside the code.
If you read this program a month from now,
it might not be obvious how it works until you read the details.
Something that would help with that,
and a good practice is to extract these string literals to constants,
for example:

private static final String TOO_HIGH_MARKER = "th";
private static final String TOO_LOW_MARKER = "tl";
private static final String CORRECT_MARKER = "y";

// ...

public static void main(String[] args) {
    // ...

    do {
        // ...

        if (myAnswer.equals(TOO_LOW_MARKER)) {  //too low
            lowerLimit = randNum + 1;
        } else if (myAnswer.equals(TOO_HIGH_MARKER)) { // too high
            upperLimit = randNum - 1;
        }

        // ...
    } while (!myAnswer.equals(CORRECT_MARKER));

Now all these magical strings are easy to see near the top.
Should you decide to use different values,
you could just edit them right at the top, simple and clear.

UPDATE

As an extra touch,
as @Tom pointed out in a comment,
it might be a good / interesting idea to make these equals comparisons a bit more fault tolerant by making them case insensitive by changing to equalsIgnoreCase, for example:

        if (myAnswer.equalsIgnoreCase(TOO_LOW_MARKER)) {  //too low

This is of course less efficient than a simple equals,
so don't use it excessively everywhere,
only when justified.

These initializers are redundant:

int randNum = 0;
String myAnswer = "";

Because, these variables will always be assigned before they are used.
As such, you can remove the initializers:

int randNum;
String myAnswer;

It's a cosmetic thing,
but your formatting doesn't follow the standard in a few places.

Instead of this:

if(myAnswer.equals("tl")) {  //too low
    lowerLimit = randNum + 1;
}
else if(myAnswer.equals("th")) { // too high
    upperLimit = randNum - 1;
}

It should be like this:

if (myAnswer.equals("tl")) {  //too low
    lowerLimit = randNum + 1;
} else if (myAnswer.equals("th")) { // too high
    upperLimit = randNum - 1;
}

And instead of this:

}while(!myAnswer.equals("y"));

It should be like this:

} while (!myAnswer.equals("y"));

Again, these are minor issues, but as your programs get larger and larger these little details can add up and seriously hurt readability.
Use editor environments like Eclipse or IntelliJ or Netbeans that can automatically reformat your code following the standard.


Replacing Items in an NSMutableArray I have a function which is supposed to replace all instances of variables (represented as NSStrings) from an NSDictionary with their corresponding values before calling another procedure. I ended up doing this with a simple for loop, though I suspect that there is a better way.  Do you have a suggestion how to do this differently?

+ (double)runProgram:(id)program usingVariables:(NSDictionary *)variables {
    NSMutableArray *stack;
    if ([program isKindOfClass:[NSArray class]]) {
        stack = [program mutableCopy];
        for (int i=0; i<stack.count; i++) {
            /* replace variable values with NSNumber values */
            NSNumber *variableValue = [variables valueForKey:[stack objectAtIndex:i]];
            if ([variableValue isKindOfClass:[NSNumber class]]) {
                [stack replaceObjectAtIndex:i withObject:variableValue];
            }
            /* all variables should be replaced now */
        }
    }
    return [self popOperandOffProgramStack:stack];
}

+ (double)runProgram:(id)program usingVariables:(NSDictionary *)variables {
    NSMutableArray *stack;
    if ([program isKindOfClass:[NSArray class]]) {

Checking the type of incoming parameters is a little suspicious. Do you really want to allow other parameters and just ignore them?

        stack = [program mutableCopy];

This process would actually be easier if you didn't create a copy, but instead create an empty NSMutableArray and add the filtered objects to it.

        for (int i=0; i<stack.count; i++) {

Presumably, you are using this because you need the indexes to replace the objects. If you stop making a copy, you should be able to replace this a foreach loop over the program.

            /* replace variable values with NSNumber values */
            NSNumber *variableValue = [variables valueForKey:[stack objectAtIndex:i]];
            if ([variableValue isKindOfClass:[NSNumber class]]) {

Again why are you checking types? Are there other things besides numbers in your variables? If there are, do you just want to ignore them?

                [stack replaceObjectAtIndex:i withObject:variableValue];
            }
            /* all variables should be replaced now */

This comment isn't true here, because its on the wrong side of the brace

        }
    }
    return [self popOperandOffProgramStack:stack];
}


Counting occurrences of Char8s in a file To learn some Data.Map and Control.Monad.State, I have written the following code, which should count the occurrences of Char8 in a given file.

module Main where

import           Control.Monad.State
import qualified Data.ByteString.Char8 as B
import           Data.Char
import           Data.Map              (Map)
import qualified Data.Map              as M
import           Data.Maybe
import           System.Environment

type Counter = ([Char], (Char, Integer), (Char, Integer), Map Char Integer)

main :: IO ()
main = do
  file <- liftM head getArgs
  string <- B.readFile file
  let (chars, seldom, often, counter) = execState (countChars string) ([], (' ', 10^12), (' ', 0), M.empty)
  mapM_ (\k -> putStrLn ('\'':k:"' " ++ show (fromJust $ M.lookup k counter))) chars
  putStrLn $ "minimum occurence: " ++ show (fst seldom) ++ ", " ++ show (snd seldom)
  putStrLn $ "maximum occurence: " ++ show (fst often)  ++ ", " ++ show (snd often)
  putStrLn $ "overal chars     : " ++ show (B.length string)

countChars :: B.ByteString -> State Counter ()
countChars bs | B.null bs = return ()
              | otherwise = do
  let c  = B.head bs
  let cs = B.tail bs
  unless (not . isPrint $ c) $ do
    (pre, minOcc, maxOcc, counter) <- get
    newPre <- return $ if (c `elem` pre)
                         then pre
                         else pre ++ [c]
    newCounter <- return (M.insertWith (+) c 1 counter)
    let newMinOcc = (\ (k1, v1) (k2, v2) -> if v2 < v1 then (k2, v2) else (k1, v1)) minOcc (c, newCounter M.! c)
    let newMaxOcc = (\ (k1, v1) (k2, v2) -> if v2 > v1 then (k2, v2) else (k1, v1)) maxOcc (c, newCounter M.! c)
    put (newPre, newMinOcc, newMaxOcc, newCounter)
  countChars cs

This code works as I want it for small files, counting and displaying the stats of the file, but it fails when the file gets bigger:

$ ls -l 46000.txt.utf-8
-rw-r--r-- 1 nobbz nobbz 156315 Jun 16 20:33 46000.txt.utf-8

$ ./Main 46000.txt.utf-8
Stack space overflow: current size 8388608 bytes.
Use `+RTS -Ksize -RTS' to increase it.

I don't grasp why a file of about 150kiB can overflow an 8Mib stack. If I increase to 10 MiB (./Main 46000.txt.utf-8 +RTS -k10M) it works, but using also -sstderr shows that the Program consumes 107 MiB in total.

How could I decrease memory usage, so it would work with large files and default stack-size?

Edit

Just to mention:

I want to keep the list of chars and their count of occurrence in the order of occurrence of the corresponding char, also I want that on equal count for most often and most seldom used chars that one "wins" that has occurred first.

Also I am aware of the fact that I have a file that states to be UTF-8 encoded and my program can only "see" ASCII, but I just took a random book from gutenberg.org to have a real text and not some "random" bytes in huge binarys or something comparable.

If you're using "Char8", then it would make a lot of sense to me to write a simple histogram type function using a mutable attay in the ST monad, and then calculate the minimum and maximum values at the end; there's no need to do it as you go.

import Data.Vector.Unboxed.Mutable as V
import Data.Vector.Unboxed as V
import Data.ByteString as B
import Control.Monad.ST
import Data.Char

histo :: B.ByteString -> (Int,Int,Int)
histo bs =
    let vec = create ( do
                mvec <- V.replicate 256 0
                forM [0 .. B.length bs - 1] $ \i -> do
                    let c = fromIntegral (B.index i bs) :: Int
                    n <- unsafeRead mvec c
                    unsafeWrite mvec c (n+1)
                return mvec
                )
        f :: (Int, Int, Int) -> Int -> Word8 -> (Int, Int, Int)
        f (mn,mx,cnt) i w =
            let mn'  = if w > 0 && w < unsafeIndex vec mn then i else mn
                mx'  = if          w > unsafeIndex vec mx then i else mx
                cnt' = if isPrint (chr . fromIntegral $ w) then cnt+1 else cnt
            in (mn', mx',cnt')

     in V.foldl' f (32, 32,0) vec -- 32 = index for ' '

This should calculate the result in constant memory (though the fold should probably be made more strict using a strict 3-tuple but all three values are forced each time f is called anyway). I haven't tested this code because I don't currently have access to a compiler so there are probably missing imports and some type errors but I hope the gist is clear.

I think this is a good demonstration of when some mutation is useful (counting lots of elements) and when functional algorithms are clearer (folding over all the data). The counting could have been done using a fold too but I think it's easier to see that here it is definitely using a constant amount of memory for the counts and should also perform quite well.


Group by consecutive values in first column and second column value I have a typing log in which rows are grouped by consecutive package_name, and within those groups rows are grouped if they are logged within 180,000 ms from each other. This is working but is there a cleaner way to achieve it? An edge case would be when more than 1000 consecutive episodes are logged for the same package_name (though I think it is unlikely).

library(dplyr)

metrics <- data.frame(timestamp = c(seq(100000, 100200, by=100), seq(200000, 200200, by=100), seq(400300, 400304, by=1), seq(600400, 600600, by=100), seq(800700, 800900, by=100)),
                      package_name = c(rep("package1", 3), rep("package2", 3), rep("package2", 5), rep("package2", 3), rep("package1", 3)))

metrics <-  metrics %>%
     mutate(typing_episode = ifelse(package_name != lag(package_name), 1, 0),
     typing_episode = ifelse(is.na(typing_episode), 0, typing_episode),
     typing_episode = cumsum(typing_episode) + 1) %>%
     group_by(typing_episode) %>%
     mutate(time_diff2 = timestamp - lag(timestamp),
             time_diff2 = ifelse(is.na(time_diff2), 0, time_diff2),
             second_group = ifelse(time_diff2 > (1000 * 60 * 3), 1, 0),
             second_group = cumsum(second_group) + 1,
             typing_episode2 = typing_episode * 1000 + second_group)

print(metrics)

In your code, you are checking for changes in a vector or for the differences between consecutive elements of a vector by using lag and then cleaning up the introduced NA value. When looking for changes, I would find it cleaner to handle the first element separately, which enables you to do the operation in a single line of code. For differences in timestamp, diff would make everything a lot cleaner:

metrics <- metrics %>%
  mutate(typing_episode = cumsum(c(1, head(package_name, -1) != tail(package_name, -1)))) %>%
  group_by(typing_episode) %>%
  mutate(second_group = cumsum(c(1, diff(timestamp) > 1000 * 60 * 3)),
         typing_episode2 = typing_episode * 1000 + second_group)
print(metrics)

As you note, typing_episode2 might have repeats if second_group can exceed 1000. A reasonable alternative might be to do something like typing_episode2 = paste0(typing_episode, "_", second_group). Then you won't need to worry about non-unique typing_episode2 values.


Walking and comparing contents of binary trees I've written a solution for http://tour.golang.org/concurrency/8, which asks for a Walk() function to traverse a randomly generated binary tree, and a Same() function to check if two trees have the same values.

Is this solution is the most efficient? (I think that the Same function is not the most efficient solution, but I don't know how to make it better.)

package main

import "code.google.com/p/go-tour/tree"
import "fmt"

// Walk walks the tree t sending all values
// from the tree to the channel ch.
func Walk(t *tree.Tree, ch chan int) {
    if t == nil {
        return
    }
    Walk(t.Left, ch)
    ch <- t.Value
    Walk(t.Right, ch)
}

// Same determines whether the trees
// t1 and t2 contain the same values.
func Same(t1, t2 *tree.Tree) bool {
    c1 := make(chan int)
    c2 := make(chan int)
    go Walk(t1,c1)
    go Walk(t2,c2)
    for z:=0;z<10;z++ {
        if <-c1 != <-c2 {
            return false
        }
    }
    return true
}

func main() {
    ch := make(chan int)
    go Walk(tree.New(1), ch)
    for z:= 0; z<10; z++ {
        fmt.Println(<-ch)
    }
    fmt.Println(Same(tree.New(1), tree.New(1)) == true)
    fmt.Println(Same(tree.New(1), tree.New(2)) == false)
}

In my opinion, you haven't completed the exercise properly, since you have hard-coded the termination after reading 10 values. If a rogue tree.New() produces a larger tree, you should be able to detect it.


Saving a subscription plan with a payment Is this acceptable for readability or would it be better to refactor it into smaller methods?

Tips?  Comments? Examples?

def save_with_payment
  if valid?
    charge = Stripe::Charge.create(:amount => (self.plan.price * 100).to_i,
                                   :currency => 'usd', :card => stripe_card_token, :description => "Charge for #{user.email}")
    self.stripe_charge_id = charge.id
    self.starts_at = Date.today
    self.amount = plan.price
    self.expires_at = 1.years.from_now
    save!
  end
rescue Stripe::InvalidRequestError => e
  logger.error "Stripe error while creating charge: #{e.message}"
  errors.add :base, "There was a problem with your credit card."
  false
end

Some notes about your code:

Indentation of a call with multi-line hash: That's subjective, but this style you use wastes a lot of space, and it's pretty hard to read (because of the longer lines). If the hash is long I prefer to use the one-line-per-key/value JSON style. More on this here.
self.stripe_charge_id = charge.id: When you have a lot of sets + save! it is better just to use update_attributes!.
save!: The method is named save_with_payment, in a ActiveRecord context that means this method won't raise an exception, so you should call save.
if valid?: No else branch? The method seems to return a boolean so returning false in that case would be more consistent.
rescue Stripe::InvalidRequestError => e: That's a long and subjective topic. My opinion: don't wrap a whole method that is doing lots of things with a rescue, the logic forms now some kind of spaghetti. Wrap the specific parts of the code that may raise that particular exception.
Stripe::Charge.create: You asked if the code is too complex. I don't think so, at least not compared with typical Ruby practices, but it's probably more orthodox to create a separate method for this call.
errors.add :base,: I don't like this mixing of calls with parens and calls without, it looks messy. DSL-style code in Rails without parens -> ok, normal code in methods -> not so sure, I'd write them. Or at least be consistent.
self.plan: It's not idiomatic to write explicit self. to call instance methods.

I'd write:

def save_with_payment
  if !valid?
    false
  elsif !(charge = create_stripe_charge)
    errors.add(:base, "There was a problem with your credit card.")
    false
  else
    update_attributes({
      :stripe_charge_id => charge.id,
      :starts_at => Date.today,
      :amount => plan.price,
      :expires_at => 1.years.from_now,
    })
  end
end

def create_stripe_charge
  Stripe::Charge.create({
    :amount => (plan.price * 100).to_i,
    :currency => 'usd',
    :card => stripe_card_token,
    :description => "Charge for #{user.email}",
  })
rescue Stripe::InvalidRequestError => e
  logger.error("Stripe error while creating charge: #{e.message}")
  nil
end


Dynamic CSS insertion I have a conf.inc.php file which has the following few lines.

// enable css using convention below:
// $css["pageName"] = ["cssName1", "cssName2", ...]
$css["all"]         = ["style"];
$css["myPage"]      = ["someStyle"];
$css["myOtherPage"] = ["otherStyle", "anotherStyle"];

// disable css
$exCss["myOtherPage"] = ["style"];

What this will do is give all of my pages the style.css. Then all the other keys of the index will give their respective page (e.g. "myPage.php" etc.) the styles listed in the array after the equals (e.g. "someStyle.css").

All of the styles are stored in a folder called "css" and there is no other place to store them as I have hard coded it in - this meant I didn't have to write relative paths in the multidimensional array.

The second array will exclude some styles from their respective page. In this instance the style called style.css is being excluded from the page named myOtherPage.php.

Note: The page names are from a function here. So they only read the file name and ignore extensions etc. This means that "myOtherPage" could also be "myOtherPage.php" or "myOtherPage.html". Although if you look at the following code they will have to be PHP see I use an include in my header of the PHP files.

// print css that are present in all pages
$a = (isset($css["all"])) ? count($css["all"]) : 0;
for ($i = 0; $i < $a; $i++)
    if ((isset($exCss["all"]) && !in_array($css["all"][$i], $exCss["all"])) || (isset($exCss["$page"]) && !in_array($css["all"][$i], $exCss["$page"])) || (!isset($exCss["all"]) || !isset($exCss["$page"])))
        echo (file_exists("{$css["all"][$i]}.css")) ? "<link rel=\"stylesheet\" type=\"text/css\" href=\"{$css["all"][$i]}.css\" />" : "";

This code is part of header.inc.php and this gets placed inside the <head> tags. This will add all the styles from the array providing that style is not listed in the $exCss array.

It is common for me to make things more difficult than they need to be...
This could be another case of that. I also tend to make functions or processes that already exist.

Anyway, I thought I would share this little snippet. If anyone has anyway of improving it then I would love to hear it. Thanks as usual!

I'm not sure why you're handling so many CSS files. If you have one CSS file for your application, which is minified and cached on the user's device, you can reduce HTTP requests and overall loading time.

Anyway. If you need to handle lots of these files and keep this approach, you could simplify your code in terms of readability.

Create an array of default CSS files:

$css = ['a', 'b', 'c'];

Create an array where you store the individual files for each page:

$pageCSS = [
    'page-a' => [
        'add' => ['d', 'e', 'f'],
        'remove' => [],
    ],
    'page-b' => [
        'add' => ['g', 'h', 'i'],
        'remove' => ['a', 'b'],
    ],
];

Now you need to test whether a requested page is in this array. If the key exists, add all new files and remove not-needed default files using array_merge and array_diff:

$page = 'page-a';

if (array_key_exists($page, $pageCSS)) {
    $css = array_diff(array_merge($css, $pageCSS[$page]['add']), $pageCSS[$page]['remove']);
}

Finally create all HTML elements. I find it easier to read, if you use single-quotes. This way you don't need to escape all double quotes with backslashes:

foreach($css as $file) {
    print '<link rel="stylesheet" type="text/css" href="' . $file . '.css">';
}


Gomoku board in JavaScript EDIT: The result looks like this: http://gomokuu.glitch.me/

This is my code for gomoku. I made it with JavaScript and HTML. I think the checkWinner function can be improved or made shorter. Also, should the functions be re-organized?

Here is the code:

Javascript:

var turn = 0;
var width = 15;
var height = 15;
var gameOver = false;
var board = Array(width * height);

function announceWinner(winner) {
  console.log("a");
  if (winner == 0) {
    window.alert("X wins");
  }
  if (winner == 1) {
    window.alert("O wins");
  }
  if (winner == 2) {
    window.alert("Board filled");
  }
}

function checkWinner() {
  var filled = true;
  for (var i = 0; i < board.length; i++) {
    if (board[i] == undefined) filled = false;
    if (board[i] !== undefined) {
      if (
        (board[i] == board[i + 1] &&
          board[i + 1] == board[i + 2] &&
          board[i + 2] == board[i + 3] &&
          board[i + 3] == board[i + 4]) ||
        (board[i] == board[i + width] &&
          board[i + width] == board[i + 2 * width] &&
          board[i + 2 * width] == board[i + 3 * width] &&
          board[i + 3 * width] == board[i + 4 * width]) ||
        (board[i] == board[i + 1 + width] &&
          board[i + 1 + width] == board[i + 2 + 2 * width] &&
          board[i + 2 + 2 * width] == board[i + 3 + 3 * width] &&
          board[i + 3 + 3 * width] == board[i + 4 + 4 * width]) ||
        (board[i] == board[i - 1 + width] &&
          board[i - 1 + width] == board[i - 2 + 2 * width] &&
          board[i - 2 + 2 * width] == board[i - 3 + 3 * width] &&
          board[i - 3 + 3 * width] == board[i - 4 + 4 * width])
      ) {
        gameOver = true;
        announceWinner(board[i]);
      }
    }
  }
  if (filled) {
    announceWinner(2);
  }
}

function tileClick(row, tile) {
  var clicked = document.getElementById("board").children[row].children[tile];
  if (clicked.innerHTML || gameOver) return;
  board[tile + row * width] = turn;
  if (turn) {
    clicked.innerHTML = "o";
    clicked.style.color = "red";
    turn = 0;
  } else {
    clicked.innerHTML = "x";
    clicked.style.color = "blue";
    turn = 1;
  }
  checkWinner();
}

//generate board and listen to click event
var domBoard = document.createElement("table");
domBoard.id = "board";
for (let i = 0; i < height; i++) {
  var row = document.createElement("tr");
  for (let j = 0; j < width; j++) {
    var tile = document.createElement("td");
    tile.onclick = function() {
      tileClick(i, j);
    };
    row.appendChild(tile);
  }
  domBoard.appendChild(row);
}
document.body.appendChild(domBoard);

HTML:

<!DOCTYPE html>
<html>
  <head>
    <title>gomoku</title>
    <link rel="stylesheet" href="style.css" />
  </head>
  <body>
    <script src="script.js"></script>
  </body>
</html>

I don't have time to give you a full review, so I'll just add some comments about checkWinner:

It will read better if you remove the mutation of global state from it.  Make it a pure function that returns true or false.
You might consider using a 2D array as your data structure.  This isn't necessary, but it maps more naturally to the visual board, and will likely clean up some of your logic.

As for the duplication, you can remove a lot of it by introducing a helper function to capture the abstract logic that's being repeated in every block.  This will also clarify the high-level logic of checking for a winner:

function checkWinner() {
  for (var i = 0; i < board.length; i++) {
    var rightWin = winAt(i, 1)
    var downWin = winAt(i, width)
    if (rightWin || downWin) return true
  }
  return false
}

function winAt(i, step) {
  if ( board[i] === undefined ) return false
  for ( var j = 1; j < 5; j++) {
    var nextIndex = i + (j * step)
    if ( board[i] !== board[nextIndex] ) return false
  }
  return true
}

EDIT: Answer to question

Green Ball, regarding the problem you mentioned with using a 2D array, there are ways around that: you can pad with extra rows of undefined, or you can abstract a cell(i, j) which hides the ugly bounds checking, and keeps it in one place.  Indeed, that last point is the key one, not whether you use a 1D or 2D array.

In your current implementation, the arithmetic logic around rows is forced out on the client: any code that wants to get an i, j pair must do the calculation itself.  Currently happens in checkWinner and tileClick: board[tile + row * width].  It also happens implicitly in the double loop that constructs the html.

Regardless of the underlying structure that holds the board, you want your client code to be able to work with the concepts of "rows" and "columns" as first class citizens, since that's how we humans are thinking about this.  It will make the code more natural.


Routine to convert decimal digits to Densely Packed Decimal I've written an amd64 assembly routine (in gas syntax using the System V calling convention) to convert three decimal digits to Densely packed decimal because I wasn't satisfied by the performance of the assembly generated by gcc and clang for all implementations of the conversion I could think of. Here is the code:

    .section .text
    .type decimal2dpd,@function
    .globl decimal2dpd

    # convert three digits abc to DPD
    # SysV calling convention: a in edi, b in esi, c in edx
    # assumes 0 <= a, b, c < 10
    .align 8
decimal2dpd:
    mov %esi,%eax
    shl $4,%eax
    or %edx,%eax
    and $0021,%eax
    shl $7,%edi
    or %edi,%eax
    and $016,%edx
    btr $10,%eax

    # at this point, eax = (a & 7) << 4 | (b & 1) << 4 | (c & 1) and CF is set if a > 7
    # and edi is free for use
    jc .Lagt7

    # here a < 8
    shl $4,%esi
    btr $7,%esi
    jc .Lbgt7

    # here a < 8 and b < 8
    or %esi,%eax
    or %edx,%eax
    ret

    .align 8
.Lbgt7: # here a < 8 and b > 7
    or $0012,%eax
    shl $4,%edx
    mov $0104,%edi
    btr $7,%edx
    cmovc %edi,%edx
    or %edx,%eax
    ret

    .align 8
.Lagt7: # here a > 7
    shl $7,%edx # eax = (a & 1 | c & 6) << 7 | (b & 1) << 5 | (c & 1)
    or %edx,%eax    # now eax[10] is set if c > 7
    btr $10,%eax
    jc .Lcgt7

    # here a > 7 and c < 8
    or $0014,%eax
    shl $4,%esi
    mov $0002,%edi
    btr $7,%esi
    cmovc %edi,%esi
    or %esi,%eax
    ret

    .align 8
.Lcgt7: # here a > 7 and c > 7
    and $016,%esi
    or $0056,%eax
    shl $7,%esi
    mov $0100,%edi
    btr $10,%esi
    cmovc %edi,%esi
    or %esi,%eax
    ret
    .size decimal2dpd,.-decimal2dpd

This routine is roughly 10% faster than the C code implementing roughly the same algorithm shown below. I would like to know if there is any obvious or less obvious way to improve this routine for better performance.

extern unsigned
decimal2dpd(unsigned a, unsigned b, unsigned c)
{
    unsigned result = c & 1 | (a & 7) << 7;

    if (a < 8) {
        if (b < 8)
            return result | b << 4 | c;
        else
            return result | (c > 7 ? 0104 : (c & 6) << 4) | (b & 1) << 4 | 0012;
    } else {
        result |= (b & 1) << 4;

        if (c < 8)
            return result | (c & 6) << 7 | (b > 7 ? 0002 : b << 4) | 0014;
        else
            return result | (b > 7 ? 0100 : (b & 6) << 7) | 0056;
    }
}

I especially believe that the fact that all arguments are in the range 0–9 can be exploited further, I didn't found a way to do so though (except for a).

Using a tiny (24 byte) lookup table and some multiplication trickery, I was able to make the following implementation (roughly 1.5 times as fast as the original):

        .section .text
        .type bcd2dpd_mul,@function
        .globl bcd2dpd_mul

        # convert BCD to DPD with multiplication tricks
        # input abcd efgh iklm in edi
        .align 8
bcd2dpd_mul:
        mov %edi,%eax           #   = 0000 abcd efgh iklm
        shl %al                 #   = 0000 abcd fghi klm0
        shr %eax                #   = 0000 0abc dfgh iklm
        test $0x880,%edi        # fast path for a = e = 0
        jz 1f

        and $0x888,%edi         #   = 0000 a000 e000 i000
        imul $0x0490,%di        #   = aei0 0000 0000 0000
        mov %eax,%esi
        and $0x66,%esi          # q = 0000 0000 0fg0 0kl0
        shr $13,%edi            # u = 0000 0000 0000 0aei
        imul tab-8(,%rdi,4),%si # v = q * tab[u-2][0]
        and $0x397,%eax         # r = 0000 00bc d00h 0klm
        xor %esi,%eax           # w = r ^ v
        or tab-6(,%rdi,4),%ax   # x = w | tab[u-2][1]
        and $0x3ff,%eax         #   = 0000 00xx xxxx xxxx
1:      ret

        .size bcd2dpd_mul,.-bcd2dpd_mul

        .section .rodata
        .align 4
tab:
        .short 0x0011 ; .short 0x000a
        .short 0x0000 ; .short 0x004e
        .short 0x0081 ; .short 0x000c
        .short 0x0008 ; .short 0x002e
        .short 0x0081 ; .short 0x000e
        .short 0x0000 ; .short 0x006e
        .size tab,.-tab

which was optimized further with help from the folks on Stackoverflow.


Finding unconnected sets I have a list of boxes, and wish to group them into unconnected sets of overlapping boxes. (Note that two boxes A and B may not overlap each other, but if they are both overlapped by a box C, they will still be grouped together).

The following works ok, but I feel like it could be made a lot more concise with LINQ, or possibly made faster.

private List<List<Box>> FindUnconnectedSets(List<Box> boxes)
{
    List<List<Box>> unconnectedSets = new List<List<Box>>();

    while (boxes.Count() != 0)
    {
        List<Box> set = new List<Box>();

        bool found = true;
        while (found)
        {
            found = false;

            foreach (Box newBox in boxes)
            {
                if (set.Count != 0)
                {
                    foreach (Box setBox in set)
                    {
                        if (newBox.Overlaps(setBox))
                        {
                            set.Add(newBox);
                            boxes.Remove(newBox);
                            found = true;
                            break;
                        }
                    }
                }
                else
                {
                    set.Add(newBox);
                    boxes.Remove(newBox);
                    found = true;
                }

                if (found)
                    break;
            }
        }

        unconnectedSets.Add(set);
    }

    return unconnectedSets;
}

Edit: added the Box class. It's not all that interesting really.

private class Box
{
    public Box(int xOrigin, int yOrigin, int size)
    {
        XOrigin = xOrigin;
        YOrigin = yOrigin;

        Size = size;
    }

    public int XOrigin;
    public int YOrigin;

    public int Size;

    public override string ToString()
    {
        string s = "Box: "
            + "(" + XOrigin.ToString() + ", " + YOrigin.ToString() + ")"
            + " " + Size.ToString();
        return s;
    }

    public bool Overlaps(Box b2)
    {
        if (XOrigin >= b2.XOrigin + b2.Size)
            return false;
        if (b2.XOrigin >= XOrigin + Size)
            return false;
        if (YOrigin >= b2.YOrigin + b2.Size)
            return false;
        if (b2.YOrigin >= YOrigin + Size)
            return false;

        return true;
    }
}

Comments welcome, both on the LINQifying side, and algorithm suggestions if you think I'm going about this the wrong way, as well as any other feedback. :)

Since we are talking about sets, it's probably better to use collection types that represent a set: HashSet<T> and ISet<T>.

Another improvement you can make is to keep track of the boxes you've added to current set at the last iteration (horizon of your expansion), and try to match candidate boxes only with them.

Note that resulting solution doesn't use the knowledge about box matching logic, so it might be a good idea to extract the interface that defines the connection between elements (i.e. a method that checks for the presence of link between nodes).

As a result I've got the following solution:

public interface IConnected<in T>
{
    bool IsConnectedTo(T other);
}

public class Box: IConnected<Box>
{
    ....

    public bool IsConnectedTo(Box other)
    {
        return Overlaps(other);
    }
}

And the FindUnconnectedSets:

public static List<ISet<T>> FindUnconnectedSets<T>(IEnumerable<T> boxes) where T : IConnected<T>
{
    var remainingBoxes = new HashSet<T>(boxes);
    var unconnectedSets = new List<ISet<T>>();

    while (remainingBoxes.Count > 0)
    {
        var newBoxes = new[] { remainingBoxes.First() }; //on each iteration will contain newly added boxes to current set
        var currentSet = new HashSet<T>();
        unconnectedSets.Add(currentSet);

        do
        {
            currentSet.UnionWith(newBoxes);
            remainingBoxes.ExceptWith(newBoxes);
            newBoxes = remainingBoxes
                .Where(remainingBox => newBoxes.Any(setBox => setBox.IsConnectedTo(remainingBox)))
                .ToArray();
        } while (newBoxes.Length > 0);
    }

    return unconnectedSets;
}