Searching and parsing text from a file I'm a student working as a research assistant and wrote this script to automate density functional theory HPC tasks with SLURM. When a calculation is complete the script checks the contents of a log file for the total force, and if it's above the desired threshold it generates a new input file, file.in.new, with relaxed atomic positions from the log which is passed to another script in the automation process. Please point out any issues with formatting, syntax, etc, and if there's anything I could do to simplify things.
Example of use: python generate.py file.log file.in
from sys import argv

def arg_parse(argv):
    try:
        log_file=argv[1]
        input_file=argv[2]
        override=False
    except IndexError as e:
        raise SystemExit

    if len(argv)==4 and argv[3].strip("-")=='o':
        override=True

    scan_and_write(log_file, input_file, override)
#---------------------------------------------------------------------
def scan_and_write(log_file, input_file, override):
    with open(log_file, 'r+') as log:
        total_force=[float(line.split()[3]) for line in log if line.rfind('Total force =') != -1][-1]
        tolerance(total_force, override, input_file)
        log.seek(0)
        total_cycles=sum([1 for line in log if line.rfind('ATOMIC_POSITIONS (crystal)') != -1])
        log.seek(0)
        index=[int(line.split("=")[1]) for line in log if ("number of atoms/cell" in line)][0]
        log.seek(0)
        for line in log:
            if line.rfind('ATOMIC_POSITIONS (crystal)') != -1:
                atomic_positions=[log.readline().split() for i in range(index)]

    new_input=open(input_file.replace('.in', '.in.new'), "w+")
    fmt = '{:2}      {:12.9f}  {:12.9f}  {:12.9f}\n'
    with open(input_file, 'r+') as old_input:
        for line in old_input:
            if len(line.split()) != 4 and not line[0].isnumeric():
                new_input.write(line)
            if ('ATOMIC_POSITIONS') in line:
                for position in atomic_positions:
                    new_input.write(fmt.format(position[0],*[float(xred) for xred in position[1:4]]))

#---------------------------------------------------------------------
def tolerance(force, override, file_name):
    print('A total force of {} was achieved in the last SCF cycle'.format(force))
    if (force < 0.001 and not override):
        print("Relaxation sufficient, total force = %s...terminating" %force)
        raise SystemExit
    if (force < 0.001 and override):
        print("Relaxation sufficient...total force = %s\n\nOverriding threshold"\
            " limit and generating %s" %(force, file_name.replace('.in', '.in.new')))
#---------------------------------------------------------------------
if __name__ == "__main__":
    arg_parse(argv)

One tip: use the argparse module.
import argparse

parser = argparse.ArgumentParser()

parser.add_argument("--log_file", dest="log_file", type=str, required=True, help="Add some help text here")
parser.add_argument("--input_file", dest="input_file", type=str, required=True, help="Add some help text here")

args = parser.parse_args()

# show the values, will reach this point only if the two parameters were provided
print(f"log_file: {args.log_file}")
print(f"input_file: {args.input_file}")

Then you call your script like this:
python3 generate.py --log_file test.log --input_file test.txt

Parameter order is free.
I have to admit I don't understand much about the purpose since I don't know about your input files.
If they are CSV then you might consider using the csv module. Then you should be able to simplify some statements like this one:
total_force=[float(line.split()[3]) for line in log if line.rfind('Total force =') != -1][-1]

Something is lacking in your script, badly: comments. They will help you too, especially when go back to reviewing code you wrote a few months ago. Probably you will have forgotten details and will have to reanalyze your own code.


Get Next and Previous "Load Day" I keep looking at this trying to figure if there's a better way.  All three functions work, but I wanted to get some insights and ideas from others.

IsLoadDay determines if the date parameter is a "Load Day".  Load Days are Tuesday through Saturday, but we skip holidays and designated No Load Days.  There are two existing tables of Holidays (HOLDY_WA_STATE) and No Load Days (afrs_daily_load_no_load_date).

GetNextLoadDay and GetPreviousLoadDay are intended to be used.  They each use IsLoadDay to determine if the date parameter is a Load Day, and then move forward or backward through time until a Load Day is found.

GetNextLoadDay will check the date parameter and determine if it is a Load Day whereas GetPreviousLoadDay will first go one day back and then start checking if the date is a Load Day.

IsLoadDay Function

CREATE FUNCTION [dbo].[IsLoadDay](@Dt DATE)
RETURNS BIT
AS
BEGIN
DECLARE @IsLoadDay BIT = 1
IF DATENAME(DW, @Dt) IN ('SUNDAY', 'MONDAY')
    BEGIN
        SET @IsLoadDay = 0
    END
ELSE
IF EXISTS(SELECT 1 FROM [DBMGMT].[dbo].[afrs_daily_load_no_load_date] WHERE [load_date] = @Dt AND [load_ind] = 'N')
    BEGIN
        SET @IsLoadDay = 0
    END
ELSE
IF EXISTS(SELECT 1 FROM [master].[dbo].[HOLDY_WA_STATE] WHERE [HOLDY_DATE] = @Dt)
    BEGIN
        SET @IsLoadDay = 0
    END
RETURN @IsLoadDay
END
GO

GetNextLoadDay Function

CREATE FUNCTION [dbo].[GetNextLoadDay](@Dt DATE)
RETURNS DATE
AS
BEGIN
DECLARE @NxtDt DATE
SET @NxtDt = @Dt
WHILE [dbo].[IsLoadDay](@NxtDt) != 1 BEGIN
    SET @NxtDt = DATEADD(D, 1, @NxtDt)
END
RETURN @NxtDt
END
GO

GetPreviousLoadDay Function

CREATE FUNCTION [dbo].[GetPreviousLoadDay](@Dt DATETIME)
RETURNS DATETIME
AS
BEGIN
DECLARE @PrvDt DATE
SET @PrvDt = DATEADD(D, -1, @Dt)
WHILE [dbo].[IsLoadDay](@PrvDt) != 1 BEGIN
    SET @PrvDt = DATEADD(D, -1, @PrvDt)
END
RETURN @PrvDt
END
GO

Is Load Day?
Your IsLoadDay function seems a little overcomplicated.
Might I suggest trying something along the lines of this, which brings all the checks you want to do pretty nicely into a single coalesce:
CREATE FUNCTION [dbo].[IsLoadDay](@Dt DATE)
RETURNS BIT
AS
BEGIN

RETURN COALESCE
(
    --If it is a monday or sunday, it's not a load day
    CASE WHEN DATENAME(DW, @Dt) IN ('SUNDAY', 'MONDAY') THEN 0 END,

    --If it is a designated not load day, it's not a load day
    (
        SELECT 0
        FROM [DBMGMT].[dbo].[afrs_daily_load_no_load_date]
        WHERE [load_date] = @Dt AND [load_ind] = 'N'
    ),

    --If it is a holiday, it's not a load day
    (
        SELECT 0
        FROM [master].[dbo].[HOLDY_WA_STATE]
        WHERE [HOLDY_DATE] = @Dt
    ),

    --Otherwise, it's a load day
    1
)
END
GO


Removing text after `:`, `:` included I need to remove last text after : in my String and : too. I have tried this. Please review. If there is a better way to do, please tell me.

String test = "temp:content:region:deposit:up";
System.out.println(test.replace(test.substring(test.lastIndexOf(":"), test.length()), ""));

Using regexp (replace) isn't the best solution. I think you should use only substring:

System.out.println(test.substring(0, test.lastIndexOf(":")))


Project Euler Problem 6: sum of squares and square of sums This is a very simple task, but I'm curious if there is a better, more 'Pythonic' way to solve this.

"""The sum of the squares of the first ten natural numbers is,

1^2 + 2^2 + ... + 10^2 = 385
The square of the sum of the first ten natural numbers is,

(1 + 2 + ... + 10)^2 = 552 = 3025
Hence the difference between the sum of the squares of the first ten natural numbers
and the square of the sum is 3025 − 385 = 2640.

Find the difference between the sum of the squares of the first one hundred natural
numbers and the square of the sum."""

square = 0
s = 0

for i in range(1, 101):
    square += i**2
    s += i

s = s**2

print(s - square)

This would be good practice for list comprehension, especially since sum() is a builtin function.

sequence = range(1, 11)
sum_of_squares = sum([i**2 for i in sequence])
square_of_sum = sum(sequence)**2
print(square_of_sum - sum_of_squares) # 2640

For even more fanciness, you can use this expression for the sum_of_squares:

sum_of_squares = sum(i**2 for i in sequence) # No square brackets

This sums over a generator expression instead of creating a new list, saving memory, especially if the sequence is something ridiculous like range(1, 1000000001).


Very simple number guessing game I created a simple 15-line number guessing game in Python 2.7:

# Guess the number game
username = raw_input("What is your name? -> ")
guess = input("%s, I'm thinking of a number between 1 and 10. -> " % username)

answer = 7 # sets the answer
answer = guess

if guess < 7:
    print "That's too low."

if guess > 7:
    print "That's too high."

if guess == 7:
    print "Good. Thanks for playing."

What is a proper way I could make it a little more efficient?

Contradicting statements

answer = 7 # sets the answer
answer = guess

Here, you are setting a variable only to immediately set it to another value in the very next line, effectively invalidating the first line. This also makes your code confusing: you do that thing you are doing in these lines, and then you start comparing the guess with this magic number 7 (which is was answer used to be).

Also, why is the answer being set to the guess? Those are two completely different things. Instead of what you are doing, to make your code much more understandable, you should get rid of this line:

answer = guess

Then, instead of comparing guess with 7, compare it with answer.

if guess < answer:
    print "That's too low."

if guess > answer:
    print "That's too high."

if guess == answer:
    print "Good. Thanks for playing."

Now your code is much more readable and follows better practices.

elif

Using elif is good because if the first conditional check fails, the rest will not be executed, which speeds up your code. Unfortunately, elif can not be used in every case due to specific code-based reasons.

Luckily for you, your code can use elif. So, instead of having three separate if statements, you should have one with two elifs:

if guess == answer:
    print "Good. Thanks for playing."

elif guess < answer:
    print "That's too low."

else:
    print "That's too high."

I also re-arranged your code. Now, the most unique conditional comes first. And, instead of a final elif, I put an else because if the guess is not greater than or equal to the answer, it has to be less than it.


OOP between projects So, I wanted a separate project to act as the data layer for my website project.

Classes in the website should reference the data layer after getting input from the user and should get back information regarding the result of whatever actions are performed and then communicate those results to the user.

The following is an example of how I've got it set out. This example deletes a user record from the database:

