Using enum to reduce page object method I had written following page object class which represents user profile page. The profile contains department, teams, email and login labels and corresponding values. Initially I wrote a page object class as  -

public class MyProfilePage extends PageObject{

public MyProfilePage() throws Exception {
    super(departmentLabel);
    throw new IllegalStateException("This is not My Profile page");
}

private static final LabelElement departmentLabel = new LabelElement("Department Label",
        locateByCSSSelector(".department"));
private static final LabelElement teamLabel = new LabelElement("Teams Label",
        locateByCSSSelector(".teams"));
private static final LabelElement emailLabel = new LabelElement("Emails Label",
        locateByCSSSelector(".email"));
private static final LabelElement loginLabel = new LabelElement("Login Label",
        locateByCSSSelector(".login"));

public String getDepartment() {
    return departmentLabel.getText();
}

public String getTeam() {
    return teamLabel.getText();
}

public String getEMail() {
    return emailLabel.getText();
}

public String getLogin() {
    return loginLabel.getText();
}
}

Then I decided to use enum and reduce the number of class variable and methods -

public class MyProfilePage extends PageObject{

public MyProfilePage(ProfileAttribute profileAttribute) throws Exception {
    super(getProfileAttributeElement(profileAttribute));
    throw new IllegalStateException("This is not My Profile page");
}

public enum ProfileAttribute {
    DEPARTMENT("department"), TEAM("teams"), EMAIL("email"), LOGIN("login");

    private String attribute;

    ProfileAttribute(String attribute) {
        this.attribute = attribute;
    }

    public String getAttribute() {
        return attribute;
    }
}

private static HtmlElement getProfileAttributeElement (ProfileAttribute profileAttribute) {
    return new LabelElement("Profile Attribute", locateByCSSSelector("."+
            profileAttribute.getAttribute()));
}

public String getAttributeValue(ProfileAttribute profileAttribute) {
    return getProfileAttributeElement(profileAttribute).getText();
}
}

A curtailed version of TopHeader class which gets to the Myrofile page -

public class TopHeader extends PageObject {

public MyProfilePage goToManageYourAccount() throws Exception {
    manageYourAccountLink.click();
    return new  MyProfilePage(MyProfilePage.ProfileAttribute.DEPARTMENT);
}

}

This is how a test uses it -

public class MyProfileTest extends SelTestCase {

LoginUserNavigation loginUserNavigation = new LoginUserNavigation();

@Test(groups = "shouldDisplayProfileAttributesOnMyProfilePage")
public void shouldDisplayProfileAttributesOnMyProfilePage() throws Exception {
    MyProfilePage myProfilePage = loginUserNavigation
            .loginAsDefaultUser(getEnvironment())
            .getTopHeader()
            .goToManageYourAccount();
    assertThat("department is wrong", myProfilePage.getAttributeValue
            (MyProfilePage.ProfileAttribute.DEPARTMENT), is(equalTo("Technology")));
}

}

Have I made it any better? Could I improve it further?

Throwing exceptions in the constructor is weird; you should either add a comment explaining OR just remove the exception. If you're going to always disallow creation of a PageObject, provide a static method which will do the things you want, rather than throwing exceptions all the time.

Exceptions are for exceptional flow - not for "business as usual". Right now your program reads like "... and then it crashes, which is completely normal." This sort of thing, without documentation, leads to maintenance nightmare. Why was this done? It's highly irregular! But if it was done this way, there must have been a reason? I mean, normally, there is no sane reason to do the sort of thing you just did...

public String getAttributeValue(ProfileAttribute profileAttribute) {
    return getProfileAttributeElement(profileAttribute).getText();
}

