Out of N number of participants, draw out M winners It could be a simple Fisher-Yates, but then if the number of participants is 1,000,000 and the number of winner is only 3, then we really don't need to shuffle the whole 1,000,000 numbers, so the following is to reduce the number of steps, written as a simpler case of drawing 9 winners out of 30 participants:
The following is to consider 30 participants, each numbered from 1 to 30, and take out 9 winners.  One will be Grand Prize, and two will be Second Prizes, and 6 Third Prizes.

const NUM_PARTICIPANTS = 30;
const NUM_DRAWN = 9;

console.log(`run: ${NUM_PARTICIPANTS} draw ${NUM_DRAWN}`);

const arr = Array.from(new Array(NUM_PARTICIPANTS), (e, i) => i + 1);

for (let i = arr.length - 1; i >= NUM_PARTICIPANTS - NUM_DRAWN; --i) {
  const j = Math.floor(Math.random() * (i + 1));
  [arr[i], arr[j]] = [arr[j], arr[i]];
}

const results = arr.slice(NUM_PARTICIPANTS - NUM_DRAWN)

console.log(`Grand prize: ${results[0]}`);
console.log(`Second prize: ${results.slice(1,3).sort((a, b) => a - b).join(" ")}`);
console.log(`Other winners:\n  ${results.slice(3).sort((a, b) => a - b).join("\n  ")}`);

To pick m from a collection of size n, there are at least a couple of approaches:

Randomly shuffle an array of numbers (O(n)), then pick the top few (O(m)), resulting in an overall complexity of O(n + m). This is what you're doing currently, though it requires creating an array.

Repeatedly pick a random number from 0 to n. If a duplicate number is picked, pick again. Repeat m times. This has an overall complexity of around O(m * C), where C is the average number of picks before finding a non-duplicate. It'll range from close to 1 (when n is significantly larger than m) to infinity (when n is equal to m).

If m is close to n, the first approach, that you're doing currently, is quickest, because the other approach results in lots of collisions. If m is significantly smaller than n, the second approach is quickest, because the collision chance C is low. So, where's the break-even point? It'll depend on the engine running the code, but we can do some basic performance tests. I'll use Blindman67's code since it's pretty much exactly what I had in mind.
The below tests aren't 100% reliable, but they'll give you an OK idea of the time required:

// Array sort method:

// Wait for the iframe to load completely
setTimeout(() => {

  const NUM_PARTICIPANTS = 1e7;
  const NUM_DRAWN = 3e6;

  const t0 = performance.now();
  // Make the array 0-indexed rather than 1-indexed
  // to avoid unnecessary addition for every element
  const arr = Array.from({ length: NUM_PARTICIPANTS }, (_, i) => i);
  const lowerLimit = NUM_PARTICIPANTS - NUM_DRAWN;
  for (let i = arr.length - 1; i >= lowerLimit; --i) {
    const j = Math.floor(Math.random() * (i + 1));
    // Use a temp variable instead of the destructuring trick, it's probably a bit faster:
    const temp = arr[i];
    arr[i] = arr[j];
    arr[j] = temp;
  }
  // Then slice the array and calculate the results
  console.log('Time taken:', performance.now() - t0);
}, 1000);

// Picking random numbers until enough non-duplicates are found:

// Wait for the iframe to load completely
setTimeout(() => {

  const NUM_PARTICIPANTS = 1e7;
  const NUM_DRAWN = 3e6;

  const randomInt = range => Math.floor(Math.random() * range);
  const results = createWinners();

  function createWinners() {
       const t0 = performance.now();
       var draw = randomInt(NUM_PARTICIPANTS);
       const winners = new Set([draw]);
       while (winners.size < NUM_DRAWN) {
           do {
               draw = randomInt(NUM_PARTICIPANTS);
           } while (winners.has(draw));
           winners.add(draw);
       }
       // Then slice the array and calculate the results
       console.log('Time taken:', performance.now() - t0);
       return [...winners.values()];
  }
}, 1000);

I tried a few different combinations of NUM_PARTICIPANTS and NUM_DRAWN numbers, and the ratio used above, 10 to 3, seems to be pretty close to the break-even point, when the numbers are large, at least on V8. (If the code runs in a different environment, the ideal ratio to use will likely be different.) At that point, both methods look to take a pretty similar amount of runtime. If you don't have a large number of participants, either approach will work just fine, since it won't take any noticeable amount of time to run the script regardless. But if you have a large number, check the ratio of participants to winners, then choose the appropriate method depending on the ratio.
const createWinners = (numParticipants, numWinners) => (
  numWinners / numParticipants < 0.3
    ? createWinnersWithSetPicking(numParticipants, numWinners)
    : createWinnersWithArraySort(numParticipants, numWinners)
);

If you change the code, you might want to run a performance test again to see what works better, and you might want to adjust the magic ratio. Take into consideration the environment the code runs (or is most likely to run) on too - Node? Chrome? Firefox? Safari?
That said, if your numbers are large enough that performance is an issue, this should almost certainly be done on the server (in Node), not on the client, so as not to be unrunnable on low-end devices.


Multiple Ajax Requests per MVC 4 View I'm using the repository pattern with a context and ninject as the IOC. I have a service which handles getting and setting page properties in the database.

public class MyContext : DbContext
{
    public MyContext() : base ("DefaultConnection")
    {
    }

    public DbSet<PageProperty> PageProperties { get; set; }
    public DbSet<Contact> Contacts { get; set; }
}

public class DefaultRepository : IRepository
{
    MyContext _context;
    public DefaultRepository(MyContext context)
    {
        _context = context;
    }

    public IQueryable<PageProperty> PageProperties { get { return _context.PageProperties; } }
    public IQueryable<Contact> Contacts { get { return _context.Contacts; } }
}

public class ModuleLoader : NinjectModule
{
    public ModuleLoader()
    {

    }

    public override void Load()
    {
        var context = new MyContext();
        context.Database.Initialize(false);
        Bind<MyContext>().ToConstant(context).InSingletonScope();
        Bind<IRepository>().To<DefaultRepository>();
        Bind<IPagePropertyProvider>().To<DefaultPagePropertyProvider>().InSingletonScope();
    }
}

public class DefaultPagePropertyProvider : IPagePropertyProvider
{
    IRepository _repository;
    object _syncLock = new object();

    public DefaultPagePropertyProvider (IRepository repository)
    {
        _repository = repository;
    }

    public string GetValue(string pageName, string propertyName
    {
        lock (_syncLock)
        {
            var prop = page.PageProperties.FirstOrDefault(x => x.Property.Equals(propertyName) && x.PageName.Equals(pageName)).Value;

            return prop;
        }
     }
     public void SetValue(string pageName, string propertyName, string value)
     {
         var pageProp = _repository.PageProperties.FirstOrDefault(x => x.Property.Equals(propertyName) && x.PageName.Equals(pageName));
         pageProp.Value = value;
         _repository.SaveSingleEntity(pageProp);
     }
}

In my view I am doing 3 ajax calls, one to get a list from contacts to fill out a table, one ajax call to determine how many pages i have depending on the page size I'm using, and one ajax call to set the page size that I want to use. so a select box changes the page size (How many contacts per page: [  30]) , a table that displays the contacts (generated from jquery which decifers json), and finally a div containing a list of page numbers to click. The workflow is, call GetContacts(), this function then queries the PageProperties to find out the page size to use, then call GetPages(), this function also queries PageProperties to find out what page size to use, SetPageSize() which sets the page size. So GetContacts() and GetPages() is used when a page is selected from the div, SetPageSize() then GetContacts() and GetPages() is called when the select box change event is fired. GetContacts() and GetPages() is only called when the first SetPageSize() $.ajax request is done() and there is a success from that function.

Now, before I added lock(syncLock) in the DefaultPageProperty service and before I added InSingletonScope to both that service and the context, I was getting two errors.

  The connection was not closed. The connection's current state is connecting.

  An EdmType cannot be mapped to CLR classes multiple times

I assumed because the connection was in a connecting state, that the context was being reused and reused and reused, so I thought putting that to SingletonScope() would mean that only one connection was made, then I thought the same about DefaultPageProperty and then because I was making async calls to that service, I should put a lock over the database querying.

It works, and the problems don't exist. But I don't know if what I have done is correct within the pattern I'm using, I'm wondering if I've missed something fundamental? My question is, is this a proper/viable solution which won't create any caveats later down the road? Have I actually solved the issue or just created more?

I don't think I completely know what is going on in your code.

but there is one thing that I think you are doing wrong.

you are creating a connection over the web and trying to force it to stay open.

your web application should

Connect
Send or receive the information that it needs
Disconnect

you should never leave a connection open for extended periods of time.

every time you need something from the server, open a new connection, when you are finished close the connection.

I may be way out of line, but that struck me as off about the way you explained that part of your application


Twitch client that notifies me when a followed streamer comes online I've been writing code for a little while but I'm a little slapdash. I'd like to get better so I've been having a glance at best practices, other peoples code and reading a whole lot. I'd appreciate it if you could read through my code and make some suggestions and explain why. You'll find some of my code below.

I want to start writing more readable code, follow best practices and just be awesome. Suggested reading would also be appreciated.

import requests
import json
import winsound
import time
import sys

USERNAME = "lirik"
FIRST_RUN = True
VERSION_NO = 1.0

def main():

    print "%s [+] Twitch Notifier %s started..." % (time.strftime("%H:%M:%S"), VERSION_NO)
    print "%s [+] Loading follows from %s" % (time.strftime("%H:%M:%S"), USERNAME)
    try:
        twitch_data = requests.get("https://api.twitch.tv/kraken/users/%s/follows/channels" % USERNAME)
    except:
        print "%s [+] Failed to contacted Twitch API" % time.strftime("%H:%M:%S")
        sys.exit(1)

    user_info = json.loads(twitch_data.text)  # load user data json into user_info
    users_online = list()  # initialise user_online list

    while True:
        for channel in user_info['follows']:  # Loop through channels in the users follow list
            stream_name = channel['channel']['name']  # Add channel name to var stream_name for simplicity
            try:
                stream_data = requests.get("https://api.twitch.tv/kraken/streams/%s" % stream_name)
            except:
                print stream_data.status_code
                print stream_data.text
                continue

            try:
                stream_info = json.loads(stream_data.text)
            except ValueError, e:
                print e
                continue