// Website/Admin/Users.aspx.cs
protected void btnDelete_Click(object sender, EventArgs e)
{
    User us = new User(Convert.ToInt32(Session["ActionUserID"]));

    if (us.Delete())
    {
        lblResult.Text = "User record was successfully deleted.";
    }
    else
    {
        lblResult.Text = us.errmsg;
    }
}

// Website/App_Code/User.cs
//
// For simplicity sake, I've removed code that isn't needed in this example
public int userid { get; protected set; }
public string errmsg { get; protected set; }

public User(int UserID)
{
    userid = UserID;
}

public bool Delete()
{
    OrtundUser ou = new OrtundUser(userid);
    if (ou.Delete())
    {
        return true;
    }
    else
    {
        errmsg = ou.errmsg;
        return false;
    }
}

// DataProject/OrtundUser.cs
public int userid { get; protected set; }
public string errmsg { get; protected set; }

OrtundData od = new OrtundData(); // instantiate the DbContext

public OrtundUser(int UserID)
{
    userid = UserID;
}

public bool Delete()
{
    try
    {
        var user = (from u in od.users
                    where u.userid == userid
                    select u).FirstOrDefault();

        if (user != null)
        {
            od.Remove(user);
            od.SaveChanges();

            return true;
        }
    }
    catch (Exception ex)
    {
        errmsg = ex.InnerException;
        return false;
    }
}

I feel like there's a lot of repetition of code here. I know this will work as intended, but I don't know if it's as efficient as it could be. (for example, the KISS principle would argue the existence of App_Code/User.cs at all ... why not just reference the DataProject directly?)

If anyone can provide any suggestions for me to improve this (particularly with a look at removing redundant code) - possibly explaining any concepts that I may seem to be misunderstanding - I'd really appreciate it.

A couple of general points:

You shouldn't be using properties to set an error message like that. The error message is associated with the action you attempted on the object- in this case the delete- not the object itself. After the error has been handled, it doesn't make any sense for the property to continue holding that error message. It also potentially adds unnecessary concurrency woes.

You could fix that by wrapping up the bool you're returning and the message into a class, like DbActionResult. Or you could make the message an out parameter. But this still isn't a great solution because you have a leaking of concerns between your layers. Why should the data layer directly dictate what's being displayed in the UI like that? Better options would be either:

Return some kind of status code from the data layer, and have the UI display a message based on that code. If you don't need any more information in the UI than just whether or not it passed, a bool would be sufficient. If you want to log the exact exception details for debugging, that can be done with some kind of logging framework

OR

Don't catch the exception at all. Or if you do catch it to do some kind of handling/logging, rethrow it. That way you can access it directly in the UI layer if necessary, but the UI decides how to handle turning that exception into a message to display to the user, rather than the data layer doing it.

Also, this may have just been because it was an example, but OrtundUser is a bad name for a class. A User is a User, it shouldn't be a UserWhichIsPersistedInAParticularWay. That violates the Single Responsibility Principle, as it now has to take responsibility not only for what it's actually supposed to do- represent a user- but also do at least some management of its own persistence. You also have a similar problem with your actual User class, though it's not reflected in the name. By putting the Delete method on the user, it now has to be concerned not only with what it actually is, but with how to persist itself

This is related to the actual meat of the question. Your first step should be to separate your User class out into User and UserRepository like so:

public class User
{
    public int userid { get; set; }
}

public class UserRepository
{
    public bool Delete(User user)
    {
        //Delete logic goes here
    }
}

That's not a full example, but it'll be refined later. Hopefully you can see how this has now separated out different responsibilities into their own classes.

From your mention of DbContext in a comment, I assume you're using Entity Framework. EF actually already provides you with a class that will serve the purpose UserRepository here, which is DbSet<User>. You should be able to get that from your OrtundData. That class will handle operations like Delete for you. So in fact the only class from the above example that you actually need is the User class.

(As a side note, this is an implementation of what's called the 'repository pattern'. If you look this up, you'll see it's often recommended that even when using Entity Framework, you add your own repository class on top- UserRepository in the above example. I wouldn't worry about that at the moment, it adds another layer of abstraction which can be valuable in some cases, but don't do it without understanding why. It's more important to get the basics in place.)

So finally coming to whether User and OrtundUser are redundant. Yes, they are. Once you remove the persistence logic from both of them as described above, you're left with nothing but a property bag which is identical for both of them. You should delete OrtundUser, and push User down into your data layer.

In fact, in large solution you could push futher- have a "Core" project referenced by both the UI and Data projects which contains your domain objects like User. Data would then reference Core, and UI would reference both Data and Core. In your case, actually structuring your solution like this is likely to be unnecessary, but it's a good way to think about it conceptually.

So, finally, with all of the above applied your code might look something like this (again this is omitting some details like construction, just leaving you with the key bits):

// Website/Admin/Users.aspx.cs
private DbSet<User> _userRepository;
private SomeSortOfLogger _logger;

protected void btnDelete_Click(object sender, EventArgs e)
{
    User us = new User(Convert.ToInt32(Session["ActionUserID"]));

    try
    {
       _userRepository.Delete(us);
       lblResult.Text = "User record was successfully deleted.";
    }
    catch(Exception e)
    {
        _logger.Log(e);
        lblResult.Text = "There was an error, see log for details";
    }
}

// DataProject/User.cs
public int userid { get; protected set; }

public User(int UserID)
{
    userid = UserID;
}

There's likely still scope for improvement- e.g. adding a better way of doing exception/error handling and logging, dependency injection. But those would be beyond the scope of this question.


Dice-rolling simulator I know there is a better way to store data.  What is the most concise way to simplify this script?

from random import randint
from sys import exit
import os

os.system('clear')
print "Welcome to the dice rolling simulator!"
raw_input("Press enter to begin.")

total = 0

completed = [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]

def roll():
    os.system('clear')
    die1 = randint(1, 6)
    die2 = randint(1, 6)
    global total
    total = die1 + die2
    storetotal()
    print "Die 1: %d \nDie 2: %d \nTotal: %d." % (die1, die2, total)
    print "\n\nRoll again?"
    roll_again = raw_input("Press enter to roll again, type 'stats' to view scores, or 'quit' to exit.\n> ")
    if roll_again == "":
        roll()
    elif roll_again == "stats":
        stats()
    elif roll_again == "quit":
        exit(0)
    else:
        print "I don't know what that means so you get to roll again."
        raw_input("> ")
        roll()

def stats():
    global total
    print "2s: %d \n3s: %d \n4s: %d \n5s: %d \n6s: %d \n7s: %d \n8s: %d" % (completed[0],
                                                    completed[1], completed[2], completed[3],
                                                    completed[4], completed[5], completed[6])
    print "9s: %d \n10s: %d \n11s: %d \n12s: %d""" % (completed[7], completed[8],
                                                    completed[9], completed[10])
    raw_input("")
    roll()

def storetotal():
    if total == 2:
        completed[0] += 1
    elif total == 3:
        completed[1] += 1
    elif total == 4:
        completed[2] += 1
    elif total == 5:
        completed[3] += 1
    elif total == 6:
        completed[4] += 1
    elif total == 7:
        completed[5] += 1
    elif total == 8:
        completed[6] += 1
    elif total == 9:
        completed[7] += 1
    elif total == 10:
        completed[8] += 1
    elif total == 11:
        completed[9] += 1
    else:
        completed[10] += 1

roll()

Line by line analysis

Use four spaces to indent python code. Never use tabs.

from random import randint
from sys import exit
import os

It is a good practice to do top-level imports (without importing module members). This way it's easier to understand what package the methods come from.

os.system('clear')
print "Welcome to the dice rolling simulator!"
raw_input("Press enter to begin.")

total = 0

completed = [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]

Use list comprehensions.

def roll():

Roll method is confusing. You expect it to roll a die, but it processes iterations of the game. A while loop would be more explicit here.

os.system('clear')
die1 = randint(1, 6)
die2 = randint(1, 6)
global total

Global variables are no-no. Never.

total = die1 + die2
storetotal()
print "Die 1: %d \nDie 2: %d \nTotal: %d." % (die1, die2, total)
print "\n\nRoll again?"
roll_again = raw_input("Press enter to roll again, type 'stats' to view scores, or 'quit' to exit.\n> ")

You're exceeding recommended 80 column length limit.

if roll_again == "":
    roll()

See how I changed this block in a code below.

elif roll_again == "stats":
    stats()
elif roll_again == "quit":
    exit(0)

Return from the method is sufficient here.

else:
    print "I don't know what that means so you get to roll again."
    raw_input("> ")
    roll()

def stats():
global total
print "2s: %d \n3s: %d \n4s: %d \n5s: %d \n6s: %d \n7s: %d \n8s: %d" % (completed[0],                                                                     completed[1], completed[2], completed[3],
                                            completed[4], completed[5], completed[6])
print "9s: %d \n10s: %d \n11s: %d \n12s: %d""" % (completed[7], completed[8],
                                                completed[9], completed[10])

Indentation here is completely messed up.

raw_input("")
roll()

def storetotal():

PEP8: store_total.

if total == 2:
    completed[0] += 1
elif total == 3:
    completed[1] += 1
elif total == 4:
    completed[2] += 1
elif total == 5:
    completed[3] += 1
elif total == 6:
    completed[4] += 1
elif total == 7:
    completed[5] += 1
elif total == 8:
    completed[6] += 1
elif total == 9:
    completed[7] += 1
elif total == 10:
    completed[8] += 1
elif total == 11:
    completed[9] += 1
else:
    completed[10] += 1

roll()

Improved code

I simplified your roll method to roll2d6 and moved all the other code into main.

import random
import os

def roll2d6():
    die1 = random.randint(1, 6)
    die2 = random.randint(1, 6)
    return die1 + die2

def display_stats(stats):
    for i, result in enumerate(stats):
        print "%ds: %d" % (i + 2, result)

def update_stats(total, stats):
    stats[total - 2] += 1
    return stats

def main():
    stats = [0 for _ in xrange(10)]

    os.system('clear')
    print "Welcome to the dice rolling simulator!"
    raw_input("Press enter to begin.")
    while True:
        os.system('clear')
        total = roll2d6()
        stats = update_stats(total, stats)
        print "Die 1: %d \nDie 2: %d \nTotal: %d." % (die1, die2, total)
        print "\n\nRoll again?"
        roll_again = raw_input((
                "Press enter to roll again, type 'stats' to view scores, "
                "or 'quit' to exit.\n> "))
        if roll_again.lower() == "stats":
            display_stats(stats)
        elif roll_again.lower() == "quit":
            break
        elif roll_again.lower() != "":
            print "I don't know what that means so you get to roll again."

if __name__ == "__main__":
    main()


Start of an ATM program I'm learning Python and have made this start of a ATM program, I'm very new to this so please be gentle..

Is there a better way to rewrite this?

