Weekend code puzzle: ChrisG's answer (CFML)

G'day:
OK, now I'm gonna look at each person's submissions for the code puzzle ("Something for the weekend? A wee code puzzle (in CFML, PHP, anything really...)").

Chris has done a CFML version. He was very quick off the mark, and had his submission in before I varied the rules, so this one just solves the initial challenge which is to find the longest subseries within a given threshold, but does not consider equal-length longest subseries. Fair enough.



Here's the code:

function getSubseries(series, threshold){

    /*
     * Setup a subseries container, to be replaced whenever a longer series is
     * found. As well, let's calculate the length of the original series to avoid
     * doing it repeatedly with every iteration of the following for loop;
     */
    var subseries = [];
    var seriesLen = arrayLen(series);
    
    /* 
     * Setup two for loops, the first to keep track of the start index (a) and the second
     * will be used to keep track of the end index (z). We'll increment through each start
     * index then reduce the array from the end, replacing the subseries whenever BOTH the 
     * sum is <= threshold AND the array length is greater than the length of the value
     * currently stored there.
     */
    for(var a = 1; a <= seriesLen; a++){
        
        var z = seriesLen;
        var buffer = {
                'array' = [],
                'sum'   = 0
            };
        
        for( z; z > a; z--){
        
            buffer.array = arraySlice(series, a, z-a+1);
            buffer.sum   = arraySum( buffer.array );
                        
            if( buffer.sum <= threshold && arrayLen(buffer.array) > arrayLen(subseries) ){
                subseries = buffer.array;
            }
            
        }

    }

    return(subseries);
}

He also provided unit tests, but I ran this through my lot:

It's fair enough the latter two tests break, as he did not code for those ones, but there is a slight glitch there with the "should return elements if there are any within the threshold" test.

I found the reason for this fairly quickly: there's an "off by one" error in his code. This:

for( z; z > a; z--){

Should be this:

for( z; z >= a; z--){

I guess my test is mislabelled: it should be "should return a match from a one-element array".

Thinking about edge cases, this prompted me to add another coupla tests to my test suite:

it("works when the subseries is equal to the threshold and is at the beginning of the series", function(){
    series = [100,100,100,100,100,100,600,150,150,150,150]
    result = getSubseries(series, 600)

    expect(result).toBe([100,100,100,100,100,100])
})

it("works when the subseries is equal to the threshold and at the end of the series", function(){
    series = [600,150,150,150,150,100,100,100,100,100,100]
    result = getSubseries(series, 600)

    expect(result).toBe([100,100,100,100,100,100])
})

These test that the subseries is correctly extracted if it's at the beginning or end of the series. Chris had these tests in place for his own code, but neglected to have the single-element test.

And [phew] my version passes all these tests still.

Back to Chris's code: other than the slight logic error, the only thing I'd observe is from a clean code perspective. The first big comment is unnecessary in my view: the code speaks for itself. On the other hand the second comment is necessary because the code doesn't speak for itself: the variables a and z are very vague. If they had decent names: subseriesStartIndex, subseriesEndIndex then the code would speak for itself, and the comment would not be necessary. I'm also not sure having the separate buffer variable is needed. I had to survey the code to work out why it existed, and in the end it serves only as a container for two other variables (and the sum variable is probably not needed anyhow, although I can see a case both ways for this). If I was to be using intermediary variables to make the code clearer, I'd perhaps change this:

buffer.sum   = arraySum( buffer.array );

if( buffer.sum <= threshold && arrayLen(buffer.array) > arrayLen(subseries) ){
    subseries = buffer.array;
}

To be this:

var withinThreshold = arraySum(buffer.array) <= threshold;
var betterLength = arrayLen(buffer.array) > arrayLen(subseries);

if (withinThreshold && betterLength){
    subseries = buffer.array;
}

But, realistically, there's not much in it.

The thing to remember here is that if you find yourself writing a big narrative comment: your code probably needs clarification or refactoring. Try not to have comments in your code: "Comments in code: avoid if possible".


OK, that's enough. Next up is Dave White's Python effort. I'm perhaps not going to be so analytical about Dave's effort as I do not know python from a bar of soap. But perhaps this will be my "in" to examine it a bit.

It'll take me a coupla days to survey Dave's code, so hopefully I'll get back to you on Saturday with some feedback. Oh... there's rugby on Saturday (Ireland v Aussie). Maybe Sunday then.

Righto.

--
Adam