            if stream_info['stream']:
                if stream_name not in users_online:
                    print "%s [+] Online: %s [%s][%s]" % (time.strftime("%H:%M:%S"),
                                                          stream_name,
                                                          stream_info['stream']['game'],
                                                          stream_info['stream']['viewers'])
                    users_online.append(stream_name)
                    winsound.Beep(1000, 1000)
            else:
                if stream_name in users_online:
                    print "%s [+]Checking if %s is offline." % (time.strftime("%H:%M:%S"),
                                                                stream_name)
                    if chk_user_online(stream_name) is False:  #Double check if user is offline. Sometimes the twitch API fucks up.
                        print "%s [-] Offline: %s" % (time.strftime("%H:%M:%S"),
                                                      stream_name)
                        users_online.remove(stream_name)
                else:
                    pass
                    #print "[*] No change in users online."

        if len(users_online) < 1 and first_run:
            first_run = False
            print "%s [*] There are currently no users online." % time.strftime("%H:%M:%S")

        time.sleep(180)

def chk_user_online(streamer):

    try:
        stream_data = requests.get("https://api.twitch.tv/kraken/streams/%s" % streamer)
        stream_info = json.loads(stream_data.text)
    except:
        print "Stream check failed."
        return Exception("Failed to access stream data")

    if stream_info['stream'] is None:
        return False
    else:
        return True

if __name__ == "__main__":
    main()

It's good that you use absolute imports instead of wildcard imports.

VERSION_NO = 1.0

While it is usually correct to use ALLCAPS for constants, the version number should be called __version__.  That way, the help() function will know which version the program is and will display it accordingly.

Using % for format strings is not officially deprecated, but it is recommended to use .format() instead since it is more flexible.

When you have a try: block, you presumable know what errors could possibly happen.  You should list those errors in your except: statement.  That way, if one comes up that you didn't expect, you will know about it.  You might want to have two except: blocks: one for the errors you know about, and one for all the rest.  The second might be except Exception as e: print e, "Something has gone wrong.  Please ..." or whatever.

except ValueError, e:

In Python2, you use parentheses around multiple errors that you want to catch, but the syntax used above means to call the error e.  In Python3, the parentheses are not needed, so what that line means is to catch all errors of type ValueError or of type e.  What you do in Python3 is to use except ValueError as e:  That is still possible in Python2, so to be in good practice, you should use it here.

if stream_info['stream'] is None:
    return False
else:
    return True

That's great that you used is None instead of == None.  Many people wouldn't do that.  The problem I have with this code is that if ... is checking if ... is True.  Since you are already creating a boolean, take advantage of it and do it in one line:

return stream_info['stream'] is not None


Handling signout flow There is backend which connects with IdentityServer using WsFederation. The backend is .NET Core project where there is a derived class from AuthenticationHandler.

The issue is when the time of user's token is expired, the user should be signed out. I've implemented this in the following way:

protected override async Task<AuthenticateResult> HandleAuthenticateAsync()
{
    var ticket = await EnsureCookieTicket();
    if (ticket == null)
        return AuthenticateResult.Fail("No ticket.");

    //...

    return AuthenticateResult.Success(new AuthenticationTicket(context.Principal, context.Properties, Options.AuthenticationScheme));
}

private Task<AuthenticationTicket> EnsureCookieTicket()
{
    var cookie = Options.CookieManager.GetRequestCookie(Context, Options.CookieName);
    if (String.IsNullOrEmpty(cookie))
        return null;

    var ticket = Options.TicketDataFormat.Unprotect(cookie, GetTlsTokenBinding());
    if (ticket == null)
        return null;

    var currentUtc = Options.SystemClock.UtcNow;
    var issuedUtc = ticket.Properties.IssuedUtc;
    var expiresUtc = ticket.Properties.ExpiresUtc;

    if (expiresUtc != null && expiresUtc.Value < currentUtc)
    {
        //...

        Context.Items.Add("IsExpired", true);
        return null;
    }
}

protected override async Task<bool> HandleUnauthorizedAsync(ChallengeContext context)
{
    if (Context.Items.ContainsKey("IsExpired")
        && Context.Items["IsExpired"].Equals(true))
    {
        // 1 case
        if (!Request.Path.HasValue ||
            !Request.Path.Value.Equals("/index.html", StringComparison.OrdinalIgnoreCase))
        {
            Context.Response.StatusCode = (int)HttpStatusCode.Forbidden;
            var content = Context.Response;

            using (var responseWriter = new StreamWriter(content.Body, Encoding.UTF8))
            {
                responseWriter.Write("The context has expired and can no longer be used");
            }
        }
        // 2 case
        else
        {
            var signoutContext = new SignOutContext(Options.AuthenticationScheme, context.Properties);
            await SignOutAsync(signoutContext);
        }

        return true;
    }

    // generate return url

    // work with singin, if it is signin message
    // or work with cleanup signout, if it is cleanup signout context
    // or await Options.Events.RedirectToLogin(redirectContext);
}

There are two cases this code serves (// 1 case and // 2 case):

When the user works with the site, suddenly the time is ended and then the backend returns 403. JavaScript on the client machine handles this error and calls Signout as if the user clicks the button Signout himself.
Another case is when the user closes the site tab, the time of the user's token is expired, then the user opens the site again and the signout flow happens.

It works, but I count it as the workaround and not so elegant. What do you think?

This isn't really a bad workaround. In fact, I wouldn't even consider it a workaround. It is now part of the business logic of the application.

The only reason it feels like a workaround is because of your // 1 case and // 2 case comments. Remove those and this is no longer a workaround.

I see the following method:

protected override async Task<AuthenticateResult> HandleAuthenticateAsync()
{
    var ticket = await EnsureCookieTicket();
    if (ticket == null)
        return AuthenticateResult.Fail("No ticket.");

    //...

    return AuthenticateResult.Success(new AuthenticationTicket(context.Principal, context.Properties, Options.AuthenticationScheme));
}

And can't help but wonder what the //... bit is because I see no reason for ticket to be a strong type. This entire method:

private Task<AuthenticationTicket> EnsureCookieTicket()
{
    var cookie = Options.CookieManager.GetRequestCookie(Context, Options.CookieName);
    if (String.IsNullOrEmpty(cookie))
        return null;

    var ticket = Options.TicketDataFormat.Unprotect(cookie, GetTlsTokenBinding());
    if (ticket == null)
        return null;

    var currentUtc = Options.SystemClock.UtcNow;
    var issuedUtc = ticket.Properties.IssuedUtc;
    var expiresUtc = ticket.Properties.ExpiresUtc;

    if (expiresUtc != null && expiresUtc.Value < currentUtc)
    {
        //...

        Context.Items.Add("IsExpired", true);
        return null;
    }
}

Never returns a non-null value either. So I'm guessing the bits you left out are substantially important to understanding why you chose the form you did.

Provided you don't need AuthenticationTicket in the HandleAuthenticateAsync() method, I would change the return result from Task<AuthenticationTicket> to Task<bool>. Then rename it to IsTicketValid().

if (expiresUtc != null && expiresUtc.Value < currentUtc)

You don't use .HasValue to check there, but you use it on Request.Path.HasValue, we should be consistent and do if (expiresUtc.HasValue && expiresUtc.Value < currentUtc).

I don't like any of these method names, and as a result the method actions themselves because it looks like each one of them has too many responsibilities. You omitted certain parts (so this should have been VTC'd probably) but I'd be willing to bet those parts do more than they should. You should try to find a way to break the flow up into smaller methods that are more maintainable.


Excel VBA object with validated bounds for an interval I'm working through a problem I have with Excel VBA. I have two object classes. Class clsLoadStep stores three copies of class clsLoad objects. Class clsLoad simply stores a value and a string stating where that value came from. One clsLoad stores a max value and another a min value. My problem comes to read/write access to the nested clsLoad objects in the clsLoadStep. Currently the properties of the clsLoad objects of clsLoadStep can be changed (via the Get property then the Let property of the clsLoad object) which can result in the Max object storing a value less than the Min object. Is there a way to restrict access to the clsLoad objects in the clsLoadStep object to read-only while still letting the definition of the clsLoad class have both read and write properties? I want to be able to read the properties of the clsLoad objects but not change them.

I have a function in the clsLoadStep object that checks if min > max and swaps the object references if that's the case. The clsLoadStep object still needs to be able to tell if the clsLoad object's values have changed. Is there a recommended way to accomplish this?

For a while I had a "Parent" property of the clsLoad object that pointed to the clsLoadStep object. This allowed the clsLoadStep object to know when the clsLoad object changed (by calling the UpdateMaxMin function of the parent). That was a problem though as mulitple objects could reference the clsLoad object and it would be restricted to one parent.

Class clsLoad

Option Explicit

Private pValue As Double
Private pSource As String

'*****************************************************************
'P R O P E R T I E S
'*****************************************************************

'''''''''''''''''''''''''
' Value property
'''''''''''''''''''''''''
Public Property Get Value() As Double
    Value = pValue
End Property
Public Property Let Value(ByVal Val As Double)
    pValue = Val
End Property

'''''''''''''''''''''''''
' Source property
'''''''''''''''''''''''''
Public Property Get Source() As String
    Source = pSource
End Property
Public Property Let Source(ByVal Val As String)
    pSource = Val
End Property

Class clsLoadStep

Option Explicit

Private pMax As clsLoad
Private pMin As clsLoad
Private pOcc As clsLoad
Private pSource As String

'*****************************************************************
'P R O P E R T I E S
'*****************************************************************

'''''''''''''''''''''''''
' Max property
'''''''''''''''''''''''''
Public Property Get Max() As clsLoad
    Set Max = pMax
    UpdateMaxMin
End Property
Public Property Set Max(ByVal newLoad As clsLoad)
    Set pMax = newLoad
    UpdateMaxMin
End Property

'''''''''''''''''''''''''
' Min property
'''''''''''''''''''''''''
Public Property Get Min() As clsLoad
    Set Min = pMin
    UpdateMaxMin
End Property
Public Property Set Min(ByVal newLoad As clsLoad)
    Set pMin = newLoad
    UpdateMaxMin
End Property

'''''''''''''''''''''''''
' Occur property
'''''''''''''''''''''''''
Public Property Get Occ() As clsLoad
    Set Occ = pOcc
End Property
Public Property Set Occ(ByVal newLoad As clsLoad)
    Set pOcc = newLoad
    UpdateMaxMin
End Property

'''''''''''''''''''''''''
' Source property
'''''''''''''''''''''''''
Public Property Get Source() As String
    Source = pSource
End Property
Public Property Let Source(ByVal Val As String)
    pSource = Val
End Property

'''''''''''''''''''''''''
' UpdateMaxMin sub - Swap max and min objects if max is less than min
'''''''''''''''''''''''''
Private Sub UpdateMaxMin()
    Dim tmpLoad As clsLoad
    If pMax.Value < pMin.Value Then
        Set tmpLoad = pMax
        Set pMax = pMin
        Set pMin = tmpLoad
        Debug.Print "Max less than min. Swapped max and min."
    End If
End Sub

As for the first issue