CHECKING_BALANCE = 4000
SAVINGS_BALANCE = 1000

def welcome_screen():
    print """
    Welcome to the Bank of Bam!
    To continue please follow the instructions:
       Press 1 for Checking Account
       Press 2 for Savings Account
       Press 3 for Balance Inquiries
       Press 4 to return card and exit
    """
    choice = int(raw_input("> "))
    if choice == 1:
        checking_account()
    elif choice == 2:
        savings_account()
    elif choice == 3:
        balance_inquiries()
    else:
        print "Exiting..."

def checking_account():
    print "You have $%i available" % (CHECKING_BALANCE)
    print """
    What would you like to do:
    [W]ithdrawl
    [D]eposit
    [T]ransfer funds
    """
    choice = raw_input("> ")
    if choice == "W":
        withdraw_funds()
    elif choice == "D":
        deposit_funds()
    elif choice == "T":
        transfer_funds
    else:
        print "Invalid input.."

def savings_account():
    print "You have $%i available" % (SAVINGS_BALANCE)
    print """
    [W]ithdrawl
    [D]eposit
    [T]ransfer
    """
    choice = raw_input("> ")
    if choice == "W":
        withdraw_funds()
    elif choice == "D":
        deposit_funds()
    elif choice == "T":
        transfer_funds()
    else:
        print "Invalid input.."

welcome_screen()

Example of usage:

    Welcome to the Bank of Bam!
    To continue please follow the instructions:
       Press 1 for Checking Account
       Press 2 for Savings Account
       Press 3 for Balance Inquiries
       Press 4 to return card and exit

> 1
You have $4000 available

    What would you like to do:
    [W]ithdrawl
    [D]eposit
    [T]ransfer funds

> Traceback (most recent call last): #<= CRTL - C
  File "atm.py", line 58, in <module>
    welcome_screen()

C:\Users\Justin\MyScripts>python atm.py

    Welcome to the Bank of Bam!
    To continue please follow the instructions:
       Press 1 for Checking Account
       Press 2 for Savings Account
       Press 3 for Balance Inquiries
       Press 4 to return card and exit

> 2
You have $1000 available

    [W]ithdrawl
    [D]eposit
    [T]ransfer

> Traceback (most recent call last): #<= CTRL - C
  File "atm.py", line 58, in <module>
    welcome_screen()
  File "atm.py", line 17, in welcome_screen
    savings_account()
  File "atm.py", line 48, in savings_account
    choice = raw_input("> ")
EOFError

The main thing I would say here is that you have 2 nigh identical functions apart from the balance being passed to them. Instead of checking_account and savings_account being different functions, make a single account_interaction function that takes an argument which you can then pass to the choden function.

def account_interaction(account):
    print "You have $%i available" % (account)
    print """
    What would you like to do:
    [W]ithdrawl
    [D]eposit
    [T]ransfer funds
    """
    choice = raw_input("> ")
    if choice == "W":
        withdraw_funds(account)
    elif choice == "D":
        deposit_funds(account)
    elif choice == "T":
        transfer_funds(account)
    else:
        print "Invalid input.."

Ideally account would be a class, or at least a dictionary that contains multiple relevant values (ie. type, amount, account holder), either of these makes it easier to pass them around functions like this.


Counting the number of bits of a positive integer #2 I think the running time of this should be \$O(log_{2}(n))\$:

def count_bits(num):
    assert num >= 0
    count = 0
    while (num != 0):
        num >>= 1
        count += 1
    return count

print count_bits(0) #0
print count_bits(2) #2
print count_bits(pow(10, 2)) #7
print count_bits(pow(10, 9)) #30

Your code is neat, variables are well named, the input validation is there, etc. It's all good. The assessment of your time complexity being \$O(\log_2{n})\$ is also right.

But, is that as good as it can be? Well, your time-complexity assessment is a hint as to what's a better solution... The base-2 log is also an indication of the number of bits used. Remember, in base 2, the number of bits needed increases at the exponential of 2 as well.

As a consequence, your function could be reduced to \$O(1)\$ with:

import math

def count_bitx(num):
    assert num >= 0
    if num == 0:
        return 0
    return 1 + int(math.log(num, 2))

Note that Python 3.1 introduced the bit_length() method, so you could do:

def count_bits(num):
    assert num >= 0
    return num.bit_length()


Simple Python username scraper I started learning Python recently and I really like it, so I decided to share one of my first projects mainly in hopes of someone telling me what I can do to make it run faster (threading/multiprocessing?).

from requests import get
from bs4 import BeautifulSoup
from time import time
from re import compile

print('***PYTHON LEAGUE OF LEGENDS USERNAME SCRAPER***')
print('This script scrapes usernames from lolprofile.net')
region = input('Enter the region for scraping(eune/euw/na/br/tr/kr/jp/lan/las/oce/ru)\n')
numStart = input('What page to start on? Min 0\n')
numEnd = input('What page to end on? Min starting page + 1\n')

size = [] #for logging
#count = -1 #for logging

def setUrl(pageNum, region):
    global url
    url = 'http://lolprofile.net/leaderboards/'+region+'/'+pageNum

def is_ascii(i):
    return all(ord(c) < 128 for c in i)

setUrl(numStart, region)

start = time()

while int(numStart) != int(numEnd):
    print(len(size))
    page = get(url)
    soup = BeautifulSoup(page.text, "lxml")
    userName = [a.string for a in soup.findAll(href=compile('http://lolprofile.net/summoner/*'))]
    with open('usernames1.txt', 'a') as file:
        for i in userName:
            if is_ascii(i) and (' ' in i) == False:
                file.write('%s\n' % i.lower())
                size.append('0')
    numStart = int(numStart)
    numStart += 1
    setUrl(str(numStart), region)
    #count += 1
    #if count % 250 == 0: #every n iterations print progress
    #    print(len(size))

end = time()

print(len(size),'usernames scraped in a total of',end-start,'seconds')

If you're after speed, I'd suggest scrapy. I was looking for an excuse to try it out and saw your question. When I ran your code on the first 10 pages of the NA leaderboard, it took a little over 4 seconds. Running the below takes about 0.3 seconds, presumably due to initiating all the HTTP requests in parallel:

test.py:

class LolSpider(scrapy.Spider):
    name = 'lolspider'
    start_urls = ['http://lolprofile.net/leaderboards/na/{}'.format(page) for page in range(10)]

    def parse(self, response):
        for name in response.xpath('//a[re:test(@href, "http://lolprofile.net/summoner/")]//text()').extract():
            yield { 'name': name }

Running:

$ scrapy runspider test.py -o names.json

names.json:

[
{"name": "<first name here>"},
{"name": "<second name here>"},
...
]

To actually provide some code review feedback:

import requests # I prefer this and then requests.get over "from requests import get", since "get" is too common a word
from bs4 import BeautifulSoup
import time # ditto here
import re # and here

print('***PYTHON LEAGUE OF LEGENDS USERNAME SCRAPER***')
print('This script scrapes usernames from lolprofile.net')
region = input('Enter the region for scraping(eune/euw/na/br/tr/kr/jp/lan/las/oce/ru)\n')
num_start = int(input('What page to start on? Min 0\n')) # cast to int once here
num_end = int(input('What page to end on? Min starting page + 1\n')) # ditto

size = 0 # use a simple count rather than a list

# Python style dictates snake case
# get the URL rather than set a global variable
def get_url(page_num, region):
    # use string formatting rather than concatenation
    return 'http://lolprofile.net/leaderboards/{}/{}'.format(region, page_num)

def is_ascii(i):
    return all(ord(c) < 128 for c in i)

start = time.time()

# for loop instead of while avoids the need to increment by hand
for page_num in range(num_start, num_end + 1):
    url = get_url(page_num, region)
    print(size)
    html = requests.get(url).text
    soup = BeautifulSoup(html, "html.parser") # html.parser
    # /.* (slash and then anything) rather than /* (any number of slashes) in the regular expression
    user_names = [a.string for a in soup.findAll(href=re.compile('http://lolprofile.net/summoner/.*'))]
    with open('usernames1.txt', 'a') as file:
        for i in user_names:
            if is_ascii(i) and ' ' not in i: # not in
                file.write('%s\n' % i.lower())
                size += 1

end = time.time()

print('{} usernames scraped in a total of {} seconds.'.format(size, end-start))


C function to fully consume a Python iterator I was reading into the proper way to consume an iterator on this question as well as here and I notices there is no actual proper function for this that clearly explains itself.

Using:

collections.deque(maxlen=0).extend(iterable)

isn't exactly very descriptive whereas making a properly named method for this that just uses the deque approach is good I wanted to do my own research as well as learn some c (which I've rarely ever actually dealt with) along the way.

So.. Using the approach that is used in _collectionsmodule.c I have come up with this:

#include "Python.h"
#include <ctype.h>

static PyObject *
consume(PyObject *self, PyObject *iterable)
{
    PyObject *(*iternext)(PyObject *);
    PyObject *item;

    if (PyObject_GetIter(iterable) == NULL)
        return NULL;

    iternext = *Py_TYPE(iterable)->tp_iternext;
    while ((item = iternext(iterable)) != NULL)
        Py_DECREF(item);

    if (PyErr_Occurred()) {
        if (PyErr_ExceptionMatches(PyExc_StopIteration))
            PyErr_Clear();
        else
            return NULL;
    }
    Py_RETURN_NONE;
}

static PyMethodDef consumeMethods[] = {
    {"consume", (PyCFunction)consume, METH_O,
      "Run an iterator to exhaustion."},
    {NULL, NULL, 0, NULL}
};

static struct PyModuleDef _consumemodule = {
    PyModuleDef_HEAD_INIT,
    "_consumemodule",
    "Consume Module",
    -1,
    consumeMethods,
    NULL,
    NULL,
    NULL,
    NULL
};

PyMODINIT_FUNC
PyInit__consumemodule(void)
{
    return PyModule_Create(&_consumemodule);
}

It performs marginally better than the deque approach which is expected as it's using almost the same approach although I was wondering if any of the c guru's out there could see if I'm missing anything or if something could be done better as all it's doing it exhausting the iterator. I tried using METH_FASTCALL but that doesn't perform as well as METH_O and in the while loop I tried usingwhile ((Py_XDECREF(iternext(iterable))) != NULL) the only way I could find to eliminate the need for the variable item was by making a macro like this:

static inline int _Ret_DECREF(PyObject *op)
{
    if (op == NULL)
        return 0;
    Py_DECREF(op);
    return 1;
}

#define Ret_DECREF(op) _Ret_DECREF(op)

Then just looping through like this:

while (1) {
    if (Ret_DECREF(iternext(iterable)))
        break;
}

But something just feels off doing that.