This can be deleted. The only constructor throws an exception, so barring weird reflection (in which case, you're doing it wrong), any methods which rely on instances cannot be called. The other option I can think of is if you have a subclass that uses this method. Then this method would see some use - but it's existence remains questionable.

Without the exception, however, you can make use of it as normal.


Sqlite select and delete operation using C API in Swift I've following sqlite code using C api within Swift having a read and a delete operation for review.
For those not familiar with Swift, defer will execute after the end of the function.
Since I'm not closing the database connection and keeping it alive, I'm calling sqlite3_db_cacheflush(database) to flush out the changes to the disk. Link
class Database {

    internal var database: OpaquePointer! = nil

    init(_ database: OpaquePointer) {
        self.database = database
    }

    func deleteById(_ id: Int64) throws -> Int32? {
        let sql = "DELETE FROM user WHERE id = \(id)"

        defer {
            sqlite3_db_cacheflush(database)
        }

        guard sqlite3_exec(database, sql, nil, nil, nil) == SQLITE_OK else {
            throw NSError(domain: String(cString: sqlite3_errmsg(database)), code: 1, userInfo: nil)
        }

        return sqlite3_changes(database)
    }

    func findAllDepartments() throws -> [Department]? {
        var statement: OpaquePointer? = nil
        let sql = "SELECT id, name FROM department"

        guard sqlite3_prepare_v2(database, sql, -1, &statement, nil) == SQLITE_OK else {
            throw NSError(domain: String(cString: sqlite3_errmsg(database)), code: 1, userInfo: nil)
        }

        defer {
            sqlite3_finalize(statement)
            sqlite3_db_cacheflush(database)
        }

        var departments: [Department] = []
        while sqlite3_step(statement) == SQLITE_ROW {
            departments.append(Department(id: sqlite3_column_int64(statement, 0), name: String(cString: sqlite3_column_text(statement, 1))))
        }

        return departments
    }
}

Usage of Database Class
do {

   if sqlite3_open_v2(url.path, &database, SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE | SQLITE_OPEN_FULLMUTEX, nil) == SQLITE_OK { // Existing database (Serialized mode)
         let db = Database(database)
         try! db.findAllDepartments()
         try! db.deleteById(4)
    } else { // Error opening database
        print(String(cString: sqlite3_errmsg(database)))
    }
catch { let error as NSError {
    print(error.description)
}

You did not specify what precisely you were looking for, so a few general observations:

You do not need sqlite3_db_cacheflush. When you perform SQL updates, they are written to disk immediately (unless you are using transactions). You can do your inserts, updates, and deletes, leave the database open, and you’ll be fine.

Use Error, not NSError. I would advise against NSError (unless you needed Objective-C backward compatibility). In Swift, we would define our own Error type:
enum DatabaseError: Error {
    case sqlError(Int32, String)
}

And then
guard sqlite3_prepare_v2(database, sql, -1, &statement, nil) == SQLITE_OK else {
    throw DatabaseError.sqlError(sqlite3_errcode(database), String(cString: sqlite3_errmsg(database)))
}

Then, when catching errors, the codes and the error messages are more easily retrieved.

Failure of sqlite3_open_v2 will leak. When handling sqlite3_open_v2 failure, make sure to close the database or else resources allocated to it will never get freed. I know that this seems unintuitive, but the documentation is clear (emphasis added):

Whether or not an error occurs when it is opened, resources associated with the database connection handle should be released by passing it to sqlite3_close() when it is no longer required.

Method names. You have a method called deleteById(_:), which does not make it clear what you are deleting. Besides the use of a preposition and parameter name in the method name is common in Objective-C, but not Swift. I would suggest deleteUser(id:) or something like that. That makes the domain and functional intent of the method clear, and moves the parameter label where it belongs.

Be careful with string interpolation of SQL. In your user deletion routine, you are building the SQL statement through string interpolation of the identifier. That happens to work because the parameters are numeric, but can be problematic with string parameters. Usually we would use a prepared statement and then bind values to ? placeholders in the SQL.

Methods that throw should not return optionals. Optionals are useful when writing functions that must return either nil or some
value. But in this case, you always either return a value or throw
errors, so use of optionals is not needed any only complicates the
calling point. Remove the ? in the return values of E.g.,
enum DatabaseError: Error {
    case sqlError(Int32, String)
}

func deleteUser(by identifier: Int64) throws -> Int32 {
    let sql = "DELETE FROM user WHERE id = \(identifier)"

    guard sqlite3_exec(database, sql, nil, nil, nil) == SQLITE_OK else {
        throw DatabaseError.sqlError(sqlite3_errcode(database), String(cString: sqlite3_errmsg(database)))
    }

    return sqlite3_changes(database)
}

func findAllDepartments() throws -> [Department] {
    var statement: OpaquePointer? = nil
    let sql = "SELECT id, name FROM department"

    guard sqlite3_prepare_v2(database, sql, -1, &statement, nil) == SQLITE_OK else {
        throw DatabaseError.sqlError(sqlite3_errcode(database), String(cString: sqlite3_errmsg(database)))
    }

    defer {
        sqlite3_finalize(statement)
    }

    var departments: [Department] = []
    while sqlite3_step(statement) == SQLITE_ROW {
        departments.append(Department(id: sqlite3_column_int64(statement, 0), name: String(cString: sqlite3_column_text(statement, 1))))
    }

    return departments
}

Strive for highly cohesive, loosely coupled types. In this case, you have some other code opening the SQLite database and passing the sqlite3 pointer to the Database class. This is bad because it tightly couples the caller with the Database class, littering SQLite calls in multiple classes. It also is problematic because the Database type is less cohesive, not encompassing all the SQLite calls.
If, for example, you replace SQLite with some other storage mechanism at some later date, you are going to have to not only replace your storage class, Database, but go through all the caller code, too.

Database is tightly coupled with model types. In a similar vein as my previous point, we generally want to keep model objects and database objects a little less tightly coupled than this. For example, rather than having Department specific methods in our base Database class, you would want to define a protocol for fetching and storing of data. Or, at the very least, I would move Department and other model-specific methods in their own extensions, to avoid cluttering the main Database class with model-specific methods.

Consider SQLite wrapper class. Before you write your own SQLite wrapper class, you might want to consider one of the existing ones out there. They have already wrestled with many of the basic considerations and might have interfaces to handle this.

Just a few observations.


Restore SQLite database backup I have an application that provides the user the ability to backup the local SQLite database to another location.  I then provide the ability to restore the backup database and overwrite the current database with the backup copy.  In my restore command, I make a backup of the database file, which seems redundant.  I feel like this code could be simplified.

public async void RestoreDatabase()
{
    bool success;
    var pickedFile = default(StorageFile);

    var picker = new FileOpenPicker();
    picker.FileTypeFilter.Add(".sqlite");
    picker.SuggestedStartLocation = PickerLocationId.DocumentsLibrary;
    var file = await picker.PickSingleFileAsync();

    if (file != null)
    {
        var pickedFileToken = StorageApplicationPermissions.FutureAccessList.Add(file);
        _settings.RestoreFileToken = pickedFileToken;

        pickedFile = file;
    }

    // first make a backup of the current database file
    var currentDatabase = await _currentFolder.GetFileAsync("MyMoney.sqlite");
    if (currentDatabase != null)
    {
        currentDatabase.CopyAsync(ApplicationData.Current.RoamingFolder, "MyMoney.bak",
            NameCollisionOption.ReplaceExisting);
    }

    try
    {
        if (pickedFile != null)
        {
            await pickedFile.CopyAsync(ApplicationData.Current.RoamingFolder, "MyMoney.sqlite",
                    NameCollisionOption.ReplaceExisting);
            success = true;
        }
        else
        {
            success = false;
        }
    }
    catch (Exception)
    {
        success = false;
    }

    if (!success)
    {
        // restore the backup file
        var backupDatabase = await _currentFolder.GetFileAsync("MyMoney.bak");

        if (backupDatabase != null)
        {
            await backupDatabase.CopyAsync(ApplicationData.Current.RoamingFolder, "MyMoney.sqlite",
                NameCollisionOption.ReplaceExisting);
        }
    }
}

I would suggest that you create some intermediate methods instead of doing everything in one method. For example the task of restoring a database is comprised of these subtasks:

Select database file
Create copy
Restore database

var pickedFile = default(StorageFile);

It's a rather convoluted way to say IStorageFile pickedFile = null, don't you think? Personally I don't see many people use default() for this purpose so I would stick with the more common way of initializing.

await _currentFolder.GetFileAsync("MyMoney.sqlite");

Why are you hardcoding the filenames? This limits your re-use in case you want to expand it to other databases as well.

currentDatabase.CopyAsync(ApplicationData.Current.RoamingFolder, "MyMoney.bak", NameCollisionOption.ReplaceExisting);

This call is not awaited!

public async void RestoreDatabase()

Always avoid async void. From a recent answer of mine:

Asynchronous methods should return Task or Task<T>, the former being for traditional void methods and the latter being for methods with T return type.
The reason for this is that a void method cannot be awaited (since it doesn't return a Task) and will in effect be a fire-and-forget kind of thing: it will start executing your asynchronous code on a separate thread and continue doing business as normal. However once the main thread reaches its end, the application will exit regardless of whether or not your asynchronous code has finished as well. This obviously leads to nasty bugs.
In case you ever want to re-use this code for a console application or just in a class library where you don't know the client, I would suggest making it async Task just for good measure.
The only exception to this are asynchronous event handlers since they have to adhere to the convention that event handlers return void. This also explains why events are "dangerous" to use in asynchronous code.

RestoreDatabase()

Convention dictates that asynchronous methods should have the Async suffix.


Multiplying binary numbers digit by digit and carry method in Python I have two numbers encoded in a reversed list of digits. First I made multiplication in "Peano" way:

#from summa import summa
#from predecessor import predecessor

#def multiply(m, n):
#  def _(b):
#    return [0] if is_zero(b) else summa(m, _(predecessor(b)))
#  return [0] if is_zero(m) or is_zero(n) else summa(m, _(predecessor(n)))

but my intention is to find out, if carry method presented below is more efficient.

While the algo itself should work, I need some assistance, if the middle part of it could be optimized. At the moment it looks overly complex, but I haven't find a way to simplify it. Of course it could be an unavoidable effect of the way I have chosen to carry and pass parameters, but if someone could either confirm the case or do some optimization suggestions, I'd greatly appreciate it.

# encode int to binary list
def bn(n):
  return list(reversed(list(map(lambda x: 1 if x == "1" else 0, "{0:b}".format(n)))))

def is_zero(n):
  return not n[1:] and not n[0]

def is_one(n):
  return not n[1:] and n[0]

# multiplication
def multiply2(a, b):
    def _(c, d, f, g, e):
        if not b and not c:
            return g + (e if e[1] else e[:1])
        if c:
            # clumsy part is the formation of A and x
            A = 0 if (0 if (1 if c[0] and d else 0) == (e[1] if e else 0) else 1) == (f[0] if f else 0) else 1
            # x = [0, 0] or [1, 0] or [0, 1] or [1, 1]
            x = [A, 1 if ((e[1] if e else 0) or (f[0] if f else 0)) and c[0] and d or \
                         ((e[1] if e else 0) and (f[0] if f else 0)) and not c[0] and d else 0]
            #print(x, c, d, e, f, g)
            return _(c[1:], d, f[1:], (g + e[:1] if e else g), x)
        return g[:1] + _(a, b.pop(0), g[1:] + e, [], [])
    return [0] if is_zero(a) or is_zero(b) else \
             b if is_one(a) else \
             a if is_one(b) else _(a, b.pop(0), [], [], [])
# test cases
for i in range(101):
  for j in range(101):
    x, y = bn(i*j), multiply2(bn(i), bn(j))
    if x != y:
      print("prod %s * %s = %s:" % (i, j, (i*j)), x, "->", y)

Please note the comment: "clumsy part is the formation of A and x" which is due to required optimization.

The last double for loop iterates numbers from 0 to 100 and multiplies them, which is for checking that algo works correctly.

  Note, that I can do only simple comparison, boolean checks and list
  cut/concat operations on the algo.

Addition

In the simple sample output:

print(multiply2(bn(10), bn(10)), bn(10*10))

both lists should be same.

Also

This algo is based on the schema presented below, althought the one below is using base ten numbers, but carrying logic is same in the right part of the spreadsheet:

Left part is the common multiplication shema learnt in the elementary schools and one really could to repeated additions as on my commented "Peano" example. But my intuition says multiply2 method is faster until we really want to go to the Fast Fourier transform algorithms.

Python style

Style is important! Will you be able to read this code after a few months? I have a really hard time understanding this at first sight.

Use a if __name__ == '__main__'
Use built-ins like this bn(n):   return list(map(int, bin(n)[:1:-1]))] as suggested by @Coal_ with some changes to reverse the bin representation and change it a an integer
Use indentation of 4 spaces, this will make your code alot more readable. Currently you have mixed 4 indent with 2 indent, my eyes don't like this :)
Use more descriptive names, _(c, d, e, f, g) what do they mean? It is not clear for a Code reviewer.
Don't try to shorten lines as much as possible. For instance,

return [0] if is_zero(a) or is_zero(b) else \
        b if is_one(a) else \
        a if is_one(b) else _(a, b.pop(0), [], [], [])

This is short I agree, but not really nice to read or discriptive what it does at first sight.

if is_zero(a) or is_zero(b):
    return [0]
elif is_one(a):
    return b
elif is_one(b):
    return a
else:
    ....

Reads alot better in my opinion.

I would suggest to read up on PEP which has alot of nice pointers for Python style.

Algo

This should be possible without recursion, when I choose a big value you will get an error:

print(multiply2(bn(100000000000), bn(100000000000)), bn(100000000000*100000000000))
RecursionError: maximum recursion depth exceeded in comparison

You could avoid this by increasing the recursion depth but this is more of a hack then a solution.

How about a stack based approach to avoid the recursion?

Alternative approach

Using a stack based approach

def bn(n):
    return list(map(int, bin(n)[:1:-1]))

def is_zero(n):
    return not n[1:] and not n[0]

def add_lists(first, second):
    n = max(len(first), len(second))
    first += [0] * (n - len(first))
    second += [0] * (n - len(second))
    return [first[i] + second[i] for i in range(n)]

def carry_over(first_num, second_num):
    stack, overflow = [], 0
    for i in range(len(second_num)):
        if second_num[i] == 1:
            stack = add_lists(stack, [0] * i + first_num)

    for i in range(len(stack)):
        stack[i] += overflow
        overflow = stack[i] // 2
        stack[i] %= 2

    while overflow > 0:
        stack.append(overflow % 2)
        overflow //= 2

    return stack

def multiply2(first_num, second_num):
    if is_zero(first_num) or is_zero(second_num):
        return [0]
    else:
        return carry_over(first_num, second_num)

if __name__ == "__main__":
    # test cases
    for i in range(101):
        for j in range(101):
            print(f"{multiply2(bn(i), bn(j))}, {bn(i*j)}")


Gem to interact with a third party API I am writing a gem for users to interact with a third party api. I want users to have easy and natural access to objects and properties delivered by the JSON payload.

The collection of entities delivered are called Entries so after making an http request I pass the json response to an Entries object like so:

  class Entries
    include Enumerable

    attr_reader :body, :entries

    def initialize(body)
      @body = body
      @entries = body.map { |entry| Entry.new(entry) }
    end

    def each(&block)
      entries.each(&block)
    end
  end

And for my single Entry object I have written:

  class Entry

    attr_reader :properties, :attributes

    def initialize(attributes)
      @attributes = attributes
      @properties = attributes.keys

      properties.each do |prop|
        define_singleton_method prop do
          @attributes[prop]
        end
      end
    end

    def to_s
      "#{attributes[:title]} created on #{attributes[:created_at]}"
    end

    def is_entry?
      true
    end
  end

I haven't tested this much but things seem to be working so far. Users of my gem can now do this:

entries = client.entries(content_type: 'shirts')
entries.each { |entry| puts entry.title }

But I am worried this is an inefficient way of doing things and that perhaps I am using too much memory in these loops.

Also I want to keep magical meta-programming to a minimal but still achieve this. Is this a reasonable way to do this in Ruby? Would love some feedback and advice.

The first thing I'd do is to create your Entry objects on demand, rather than in Entries#initialize:

class Entries
  include Enumerable
  ...
  def initialize(body)
    @body = body
  end
  ...
  def each(&block)
    @body.each do |entry|
      block.call(Entry.new(entry))
    end
  end
end

As for dynamically creating methods for each attribute, I'd be wary of doing that. Without knowing your JSON API schema, there could be two possibilities going on:

Each Entry always has the same attributes, so you are spending processing time and memory dynamically generating the same methods every time.
Each Entry may have wildly differing attributes – in which case your clients won't know at runtime which methods each entry object actually responds to, and will have to use some checks of their own, e.g., #respond_to? in order for their code to work reliably.

In either case, you really want your clients to know that each Entry object they receive will have the same attributes at all times. I'd be inclined to explicitly define each attribute method, e.g.:

class Entry
  attr_reader :attributes
  ...
  def title
    attributes[:title]
  end
end

This gives you the option of how to deal with missing attributes in the API. For example, if an array field, such as authors, is missing, you could choose to return an empty array instead:

def authors
  attributes.fetch(:authors, [])
end

I know the non-metaprogramming way can seem like a more boring way of doing things, but even though your entry.rb source code will be much longer, everything will be much clearer and easier to maintain and debug.

And at the end of the day, your gem's users will end up with an interface which is much more predictable – and the methods you include will be more easily documentable through YARD or any other automated documentation tool.


Write a clean convertToNumber function, if value is a number I have a function that creates filters for some data. The filters get added to an array and then (later) chained through the reduce function (the chaining part not part of my question).
The data I receive are all strings but can actually be numbers, eg {name:"-45.6"}. If it can be a number, I want the value to be converted to a number.
formFilters = (builder) => {
    const filters = [];
    builder.forEach(b => {
        let {filtername, operator, value} = b;

        if(!value) return;

        if(isNumber(value)){
            value = Number(value);
        }

        switch (operator) {
            case "=":
                filters.push((item) => convertToNumber(item[filtername]) === value);
                break;
            case "<":
                filters.push((item) => convertToNumber(item[filtername]) < value);
                break;
            case ">":
                filters.push((item) => convertToNumber(item[filtername]) > value);

        }
    });
    return filters;
}

function convertToNumber(value){
    const number = Number(value);
    if (!Number.isNaN(number)) return number;
    return value;
}

This works but I'm wondering if there's a cleaner way of converting the value to a number if it is a number?

One problem with Number(value); is that it casts falsy values as 0 and truthy values as 1.
That means that an empty string becomes 0, which may not be what you expect.
You could make a regular expression test on the input to determine if it fits your expectations for a number.

let input = document.getElementById('input');
let result = document.getElementById('result');

function IsANumber() {
  let isItANumber = input.value.match(/^-?\d*\.?\d+$/);
  result.innerHTML = isItANumber ? 'true' : 'false';
}
<input id="input" onchange="IsANumber()" onkeyup="IsANumber()" />
 Is a number:

The example above will not fail in truthy/falsy cases, but it will determine strings like "Infinity" to not be numbers too.
Therefore it may need to be modified if you have to parse these to numeric values.


Refactoring method to make it unit testing friendly I have a completed application which I'm trying to write unit tests for (Yeah I know, talk about bad practices)

I have the following class here

public class UserManagementService : IUserManagementService
{
    private static readonly IUserDao userDao = DataAccess.UserDao;
    private static readonly ILog Log = LogManager.GetLogger(MethodBase.GetCurrentMethod().DeclaringType);

    public LoginResponse Login(LoginRequest request)
    {
        var response = new LoginResponse(request.RequestId);
        try
        {
            var user = userDao.GetByUserId(request.UserId);
            if (user != null)
            {
                if (request.Password != "")
                {
                    if (Authenticate(user, request.Password))
                    {
                        return response;
                    }
                    response.ErrorCode = "IncorrectPassword";
                }
                else
                {
                    response.ErrorCode = "PasswordNotFound";
                }
            }
            else
            {
                response.ErrorCode = "UserNotFound";
            }
            response.Acknowledge = AcknowledgeType.Failure;
            return response;
        }
        catch (Exception ex)
        {
            Log.Error(ex);
            response.Acknowledge = AcknowledgeType.Failure;
            response.ErrorCode = "Exception";
            return response;
        }
    }

    public bool Authenticate(User user, string password)
    {
        if (user == null) return false;

        using (var deriveBytes = new Rfc2898DeriveBytes(password, user.Salt))
        {
            var derivedPassword = deriveBytes.GetBytes(20);
            if (!derivedPassword.SequenceEqual(user.Password)) return false;
        }
        return true;
    }
}

userDao follows the singleton pattern and this userDao.GetByUserId(request.UserId); basically makes a call to my DB

I have written the following test, which will definitely give rise to some issues because I haven't figured how to Mock the userDao, and the Log

[Theory]
[InlineData("manager", "manager")]
public void LoginTest(string userId, string password)
{
    // Arrange
    // System under test
    IUserManagementService userService = new UserManagementService();

    var request = new LoginRequest().Prepare();
    request.UserId = userId;
    request.Password = password;

    var expectedResponse = new LoginResponse(request.RequestId)
            {
                Acknowledge = AcknowledgeType.Success
            };

    //Act
    var actualResponse = userService.Login(request);

    //Assert
    Assert.AreEqual(actualResponse.Acknowledge, expectedResponse.Acknowledge);
}

How can I refactor my Login method, and UserManagementService class without breaking too much of its structure to support unit testing so I can inject userDao and Log?

Any help would be great to help me kickstart what is going to be a long tedious process of refactoring the rest of my classes.

EDIT: This is a WCF service which follows the facade pattern. I have removed the 10 other static DAO similar to userDao to lessen the code clutter for this example.

Following on what Magnus said in comments can you not inject the necessary dependencies.

For example:

public class UserManagementService : IUserManagementService
{
    private readonly IUserDao userDao ;
    private readonly ILog log;

    public UserManagementService(IUserDao userDao, ILog logger)
    {
       this.userDao = userDao;
       this.log = logger;
    }

    public LoginResponse Login(LoginRequest request)
    {
       // etc
    }
}

Now the class has no dependencies on concrete implementations and you can mock the interfaces however you feel.  For example, the ILog interface you might want to mock so it logs to the console only.  The IUserDao interface might be mocked to a internal list implementation or you might use a mocking framework such as Moq.


Polling multiple servers I have an application which polls a bunch of servers every few minutes. To do this, it spawns one thread per server to poll (15 servers) and writes back the data to an object:

import requests
import threading
import time

servers = ['1.1.1.1', '1.1.1.2']

class CallThreads(threading.Thread):
    """
    Auxiliary class used to provide arguments to threads
    """

    def __init__(self, target, *args):
        self.target = target
        self.args = args
        threading.Thread.__init__(self)

    def run (self):
        self.target(*self.args)

class ServerResults(object):
    def __init__(self):
        self.results_list = []

    def add_server(some_argument):
        self.results_list.append(some_argument)

def poll_server(server, results):
    response = requests.get(server, timeout=10)
    results.add_server(response.status_code);

def process_results(results):
    # Do something with the results

def main():
    while True:
        results = ServerResults()
        for s in servers:
            t = CallThreads(poll_server, s, results)
            t.daemon = True
            t.start()
        time.sleep(300)
        process_results(results.results_list)

if __name__ == '__main__':
    main()

This is my first non-trivial Python application, so I would appreciate any critique, comments, or suggestions. Thank you!

The two classes seem useless to me.

ServerResults only contains a list, so just use a list. Edit: see comment below.

The CallThreads class is unnecessary, this:

t = CallThreads(poll_server, s, results)

can be written like that:

t = Thread(target=poll_server, args=(s, results))

Note that you could also use the partial function:

t = Thread(target=partial(poll_server, s, results))

or a lambda:

t = Thread(target=lambda: poll_server(s, results))


Simple RSS to HTML converter Is this code pythonic, easy to read, stable, or missing something?

# -*- coding: utf-8 -*-
""" Simple rss to html converter """

__version__ = "0.0.1"
__author__ = "Ricky L Wilson"

import StringIO
from feedparser import parse as parsefeed
from bs4 import BeautifulSoup as bs
from unicodedata import normalize

def flatten_unicode_keys(dic):
    '''pass unicode keywords to **kwargs '''
    for key in dic.keys():
        if isinstance(key, unicode):
            value = dic[key]
            del dic[key]
            dic[normalize('NFKD',key).encode('ascii','ignore')] = value

def entry2html(**kwargs):
    """ Format feedparser entry """
    flatten_unicode_keys(kwargs)
    title = kwargs['title']
    link = kwargs['link']
    description = kwargs['description']
    template = u"""

{title}

    {title}
    {description}
    """
    return template.format(title=title, link=link, description=description).encode('utf-8')

def convert_feed(**kwargs):
    """ Main loop """
    flatten_unicode_keys(kwargs)
    out = StringIO.StringIO("")
    for entry in parsefeed(kwargs['url']).entries:
        title = entry['title']
        link = entry['link']
        description = entry['description']
        print >> out, entry2html(title=title, link=link, description=description)
    return bs(out.getvalue(), 'lxml').prettify()

def save_file(url, fname):
    ''' Save data to disc'''
    with open(fname, 'w') as file_object:
        file_object.write(convert_feed(url=url).encode('utf-8'))

save_file('http://stackoverflow.com/feeds', 'index.html')

The small improvement I would like to suggest you is related to performance in terms of speed.

In flatten_unicode_keys(dic), you use for key in dic.keys(): which is less fast than for key in dic:. This is mentioned in PEP 234:

  Dictionaries implement a tp_iter slot that returns an efficient
  iterator that iterates over the keys of the dictionary. [...] This
  means that we can write

for k in dict: ...

  which is equivalent to, but much faster than

for k in dict.keys(): ...

An other side note: it is a better coding habit not to encode the container type into the name. So instead, for example, of typing dic, use a name which actually better describes what that dictionary consists of.


Parsing time if valid I need to parse only valid timestamp formats:

Hours followed by minutes followed by seconds: t=1h2m3s
Minutes followed by seconds: t=2m3s
Only hours: t=3h
Only minutes: t=4m
Only seconds: t=5s and escape other timestamp formats like t=2m1h or t=3s2m or t=3s1h2m.

To get the hours, minutes and seconds, I use this Regex. Let me know any improvement can be done to make it simple.

function calculateInSeconds(timeStamp) {
  var timeInSeconds=0;
  if(timeStamp.match(/t=[0-9]*h?[0-9]*m?[0-9]*s?/g).toString()==timeStamp){
    timeStamp.replace(/([0-9]+)[h|m|s]/g, function(match, value) {
       if (match.indexOf("h") > -1) {
          timeInSeconds += value * 60 * 60;
       } else if (match.indexOf("m") > -1) {
                timeInSeconds += value * 60;
       } else if (match.indexOf("s") > -1) {
                timeInSeconds += value * 1;
       }
     });
   }
   console.log("timeInSeconds"+timeInSeconds);
}

calculateInSeconds("t=3m59s1h");//invalid
calculateInSeconds("t=1m59s");//valid
calculateInSeconds("t=1h");//valid

First you may improve your regexps a bit using classes: replace [0-9] by \d.

In the other hand, you may strongly improve performance by:

defining an object with the unit values: units: {h: 3600, m: 60, s: 1};
modifying the 2nd regexp to directly provide the current unit:
timeStamp.replace(/(\d+)(\w)/g, function(match, value, unit) {
(note that \w is enough since your previous test eliminated any wrong value)
then using a unique computation in the function: timeInSeconds += value * units[unit];

So the entire snippet becomes:

function calculateInSeconds(timeStamp) {
  var timeInSeconds = 0,
    units = {
    h: 3600,
    m: 60,
    s: 1,
  };
  if (timeStamp.match(/t=\d*h?\d*m?\d*s?/g).toString() == timeStamp){
    timeStamp.replace(/(\d+)(\w)/g, function(match, value, unit) {
      timeInSeconds += value * units[unit];
    });
  }
  console.log(timeStamp + ' -> ' + timeInSeconds + ' seconds');
}

calculateInSeconds("t=3m59s1h"); //invalid
calculateInSeconds("t=1m59s"); //valid
calculateInSeconds("t=1h"); //valid

BTW I like the idea of your first test to isolate valid timeStamps.


Output CSS class based on day-of-the-week match The goal is to loop through an array of days-of-the-week (array values are stored as strings, eg: 'Mon, Tues, Weds', etc) and output some HTML when today's day is encountered.

The file is a CMS's .tpl file, so the PHP sits directly alongside the HTML.

My current, working code is:

<?php
 $dayofweek = date('w');

 foreach ($dataarray['openingDays'] as $day_key => $opening_day):
   $i++;
   //if $i is equal to 7 make it 0 to match array
   $i = ($i == 7 ? 0 : $i);
   //If today's date matches $i
   if ($dayofweek == $i):
?>

<?php else: ?>

<?php
  endif;
  print $opening_day;
?>

<?php endforeach; ?>

This is one of the first times I've used ternary and would like to know if it is done correctly. I am also concerned about not initializing $i and would like to know if that is acceptable.

I would suggest you familiarize yourself with the DateTime class and it's related classes including DateInterval and DatePeriod as these are your most powerful tools for working with date/times, intervals of time, and iterating over intervals of time.

You should have no reason to build an array for days for the week or anything like that.

Let me give an example for how you might approach this problem using these classes.

// current DateTime
$now = new DateTime();
// create a DateTime representing the beginning of the week
$monday = clone $now;
$monday->setTime(0,0,0)
       ->modify('Monday this week');
// create a DateInterval to represent a one day interval
$dayInterval = new DateInterval('P1D');
// create a DatePeriod to represent a full week
$weekPeriod = new DatePeriod($monday, $dayInterval, 7);
// iterate the DatePeriod
foreach($weekPeriod as $day) {
    if($day->format('D') === $now->format('D')) {
        // today matches this day of week
    }
}

Beyond that, I would suggest being cognizant of your styling.  Right now:
- your indentations are inconsistent
- your variable naming style in inconsistent between using snake_case, camelCase (like in $dataarray keys), and no way to differentiate words in your variable name at all (i.e.$dayofweek, $dataarray).  You should pick either snake_case or camelCase to make your code more readable and stick with whichever style you choose.  I would actually recommend camelCase as PHP, as a language, is moving more in that direction with regard to industry styling standards.


ngCordova plugin for audio player I wrote a Cordova plugin for an app that I'm working on, and now I'm trying to write an Angular definition to wrap my plugin.  So far, I've followed the model in ng-cordova.js:

angular.module('ngCordova.plugins.audioPlugin', [])
.factory('$audioPlugin', ['$q', function($q) {

    return {
        init: function(sessionKey) {
            var q = $q.defer();

            AudioPlugin.init(sessionKey, function(response) {
                q.resolve(response);
            }, function (error) {
                q.reject(error);
            });
            return q.promise;
        },

        playAudio: function(contentId) {
            var q = $q.defer();

            AudioPlugin.playAudio(contentId, function(response) {
                q.resolve(response);
            }, function (error) {
                q.reject(error);
            });
            return q.promise;
        },

        seekChapter: function(contentId, partIndex, chapterIndex) {
            var q = $q.defer();

            AudioPlugin.seekChapter(contentId, partIndex, chapterIndex function(response) {
                q.resolve(response);
            }, function (error) {
                q.reject(error);
            });
            return q.promise;
        },

        getCurrentPosition: function() {
            var q = $q.defer();

            AudioPlugin.getCurrentPosition(function(response) {
                q.resolve(response);
            }, function (error) {
                q.reject(error);
            });
            return q.promise;
        },

        getCurrentChapter: function() {
            var q = $q.defer();

            AudioPlugin.getCurrentChapter(function(response) {
                q.resolve(response);
            }, function (error) {
                q.reject(error);
            });
            return q.promise;
        }

    }

}]);

I'm looking for feedback on how to structure this wrapper.  I'm a little unhappy with the amount of repetition involved to convert the callback-based architecture in the Cordova plugin to the angular promise-based style.

Bug:

You have a bug in the following line:

AudioPlugin.seekChapter(contentId, partIndex, chapterIndex function(response) {

You're missing a comma after chapterIndex.

Structure:

So, as init and playAudio follow the same style, as well as getCurrentChapter and getCurrentPosition following the same style, so you can substitute those out. Additionally, the resolve and reject functions can be taken out, leaving:

angular.module('ngCordova.plugins.audioPlugin', [])
.factory('$audioPlugin', ['$q', function($q) {
    function resolve(response){ return $q.defer.resolve(response).promise; }
    function error(error){ return $q.defer().reject(error).promise; }
    function generic(audioPluginMethod, parameter){
        if (parameter){
            return AudioPlugin[audioPluginMethod](parameter, resolve, error);
        }
        return AudioPlugin[audioPluginMethod](resolve, error);
    }
    return {
        init: generic('init', sessionKey),
        playAudio: generic('playAudio', contentId),
        getCurrentPosition: generic('getCurrentPosition'),
        getCurrentChapter: generic('getCurrentChapter'),
        seekChapter: function(contentId, partIndex, chapterIndex) {
            AudioPlugin.seekChapter(contentId, partIndex, chapterIndex, resolve, error);
        }
    }
}]);


Calculating the Percentage of a Pair Occurring Across a Lot of Lists I'm looking for a way to speed this code up. It took about 4.5 hours to run on ~20k decks. I'm open to restructuring my SQL query, but feel adjusting the python would be more effective. The goal of this code is to see how many times a pair of cards occurs in all the decks. For example if ("card", "name") appears in 27/100 decks, it returns .27.
This code takes every deck in a format, and returns the cards in those decks. This gives me a list of a lot of cards ((cardId, "card name", deckId), ... (cardId "card name", deckId)). I use the deckId to make a bunch of lists representing each deck (newList). Using those lists I get each unique card and put it into a separate list (superSet).
From there I loop through superSet twice to get each possible pair. Then loop through newList to see if a pair is in a deck.
I know it's somewhat complicated and I probably didn't explain it the best. I'll gladly update this post to clarify anything I can.
from classes.general import Database

def main():
    dbm = Database()
    with dbm.con:
        dbm.cur.execute("""SELECT c.id, c.name, ctd.deckId FROM cards c
        JOIN cardToDeck ctd ON ctd.cardId = c.id
        JOIN deckToEvent dte ON dte.deckId = ctd.deckId
        JOIN eventToFormat etf ON etf.eventId = dte.eventId
        WHERE etf.formatId = 5
        ORDER BY ctd.deckId""")
        decks = dbm.cur.fetchall()

        #print(decks)

        #I'm not entirely sure how this works
        #Somehow it takes everything with the same deckId (index:2) and puts them into one list
        values = set(map(lambda x:x[2], decks))
        newlist = [[y[1] for y in decks if y[2] == x] for x in values]

        #print(newlist[0])

        superSet = []
        for l in newlist:
            for c in l:
                if c not in superSet:
                    superSet.append(c)

        #print(superSet)

        count = 0
        for x in superSet:
            for y in superSet:
                if x is y:
                    continue
                #print(x)
                #print(y)
                for d in newlist:
                    #print(d)
                    if x in d and y in d:
                        count += 1
                deckPerc = count / len(newlist)
                if deckPerc != 0.0:
                    print(count / len(newlist)) #needs to go into the db
                count = 0

if __name__== "__main__":
    main()

Creating your list of decks
"""-- ORIGINAL CODE --"""
#I'm not entirely sure how this works
#Somehow it takes everything with the same deckId (index:2) and puts them into one list
values = set(map(lambda x:x[2], decks))
newlist = [[y[1] for y in decks if y[2] == x] for x in values]

map() takes the function in the first argument, and applies it to every item in the iterable in the second argument.  In this case, each item is a (cardId, "card name", deckId) tuple, and  the function returns the value in index 2 of that tuple, namely deckId.  set() takes the iterable returned by map() and converts it into a set, which contains exactly one of each unique value in the iterable - in this case, each unique deckId.
Your second line uses two list comprehensions.  The outer one iterates over values (your set of deckIds) and for each deckId it uses another list comprehension to iterate over decks and returns index 1 of each tuple (namely "card name") if index 2 of the item equals the deckId currently being used from the outer list comprehension.
You could also write this line as a double for-loop:
newlist = []
for x in values:
    temp = []
    for y in decks:
        if y[2] == x:
            temp.append(y[1])
    newlist.append(temp)

But in general, this is pretty messy.  You loop over the entirely of decks in order to create values, then loop over values * decks.  That's very inefficent
You'd be much better off using a dictionary, where you can then fold this all into a single loop
deck_dict = {}
for (id, name, deck_id) in decks:
    if deck_id not in deck_dict:
        deck_dict[deck_id] = []
    deck_dict[deck_id].append(name)

You can even ditch the initial check to create the list if you use a defaultdict
from collections import defaultdict

deck_dict = defaultdict(list)
for (id, name, deck_id) in decks:
    deck_dict[deck_id].append(name)

However, this is still the wrong structure, because you don't actually cares about which cards are in each deck - you care about which decks contain each card.
from collections import defaultdict

card_dict = defaultdict(set)
for (id, name, deck_id) in decks:
    card_dict[name].add(deck_id)

Creating you superset
"""-- ORIGINAL CODE --"""
superSet = []
for l in newlist:
    for c in l:
        if c not in superSet:
            superSet.append(c)

Then you create loop through every item in an item in newlist to create your superSet.  You have the same number of items in items in newlist as you had in decks, just organized differently, so all you get for using it instead is a slightly more complicated for loop
superSet = []
for (id, name, deck_id) in decks:
    if name not in superSet:
        superSet.append(name)

Notably, since now you're iterating over decks, you could roll this into the same loop you used to create deck_dict.  But there's still room for improvement, because you made superSet a list, while it would make much more sense for it to be a set.
superSet = set()
for (id, name, deck_id) in decks:
    superSet.add(name)

Since a set only ever contains one of each unique value, this saves you from having to check whether each object is in superSet before you add it.  The operation x in s is a lot faster for a set than a list (O(1) on average instead of O(n)).
If superSet is a set, you can actually use the same assignment you were using earlier for values: superSet = set(map(lambda x:x[1], decks)).  Note that we're using x[1] here instead of x[2] because we want the card name instead of the deckId.  You can also replace map with a generator comprehension: superSet = set(x[1] for x in decks), which is generally considered more 'pythonic' than using map().
Counting your pairs
"""-- ORIGINAL CODE --"""
count = 0
for x in superSet:
    for y in superSet:
        if x is y:
            continue
        #print(x)
        #print(y)
        for d in newlist:
            #print(d)
            if x in d and y in d:
                count += 1
        deckPerc = count / len(newlist)
        if deckPerc != 0.0:
            print(count / len(newlist)) #needs to go into the db
        count = 0

Iterating over superSet twice like this actually gives you every pair you need twice - once as x='Shock' and y='Bolt', and once as x='Bolt' and y='Shock'.  Fortunately, the itertools library has you covered with the combinations() function.  And with card_dict providing the set of decks that contain each card, finding out the total number of decks that contain a pair is simply a matter of taking the union of the sets like so:
from itertools import combinations
for card1, card2 in combinations(superSet, 2):
    shared_decks = card_dict[card1] & card_dict[card2]
    count = len(shared_decks)
    if count > 0:
        print(f'{card1} + {card2}: {count}')

Now, rather than calculate the deck percentage, I merely used the count of decks that share the pair.  This is because I ditched deck_dict in favor of card_dict, and don't actually have a simple way of calculating the total number of decks at this point in the code.  This isn't a problem for the code shown, as if deckPerc != 0.0 is equivalent to if count != 0, but if you need the percentage for your actual use case, then we need to go back to before this loop and create it.  You can use deck_count = len(set(card[2] for card in decks)), or you can fold the set construction into the existing loop through decks to avoid having to loop through it twice.
Bringing it all together
from classes.general import Database
from collections import defaultdict
from itertools import combinations

def main():
    dbm = Database()
    with dbm.con:
        dbm.cur.execute("""SELECT c.id, c.name, ctd.deckId FROM cards c
        JOIN cardToDeck ctd ON ctd.cardId = c.id
        JOIN deckToEvent dte ON dte.deckId = ctd.deckId
        JOIN eventToFormat etf ON etf.eventId = dte.eventId
        WHERE etf.formatId = 5
        ORDER BY ctd.deckId""")
        decks = dbm.cur.fetchall()

        deck_set = set()
        card_set = set()
        card_dict = defaultdict(set)
        for card_id, card_name, deck_id in decks:
            deck_set.add(deck_id)
            card_set.add(card_name)
            card_dict[card_name].add(deck_id)

        deck_count = len(deck_set)

        for card1, card2 in combinations(card_set, 2):
            shared_decks = card_dict[card1] & card_dict[card2]
            deckPerc = len(shared_decks) / deck_count
            if deckPerc > 0:
                print(f'{card1} + {card2}: {count}') # needs to go into the db

if __name__ == "__main__":
    main()

And there you have it!  You have two loops, one of which iterates through decks once, and the other loops through N*(N-1)/2 items, where N is the number of unique cards.  Using sets and dicts mean that the time complexity of the operations inside the loops are all O(1), so this is pretty much as fast as you can make it.
Additional Notes
I know very little about databases, so there may be improvements possible in that area that I am unaware of.  I noticed that you kept the entirety of the code in the with block with the connection open, even though you never use it after you create decks.  This doesn't harm anything that I'm aware of, but it's good practice to exit a context as soon as you no longer need it (and it reduces the level of indenting your code is at, which is nice).  On the other hand, if you plan on using the connection to modify the database at the end of the code, then obviously leave everything as it is.
Style-wise, you could stand to improve your naming conventions.  values, newlist, and superSet are not very descriptive names.  Notice how I used deck_set, card_set, and card_dict to provide some indication as to what is in each object.  Additionally, most python style guides (in particular PEP 8 the style guide for Python's standard library) recommend using lower_case_with_underscores for the names of variables rather than camelCase as you are using.  You are of course free to pick any naming convention you want, but you should conciously choose your convention, and (unless you have a reason not to) it is best to use one of the standard conventions.


Check the validity of a South African ID number using the Luhn Algorithm import java.math.BigInteger;

/**
 * Created by lungisani on 2017/02/25.
 */
public class Luhn {
    public Boolean getIdentitySummation(BigInteger identities){
        String identify = String.valueOf(identities);
        int sumOdd = 0, sumEven = 0, _doubled;
        int summation;
        try {
            String identity = String.valueOf(identify);
            int length = identity.length() - 1;
            char[] chars = identity.toCharArray();
            String check = identity.substring(12);
            for(int x  = 0; x < length; x++){
                if(x % 2 == 0 ){
                        String numString = String.valueOf(chars[x]);
                        int numbers = Integer.valueOf(numString);
                        sumOdd += numbers;
                }else if(x % 2 != 0){
                        String numString = String.valueOf(chars[x]);
                        int numbers = Integer.valueOf(numString);
                        int doubled = numbers * 2;
                        if(doubled > 9){
                            _doubled = doubled - 9;
                        }else{
                            _doubled = doubled;
                        }
                        sumEven += _doubled;
                }
            }
            summation = sumOdd + sumEven;
            int checksum = Integer.valueOf(check);
            if((summation * 9) % 10 == checksum)
                return Boolean.TRUE;
            else
                return Boolean.FALSE;
        } catch (Exception e) {
            e.printStackTrace();
            return Boolean.FALSE;
        }
    }
}

Let's work through some of the simpler issues, like code redundancy, and some helper-functions in the core library that help a lot, and will make your code simpler.

Double-checking else

When you have an either-or check in an if-condition, there's no need to double-check the else side of things.

you check for odd-positioned digits, which implies every other digit is even-positioned... This code:

if(x % 2 == 0 ){
    .....
}else if(x % 2 != 0){
    .....
}

can be just:

if(x % 2 == 0 ){
    .....
} else {
    .....
}

Try-Catch

There's no code in your function that throws an explicit exception - why do you have a try/catch? All integer parsing comes from digits in a BigInteger, so there can be no illegal characters, etc.

Character-to-digit-value

Your code has a lot of this type of logic:

String numString = String.valueOf(chars[x]);
int numbers = Integer.valueOf(numString);

but that can be simplified to just:

int numbers = Character.getNumericValue(chars[x]);

Separated sums

You have both sumOdd and sumEven, but there's no need for both. You can have one sum accumulator and use it in each side.

Simplifying the doubled digits

The algorithm requires doubles that are larger than 9 to be reduced by 9. Your code is:

int doubled = numbers * 2;
if(doubled > 9){
    _doubled = doubled - 9;
}else{
    _doubled = doubled;
}
sumEven += _doubled;

Using a "ternary" expression, and a little bit of manipulation on the math, you can reduce that to just:

sum += digit < 5 ? digit * 2 : digit * 2 - 9;

Autoboxing and conditionals

Java will "autobox" primitive variables like boolean to their full class types Boolean when needed, without any explicit handling. Let's take this exit segment:

if((summation * 9) % 10 == checksum)
    return Boolean.TRUE;
else
    return Boolean.FALSE;

This should have braces on the 1-liners, to be:

if((summation * 9) % 10 == checksum) {
    return Boolean.TRUE;
} else {
    return Boolean.FALSE;
}

but really, that's all unnecessary, because autoboxing comes to the rescue:

return (summation * 9) % 10 == checksum

That's all you need.

Conclusion

Putting all these suggestions together, you can significantly reduce the complexity of the code, to something like:

public static Boolean getIdentitySummationRL(BigInteger identities){
    char[] idchars = identities.toString().toCharArray();
    int sum = 0;
    // loop over each digit, except the check-digit
    for (int i = 0; i < idchars.length - 1; i++) {
        int digit = Character.getNumericValue(idchars[i]);
        if ((i % 2) == 0) {
            sum += digit;
        } else {
            sum += digit < 5 ? digit * 2 : digit * 2 - 9;
        }
    }
    int checkdigit = Character.getNumericValue(idchars[idchars.length - 1]);
    int compdigit = (sum * 9) % 10;

    return checkdigit == compdigit;
}

(I have checked that using my ID number, and it's OK)

Update

Note I have been doing a little more reading on Luhn's algorithm because of @Molvalio's comment and also I remember doing it a couple of decades ago for other numbers (not ID numbers) and I remember the checking algorithm to be different to this implementation. I was right that checking the number is simpler than your code. See the algorithm here: Luhn's Algorithm Verification

The point is that your code is computing a check-digit and comparing it with the existing digit, but you're missing the fact that the digit is designed to be incorporated in to the same calculations as the checksum, and a valid number has a resulting digit of 0.

Additionally, the Luhn's algorithm is computed right-to-left. In your case, because the length of SA ID numbers is 13, you're OK (both left and right digits are odd) but you should implement the algorithm more closely....

So, the code can be simplified further:

public static Boolean checkLuhn(BigInteger identities){
    char[] idchars = identities.toString().toCharArray();
    int sum = 0;
    // loop over each digit right-to-left, including the check-digit
    for (int i = 1; i <= idchars.length; i++) {
        int digit = Character.getNumericValue(idchars[idchars.length - i]);
        if ((i % 2) != 0) {
            sum += digit;
        } else {
            sum += digit < 5 ? digit * 2 : digit * 2 - 9;
        }
    }
    return (sum % 10) == 0;
}


Best practices to do complex forms with javascript? In a Rails application, I want to have a complex form with some dynamic behaviour. For example, I want to show a part of the form want I check a box or I want to have different values in a select box when I choose a radio button.

I do this with JQuery. My problem concern the archirecture. I created a form object and I create a init method which init every behaviour of every components like this :

  PaintingForm.prototype.init = function() {
    var country_listener;
    if ($('form.painting').size() === 1) {
      this.form = $('form.painting').first();
      this.initImageUploader();
      this.initSubmit();
      this.assignInputs();
      this.initAutoComplete();
      this.listenCategories();
      country_listener = new CountryInputListener('#painting_country_id', '#painting_region_id');
      return country_listener.init();
    }
  };

  PaintingForm.prototype.initImageUploader = function() { //some code };

  PaintingForm.prototype.initSubmit = function() { //some code };

  PaintingForm.prototype.onSubmit = function() { //some code };

  PaintingForm.prototype.assignInputs = function() { //some code };

  PaintingForm.prototype.listenCategories = function() { //some code };

  PaintingForm.prototype.onCategoryInputChange = function() { //some code };

  PaintingForm.prototype.loadTechnics = function() { //some code };

  PaintingForm.prototype.showDepthIfSculptureChoosen = function() { //some code };

  PaintingForm.prototype.onLoadTechnicsSuccess = function() { //some code };

  PaintingForm.prototype.initAutoComplete = function() { //some code };

I'm not satisfied. I think I can split the form like this :

PaintingForm.prototype.init = function() {
  var country_listener;
  if ($('form.painting').size() === 1) {
    this.form = $('form.painting').first();
    (new PaintingForm.Category).init()
    (new PaintingForm.AutoComplete).init()
    (new PaintingForm.Depth).init()
    (new CountryInputListener('#painting_country_id', '#painting_region_id')).init();
  }
};

//other behaviour for the form

this.PaintingForm.Category = (function() {
  //Behaviour for categories
})();

this.PaintingForm.Depth = (function() {
  //Behaviour for depth
})();

this.PaintingForm.AutoComplete = (function() {
  //Behaviour for autocomplete
})();

It's better but I'm not sure. There is a lot of interactions between fields. Should I create one object for every fields? Is there a solution with a framework like or angular? Do you have correct examples?

For a single form, this is the exact reason Knockout.js was created - tutorials here: http://learn.knockoutjs.com/#/?tutorial=intro

Angular is more of a full blown "SPA" framework, Knockout is data-binding for JavaScript with things like this: http://knockoutjs.com/documentation/if-binding.html


Trie implementation for left to right wildcard search I was working on a task to implement auto suggest from an array of couple of thousands of strings.

Of course, the first and the easiest implementable was to go through each element and do a[i].startWith(query). Technically the time complexity of this algorithm then would be \$O(n+(q*m))\$. Here, n equals the number of elements in the array, q equals the number of total matches and m equals the number of characters in the query (I am not sure how accurate I am here).

Regardless, I have created a following Trie which at every node keeps track of every child that has a word ending there.

For example, if the query is "can" then with only three traversal the tree will return cancel, cancer, can and cancellation because the node 'n' will have connections to all the sub trees that has a word ending!

I would really like to hear some comments on this.

package utils;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Set;

/**
 *
 * @author Sapan
 */
public class Trie {

    private Map<Character, Trie> children;
    private String word = null;
    private Set<Trie> connections;
    static int count = 0;

    public Trie() {
        this.children = new HashMap<>();
        connections = new HashSet<>();
    }
    /*
     * Builds a Trie with given query
     */

    public void add(String word) {
        if (word == null || "".equals(word.trim())) {
            throw new IllegalArgumentException("Query can not be empty or null");
        }
        Trie t = this;
        for (Character c : word.toCharArray()) { //for every character in query do the following....
            //add character to the current instance and get the child instance right after adding the current char
            if (!t.children.containsKey(c)) {
                t.children.put(c, new Trie());
            }
            t = t.children.get(c);
        }
        t.word = word;
    }

    public Set search(String word) {
        count = 0;
        Trie current = this;
        Set<Trie> matches = new HashSet();
        for (Character c : word.toCharArray()) {
            ++count;
            current = current.children.get(c);
            if (current == null){
                return matches;
            }
        }
        //System.out.println(current);
        if (current == null) {
            return matches;
        }

        matches = current.connections;

        return matches;
    }
    public Set build() {
        //matches = new HashSet<>();
        ++count;
        for (Character c : this.children.keySet()) {
            this.connections.addAll(this.children.get(c).build());
        }
         if (this.word != null && this.word.length() > 0) {
             this.connections.add(this);
        }
         if (this.children.keySet().isEmpty()) {// we are at the leaf node
            this.connections.add(this);
        }
        return this.connections;
    }

    public static void main(String[] args) {
        Trie t = new Trie();
        t.add("can");
        t.add("kangaroo");
        t.add("roo");
        t.add("rash");
        t.add("ram");
        t.add("rat");
        t.add("kansas");
        t.add("banana");
        t.add("ban");
        t.add("cancel");
        t.add("cancer");
        t.add("add");
        t.add("addition");
        t.add("sub");
        t.add("subtraction");
        t.add("ape");
        t.add("apple");
       // t.build();
        t.build();
        System.out.println(t);
        System.out.println(t.count+"----");
        //System.out.println(t);
       // List<Trie> l = t.search("ap");

        //System.out.println(l);
       // for (char c = 'a'; c <= 'z'; c++) {
           Set<Trie> l = t.search("ca");
           System.out.println(t.count);
            System.out.println("words starting from ca ");
            for (Trie trie : l) {
                System.out.println(trie.word);
            }
    }
}

Eclipse says that the second null check in the search method is dead code:

if (current == null) {
    return matches;
}

Furthermore, it could be refactored to the following:

public Set<Trie> search(final String word) {
    count = 0;
    Trie current = this;
    for (final Character c: word.toCharArray()) {
        ++count;
        current = current.children.get(c);
        if (current == null) {
            return Collections.emptySet();
        }
    }
    return current.connections;
}

You should specify the return types:

public Set<Trie> search(final String word) { ... }
public Set<Trie> build() {... }

instead of

public Set search(final String word) { ... }
public Set build() {... }

Consider using

StringUtils.isBlank instead of the (word == null || "".equals(word.trim())) and
StringUtils.isNotEmpty instead of (this.word != null && this.word.length() > 0).

I've found them more readable.
If I'm right you could iterate over the values directly in the build method:

for (final Trie childTrie: children.values()) {
    connections.addAll(childTrie.build());
}

I'd separate the builder logic from the Trie class. Currently clients can easily misuse this class:

t.build();
t.add("can2");

(Effective Java, 2nd Edition, Item 2: Consider a builder when faced with many constructor parameters)
connections does not seem a good name. It took some time to figure out that this set contains the descendant Trie objects which has a word. I might call it descendantsWithWord. Furthermore, it could contain only the words (strings), not the Trie objects, so it could be simply Set<String> descendantWords.
The static count field does not smells well. It is used for two different purposes. I'd create a createdTrieObjects field:

private int createdTrieObjects = 1;

and increment it in the add method:

t.children.put(c, new Trie());
createdTrieObjects++;

On the other hand, in the search method you could return a SearchResult object which contains the search depth (it's the count currently) and the result strings:

public class SearchResult {

    private final Set<String> words;
    private final int searchDepth;

    ...

}

It contains only a set of string instead of the Set<Trie> collection. I think clients should not know about the internal Trie objects. If you don't need the search depth return with Set<String> only:

public Set<String> search(final String wordStart) {
    Trie current = this;
    for (final Character c: wordStart.toCharArray()) {
        current = current.children.get(c);
        if (current == null) {
            return Collections.emptySet();
        }
    }
    return Collections.unmodifiableSet(current.descendantWords);
}

Please note the Collections.unmodifiableSet call. It prohibits malicious clients to modify the Trie's internal structure. (Effective Java, 2nd Edition, Item 39: Make defensive copies when needed)
You could eliminate the build method and the word field if you fill the descendantWords in the add method:

 public void add(final String word) {
    checkArgument(StringUtils.isNotBlank(word), "word can not be blank");
    Trie current = this;
    descendantWords.add(word);
    for (final Character c: word.toCharArray()) {
        if (!current.children.containsKey(c)) {
            current.children.put(c, new Trie());
            createdTrieObjects++;
        }
        current = current.children.get(c);
        current.descendantWords.add(word);
    }
}

Some small notes about the edit:

Instead of the checked IllegalAccessException I'd throw runtime IllegalStateException. It's rather a programming error if a client tries to add new element to a built trie.  (Effective Java, 2nd Edition, Item 58: Use checked exceptions for recoverable conditions and runtime exceptions for programming errors)
About the s variable in the search method: Try to minimize the scope of local variables. It's not necessary to declare them at the beginning of the method, declare them where they are first used. (Effective Java, Second Edition, Item 45: Minimize the scope of local variables)
You could copy the descendantsWithWord set with the HashSet's constructor instead of the for loop in the search method:

return new HashSet<String>(current.descendantWords);

The isBuilt flag should be at the beginning of the class with the other fields.

Here is the refactored version:

import java.util.Collections;

import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;

import org.apache.commons.lang3.StringUtils;

import static com.google.common.base.Preconditions.*;

public class Trie {

    private final Map<Character, Trie> children = new HashMap<Character, Trie>();
    private final Set<String> descendantWords = new HashSet<String>();

    public Trie() {
    }

    public void add(final String word) {
        checkArgument(StringUtils.isNotBlank(word), "word can not be blank");
        Trie current = this;
        descendantWords.add(word);
        for (final Character c: word.toCharArray()) {
            if (!current.children.containsKey(c)) {
                current.children.put(c, new Trie());
            }
            current = current.children.get(c);
            current.descendantWords.add(word);
        }
    }

    public Set<String> search(final String wordStart) {
        checkNotNull(wordStart, "wordStart cannot be null");
        Trie current = this;
        for (final Character c: wordStart.toCharArray()) {
            current = current.children.get(c);
            if (current == null) {
                return Collections.emptySet();
            }
        }
        return Collections.unmodifiableSet(current.descendantWords);
    }
}

Edit for the comment:

In your original implementation there is a set (descendantsWithWord) in every node which refers to Trie objects. In this answer there is a set (descendantWords) in every level which refers to String objects.

The size of the sets are the same, Strings (as well as other objects, like Tries) are on the heap, the set contains only references to objects, so it does not count if they are Strings, Tries or other Objects, there is no difference in memory consumption and there is no duplication. The only difference is the method where the trie builds the set and the generic type of the set (references in the set).

I'm not too familiar with tries and articles on the Wikipedia do not mention that trie nodes should or should not store each descendants (not just references to the nodes of the next level), so I'm not sure that this is "officially" a trie or not.

Anyway, if I'm right you need fast search, so memory consumption looks is secondary here. Precalculating and storing the result in a cache (i.e. descendantWords) seems a better solution that traversing a subtree and building strings from characters on every search request as you did it too.

Furthermore, I'd consider using a Map<String, Set<String>> structure:

c,      [can, cancel, cancer]
ca,     [can, cancel, cancer]
can,    [can, cancel, cancer]
canc,   [cancel, cancer]
cance,  [cancel, cancer]
cancel, [cancel]
cancer, [cancel]
...

It's very similar to the trie above since (1) the trie's nodes are the same as the keys in this map and (2) the values of the map are the same as the descendantWords set in the trie. An advantage could be a simpler implementation, especially with a Multimap from Guava:

public class AutoSuggestCache {

    private final SetMultimap<String, String> words = HashMultimap.create();

    public AutoSuggestCache() {
    }

    public void add(final String word) {
        checkArgument(StringUtils.isNotBlank(word), "word can not be blank");
        for (int endIndex = 0; endIndex < word.length(); endIndex++) {
            final String wordStart = word.substring(0, endIndex);
            words.put(wordStart, word);
        }
    }

    public Set<String> search(final String wordStart) {
        checkNotNull(wordStart, "wordStart cannot be null");
        return Collections.unmodifiableSet(words.get(wordStart));
    }
}


Making a faster parallel quicksort I'm trying to take my existing parallel Quicksort and make it execute faster. Below is what I have but half the time the optimized version doesn't give me a faster time.

I use them to compare big arrays whose sizes are 10,000 or more. Any advice on what I should change? Nthreads is declared in the main function and set to 4.

void quickSort_parallel_omp(int arr[], int low, int high)
{
    int pi;
    if (low < high)
    {
        pi = partition(arr, low, high);
        omp_set_nested(1);
        #pragma omp parallel sections
        {
            #pragma omp section
            quickSort_parallel_omp(arr, low, pi - 1);
            #pragma omp section
            quickSort_parallel_omp(arr, pi + 1, high);
        }
    }
}

void quickSort_Optparallel_omp(int arr[], int low, int high)   //optimized version
{
    int pi;
    if (low < high)
    {
        pi = partition(arr, low, high);
        omp_set_nested(1);
        #pragma omp parallel sections num_threads(Nthreads)
        {
            #pragma omp section
            quickSort_Optparallel_omp(arr, low, pi - 1);
            #pragma omp section
            quickSort_Optparallel_omp(arr, pi + 1, high);
        }
    }
}

Nested parallelism is expensive. It was designed for around 1 nesting level when the first level is not parallel enough, and loop slicing. This kind of recursive problems are much better handled by tasks.

void quickSort_tasks(int arr[], int low, int high)
{
    int pi;
    if (low < high)
    {
        pi = partition(arr, low, high);
        #pragma omp task
        quickSort_Optparallel_omp(arr, low, pi - 1);
        #pragma omp task
        quickSort_Optparallel_omp(arr, pi + 1, high);
    }
}

void quickSort_omp(int arr[], int low, int high) {
   #pragma omp parallel
   #pragma omp single
   quickSort_tasks(arr,low,high);
   // implicit synchronization at the end of parallel section
   // otherwise use
   // #pragma omp taskwait
}

Other sorting algorithms may offer higher performance or more efficient parallelization, although being algorithmically more complex. I'm thinking about radix, pigeonhole and merge sorting.

Parallel radix sort explanation: http://projects.csail.mit.edu/wiki/pub/SuperTech/ParallelRadixSort/Fast_Parallel_Radix_Sort_Algorithm.pdf


Efficient random code generator I made a 16 character code generator. It works like a charm now, but I was wondering if there's any way to do it more efficiently.

public partial class MainWindow : Window
{

    //Globals
    string[] Letters = new string[27] { "A", "B", "C", "D", "E", "F", "G", "H", "H", "I", "J", "K", "L", "M", "N", "O", "P", "Q", "R", "S", "T", "U", "V", "W", "X", "Y", "Z"};
    string[] Numbers = new string[9] { "1", "2", "3", "4", "5", "6", "7", "8", "9" };

    Random r = new Random();
    int i = 0;
    int a = 0;

    private string[] generateCode()
    {

        string[] RL = new string[9];
        string[] RN = new string[9];

        //Random letters
        while (i < 9)
        {
            RL[i] = Letters[r.Next(0, Letters.Length - 1)];
            i++;
        }
        //Random numbers
        while (a < 9)
        {
            RN[a] = Numbers[r.Next(0, Numbers.Length - 1)];
            a++;
        }

        TextBox.Text = String.Format("{0}{1}{2}{3}-{4}{5}{6}{7}-{8}{9}{10}{11}-{12}{13}{14}{15}", RN[0], RL[1], RN[1], RL[2], RN[2], RL[3], RN[3], RL[4], RN[4], RL[5], RN[5], RL[6], RN[6], RL[7], RN[7], RL[8], RN[8]);

        //Resetting the while loop
        i = 0;
        a = 0;

        //Clipboard
        Clipboard.SetText(TextBox.Text);
        Message.Content = ("Copied to clipboard!");

        return RL;
    }

    public MainWindow()
    {
        InitializeComponent();
    }

    private void Generate_Click(object sender, RoutedEventArgs e)
    {
        generateCode();
    }

First a bit of nitpicking:

Naming conventions:

Please read the Capitalization Conventions by Microsoft. Method names will always be capitalized:

generateCode() becomes GenerateCode()

Your private fields (globals in your code) should be lower case: letters and numbers.

The var keyword:

From the C# Programming Guide:

  The var keyword can also be useful when the specific type of the variable is tedious to type on the keyboard, or is obvious, or does not add to the readability of the code.

So lines like:

string[] RL = new string[9];

would become:

var RL = new string[9];

Separation of concerns:

From the method that generates the code, you set the value of the textbox. This is not a good practice reagarding maintenance of your code. What if you want to use that method somewhere else?

Assign the value of the method to a variable in the click event of the button and assign that value to the textbox.

private string GenerateCode()
{
    //...
}

private void Generate_Click(object sender, RoutedEventArgs e)
{
    var code = GenerateCode();
    YourTextBox.Text = code;
}

And is there a specific reason why your code returns the RL array?

The initialisation of your arrays could also be made simpler:

private readonly char[] letters= "ABCDEFGHIJKLMNOPQRSTUVWXYZ".ToCharArray();
private readonly char[] numbers = "01234567489".ToCharArray();

The answer of austin wernli already provides a nice solution, I have written another way of achieving this. This method uses only one loop and no if statements:

private readonly char[] _letters = "ABCDEFGHIJKLMNOPQRSTUVWXYZ".ToCharArray();
private readonly char[] _numbers = "01234567489".ToCharArray();
private readonly Random _random = new Random();

private string GenerateCode()
{
    var parts = new string[4];

    for (var i = 0; i < 4; i++)
    {
        parts[i] = String.Format("{0}{1}{2}{3}", _numbers[_random.Next(0, _numbers.Length)], _letters[_random.Next(0, _letters.Length)], _numbers[_random.Next(0, _numbers.Length)], _letters[_random.Next(0, _letters.Length)]);
    }

    return String.Join("-", parts);
}

Example usage:

private void Generate_Click(object sender, RoutedEventArgs e)
{
    YourTextBox.Text = GenerateCode();
}

This can be a bit simplified by generating an extension method to get a random element from the array:

The extension method:

public static class Extensions
{
    private static readonly Random Random = new Random();

    public static char GetRandom(this char[] collection)
    {
        return collection[Random.Next(0, collection.Length)];
    }
}

The method:

private string GenerateCode()
{
    var parts = new string[4];

    for (var i = 0; i < 4; i++)
    {
        parts[i] = String.Format("{0}{1}{2}{3}", _numbers.GetRandom(), _letters.GetRandom(), _numbers.GetRandom(), _letters.GetRandom());
    }

    return String.Join("-", parts);
}


Copy and paste formatting is slow using VBA The following code is a function that works. It's just slow and I don't know how to speed it up. It takes an Excel row number and the value of its headerval (string) and finds the same headerval on a different sheet then copies the formatting and applies it to our new sheet. The true false is because the source sheet has 2 different formatting options. It passes in the row to use either 23 or 24. ZROW is a public variable which is set with the ROW to start looking. srccolbyname function gets a col number from the source sheet which has the same headerval.

Function formatrow(roww As Long, header As Boolean)

Application.Calculation = xlCalculationManual
Application.ScreenUpdating = False

Dim headerval As String
Dim sht As Worksheet
Set sht = ThisWorkbook.Sheets("DEALSHEET")
Dim sht2 As Worksheet
Set sht2 = ThisWorkbook.Sheets("Sheet1")
If header = True Then: srcrow = 23: Else: srcrow = 24
LastColumn = sht.Cells(ZROW + 1, sht.Columns.Count).End(xlToLeft).Column

For x = 2 To LastColumn
headerval = sht.Cells(ZROW + 1, x).Value
srccol = srccolbyname(headerval)
sht2.Cells(srcrow, srccol).Copy 'THIS IS SLOW
sht.Cells(roww, x).PasteSpecial Paste:=xlPasteFormats,    Operation:=xlNone, _
    SkipBlanks:=False, Transpose:=False
Application.CutCopyMode = False
Next x

Application.Calculation = xlCalculationManual
Application.ScreenUpdating = False

End Function

As requested here is the support function referenced above.

Public Function srccolbyname(strng_name As String) As Integer
Call findcol 'find ZROW
Dim x As Integer
Dim sht As Worksheet
Set sht = ThisWorkbook.Sheets("Sheet1")
LastColumn = sht.Cells(22, sht.Columns.Count).End(xlToLeft).Column
    For x = 2 To LastColumn
        chkval = sht.Cells(22, x).Value
            If Trim(UCase(chkval)) = Trim(UCase(strng_name)) Then
            srccolbyname = x
            Exit For
        Else
        srccolbyname = 2
        End If
    Next x
End Function

This:

If header = True Then: srcrow = 23: Else: srcrow = 24

Would be much better off as an IIf statement:

srcrow = IIf(header, 23, 24)

Also note, this:

If {boolean-expression} = True Then

..is always redundant and can be written as:

If {boolean-expression} Then

With header being a Boolean already, there's no need to compare it to a Boolean literal to obtain a {boolean-expression}!

One of the most important things to do when you have blocks in your code (If...End If, For...Next, While...Wend, Do...Loop, but also Sub...End Sub, etc.), is indentation.

Here's your function, properly indented:

Function formatrow(roww As Long, header As Boolean)

    Application.Calculation = xlCalculationManual
    Application.ScreenUpdating = False

    Dim headerval As String
    Dim sht As Worksheet
    Set sht = ThisWorkbook.Sheets("DEALSHEET")
    Dim sht2 As Worksheet
    Set sht2 = ThisWorkbook.Sheets("Sheet1")
    srcrow = IIf(header, 23, 24)
    LastColumn = sht.Cells(ZROW + 1, sht.Columns.Count).End(xlToLeft).Column

    For x = 2 To LastColumn
        headerval = sht.Cells(ZROW + 1, x).Value
        srccol = srccolbyname(headerval)
        sht2.Cells(srcrow, srccol).Copy 'THIS IS SLOW
        sht.Cells(roww, x).PasteSpecial Paste:=xlPasteFormats, Operation:=xlNone, _
                                        SkipBlanks:=False, Transpose:=False
        Application.CutCopyMode = False
    Next x

    Application.Calculation = xlCalculationManual
    Application.ScreenUpdating = False

End Function

Notice how much easier it is to see where the loop starts, where it ends, and what's in its body.

Now sure what ZROW is, it's not declared in the scope of formatrow so I'm assuming it's a module-scoped constant and that other procedures in that module as using it. If only formatrow uses it, it should be scoped to formatrow.

You declared sht and sht2 as Worksheet objects; you should be querying the Worksheets collection, not Sheets (which contains charts and other sheet types).

But then, if these sheets aren't dynamically generated, you shouldn't need to query any worksheet collection and get the objects by their "sheet name" (which the users can change at any time!) - instead, use their "code name": VBA defines a global-scope object for every Excel object (including ThisWorkbook, but also Sheet1 and every sheet in the workbook), so you can use the Properties toolwindow (F4) to set their (Name) property to a meaningful identifier, and then use that identifier in code, so you can delete all these:

Dim sht As Worksheet
Set sht = ThisWorkbook.Sheets("DEALSHEET")
Dim sht2 As Worksheet
Set sht2 = ThisWorkbook.Sheets("Sheet1")

And then do this (assuming the sheet labelled "DEALSHEET" is named DealSheet):

LastColumn = DealSheet.Cells(ZROW + 1, DealSheet.Columns.Count).End(xlToLeft).Column

And this:

headerval = DealSheet.Cells(ZROW + 1, x).Value

And that:

Sheet1.Cells(srcrow, srccol).Copy 'THIS IS SLOW

And so on.

Now, about performance. You need a radically different approach. How would you do it if you were going to do it manually? Would you Copy+Paste one cell at a time, or Copy+Paste the whole row at once?

You want to copy/paste the formats on srcrow from column 2 through LastColumn: do that.

Sheet1.Range(Sheet1.Cells(srcrow, 2), Sheet1.Cells(srcrow, LastColumn)).Copy
DealSheet.Range(DealSheet.Cells(roww, 2), DealSheet.Cells(roww, LastColumn)) _
         .PasteSpecial Paste:=xlPasteFormats, _
                       Operation:=xlNone, _
                       SkipBlanks:=False, _
                       Transpose:=False

No need to loop, no need to lookup column numbers. Unless I missed something. Should be much faster!

I also ran Rubberduck inspections (build 2.0.10, not released yet). A few things to note:

formatrow is implicitly public. Consider specifying an explicit access modifier.
sht, sht2, strng_name and x are poor names. Consider renaming them; avoid disemvoweling, numeric suffixes, underscores, type prefixes, and 1-2 character identifiers.
formatrow is a Function, but its return value is never even assigned so it always returns an implicit Variant/Empty; it should probably be a Sub.
Parameters roww, header, and strng_name are implicitly passed by reference, and can safely be passed by value (ByVal) instead.
Explicit Call syntax is obsolete. Use the implicit call syntax instead (which you've used to call srccolbyname anyway).


GUI implementation with Racket I am studying the book Realm of Racket.

On chapter 5, there is a challenge:

Find an image of a locomotive. Create an animation that runs the locomotive
from just past the left margin to just past the right margin of the screen. Next, modify your program so the locomotive wraps around to the left side of the screen after passing the right margin.

I created the following code using Racket and Dr. Racket.

However, I bet it could be improved.

My doubt is specially about the transition from the right side to the left side. I think there should be a better way of doing this:

#lang racket

(require 2htdp/universe 2htdp/image)

(define LOCOMOTIVE image-inserted-through-DrRacket )

(define HEIGHT 800)

(define WIDTH 800)

;essa função muda a velocidade com que a figura se mexe

(define (add-3-to-state current-state)
  (if (< current-state 1055)
      (+ current-state 6)
      (- current-state 800)))

(define (locomotive-running current-state)
  (place-image LOCOMOTIVE current-state (/ WIDTH 2)
               (empty-scene WIDTH HEIGHT)))

(big-bang 0
          (on-tick add-3-to-state)
          (to-draw locomotive-running))

That's the image that I used:

Broadly speaking, I'd say it looks pretty good. I'm a bit confused by the '1055'; I'm guessing that you're trying to wait until the train leaves the screen entirely? Perhaps you could write this as

(+ WIDTH (/ (image-width LOCOMOTIVE) 2))

... and also replace the 800 with WIDTH. Alternatively, you could return either 0 or

(- (/ (image-width LOCOMOTIVE) 2))

when the train goes off the right edge, so that it makes its entrance smoothly.


Empty block in recursion base case I wrote a recursive function to uncheck all menu nodes. It works well, but I need to know if it is right to leave empty block inside if (nodes[i].children.length === 0) base case.

Is there a better design choice?

The function:

function uncheck_all_tree (nodes) {
  var i, n, n_dom_el;

  for (i = 0; i < nodes.length; i++) {
    if (nodes[i].children.length === 0) {
      // recursion bottom;
    } else {
      uncheck_all_tree(nodes[i].children);
    }

    // pop nodes from recursion stack
    n = tree.jstree('get_node', nodes[i]);
    n_dom_el = document.getElementById(n.id + '_anchor');
    if (n_dom_el !== null) {
      console.log(n.id + '_anchor');
      n_dom_el.className = 'jstree-anchor';
    }
    n.state.checked = false;
  }
}

More things that can be improved:
Variable declarations
To allow for easier understanding you should declare variables as close as possible to their usage (even though it's semantically no different)
Additionally IIRC javascript variables are usually camelCase and not snake_case. Furthermore it's useful to write out variable names instead of shortening them as much as possible. This reduces cognitive load and makes the code easier to read.
General case
Simply through the way your code works, I'm dead sure that you don't even need that if-statement you have there. It's superfluous, because it's already working for the general case. If your nodes have 0 elements, the for-loop is not executed and the "recursion bottom" is reached.
"Final" result:
function uncheck_all_tree (nodes) {
    for (var i = 0, length = nodes.length; i < length; i++) {
        uncheck_all_tree(nodes[i].children);
        var node = tree.jstree('get_node', nodes[i]);
        var nDomElement = document.getElementById(n.id + '_anchor');
        if (nDomElement !== null) {
            console.log(node.id + '_anchor');
            nDomElement.className = 'jstree-anchor';
        }
        node.state.checked = false;
    }
}

In addition to that Ismael Miguel has suggested in a comment to use Array.forEach.call(nodes[i].children, function(node) { [...] }); to further improve performance. More information on how that works can be found at the MDN Documentation


feedback needed for my first python tkinter simple MVC, list element to print to label Just starting to get the idea of MVC with tkinter. I wrote this simple program and I'm looking for feedback. There are no errors, but I don't know if I set it up right or not.

import tkinter as tk

class Model():

    def __init__(self):
      self.list = ["email", "sms", 'voice']

class View(tk.Frame):
    def __init__(self, parent, controller):
        self.controller = controller
        tk.Frame.__init__(self, parent, bg="yellow", bd=2,
           relief=tk.RIDGE)
        self.parent = parent
        self.pack()

        self.labelVariable=tk.StringVar()
        self.output=tk.Label(self, textvariable=self.labelVariable,
             bg="orange", fg="white")
        self.output.pack()

        self.hello = tk.Button(self, text="Show", command=
             self.controller.hello_Button_Pressed,  bd=2,
             relief=tk.RIDGE)
        self.hello.pack(side="left")

class Controller():
    def __init__(self):
        self.root = tk.Tk()
        self.model = Model()
        self.view = View(self.root, self)

        self.root.title("MVC example")
        self.root.geometry("250x350")
        self.root.config(background="LightBlue4")
        self.root.mainloop()

    def hello_Button_Pressed(self):
        self.view.labelVariable.set(self.model.list)

if __name__ == '__main__':
    c = Controller()

The code looks like it is set up fine for MVC. I'm not an expert but I have enogh experience to tell that it's fine.


Casino jackpot models In the world of casinos, there are different types of games and jackpots. In this example, there are normal jackpots, multi-casino jackpots. I have a separate model for each of these. I need to display these in-line with each other. For instance, I'm grabbing the top 10 Caribbean Stud jackpots (Model: Jackpot), then displaying the top 10 multi-casino Caribbean stud jackpots (Model: MultiJackpot), and then displaying the top 10 Blackjack jackpots (Model: Jackpot) followed by the top 10 multi-casino Blackjack jackpots (Model: MultiJackpot).

I have two main problems with a chunk of code: I need to interpolate 2 different models in a specific order in a list.

This is a simplified example, but I have a similar scenario with slot jackpots where the models differ even more, which is preventing me from merging the models themselves.

Currently (and very inefficiently) I am doing 1 database call for each game and assigning it to its own instance variable for the view.

@caribbean_stud =                   Jackpot.where(:jackpot_game_id => JackpotGame.find_by_name("Caribbean Stud")).order("prize DESC").limit(10)
@multi_caribbean_stud =             MultiJackpot.where(:multi_jackpot_game_id => MultiJackpotGame.find_by_name("Caribbean Stud")).order("prize DESC").limit(10)
@progressive_blackjack =            Jackpot.where(:jackpot_game_id => JackpotGame.find_by_name("Progressive Blackjack")).order("prize DESC").limit(10)
@multi_progressive_blackjack =      MultiJackpot.where(:multi_jackpot_game_id => MultiJackpotGame.find_by_name("Progressive Blackjack")).order("prize DESC").limit(10)

In the view, I'm taking each instance variable and hard coding the order/interpolation.

The obvious 2 problem I'm running into:

Many more database calls than necessary (I have over 30 games I'm doing this for)
The view is not DRY (I could move the output for each model here into a partial, but I'm still hard coding the actual order & interpolation.)

First of all, the fact that you are mixing the use of Jackpot and MultiJackpot in this way suggests that they are really 2 different instances of a common, underlying concept. That calls into question whether they really need to be implemented as two different models. But that's up to you; you know the specifics of your own application.

As for removing the duplication, how about something along the lines of:

games     = ['Caribbean Stud', 'Progressive Blackjack']
@jackpots = games.map do |game|
  [Jackpot.where(...),
   MultiJackpot.where(...)]
end

You end up with an extra level of array nesting in the results, which could be eliminated with flatten(1). Or if don't mind adding your own methods to Array, I like to add one called mappend... I find it useful in almost every project. It it like map, but it appends all the returned arrays together to form a single array.

If you need the names of the games together with the results, you can games.zip(@jackpots) (you need the extra level of nesting for that one).

As for the DB calls, you'll find it hard to work around that while still using your nice Active Record query helpers. When DB performance is critical, I usually end up rolling my own SQL, perhaps using custom helpers added to model classes.