Function for checking leap years As part of one of my AP Computer Science labs, I was required to write a function to determine whether a year was a leap year or not. Included as comments are a few reasons why I did things, as well as a more detailed version of the rules for leap years. Is there anything I could do better or have forgotten?

/**
 * Returns if the current year is a leap year
 */
private boolean isLeapYear()
{
    // No leap years occured before 1582 because that is when they were added to the calendar
    if(year > 1582)
    {
        // All code past here only executed when year mod 4 is true
        if(year % 4 != 0)
        {
            return false;
        }

        // Makes sure that it does not register the beginning of centuries as leap years
        // unless they actually are.
        else if(year % 100 == 0 && year % 400 != 0)
        {
            return false;
        }
        // Also is the same as checking for mod 4 and mod 100, since they are factors of 400
        else if(year % 400 == 0)
        {
            return true;
        }
        else
        {
            return true;
        }

        /*
        • A year that is not divisible by 4. - NOT LEAP YEAR

        • A year that is divisible by 4 but not divisible by 100 or 400. - LEAP YEAR

        • A year that is divisible by 4 and 100 but not 400. - NOT LEAP YEAR

        • A year that is divisible by 4 and 100 and 400. - LEAP YEAR

        • A year prior to 1582 that is a leap year. - NOT A LEAP YEAR

        • A year prior to 1582 that is not a leap year. - NOT A LEAP YEAR */
    }
    return false;
}