You’ve got an object leak.

PyObject_GetIter() returns a new instance, which you aren’t calling Py_DECREF() on.

If you pass in an iterable object to your function (a list, a tuple, a dictionary, etc), PyObject_GetIter() will return a brand new iterator over than object.  You test the return value, discover it is not NULL, and then lose the returned value, since it was not stored in a local variable.  Then, you try to retrieve tp_iternext from the iterable’s type, which is likely not defined, so chaos will ensue.

If you pass in an iterator object to your function, PyObject_GetIter() will return it, but also increment the reference count of that object by one.  At the end of your function, you don’t call Py_DECREF() on the returned iterator, so the reference count will remain incremented, and the iterator will never be released.

static PyObject *
consume(PyObject *self, PyObject *iterable)
{
    PyObject *(*iternext)(PyObject *);
    PyObject *item;
    PyObject *iterator;

    iterator = PyObject_GetIter(iterable);  // No references to iterable beyond this point.
    if (iterator == NULL)
        return NULL;

    iternext = *Py_TYPE(iterator)->tp_iternext;
    while ((item = iternext(iterator)) != NULL)
        Py_DECREF(item);

    Py_DECREF(iterator);

    if (PyErr_Occurred()) {
        if (PyErr_ExceptionMatches(PyExc_StopIteration))
            PyErr_Clear();
        else
            return NULL;
    }

    Py_RETURN_NONE;
}


The Odd-Even Challenge After posting my first question on here, I got a lot of great feedback. I've decided to try another challenge and write it as cleanly as possible.

The rules of Odd-Even consists of the following tasks:

Given a max number a string should be returned counting up to and including that max number and adhere to the following rules in order:

Print "Even" instead of number, if the number is even, which means it is divisible by 2.
Print "Odd" instead of number, if the number is odd, which means it is not divisible by 2 and is not a prime (it should be a composite).
Print the number if it does not meet above two conditions.

I came up with the following code:

def create_oddeven_string(max_number):
    """Returns a string with numbers up to and including max_number."""

    return ",".join(create_oddeven_list(max_number))

def create_oddeven_list(max_number):
    """Returns a list with string representations of oddeven parsed numbers up to and
    including the max_number."""

    return map(parse_oddeven_number, xrange(1, max_number+1))

def parse_oddeven_number(number):
    """Returns the string "Even" when number is even, "Odd" when number is odd
    and composite, or the string representation of the number if prime."""

    if is_divisable(number, 2):
        return "Even"
    elif is_composite(number):
        return "Odd"
    else:
        return str(number)

def is_divisable(number, modulo):
    """Returns True if number % modulo == 0."""

    return number % modulo == 0

def is_composite(number):
    """Returns True if number is not a prime number (only divisable by itself and 1)."""

    if number <= 3:
        return False
    elif is_divisable(number, 2) or is_divisable(number, 3):
        return True
    else:
        i = 5
        while i*i <= number:
            if is_divisable(number, i) or is_divisable(number, (i+2)):
                return True
            i = i+6
        return False

What does not sit well with me is the parse_oddeven_number(number) function. If new situations arrive then that list is going to be a long if, elif, else structure that I'd rather not have. But I don't know what I do about simplifying that function.

Your code looks good and is well documented.

A few details :

instead of having is_composite function, I'd rather have a is_prime function : it is more common and it reuses the terms used to describe the problem. (You just have to swap True/False).
I wouldn't define a is_divisable function but it may be a good idea if you think it is necessary. However, the proper English word is "divisible" and not "divisable". Also, I wouldn't call the second argument "modulo" to avoid mixing argument name and operator name. Wikipedia suggests "modulus". Finally, the docstring should tell more about what is intended and less about how it is implemented. """Returns whether number is divisable by modulus""".
Because you know an upper bound for the number you'll consider during primality check, it may be a good idea to use the Sieve of Eratosthenes.


String splitting in C I am trying to improve my C skills, and I hope someone might be able to provide me with some feedback on the following code.

It is just basic string splitting, it should behave the similar to Ruby's String#split or Clojure's clojure.string.split. I couldn't think of a simple/efficient way to create an array of variable-size strings so I went the callback route.

Anyway, any and all feedback is greatly appreciated, thank you! Check out the code:

void strsplit(char *str, char *delim, int limit, void (*cb)(char *s, int idx))
{
  char *search = strdup(str);

  if (limit == 1) {
    cb(search, 0);
  }
  else {
    int i = 0, count = 0, len = strlen(str), len_delim = strlen(delim);
    char *segment = NULL, *leftover = NULL;
    limit = limit > 0 ? limit - 1 : len;

    segment = strtok(search, delim);
    for (i = 0; segment != NULL && i < limit; i++) {
      count += strlen(segment) + len_delim;
      cb(segment, i);
      segment = strtok(NULL, delim);
    }

    if (len != limit && count < len) {
      leftover = (char*) malloc(len - count + 1);
      memcpy(leftover, str + count, len - count);
      leftover[len - count] = '\0';
      cb(leftover, i);
      free(leftover);
    }
  }
}

Also see the code with a test framework.

I'd suggest not rolling your own implementation; just use strtok_r properly and save yourself some time. For memory allocation, you can either use the offsets into the string in-place, or use strndup to get copies of each token as you find it.


Initializing the edge values of 2D array in Java I have been asked to write Java method code to do the below:

+---+---+---+---+---+
| 0 | 0 | 0 | 0 | 0 |
+---+---+---+---+---+
| 0 |   |   |   | 0 |
+---+---+---+---+---+
| 0 |   |   |   | 0 |
+---+---+---+---+---+
| 0 |   |   |   | 0 |
+---+---+---+---+---+
| 0 | 0 | 0 | 0 | 0 |
+---+---+---+---+---+

I am assuming that they are NOT asking me to create a char[5][5];
i.e :

char[][] grid = {
    {'0','0','0','0','0'},
    {'0',' ',' ',' ','0'},
    {'0',' ',' ',' ','0'},
    {'0',' ',' ',' ','0'},
    {'0','0','0','0','0'}
};

I am assuming that the method will take a 2D array of int i.e int[][] and just reset the fence values to zero, and not changing the inner values. And that the array sizes is not fixed.

Having assumed that, I came up with below answer:

// Original answer
public int[][] initGrid1(int[][] grid) {
    int initVal = 0;
    int maxRows = grid.length;
    int maxCols = grid[0].length;

    for (int rows = 0; rows < grid.length; rows++) {
        for (int cols = 0; cols < grid[0].length; cols++) {
            if (rows == 0 || rows == maxRows - 1) {
                grid[rows][cols] = initVal;
            }
            if (cols == 0 || cols == maxCols - 1) {
                grid[rows][cols] = initVal;
            }
        }
    }
    return grid;
}

The response I got was: Isn't there a simpler way to do this?

Below are some of options I later came up with. I would like to know if you know of simpler way to do. Hoping to do this with Java8 features but found it difficult as individual occurrence needed to be identified for location.

// Option 2
public int[][] initGrid2(int[][] grid) {
    int initVal = 0;
    int maxRow = grid.length - 1;
    int maxCol = grid[0].length - 1;

    for (int rows = 0; rows < grid.length; rows++) {
        grid[rows][0] = initVal;
        grid[rows][maxCol] = initVal;
        for (int cols = 0; cols < grid[0].length; cols++) {
            grid[0][cols] = initVal;
            grid[maxRow][cols] = initVal;
        }
    }
    return grid;
}

If the grid is assumed to be a square.

// Option 3
public int[][] initGrid3(int[][] grid) {
    int initVal = 0;
    int maxlen = grid.length - 1;
    for (int j = 0; j < grid.length; j++) {
        grid[j][0] = initVal;
        grid[j][maxlen] = initVal;
        grid[0][j] = initVal;
        grid[maxlen][j] = initVal;
    }
    return grid;
}

If I traverse through the array with a cursor (c):

// Option 4
public int[][] initGrid4(int[][] grid) {
    int initVal = 0;
    int xL = grid.length;
    int yL = grid[0].length;
    int max = xL * yL;
    for (int c = 0; c < max; i++) {
        int x = c / xL;
        int y = (c == 0) ? 0 : c % yL;
        if (x == 0 || x == (xL - 1)) {
            grid[x][y] = initVal;
        }
        if (y == 0 || y == (yL - 1)) {
            grid[x][y] = initVal;
        }
    }
    return grid;
}

Using Java 8 feature, I was stuck with below, I don't if there is an option here:

grid[0] = IntStream.range(0, grid.length).map(i -> 0).toArray();
...don't know what to do in between.
grid[maxlen-1] = IntStream.range(0, grid.length).map(i -> 0).toArray();

1) So the big question; what is simpler?: (readability and maintainability) or smaller code or using latest features? Of course your assistance should not be just words but also code snippet.

2) And do you agree with my first answer?

Could it be that you make too many assumptions? Probably you should really create a new array and not "write the fence" in an existing one.

I guess this would simplify the code by just creating an all zero arrays for the first and last element of the outer array and an array with only the zero at start and the end for all other rows.

Given your assumptions are correct. Using a function to fill the array would simplify your solution significant. In Java this is done with Array.fill.

private static int[][] initGrid(final int[][] grid) {
    assert grid.length >= 2;
    final int fence = 0;
    Arrays.fill(grid[0], fence);
    for(int[] row : grid) {
       row[0] = row[row.length -1] = fence;
    }
    Arrays.fill(grid[grid.length - 1], fence);
    return grids;
}


Bitmap problem performance boost Problem statement:

  Input is a rectangular bitmap like this:

0001
0011
0110

  The task is to find  for each black (0) "pixel", the distance to the
  closest white (1) "pixel". So, the output to the above should be:

3 2 1 0
2 1 0 0
1 0 0 1

Below is a working, heavily-commented 78-lines solution. It is very naive, though.  How can I make it faster? I know I can search for ones in rectangles centered at a given zero, but I am not sure I need it that complex. I just want to run it under 4s with 200*200 input on Pentium III.

#include <stdio.h>
#include <vector>
#include <utility>

using namespace std;

typedef unsigned short T;

//Maximum size of the array
const size_t MAX = 200;

//Converts string to unsigned integer
T atou(char* s) {
    T x = 0;
    while(*s) x = x*10 + *(s++) - '0';
    return x;
}

//Calculates absolute value
T abs(int a) {
    if(a >= 0) return a;
    else return -a;
}