  Is there a way to restrict access to the clsLoad objects in the
  clsLoadStep object to read-only while still letting the definition of
  the clsLoad class have both read and write properties?

I'd say there's only one way to go: remove all Public Property Get ... As clsLoad and substitute them with a couple of Property Get each, as follows:

'''''''''''''''''''''''''
' Max property
'''''''''''''''''''''''''

'''Public Property Get Max() As clsLoad '<--| remove it and replace with MaxValue and MaxSource
'''    Set Max = pMax
''''    UpdateMaxMin
'''End Property

Public Property Get MaxValue() As Double '<--| this will let the user of 'clsLoadStep' class to get the 'Value' property of 'Max' 'clsLoad' object
    MaxValue = GetValue(pMax)
End Property

Public Property Get MaxSource() As String '<--| this will let the user of 'clsLoadStep' class to get the 'Source' property of the 'Max' 'clsLoad' object
    MaxSource = GetSource(pMax)
End Property

where I used the following helper functions:

'helpers
Function GetValue(p As clsLoad) As Double
    If Not p Is Nothing Then GetValue = p.Value
End Function

Function GetSource(p As clsLoad) As String
    If Not p Is Nothing Then GetSource = p.Source
End Function

of course you have to act similarly for Min and Occ properties

As for the second issue