This question of leap years is an interesting one... and seems to be a rite-of-passage (when you do Zeller's Congruence, you should get that reviewed too... ;-)

In Java, (and other languages), it is common to return-early. You can use this to your advantage to reduce code-nesting, by choosing whether to do a negative, or positive test in if conditionals. You have done this a couple of times in your code, but not always. For example, your code contains:

// No leap years occured before 1582 because that is when they were added to the calendar
if(year > 1582)
{
    // All code past here only executed when year mod 4 is true
    if(year % 4 != 0)
    {
        return false;
    }

You have a lot of code inside that "if" condition block, but, if you reverse the logic:

// No leap years occured before 1582 because that is when they were added to the calendar
if(year <= 1582) {
    return false;
}

// All code past here only executed when year mod 4 is true
if(year % 4 != 0)
{
    return false;
}

You have immediately 'un-nested' a large portion of your code, which makes readability better.

When you write code to match a specification, like you have here, it is valuable to include the specification you used inside your code, which, you have also done... but, do that before the code, not after it.

If you have an if-else block, and the if-side (the true side) always does a return, then there is no need for the else. This can simplify the code further....

Performing these changes, we get to:

private boolean isOPLeapYear() {
    /*
     • A year that is not divisible by 4. - NOT LEAP YEAR
     • A year that is divisible by 4 but not divisible by 100 or 400. - LEAP YEAR
     • A year that is divisible by 4 and 100 but not 400. - NOT LEAP YEAR
     • A year that is divisible by 4 and 100 and 400. - LEAP YEAR
     • A year prior to 1582 that is a leap year. - NOT A LEAP YEAR
     • A year prior to 1582 that is not a leap year. - NOT A LEAP YEAR
     */

    // No leap years occurred before 1582 because that is when they were
    // added to the calendar
    if (year <= 1582) {
        return false;
    }
    // All code past here only executed when year mod 4 is true
    if (year % 4 != 0) {
        return false;
    }

    // Makes sure that it does not register the beginning of centuries as
    // leap years
    // unless they actually are.
    if (year % 100 == 0 && year % 400 != 0) {
        return false;
    }
    // Also is the same as checking for mod 4 and mod 100, since they are
    // factors of 400
    if (year % 400 == 0) {
        return true;
    }
    return true;

}

Once we have the code like that, it is apparent that the last % 400 check is unnecessary, and since the comments are repetitive (the // comments duplicate much of what the spec and code say), they can be mostly removed, reducing the code to:

private boolean isOPLeapYear() {
    /*
     • A year that is not divisible by 4. - NOT LEAP YEAR
     • A year that is divisible by 4 but not divisible by 100 or 400. - LEAP YEAR
     • A year that is divisible by 4 and 100 but not 400. - NOT LEAP YEAR
     • A year that is divisible by 4 and 100 and 400. - LEAP YEAR
     • A year prior to 1582 that is a leap year. - NOT A LEAP YEAR
     • A year prior to 1582 that is not a leap year. - NOT A LEAP YEAR
     */

    if (year <= 1582) {
        return false;
    }

    if (year % 4 != 0) {
        return false;
    }

    if (year % 400 == 0) {
        return true;
    }

    if (year % 100 == 0) {
        return false;
    }

    return true;

}

The above code is the same logic as yours, but.... more readable, that's all.


2048 Pythonic Merge Function This post goes over how I updated my 2048 merge function code from spaghetti code to being somewhat more readable.

Spaghetti Old Code

I incorporated a few techniques from pretty much all of your suggestions! I realized there were a lot of logical mistakes and redundancy in my code, like why append 0's over and over again when it's not necessary at all, because in the end I back-track, account for missing numbers, and then append 0(s) in their place.

Improvements in new code vs. old code:

Use of list comprehensions
Use of Python's built-in truthiness
Minimal branches (fewer if(s), else(s), etc.)
Infusing new and useful methods like extend into my code
Improving the code style by enhancing readability
Removing redundandcy and flawed logic

def merge(nums):
    '''
    Takes a list as input
    returns merged pairs with
    non zero values shifted to the left.
    fancy interactive doc test below, no output means no problems.
    >>> merge([2, 0, 2, 4])
    [4, 4, 0, 0]
    >>> merge([0, 0, 2, 2])
    [4, 0, 0, 0]
    >>> merge([2, 2, 0, 0])
    [4, 0, 0, 0]
    >>> merge([2, 2, 2, 2, 2])
    [4, 4, 2, 0, 0]
    >>> merge([8, 16, 16, 8])
    [8, 32, 8, 0]
    '''
    slide = [num for num in nums if num]
    pairs = []
    for idx, num in enumerate(slide):
        if idx == len(slide)-1:
            pairs.append(num)
            break
        elif num == slide[idx+1]:
            pairs.append(num*2)
            slide[idx+1] = None
        else:
            pairs.append(num)  # Even if not pair you must append
    slide = [pair for pair in pairs if pair]
    slide.extend([0] * (len(nums) - len(slide)))
    return slide

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

Sidenote: Pylint yells at me for using i instead of idx.

I didn’t review or see your old code when it was posted, but this is noticeably improved. However, I can still make a few suggestions:

num and nums are not great variable names. Characters are cheap; just write out the word number in full. I would also suggest giving numbers a slightly different/longer name to make it easier to read the line where you define slide.

Likewise pair/pairs could be a little easier to read, I think.
A few comments on the different branches to explain why you’re behaving in a particular way would be useful – to somebody unfamiliar with the rules of 2048, it might not be immediately obvious why it behaves the way it does.
A description of the doctest doesn’t belong in the function docstring, it probably belongs in a comment on the main() function. If I import your function into another file and then inspect merge.__doc__, the mention of a doctest “below” could be confusing.
Having “no output” mean success could be a problem, because there’s no way for me to distinguish between a success and the test not running. It should be totally clear whether your test passed, failed, or didn’t run. Anything else can hide problems.


Multiple DB record creation after a successful operation I am working on a payment process. Upon a successful payment process, I have to create multiple records. For that, I have made this class:

class DatabaseMaintainer

  def initialize(merchant, card, payment_method)
    @merchant = merchant
    @card = card
    @payment_method = payment_method
  end

  def maintain_records(customer, payment_details, response)
    rate = RateCatalog.get_rate(@merchant.id, @card.id, @payment_method.id)
    amount =  payment_details['total'].to_d
    commission = rate.rate *  amount / 100
    payable = amount - commission
    customer.organization_id = @merchant.id
    customer.save

    cash_transaction = CashTransaction.create!(
        amount: amount,
        receipt_id: response.authorization,
        rate_catalog_id: rate.id,
        customer_id: customer.id,
        redirect_url: payment_details['redirect_url']
    )

    Settlement.create!(
        commission_amount: commission,
        payable_amount: payable,
        cash_transaction_id: cash_transaction.id
    )

    invoice = Invoice.create!(
                    invoice_number: "#{CommonConstants::INVOICE_NUMBER_PREFIX}#{@merchant.abbreviation}_#{payment_details['invoice_id']}",
                    total: amount,
                    status: Invoice.statuses[:paid],
                    organization_id: @merchant.id,
                    cash_transaction_id: cash_transaction.id,
                    description:  payment_details['description']
    )

    item_details = payment_details['items']
    item_details.each do  |x|
      x[:bill_id] = invoice.id
      x[:bill_type] = 'Invoice'
    end

    PurchasedGood.create!(item_details)
  end
end

And I am using it as follows:

DatabaseMaintainer.new(@merchant, card_brand, payment_method).maintain_records(@customer, invoice_details, gateway_reply[:message])

Now, even if the code works fine, it clearly is not an optimal design.

I am looking for suggestions to refactor it. Also, is having a class for creating DB records solely justified in my scenario?

My Suggestions are

If this class is not going to be instantiated you can remove the
      initialize method and even make it a module
Now you donot need to use instance variables , local variable passed via
params    work here
According to the book 'Object Oriented Design in Ruby - By Sandi Metz' a method need not know the order of
params and even    the
      ClassNames it is going to create objects from. Rather you can pass
      Hash of parameters and you can use like

Advantage: You can pass params in any order

def maintain_records(param)
  # set fallback/default params
  param.merge(default_params)
  customer, payment_details, response = param[:customer], param[:payment_details], param[:response]


List all checked boxes from webpage This is part of a Tampermonkey script that generates a box in the top right of a webpage and generates a list of checked boxes from the specified list selector (list chosen through buttons). The script works as expected; however, I am not a web guy and do not pretend to be one. My main source of contention is how poorly designed the UI is. Any advice on improving the code and UI would be much appreciated.

var zNode       = document.createElement ('div');
zNode.innerHTML = '
Create List of Items

Not SelectedSelected
'  +
    '<button id="hospNotSelectBtn" type="button">Hospitals</button>'                   +
    '<button id="hospSelectBtn" type="button">Hospitals</button>
'                               +
    '<button id="deptNotSelectBtn" type="button">Departments</button>'                 +
    '<button id="deptSelectBtn" type="button">Departments</button>
'                             +
    '<button id="jobNotSelectBtn" type="button">Job Titles</button>'                   +
    '<button id="jobSelectBtn" type="button">Job Titles</button>
'                               +
    '<button id="jobFunctionNotSelectBtn" type="button">Job Functions</button>'        +
    '<button id="jobFunctionSelectBtn" type="button">Job Functions</button>'
;
zNode.setAttribute ('id', 'myContainer');
document.body.appendChild (zNode);

GM_addStyle ( multilineStr ( function () {/*!
    #myContainer {
        position:               absolute;
        top:                    0;
        right:                  0;
        font-size:              20px;
        color:                  white;
        background:             green;
        border:                 3px outset black;
        margin:                 2px;
        padding:                2px 2px;
        text-align:             center;
    }
    #myContainer span {
        margin:                 0 10px;
    }
*/} ) );

//Function provided by Brock Adams of SE
function multilineStr (dummyFunc) {
    var str = dummyFunc.toString ();
    str     = str.replace (/^[^\/]+\/\*!?/, '') // Strip function () { /*!
        .replace (/\s*\*\/\s*\}\s*$/, '')   // Strip */ }
        .replace (/\/\/.+$/gm, '') // Double-slash comments wreck CSS. Strip them.
    ;
    return str;
}

In order to simplify the user interface I switched to using a drop down list. It has minimized the amount of visual interference the original script had on the webpage as well as streamlined the code.

var zNode       = document.createElement ('div');
zNode.innerHTML = `
Selection Report
<select name='selection' id='numbers'">
    <option value="Choose" selected="selected">Choose Option</option>
    <option value="#ctl00_CPH_lsD_pnlListSelector input:checked">Departments Selected</option>
    <option value="#ctl00_CPH_lsD_pnlListSelector input:not(:checked)">Departments Not Selected</option>
    <option value="#ctl00_CPH_lstSelectorPosition3_pnlListSelector input:checked">Hospitals Selected</option>
    <option value="#ctl00_CPH_lstSelectorPosition3_pnlListSelector input:not(:checked)">Hospitals Not Selected</option>
    <option value="#ctl00_CPH_lsJT_pnlListSelector input:checked">Job Titles Selected</option>
    <option value="#ctl00_CPH_lsJT_pnlListSelector input:not(:checked)">Job Titles Not Selected</option>
    <option value="#ctl00_CPH_lsJF_pnlListSelector input:checked">Job Functions Selected</option>
    <option value="#ctl00_CPH_lsJF_pnlListSelector input:not(:checked)">Job Functions Not Selected</option>
</select>`;

zNode.setAttribute ('id', 'myContainer');
document.body.appendChild (zNode);

GM_addStyle ( `
    #myContainer {
        position:               absolute;
        top:                    0;
        right:                  0;
        font-size:              20px;
        color:                  white;
        background:             green;
        border:                 3px outset black;
        margin:                 2px;
        padding:                2px 2px;
        text-align:             center;
    }
    #myContainer span {
        margin:                 0 10px;
    }
    #hospNotSelectBtn {
        cursor:                 pointer;
    }
    #hospSelectBtn {
        cursor:                 pointer;
    }
    #deptNotSelectBtn {
        cursor:                 pointer;
    }
    #deptSelectBtn {
        cursor:                 pointer;
    }
    #jobSelectBtn {
        cursor:                 pointer;
    }
    #jobNotSelectBtn {
        cursor:                 pointer;
    }
    #jobFunctionSelectBtn {
        cursor:                 pointer;
    }
    #jobFunctionNotSelectBtn {
        cursor:                 pointer;
    }
    #myAlertBox {
        font-size: small;
        background: white;
        border: 5px solid green;
        padding: 4px;
        position: absolute;
        top: 280px; right: 80px;
        max-width: 300px;
        white-space:pre-wrap;
    }`);


Guttag's Finger Exercises: Enter 10 integers, find largest odd integer and print result I'm making my way through the Intro to Computation and Programming test by John Guttag and he gives these small exercises to practice gearing up into computational thinking. As of Ch.2, we've covered basic input and print commands as well as some simple iteration stuff.

The question is as follows:

  Write a piece of code that asks the user to input 10 integers, and
  then prints the largest odd number that was entered. If no odd number
  was entered, it should print a message to that effect.

counter = 0
ans = 0
even=0
while counter < 10:
    x=int(raw_input('Please enter an integer: '))
    if x%2!=0:          #Checks if the input is odd
        if ans < x:     #If it is, is the input larger than the held value for 'ans'?
            ans = x     #If yes, replace the value for 'ans' with the input
            counter +=1 #Move the counter up by one, ending at 10
    else:               #If it wasn't odd, add 1 to the even counter and to the integer counter, keeping ans the same
        even+=1
        counter+=1
if even == 10:          #if all numbers entered were even, this counter would be 10, and thus would give this response
    print 'All numbers were even'
else:                   #if at least 1 odd number was entered, the even counter would not be 10 and it would print the answer
    print 'the largest odd number entered was ', ans

Is there a more efficient way of writing this out? I wonder if I can word my if statements to include more parameters to cut down on the number of if/else statements? Also keep in mind this text uses python 2.7 and that I'm not super far into the book. I'm mainly trying to get the whole computational thinking thing down.

Python 2.x vs. Python 3.x

As a start, welcome to code review and since this is your first time here, well done, you did a good job here for a first post. One of the most confusing topics for beginner programmers is to choose which version of Python to learn. However there are more benefits to learn Python 3.x since it's the current version of Python and very soon there will be no future security or bug fixes for Python 2.x. and most of the developers nowadays are using Python 3.x, and as I'm familiar with the book you indicated, I know it's written in Python 2.x therefore I suggest you try to look things up and try to apply the same logic you encounter in exercises in Python 3.x (you will just be using a slightly different syntax).

Besides Python 3.x contains a bunch of new features that you'll eventually start using:

the print function
f strings
integer division
and many other features

Style

Here's a reference style guide (pep8) https://www.python.org/dev/peps/pep-0008/, the official Python guide to style you could read and here are a few comments about style:

If an assignment has a right hand side, then the equality sign should have exactly one space on both sides.

even=0

x=int(raw_input('Please enter an integer: '))

if x%2!=0:
Use inline comments sparingly.
An inline comment is a comment on the same line as a statement.
Inline comments should be separated by at least two spaces from the
statement. They should start with a # and a single space.
Use f-strings for better formatting.

ex: print(f'largest odd: {odd_num}')

The if elif else statements:

An else statement can be combined with an if statement. An else statement contains the block of code that executes if the conditional expression in the if statement resolves to 0 or a FALSE value.

The else statement is an optional statement and there could be at most only one else statement following if.

The use of functions and lists:

You can use functions in programming to bundle a set of instructions that you want to use repeatedly or that, because of their complexity, are better self-contained in a sub-program and called when needed. That means that a function is a piece of code written to carry out a specified task. To carry out that specific task, the function might or might not need multiple inputs. When the task is carried out, the function can or can not return one or more values.

You can also use lists when you have more than a few items to do operations on.
A list is a data structure in Python that is a mutable, or changeable, ordered sequence of elements. Each element or value that is inside of a list is called an item. Just as strings are defined as characters between quotes, lists are defined by having values between square brackets [ ]

The code can be simplified as follows:

def get_odd_max(n):
    """Return maximum odd number from n numbers, print a message for no odds"""
    odds = []
    evens = []
    for _ in range(n):
        # the use of try and except to catch illegal types without stopping the code execution.
        try:
            number = int(input('Enter an integer: '))
            if number % 2 == 0:
                evens.append(number)
            if number % 2 != 0:
                odds.append(number)
        except ValueError:
            print('Expected int.')
    if odds:
        # f-strings for a cleaner code
        # usually a function does not print anything(it's for the sake of the example)
        print(f'Maximum odd value: {max(odds)}')
        return max(odds)
    else:
        print('No odds found.')
        return 0

if __name__ == '__main__':
    print(get_odd_max(10))


Detecting if image taken by camera is too dark I am trying to build functionality to check if an image taken by the camera is too dark. So I added the following 2 functions to a utility class:

public static boolean checkIfImageIsTooDark(BitmapRegionDecoder bitmapRegionDecoder)
{
    if(bitmapRegionDecoder == null) {
        Timber.e("Expected bitmapRegionDecoder was null");
        return false;
    }

    int loadingBitmapHeight = bitmapRegionDecoder.getHeight();
    int loadingBitmapWidth = bitmapRegionDecoder.getWidth();

    boolean isImageTooDark = true;

    int checkBitmapWidth = loadingBitmapWidth / 10;
    int checkBitmapHeight = loadingBitmapHeight / 10;
    int topBitmapPosition = 0;
    int leftBitmapPosition = 0;

    while((checkBitmapWidth <= loadingBitmapWidth) && (leftBitmapPosition < checkBitmapWidth)) {
        while((checkBitmapHeight <= loadingBitmapHeight) && (topBitmapPosition < checkBitmapHeight)) {
            Rect bitmapRect = new Rect(leftBitmapPosition,topBitmapPosition,checkBitmapWidth,checkBitmapHeight);
            Timber.d("left: " + leftBitmapPosition + " right: " + checkBitmapWidth + " top: " + topBitmapPosition + " bottom: " + checkBitmapHeight);

            Bitmap processBitmap = bitmapRegionDecoder.decodeRegion(bitmapRect,null);

            if(!checkIfImageIsTooDark(processBitmap)) {
                isImageTooDark = false;
                break;
            }

            topBitmapPosition = checkBitmapHeight;
            checkBitmapHeight += (checkBitmapHeight < (loadingBitmapHeight - checkBitmapHeight)) ? checkBitmapHeight : (loadingBitmapHeight - checkBitmapHeight);
        }

        if(!isImageTooDark) {
            break;
        }

        topBitmapPosition = 0;
        checkBitmapHeight = loadingBitmapHeight / 10;
        leftBitmapPosition = checkBitmapWidth;
        checkBitmapWidth += (checkBitmapWidth < (loadingBitmapWidth - checkBitmapWidth)) ? checkBitmapWidth : (loadingBitmapWidth - checkBitmapWidth);
    }

    return isImageTooDark;
}

public static boolean checkIfImageIsTooDark(Bitmap bitmap) {
    if(bitmap == null)
    {
        Timber.e("Expected bitmap was null");
        return false;
    }

    int[] brightnessHistogram = new int[256];

    for(int i = 0; i < bitmap.getHeight(); i++) {
        for(int j = 0; j < bitmap.getWidth(); j++) {
            int pixel = bitmap.getPixel(j,i);

            int r = Color.red(pixel);
            int g = Color.green(pixel);
            int b = Color.blue(pixel);

            int brightness = (int)(0.2126 * r + 0.7152 * g + 0.0722 * b);
            brightnessHistogram[brightness]++;
        }
    }

    int allPixelsCount = bitmap.getWidth() * bitmap.getHeight();

    //counting pixels with brightness less than 10
    int darkPixelCount = 0;
    for(int i = 0; i < 10; i++)
    {
        darkPixelCount += brightnessHistogram[i];
    }

    //if more than 70% pixels are too dark then image is too dark
    return darkPixelCount > allPixelsCount * 0.70;
}

The brightness histogram approach I found over here -> https://stackoverflow.com/a/35914745/3287853. And it works quite well.

The only problem however is that processing an image takes way too much time. I tried it on my phone which produces 4160x3120 and it takes close to a minute to get a result off it. Also, the 4 loops don't look great and I think there should be a better way to make this work, personally.

I am using an AsyncTask to do this in the background so one improvement that I could think of is to break up the task across multiple threads.

I could also compress the image initially bringing it down to a certain limit of width and height while attempting to maintain its aspect ratio and then process the image. However I wonder if that would actually help in reducing the amount of time taken at all.

Sampling: Don't process ALL pixels

The speed mainly depend on the number of operations, in our case, pixels. And a lot of pixels. This means that the larger the image is, the slower your code will run.

You probably don't need to process all pixels, just a representative subset of them.

Taking this subset is called sampling. You can for example only process every 100th pixel.

If you want to go more fancy, try Monte Carlo sampling.

If you really need all the pixels, check OpenCV, that is optimized for image processing.


Filtering query by date built from integers I am trying to make a stored procedure and pass some parameters for filtering. As soon as filtering is in horizon, code has to be as generic as possible with as few as possible if, else, etc.

How could I make this more readable? Can I use functions for this?

ALTER PROCEDURE [dbo].[Stats] @network VARCHAR(9) = NULL,
                                               @name    VARCHAR(100) = NULL,
                                               @version VARCHAR(15) = NULL,
                                               @year    INT = NULL,
                                               @month   INT = NULL,
                                               @day INT = NULL,
                                               @from DATE = null,
                                               @to DATE = null
AS
  BEGIN
      SET nocount ON;

          DECLARE @fromYear INT = NULL;
      SET @fromYear = CASE WHEN @from IS NOT NULL THEN Datepart(year, @from) END

          DECLARE @fromMonth INT = NULL;
      SET @fromMonth = CASE WHEN @from IS NOT NULL THEN Datepart(month, @from) END

          DECLARE @fromDay INT = NULL;
      SET @fromDay = CASE WHEN @from IS NOT NULL THEN Datepart(day, @from) END

          DECLARE @toYear INT = NULL;
      SET @toYear = CASE WHEN @to IS NOT NULL THEN Datepart(year, @to) ELSE @fromYear END

          DECLARE @toMonth INT = NULL;
      SET @toMonth = CASE WHEN @to IS NOT NULL THEN Datepart(month, @to) ELSE @fromMonth END

          DECLARE @toDay INT = NULL;
      SET @toDay = CASE WHEN @to IS NOT NULL THEN Datepart(day, @to) ELSE @fromDay END

      SELECT GS.[contractaddress],
             GS.[network],
             GS.[rounds],
             GS.[sessions],
             GS.[handle],
             GS.[hold],
             GS.[year],
             GS.[month],
             GS.[day],
             G.[name],
             G.[version],
             DATEFROMPARTS(GS.[Year], GS.[Month], GS.[Day]) as [Date]
      FROM   [dbo].[gamestatsdaily] AS GS
             INNER JOIN [dbo].[game] AS G
                     ON GS.contractaddress = G.[contractaddress]
      WHERE  ( @network IS NULL
                OR ( GS.network = Upper(@network) ) )
             AND ( @name IS NULL
                    OR ( GS.[contractaddress] IN (SELECT [contractaddress]
                                                  FROM   [dbo].[game]
                                                  WHERE  [name] = @name
                                                         AND [version] =
                                                             @version
                                                         AND [network] =
                                                             @network)
                       ) )
             AND ( @year IS NULL OR GS.[Year] = @year )
             AND ( @month IS NULL OR GS.[Month] = @month )
             AND ( @day IS NULL OR GS.[Day] = @day )
             AND ( @from IS NULL OR ( GS.[Year] <= @fromYear AND @toYear >= GS.[Year] AND GS.[Month] <= @fromMonth AND @toMonth >= GS.[Month] AND GS.[Day] <= @fromMonth AND @toMonth >= GS.[Day]))

      ORDER  BY GS.[year] ASC,
                GS.[month] ASC,
                GS.[day] ASC
  END

Here is the revised SQL I came up with

Below this code is a list of tips for writing better SQL.

ALTER PROCEDURE [dbo].[Stats]
      @network VARCHAR(9) = NULL
    , @name VARCHAR(100) = NULL
    , @version VARCHAR(15) = NULL
    , @year INT = NULL
    , @month INT = NULL
    , @day INT = NULL
    , @from DATE = NULL
    , @to DATE = NULL
AS

BEGIN

    BEGIN TRY

        SET NOCOUNT ON;

        WITH
        contract_address_list
        AS
        (
            SELECT DISTINCT
                [contractaddress]
            FROM
                [dbo].[game]
            WHERE
                1=1
                AND [name] = @name
                AND [version] = @version
                AND [network] = @network
        )
        SELECT
              GS.[contractaddress]
            , GS.[network]
            , GS.[rounds]
            , GS.[sessions]
            , GS.[handle]
            , GS.[hold]
            , GS.[year]
            , GS.[month]
            , GS.[day]
            , G.[name]
            , G.[version]
            , [Date] = DATEFROMPARTS(GS.[Year], GS.[Month], GS.[Day])
        FROM
            [dbo].[gamestatsdaily] AS GS
            INNER JOIN [dbo].[game] AS G ON GS.[contractaddress] = G.[contractaddress]
            LEFT JOIN contract_address_list AS CAL ON CAL.[contractaddress] = GS.[contractaddress]
        WHERE
            1=1
            AND (@name IS NULL OR CAL.[name] IS NOT NULL)
            AND (@network IS NULL OR (GS.network = UPPER(@network)) )
            AND (@year IS NULL OR GS.[Year] = @year )
            AND (@month IS NULL OR GS.[Month] = @month )
            AND (@day IS NULL OR GS.[Day] = @day )
            AND (
                @from IS NULL
                OR (
                        GS.[Year] >= DATEPART(year, @from)
                    AND GS.[Year] <= COALESCE(DATEPART(year, @to), DATEPART(year, @from))
                    AND GS.[Month] >= DATEPART(month, @from)
                    AND GS.[Month] <= COALESCE(DATEPART(month, @to), DATEPART(month, @from))
                    AND GS.[Day] >= DATEPART(day, @from)
                    AND GS.[Day] <= DATEPART(day, @to)
                    )
                )
        ORDER BY
              GS.[year] ASC
            , GS.[month] ASC
            , GS.[day] ASC

    END TRY
    BEGIN CATCH
        SELECT
                [ErrorNumber] = ERROR_NUMBER()
            , [ErrorSeverity] = ERROR_SEVERITY()
            , [ErrorState] = ERROR_STATE()
            , [ErrorProcedure] = ERROR_PROCEDURE()
            , [ErrorLine] = ERROR_LINE()
            , [ErrorMessage] = ERROR_MESSAGE();
    END CATCH

END

Tips for your SQL

For formatting, I would download the following tool, Poor Man's T-Sql Formatter and on GitHub. I use it when I have to edit other developer's code.

  Here are the settings I used

I would put the commas in front to clearly define new columns. Versus code wrapped in multiple lines. It also makes trouble-shooting code easier.
If you put 1=1 at the top of a WHERE condition, it enables you to freely change the rest of the conditions when debugging a query. The SQL query engine will end up ignoring the 1=1 so it should have no performance impact. Reference
If you use common table expressions (CTE) in your SQL it helps with documentation. The expression name can then let other developers know why you used that expression e.g. contract_address_list or active_projects.
Use the COALESCE function to evaluate the expression in the WHERE clause for the DATEPARTS.
I find it more convenient to see column alias's referenced from the left side e.g. , [Date] = DATEFROMPARTS(GS.[Year], GS.[Month], GS.[Day])
Avoid using keywords as object names. Microsoft Reference
Check the Estimated Execution Plan Ctrl + L in Microsoft SQL Server Management Studio (SSMS) to check the performance of the query by pasting both SQL statements in a query window.

Here's an example.


CodeEval Interrupted Bubble Sort In interrupted bubble sort, we need to stop the sorting based on the iteration count. This seems pretty straightforward to me. I have this code, which seems to work fine, but it isn't fast enough for larger input sets. How can I make this faster?

s = '36 47 78 28 20 79 87 16 8 45 72 69 81 66 60 8 3 86 90 90 | 2'
ls = s.split('|')
l = [int(n) for n in ls[0].split(' ') if n.isdigit()]
icount = int(ls[1].strip())
print "list is", l, len(l)
print "count is ", icount

ic = 0
while ic < icount:
    i = 0
    while i < len(l) - 2:
        if l[i + 1] < l[i]:
            tup = l[i], l[i + 1]
            l[i + 1], l[i] = tup
        i += 1
    ic += 1
print ' '.join(str(i) for i in l)

It is not fast enough for larger input sets. How can I make this faster?

First, move these into well-defined functions:

def bubble(l):
    i = 0
    while i < len(l) - 2:
        if l[i + 1] < l[i]:
            tup = l[i], l[i + 1]
            l[i + 1], l[i] = tup
        i += 1

def partial_bubble_sort(l, iterations):
    ic = 0
    while ic < icount:
        bubble(l)
        ic += 1

def main():
    s = '36 47 78 28 20 79 87 16 8 45 72 69 81 66 60 8 3 86 90 90 | 2'
    ls = s.split('|')
    l = [int(n) for n in ls[0].split(' ') if n.isdigit()]
    icount = int(ls[1].strip())
    print("list is", l, len(l))
    print("count is ", icount)

    partial_bubble_sort(l, icount)
    print(' '.join(str(i) for i in l))

main()
#>>> list is [36, 47, 78, 28, 20, 79, 87, 16, 8, 45, 72, 69, 81, 66, 60, 8, 3, 86, 90, 90] 20
#>>> count is  2
#>>> 36 28 20 47 78 16 8 45 72 69 79 66 60 8 3 81 86 87 90 90

Your while i < len(l) - 2 should be while i <= len(l) - 2; currently you are missing the last element.

Change the i = 0; while i < x; i += 1 loops to for i in range(x).

Your tup = l[i], l[i + 1]; l[i + 1], l[i] = tup would be faster on one line; CPython won't optimize out the intermediate for you.

Since you only want to split the string into two, I recommend:

in_string = '36 47 78 28 20 79 87 16 8 45 72 69 81 66 60 8 3 86 90 90 12 | 2'

numbers, sep, iterations = in_string.rpartition('|')
assert sep

numbers = [int(n) for n in numbers.split()]
iterations = int(iterations.strip())

...although I don't see why you're not just doing

numbers = [36, 47, 78, 28, 20, 79, 87, 16, 8, 45, 72, 69, 81, 66, 60, 8, 3, 86, 90, 90, 12]
iterations = 2

Anyhow, I'm finding this takes 30-50% less time than the original.

There are still more optimizations you can do. This is a more efficient bubble which avoids shifts by holding the moving element. This can be further improved by using enumerate to avoid the indexing.

def bubble(lst):
    held, rest = lst[0], lst[1:]

    for i, v in enumerate(rest):
        if v > held:
            v, held = held, v

        lst[i] = v

    lst[-1] = held

Each pass of bubble, one more element on the end of the array is already sorted. This means we can give bubble a paramenter of how many elements to skip.

def bubble(lst, skip):
    held, rest = lst[0], lst[1:len(lst)-skip]

    for i, v in enumerate(rest):
        if v > held:
            v, held = held, v

        lst[i] = v

    lst[-skip-1] = held

def partial_bubble_sort(lst, iterations):
    for skip in range(iterations):
        bubble(lst, skip)

You need len(lst)-skip here instead of just -skip since skip could be 0.

This explicitly checks lst[0], so you should cap iterations in partial_bubble_sort to len(lst) - 1 to prevent it ever being called for empty lists (or do a check in the function itself):

def partial_bubble_sort(lst, iterations):
    for skip in range(min(iterations, len(lst)-1)):
        bubble(lst, skip)

This should all end up several times (4-5) as fast as the original.


Review my "delayed for-loop" in JavaScript I have written a small JavaScript class that makes use of setInterval to create a delayed loop for iterating an array. I've used this technique in the past but never utilised a library to do so (and as a result it produced quite messy spaghetti code), and so I wrote up this:

var DelayedLoop = function() { };

DelayedLoop.forEach = function(collection, delay, callback, completedCallback) {
    var index = 0;
    var timerObject;

    var executor = function() {
        // Stop the delayed loop.
        clearInterval(timerObject);

        // Executes the callback, and gets the returned value to indicate what the next delay will be.
        var newInterval = callback(collection[index++]);

        // If they returned false, quit looping.
        if(typeof(newInterval) == "boolean" && newInterval == false) {
            return;
        }

        // If nothing / non-number is provided, re-use initial delay.
        if(typeof(newInterval) != "number") {
            newInterval = delay;
        } else if(newInterval < 0) { // If a negative number is returned, quit looping.
            return;
        }

        // If there are more elements to iterate, re-set delayed loop. Otherwise, call the "completed" callback.
        if(index < collection.length) {
            timerObject = setInterval(executor, newInterval);
        } else {
            completedCallback();
        }
    };

    // Initial loop setup.
    timerObject = setInterval(function() {
        executor();
    }, delay);
};

An example usage can be seen here:

$(document).ready(function() {
    var myArray = [ "aaa", "bbb", "ccc" ];
    DelayedLoop.forEach(myArray, 1000, function(arrayElement) {
        console.log(arrayElement);
    }, function() {
        console.log("Finished all!");
    });
});

How does it look? Are there any browser compatibility issues? Is there any potential flaw I'm not seeing?

My experience with Javascript isn't quite as developed as it is for other languages, but here's what I have to say about the code.

Why use setInterval() and clearInterval()?  You'd use those methods if you wanted to call a function repeatedly.  Since all you do is set it and immediately clear it, there's no point in really using it.  Better to use setTimeout() instead.

I'd consider rearranging your parameters so the delay was last.  That way you can make the delay optional, possibly provide a default delay instead.  Also consider making complatedCallback optional too.  It could be possible that you don't need the callback so you're forcing yourself to have to provide one.

I don't think you'd want to put a delay of the first invocation on the first item.  So probably should remove that initial timeout and just call the executor() directly.

DelayedLoop.forEach = function(collection, callback, completedCallback, delay) {
    var index = 0;

    var executor = function() {
        // Executes the callback, and gets the returned value to indicate what the next delay will be.
        var newInterval = callback(collection[index++]);

        // If they returned false, quit looping.
        if (typeof(newInterval) == "boolean" && newInterval == false) {
            return;
        }

        // If nothing / non-number is provided, re-use initial delay.
        if (typeof(newInterval) != "number") {
            newInterval = delay;
        } else if (newInterval < 0) { // If a negative number is returned, quit looping.
            return;
        }

        // If there are more elements to iterate, re-set delayed loop. Otherwise, call the "completed" callback.
        if (index < collection.length) {
            setTimeout(executor, newInterval);
        } else if (typeof(completedCallback) == "function") {
            completedCallback();
        }
    };

    // Initial loop setup.
    executor();
};

Otherwise I don't see anything glaringly wrong with the code.


Better option than "errno" for file IO error handling I have the following method for opening a file:

void TankFile::OpenForReading(const std::string & filename)
{
    assert(!filename.empty());

    errno = 0;
    file.exceptions(0); // Don't throw
    file.open(filename, (std::fstream::in | std::fstream::binary));

    if (!file.is_open() || !file.good())
    {
        const char * errorStr = strerror(errno);
        throw TankFileError(Format("Failed to open Tank file \"%s\": '%s'", filename.c_str(), errorStr));
    }
}

The objective here is to attempt to open a file and throw TankFileError with a proper error description on failure. The caller will be expecting this exception type.

Everything works fine and I get a nice error message like this if the exception is thrown:

  Failed to open Tank file "unexistent_file": 'No such file or directory'

But what I don't like in that block is having to use the errno global and strerror().

A way around it would be to let the stream throw an exception, then catch it, get the error message from the what() member and re-throw with TankFileError, but I find this solution also a bit hackish, plus, in the tests I did, the resulting error message from std::fstream::failure was pretty cryptic:

void TankFile::OpenForReading(const std::string & filename)
{
    assert(!filename.empty());

    try
    {
        file.exceptions(std::fstream::failbit);
        file.open(filename, (std::fstream::in | std::fstream::binary));
    }
    catch (std::fstream::failure & err)
    {
        throw TankFileError(Format("Failed to open Tank file \"%s\": '%s'", filename.c_str(), err.what()));
    }
}

Produced the error message:

  Failed to open Tank file "unexistent_file": 'ios_base::clear: unspecified iostream_category error'.

Is there a better way to implement this? I was hoping that the new C++11 system_error library would provide a way to query this kind of error messages, but from what I've seen, you still have to pass errno around to get an error string.

I currently don't see any seamless alternative to using errno and ::strerror;.

#include <stdexcept>
#include <system_error>
#include <string>
#include <iostream>

#include <cstring> // strerror

#include <fstream>

#if defined(_LIBCPP_VERSION) && (_LIBCPP_VERSION >= 1000)
#define HAS_IOS_BASE_FAILURE_DERIVED_FROM_SYSTEM_ERROR 1
#else
#define HAS_IOS_BASE_FAILURE_DERIVED_FROM_SYSTEM_ERROR 0
#endif

using std::cerr;
using std::cout;
using std::endl;

int main(/*int argc, char** argv*/) {
    int rv = EXIT_SUCCESS;
    errno = 0;

    try {
        std::ifstream ifs;
        ifs.exceptions(std::ios::badbit | std::ios::failbit);
        ifs.open("DOESN'T EXIST");

    } catch (const std::ios_base::failure& e) {

#if (HAS_IOS_BASE_FAILURE_DERIVED_FROM_SYSTEM_ERROR)
        //
        // e.code() is only available if the lib actually follows iso §27.5.3.1.1
        // and derives ios_base::failure from system_error
        // like e.g. libc++
        // http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/ios?revision=193085&view=markup
        // (line 415)
        //
        // and not keeps on deriving it directly from runtime_error
        // like libstdc++
        // https://github.com/mirrors/gcc/blob/master/libstdc%2B%2B-v3/include/bits/ios_base.h#L209
        //
        cout << "libc++ error #"    << e.code().value()
                << ' '              << e.code().message()
                << ',' << endl << "    ";
#endif
        cout << "libc error #"      << (rv = errno)
                << ": "             << ::strerror(errno)
                << endl;

        cout << "handled" << endl;
    }

    return rv;
}

using the the new e.code() semantics yields (with clang 3.4 and libc++ 1101) only the first line of

libc++ error #1 unspecified iostream_category error,
    libc error #2: No such file or directory
handled

Live

So even if one's lucky enough to have a std:: implementation that cares about iso §27.5.3.1.1 and actually derives ios_base::failure from system_error, the msg generated is too poor to be presented to users.

The only thing to be discussed is how the libc messages are best incorporated into wrapper classes.


Avoiding code duplication for attribute-removal code I need to use the same code in different jQuery attributes.  Is it possible to put this if in a external function and call it inside a change event?  I would like avoid duplicating code.

$("#Max").change(function () {
var max = $("#Max").val();
var min = $("#Min").val();

if (max != min) {
    $("#Result").attr("disabled", "disabled");
}
else {
    $("#Result").removeAttr("disabled");
}
});

$("#Min").change(function () {
var max = $("#Max").val();
var min = $("#Min").val();

if (max != min) {
    $("#Result").attr("disabled", "disabled");
}
else {
    $("#Result").removeAttr("disabled");
}
});

I think that @david-harkness code could be simplified further:

(function () {
    var $max = $("#Max"),
        $min = $("#Min"),
        $result = $("#Result"),
        changeHandler = function () {
            $result.prop("disabled", $max.val() != $min.val());
        };

    $max.change(changeHandler);
    $min.change(changeHandler);
}());

but I'm not really sure about the side effects of using prop instead of attr.


Haskell: Efficient and clear list concatenation I'm trying to write a function explodeBy:

explodeBy :: Int -> [Int] -> [Int]
explodeBy n arr = foldr (\x acc-> (take n.repeat) x ++ acc ) [] arr

This works as follows:

λ> explodeBy 2 [1,2,3]
[1,1,2,2,3,3]

The above uses list concatenation, so I tried writing a more efficient version:

explodeBy2 n arr = foldr (\x acc -> f n x acc) [] arr

f 0 _ rest = rest
f rep val rest = f  ( rep -1) val (val:rest)

Though this looks ugly, I expected it would work better. Reason: here we are using the (:) operator. Since rest is larger than (take n.repeat) x, I believe the second approach should have always been faster.

Surprisingly:

λ> sum $ explodeBy2 500 [2..1001]
250750000
(0.48 secs, 199527400 bytes)
λ> sum $ explodeBy 500 [2..1001]
250750000
(0.14 secs, 102259320 bytes)

explodeBy2 is consuming more time and memory!

Is there something wrong with this code or my understanding of (++)? Are there also any tips on how to write explodeBy better?

PS: This is where I got the impression ++ isn't efficient. Is this outdated?

GHCi is frequently significantly slower than compiled code even if you compile without any optimizations turned on, consider the timing information that GHCi gives you with great suspicion. Compile your code with -O2 and then time and the difference should largely disappear.

One of the nice things about the Haskell Prelude is that there is very little magic going on behind the scenes. Haskell lists use some privileged constructor names ((:) and []) but besides that they are a data type like any other. So let's peek behind the curtain and take a look at how (++) is defined.

(++) :: [a] -> [a] -> [a]
(++) []     ys = ys
(++) (x:xs) ys = x : xs ++ ys

You should recognize the similarity to your function f. There is no way around the fact that growing a list by n elements takes n cons operations. When you want to add one element to the front of a list, you use (:) and that's it, you're done. When you want to add one element to the back of a list there's no constructor for that, you have to consider that element to be a new list of length one, and prepend your original list in front of it, causing you to have to walk down every single element of the original list. That is “slower” than the other case, but if the ordering of elements in your list is important you gotta do what you've gotta do, right?

Take some time to familiarize yourself with the functions available in Prelude and Data.List. There are many easier ways to write explodeBy.

As always, consider the types and use Hoogle. Looking at take n . repeat $ x think about the type you would give that function on its own. n is an Int, and x is any type a. So you'd have a type like func :: Int -> a -> [a]. The first hit is for replicate, which does exactly what you want.

Next consider what easy intermediate values you can construct that get you closer to the result you want. The first step would be to replicate each element in the original list n times. That sounds like a job for map.

map (replicate n) xs :: [[a]]

Now we need :: [[a]] -> [a], which we could also search for. But this is of course list concatenation. There's one more trick you might not find by searching though, so here is when it comes in handy to have read through the documentation for Prelude. concatMap handles mapping and list concatenation in one go.

explodeBy :: Int -> [a] -> [a]
explodeBy n = concatMap (replicate n)


NULL-eating / elimination version of fgets() in C I've come up with the following routine to read data from a file stream, but I don't want any NULLs in the stream to terminate the string early.  This function will read in a "line", and allow it to be processed with normal C string functions.  Does anyone see any inherent problems or potential bugs in this routine?

char *my_fgets(char *Buf, int iMaxCount, FILE *fp)
{
    // Handle basic failures.
    if(!Buf) return NULL;
    if(iMaxCount < 1) return NULL;
    if(!fp) return NULL;

    // Create a buffer on the heap for our data to be read in
    char *lbuf = (char*)malloc(iMaxCount);
    char *p = lbuf;

    // Zero out the buffer, no stray chars please.
    memset(Buf, 0x00, iMaxCount);

    // Where are we at in the file?
    int b = ftell(fp);

    // Read in the buffer, up to iMaxCount-1.  Leave one char for a final NULL.
    size_t read = fread((void*)&lbuf[0], sizeof(char), iMaxCount-1, fp);
    lbuf[iMaxCount-1] = NULL;

    // Failed.
    if(!read) {
        free(lbuf);
        return NULL;
    }
    // Walk the buffer, looking for a newline char.  Once we find it, we know we are at the end of the line,
    // but we don't want to read past the number of bytes we originally read, in case we did not hit a newline.
    size_t left = read;
    while((left) && *(p++) != '\n') left--;

    // Copy the data we read to the return Buffer, up to the newline char.
    memcpy(Buf, lbuf, p - lbuf);
    // Set the file pointer to the newline char + 1
    fseek(fp, b+(p - lbuf), SEEK_SET);

    // Setup how many bytes we have left in the buffer to look at.
    left = (p - lbuf);
    p = Buf;

    while(left)
    {
        // Walk the buffer, looking for NULL chars.
        if(*(p++) == NULL) {
            char *p1;
            // We hit a NULL, so shift the buffer left, eat the NULL, and continue our search.
            for(p1 = p-1;p1 < (Buf + (iMaxCount-1));p1++) p1[0] = p1[1];
            // We ate one char, so we need to step back one byte, and continue our search.  This handles a case where two NULLs may be back to back.
            p--;
        }
        left--;
    }

    // If our last char is not a NULL, we need to set it to NULL to properly terminate the string.
    if((p > Buf) && *(p-1) != 0x0a && *p != NULL) *(--p) = 0x00;

    // Free our work buffer, and return the new 'clean' data.
    free(lbuf);
    return Buf;
}

It might of importance to note that this code is called by a Kernel driver, and MUST be fast.  Speed is totally of the essence here, which is why this is all in one function, and uses calls like fread() over fgets() to get a chunk of data in one shot.  That might not matter in the final analysis, but I thought I would mention it.

There's absolutely no need for an intermediate (dynamic) buffer. Everything you do can be done directly in the client buffer (Buf).

I don't see a reason for fread-ing data. fgets works as good, and spares you from calling ftell and fseek.

The way you remove null chars is suboptimal. I'd keep count of null chars encountered, and shift non-null chars by that count, in a single loop. In any case, I'd factor removal code into a separate function (No Raw Loops rule strikes again).

Looking for a newline is better be done with a standard memchr().

That said, the main code condenses to

char * fgets_and_remove_nulls(char * buf, size_t count, FILE * fp)
{
    memset(buf, 0, size);
    if(fgets(buf, size, fp) == NULL) return NULL;
    char * end = memchr(buf, '\n', size);
    if (end == NULL) end = buf + size;
    remove_null_chars(buf, end);
    return buf;
}


Finding roughly matching genome sequences in Python dictionary The purpose of my code here is to play a part in genome sequencing analysis, and while functional it takes days to run, so I am looking for any way I can improve speed. The input is up to 500 million lines long (making speed code efficiency important) and contains sequencing reads and corresponding info. Each read takes up 4 lines within the input file and looks something like this:

@A001             <-header
AAAAACCCCCCCCCCCC <-seq read (finalRead)
+
################# <-quality (trimmed_quality)

The portion of my code that is very slow takes a dictionary as input, which contains all of the data found within the input sequencing file and is in the form shown below:

duplexDict[umi] = {'header':header, 'seq':finalRead, 'qual':trimmed_quality}

In the first part of the code I am looking for pairs of sequences by checking for similar keys (termed umi in the code). The goal is to find keys that when converted to complement sequence are only different by a single letter. Then for each key if there is only one closely matching key, the associated dictionaries are retained. If there are no matches or more than one matching key, all of these keys should be ignored.

from Levenshtein import distance
deDuplexDict = {} # dict that will contain key pairs
finalList = [] # list to keep track of valid key pairs
for i in duplexDict: # dict with sequencing file info
    tempList = []
    for j in duplexDict:
        complement = str(Seq(j).complement()) # this is just finding complementary sequence
        if distance(i,complement) <= 1: # find similar umi/read seq pairs
            tempList.append(j) # make a list of all similar pairs
    # only keep a complementary pair if there are exactly two matching consensus reads
    if len(tempList) == 1:
        if i not in finalList and j not in finalList:
            finalList.append(i)
            finalList.append(j)

# only retain those dict values that are true pairs
for key in finalList:
    deDuplexDict[key] = duplexDict[key]

The second piece is designed to now collapse combine the sequences of two matching dictionary keys together and output to file. This is done by taking the complement of one of the sequences and then comparing each character position along the sequence strings. If anything doesn't match the character in a final string is just set to 'N' rather than the character found in the reads.

from itertools import combinations
prevScanned=[]
plus = '+'

# only pairs now exist, just search for them
for key1, key2 in combinations(deDuplexDict, 2):
    finalRead = ''
    complement = str(Seq(key2).complement()) # complement of second read sequence
    # if neither key has been analysed and they are a matching pair then use for consensus read
    if distance(key1, complement) <= 1 and key1 not in prevScanned and key2 not in prevScanned:
        prevScanned.extend([key1,key2]) # keep track of analyzed keys

        # convert to complementary matches
        refRead = deDuplexDict[key1]['seq']
        compRead = str(Seq(deDuplexDict[key2]['seq']).complement())

        # iterate through by locus and derive consensus
        for base in range(readLength):
            if refRead[base] == compRead[base]:
                finalRead += refRead[base]
            else:
                finalRead += 'N' # only perfect matches are permitted

        # output consensus and associated info
        target = open(final_output_file, 'a')
        target.write(deDuplexDict[umi]['header'] + '\n' + finalRead + '\n' + plus + '\n' + deDuplexDict[umi]['qual'] + '\n')
target.close()

You need a faster search method

You are comparing every entry in duplexDict directly with every entry in duplexDict. This means the number of operations will increase with the square of the number of entries in duplexDict. This stands out from the lines:

    for i in duplexDict:
        ...
        for j in duplexDict:

More formally, you algorithm runs in \$\mathcal{O}(n^2)\$, where n is the length of the input dictionary. So, for 500 million (5e8) reads of sequence data, you need to run about 250 thousand trillion (25e16) operations. This is why it takes days to run.

You will need to index your reads based on the sequences themselves. Find and implement an architecture, whether hash tables, binary trees, or something else, that allows fast searching of the input list of sequencing reads.

Of course, a hashing method is built-in with Python's dict. There is no hard limit on the length of the key strings, and the number of entries you can put in the dict is limited by available memory.

Using dictionary search

In your case, in order to use Python's built-in dictionary to make and search the hash table, you might first do something like this:

seq_dict = {}
for i in duplexDict:
    seq_dict[i['seq']] = {info: i['header']}  # add whatever info you need to find
                                              # the original entry again in `duplexDict`}

The resulting dictionary has the sequences themselves as the keys. Searching for a particular sequence takes \$\mathcal{O}(1)\$ operations.

(Note: I am assuming you need to be able to refer back to the original duplexDict once you have your hits. Then, you don't need all of the extra information to accompany the sequencing reads in the new dictionary. If each entry in duplexDict has an identifier like umi, just put that alone as the value in the new dictionary.)

Generate mismatches and search

As you are only looking for sequences different by one base, you can just generate all possible mismatches. If you eventually want to include other types of sequence similarity, you will need to use more specialized sequence analysis tools (such as BLAST).

So, you will need a simple function to generate all possible one-base-pair mismatches from each input sequence. Assuming you are using only genomic bases A,T,C,G, for each sequence, there will be 3 x n possible one-base-pair mismatches, where n is the length of the sequencing reads.

def get_one_bp_mismatches(seq):
    mismatches = []
    bases = ['A','T','G','C']
    for e,i in enumerate(seq):
        for b in bases:
            mismatches.append(seq[:e] + b + seq[e+1:])
    return mismatches

Then, search the dictionary like this:

for seq_list,info in [get_one_bp_mismatches(i['seq']),i['info'] for i in duplexDict.items()]:
    for seq in seq_list:
        if seq in seq_dict:
            finalList.append({'search_seq': info,
                              'found_seq': seq_dict[seq]['info']})

Your finalList will contain all matching pairs, identified by whatever information you use to look them up in the original duplexDict.

The whole search process will take on the order of \$\mathcal{O}(n)\$ operations, and should likely finish within minutes for 500 million sequencing reads. You can then use the last lines of your existing code to generate the output file.


POST to endpoint and GET data back I'm working on a little project and although it all works, I think I might be doing it in an incorrect way. The front end consists of a simple form that gathers input data (make and color) and once submitted, it sends the input data to a POST endpoint in express via AJAX and gets filtered to return the items I need in a JSON file. Once the data posts, I use another AJAX call to get the filtered data back to my front end to display. Again this all works, but I'm posting and getting from the same route in express. Is this all good or can I accomplish this in a better manner?

Front end:

var carList = [];

form.addEventListener('submit', function(e) {
  e.preventDefault();

  var make  = document.querySelector('.make').value.toLowerCase(),
      color = document.querySelector('.color').value.toLowerCase();

  $.ajax({
    method: 'POST',
    url: '/cars',
    data: {
      make: make,
      color: color
    },
    success: function(data) {}
  });

  $.ajax({
    method: 'GET',
    url: '/cars',
    success: function(data) {
      carList = data;
      populateList();
    }
  });
});

ExpressJS:

function readJSONFile(filename, callback) {
  fs.readFile(filename, (err, data) => {
    if(err) {
      callback(err);
      return;
    }
    try {
      callback(null, JSON.parse(data));
    } catch(exception) {
      callback(exception);
    }
  });
}

let make  = '',
    color = '';

app.get('/cars', (req, res) => {
  let newList = [];

  readJSONFile('cars.json', (err, json) => {
    var items = json.cars.filter((item) => {
      if (item.type.toLowerCase() == make || item.color.toLowerCase() == color) {
        newList.push(item);
      }
    });
    if (err) throw err;
    res.send(newList);
  });
});

app.post('/cars', (req, res) => {
  make = req.body.make;
  color = req.body.color;
});

Avoid making multiple calls to express server, you can return filtered data from the POST call. Use single AJAX POST call. As your calls are being made parallel there is no way to know if the make and color would be set when you make the GET request.
Use === to strict-compare values
Array.filter()
returns a filtered array so you can remove the newList. Look into Destructing

readJSONFile('cars.json', (err, json) => {
    if (err) {
        res.status(400).json({
            error: err
        })
    }
    const items = json.cars
        .filter((item) =>
            item.type.toLowerCase() === make || item.color.toLowerCase() === color
        );
    res.send(items);
});

Convert the callback functions to promise and chain for readability. Promisify


Catching all exceptions and errors in PHP I've tried to catch all exceptions and errors in PHP in a way so that I can deal with them consistently.

This is the code with the exception of the very last bit where I instead pass $e on to a method that outputs an error page as HTML.

// Only let PHP report errors in dev
error_reporting(ENV === 'dev' ? E_ALL : 0);

// Register handler
set_error_handler("error_handler");
set_exception_handler("error_handler");
register_shutdown_function("error_handler");

function error_handler()
{
    // Check for unhandled errors (fatal shutdown)
    $e = error_get_last();

    // If none, check function args (error handler)
    if($e === null)
        $e = func_get_args();

    // Return if no error
    if(empty($e))
        return;

    // "Normalize" exceptions (exception handler)
    if($e[0] instanceof Exception)
    {
        call_user_func_array(__FUNCTION__, array(
            $e[0]->getCode(),
            $e[0]->getMessage(),
            $e[0]->getFile(),
            $e[0]->getLine(),
            $e[0]));
        return;
    }

    // Create with consistent array keys
    $e = array_combine(array('number', 'message', 'file', 'line', 'context'),
                       array_pad($e, 5, null));

    // Output error page
    var_dump($e);
    exit;
}

I'm wondering if this is a decent way of doing it or if I have missed anything crucial. Any comments?

I don't think that printing raw errors/data to the users is a good idea:

var_dump($e);

They (usually) can't fix it, so it doesn't help them. (See #3 here) It might also contain sensitive information which could help attackers.

Instead of that you should log the errors to a log file, hide the details from the user (don't forget to print a friendly error message) and fix the errors. Your users might not call/mail you when they found a bug. They just simply use one of your competitor's service.
Instead of using the same function for three more or less different purposes:

set_error_handler("error_handler");
set_exception_handler("error_handler");
register_shutdown_function("error_handler");

I'd use three different functions (with proper signatures) which might call a fourth one. It would be cleaner (easier to read, understand and maintain). It would make unnecessary the internal magic with error_get_last(), func_get_args() and instanceof.

// Only let PHP report errors in dev
error_reporting(ENV === 'dev' ? E_ALL : 0);

It seems to me as a code smell: Test Logic in Production. I'd use php.ini to set this, I guess php.ini is a straightforward place for most of the PHP developers to set this instead of a PHP file.


Cleaner way to construct a name using Immutable.js This is what I've come up with to construct a name using a javascript object and immutable.js:
const getFullName = ({ user }) => {
    if (!user || !user.get('response')) {
        return '';
    }
    return `${user.getIn(['response', 'firstName']) || ''} ${user.getIn(['response', 'lastName']) ||
        ''}`;
};

As you can see, it looks pretty ugly. I'm wondering if there is a way to clean this code up. This function is called in loop to show all the names in the dataset. I can imagine that this solution may not be a very efficient in such case.

Rather than repeating user.getIn(['response' twice, you can extract the response once into a variable. Also, both .getIn and .get accept a default value if the property isn't found in the collection, which you can use instead of || ''. For example, .get('foo') || '' is (existing falsey values aside) equivalent to .get('foo', '').
In up-to-date environments or if the code is being transpiled, you can use optional chaining to more concisely get the nested response if it exists.
Since you want to return something if one condition is fulfilled, and something else otherwise, rather than using if, you can make things more concise by using the conditional operator:

const getFullName = ({ user }) => {
    const response = user?.get('response');
    return !response
        ? ''
        : `${response.get('firstName', '')} ${response.get('lastName', '')}`;
};

console.log(getFullName({}));
console.log(getFullName({ user: Immutable.fromJS({}) }));
console.log(getFullName({ user: Immutable.fromJS({ response: { firstName: 'foo' } }) }));
console.log(getFullName({ user: Immutable.fromJS({ response: { firstName: 'foo', lastName: 'bar' } }) }));
<script src="https://cdnjs.cloudflare.com/ajax/libs/immutable/3.8.2/immutable.min.js"></script>


Formatting 3 integers (hours, mins, secs) to `00:00:00`? I have this at the moment, wondering if there's a cleaner way to do it or if this is already as compact as it gets.

 h = "%s" % str(h) if h >= 10 else "0%s" % str(h)
 m = "%s" % str(m) if m >= 10 else "0%s" % str(m)
 s = "%s" % str(s) if s >= 10 else "0%s" % str(s)
 print("{}:{}:{}".format(h, m, s))

The % formatting operator already supports 0-padding, just specify the width as 02. Also, you can pass a tuple to combine multiple numbers into the same string with a single format: "%02d:%02d:%02d" % (h, m, s).

In Python 3 the format string method is slightly preferred to the % operator, with Python 3.6 further allowing the format string to reference the variables directly. Using such formatted string literals or f-strings, the formatting would be expressed as: f"{h:02}:{m:02}:{s:02}".

As pointed out in the comments, if you're starting with datetime objects to begin with, you can use their own formatting functionality instead.


Naive JS implementation of tidyr's gather function Tidyr is an R package for creating tidy data.
(tl;dr)
In short, it provides an interface (gather) for restructuring a table into a "tidy" format. For our purposes here, it suffices to explain that each row should only contain a value for one observation. The row could contain several fields, but these are fixed factors / categorical variables (at least for the sake of this post.)

Understanding the transformation

Tidyr is intended for tabular data. However, I'm working with D3.js, in which data rows are represented as objects, not arrays.

We'd like to be able to do the following in JavaScript:

const data = [
  {Factor1: 'x1', A: 1, B: 74, C: 0.3},
  {Factor1: 'x2', A: 2, B: 89, C: 0.12},
  {Factor1: 'x3', A: 3, B: 30, C: 0.5}
  ];

const fields = ['A', 'B', 'C'];
const out = gather(data, 'Factor2', 'Value', ...fields);
out.forEach(item => { console.log(item); });

...which should produce...

Object {Factor2: "A", Value: 1, Factor1: "x1"}
Object {Factor2: "B", Value: 74, Factor1: "x1"}
Object {Factor2: "C", Value: 0.3, Factor1: "x1"}
Object {Factor2: "A", Value: 2, Factor1: "x2"}
Object {Factor2: "B", Value: 89, Factor1: "x2"}
Object {Factor2: "C", Value: 0.12, Factor1: "x2"}
Object {Factor2: "A", Value: 3, Factor1: "x3"}
Object {Factor2: "B", Value: 30, Factor1: "x3"}
Object {Factor2: "C", Value: 0.5, Factor1: "x3"}

Gather

Again, gather provides an interface for performing such transformations. Here's the signature of the original function, from Tidyr's documentation:

gather(data, key, value, ..., na.rm = FALSE, convert = FALSE, factor_key = FALSE)

Above, key and value are the names of the two resulting columns. The column labeled by the key parameter is to take on the names of the headings of the altered columns. This is Factor3 above. Similarly, the column labeled by the value parameter is to take on the values of the same column's value... this is Value in the example above.

Anyway, the code is otherwise the same as originally posted. Here's my version of gather:

function with_fields(record, fields) {
  // Returns record with only properties specified in fields
  return fields.reduce((acc, key) =>
    {acc[key] = record[key]; return acc;}, {});
}

function split_record(record, ...fields) {
  let with_f = with_fields(record, fields);
  let other_fields = Object.keys(record)
    .filter(key => !(fields.includes(key)));
  let without_f = with_fields(record, other_fields);
  return [without_f, with_f];
}

function gather(data, key_label, value_label, ...columns) {
  // Convert wide JSON representation of CSV into long format
  let lengthen = record => Object.keys(record)
    .map(key => ({[key_label]: key,
                [value_label]: record[key]}));

  return data
    // Separate columns to be made long
    .map(record => split_record(record, ...columns))
    .map(([left, right]) => [left, lengthen(right)]) // lengthen
    // nested arrays of long records
    .map(([left, right]) => right
      .map(long_pair => Object.assign({}, long_pair, left)))
    .reduce((acc, arr) => acc.concat(arr), []); // flatten
}

Note

My implementation of gather is not as fully-featured as the original (e.g. no slicing columns out of the data set by index.)
I've also renamed key and value to key_label and value_label so that it's clear that labels - not an actual key:value pair - are being provided as input.
Finally, I had some errors in the original example code that I've fixed. The original post was somewhat broken, so I've rewritten it to avoid a lock. @MikeBrant isn't crazy. I've also implemented a number of his suggestions in this gist.

First, I would be concerned about the performance of your three consecutive map operations followed by reduce, particularly as the size of data grows in terms of number of items in array.

Right now, you have to iterate data 4 times to complete all those operations.  While the 3x mapping operation probably does provide some clarity to the operations by breaking them down to simple transformation steps, I would consider creating a single map callback that could complete all steps of transformation on an item in one pass, minimizing repetitive iteration of the array.  Of course you would probably want to run some performance tests for your expected use cases to understand the performance trade-offs here.  It might be that a usage pattern like the following makes sense:

data.map( record => {
    // mapping step 1
    // mapping step 2
    // mapping step 3
    return mappedRecord;
}

Maybe the usage example you show is not very meaningful, but I am having a hard time understanding function signature for gather().

What is the difference between the two key and value labels parameters and the columns specified passed from columns array parameter?
The signature does not make it clear to me as to which fields in the input data are to be assigned to the Letter and Value properties (for this example) in the output array.
The parameter naming doesn't seem to make sense.  Why is one called key_label and one called value_label when both are used as keys in the output structure?
From looking at this signature, how is one to determine what the logic is that is to be applied in "splitting" apart the input records?

Do the with_fields() and split_record() methods have any value outside the context of the gather() function?  If not, consider nesting them inside gather as "private" functions only in context of gather();

Stylistically:

I don't like your use of snake_case in javascript, as it is common practice in JS world to use camelCase.
Consider sticking to comments before lines of code to they apply, rather than using commments at the end of the line (which I generally avoid, as I feel they make code harder to read).
There are a few cases where variable naming is not that meaningful - without_f, with_f.  Dropping a few characters from a variable is oftentimes not worth the value you get from having clearly understandable variable names.
When you get a more complex or longer return operation on an arrow function, you might consider using bracket syntax and/or line breaks to make the code easier to read.

For example:

.map(([left, right]) => right
  .map(long_pair => Object.assign({}, long_pair, left)))

Could become:

.map(([left, right]) => {
    return right.map(
        long_pair => Object.assign({}, long_pair, left)
    );
});

// or
.map(
    ([left, right]) => right.map(
        long_pair => Object.assign({}, long_pair, left)
    )
 );

To me, this makes it much clearer that there is a nested mapping operation happening here. This is much easier than trying to count/balance opening closing parenthesis in your head as one might have to do when looking at original code that ends with three closing parenthesis in a row.


Dynamically combine ActiveRecord Relation results This isn't the actual use-case I have but for simplicity, let's say I have an ActiveRecord Animal model in a Rails app. The Animals have a name (string) and rating (int). There could be animals with the same name but different ratings. In my Animal model I have a hardcoded constant like this:

SPECIAL_ANIMAL_TYPES = [
  { name: 'Cat', rating: 3 },
  { name: 'Dog', rating: 5 },
  { name: 'Fish', rating: 1 }
]

I want to grab a single ActiveRecord:Relation with all Animals that match these attributes exactly. I can't pass an array to the where clause like where(name: ['Cat', 'Dog', etc], rating: [3, 5, 1]) because I only want cats that are rating 3, and dogs that are rating 5 and so on. Like I said, there could be multiple records with the same name but different ratings.

In the Animal model I have a class method (basically a scope) to grab the records that match the types specified in SPECIAL_ANIMAL_TYPES. The following code does what I want, but clearly is pretty ugly and if I added more types to SPECIAL_ANIMAL_TYPES I'd have to modify this method:

def self.special_animals_only
 where(SPECIAL_ANIMAL_TYPES[0]).or(where(SPECIAL_ANIMAL_TYPES[1])).or(where(SPECIAL_ANIMAL_TYPES[2]))
end

I also tried something like:

def self.special_animals_only
  SPECIAL_ANIMAL_TYPES.collect { |types| where(types) }.sum
end

But that returns an array and I want an ActiveRecord Relation. So is there a more elegant way to write this class method so I can continue modifying/adding types to the constant and have this method work dynamically?

Turns out the last code snippet in my question was close to what I ended up needing:

def self.special_animals_only
  SPECIAL_ANIMAL_TYPES.collect { |types| where(types) }.reduce(:or)
end


Clojure solution to HackerRank Challenge 'Restaurant' under Number Theory My main concern is how messy the min-number-of-slices function is.  I would appreciate help finding good ways to clean that up since it has a lot of nesting/scope.

This is my first ever Clojure program, so I know this is probably not idiomatic Clojure, nor is it neat, nor probably fast, either.  Please give as much feedback as you would like on my code (especially regarding stuff that's bad about it).

The challenge is, given a list of 2D dimensions for pieces of bread, find the minimum number of perfect squares you could cut out of each piece without wasting any bread.
https://www.hackerrank.com/challenges/restaurant

(ns clojure-solution.core)
(require '[clojure.string :as string])

(defn parse-int [s]
  (Integer. s))

(defn parse-line [s]
  (map parse-int (string/split s #" ")))

(defn get-loaf-dimensions []
  (let [numLoaves (parse-int (read-line))]
    (map #(%1) (repeat numLoaves (comp parse-line read-line)))))

(def squares (map #(* % %) (range 1 1001)))

(defn min-number-of-slices [dims]
  (let [l              (first dims)
        b              (second dims)
        area           (* l b)]
    (letfn [(perfect-slice-dimension? [slice-area]
              (= 0 (+ (mod area slice-area)
                      (mod b (int (Math/sqrt slice-area)))
                      (mod l (int (Math/sqrt slice-area))))))]
      (let [largest-square (last (filter perfect-slice-dimension? (take (min l b) squares)))]
        (int (/ area largest-square))))))

(defn -main [& args] (dorun (map println (map min-number-of-slices (get-loaf-dimensions)))))

You've implemented a brute-force solution, trying every square dimension from 1 up to the minimum of l, b, and 1000.

The optimal square size is simply the greatest common divisor of l and b.  A very simple algorithm to calculate the GCD is the Euclidean algorithm.

(defn gcd [a b]
      (if (= b 0)
          a
          (gcd b (mod a b))))

Calculating the minimum number of slices follows easily.

(defn min-number-of-slices [l b]
      (let [side (gcd l b)]
           (/ (* l b) (* side side))))

You can try calling min-number-of-slices in this ClojureScript demo:

.code { font-family: Monaco, monospace; font-size: 10pt; } #log { height: 10em; overflow: auto; white-space: pre-wrap; } #input { width: 100%; height: 12em; }

<textarea id="input" class="code"
> (defn gcd [a b]
        (if (= b 0)
            a
            (gcd b (mod a b))))

  (defn min-number-of-slices [l b]
        (let [side (gcd l b)]
             (/ (* l b) (* side side))))

  (min-number-of-slices 6 9)
</textarea>


Getting a Future from a Vector and a function I'm pretty new to Rust and to it's async/await model, and I'm trying to do something that looks like a specialized Haskell's traverse function. Given a Vec<T> and a function T -> Future<Output = R> I want to get a Future<Output = Vec<R>>.

At the moment, I have the following:

use futures::{FutureExt, StreamExt};

pub async fn traverse<I, T, R, F, FN>(xs: I, f: FN) -> Vec<R>
where
    I: IntoIterator<Item = T>,
    F: FutureExt<Output = R>,
    FN: Fn(T) -> F,
{
    futures::stream::iter(xs)
        .fold(vec![], |acc, item| {
            f(item).map(move |app| {
                let mut a = acc;
                a.push(app);

                a
            })
        })
        .await
}

It works as expected, but doesn't feel very idiomatic. Does anybody has suggestion how to improve this function?

I think the best is to simply use then() and collect():

use futures::{Future, StreamExt};

pub async fn traverse<I, T, F, Fut, O>(xs: I, f: F) -> Vec<O>
where
    I: IntoIterator<Item = T>,
    F: Fn(T) -> Fut,
    Fut: Future<Output = O>,
{
    futures::stream::iter(xs).then(f).collect().await
}

Note:

futures generic should be name Fut
function generic should be name F
I rename R as O but I don't know if it's better but it's my way.
Doesn't need FutureExt


Regex for mobile number validation In my country (South Africa) mobile numbers have 10 digits and always start with a "0" (e.g. 0821231231).

But they can also have the international dialing code ("+27") replacing the "0" (e.g. +27821231231).

I have this regex:

((\+27)|(^(?!\+)(0)))\d{9}

Is there any way that this can be improved upon?

I guess it depends a bit on what kind of input text you have but if you wanna do exact match per line

^(\+27|0)\d{9}$

would work well. If you wanna be liberal what comes before and after then

.*(\+27|0)\d{9}.*

I agree with st88 that negative lookahead makes it confusing. Better to provide your expectation before and after the main pattern (like above)


Word count and most frequent words from input text, excluding stop words # Program will display a welcome message to the user
print("Welcome! This program will analyze your file to provide a word count, the top 30 words and remove the following stopwords.")

s = open('Obama 2009.txt','r').read()  # Open the input file

# Program will count the characters in text file
num_chars = len(s)

# Program will count the lines in the text file
num_lines = s.count('\n')

# Program will call split with no arguments
words = s.split()
d = {}
for w in words:
    if w in d:
        d[w] += 1
    else:
        d[w] = 1

num_words = sum(d[w] for w in d)

lst = [(d[w],w) for w in d]
lst.sort()
lst.reverse()

# Program assumes user has downloaded an imported stopwords from NLTK
from nltk.corpus import stopwords # Import the stop word list
from nltk.tokenize import wordpunct_tokenize

stop_words = set(stopwords.words('english')) # creating a set makes the searching faster
print ([word for word in lst if word not in stop_words])

# Program will print the results
print('Your input file has characters = '+str(num_chars))
print('Your input file has lines = '+str(num_lines))
print('Your input file has the following words = '+str(num_words))

print('\n The 30 most frequent words are /n')

i = 1
for count, word in lst[:50]:
    print('%2s. %4s %s' %(i,count,word))
    i+= 1

print("Thank You! Goodbye.")

The code is clear, if a bit overly commented.  All in all very good I'd
say.  Once you do become more familiar with it, things like len(...)
should be obvious enough that they generally don't warrant a comment on
their own.

Also in general, single character names are discouraged as they don't
carry much meaning and can be harder to read.  On the top of my head s
is vaguely a string, d could be a dictionary ... but really, what
string and what dictionary I have no idea.

Next, while it works, the general recommendation in longer scripts is
not to use open directly without with, unless you take care to close
the file.  Therefore the input file should be read like follows:

# Open the input file
with open('Obama 2009.txt', 'r') as f:
    s = f.read()

The with
statement will take care to always call close on the open file, which
would otherwise lead to issues in long-running processes taking up too
much system resources (open files).

For the word counting I'd again say it's fine, but can be written more
compact.  Notably using a class from collections,
Counter.

num_words on the other hand does too much work and can just be
len(words).

list.sort actually has a reverse parameter, so you can just use that
instead of the extra call:

lst.sort(reverse=True)

Usually imports go to the start of the file, even though it doesn't
affect function too much (but you'd get an early warning if a library
wasn't available for example).

For formatting output there are a ton of options.  At some point you
might want to look at the
format
method for strings.

Lastly, the loop at the end prints the 50 most frequent words, not
30 like the output suggests.  That is a good opportunity to introduce
a constant for the number of words to print:

PRINT_WORDS = 50

print('\n The {} most frequent words are /n'.format(PRINT_WORDS))

Since it's a constant it's in upper case.  I've also used format here
since using placeholders looks a bit nicer and also is much regular than
either concatenation or the % operator.  But it's kind of up to
preference anyway.

Lastly, the final loop uses an explicit counter, which, again, has an
equivalent and nicer standard library helper called
enumerate.

for i, (count, word) in enumerate(lst[:PRINT_WORDS], 1):
    print('%2s. %4s %s' % (i, count, word))

Which starts at zero normally, but I actually just saw that it supports
an optional start parameter, so might as well use that.


WPF Extension to directly inject dependencies in the Window constructor I just finished writing the core for my WPF application and because I'm still a very beginner at software design I wanted to get some feedback about the design decisions I've made.

I've uploaded the project to Github and I'll post the most important parts here (the project is to big to be posted as a whole). To see the full code base please follow the link.

Purpose

The general purpose of the core I've written was to allow me to use dependency injection to inject directly in to the XAML System.Windows.Window throughout the application lifecycle:

public partial class MainWindow : Window
{
    public MainWindow(object myObj)
    {
        InitializeComponent();
    }
}

When specifying a StartupUri inside the WPF application for the startup window to use the WPF framework doesn't allow you to have a parameterized constructor inside the window. Because I'm a huge fan of dependency injection this was kind of a downside for me and the application core I've written has the purpose to allow me to use a startup window which gets dependencies injected in the constructor.

Component Management Module

One of the most important parts of the application is the component management module. In order to build and activate a System.Windows.Window I've written a WindowManager which itself is just a proxy for an IWindowManager which is being resolved by an IServiceResolver at runtime:

internal class WindowManager
{

    private static IWindowManager _windowManagerImpl;
    internal static IWindowManager WindowManagerImpl
    {
        get
        {
            if (_windowManagerImpl == null)
                _windowManagerImpl = ServiceResolverBuilder.GetServiceResolver()?.Resolve<IWindowManager>();
            return _windowManagerImpl;
        }
        set
        {
            Guard.NotNull(value, nameof(value));

            _windowManagerImpl.Dispose();
            _windowManagerImpl = value;
        }
    }

    public static Window BuildWindow(Type windowType) =>
        WindowManagerImpl?.BuildWindow(windowType);

    public static TWindow BuildWindow<TWindow>() where TWindow : Window =>
        WindowManagerImpl?.BuildWindow<TWindow>();

    public static void ActivateWindow(Window window) =>
        WindowManagerImpl?.ActivateWindow(window);

    public static void ShowAsDialog(Window window) =>
        WindowManagerImpl?.ShowAsDialog(window);

}

The resolved IWindowManager depends on an IWindowBuilder implementation for creating the window and an IWindowService for activating the window.

Application Startup

On application startup I use an IStartup mainly for setting up the dependency injection container and a StartupWindowHelper which gets the startup window type from the application config. Then I call into the component service module to create and activate the window:

public partial class App : Application
{

    protected override void OnStartup(StartupEventArgs e)
    {
        RunApplicationStartup();
        BuildAndActivateStartupWindow();

        base.OnStartup(e);
    }

    private void RunApplicationStartup()
    {
        var applicationStartup = ApplicationStartupBuilder.BuildApplicationStartup();
        applicationStartup.Run();
    }

    private void BuildAndActivateStartupWindow()
    {
        var startupWindowType = StartupWindowHelper.GetStartupWindowType();

        var startupWindow = WindowManager.BuildWindow(startupWindowType);
        WindowManager.ActivateWindow(startupWindow);
    }

}

I've tried my very best writing the core as clean as possible but as I've already mentioned I'm a beginner and not very experienced with good design.

Constructor injection is indeed preferable, but it's not the only way to inject dependencies - property injection is just as valid, and in the case of a WPF Window, the only dependency that should be injected, is the ViewModel - and Window already has a property exposed for that: DataContext.

Assuming a Model-View-ViewModel architecture, I cannot think of a single valid reason to constructor-inject anything into a View. The role of the View in MVVM is to be nothing more than a simple I/O device presenting data to the user, and exposing means for that user to provide inputs.

That means a View needing dependencies injected is highly suspicious, because it suggests that the View is at least partly doing the ViewModel's job, and this is going to make that logic much more difficult to test than if it were done elsewhere: the idea behind separating the View from the ViewModel isn't just to leverage the powerful XAML data bindings, it's also to get as much of the not-stricly-presentation concerns out of the View so that they can be unit-tested without popping up a UI.

Now, as I said in an earlier comment, maybe I'm doing this wrong, but I've never had issues with this approach: I remove the StartupUri in the App.xaml markup, and then edit App.xaml.cs to override OnStartup as follows:

protected override void OnStartup(StartupEventArgs e)
{
    base.OnStartup();
    // if you're using an IoC container, this is where you set it up:
    // ...

    // if you're using an IoC container, this is where you resolve your VM's dependencies:
    // ...
    // otherwise, just new it up:
    var vm = new MainWindowViewModel(/*ctor-inject dependencies manually here*/);
    var window = new MainWindow { DataContext = vm };
    window.ShowDialog();
}

The IoC configuration code should go into its own method (could be in App.xaml.cs, or in some static helper), of course. But the point remains: the view doesn't have any dependencies, other than its view model.

The MainWindow.xaml.cs code-behind might look like this:

public partial class MainWindow : Window
{
    public MainWindow()
    {
        InitializeComponent();
        DataContextChanged += MainWindow_DataContextChanged;
    }

    MainWindowViewModel ViewModel => DataContext as MainWindowViewModel;

    private void MainWindow_DataContextChanged(object sender, DependencyPropertyChangedEventArgs e)
    {
        var vm = ViewModel;
        if (vm != null)
        {
            vm.Close += HandleViewModelCloseCommand;
        }
    }

    private void HandleViewModelCloseCommand(object sender, EventArgs e)
    {
        Close();
    }
}

Everything else belongs in the ViewModel: any ICommand you might want to inject, any service, unit-of-work/repository, "business logic" worker objects - none of these are dependencies of the window itself.

Since a WPF window needs no dependencies, the base premise falls apart, and all that code can be deleted.


Print all permutations with some constraints I've the following problem:

  Print all valid phone numbers of length n subject to following
  constraints:
       1.If a number contains a 4, it should start with 4
       2.No two consecutive digits can be same
       3.Three digits (e.g. 7,2,9) will be entirely disallowed, take as input

Here's my solution:

public static void ans(int length, int d1, int d2, int d3) {
    StringBuffer numbers = new StringBuffer();

    for(int i = 0; i < 10 ;i++) {
        if(i != d1 && i != d2 && i != d3) {
            numbers.append(i);
        }
    }
    printNumbers("", numbers.toString(), length);
}

public static void printNumbers(String soFar, String remaining, int length) {
    if(soFar.length() == length) {
        boolean four = false;
        boolean sameAdjDigits = false;

        for(int i = 1; i < soFar.length(); i++) {
            if(soFar.charAt(i-1) == soFar.charAt(i)) sameAdjDigits = true;
            if(soFar.charAt(i) == '4') four = true;
        }

        if(!sameAdjDigits) {
            if(!four || (four && soFar.charAt(0) == '4'))
                System.out.print(soFar + " ");
        }

        return;
    }

    if(soFar.length() < length) {
        for(int i = 0; i < remaining.length(); i++) {
            String newSoFar = soFar + remaining.charAt(i);
            String newRemaining = remaining.substring(0, i) + remaining.substring(i+1);
            printNumbers(newSoFar, newRemaining, length);
        }
    }
}

I ran some tests on this code and checked all the printed values. It worked for length 1 and 2. However, I couldn't verify for length > 2 because there were too many numbers.

Can anyone verify if my code runs a correct result? Is there any part of the code that can be improved?

Nitpicks

public static void ans(int length, int d1, int d2, int d3) {

Any time that I see numbered variables, I want to replace them with a collection.  Since we know that there will be no duplicate values, I'd use a Set.

public static void generatePhoneNumbers(int length, Set<Character> disallowedDigits) {

Also, I find d an unclear name, so I switched to disallowedDigits which I find clearer.

I switched from int to Character as you are building a string.  We could just as easily be working with the letters of the alphabet as with numeric digits.

StringBuffer numbers = new StringBuffer();

This seems an ideal place to use a StringBuilder.

StringBuilder digits = new StringBuilder();

I also changed the name to digits, as that tells me that we're talking about one digit at a time rather than multi-digit numbers.

            if(!four || (four && soFar.charAt(0) == '4'))

You could just as well write

            if( ! four || soFar.charAt(0) == '4' )

If the string begins with a 4, then we don't care if none of the rest have a 4.

Fulfilling the Problem Statement

        String newRemaining = remaining.substring(0, i) + remaining.substring(i+1);

This is actively wrong given your problem statement:

  Print all valid phone numbers of length n subject to following constraints:
  1.  If a number contains a 4, it should start with 4
  2.  No two consecutive digits can be same
  3.  Three digits (e.g. 7,2,9) will be entirely disallowed, take as input

From #2, only consecutive digits can't be the same.  So you should not be removing digits from your remaining String.  Like length, that will be constant for all calls.

We can rewrite #1 to say:  only numbers that start with 4 can contain 4.  This effectively gives us different rules for 4 than other values.  Since we're supposed to generate all numbers, I'd suggest that we generate all numbers starting with 4 separately from other numbers.  Fortunately, #3 means that we already need a way to exclude digits from the set.

ans

Here's how I'd write the method that you called ans:

private final static char [] DIGITS = "0123456789".toCharArray();

public static void generate(int length, Set<Character> disallowedDigits) {
    Set<Character> allowedDigits = new TreeSet<Character>();

    for ( Character digit : DIGITS ) {
        if ( ! disallowedDigits.contains(digit) ) {
            allowedDigits.add(digit);
        }
    }

    if ( allowedDigits.contains('4') ) {
        printNumbers("4", allowedDigits, length);
        allowedDigits.remove('4');
    }

    for ( Character digit : allowedDigits ) {
        printNumbers("" + digit, allowedDigits, length);
    }
}

First, we define a constant to hold the possible digit values.  Then we make a new Set that masks out the disallowedDigits.  If 4 is allowed, then we generate all the numbers that start with 4 separately.  Then we remove 4 from the allowedDigits, as it won't be allowed in any of the other numbers.

printNumbers

    if(soFar.length() == length) {
        boolean four = false;
        boolean sameAdjDigits = false;

        for(int i = 1; i < soFar.length(); i++) {
            if(soFar.charAt(i-1) == soFar.charAt(i)) sameAdjDigits = true;
            if(soFar.charAt(i) == '4') four = true;
        }

        if(!sameAdjDigits) {
            if(!four || (four && soFar.charAt(0) == '4'))
                System.out.print(soFar + " ");
        }

        return;
    }

This whole section of code is unnecessary, except for one line (the print).  Instead of generating invalid strings and masking them out, just don't generate them in the first place.  That will save all subsequent recursive calls down that path.

    if(soFar.length() < length) {
        for(int i = 0; i < allowedDigits.length(); i++) {
            String newSoFar = soFar + allowedDigits.charAt(i);
            String newRemaining = allowedDigits.substring(0, i) + allowedDigits.substring(i+1);
            printNumbers(newSoFar, newRemaining, length);
        }
    }

I switched to a Set to hold the digits that can be used, so this is going to be a bit different.

private static void printNumbers(String soFar, Set<Character> allowedDigits, int length) {
    int lengthSoFar = soFar.length();

    if ( lengthSoFar < length ) {
        Character last = soFar.charAt(lengthSoFar - 1);

        for ( Character digit : allowedDigits ) {
            if ( ! digit.equals(last) ) {
                printNumbers(soFar + digit, allowedDigits, length);
            }
        }
    } else {
        System.out.println(soFar);
    }
}

First, we already took care of 4 in our generate function.  Here, it will either be in allowedDigits or it won't.

Second, we take care of the rule about consecutive digits by simply not starting the recursive generation in that case:  if ( ! digit.equals(last) ) {.

I put each number on its own line.  For all but trivial lengths, there are too many numbers to fit on a single line.

Note that soFar is the only parameter that changes from call to call.

Testing

This is a bit difficult.  Because we print the numbers to standard output, we can't run normal unit tests on them.  The simplest way to handle this is to direct the output to a file.  Then we can search the file for values that should or shouldn't be there.

For example, we should never have any of the following sequences:  00, 11, 22, 33, 44, 55, 66, 77, 88, 99, 00.  So search for those in the file.  If they don't exist, then you know that rule is working.

Search for lines containing the disallowedDigits.  There shouldn't be any.

Find all the lines in the file that contain a 4 and verify that 4 is the first digit of the number.  On Linux, that might be written something like

fgrep '4' < phone_numbers.txt | cut -c1 | sort | uniq -c

If there are any values other than 4, then you can look them up more specifically.

Also search for a few values that you construct, e.g. for a length of 8 where 7, 2, and 9 are disallowed, 40134568 should be in the list.  Also 86868686 should be there.