int main() {
    T testCases, n, m, i, j, min, curDist;
    T A[MAX][MAX];
    char row[MAX];
    //Storing coordinates (i,j) of ones and zeroes from the input matrix
    //hopefully saves a bit of time searching for closest ones
    vector< pair<T,T> > cOnes;
    vector< pair<T,T> > cZeroes;
    pair<T,T> nextPair;
    scanf("%hu",&testCases);
    while(testCases--) {
        //Input matrix is n X m
        scanf("%hu",&n);
        scanf("%hu",&m);
        //Starting from i=1, j=1 for clarity: dealing with i-th row, j-th column
        for(i = 1; i <= n; i++) {
            scanf("%s",row);
            for(j = 1; j <= m; j++) {
                //But then have to subtract one when dealing with char[]
                A[i][j] = atou(&row[j-1]);
                if(row[j-1] == '1') {
                    cOnes.push_back(pair<T,T>(i,j));
                    //Clearing the array for the next test case
                    A[i][j] = 0;
                }
                else cZeroes.push_back(pair<T,T>(i,j));
            }
        }
        //For all zeroes find the closest one
        while(!cZeroes.empty()) {
            nextPair = cZeroes.back();
            i = nextPair.first;
            j = nextPair.second;
            cZeroes.pop_back();
            std::vector< pair<T,T> >::iterator it = cOnes.begin();
            min = abs(it->first - i) + abs(it->second - j);
            while(it != cOnes.end()) {
                curDist = abs(it->first - i) + abs(it->second - j);
                if(curDist < min) min = curDist;
                it++;
            }
            A[i][j] = min;
        }
        //Print the result
        for(i = 1; i <= n; i++) {
            for(j = 1; j <= m; j++) {
                printf("%hu ", A[i][j]);
                A[i][j] = 0;
            }
            printf("\n");
        }
    }
    return 0;
}

I'd start by converting all the 1's in the input to some value that can't appear in the output  (e.g., -1) and saving each of those locations.

Then I'd basically do a modified flood-fill, starting from each -1. Look at each of its neighbors, and any there are currently 0, set to 1 and save that location. Repeat for every other -1 in the input.

Then walk through all the saved locations. Each neighbor of those that's a zero, set to 2 (and, again, save those locations).

Repeat until you reach an iteration that doesn't find any more 0s in the input (and obviously, incrementing the distance number for each of these outer iterations).

Finally, change all the -1s back to 0s.

As far as your code itself goes, it's tagged C++, but a great deal of it is very C-like. You have a couple of vectors, but quite a few C-things that most C++ programmers would rather avoid (e.g., printf, scanf and built-in arrays). I'm also less than excited about some of the names you've used (e.g., cZeroes and currDist). I'd rather see the names spelled out more completely (e.g., current_distance or, if you insist on camelCase, currentDistance).


Property file loader I have the following code:

public final class PropertyFileLoader {

    private static final Logger logger = LoggerFactory.getLogger(PropertyFileLoader.class);

    private final String propertyFileName;

    public PropertyFileLoader (String propertyFileName) {
        this.propertyFileName = propertyFileName;
    }

    public Properties getClasspathFileAsStream () {
        final Properties properties = new Properties();
        final InputStream is = getClass().getClassLoader().getResourceAsStream(propertyFileName);

        try {
          properties.load(is);
        }
        catch (final IOException e) {
          logger.error(e.getLocalizedMessage(), e);
         //      throw new IOException();
        }
        return properties;
    }
}

How do you think, should I use static methods instead of constructor?
The usage of this class:

final PropertyFileLoader loader = new PropertyFileLoader("flyway.properties");
final Properties flywayProperties = loader.getClasspathFileAsStream();

And I think add the static method such as