  The clsLoadStep object still needs to be able to tell if the clsLoad
  object's values have changed

I'd put this functionality inside a helper function like follows:

Sub SetIt(load As clsLoad, newLoad As clsLoad, oldLoad As clsLoad, name As String)
    Set load = newLoad '<--| set the object
    If Not oldLoad Is Nothing Then '<--| if there was a "previous" object
        If oldLoad.Value <> load.Value Then MsgBox name & " changed" '<--| check if value changed
    End If
    Set oldLoad = load '<--| update the "previous" object
End Sub

of course you have to set some new fields:

Private pMaxOld As clsLoad
Private pMinOld As clsLoad
Private pOccOld As clsLoad

and exploit it as follows:

'''''''''''''''''''''''''
' Max property
'''''''''''''''''''''''''
Public Property Set Max(ByVal newLoad As clsLoad)
    SetIt pMax, newLoad, pMaxOld, "Max"
    UpdateMaxMin
End Property

and the likes

Finally the clsLoadStep Class code would be:

Option Explicit

Private pMaxOld As clsLoad
Private pMinOld As clsLoad
Private pOccOld As clsLoad
Private pMax As clsLoad
Private pMin As clsLoad
Private pOcc As clsLoad
Private pSource As String

'*****************************************************************
'P R O P E R T I E S
'*****************************************************************

'''''''''''''''''''''''''
' Max property
'''''''''''''''''''''''''
Public Property Set Max(ByVal newLoad As clsLoad)
    SetIt pMax, newLoad, pMaxOld, "Max"
    UpdateMaxMin
End Property

'''Public Property Get Max() As clsLoad
'''    Set Max = pMax
''''    UpdateMaxMin
'''End Property

Public Property Get MaxValue() As Double
    MaxValue = GetValue(pMax)
End Property

Public Property Get MaxSource() As String
    MaxSource = GetSource(pMax)
End Property

'''''''''''''''''''''''''
' Min property
'''''''''''''''''''''''''
Public Property Set Min(ByVal newLoad As clsLoad)
    SetIt pMin, newLoad, pMinOld, "Min"
    UpdateMaxMin
End Property

'''Public Property Get Min() As clsLoad
'''    Set Min = pMin
'''    UpdateMaxMin
'''End Property

Public Property Get MinValue() As Double
    MinValue = GetValue(pMin)
End Property

Public Property Get MinSource() As String
    MinSource = GetSource(pMin)
End Property

'''''''''''''''''''''''''
' Occur property
'''''''''''''''''''''''''
Public Property Set Occ(ByVal newLoad As clsLoad)
    SetIt pOcc, newLoad, pOccOld, "Occ"
End Property

'''Public Property Get Occ() As clsLoad
'''    Set Occ = pOcc
'''End Property

Public Property Get OccValue() As Double
    OccValue = GetValue(pOcc)
End Property

Public Property Get OccSource() As String
    OccSource = GetSource(pOcc)
End Property

'''''''''''''''''''''''''
' Source property
'''''''''''''''''''''''''
Public Property Get Source() As String
    Source = pSource
End Property
Public Property Let Source(ByVal Val As String)
    pSource = Val
End Property

'''''''''''''''''''''''''
' UpdateMaxMin sub - Swap max and min objects if max is less than min
'''''''''''''''''''''''''
Private Sub UpdateMaxMin()
    Dim tmpLoad As clsLoad
    If pMax Is Nothing Or pMin Is Nothing Then Exit Sub
    If pMax.Value < pMin.Value Then
        Set tmpLoad = pMax
        Set pMax = pMin
        Set pMin = tmpLoad
        Debug.Print "Max less than min. Swapped max and min."
    End If
End Sub

'''''''''''''''''''''''''
'helpers
'''''''''''''''''''''''''
Sub SetIt(load As clsLoad, newLoad As clsLoad, oldLoad As clsLoad, name As String)
    Set load = newLoad
    If Not oldLoad Is Nothing Then
        If oldLoad.Value <> load.Value Then MsgBox name & " changed"
    End If
    Set oldLoad = load
End Sub

Function GetValue(p As clsLoad) As Double
    If Not p Is Nothing Then GetValue = p.Value
End Function

Function GetSource(p As clsLoad) As String
    If Not p Is Nothing Then GetSource = p.Source
End Function

an example of usage of this class is the following:

Option Explicit

Sub main()

    Dim load1 As clsLoad, load2 As clsLoad, load3 As clsLoad
    Dim loadStep As clsLoadStep

    Set load1 = New clsLoad
    Set load2 = New clsLoad
    Set load3 = New clsLoad

    load1.Source = "source1"
    load1.Value = 1

    load2.Source = "source2"
    load2.Value = 2

    load3.Source = "source3"
    load3.Value = 3

    Set loadStep = New clsLoadStep
    With loadStep
        Set .Max = load1
        Set .Min = load3 '<--| this will trigger the "swap" functionality inside 'clsLoadStep' class
        Set .Max = load2 '<--| this will trigger the check against the previous 'Max' 'clsLoad' object of 'LoadStep' class
        Set .Occ = load2
        MsgBox .MaxValue '<--| this will return '2'
        MsgBox .Max.Value '<--| though still allowed by Intellisense, this will not compile
    End With

End Sub


SE Data Explorer Query: Average score for questions and answers, by tag Being curious to see which tags on this site get the most attention, I developed a Stack Exchange Data Explorer query:

WITH CanonicalTags AS (
    SELECT Id, TagName, Id AS AliasId, TagName AS Alias
        FROM Tags
        WHERE NOT EXISTS (
            SELECT Id
                FROM TagSynonyms
                WHERE SourceTagName=TagName AND ApprovedByUserId IS NOT NULL
        )
        UNION
    SELECT Tags.Id, Tags.TagName, Alias.Id AS AliasId, Alias.TagName
        FROM Tags
            INNER JOIN TagSynonyms
                ON TargetTagName=TagName
            INNER JOIN Tags AS Alias
                ON Alias.TagName=SourceTagName
        WHERE ApprovedByUserId IS NOT NULL
), TagQuestionStats AS (
    SELECT Tags.Id AS Id
         , Tags.TagName AS TagName
         , AVG(CAST(Questions.Score AS DECIMAL)) AS AvgScore
         , STDEV(Questions.Score) AS StdDevScore
         , COUNT(Questions.Id) AS N
        FROM CanonicalTags AS Tags
            INNER JOIN PostTags
                ON PostTags.TagId=Tags.AliasId
            INNER JOIN Posts AS Questions
                ON Questions.Id=PostTags.PostId
        GROUP BY Tags.Id, Tags.TagName
), TagAnswerStats AS (
    SELECT Tags.Id AS Id
         , Tags.TagName AS TagName
         , AVG(CAST(Answers.Score AS DECIMAL)) AS AvgScore
         , STDEV(Answers.Score) AS StdDevScore
         , COUNT(Answers.Id) AS N
        FROM CanonicalTags AS Tags
            INNER JOIN PostTags
                ON PostTags.TagId=Tags.AliasId
            INNER JOIN Posts AS Questions
                ON Questions.Id=PostTags.PostId
        INNER JOIN Posts AS Answers
            ON Answers.ParentId=Questions.Id
        WHERE
            Answers.PostTypeId=2
        GROUP BY Tags.Id, Tags.TagName
)
SELECT '<' + QStats.TagName + '>' AS [TagName]
     , ROUND(QStats.AvgScore, 2) AS [Avg Qst Score]
     , ROUND(QStats.StdDevScore, 2) AS [Qst Score Std Dev]
     , QStats.N AS [# Qst]
     , ROUND(AStats.AvgScore, 2) AS [Avg Ans Score]
     , ROUND(AStats.StdDevScore, 2) AS [Ans Score Std Dev]
     , AStats.N AS [# Ans]
     , ROUND(CAST(AStats.N AS DECIMAL) / CAST(QStats.N AS DECIMAL), 2) AS [Avg # Ans/Qst]
    FROM
        TagQuestionStats AS QStats
            LEFT OUTER JOIN TagAnswerStats AS AStats
                ON AStats.Id=QStats.Id
    ORDER BY 5 DESC, 2 DESC, 7, 4, 1;

I'd like to ask:

Some of the numbers don't round to two decimal places very well.  Is that due to a bug in SQL Server, or am I using inappropriate rounding techniques?  (The exact numbers don't matter; I'd much rather have a compact presentation.)
Is there an easier way to canonicalize the tags?
Any comments about my SQL formulation in general?
Any suggestions for making the query itself more meaningful?

Your code is, for the most part, consistently styled, and predictably structured, which is nice. I am not a fan of the comma-on-the-new-line style of continuation. I prefer the comma at the end. But, at least you are consistent with this.

One inconsistency which I see is that for smaller selects you have multiple selected columns on a single line.

SELECT Tags.Id, Tags.TagName, Alias.Id AS AliasId, Alias.TagName
    ....

To be consistent with the other selects, as much as these are short statements, you should maintain consistency:

SELECT Tags.Id
     , Tags.TagName
     , Alias.Id AS AliasId
     , Alias.TagName
   ....

There is only one example of it in your code, but I really dislike multiple conditions on a single line:

WHERE SourceTagName=TagName AND ApprovedByUserId IS NOT NULL

This really should be:

WHERE SourceTagName=TagName
  AND ApprovedByUserId IS NOT NULL

The UNION condition is also very buried in there, and it has too much indentation ;-)

Finally, white-space is cheap in SQL, I really don't like the space-less arg=val expressions, spacing them is easy arg = val.

Right, that's the nit-picky stuff done.

Talking about the UNION, the statement could be rewritten as:

SELECT IsNull(Main.TagName, Alias.TagName) AS TagName
     , IsNull(Main.Id,      Alias.Id     ) AS Id
     , Alias.Id                            AS AliasId
    FROM Tags AS Alias
    LEFT JOIN TagSynonyms
            ON SourceTagName = Alias.TagName
    LEFT JOIN Tags AS Main
            ON TargetTagName = Main.TagName

I prefer the more compact result, although it is debatable as to whether the "Outer" Join is a better solution than the UNION. Outer joins can lead to confusion. So can UNIONS and I prefer the outer join.

In the above query I have removed the Alias TagName since it is not used outside the sub-select.

I also found the final 'assimilation' of all the data in the final select to have too many casts and data conversions. The actual logic was lost in the detail. I moved the complicating CASTS to the previous with clauses.

Finally, the indexed ORDER BY has caught a number of people out in the past, as it is too easy to change columns and suddenly the data ordering is off, even though the query still runs. I always recommend named-column ordering.

Putting this all together, I have the SQL:

WITH CanonicalTags AS (
    SELECT IsNull(Main.TagName, Alias.TagName) AS TagName
         , IsNull(Main.Id,      Alias.Id     ) AS Id
--         , Alias.TagName                       AS AliasName
         , Alias.Id                            AS AliasId
        FROM Tags AS Alias
            LEFT JOIN TagSynonyms
                ON SourceTagName = Alias.TagName
            LEFT JOIN Tags AS Main
                ON TargetTagName = Main.TagName
), TagQuestionStats AS (
    SELECT Tags.Id AS Id
         , Tags.TagName AS TagName
         , CAST(AVG(CAST(Questions.Score AS DECIMAL)) AS DECIMAL(8,2)) AS AvgScore
         , CAST(STDEV(Questions.Score) AS DECIMAL(8,2))  AS StdDevScore
         , CAST(COUNT(Questions.Id) AS DECIMAL(8,2))  AS N
        FROM CanonicalTags AS Tags
            INNER JOIN PostTags
                ON PostTags.TagId=Tags.AliasId
            INNER JOIN Posts AS Questions
                ON Questions.Id=PostTags.PostId
        GROUP BY Tags.Id, Tags.TagName
), TagAnswerStats AS (
    SELECT Tags.Id AS Id
         , Tags.TagName AS TagName
         , CAST(AVG(CAST(Answers.Score AS DECIMAL)) AS DECIMAL(8,2)) AS AvgScore
         , CAST(STDEV(Answers.Score) AS DECIMAL(8,2)) AS StdDevScore
         , CAST(COUNT(Answers.Id) AS DECIMAL(8,2)) AS N
        FROM CanonicalTags AS Tags
            INNER JOIN PostTags
                ON PostTags.TagId=Tags.AliasId
            INNER JOIN Posts AS Questions
                ON Questions.Id=PostTags.PostId
        INNER JOIN Posts AS Answers
            ON Answers.ParentId=Questions.Id
        WHERE
            Answers.PostTypeId=2
        GROUP BY Tags.Id, Tags.TagName
)
SELECT '<' + QStats.TagName + '>' AS [TagName]
     , QStats.AvgScore AS [Avg Qst Score]
     , QStats.StdDevScore AS [Qst Score Std Dev]
     , QStats.N AS [# Qst]
     , AStats.AvgScore AS [Avg Ans Score]
     , AStats.StdDevScore AS [Ans Score Std Dev]
     , AStats.N AS [# Ans]
     , ROUND(AStats.N / QStats.N, 2) AS [Avg # Ans/Qst]
    FROM
        TagQuestionStats AS QStats
            LEFT OUTER JOIN TagAnswerStats AS AStats
                ON AStats.Id=QStats.Id
    ORDER BY [Avg Ans Score] DESC
           , [Avg Qst Score] DESC
           , [# Ans]
           , [# Qst]
           , [TagName];

This query supports this fork of your original question


Toggling buttons within a div This code is used to toggle buttons within a div:

var chosen = {};
var form_name = 'the_div';
function(what) {
   if('number' != typeof what) { // toggle-one
        var el = $(what);
        var name = el.html();
        var ch = ['success','default'];
        if( !chosen[name] ) ch.unshift(ch.pop());
        el.removeClass('btn-' + ch[0]).addClass('btn-' + ch[1]);
        chosen[name] = !chosen[name];
        return;
    }
    var choices = $('#'+form_name).find('div');
    switch(what) { // when toolbar button clicked
        case 1: // select-all
            choices.each(function(idx,el){
                var el = $(el);
                var name = el.html();
                if(!chosen[name]) {
                    el.removeClass('btn-default').addClass('btn-success');
                    chosen[name] = true;
                }
            });
            break;
        case 0: // select-none
            choices.each(function(idx,el){
                var el = $(el);
                var name = el.html();
                if(chosen[name]) {
                    el.removeClass('btn-success').addClass('btn-default');
                    chosen[name] = false;
                }
            });
            break;
        case -1: // select-inverse / toggle-all
            choices.each(function(idx,el){
                var el = $(el);
                var name = el.html();
                var ch = ['success','default'];
                if(!chosen[name]) ch.unshift(ch.pop());
                el.removeClass('btn-'+ch[0]).addClass('btn-'+ch[1]);
                chosen[name] = !chosen[name];
            });
            break;
    }
}

Any suggestion on how to simplify this code?

Here are some of my suggestions:

Declare static things at the top. i.e. var ch = ['success','default']; it is declared twice.
Duplicate code should be pulled into functions (see below).
Be consistent: el.removeClass('btn-default').addClass('btn-success'); and el.removeClass('btn-'+ch[1]).addClass('btn-'+ch[0]); Either make use of ch or get rid of it (I suggest the first).

Duplicate code:

var el = $(el);
var name = el.html();
var ch = ['success','default'];
if(!chosen[name]) ch.unshift(ch.pop());
el.removeClass('btn-'+ch[0]).addClass('btn-'+ch[1]);
chosen[name] = !chosen[name];

Something like:

var ch = ['success','default'],
    toggleElement = function (el) {
        var el = $(el),
            name = el.html();

        if(!chosen[name]) ch.unshift(ch.pop());
        el.removeClass('btn-'+ch[0]).addClass('btn-'+ch[1]);
        chosen[name] = !chosen[name];
    };

Your case 0 and case 1 statements could be made into a function as well as they're identical (other than the true/false).


Fastest way to cut an array into two arrays at a particular index Suppose I have the array:

double[] arrayToCut = new double[100];
for(int i = 0 ; i < 100 ; ++i) {
    arrayToCut[i] = Math.random();
}

My objective is to end up with two arrays, firstArray and secondArray. firstArray is equal to (for example);

double[] firstArray = new double[50];
for(int i = 0 ; i < 50 ; ++i) {
    firstArray[i] = arrayToCut[i];
}

and secondArray is defined analogously (taking values from arrayToCut from 50 to 99 index).

However, this is linear time complexity and I want to cut gigantic arrays over 1 million in length. How can I speed up the above code (reduce computational overhead in any way for large arrays, whether by time complexity reduction or some other approach)?

I am pretty sure you can use copyOfRange(double[] original, int from, int to) from java.util.Arrays. @Rolfl said, and I quote : It is recommended practice to use Arrays.copyOf instead of System.arraycopy().

If you need to cut up the million+ array for parallel processing, then I would suggest you chunk the data while reading it, this should save you a processing step.

Edit: Internally, Arrays.copyOf() uses System.arraycopy() to do the hard work of duplicating the Array data in the new copy, and it will make your code look like this simple 1-liner:

double[] firstArray = Arrays.copyOf(arrayToCut, 50);
double[] secondArray = Arrays.copyOfRange(arrayToCut, 50, 100);

While using System.arraycopy() may seem like the fastest alternative, whenever you copy an array you have to:

first create the destination for the data to be copied to. This step requires initializing all the destination array points to their initialization values, which is a linear operation, but done in native code, so is fast.
then call the System.arraycopy() for the actual data re-write.

There are many ways that people may want to do this operation, and the various Arrays.copy*() methods are designed to make the relatively mundane, yet still rather complicated and bug-prone process a lot simpler, and less buggy.

Whenever you have a use case where you are copying data from one array to a new array, you should by default use the Arrays.copy*() methods. Only when you have a use case where you are overwriting data in to an existing array (or relocating data inside an array) should you use System.arraycopy(). As for the performance aspect, it appears that the performance of most native mechanisms are similar.

Arrays.copy* methods lead to better, more maintainable, and less buggy code, and, there is essentially no performance penalty because it is just doing the work you were going to do anyway.


Possible combinations following a rule The problem is the following:

We have a number of months to buy a number of supplies. The sooner we finish buying them, the better, but we don't know how much we can buy per month and would like to calculate all possible scenarios.

Suppose months = 5 and supplies = 9, and (72000) means 7 in the first month, 2 in the second and 0 in the following months.

VALID:   (90000)
VALID:   (81000)
VALID:   (72000)
VALID:   (71100)
INVALID: (61200)
INVALID: (80100)

This is basically a problem of combinations following a rule: in a MONTHS sized array, the n-th element cannot be larger than the (n-1)-th element, and the sum of all elements must be SUPPLIES.

My solution to do this was slicing the array into smaller parts and carrying digits over:

var months = 5;
var supplies = 9;

function calc(sum, nslots, maxdigit){
    if (sum === 0) return [];
    if (nslots === 0) return [];

    maxdigit = maxdigit || sum;

    var results = [];

    var firstDigit = Math.min(maxdigit,sum);
    if (maxdigit >= sum){
        var digits = [];
        for(var i=0;i<nslots;i++) digits.push(0);

        digits[0] = firstDigit;

        results = [digits];
    }

    for(;firstDigit>0;firstDigit--){
        var pointsLeft = sum - firstDigit;
        if (pointsLeft === 0) continue;

        var subArrays = calc(pointsLeft, nslots-1, firstDigit);

        if (subArrays.length === 0) continue;

        var subResults = subArrays.map(function(i){return [firstDigit].concat(i);});
        results = results.concat(subResults);
    }

    return results;
}

var results = calc(supplies,months);
console.log(results);

It's not in any way clear what this does though, and I myself would probably have a hard time understanding this code a year from now.

The clearest way I can imagine is just generating all numbers from 90000 down to 0 (in the 9 supplies and 5 months example) and checking if each number follows the rule, but it's ridiculous for bigger values of MONTHS and SUPPLIES.

I'd prefer a solution in ALGOL-descending languages. I imagine Python, Haskell or Erlang probably have some awesome way to solve this in just a few lines, but I couldn't use them here.

What you're looking for is all (ordered) integer partitions (of supplies) up to a maximum length (months).

Here is a solution in C#, as I'm not that comfortable with JavaScript.

private static IEnumerable<int[]> GetPartitions(int n, int length)
{
    return GetPartitions(n, length, n);
}

private static IEnumerable<int[]> GetPartitions(int n, int length, int max)
{
    if (n == 0)
    {
        // Only one way to partition 0.
        yield return new int[length];
        yield break;
    }

    if (n > length * max)
    {
        // No ways to partition n, as each of the elements
        // must be <= max.
        yield break;
    }

    for (var i = Math.Min(n, max); i > 0; i--)
    {
        foreach (var partition in GetPartitions(n - i, length - 1, i))
        {
            var newPartition = new List<int>(length);
            newPartition.Add(i);
            newPartition.AddRange(partition);
            yield return newPartition.ToArray();
        }
    }
}

And the output of GetPartitions(9, 5):

9, 0, 0, 0, 0
8, 1, 0, 0, 0
7, 2, 0, 0, 0
7, 1, 1, 0, 0
6, 3, 0, 0, 0
6, 2, 1, 0, 0
6, 1, 1, 1, 0
5, 4, 0, 0, 0
5, 3, 1, 0, 0
5, 2, 2, 0, 0
5, 2, 1, 1, 0
5, 1, 1, 1, 1
4, 4, 1, 0, 0
4, 3, 2, 0, 0
4, 3, 1, 1, 0
4, 2, 2, 1, 0
4, 2, 1, 1, 1
3, 3, 3, 0, 0
3, 3, 2, 1, 0
3, 3, 1, 1, 1
3, 2, 2, 2, 0
3, 2, 2, 1, 1
2, 2, 2, 2, 1

And here is my attempt in JavaScript:

function getPartitions(n, length, max) {
    if (n === 0) {
        // Only one way to partition 0.
        var result = [];
        for (var i = 0; i < length; i++) {
            result[i] = 0;
        }
        return [result];
    }

    if (n > length * max) {
        // No ways to partition n, as each of the elements
        // must be <= max.
        return [];
    }

    var partitions = [];
    for (var i = Math.min(n, max); i > 0; i--) {
        getPartitions(n - i, length - 1, i).forEach(function(partition) {
            partition.unshift(i);
            partitions.push(partition);
        });
    }

    return partitions;
}

getPartitions(9, 5, 9).forEach(function(partition) {
    console.log(partition);
});


Generic and accurate floating point "equality" Like those who have come before me, I foolishly have sought to implement a generic, efficient, idiomatic, and most importantly correct method of comparing floating point numbers for equality. Like those who have come before me, I have most likely failed in at least one of those regards.  I'd appreciate comments on any aspect of the following code

#include <cmath>
#include <cstddef>
#include <cstdint>
#include <limits>
#include <type_traits>

template<bool B, typename T, typename DEFAULT = void>
struct cond
{
    static constexpr bool value = B;
    using default_type = DEFAULT;
    using type = T;
};

template<typename Head, typename... Tail>
struct select
{
    using type =
        typename std::conditional<Head::value,
                                 typename Head::type,
                                 typename select<Tail...>::type>::type;
};

template <typename T>
struct select<T>
{
    using type = T;
};

template <bool B, typename T, typename DEFAULT>
struct select<cond<B, T, DEFAULT>>
{
    using type = typename std::conditional<B, T, DEFAULT>::type;
};

template <typename Head, typename... Tail>
using select_t = typename select<Head, Tail...>::type;

template <typename FloatingPoint>
struct SelectIntegralOfSameSize {
    using type = select_t<
        cond<sizeof(FloatingPoint) == sizeof(int64_t), int64_t>,
        cond<sizeof(FloatingPoint) == sizeof(int32_t), int32_t>,
        cond<sizeof(FloatingPoint) == sizeof(int16_t), int16_t>,
        cond<sizeof(FloatingPoint) == sizeof(int8_t), int8_t>
    >;
    static_assert(!std::is_void<type>::value, "Could not find sane integral type");
};

template <typename FP, typename I = typename SelectIntegralOfSameSize<FP>::type>
union Fp_Int_Union {
    FP fp_;
    I i_;
};

template <typename FP>
bool nearlyEqual(FP left, FP right, FP maxDiff, std::size_t maxUlpsDiff) {
    // handle NaNs and infinities
    if (!std::isfinite(left) || !std::isfinite(right)) {
        return false;
    }

    // ULP comparison breaks for different signs
    if ((left < 0) != (right < 0)) {
        // Handle -0 == +0
        return left == right;
    }

    // ULP comparison fails near zero
    if (std::abs(left - right) <= maxDiff) {
        return true;
    }

    Fp_Int_Union<FP> l, r;
    l.fp_ = left;
    r.fp_ = right;

    return std::abs(l.i_ - r.i_) <= maxUlpsDiff;
}

Consider using std::nextafter()

The math library includes the function std::nextafter() that will give you the next floating point number with the smallest possible, strictly positive difference with the given number. This is much safer than union-casting a float into an int, because there is no guarantee that two close floating point numbers will also be close as integers. In particular, that is only true if the exponent of both floating point numbers is the same. For example:

bool closeEnough = false;

if (left <= right) {
    do {
        if (left >= right) {
            closeEnough = true;
            break;
        }
        left = std::nextafter(left);
    } while (maxUlpsDiff--);
} else {
    // handle the left > right case
}

return closeEnough;

You'd obviously only want to use this method for small values of maxUlpsDiff.

Is it really what you need?

You are trying to check whether two floating point numbers are close, for two possible ways of closeness: floating point difference, and ULP distance. Is that really what you need? Are you sure that the values you choose for maxDiff and maxUlpsDiff are exactly right? If not, you might get false positives or negatives.

Can you reformulate your floating point calculations into integer calculations? Integers may have a limitted range, but at least they are exact. Maybe you can use them to perform fixed-point arithmetic, or perhaps you can use two integers to form a rational number.


Variadic template for concisely defining a tuple whose components are all the same type C++11 provides a std::tuple template, but it requires the types of all the fields to be listed individually, which is inconvenient if there are many fields and they're all the same type.  If I want a tuple of, say, 10 integers, instead of writing std::tuple<int, int, int, int, int, int, int, int, int, int>, I'd like to be able to just write something like tuple_of<10, int>.

To that end, I've written a template that expands a number into that many copies of a type:

template <unsigned int n, typename T>
struct tuple_of {
private:

  // Adds another copy of T to the list
  template <unsigned int m, typename... Ts>
  struct tuple_builder {
    using type = typename tuple_builder<m-1, T, Ts...>::type;
  };

  // Makes a tuple out of all the copies of T that have been listed
  template <typename... Ts>
  struct tuple_builder<0, Ts...> {
    using type = std::tuple<Ts...>;
  };

public:

  tuple_of() = delete;  // Prevent accidental instantiation

  using type = typename tuple_builder<n, T>::type;
};

// Convenience alias
template <unsigned int n, typename T>
using tuple_of_t = typename tuple_of<n, T>::type;

Now you can write either tuple_of<10, int>::type or tuple_of_t<10, int>.

This works, but it seems a little awkward.  A few things in particular bother me:

I don't really like having to write either ::type or the _t suffix of the convenience alias; I'd rather just write tuple_of<10, int>.  I've followed the pattern established by the templates in the standard <type_traits> header — e.g. std::result_of_t<T> is an alias for std::result_of<T>::type — but tuple_of isn't a type trait so I don't know if I should be following that convention.
I've made the template a struct because that seems to be the norm for templates used in metaprogramming, but it contains a private helper template that I don't really want to expose, and it's a little weird to have private stuff in a struct.
This is the first variadic template I've written, so I'm not sure that my tuple_builder is the most elegant way to build a list of n copies of a type.
While testing, I accidentally created an instance of the template type itself, rather than the tuple type it produces, by writing tuple_of<10, int> instead of tuple_of<10, int>::type.  To avoid that mistake in the future, I deleted the constructor.  I don't know whether that's something templates like this "should" do; the templates in <type_traits> don't have deleted constructors.

Can this template be improved?

As nwp pointed out in a comment, std::array is a better choice for this.  I didn't need to write this template at all.


Find if one list is a subsequence of another So the problem of verifying if a list is a subsequence of another came up in a discussion, and I wrote code that seems to work (I haven't rigorously tested it).

IsSubequence.py

def is_subsequence(lst1, lst2):
    """
        *   Finds if a list is a subsequence of another.

        *   Params:
            *   `lst1` (`list`): The candidate subsequence.
            *   `lst2` (`list`): The parent list.
        *   Return:
            *   (`bool`): A boolean variable indicating whether `lst1` is a subsequence of `lst2`.
    """
    l1, l2 = len(lst1), len(lst2)
    if l1 > l2:  #`l1` must be <= `l2` for `lst1` to be a subsequence of `lst2`.
        return False
    i = j = 0
    d1, d2 = l1, l2
    while i < l1 and j < l2:
        while lst1[i] != lst2[j]:
            j += 1
            d2 -= 1
            if d1 > d2:  #At this point, `lst1` cannot a subsequence of `lst2`.
                return False
        i, j, d1, d2 = i+1, j+1, d1-1, d2-1
        if d1 > d2:
            return False
    return True

I'm primarily concerned about performance.

Review

Testing

  (I haven't rigorously tested it).

Well you should write some test to ensure validity of the function. So even after changes you can be sure it will still work. doctest is a pretty nice module to use, and is a nice extension of your docstring
Naming

Variables should have descriptive names!

lst1, lst2 if it wasn't for that docstring I would not have known what is the subseq and the parent, so instead I propose to rename them to needle and haystack here the intent is more clear

Same goes for d1, d2... I can see that they are the remaining length of the list, but it is hard to tell from the variable name.
for is considered more Pythonic vs while

For loops are Pythons greatest feature IMHO, they are easy to read and short to write

You should start writing for loops instead of a while, "Loop like a native" might be an interesting talk to view
Too many assignments in a line

Might be preference, but I find this line hard to read:

i, j, d1, d2 = i+1, j+1, d1-1, d2-1

There are too many values with not enough descriptive names on this line

Alternative

We can instead loop over the haystack and use slicing to compare the sliced haystack with the needle, lastly top it off with the any keyword and write some tests with the doctest module

import doctest

def is_subsequence(needle, haystack):
    """
    Finds if a list is a subsequence of another.

    * args
        needle: the candidate subsequence
        haystack: the parent list

    * returns
        boolean

    >>> is_subsequence([1, 2, 3, 4], [1, 2, 3, 4, 5, 6])
    True
    >>> is_subsequence([1, 2, 3, 4], [1, 2, 3, 5, 6])
    False
    >>> is_subsequence([6], [1, 2, 3, 5, 6])
    True
    >>> is_subsequence([5, 6], [1, 2, 3, 5, 6])
    True
    >>> is_subsequence([[5, 6], 7], [1, 2, 3, [5, 6], 7])
    True
    >>> is_subsequence([1, 2, 3, 4, 5, 6, 7, 8], [1, 2, 3, [5, 6], 7])
    False
    """
    return any(
        haystack[i:i+len(needle)] == needle
        for i in range(len(haystack) - len(needle) + 1)
    )

if __name__ == '__main__':
    doctest.testmod()


Idiomatic letterpress cheater I need some guidance on making the following code more idiomatic:

(ns cheaterpress.algorithm
  (:use [clojure.string :only [split lower-case]])
  (:use [clojure.test]))

(def words (split (slurp "words") #"\s"))

(defn string-to-letter-hash [string]
  (loop [letters (seq string) letter-hash {}]
    (if (empty? letters)
      letter-hash
      (let [letter (first letters) letter-freq (get letter-hash letter 0)]
        (recur (rest letters) (assoc letter-hash letter (inc letter-freq)))))))

(defn can-play [word letters index]
  (loop [letters letters index index]
    (if (and (= (inc index) (count word)) (not (= 1 (count word))))
      true
      (let [letter (get word index) frequency (get letters letter 0)]
        (when (not (= 0 frequency))
          (recur (assoc letters letter (dec frequency)) (inc index)))))))

(println
  (time
    (count (doall (filter #(can-play % (string-to-letter-hash "abcdefghijklmnopqrstuvxyz") 0) (map lower-case words))))))

string-to-letter-hash takes a string and returns a map with the letter frequencies, e.g.:

(string-to-letter-hash "hello")
;=> {\h 1 \e 1 \l 2 \o 1}

can-play takes a letter frequency hash and a word, and then it finds out if it can make the word from the letters in the hash map. If it can, it returns true, otherwise, nil.

(can-play "hello" {\h 1 \e 1 \l 2 \o 1} 0)
;=> true
(can-play "lol" {\h 1 \e 1 \l 2 \o 1} 0)
;=> true
(can-play "hello world" {\h 1 \e 1 \l 2 \o 1} 0)
;=> nil

The last bit of code in my script basically gets all the words in a file and finds which of them can be played according to the can-play function.

Well there is a standard library function: frequences :)

user> (doc frequencies)
-------------------------
clojure.core/frequencies
([coll])
  Returns a map from distinct items in coll to the number of times
  they appear.

Since in Clojure many many types can be converted to sequences, frequencies will work on many types, including strings. In Clojure you can get an amazing amount of work done simply by using the sequence library. So your string-to-letter-hash is actually just an example of frequences.

For your can-play: the same type of thinking applies. Try to resist your first impulse of writing a loop. The Clojure library functions will almost always do the job - the thing that requires training is knowing the library well enough to find a good way to compose the functions.

(defn can-play? [word freqs]
  (every? #(>= % 0) (vals (merge-with - freqs (frequencies word)))))

(updated to comment version: https://gist.github.com/4105644)


Logging Entity Framework Changes - improve performance I have the following code to log changes using Entity Framework 6.

At the moment it only logs changes to a particular table - but I intend to expand it to cope with any table.  I'm not sure of the best way to extract the table name.

I call this code before Db.SaveChanges.  Db is the DBContext.

private void LogHistory()
    {
        var entries = this.Db.ChangeTracker.Entries()
            .Where(e => e.State == EntityState.Added || e.State == EntityState.Modified || e.State == EntityState.Deleted)
            .ToList();
        foreach (var entry in entries)
        {
            foreach (string o in entry.CurrentValues.PropertyNames)
            {
                var property = entry.Property(o);
                var currentVal = property.CurrentValue == null ? "" : property.CurrentValue.ToString();
                var originalVal = property.OriginalValue == null ? "" : property.OriginalValue.ToString();

                if (currentVal != originalVal)
                {
                    if (entry.Entity.GetType().Name.Contains("PropetyPair"))
                    {
                        //  make and add log record
                    }
                }
            }
        }
    }

Not a big deal, but this:

var currentVal = property.CurrentValue == null ? "" : property.CurrentValue.ToString();
var originalVal = property.OriginalValue == null ? "" : property.OriginalValue.ToString();

Can be shortened to this:

var currentVal = (property.CurrentValue ?? string.Empty).ToString();
var originalVal = (property.OriginalValue ?? string.Empty).ToString();

  I'm not sure of the best way to extract the table name.

You're not doing that. You're looking at the name of the type of the entity - being an object/relational mapper, EF maps that entity to a table; if you need the table name then you need to look at the entity's table mappings.

  I intend to expand it to cope with any table

How about having an IEnumerable<Type> where you have all the entity types you want to log changes for, and then instead of getting the type's name and comparing with some magic string, you can just do _monitoredTypes.Contains(typeof(entry.Entity)) (not tested, just a thought).

In terms of performance, you're looping too much:

if (entry.Entity.GetType().Name.Contains("PropetyPair"))

This condition doesn't need string o, as it's working off entry - thus, you can move that condition up two levels and only enter the 2nd loop when the entity type is interesting.


Re-creating user discounts nightly I'm calling these two methods on a Worker that is executed every 24 hours at midnight in the background.

The code seems to be a little bit difficult to read and I'm not sure if i'm iterating through the collections in the smartest way.

Is there a more elegant or faster way to iterate on these methods?
Would you suggest any other change?

  # Deletes UserDiscounts when the discount belongs to a segment that the user does not belong to
  def clean_user_discounts
    UserDiscount.all.each do |user_discount|
      user_segment = UserSegment.find_by(user: user_discount.user)
      discount_segment = DiscountSegment.find_by(discount: user_discount.discount)
      user_segment_id = user_segment ? user_segment.segment_id : nil
      discount_segment_id = discount_segment ? discount_segment.segment_id : nil
      user_discount.delete unless discount_segment_id == user_segment_id
    end
  end

  # Creates UserDiscounts when a user belongs to a segment and that segment has a discount
  def create_user_discounts
    UserSegment.all.each do |user_segment|
      DiscountSegment.where(segment: user_segment.segment).each do |discount_segment|
        unless UserDiscount.find_by(discount: discount_segment.discount, user: user_segment.user)
          UserDiscount.create(discount: discount_segment.discount, user: user_segment.user)
        end
      end
    end
  end

The first thing that I noticed that could be improved is to use find_each instead of all.

all will load all the records from the database at once, which can blow up your server. Using find_each instead, you'll load records in batches.

Also, another thing here is that you're using heavily object in queries, for instance: UserSegment.find_by(user: user_discount.user) instead of UserSegment.find_by(user_id: user_discount.user_id)

When using object references like that, if the object isn't loaded rails will perform a query to retrieve the object - therefore this code have a N+1 problem in it - meaning you end up loading those objects (such as user, discount and segment) even though have basically the same info in the parent object already loaded.

So always prefer to ids instead of objects for those type of queries.

If I understood your problem correctly I think you can simplify greatly its performance and syntax by just leveraging ActiveRecord query options. Instead of loading the objects in memory and using ruby to do the verifications for you, you can just instruct ActiveRecord to perform joins and whatnot and just return to you the information that you need.

Here's how I'd do it:

First, for removing invalid user discounts I'd create a query that return them to me, like so:

class UserDiscount < ApplicationRecord
  belongs_to :user
  belongs_to :discount

  def self.invalid_user_discounts
    UserDiscount.
      joins("INNER JOIN discount_segments ON user_discounts.discount_id = discount_segments.discount_id").
      joins("INNER JOIN user_segments ON user_discounts.user_id = user_segments.user_id").
      where("user_segments.segment_id <> discount_segments.segment_id")
  end
end

See how instead of loading multiple classes I just let the powerful SQL and rails do the job for me? This query isn't too complicated to understand and does the same job as the previous loop.

Once we have a query that returns invalid discounts, I can use find_each and remove them:

def self.clean_user_discounts
  invalid_user_discounts.find_each do |user_discount|
    user_discount.destroy
  end
end

For creating discounts, I use the same approach, I let AR and SQL do the heavy lifting in this case, and just use the final formatted response to iterate over the results, like so:

def self.create_user_discounts
  query = UserSegment.
    joins("INNER JOIN discount_segments ON user_segments.segment_id = discount_segments.segment_id").select(:id, :discount_id, :user_id)

  query.find_each do |user_and_discount_segment|
    UserDiscount.find_or_create_by(
      discount_id: user_and_discount_segment.discount_id,
      user_id: user_and_discount_segment.user_id)
  end
end

Rails allows us to merge the two tables UserSegment and DiscountSegment and as well to select just the fields from that query that we need, in this case: user_id, discount_id - making the creation of UserDiscount much easier.

Finally, you can see the usage of find_or_create_by - this is provided by Rails as well, so we don't have to issue two commands.

full source code: https://gist.github.com/jonduarte/c54146cb7e00f0045193aad739301c13


Reading a long file and getting sum of chars by multithreading import threading
import os
import sys
import time
l=0
length=[]
def File_Opener(keys,l):
    for lines in keys:
        l = l + len(lines)-1
    length.append(l)
    l=0
if __name__ == "__main__":
    start=time.time()
    f=open(sys.argv[1],"r")
    print "File to be anaysed is %s" %sys.argv[1]
    Threads=[]
    keys=[]
    for i in range(100):
        try:
            for j in range(200000):
                keys.append(f.next())
        except:
            pass
        t = threading.Thread(target=File_Opener,args=(keys,l))
        t.start()
        keys=[]
        print "Created thread %d" %i
        Threads.append(t)
    for thread in Threads:
        thread.join()
print "Total Length of words are", sum(length)
print "Time taken is %f" %(time.time()-start)
print "Done!!"

Issue : Takes 5x more time than non-multi threaded code.

I am trying to get sum of all chars in a file of very big size. The reason why i used the for loop is to divide the file into small list and pass each to a thread and except is used for in case the file does't come complete in 200000 chars I want to know as to how it can be done with multi threading also.
Doing it in python2.7 in Linux

Before starting to talk about threads in Python, let's compare to a sequential implementation. I’ll start by building a test file using the following code:

import string
import random

with open('test.txt', 'w') as f:
    for _ in range(20000000):
        line = ''.join(random.choice(string.printable) for _ in range(random.randint(0, 10000)))
        print(line, file=f)

This is Python 3 syntax, and I could not recommend more to use it instead of the end-of-life Python 2.7.

Anyway, after a while, I have a 5.9G file of 64 or so million lines (yes, string.printable contains the newline character) so I stopped it. Let's read it and count the length of its lines.

For starter (and you may have noticed in the snippet above), you should use the with statement to open files so you don't accidentally forget to close them. You can also iterate directly over the content of the file instead of using next; and if you want to know the line number (to spawn a thread each 200000 lines, for instance), you can always delegate the task to enumerate. This way, you’ll avoid using range instead of xrange and consume memory where you don't need to.

Second, timing should be done using timeit for better precision and less boilerplate. Let's start by timing the reading of the file only to get a ballpark figure of how much time we can expect to gain. We’ll use part of the consume recipe from itertools:

from collections import deque

def read_file(filename):
    with open(filename) as f:
        deque(f, maxlen=0)

if __name__ == '__main__':
    import timeit
    time = timeit.timeit('read_file("test.txt")', 'from __main__ import read_file', number=1)
    print('execution took {}s'.format(time))

Result is execution took 41.8203251362s on my machine. This is roughly as fast as I can expect the file to be read… without taking caching into account. If we change the timing part to min(timeit.repeat('read_file("test.txt")', 'from __main__ import read_file', number=1, repeat=5)) we get somewhere around 3.5s.

Now, on to counting characters. As I read it, you want to remove the newline character that is found at the end of each line. You can, as you do here, subtract 1 from each line length; or, alternatively, sum each line length and subtract the amount of lines afterwards. The second approach is easier as we can use map to produce a list of all line lengths and then, sum it before subtracting its len:

def count_characters(filename):
    with open(filename) as f:
        line_lengths = map(len, f)
    return sum(line_lengths) - len(line_lengths)

Timings approaches the same 40s figure for a single execution, but is about 6.5s on repeated executions. This is only 3 seconds of counting characters in 64 million lines; that's somewhat acceptable, but map is building a list in memory which is 64 million items long, so maybe we could speed things up if we didn't:

def count_characters(filename):
    count = 0
    with open(filename) as f:
        for line in f:
            count += len(line) - 1
    return count

Unfortunately, a for loop in Python is slower than a for loop in C (which is what map does) so this code is not faster (albeit being roughly as fast).

Now, trying to speed things up. To parallelize tasks on chunk of data, we usually use map from multiprocessing.Pool. But serializing this much data to feed other processes takes time and isn't worth it. So we are left trying to use threading as you did. However, due to the GIL, you will not be able to run more than one thread in parallel and will be either reading the file or summing the lines; exactly as it is done in sequential code, but you’ll have more overheads due to using threads and helper data structures to move data around. So all in all, for this kind of problem, you’re better off sticking to the sequential implementation.

But let's analyze your code anyway:

The l parameter is useless, since it's an integer, it is immutable and the local version on each thread won't interfere with others so passing it as a parameter with seemingly a value of 0 and then resetting it to 0 after the computation is useless;
Contrarily, the length list as a global variable is more problematic, I’d rather pass it as a parameter, leading to the threaded functions being:

def count_characters_in_chunk(lines, accumulator):
    length = sum(len(line) - 1 for line in lines)
    accumulator.append(length)

As said previously, organizing lines into chunks without prior knowledge of its length can be done using enumerate but we need to account for the last chunk not being the full size, if any:

def count_characters_in_file(filename, chunk_size=200000):
    threads = []
    lengths = []

    with open(filename) as f:
        lines = []
        for line_number, line in enumerate(f, start=1):
            lines.append(line)
            if line_number % chunk_size == 0:
                t = threading.Thread(target=count_characters, args=(lines, lengths))
                t.start()
                threads.append(t)
                lines = []

        if lines:
            t = threading.Thread(target=count_characters, args=(lines, lengths))
            t.start()
            threads.append(t)

    for t in threads:
        t.join()
    return sum(lengths)

However, there is still two issues with this code:

The repetition of thread management is ugly and error-prone, plus the manual handling of the lines list is unnecessarily verbose. You can use itertools.islice to simplify all that;
The use of a simple list to store resulting lengths is a bad habit to have as threaded code is prone to race-conditions (although highly unlikely in this case) that can lead to loss of data. You should use a Queue.Queue instead.

Final code being:

import itertools
import threading
import Queue

def extract_from_queue(queue):
    while not queue.empty():
        yield queue.get()

def count_characters(lines, accumulator):
    length = sum(len(line) - 1 for line in lines)
    accumulator.put(length)

def count_characters_in_file(filename, chunk_size=200000):
    threads = []
    lengths = Queue.Queue()

    with open(filename) as f:
        while True:
            lines = list(itertools.islice(f, chunk_size))
            if not lines:
                break
            t = threading.Thread(target=count_characters, args=(lines, lengths))
            t.start()
            threads.append(t)

    for t in threads:
        t.join()
    return sum(extract_from_queue(lengths))

if __name__ == '__main__':
    import sys
    count_characters_in_file(sys.argv[1])

But timings using caching indicates 44 seconds on my machine, so not really worth it given the speed and simplicity of the sequential implementation.


A first Hangman game in C++ I wonder whether I included too many libraries; also can you point out obvious beginner mistakes?

Is the while loop the best for what I intend, or is there a better way?

Also, is it possible to put more into functions such that the main() looks like this:

int main()

{

call function1

call function2

etc.

return 0;

}

#include <iostream>
#include <fstream>
#include <stdlib.h>
#include <time.h>
#include <string>
#include <vector>
#include <cstring>
using namespace std;

string random_line()
{
    string line;
    vector<string> lines;
    srand(time(0));
    //input file stream
    ifstream file("wordlist.txt");
    //count number of total lines in the file and store the lines in the string vector
    int total_lines=0;
    while(getline(file,line))
    {
        total_lines++;
        lines.push_back(line);
    }
    //generate a random number between 0 and count of total lines
    int random_number=rand()%total_lines;
    //fetch the line where line index (starting from 0) matches with the random number
    return lines[random_number];
}

int main()
{
    string secret_word=random_line();
    cout << secret_word; //this has to be cut at the end
    int word_length=secret_word.length();
    string guess_word="";
    string guessed_letters="";
    for (int i=0; i<word_length; ++i)
    {
        guess_word.append("_");
    }
    int remaining_fails=10;
    string guess="";

    while(remaining_fails!=0)
    {
        if(guess_word==secret_word)
        {
            cout << "Congratulations, the word is: "<<secret_word<<endl;
            return 0;
        }
        else
        {
            cout << string(50, '\n');
            cout << guess_word << endl;
            cout << "Remaining fails: " << remaining_fails << endl;
            cout << "Guessed letters: " << guessed_letters << endl;
            cout << "Guess your letter!" << endl;
            cin  >> guess;
            if(guess.length() !=1)
            {
                cout << "Please only enter one character!";
            }
            else if(strstr(secret_word.c_str(),guess.c_str()))
            {
                for(int i=0; i<word_length; ++i)
                {
                    if (guess.at(0)==secret_word.at(i))
                    {
                        guess_word.at(i)=guess.at(0);
                    }
                    else
                    {

                    }
                }
            }
            else
            {
                guessed_letters.append(guess);
                remaining_fails--;
            }
        }

    }
    cout << "You loose!"<<endl;
    return 0;
}

The wordlist.txt contains?

word
trio
list

I'm not sure if I'd use the phrase "too many", but I think you've included a header or two you'd probably be better off without.

The most obvious example would be <cstring>. There's rarely a good reason to use any of the <cstring> functions in C++ code (and I don't see any hint that this code is one of the rare exceptions either).

Right now, the main (only?) thing I see you using from <cstring> is strstr. std::string has a find member that can find a substring like strstr does.
Your code also seems somewhat inconsistent, at least to me. For example, in one place you create a string with N repetitions of a single character using code like: std::string(character, repetitions). In another place you do roughly the same thing using a loop to append on underscore at a time. Since they're doing the same thing, it would be better for them to do it the same way (preferably the constructor for std::string).
Right now you use a string to hold a single character (and enforce its being only one character). Since it's only a single char, I'd rather just use a char to hold it.
As a general rule of thumb, I'd prefer a well-chosen algorithm from the standard library over a raw loop.
Yes, you can (and in my opinion, should) break the code up into more functions (and based on your question, it seems like you already agree, but may not be sure how to do it).
I'd generally prefer to use the new random-number generation routines in <random> over the srand/rand from the C library. Likewise, I'd rather use the distribution classes in the standard library over the rand() % total_lines you have now. (but in fairness, they are more work to use, and it's unlikely to make a difference anybody's likely to notice or care about (but in many other cases, it's more important).
I'd avoid using namespace std; I know at first it seems painful to type in std:: everywhere, but it's still better than using namespace std;.
There are also neater ways to read words from a file into an array of strings, such as:

std::ifstream infile("wordlist.txt");

std::vector<std::string>{std::istream_iterator<std::string>(infile), {}};

One point though: this will produce different results if your input file has multiple words on one line. In this case your code treats a whole line as one word, but this treats each word separately.
I generally advise against using std::endl. It's probably harmless in this case, but a habit I'd try to avoid anyway.


Database interaction logic I have a database-intensive app, almost every activity requires access to the database, both reading and writing.
I had a lot of problems with separate threads and activities on the stack closing connections to the database when the current activity was using it, even if I opened and closed the database immediately before and after use.

So I opted to use a single instance of the database. Now I have no problems, but, is this right?

Application class

public class ApplicationClass extends Application
{
    private DatabaseAdapter PrDatabaseAdapter;

    public DatabaseAdapter getDatabaseAdapter()
    {
        return PrDatabaseAdapter;
    }

    @Override
    public void onCreate()
    {
        super.onCreate();

        PrDatabaseAdapter = new DatabaseAdapter(this);
        PrDatabaseAdapter.open();
    }
}

Adapter Class
The database adapter just sits on top of the DatabaseManager (my SQLiteOpenHelper)

private Context context;
private SQLiteDatabase db;
private DatabaseManager dbManager;

/**
 * The DatabaseAdapter contains all interaction logic for the database
 * @param context
 */
public DatabaseAdapter(Context context)
{
    this.context = context;
}

/**
 * Opens writable database
 * @return DatabaseAdapter;
 * @throws SQLiteException
 */
public DatabaseAdapter open() throws SQLiteException
{
    dbManager = new DatabaseManager(context);
    db = dbManager.getWritableDatabase();
    return this;
}

/**
 * Closes database connection
 */
public void close()
{
    dbManager.close();
}

I'm still learning a lot about Java and Android and I would appreciate some insight to the pros and cons of this approach.

I'm not familiar with Android, so just some generic Java notes:

PrDatabaseAdapter should be prDatabaseAdapter (with lowercase first letter). See Effective Java, 2nd edition, Item 56: Adhere to generally accepted naming conventions and The Java Language Specification, Java SE 7 Edition, 6.1 Declarations:

  Names of fields that are not final should be in mixed
  case with a lowercase first letter and the first letters of subsequent words capitalized.

Javadoc comments like @param context aren't too useful, they say nothing more than the source code alone. I'd remove them.
Does it make sense to create a DatabaseAdapter with null context? I'd check this in the constructor and throw and NullPointerException if it's null. See Effective Java, 2nd edition, Item 38: Check parameters for validity


Deque Implementation in Java Please review my Deque implementation in Java and suggest ways of improving this.

    public class DequeImplementation<T> {
    private QueueNode<T> first;
    private QueueNode<T> last;
    private int size;

    private static class QueueNode<T> {
        private T val;
        private QueueNode<T> next;
        private QueueNode<T> prev;

        public QueueNode(T val) {
            this.val = val;
            this.next = null;
            this.prev = null;
        }

        public T getValue() {
            return val;
        }

        public void setNext(QueueNode<T> nextNode) {
            next = nextNode;
        }

        public QueueNode<T> getNext() {
            return next;
        }

        public void setPrev(QueueNode<T> prevNode) {
            prev = prevNode;
        }

        public QueueNode<T> getPrev() {
            return prev;
        }

        public String toString() {
            if(val == null) {
                return "null";
            }
            return val.toString();
        }
    }

    public int size() {
        return size;
    }

    public boolean isEmpty() {
        return size == 0;
    }

    public void offerLast(T val) {
        QueueNode<T> cur = new QueueNode(val);
        if(size == 0) {
            cur.setNext(first);
            first = cur;
            last = cur;
            size++;
            return;
        }

        last.setNext(cur);
        cur.setPrev(last);
        last = cur;
        size++;
    }

    public void offerFirst(T val) {
        QueueNode<T> cur = new QueueNode(val);
        if(size == 0) {
            cur.setNext(first);
            first = cur;
            last = cur;
            size++;
            return;
        }

        cur.setNext(first);
        first.setPrev(cur);
        first = cur;
        size++;
    }

    public T peekLast() {
        if(isEmpty()) {
            return null;
        }
        return last.getValue();
    }

    public T peekFirst() {
        if(isEmpty()) {
            return null;
        }
        return first.getValue();
    }

    public QueueNode<T> pollFirst() {
        if(isEmpty()) {
            return null;
        }

        QueueNode<T> cur = first;
        first = first.getNext();
        if(first != null) {
            first.setPrev(null);
        }
        size--;
        return cur;
    }

    public QueueNode<T> pollLast() {
        if(isEmpty()) {
            return null;
        }

        QueueNode<T> cur = last;
        last = last.getPrev();
        if(last != null) {
            last.setNext(null);
        }
        size--;
        return cur;
    }

    public void clear() {
        size = 0;
        first = null;
        last = null;
    }

    @Override
    public String toString() {
        if(size == 0) {
            return "[]";
        }

        StringBuilder sb = new StringBuilder("null<-");
        QueueNode<T> cur = first;
        sb.append(cur.getValue() + "<-");

        while(cur.getNext() != null && !cur.equals(last)) {
            sb.append(cur.getNext().getValue() + "<-");
            cur = cur.getNext();
        }

        sb.append("null");
        return sb.toString();
    }

    public static void main(String[] args) {
        DequeImplementation<Integer> dq = new DequeImplementation<>();
        System.out.println(dq.size());
        System.out.println(dq.pollLast()); //returns null
        System.out.println(dq);
        dq.offerLast(100);
        dq.offerFirst(10);
        dq.offerFirst(1);
        System.out.println(dq.pollFirst());
        System.out.println(dq);
        System.out.println(dq.size());
        System.out.println(dq.peekLast());
        System.out.println(dq.pollLast());
        System.out.println(dq);
        System.out.println(dq.size());
        System.out.println(dq.pollFirst());
        System.out.println(dq.pollLast()); //returns null
        dq.clear();
        System.out.println(dq.size());
    }
}

I have embedded my comments straight in the code:

import java.util.Objects;

// 'Implementation' does not provide any more information on this class.
// Remove it, so less typing.
//public class DequeImplementation<T> {
public class Deque<T> {

    private QueueNode<T> first;
    private QueueNode<T> last;
    private int size;

    // You can add 'final' if you WANT:
    private static final class QueueNode<T> {

        private T val;
        private QueueNode<T> next;
        private QueueNode<T> prev;

        // You don't really need 'public' since the class is the inner private
        // static class:
        /*public*/ QueueNode(T val) {
            this.val = val;
            // Reference fields are 'null' by default:
            //this.next = null;
            //this.prev = null;
        }

        // Remove 'public' as well:
        /*public*/ T getValue() {
            return val;
        }

        /*public*/ void setNext(QueueNode<T> nextNode) {
            next = nextNode;
        }

        /*public*/ QueueNode<T> getNext() {
            return next;
        }

        /*public*/ void setPrev(QueueNode<T> prevNode) {
            prev = prevNode;
        }

        /*public*/ QueueNode<T> getPrev() {
            return prev;
        }

        // Here we need 'public'.
        public String toString() {
            // if (val == null) {
            //    return "null";
            //}
            //return val.toString();
            // Does exactly the same as above 3 lines:
            return Objects.toString(val);
        }
    }

    public int size() {
        return size;
    }

    public boolean isEmpty() {
        return size == 0;
    }

    public void offerLast(T val) {
        QueueNode<T> cur = new QueueNode(val);
        if (size == 0) {
            first = last = cur;
            size = 1;
            return;
        }

        last.setNext(cur);
        cur.setPrev(last);
        last = cur;
        size++;
    }

    public void offerFirst(T val) {
        QueueNode<T> cur = new QueueNode(val);
        if (size == 0) {
            first = last = cur;
            size = 1;
            return;
        }

        cur.setNext(first);
        first.setPrev(cur);
        first = cur;
        size++;
    }

    public T peekLast() {
        if (isEmpty()) {
            return null;
        }
        return last.getValue();
    }

    public T peekFirst() {
        if (isEmpty()) {
            return null;
        }
        return first.getValue();
    }

    // No reason to return a node since everything the user can do with it is
    // to ask its value; return the value:
    // public QueueNode<T> pollFirst() {
    public T pollFirst() {
        if (isEmpty()) {
            return null;
        }

        QueueNode<T> cur = first;
        first = first.getNext();
        if (first != null) {
            first.setPrev(null);
        }
        size--;
        return cur.getValue();
    }

    // Same argument as in 'pollFirst':
    public T pollLast() {
        if (isEmpty()) {
            return null;
        }

        QueueNode<T> cur = last;
        last = last.getPrev();
        if (last != null) {
            last.setNext(null);
        }
        size--;
        return cur.getValue();
    }

    public void clear() {
        size = 0;
        first = null;
        last = null;
    }

    // This is a matter of taste, yet I don't quite like your idea to prepend
    // and append the output with 'null'. Think what the user sees if he really
    // has null values in the deque..a
    @Override
    public String toString() {
        if (size == 0) {
            return "[]";
        }

        StringBuilder sb = new StringBuilder("[");
        sb.append(first.getValue());
        QueueNode<T> node = first.getNext();

        while (node != null) {
            sb.append(" -> ").append(node.getValue());
            node = node.getNext();
        }

        return sb.append("]").toString();
    }

    public static void main(String[] args) {
        Deque<Integer> dq = new Deque<>();
        System.out.println(dq.size());
        System.out.println(dq.pollLast()); //returns null
        System.out.println(dq);
        dq.offerLast(100);
        dq.offerFirst(10);
        dq.offerFirst(1);
        System.out.println(dq.pollFirst());
        System.out.println(dq);
        System.out.println(dq.size());
        System.out.println(dq.peekLast());
        System.out.println(dq.pollLast());
        System.out.println(dq);
        System.out.println(dq.size());
        System.out.println(dq.pollFirst());
        System.out.println(dq.pollLast()); //returns null
        dq.clear();
        System.out.println(dq.size());
    }
}

Hope that helps.


Shapes that can be saved to a database I have been asked to refactor the below code using SOLID principles and I was just wondering how you would go about it. I am not really sure if I am winning or not.

Could you have a stab at it and let me see what you come up with?

public class Box
    {
        private BoxType _boxType;

        public double Width { get; set; }
        public double Height { get; set; }
        public double Radius { get; set; }

        public BoxType BoxType
        {
            get { return _boxType; }
            set { _boxType = value; }
        }

        public void Add()
        {
            using (var con = new SqlConnection())
            {
                SqlCommand cmd;

                if (_boxType == BoxType.Circle)
                {
                    cmd = new SqlCommand(string.Format("insert into [Table] values({0}, {1})", _boxType, Radius), con);
                }
                else
                {
                    cmd = new SqlCommand(string.Format("insert into [Table] values({0}, {1}, {2})", _boxType, Width, Height), con);
                }

                cmd.ExecuteNonQuery();
            }
        }

        public double CalculateArea()
        {
            if (_boxType == BoxType.Rectangle)
            {
                return Width * Height;
            }
            else if (_boxType == BoxType.Square)
            {
                return Width * Height;
            }
            else if (_boxType == BoxType.Circle)
            {
                return Radius * Math.PI;
            }
            else
            {
                return 0;
            }
        }
    }

    public enum BoxType
    {
        Unknown,
        Square,
        Rectangle,
        Circle
    }

Note: I don't claim to be an expert in SOLID principles, I do understand abstraction a bit though.

Back to SOLID, CalculateArea violates the Open/Closed principle (The most frustrating concept to me). That is, when you add a shape  you have to go and modify the code by adding an if-statement, this can easily solved by using interfaces,

 interface IShape{
    double CalculateArea();
 }

 class Rectangle : IShape{
    double CalculateArea(){
       return Width * Height;
    }
 }

And so on for different shapes, so there is no need for if-statements. Btw: how come you representing a circle in a class called Box??

Single responsibility principle is being violated as well, where your class represents a Box Object and persists it to a database, these are two different responsibilities. You should rather have Data Access Layer to deal with a very HIGH dependency like a database.

In regards to SQL generation you can abstract those as well using interfaces, and that is, given a shape get its sql code.

  interface ISQLShapeTemplates{
      DbCommand InsertionCode(IShape shape);
      DbCommand SelectCommand(IShape shape);
  }

I used DbCommand and not SqlCommand because it's more abstract .