public static Properties getClasspathProperties(String propertyFileName){
    final PropertyFileLoader loader = new PropertyFileLoader(propertyFileName);
    return loader.getClasspathFileAsStream();`
}

Should I throw IOException, if I use classpath directory? If yes, why?

Could I use this class as Spring @Component with @Value?
I found how to @Autoware constructor with param using @Value annotation, but how to use this at another class?

Encapsulation and information hiding

First of all, this looks poorly conceived:

public Properties getClasspathFileAsStream() {

It seems you named the method according to how it's implemented,
which is a sign of bad design.
Following good encapsulation / information hiding principles,
users shouldn't need to know about implementation details.
A method that returns Properties is better named getProperties or loadProperties. I use the former naming style when the properties are ready to return, and the latter to imply I/O operations behind the scenes (loading from filesystem, database, remote service), hinting the possibility of slowness and failures.

Initialize in constructor use static method?

This seems to be one of your main questions:
should you pass the properties file name as constructor parameter,
or use a static method with a filename parameter?
It depends on how often you want to load the properties file.

If you need to load the properties file only once during the lifetime of the program,
then there's no need to store the filename in a class field,
a static method will be simple and straightforward.

If on the other hand you want to use getProperties multiple times,
then it makes sense to store data in class fields.
Even so, you have two alternatives:

Store the filename in a field, and use a loadProperties method to load the content of the file on every call.
Load the content of the file in the constructor, and store the Properties data in a field, returned by getProperties. In this alternative there's no need to store the filename in a field at all.

Again, the suitable alternative depends on how you want to use this class,
and if the content of the properties file is expected to change during the execution of the program.

Throw an exception or not

Throw an exception if the caller can or should handle it.
Perhaps the caller should try to load properties from a different source.
Or perhaps it should exit the program in case the properties cannot be loaded.

Don't throw an exception if properties are not essential data,
and missing data is not a problem.
In this case you can handle I/O exceptions in this class,
and return new Properties().

The right approach depends on your use case.


Classes for user registration and authentication <?php
namespace Users;

use mysqli;

class Users {
    protected $db;
    public function __construct() {
        $this->db = new mysqli('localhost', 'root', 'php123', 'sms');
    }
    public function login($username, $password) {
        $query = $this->db->prepare("SELECT username, password FROM users WHERE username=? AND password=? LIMIT 1");
        $password = UserData::hash($password);
        $query->bind_param('ss', $username, $password);
        $query->execute();
        $query->bind_result($username, $password);
        $query->store_result();
        if($query->num_rows ==1) {
            while($query->fetch()) {
                $_SESSION['loggedin'] = 1;

                UserData::getAndSet($username);

                header("Location: dashboard.php");
            }
        } else {
            return false;
        }
        $query->free_result();
        $query->close();
    }
    public function register($username, $password, $rank, $name) {
        $errors = array();
        $query = $this->db->prepare("INSERT INTO users VALUES('', $username, $password, $rank, $name);");

        $password = UserData::hash($password);

        $query->bind_param('ssss', $username, $password, $rank, $name);

        if(strlen($username) < 3 || strlen($username) > 15) {
            $errors[] = "The length of the username can only range from 3 to 15 characters.";
        }
        if(strlen($password) < 3 || strlen($password) > 15) {
            $errors[] = "The length of the password can only range from 3 to 15 characters.";
        }
        if(strlen($name) < 3 || strlen($name) > 15) {
            $errors[] = "The length of the name can only range from 3 to 15 characters.";
        }

        $query->execute();
        $query->close();
    }
}
class UserData {
    protected $db;

    public $username;
    public $password;
    public $rank;
    public $name;

    public function __construct() {
        $this->db = new mysqli('localhost', 'root', 'php123', 'sms');
    }

    public function getAndSet($username) {
        $query = $this->db->prepare("SELECT username, password, rank, name FROM users WHERE username=?");
        $query->bind_param('s', $username);
        $query->execute();
        $query->bind_result($username, $password, $rank, $name);
        $query->store_result();
        while($query->fetch()) {
            $this->username = $username;
            $this->password = $password;
            $this->rank     = $rank;
            $this->name     = $name;
        }
        $query->free_result();
        $query->close();
    }
    public function hash($string) {
        $fSalt = "{z6Q9&2eHn69,9/cXa=<,Nm[?^^Tw-";
        $sSalt = ":L+;fwS<q%)3f?N_d)(Ta_*fN&ax}+";

        $sting = md5(md5($fSalt) . md5($string) . md5($sSalt));

        for($i=0; $i < 3; $i++) {
            $string = md5($string);
            $string = sha1($string);
            $string = bin2hex($string);
            $string = md5($string);
        }

        return $string;
    }
}
class UserUtil {
    protected $db;
    public function __construct() {
        $this->db = new mysqli('localhost', 'root', 'php123', 'sms');
    }
}

$Users    = new Users();
$UserData = new UserData();
$UserUtil = new UserUtil();

?>

This is the beginning of my project, "Student Management System" and before I proceed I want this code to be rated, and how can I improve this code with better ways.

Why are you starting a new database connection for each class? Pass in a database connection object to the constructor:

public function __construct(mysqli $dbConnection) {

This allows your class to work without worrying about actually doing the connection. The class's job is to handle users, not handle database connections!
You not only said LIMIT 1 in your query, but you also check that $query->num_rows is 1, and then you even loop over the results (even though there's only one). This sort of triple check is redundant and unreadable. You know there's either 1 or 0 results because you LIMITed the query!
Your hashing algorithm is not secure. Use PHP 5.5's password_hash(), or if you don't use 5.5 (And you should!!!) use Password Compat, a library written by the same guy who wrote the password_hash() function for PHP's core.
Do not limit your users' password length - Setting a minimum length is good. Setting a maximum length is a kitten killing crime. Don't.
Your login() method returns false on error, but nothing on success. Either return true on success, or simply throw an exception on errors.
Your UserData class is not a class, it's a namespace. You only use it for static functions (even though you don't declare them as static in your class!).
What does your getAndSet method does? It's not clear. Use a proper name or add documentation.
Your UserUtil class does absolutely nothing.

Generally, I'd say you can improve the structure of your code by having the following 3 classes:

UserService - is responsible for registering, authenticating and verifying users using the following two objects. Basically higher level management.
User - Represents a single user in the system. Has absolutely no database interaction whatsoever, and has no idea where it came from. It accepts parameters through the constructor and getters/setters:

class User {
    public function __construct($id, $username, $hashedPassword, $name) {

UserMapper - Which is responsible for mapping and/or creating user objects to/from the database. For example

class UserMapper {
    public function fetch(User $user) {
        //$user object will already have the ID filled in. Fetch according to that and fill the same $user object.
    }
}

Example of execution:

class UserService {

    private $mapper;

    public function __construct(UserMapper $mapper) { ... }

    public function register($username, $password, $name) {
        $user = new User($username, $password, $name);
        $user->validateRegistrationDetails(); //Throws ValidationException on validation error.
        $user->generateHash();
        $this->mapper->save($user); //Save user to database;
    }
}

Note that in the above, the User is unaware of the database, which means it's perfectly reusable across applications with different storage systems.


Return list of common email ids from a list of lists Input: ArrayList of ArrayLists with email ids.
Output: Return ArrayList of email ids which appears in each inner ArrayList.

Ex: Input: [[abc@gmail.com, bca@gmail.com],[abc@gmail.com,bca@gmail.com,bca@gmail.com],[abc@gmail.com,xyz@gmail.com]]
Output: [abc@gmail.com]

I have written the code using HashSet and HashMap and could get the required output but I'm not sure how optimized and efficient my method is.

public static List<String> Common(List<List<String>> email1){

        List<String> res= new ArrayList<String>();
        HashMap<String,Integer> map = new HashMap<String,Integer>();
        Set<String> set ;
        int cou = 0;
        for( List list : email1)
        {
            set = new HashSet<String>();
            List<String> temp = list;
            for(String i: temp){
                if(!set.contains(i)){
                    if(map.get(i) !=null){
                        map.put(i, map.get(i)+1);
                    }else{
                        map.put(i, 1);
                    }
                }
            }
            cou++;
        }

        int size = email1.size();

        for(String count: map.keySet()){
            if(map.get(count)== size)
            {
                System.out.println(count);
                res.add(count);
            }
        }

        return res; // << this
    }

Using a HashSet is fine, but you don't need to keep the count of occurrence of each email, you can just keep in the HashSet those that appear in all list, and remove all other emails.

//Need to check if email1 is empty before this step!
HashSet<String> set = new HashSet();
set.addAll(email1.get(0));
for(int i = 1; i < email1.size(); i++){
    HashSet<String> tmp = new HashSet();
    for(String email : email1.get(i)){
        if(set.contains(email)){//If set contains this email, keep it!
           tmp.add(email);
        }
    }
    set = tmp;//Important step
}
//All emails will be in the set
System.out.println(set);


Fibonacci sequence and quadratic equation using functions I know this code works, but is it correct to define them as functions or should I use something else?

import math

def Fibonnacci_sequence():
    i=int(input("How many numbers of the fibonnacci sequence do you want to see: "))
    a = 1
    b = 1
    n = 1
    print(a)
    print(b)
    n = n + 1
    while n < i:
        c = a + b
        print(c)
        n = n + 1
        a = b
        b = c

def Fib_seqnum():
    i=int(input("What number of the fibonnacci sequence do you want to know: "))
    a = 1
    b = 1
    n = 1
    n = n + 1
    while n < i:
        c = a + b
        n = n + 1
        a = b
        b = c
    print(c)

def solve_qaudratic():
    a=float(input("Enter A: "))
    b=float(input("Enter B: "))
    c=float(input("Enter C: "))
    d = (b*b) - 4*a*c
    if d < 0:
        print("No real roots")
    elif d == 0:
        x = -b/(2*a)
        print("equal roots" (x))
    else:
        x = math.sqrt(d)
        i = (-b -x)/(2*a)
        o = (-b +x)/(2*a)
        print("X1 = " (i))
        print("X2 = " (o))

print("Welcome to maths")
print("PRESS 1 TO LOOK AT THE FIBONNACCI SEQUENCE")
print("PRESS 2 TO SOLVE A QUADRATIC")
print("PRESS 9 TO EXIT")
answer=int(input("Enter Choice: " ))
if answer==1:
    Fibonnacci_sequence()
elif answer==2:
    solve_qaudratic()
elif answer==3:
    Fib_seqnum()
elif answer==9:
    exit()
else:
    print("WHAT?!")

Your functions do too many things:

get input from user
do some calculation
print result

It would be better to separate these responsibilities,
following the single responsibility principle.

When you separate these responsibilities, the Fibonacci generation logic will naturally end up in its own function, and the reusability becomes obvious.

Consider this rewrite of your Fibonnacci_sequence function:

def fib_sequence(n):
    """
    >>> fib_sequence(10)
    [1, 1, 2, 3, 5, 8, 13, 21, 34, 55]

    :param n: the number of items to calculate
    :return: list of the first n items in the sequence
    """
    a = 1
    b = 1
    k = 2
    seq = [a, b]
    while k < n:
        c = a + b
        seq.append(c)
        k += 1
        a = b
        b = c

    return seq

The improvements:

removed the printing
removed the user input
follows PEP8
better variable names
documented
has a doctest

The most interesting part is the doctest.
You can run doctests with the command:

python -m doctest script.py

Let's add a bunch more doctests:

>>> fib_sequence(0)
[]

>>> fib_sequence(1)
[1]

>>> fib_sequence(2)
[1, 1]

>>> fib_sequence(3)
[1, 1, 2]

And let's rerun the doctests. Which of these new tests will fail? fib_sequence(0) and fib_sequence(1) will fail. Just like your original implementation always printed the first two 1s, this one also always returns a list starting with [1, 1, ...].
The good thing is that now that we have the tests, we can go ahead and confidently refactor the implementation.

As a matter of fact, a nice way to generate sequences in Python is using generators. Here's an implementation of the Fibonacci sequence using a generator:

def fib():
    yield 1
    yield 1
    prev = 1
    current = 1
    while True:
        prev, current = current, prev + current
        yield current

Now we can rewrite fib_sequence in terms of this new fib function:

it = fib()
return [next(it) for _ in range(n)]

If we rerun the doctests, now they all pass.
We can further simplify this implementation using itertools.islice:

return list(itertools.islice(fib(), 0, n, 1))

To finish up, let's rewrite Fibonnacci_sequence in terms of fib_sequence:

def fibonacci_sequence():
    n = int(input("How many numbers of the Fibonacci sequence do you want to see: "))
    for num in fib_sequence(n):
        print(num)

It's a lot cleaner isn't it.

I invite you to write a fib_nth method similarly, following the same logical steps we took for fib_sequence:

remove the prompting and printing, make it return a value
write doctests, for all the interesting cases
rewrite fib_nth in terms of fib
rewrite fib_seqnum in terms of fib_nth


WCF service and database I have a WCF service that work with databse. The service should handle incoming message and store messagId into database. Please suggest me how can I improve it?

public class DBWorker
{
    static DBWorker instance;
    private string connectionString;
    private SqlConnection connection;

    protected DBWorker()
    {
        connectionString = GetConncetionString();
        connection = new SqlConnection(connectionString);
        connection.Open();
    }

    public static DBWorker Instance()
    {
        if (instance == null)
            instance = new DBWorker();
        return instance;
    }

    private string GetConncetionString()
    {
        var conn = ConfigurationManager.ConnectionStrings["FsConnectionString"];
        if (conn == null | string.IsNullOrEmpty(conn.ConnectionString))
          throw new Exception("Couln't find connection string in app.config.");
        return conn.ConnectionString;
    }

    public void StoreMessage(string messageId)
    {
        SqlCommand cmd = new SqlCommand("insert into test (MessageId) values (@MessageId)", connection);
        cmd.Parameters.AddWithValue("@MessageId", messageId);
        cmd.ExecuteNonQuery();
    }

}

My WCF:

public class HostTest : IHostTest
{
    private DBWorker dbWorker;

    public HostTest()
    {
        dbWorker = DBWorker.Instance();
    }

    public Stream HandleIncomingMessage()
    {
        var currentContext = WebOperationContext.Current;
        var request = currentContext.IncomingRequest;
        var response = currentContext.OutgoingResponse;

        string messageId = request.Headers["X-Message-Id"];

        response.StatusCode = HttpStatusCode.OK;
        response.ContentType = "text/plain";

        if (request.ContentLength == 0 )
        {
            response.StatusCode = HttpStatusCode.NoContent;
        }
        else
        {
            try
            {
                dbWorker.StoreMessage(messageId);
            }
            catch(Exception e)
            {
                response.StatusCode = HttpStatusCode.InternalServerError;
            }
        }

        return new MemoryStream(UTF8Encoding.Default.GetBytes(messageId)); ;
    }
}

SqlConnection, SqlCommand, etc. all implement IDisposable and thus they should be encapsulated in using blocks. Instead you tie SqlConnection to a singleton class, which is the opposite of that.

Here's how they should be used:

using (SqlConnection con = new SqlConnection(connectionString))
{
    con.Open();

    using (SqlCommand command = new SqlCommand("SELECT TOP 2 * FROM Dogs1", con))
    using (SqlDataReader reader = command.ExecuteReader())
    {
        while (reader.Read())
        {
            Console.WriteLine("{0} {1} {2}",
            reader.GetInt32(0), reader.GetString(1), reader.GetString(2));
        }
    }
}


Finding all factors of n efficiently This function generates all factors of an input n.

It begins at the square root of n, and checks numbers down to zero. When a factor x is found, then another factor must be n divided by x.

Also, if n is odd, only odd numbers need to be checked.

from math import sqrt

def factors(n):
    """
    Generates all factors of n. Iterates from sqrt(n) down to zero.
    When a factor x is found, another factor must be n divided by x.
    If n is odd, only odd numbers need to be checked.

    >>> sorted(list(factors(1001)))
    [1, 7, 11, 13, 77, 91, 143, 1001]
    """
    root = sqrt(n)
    start = int(root)  # Default starting number is sqrt(n)

    if n % 2 == 0:
        step = -1  # n is even, so check both evens and odds
    else:
        step = -2  # n is odd, so check only odds
        start = start // 2 * 2 + 1  # Round start to odd number

    if root.is_integer():
        yield int(root)  # sqrt(n) is a factor of n
        # Start at numbers < sqrt(n), so that sqrt(n) is not yielded twice
        start += step

    for x in range(start, 0, step):
        if n % x == 0:
            yield x
            yield n // x

For example,

>>> sorted(list(factors(1000)))
[1, 2, 4, 5, 8, 10, 20, 25, 40, 50, 100, 125, 200, 250, 500, 1000]

This takes 31 iterations. But when the number is odd:

>>> sorted(list(factors(1001)))
[1, 7, 11, 13, 77, 91, 143, 1001]

This takes only 16 iterations.

I'm wondering if there are other tricks that could make the process more efficient.

For big numbers and for many numbers checking for divisibility with prime numbers under the sqrt will give a big speed increase, you could still use the same method. But there are many ways to optimize factorization, many many ways.

I like your code! But some styling notes and then a suggestion.
I think the usual way to check if a number is a perfect square is:

if root*root == n:
        yield int(root)
        start += step

the check:

root.is_integer()

is more implicit and explicit is better.

Now, in line comments is hard to read. You shouldn't use those.

if n % 2 == 0:
    step = -1  # n is even, so check both evens and odds
else:
    step = -2  # n is odd, so check only odds
    start = start // 2 * 2 + 1  # Round start to odd number

instead:

if n % 2 == 0:
    # n is even, so check both evens and odds
    step = -1
else:
    # n is odd, so check only odds
    step = -2
    # Round start to odd number
    start = start // 2 * 2 + 1

but even better would be to not repeat what the if statement already says.

if n % 2 == 0:
    # only check evens
    step = -1
else:
    # check only odds
    step = -2
    # Round start to odd number
    start = start // 2 * 2 + 1

start = start // 2 * 2 + 1

might be implicit, could be more readable:

start = int(root)
start += 1 if start % 2 == 0 else 0

Now to the suggestion

If with any number that is even, it has a number of factors 2. So remove the factors 2 and do you algorithm on that number. Like this:

def factors(n):
    """
    Generates all factors of n. Iterates from sqrt(n) down to zero.
    When a factor x is found, another factor must be n divided by x.
    If n is odd, only odd numbers need to be checked.

    >>> sorted(list(factors(1001)))
    [1, 7, 11, 13, 77, 91, 143, 1001]
    """

    root = sqrt(n)
    step = -2
    start = int(root)
    start += 1 if start % 2 == 0 else 0

    if root*root == n:
        yield int(root)
        start += step

    for x in range(start, 0, step):
        if n % x == 0:
            yield x
            yield n // x

def brigde(n):
    twos = 1
    while n % 2 == 0:
        twos += 1
        n //= 2

    for num in factorss(n):
        for i in range(twos):
            yield num*(2**i)

You'll have to multiply each of the factors returned with the factors of two that the original number contained. But you remove the need to check even numbers, ending up with only the faster of the two ways your code can go.


PDO prepared statement - binding variable number of values Please let me know if I have over/under explained my question :)

HTML table row - the numerals from each id attribute, in this example "408" and "409", are the database tables primary ID numbers, one database table row per HTML table cell.

    Delete

jQuery - I push these ids to an array and POST it to ajax.php

$('tbody').on('click','.deleteTableRow',function(){
var IDs = [];
$(this).closest('tr').find('td').each(function(){ IDs.push(this.id); });
var deleteTableRow = IDs;
$(this).parent().parent().remove();
    jQuery.ajax({
        type: "POST",
        url: "includes/ajax.php",
        data: {deleteTableRow: deleteTableRow},
        success: function (response) {
            $('body').append(response);
        },
        error: function (response) {
            alert("Error getting php file");
        }
    });
});

PHP - ajax.php. This works wonderfully, but is there a better way to bind a variable number of values into a single prepared statement? Currently I have HTML tables containing 2 MySql rows or 3 MySql Rows, but this will need to handle around a maximum of 10 MySql rows

if(isset($_POST['deleteTableRow'])){

    // Remove empty array value generated by the table cell containing my delete button
    $deleteRowArray = array_filter($_POST['deleteTableRow']);

    // Create an array of '?,' values - one for each id
    foreach($deleteRowArray as $addBind) {
        $array[] = '?,';
    }

    // Remove the last array values comma
    end($array);
    $array[key($array)] = '?';
    reset($array);

    // Create the finished query with as many binded values as needed
    $sql = 'DELETE FROM dataformset WHERE DataFormSetId IN (' . implode($array) .') LIMIT 3';
    $q = $db->prepare($sql);

    $i = 1;
    foreach($deleteRowArray as $deleteRow) {

        // Remove the text "FieldId" and bind each ID into a single prepared statement
        $deleteRow = preg_replace("/[^0-9,.]/", "", $deleteRow);
        $q->bindValue($i, $deleteRow, PDO::PARAM_STR);
        ++$i;
    }
$q->execute();
}

For this:

// Create an array of '?,' values - one for each id
foreach($deleteRowArray as $addBind) {
    $array[] = '?,';
}

// Remove the last array values comma
end($array);
$array[key($array)] = '?';
reset($array);

// Create the finished query with as many binded values as needed
$sql = 'DELETE FROM dataformset WHERE DataFormSetId IN (' . implode($array) .') LIMIT 3';

I guess the following is the same but much simpler:

$array = array_fill(0, sizeof($deleteRowArray), "?");
$sql = 'DELETE FROM dataformset WHERE DataFormSetId IN (' . implode(", ", $array) .') LIMIT 3';

You don't need to put the separator char into the array, implode can handle it and it doesn't put separator after the last element, so you don't have to remove it.

// Remove the text "FieldId" and bind each ID into a single prepared statement
$deleteRow = preg_replace("/[^0-9,.]/", "", $deleteRow);

I'd be a little bit more defensive here and remove only FieldId from the beginning of the string, not every non-allowed character. It's a sign of an error of the caller if you got a $deleteRow which doesn't start with FieldId. In that case you should indicate it somehow (throw an exception). There is no point to delete rows if the data is definitely invalid. (The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies.)


Linux bonding status check I have created the below code which works for me, but I feel there might be a better way to accomplish this in a new Pythonic way or better Python syntactic way.

Code snippet:

bondCheck.py

#!/usr/bin/python
# Port bonding checker.
# cat /proc/net/bonding/bond0

import sys
import re

def usage():
        print '''USAGE: %s [options] [bond_interface]

Options:
        --help, -h      This usage document

Arguments:
        bond_interface  The bonding interface to query, eg. 'bond0'. Default is 'bond0'.
''' % (sys.argv[0])
        sys.exit(1)

# Parse arguments
try:
        iface = sys.argv[1]
        if iface in ('--help', '-h'):
                usage()
except IndexError:
        iface = 'bond0'

# Grab the inf0z from /proc
try:
        bond = open('/proc/net/bonding/%s' % iface).read()
except IOError:
        print "ERROR: Invalid interface %s\n" % iface
        usage()

# Parse and output
active = 'NONE'
Link = 'NONE'
slaves = ''
state = 'OK'
links = ''
bond_status = ''
for line in bond.splitlines():
        m = re.match('^Currently Active Slave: (.*)', line)
        if m:
                active = m.groups()[0]

        m = re.match('^Slave Interface: (.*)', line)
        if m:
                s = m.groups()[0]
                slaves += ', %s' % s

        m = re.match('^Link Failure Count: (.*)', line)
        if m:
                l = m.groups()[0]
                links += ', %s' % l

        m = re.match('^MII Status: (.*)', line)
        if m:
                s = m.groups()[0]
                if slaves == '':
                        bond_status = s
                else:
                        slaves += ' %s' % s
                if s != 'up':
                        state = 'FAULT'

print "%s: %s (%s), Active Slave: %s, PriSlave: %s (%s), SecSlave: %s (%s), LinkFailCountOnPriInt: %s, LinkFailCountOnSecInt: %s"  % (iface, state, bond_status, active, slaves.split(',')[1].split()[0], slaves.split(',')[1].split()[1], slaves.split(',')[2].split()[0], slaves.split(',')[2].split()[1], links.split(',')[1], links.split(',')[2])

Code Returned Result:

$ ./bondCheck.py
bond0: OK (up), Active Slave: ens3f0, PriSlave: ens3f0 (up), SecSlave: ens3f1 (up), LinkFailCountOnPriInt:  0, LinkFailCountOnSecInt:  0

Edit:

Bond config

cat /proc/net/bonding/bond0
Bonding Mode: fault-tolerance (active-backup)
Primary Slave: ens3f0 (primary_reselect always)
Currently Active Slave: ens3f0
MII Status: up
MII Polling Interval (ms): 100
Up Delay (ms): 2500
Down Delay (ms): 300
Slave Interface: ens3f0
MII Status: up
Speed: 20000 Mbps
Duplex: full
Link Failure Count: 0
Permanent HW addr: 5a:19:bb:00:00:48
Slave queue ID: 0
Slave Interface: ens3f1
MII Status: up
Speed: 20000 Mbps
Duplex: full
Link Failure Count: 0
Permanent HW addr: 5a:19:bb:00:00:49
Slave queue ID: 0

As a rule I don't like to trigger exceptions, even controlled ones. One reason is that they are more expensive to handle, and as the name implies they should remain exceptions.

So rather than doing this:

try:
        bond = open('/proc/net/bonding/%s' % iface).read()
except IOError:
        print "ERROR: Invalid interface %s\n" % iface
        usage()

I would have something like this:

import sys, os

iface = 'bond0'
bond = ('/proc/net/bonding/%s' % iface)

if not os.path.exists(bond):
    print "ERROR: Invalid interface %s\n" % iface
    sys.exit(1)

# continue

NB: seeing that you are using Python 2.x I have tried to provide compatible code.

Rather than blindly trying to read a file that may not be there in the first place, I simply use os.path to ascertain its existence (could use the isfile function as well).

I also return an exit code, which could be useful if you will be calling this script from another bash script, then you might like to know if the output was as expected or if there is a possible error like a broken network link. Rather than just return 0 (success) or 1 (failure) you could adjust the value depending on the kind of error encountered. Then it's useful to have the special codes in mind.

I see that you are already using sys.exit(1) in your code, which is excellent, but it is in the usage function, where it's the least useful. Consider returning an exit code in the other parts of the code including exceptions. One way of doing it would be to have a main try catch block with a finally so that an exit code is always returned, which could be 0 by default.

You can still have exception handling in this block though, because you might run into some permission issue, depending on the user and perhaps the OS. It's just that I would avoid raising exceptions for situations that are easily anticipated and tested.

Consider logging exceptions and even console output or debugging information. A recent discussion on the topic: Custom exception handling function or logging library?

This is especially important for unattended scripts. In this case, it's probably less important if the script will always be run interactively. But it's good to have a trace on file when you are working in some tiny SSH or screen terminal session with a small buffer size and difficulty scrolling text.

Regarding semi-constant variables like /proc/net/bonding/ or bond0 I try to regroup them together on top of the script. The paths and interface names may vary from an OS to another and porting code to another platform is easier when essential variable names are not scattered all over the place. Obviously, they should be defined only once and repetition is to be avoided absolutely. I develop scripts for servers that run on at least 3 different flavors of Linux and I sometimes have surprises...

Regarding the code proper: the final line doing the print is quite hard to read. All that splitting is unnecessary and even unsafe. If the output of /proc/net/bonding/bond0 changes, or the number of slaves is less than 2, your code could fail.

Ideally, you should have ready-to-use variables and just print them. All the processing, parsing, validation etc has to take place before.

Let's take this snippet as an example:

    m = re.match('^Slave Interface: (.*)', line)
    if m:
            s = m.groups()[0]
            slaves += ', %s' % s

Your code iterates twice. At the first iteration the value of slaves is: , ens3f0. At the second iteration it is: , ens3f0 up, ens3f1. Note the leading comma.
It seems that you are not yet familiar with Python structures such as lists or dictionaries, so you are resorting to string manipulations techniques that are superfluous.
Here how I would do it, using a simple list:

First add this somewhere in your code, for example before the for loop:

slave_interfaces = []

We simply define an empty list. Then your code becomes:

    m = re.match('^Slave Interface: (.*)', line)
    if m:
            slave_interfaces.append(m.group(1).strip())

When a match if found, the value is appended to the list, which means adding an element. Note the addition of strip() to trim whitespace that may surround the interface name.

The regular expression can not only match but capture as well. Since m.group(1) already contains the name of the interface you can use it. Now, at the first iteration the value of slave_interfaces is: ['ens3f0']. At the second iteration it is: ['ens3f0', 'ens3f1']. You have a list of two elements, and you can address each of them by index number, thus slave_interfaces[0] = 'ens3f0' and slave_interfaces[1] = 'ens3f1'. You can check the number of elements: len(slave_interfaces) will return 2. So you know in advance that using an index number greater then 1 will result in an error (IndexError: list index out of range).

Producing a comma-separated list is as easy as joining:

','.join(slave_interfaces)

=> 'ens3f0,ens3f1'

On a final note, for parsing arguments and if you have Python 2.7 at least, the argparse library provides more flexibility. Have a look at the add_help function too. When you develop more sophisticated scripts with multiple arguments that may be in no particular order, your way of doing it will not scale well.


Use callback style to start a worker thread and receive result in Qt After using JavaScript for a while, I think callbacks are pretty good for asynchronous programming, but I haven't seen anything similar in C++. What I want to achieve is to start a worker thread from a function/lambda, and then use a callback to receive the result, which will be run in the main thread. The callback is required to run in the main thread because Qt GUI code is limited to run there. asyncRun is a convenient function to achieve the above. Let me know what you think.

template <typename AsyncFunc, typename Callback>
void asyncRun(QObject* obj, AsyncFunc asyncFunc, Callback callback) {
    auto asyncFuncWrapper = [=]() {
        auto result = asyncFunc();
        QTimer::singleShot(0, obj, std::bind(callback, result));
    };
    QtConcurrent::run(asyncFuncWrapper);
}

// Convenient function to run the callback in the main thread.
template <typename AsyncFunc, typename Callback>
void asyncRun(AsyncFunc asyncFunc, Callback callback) {
    asyncRun(QCoreApplication::instance(), asyncFunc, callback);
}

// Example usages
asyncRun([]() {
    qDebug() << "thread:" << QThread::currentThreadId();
    QThread::msleep(500);
    return 666;
}, [=](int result) {
    qDebug() << "thread:" << QThread::currentThreadId() << result;
});

This answer seems to be what you are looking for. I copy it here for reference:

There is this library that is highly integrated with Qt and implements javascript-like promises:

https://github.com/juangburgos/QDeferred

It allows to create a thread-safe async API as follows:

multiplyNumbersInThread(3, 4)
.fail([](int res) {
    Q_UNUSED(res);
    qDebug() << "multiplyPositiveNumbers failed!";
})
.done([](int res) {
    qDebug() << "multiplyPositiveNumbers succeded! Result :" << res;
});


Number of distinct elements in an array This is my solution to find the Number of distinct elements in an array. Is there away fast way to do this? It run-time is O(N).

    public int DistinctNumberOfItems( int[] A ) {

        if (A.length == 0) return 0;
        if(A.length == 1) return 1;

        Set<Integer> set = new TreeSet<Integer>();

        for (int i : A) {

            set.add(i);
    }
        return set.size();
}

Your code was better in Rev 1.  I don't know why you changed it.  Whenever you can get away without special cases, it's probably better to do so.

This code is a pure function of the input array and nothing else.  Therefore, it would probably be better as a static function.

Renaming the function from NumberOf… to …Count would make it less verbose.  You should also respect Java naming conventions, in which method names start with a lowercase letter.  The parameter name A also has unconventional capitalization.

As others have said, your algorithm, using TreeSet, is O(n log n).  You would need a HashSet for O(n).  When constructing a HashSet, it's a good idea to provide an estimate of the collection size, since a wrong guess would hurt performance.

public static int distinctItemCount(int[] array) {
    Set<Integer> set = new HashSet<Integer>(array.length);
    for (int i : array) {
        set.add(i);
    }
    return set.size();
}


Divide two numbers, then apply a custom rounding rule If the fractional part of the number is greater than 0.6, round up; if less, round down.
I need feedback about my code.

public static double DivisionMethod(double a, double b)
{
      double div = a / b;
      double temp = Math.Floor(div);
      double fractional = div - temp;

      if (fractional > 0.6)
      {
          return Math.Ceiling(div);
      }
      else
      {
          return Math.Floor(div);
      }
}

DivisionMethod is a pretty vague/poor name for your method; any method with the word "method" in it should raise a "bad name" flag!

Also, hard-coding the threshold into the function takes away flexibility. Perhaps I'd consider an optional parameter, like this:

public static double RoundedDivision(double a, double b, double threshold = 0.6)

Okay, not sure RoundedDivision is a much better name... naming is hard!. This is where XML comments can help:

/// <summary>
/// Divides <c>a</c> by <c>b</c> and round up if quotient is
/// greater than <c>threshold</c>, round down if it's less.
/// </summary>
public static double RoundedDivision(double a, double b, double threshold = 0.6)

It's probably best to call a cat, a cat: a is the dividend and b is the divisor, so the method would be clearer with a signature like this:

/// <summary>
/// Divides <c>dividend</c> by <c>divisor</c> and round up if quotient is
/// greater than <c>threshold</c>, round down if it's less.
/// </summary>
public static double RoundedDivision(double dividend, double divisor, double threshold = 0.6)

As was mentioned in the comments, there's an ambiguity issue with the threshold: what happens if the quotient is exactly equal to the threshold?


Organizing page functions and page state using the Module pattern So I'm developing a webpage using MVC which will be mainly JavaScript driven.

Instead of using global variables, and global functions, I would like to do it right this time. I don't want to use something to complex, because the team and I are new to large JavaScript applications. I tried implementing the Module pattern to organize my functions and page state.

This is what I came up with so far.

Am I misusing the Module pattern? What am I doing wrong? Where will I get into trouble?

<script type="text/javascript">
    var ui = (function ($) {
        var _ui = {};

        _ui.submitMainForm = function () {
            document.main.submit();
        };

        return _ui;
    } (jQuery));

    ui.tabs = (function ($) {
        var _tabs = {};

        var resourceTabs = $("#resourceTabs");
        var selectedTabPersistence = $("#SelectedTab");

        _tabs.getSelectedTab = function () {
            return selectedTabPersistence.val();
        };
        _tabs.setSelectedTab = function (value) {
            selectedTabPersistence.val(value);
        };
        _tabs.initialize = function () {
            resourceTabs.tabs({
                select: function (event, ui) { _tabs.setSelectedTab(ui.tab.hash); }
            });
            resourceTabs.tabs("select", _tabs.getSelectedTab());
        };

        return _tabs;
    } (jQuery));

    ui.filter = (function ($) {
        var _filter = {};

        var personLevels = $("#personLevels");
        var vehicleLevels = $("#vehicleLevels");
        var personLevelsRadio = $("#personLevels :radio");
        var vehicleLevelsRadio = $("#vehicleLevels :radio");

        _filter.initialize = function () {
            personLevels.buttonset();
            vehicleLevels.buttonset();
            personLevelsRadio.bind("change", function () { ui.submitMainForm() });
            vehicleLevelsRadio.bind("change", function () { ui.submitMainForm() });
        };

        return _filter;
    } (jQuery));

    $(document).ready(function () {
        ui.tabs.initialize();
        ui.filter.initialize();
    });
</script>

You have multiple anonymous functions when you could do one and on the last line return your ui.* object.

var ui = (function($) {
    // local ui ref
    var ui = {};

    // other namespaces .. etc
    ui.tabs = {};

    return ui;
})(jQuery));


Isomorphic strings in LeetCode I solved this problem:

  Given two strings s and t, determine if they are isomorphic.

  Two strings are isomorphic if the characters in s can be replaced to
  get t.

  All occurrences of a character must be replaced with another character
  while preserving the order of characters. No two characters may map to
  the same character but a character may map to itself.

For example:

  Given "egg", "add", return true.

  Given "foo", "bar", return false.

  Given "paper", "title", return true.

Note:

  You may assume both s and t have the same length.

The code in C# on average took around 109ms to execute, which is pretty slow compared to the C++ version of the same algorithm, which executes in under 6ms. Although, the fastest C# solution for this problem is around 100ms.

public class Solution {
    public bool IsIsomorphic(string s, string t) {
            int[] map = Enumerable.Repeat(-1, 175).ToArray();
            bool[] marked = Enumerable.Repeat(false, 175).ToArray();
            for(int i=0; i<s.Length; ++i)
            {
                if (map[s[i]] ==  -1) // Unvisited
                {
                    if (marked[t[i]]) // Already has a mapping
                    {
                        return false;
                    }

                    marked[t[i]] = true;

                    map[s[i]] = t[i];
                }
                else if(map[s[i]] != t[i])
                {
                    return false;
                }
            }

            return true;
    }
}

Optimizations I had done:

Converted List<T> to array after reading this
Changed foreach to a plain for loop that helped remove another index variable for iterating over string t

Can you suggest any better way to squeeze some performance from the C# implementation?

And from the FAQ section of LeetCode I could see that C# uses mono 4.2.1 and C++ uses g++ 5.4.0.

I'm attaching the C++ solution as well just for reference:

#define MAX_CHARS 175
class Solution {
public:
    bool isIsomorphic(string str1, string str2) {
    bool marked[MAX_CHARS] = {false};

    // To store mapping of every character from str1 to
    // that of str2. Initialize all entries of map as -1.
    int map[MAX_CHARS];
    memset(map, -1, sizeof(map));

    // Process all characters one by on
    for (int i = 0; i < str1.size(); i++)
    {
        // If current character of str1 is seen first
        // time in it.
        if (map[str1[i]] == -1)
        {
            // If current character of str2 is already
            // seen, one to one mapping not possible
            if (marked[str2[i]] == true)
                return false;

            // Mark current character of str2 as visited
            marked[str2[i]] = true;

            // Store mapping of current characters
            map[str1[i]] = str2[i];
        }

        // If this is not first appearance of current
        // character in str1, then check if previous
        // appearance mapped to same character of str2
        else if (map[str1[i]] != str2[i])
            return false;
    }

    return true;

    }
};

I don't know how much runtime you'll save, but you're iterating 175 more times  than you need to here.

int[] map = Enumerable.Repeat(-1, 175).ToArray();
bool[] marked = Enumerable.Repeat(false, 175).ToArray();

You can replace this with an old fashioned for loop and set them both at once.

const int maxChars = 175;

int[] map = new int[maxChars];
bool[] marked = new bool[maxChars];

for (var i = 0; i < maxChars; i++)
{
    map[i] = -1;
    marked[i] = false;
}

It's more verbose, but might save you a millisecond or 2.