Load txt to Scripting.Dictionary This code load a 40MB txt into a dictionary. It runs in about 40 seconds (sometimes 20, no idea why). Is there a way to make it run under 4 seconds, or hopefully in 1 second?

Sub ScriptDic()

Dim FileNum As Integer
Dim DataLine As String
Dim tmp As Variant
Dim Dict As Object
Dim duplicatecount As Object
Dim key As String
Dim count As Long

Set Dict = CreateObject("Scripting.Dictionary")
Set duplicatecount = CreateObject("Scripting.Dictionary")

Sheets("Control").Cells(5, 3).Value2 = Now
Filename = "C:\Users\MyFolder\Documents\PerformanceTests\MyData.txt"
FileNum = FreeFile()

Open Filename For Input As #FileNum
While Not EOF(FileNum)
    Line Input #FileNum, DataLine ' read in data 1 line at a time
    tmp = Split(DataLine, Chr(9))
    key = tmp(7) & "-" & tmp(8)
    If Not Dict.exists(key) Then
        Dict.Add key, tmp
        duplicatecount.Add key, 1
    Else
        count = duplicatecount(key)
        duplicatecount.Remove (key)

        Dict.Add key & ">" & count, tmp
        duplicatecount.Add key, count + 1
    End If
Wend

Sheets("Control").Cells(6, 3).Value2 = Now
End Sub

I assume you're working with a 40MB file instead of a 40GB file. The performance will vary greatly depending upon:

the length of each line in the file
the number of lines in the file
the number of uniquely keyed-lines in the file

I've worked with a contrived data-file that is 5.127MB in size and has 250,000 data rows like:

a   b   c   d   e   f   g   h   i   j
b   c   d   e   f   g   h   i   j   k
a   b   c   d   e   f   g   h   i   j

The file has nearly all duplicated records, so I'm pushing the limits on the unique key approach. A file with all unique values will perform differently.

Using your code against the 5MB file, it runs in 6.18s. If I only read the file, it reads in 0.11s. If I only read the file, split each line and build a key, it runs in 0.76s. So approximately 5.42s, or 88% of the duration is related to dictionary manipulation.

So, what can we do in VBA, to improve your code

Option Explicit
You haven't included it in your code, so I assume it isn't declared.

Declare filename As String
The filename variable isn't declared, although you may have declared it at global scope. Option Explicit shows e this right away.

Dim filename As String

Declare tmp as a String Array
tmp is assigned with the Split function, which returns a String array, so declare tmp accordingly and you'll use a great deal less memory.

Dim tmp() As String

Early Binding
You're using CreateObject("Scripting.Dictionary"), so your code will fail anyway, if Microsoft Scripting Runtime isn't available. But more importantly, by using late-binding, COM has to work harder to find the methods you're using. It's much better to early-bind by adding a reference to Microsoft Scripting Runtime.:

Dictionary variable names
You've used ambiguous names: Dict and duplicatecount. You'd be better off with more meaningful names (I don't know what your data is, so I'm using less un-meaningful names) like:

Dim allRecords As Scripting.Dictionary
Dim uniqueKeyCounts As Scripting.Dictionary

Set allRecords = New Scripting.Dictionary
Set uniqueKeyCounts = New Scripting.Dictionary

Avoid using magic function/literal values
You're referring to the Tab character Chr(9) which incurs time on every line. VBA has a built-in constant for Tab: vbTab, which makes it more efficient and easier to read.

tmp = Split(DataLine, vbTab)

Unique keys and Dictionary usage
The code to build a unique key and populate the dictionaries is the most expensive, so let's be careful about checking the smallest dictionary more frequently than the large dictionary.

Also, there's no need to Remove and then Add back, when we can just increment the count of the existing entry.

If uniqueKeyCounts.Exists(key) Then
  'Retrieve the current count once
  Dim currentCount As Long
  currentCount = uniqueKeyCounts.Item(key)
  allRecords.Add key & ">" & currentCount, DataLine
  'Don't remove and re-add, just increment the counter
  uniqueKeyCounts(key) = currentCount + 1
Else
  uniqueKeyCounts.Add key, 1
  allRecords.Add key, tmp
End If

Close the file
You've opened the file and read it all, but you've forgotten to close the file:

Close #FileNum

Timing events
Using Now() offers very limited granularity. Timer gives you improved granularity but also a few problems when timed-code runs through midnight, but it avoids the need for win32 functions. If you want better timing see GetTickCount, or better still, QueryPerformanceCounter.
I've used Timer for the sake of simplicity.

Dim start As Double
start = Timer
'...Do stuff...
Debug.Print Timer - start

So, when I put it altogether, the file is read in 4.85s, or about 22% faster.

Sub ScriptDic()

  'Declare fileName
  Dim filename As String

  Dim FileNum As Integer
  Dim DataLine As String
  'Declare tmp as a string array
  Dim tmp() As String
  'Declare as Scripting.Dictionary to get early-binding benefits
  Dim allRecords As Scripting.Dictionary
  Dim uniqueKeyCounts As Scripting.Dictionary
  Dim key As String
  Dim count As Long

  'USe early binding
  Set allRecords = New Scripting.Dictionary
  Set uniqueKeyCounts = New Scripting.Dictionary

  Sheets("Control").Cells(5, 3).Value2 = Now
  filename = "C:\Temp\test100k.txt"
  FileNum = FreeFile()

  Dim start As Double
  start = Timer
  Open filename For Input As #FileNum
  Do While Not EOF(FileNum)
      Line Input #FileNum, DataLine ' read in data 1 line at a time
      'USe the vbTab constant
      tmp = Split(DataLine, vbTab)
      key = tmp(7) & "-" & tmp(8)
      'Check the unique dictionary - it's almost always smaller
      If uniqueKeyCounts.Exists(key) Then
        'Retrieve the current count once
        Dim currentCount As Long
        currentCount = uniqueKeyCounts.Item(key)
        allRecords.Add key & ">" & currentCount, DataLine
        'Don't remove and re-add, just increment the counter
        uniqueKeyCounts(key) = currentCount + 1
      Else
        uniqueKeyCounts.Add key, 1
        allRecords.Add key, tmp
      End If
  Loop

  'Close the file
  Close #FileNum

  Debug.Print Timer - start
  Sheets("Control").Cells(6, 3).Value2 = Now
End Sub

But do you really want a Dictionary?
Using the existing and optimized approach, you'll end up with a dictionary that is keyed by what seems to be an arbitrary index. You may have your reasons, but if you're not going to use the features of a dictionary, you might be better off with an array:

This code runs in just 1.56s.

Sub ScriptDic1()

  'Declare fileName
  Dim filename As String

  Dim FileNum As Integer
  Dim DataLine As String
  'Declare tmp as a string array
  Dim tmp() As String
  'Declare as Scripting.Dictionary to get early-binding benefits
  Dim uniqueKeyCounts As Scripting.Dictionary
  Dim key As String
  Dim count As Long

  Dim lineNumber As Long

  'Create an array that is larger than our needs, we'll resize it when we're done.
  ReDim allRecords(1, 500000) As Variant

  'USe early binding
  Set uniqueKeyCounts = New Scripting.Dictionary

  Sheets("Control").Cells(5, 3).Value2 = Now
  filename = "C:\Temp\test100k.txt"
  FileNum = FreeFile()

  Dim start As Double
  start = Timer
  Open filename For Input As #FileNum
  Do While Not EOF(FileNum)
      Line Input #FileNum, DataLine ' read in data 1 line at a time
      'USe the vbTab constant
      tmp = Split(DataLine, vbTab)
      key = tmp(7) & "-" & tmp(8)
      'Check the unique dictionary - it's going to be smaller
      If uniqueKeyCounts.Exists(key) Then
        'Retrieve the current count once
        Dim currentCount As Long
        currentCount = uniqueKeyCounts.Item(key)
        allRecords(0, lineNumber) = key & ">" & currentCount
        allRecords(1, lineNumber) = tmp
        'Don't remove and re-add, just increment the counter
        uniqueKeyCounts(key) = currentCount + 1
      Else
        uniqueKeyCounts.Add key, 1
        allRecords(0, lineNumber) = key
        allRecords(1, lineNumber) = tmp
      End If
      lineNumber = lineNumber + 1
  Loop

  ReDim Preserve allRecords(1, lineNumber - 1) As Variant
  'Close the file
  Close #FileNum

  Debug.Print Timer - start
  Sheets("Control").Cells(6, 3).Value2 = Now
End Sub


Extensible FizzBuzz in Ruby The title says it all. This is yet another FizzBuzz implementation where you can specify the words you want to use, though it autoselects the divisors as the first n primes other than 2, rather than having you enter them.

I'm looking for tips on variable/function naming and that kind of thing; I couldn't think of good ones when I wrote this. I'd also like any performance improvements that can be put in.

def n_primes(count)
    primes = []
    number = 2
    until primes.length == count
        primes << number if primes.inject(true) { |memo, cur| memo and number % cur != 0 }
        number += 1
    end
    primes
end

puts 'Enter the words you would like to use'
word_array = gets.chomp.split
puts 'Enter the number to count up to'
up_to = gets.chomp.to_i

words = {}
primes = n_primes(word_array.length + 1)[1..-1]
primes.each_with_index { |prime, index| words[prime] = word_array[index] }

1.upto(up_to) do |number|
    to_print = words.reject { |n, _| number % n != 0 }.values
    to_print = [number] if to_print.length == 0
    puts to_print.join ''
end

Note: I extracted n_primes into a function because it means I don't have to use meaningless names like p or num to avoid naming conflicts. If there's a better solution, I'd like to know it.

Sample output:

Enter the words you would like to use
> Fizz Buzz Wolf Foo Bar
Enter the number to count up to
> 100
1
2
Fizz
4
Buzz
Fizz
Wolf
8
Fizz
Buzz
Foo
Fizz
Bar
Wolf
FizzBuzz
16
17
Fizz
19
Buzz
FizzWolf
Foo
23
Fizz
Buzz
Bar
Fizz
Wolf
29
FizzBuzz
31
32
FizzFoo
34
BuzzWolf
Fizz
37
38
FizzBar
Buzz
41
FizzWolf
43
Foo
FizzBuzz
46
47
Fizz
Wolf
Buzz
Fizz
Bar
53
Fizz
BuzzFoo
Wolf
Fizz
58
59
FizzBuzz
61
62
FizzWolf
64
BuzzBar
FizzFoo
67
68
Fizz
BuzzWolf
71
Fizz
73
74
FizzBuzz
76
WolfFoo
FizzBar
79
Buzz
Fizz
82
83
FizzWolf
Buzz
86
Fizz
Foo
89
FizzBuzz
WolfBar
92
Fizz
94
Buzz
Fizz
97
Wolf
FizzFoo
Buzz

(The user input is prefixed with >, though it doesn't show up when you actually run the code)

#n_primes

Ruby has the Prime class as part of its stdlib, so you don't need to roll your own prime number generator.
n_primes(word_array.length + 1)[1..-1]

A more Rubyesque version might use drop(1) instead of the [1..-1] slice.
words = {} + primes.each_with_index

Instead of creating an empty Hash and then adding elements to it, you can use Array#zip to create an array of word/number pairs. You can turn that into a hash with Hash[], if you'd like.
words.reject

Purely from a semantic standpoint, I think select would be more natural to use. I think of it as "finding the words to print" rather than "discarding the words that shouldn't be printed" which sounds like a double negative.

You could also consider using #inject or #each_with_object to collect matches instead of #reject + #values
to_print.length == 0

Use Array#empty? instead.
to_print.join ''

You don't need the argument for Array#join.

I'd do this:

require "prime"

puts "Enter the words you would like to use (separated by spaces)"
words = gets.strip.split

puts "Enter the number to count up to"
count = gets.strip.to_i

pairs = Prime.first(words.count + 1).drop(1).zip(words)

1.upto(count) do |n|
  line = pairs.each_with_object([]) do |(prime, word), memo|
    memo << word if (n % prime).zero?
  end
  puts line.empty? ? n : line.join
end

Or, more in line with yours, the last part could be:

1.upto(count) do |n|
  line = pairs.select { |prime, _| (n % prime).zero? }.map(&:last)
  puts line.empty? ? n : line.join
end


Improvements in repository pattern I am trying to learn the clean architecture by Uncle Bob. I wanted some basics and following this good post I tried to implement it in python with some minor differences.

class ValidationError(Exception):
    pass

class Repository(object):

    repositories = {}

    @classmethod
    def register(cls, type_, repo):
        cls.repositories[type_] = repo

    @classmethod
    def get(cls, type_):
        return cls.repositories[type_]

    @classmethod
    def all(cls):
        return cls.repositories

class UserValidator(object):

    def validate(self, user):
        if not (user.name and user.email):
            raise ValidationError

class UserRepo(object):

    def __init__(self, validator=None):
        self.users = {}
        self.next_id = 1
        self.validator = validator

    def save(self, user):
        self.validator.validate(user)
        user.id = self.next_id
        self.users[self.next_id] = user
        self.next_id += 1
        return user

    def all(self):
        return self.users

    def get(self, id):
        return self.users[id]

    def delete(self, id):
        return self.users.pop(id)

class CompanyRepo(object):

    def __init__(self):
        self.companies = {}
        self.next_id = 1

    def save(self, company):
        company.id = self.next_id
        self.companies[self.next_id] = company
        self.next_id += 1
        return company

    def all(self):
        return self.companies

    def get(self, id):
        return self.companies[id]

    def delete(self, id):
        return self.companies.pop(id)

class BaseEntity(object):

    def __init__(self):
        self.id = None
        self.created_at = None
        self.updated_at = None

class Company(BaseEntity):

    def __init__(self, name, address):
        self.name = name
        self.address = address
        super(Company, self).__init__()

class User(BaseEntity):

    def __init__(self, name, email):
        self.name = name
        self.email = email
        self.company_id = None
        super(User, self).__init__()

    def serialize(self):
        return {'id': self.id, 'name': self.name, 'email': self.email, 'company_id': self.company_id}

    def value(self):
        return self.serialize()

class WorkerAdder(object):

    def __init__(self, name, email):
        self.user_repo = Repository.get('user')
        self.company_repo = Repository.get('company')

        self.company = self.company_repo.save(Company('FooBar', 'Dummy street'))
        self.user = User(name, email)

    def add(self):
        try:
            self.user.company_id = self.company.id
            print self.user_repo.save(self.user).value()
            print self.user_repo.all()
            print self.user_repo.get(1)
            print self.company_repo.all()
        except ValidationError as ve:
            print "Validation error!"

if __name__ == '__main__':

    Repository.register('user', UserRepo(validator=UserValidator()))
    Repository.register('company', CompanyRepo())

    adder = WorkerAdder('Foo', 'foo@example.com')
    adder.add()

O/P from above code:

{'company_id': 1, 'email': 'foo@example.com', 'id': 1, 'name': 'Foo'}
{1: <__main__.User object at 0x7f4b34139d50>}
<__main__.User object at 0x7f4b34139d50>
{1: <__main__.Company object at 0x7f4b34139d10>}

I have basically tried to implement the in memory repository pattern. I would like to hear from others what they think, and why there is so scarcity in the python community regarding the design patterns and different architectures as compared to ruby?

PS: The code above is just for learning purpose hence error handling and error cases are not handled properly.

I'm skeptical of the value of this design pattern, for two reasons:

The Repository class is basically just a Python dictionary.  You never use Repository.all() anyway.
Repository is actually a glorified namespace.  It's a disguised mechanism for you to make a bunch of global variables.

Additionally, I see some puzzling aspects to the code:

Why would UserRepo() make its .validator configurable?  And why would it default to None?
Since UserRepo and CompanyRepo contain nearly identical code, why not consolidate them?  At the least they should have a common base class.  Maybe they could even be handled by one class.
The WorkerAdder object seems contrived.  Why split the work between a WorkerAdder() constructor and an .add() method?  Why is the company name and address hard-coded?

Some minor remarks:

It's customary to chain to the superclass constructor first, not as the last statement in the subclass's constructor.
Serialization means turning an object into a string representation; your User.serialize() returns a dictionary instead.  The string representation needs to  contain enough detail to reconstruct the object exactly; your User.serialize() omits the .created_at and .update_at timestamps.

In summary, Repository is just a dictionary that serves as a namespace for global variables.  There's not much point to it, since symbol tables in Python are also dictionaries.  UserRepo and CompanyRepo do serve a purpose, but you would have to reinvent the wheel (implementing the persistence mechanism, for example) to make them useful.  You might as well use a real database (which could be as simple as sqlite) with an ORM.


Using generic methods for basic crud operations Regarding re-usability, is this OK?  What might go wrong?  What would be a better design?  Performance-related issues and any other comments are welcome.

package sample.library.dao.util;

import java.io.Serializable;
import java.util.List;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.orm.hibernate3.HibernateTemplate;
import org.springframework.transaction.annotation.Transactional;

/**
 * @author mulean
 *
 */
 @Transactional(value = "transactionManager", timeout = 30,
 rollbackFor = java.lang.Exception.class)
 public class DAOUtil {

@Autowired
HibernateTemplate template;

public  <T> T updateData(T t){
    template.update(t);
    return t;
}
public  <T> T delete(T t){
    template.delete(t);
    return t;
}

public  <T> Boolean save(T t){
    boolean success = false;
    try{
        template.saveOrUpdate(t);
        success = true;
    }catch(Exception e){
        success = false;
        return success;
    }
        return success;
}
@SuppressWarnings("unchecked")
public  <T> List<T> listData(String className){
    List<T> items = null;
    items = template.find("from "+className.toUpperCase());
    return items;
}
}

What about having generic abstract dao class which will contain these methods? You will most likely have to create some concrete dao classes anyway to have specific methods.
We are using this approach in our projects (behind JPA facade)

public abstract class AbstractJpaDao<T extends AbstractEntity> implements GenericDao<T> {

    @PersistenceContext
    protected EntityManager entityManager;

    protected final Class<T> entityClass;

    public AbstractJpaDao(Class<T> entityClass) {
        this.entityClass = entityClass;
    }

    @Override
    public T get(Serializable id) {
        Assert.notNull(id, "Entity ID cannot be null");
        return entityManager.find(entityClass, id);
    }

    @Override
    public List<T> findAll() {
        CriteriaBuilder builder = entityManager.getCriteriaBuilder();
        CriteriaQuery<T> query = builder.createQuery(entityClass);
        Root<T> root = query.from(entityClass);
        query.select(root);
        return entityManager.createQuery(query).getResultList();
    }

EDIT
Also do not put @Transactional on your DAO classes but on Services. Service method describes unit of work which should be done whole or rollbacked.
Consider this simple code

public void save() {
 dao1.delete(entity1);
 dao2.delete(entity2);
}

When second dao fails to delete and you have transactional daos not service, the first entity is lost.

And you save method should not return boolean, but let bubble the exception up into the service layer and let service handle the exception.


Algorithm for subdividing html-elements I am trying to write a module that automatically subdivides  div-elements, given some initial parameters.

I've prepared the following jsfiddle.

What I am doing at the moment is:

Setting the width and height of the parent element.
Setting the child elements to float left and assigning percentual width and height to them, based on the specified parameters.

that.createDivisionData = function(paramObj) {

    var
        numOfDivisions = paramObj.divisions,
        widthTop = paramObj.widthTop,
        heightTop = paramObj.heightTop,
        widthBottom = paramObj.widthBottom,
        widthBlock = paramObj.blockWidth,
        heightBlock = paramObj.blockHeight,
        data = [];

    for ( var i = 1; i <= numOfDivisions; i++) {
        var divisionObj = {};
        if (i < 3) {
            divisionObj.widthPerc = i % 2 ? widthTop + '%' : (100 - widthTop) + '%';
            divisionObj.width = i % 2 ? widthTop * widthBlock / 100 : (100 - widthTop) * widthBlock / 100;
            divisionObj.heightPerc = heightTop + '%';
            divisionObj.height = heightBlock * heightTop / 100;
        } else {
            divisionObj.widthPerc =  i % 2 ? widthBottom + '%' : (100 - widthBottom) + '%';
            divisionObj.width =  i % 2 ? widthBottom * widthBlock / 100 : (100 - widthBottom) * widthBlock / 100;
            divisionObj.heightPerc = (100 - heightTop) + '%';
            divisionObj.height = (100 - heightTop) * heightBlock / 100;
        }
        data.push(divisionObj);
    }
    return data;
};

The way I am doing it at the moment actually works. But it feels somehow wrong, dirty, unfinished, you name it. I know this is a subjective question, but maybe someone has an idea. How can I do this in a cleaner way?

A lot of what you are doing is repeated code so you should DRY it out.  For instance, widthPerc is the same code except for the values of widthTop and widthBottom. We can also simply all of our checks by setting them up as vars.  So that could be re-written like this:

var isFirstTwo = !!( i < 3 ); // go ahead and just eval once
var isEven = !!( i % 2 ); // go ahead and just evalonce

//which width value do we want?
var widthDir = ( isFirstTwo ) ? widthTop : widthBottom;
//is it an even number?
widthDir = ( isEven ) ? widthDir : 100 - widthDir;
widthBlock = widthDir * widthBlock / 100;

divisionObj.widthPerc = widthDir +'%';
divisionObj.width = widthBlock;

This reduces a lot of your code.  You could follow a similar technique for height (it's actually easier than width).

var heightTop = ( isEven ) ? heightTop : (100 - heightTop);
var heightBlock =  heightBlock * heightTop / 100
divisionObj.heightPerc = heightTop + '%';
divisionObj.height = heightBlock;

Hope that helps. I have updated the fiddle. Let me know if you have questions or comments.


Get images for PDFs from URLs I Have the below code that gets urls of PDF files, send them to a convert function and returns them as images, is there a way to make this code more readable?
Is there a way to avoid functions chaining?
import PDFtoImages from '../PDFtoImages'

export async function convertPDF(file, category) {
const images= await PDFtoImages(file.data)
return {
   category: category ? [category] : [],
    images
  }
}

export function fetchBlob(url, opts = {}) {
  return fetch(url, opts).then(res => {
    if (!res.ok) return Promise.reject(new Error(res.statusText))
      return res.blob()
    })
}

export async function fetchFilesAsImages(filesUrls) {
  const pdfAsImages= []
  await Promise.all(
    filesUrls.map(async file=> {
      pdfAsImages.push(
        await convertPDF(
          {data: await fetchBlob(file.url)},
          file.category
        )
      )
    })
  )
  return pdfAsImages
}

The main issue I see with this code is in the fetchFilesAsImages() function. You're needlessly building a results array to return at the end of the call fetchFilesAsImages(). Why not just take advantage of the fact that Promise.all() will already return a list of what each promise resolved to. So, you could instead write fetchFilesAsImages() as follows:
export async function fetchFilesAsImages(filesUrls) {
  return await Promise.all(
    filesUrls.map(async ({ url, category }) => {
      const data = await fetchBlob(url);
      return await convertPDF({ data }, category);
    })
  );
}

One other thing I would suggest is to just use async/await syntax everywhere. Your fetchBlob() could look a little cleaner with it:
export async function fetchBlob(url, opts = {}) {
  const res = await fetch(url, opts);
  if (!res.ok) throw new Error(res.statusText);
  return res.blob();
}


Created a DialogBuilder object, but having issues with subscribing/unsubscribing to dialog events var WorkflowDialogBuilder = _.once(function () {
    'use strict';
    var workflowDialog = $('#WorkflowDialog');
    var workflowDialogContent = $('#WorkflowDialogContent');

     var events = {
        onApplyChangesSuccess: 'onApplyChangesSuccess',
        onValidationFailed: 'onValidationFailed',
        onDialogOpen: 'onDialogOpen'
    };

    var dialogConfig = {
        autoOpen: false,
        buttons: {
            OK: function () {
                var form = $(this).find('form');

                if (form.valid()) {
                    var self = this;
                    $.ajax({
                        type: "POST",
                        url: 'Form/Save',
                        dataType: 'json',
                        data: form.serialize(),
                        success: function (data) {
                            $(self).dialog('close');
                            $(self).trigger(events.onApplyChangesSuccess, data);
                        },
                        error: function (error) {
                            console.error(error);
                        }
                    });
                } else {
                    $(this).trigger(events.onValidationFailed);
                }
            },
            Cancel: function () {
                $(this).dialog('close');
            }
        },
        open: function () {
            $(this).trigger(events.onDialogOpen);
        }
    };

    workflowDialog.dialog(dialogConfig);

    return {
        onApplyChangesSuccess: function (event) {
            workflowDialog.on(events.onApplyChangesSuccess, event);
        },
        onDialogOpen: function (event) {
            workflowDialog.on(events.onDialogOpen, event);
        },
        onValidationFailed: function (event) {
            workflowDialog.on(events.onValidationFailed, event);
        },
        buildAddTaskDialog: function () {
            workflowDialogContent.load('NewTaskDetails', function () {
                //Load ViewModel client-side helpers and then when its ready, open the dialog.
                $.getScript('../Scripts/Orders/NewTaskDetailsModel.js', function () {
                    workflowDialog.dialog('open');
                });
            });
        },
        buildAddOrderDialog: function () {
            workflowDialogContent.load('NewOrderDetails', function () {
                //Load ViewModel client-side helpers and then when its ready, open the dialog.
                $.getScript('../Scripts/Orders/NewOrderDetailsModel.js', function () {
                    workflowDialog.dialog('open');
                });
            });
        }
    }
}

So, I've made the above builder. It is responsible for initializing a dialog element, opening it, and responding appropriately when the dialog closes.

Now, I was pretty happy with this implementation until I realized that, given the following scenario, events were subscribing and not unsubscribing.

//User calls WorkflowDialogBuilder.buildAddTaskDialog();
//NewTaskDetailsModel.js is loaded.
//NewTaskDetailsModel.js calls the following:

WorkflowDialogBuilder.onValidationFailed(function () {
    console.log("NewTaskDetailsMode.js onValidationFailed fired.");
});

//AddTaskDialog is closed by the user. NewTaskDetailsModel.js has already been loaded.
//AddTaskDialog continues to be subscribed to DialogBuilder's onValidationFailed.

//User calls WorkflowDialogBuilder.buildAddOrderDialog();
//User works with the dialog and clicks 'OK' to apply validation.
//AddTaskDialog's onValidationFailed event runs because WorkflowDialogBuilder triggers an event.

Now, I can think of one very simple solution to this. I could simply respond to the dialog's onClose event and, at that pont in time, unsubscribe all events.

Question: Does that seem like a proper solution? Or, have I gone down a bad path from the getgo and should rethink my architecture? If so, advice on how to restructure?

I would refactor it into a plugin that takes the content as an option. Then you will have two instances of "dialog controllers" each with its own events.

Have a look at the "plugin with data" example in the jQuery docs.

http://docs.jquery.com/Plugins/Authoring#Data

You'd set it up like

Otherwise I'd switch to $.unbind followed by $.bind in the subscribe functions.


Struggling to refactor code to remove duplication I really need some help removing some duplicated code. I've linked the methods causing me issues. http://pastebin.com/cZJihM4J

As you can see each method does similar things but retrieve different values. i.e one method to insert a string at a certain line. Another to get the the lines index and another to get the contents at a particular line

There are things like:

String text = textArea.getText();
int start = 0;
int count = 0;
String buildNewTextArea = "";

I could make these values global but not sure if this is the best way to approach this.
My brain can't find away around this :(

Thanks in advance

 public String insertCodeBlock(String newBlock) {
    String text = textArea.getText();
    int start = 0;
    int count = 0;
    String buildNewTextArea = "";
    while (start >= 0) {
        int nextLineStart = text.indexOf(NEW_LINE, start + NEW_LINE.length());
        if (nextLineStart == -1) {
            nextLineStart = text.length();
        }
        String[] temp = diffBlocks.get(indexOfLine).split(",");
        int startLine = Integer.valueOf(temp[0]);
        int endLine = (Integer.valueOf(temp[1]) - 1) + startLine;
        if (count == startLine) {
            buildNewTextArea += newBlock;
        } else if (count < startLine || count > (startLine + (endLine - startLine))) {
            buildNewTextArea += text.substring(start, nextLineStart);
        }
        count++;
        start = text.indexOf(NEW_LINE, nextLineStart);
    }
    return buildNewTextArea;
}

private int getLineIndex(int line) {
    String text = textArea.getText();
    int start = 0;
    int count = 0;
    String buildNewTextArea = "";
    int index = 0;
    while (start >= 0) {
        int nextLineStart = text.indexOf(NEW_LINE, start + NEW_LINE.length());
        if (nextLineStart == -1) {
            nextLineStart = text.length();
        }
        if (count == line) {
            return index;
        }
        buildNewTextArea = text.substring(start, nextLineStart);
        index += buildNewTextArea.length();
        count++;
        start = text.indexOf(NEW_LINE, nextLineStart);
    }
    return text.length();
}

public String getLineContent(int startLine, int endLine) {
    String text = textArea.getText();
    int start = 0;
    int count = 0;
    String buildNewTextArea = "";
    while (start >= 0) {
        int nextLineStart = text.indexOf(NEW_LINE, start + NEW_LINE.length());
        if (nextLineStart == -1) {
            nextLineStart = text.length();
        }
        if (count >= startLine && count <= endLine) {
            buildNewTextArea += text.substring(start, nextLineStart);
        }
        count++;
        start = text.indexOf(NEW_LINE, nextLineStart);
    }
    return buildNewTextArea;
}

It seems like each method is iterating over the list of lines and doing something. It is the "doing something" that is unique. Consider having a single method that iterates over the lines and takes an interface to do the "something". For each method, an appropriate interface instance would be passed (possible anonymous inner class) that takes the appropriate action.

Something like this:

private interface ProcessLine{
     void processLine(String line, int index, int charIndex);
     void complete();
}

private ProcessLine process(ProcessLine processor){
  String text = textArea.getText();
  int start = 0;
  int count = 0;
  String buildNewTextArea = "";
  int index = 0;
  while (start >= 0) {
    int nextLineStart = text.indexOf(NEW_LINE, start + NEW_LINE.length());
    if (nextLineStart == -1) {
        nextLineStart = text.length();
    }

    processor.processLine(line, count);
    index += buildNewTextArea.length();
    count++;
    start = text.indexOf(NEW_LINE, nextLineStart);
  }
  return processor;
}

private int getLineIndex(final int line) {
     int result = -1;
     return process(new ProcessLine(){
         public void processLine(String line, int index, int charIndex){
             if (index == line)
                 result = charIndex;
         }
     });
     return result;
}


First program with scraping, lists, string manipulation I wanted to find out which states and cities the USA hockey team was from, but I didn't want to manually count from the roster site here.

I'm really interested to see if someone has a more elegant way to do what I've done (which feels like glue and duct tape) for future purposes. I read about 12 different Stack Overflow questions to get here.

from bs4 import BeautifulSoup
from collections import Counter
import urllib2

url='http://olympics.usahockey.com/page/show/1067902-roster'
page=urllib2.urlopen(url)
soup = BeautifulSoup(page.read())

locations = []
city = []
state = []

counter = 0
tables = soup.findAll("table", { "class" : "dataTable" })
for table in tables:
    rows = table.findAll("tr")
    for row in rows:
        entries = row.findAll("td")
        for entry in entries:
            counter = counter + 1
            if counter == 7:
                locations.append(entry.get_text().encode('ascii'))
        counter = 0

for i in locations:
    splitter = i.split(", ")
    city.append(splitter[0])
    state.append(splitter[1])

print Counter(state)
print Counter(city)

I essentially did a three tier loop for table->tr->td, and then used a counter to grab the 7th column and added it to a list. Then I iterated through the list splitting the first word to one list, and the second word to a second list. Then ran it through Counter to print the cities and states. I get a hunch this could be done a lot simpler, curious for opinions.

You can use enumerate in order not to play with counter.

for counter,entry in enumerate(entries):
    if counter == 6:
        locations.append(entry.get_text().encode('ascii'))

states and cities would probably be a better name for collections of states/cities.
You can use unpacking when splitting the string

For instance :

for i in locations:
   city, state = i.split(", ")
   cities.append(city)
   states.append(state)

You don't need to store locations in a list and then iterate over the locations, you can handle them directly without storing them.

This is pretty much what my code is like at this stage :

#!/usr/bin/python

from bs4 import BeautifulSoup
from collections import Counter
import urllib2

url='http://olympics.usahockey.com/page/show/1067902-roster'
soup = BeautifulSoup(urllib2.urlopen(url).read())

cities = []
states = []

for table in soup.findAll("table", { "class" : "dataTable" }):
    for row in table.findAll("tr"):
        for counter,entry in enumerate(row.findAll("td")):
            if counter == 6:
                city, state = entry.get_text().encode('ascii').split(", ")
                cities.append(city)
                states.append(state)

print Counter(states)
print Counter(cities)

Now, because of the way American cities are named, it might be worth counting the cities keeping their state into account (because Portand and Portland are not quite the same city). Thus, it might be worth storing information about city and state as a tuple.

This is how I'd do it :

#!/usr/bin/python

from bs4 import BeautifulSoup
from collections import Counter
import urllib2

url='http://olympics.usahockey.com/page/show/1067902-roster'
soup = BeautifulSoup(urllib2.urlopen(url).read())

cities = []
for table in soup.findAll("table", { "class" : "dataTable" }):
    for row in table.findAll("tr"):
        for counter,entry in enumerate(row.findAll("td")):
            if counter == 6:
                city, state = entry.get_text().encode('ascii').split(", ")
                cities.append((state,city))

print Counter(cities)
print Counter(state for state,city in cities)

Also, something I have forgotten but might be useful to you is to use defaultdict if all you want is to count elements.

from collections import defaultdict

url='http://olympics.usahockey.com/page/show/1067902-roster'
soup = BeautifulSoup(urllib2.urlopen(url).read())

cities = defaultdict(int)
for table in soup.findAll("table", { "class" : "dataTable" }):
    for row in table.findAll("tr"):
        for counter,entry in enumerate(row.findAll("td")):
            if counter == 6:
                city, state = entry.get_text().encode('ascii').split(", ")
                cities[state,city] += 1


Is this the correct way to create a route to a specific .hbs file? This is my /forums page

2019/2020 Premier League Forum

  View

For the above code you can see the href pointing to a directory to premierLeague.hbs.
This is how my file structure looks.
├── app.js
├── views/
│   ├── index.hbs
│   ├── forums.hbs
│   ├── login.hbs
│   ├── Leagues/
│       ├── EnglishDivision/
│           ├── PremierLeague.hbs
│
├── routes/
│   ├── Pages.js

This is my app.js
// Public dir
var publicDir = path.join(__dirname, './public');
app.use(express.static(publicDir));

// Define routes
app.use('/', require('./routes/pages'));

This is my pages.js
// Forum
router.get('/forums', authController.isLoggedIn, function(req, res) {
  if(req.user) {
    res.render('forums');
  } else {
    res.redirect('login');
  }
});

// English Division //

// Premier League
router.get('/Leagues/EnglishDivision/PremierLeague', authController.isLoggedIn, function(req, res) {
  if(req.user) {
    res.render('./Leagues/EnglishDivision/PremierLeague');
  } else {
    res.redirect('login');
  }
});

Question:
In order for premier league html file to be accessed via the a tag from forums page, have I set the routing correctly?

Somewhere in your code you should be setting the template engine and the base views path, in the likes of
app.set('views', __dirname+'/views/');
app.set('view engine', 'hbs');

(I assume you have that figured out already)
Having said that, you can either reference a view by its location relative to the views setting, therefore:
res.render('Leagues/EnglishDivision/PremierLeague');

(note there's no leading ./ in there)
OR, reference a view by its template file path (without extension). If you do this, the relative path is computed using the base path from which you start your app. Therefore, if your structure looked like:
  /var/www/project
  │
  ├─ packjage.json
  └─ src/
       │
       ├── app.js
       ├── views/
       │   └─── index.hbs
       │   └─── Leagues/
       │       └─── EnglishDivision/
       │           └─── PremierLeague.hbs
       │
       └─── routes/
          └── Pages.js

Moving to /var/www/project and executing node src/app.js means
res.render('./Leagues/EnglishDivision/PremierLeague');

Will look for a template at /var/www/project/Leagues/EnglishDivision/PremierLeague.hbs
Whereas moving to /var/www/project/src and executing node app.js would instead look for /var/www/project/src/Leagues/EnglishDivision/PremierLeague.hbs
If you need to reference the template by its filepath, you should use the absolute path. For example,  __dirname always holds the current folder, regardless of where were you when executing the start script. Which means__dirname called from Pages.js is /var/www/project/src/routes, and therefore
res.render(__dirname+'/../views/Leagues/EnglishDivision/PremierLeague');

Would point to the intended file.


HackerRank “Save Humanity” The "Save Humanity" problem on Hackerrank asks us to:

  ... find all substrings in the patient DNA that either exactly matches the virus DNA, or has at most one mismatch.

  For example: "aa" and "aa" are matching, "ab" and "aa" are matching, while "ab" and "ba" are not.

  Input Format

  The first line contains the number of test cases T. T test cases follow.
  Each case contains two strings P(Patient DNA) and V(Virus DNA) separated by space.

  Output Format

  Output T lines, one corresponding to each test case. For each test case, output a space delimited list of starting indices (0 indexed) of substrings of P which are matching with V according to the condition mentioned above. The indices have to be in an increasing order. If there is no matching output No Match!.

In a nutshell, it requires me to compare two strings and find the beginning index value for all occurrences of the second substring in the first. The catch is that upto one mismatch in the substring is allowed.

I've coded the following solution which seems to work well for the first 3 testcases. The problem is it timed-out after the 3rd test case. I can't seem to optimize it any further than what I've already done unless my logic is somehow overly complicated or I'm not using enough in-built methods to quicken things up.

inp = inp.split('\n')
del inp[0]
for h in inp:
    P,sep,V = str(h).partition(' ')
    upperlimit = len(P) - len(V) + 1
    if P == V:
            print("0")
            continue
    else:
        matches=0
        for i in range(0,upperlimit):
            Psub = P[i:i+len(V)]
            if Psub == V:  # Check if they match exactly
                print(str(i) + " ", end='')
                matches += 1
                continue   # Just proceed with the next iteration of loop if they match
            mismatches = 0
            for j in range(0,len(V)): # If we hit this code then, we're about to see how many characters mismatch in the substring
                if Psub[j] != V[j]:
                    mismatches += 1
                if mismatches>=2: # No point checking the substrings alphabet by alphabet any further, more than 1 mismatch present here.
                    break
            if mismatches<2:
                print(str(i)+" ",end='')
                matches += 1
        if matches==0:      # This is possible ONLY if none of the above conditions were ever met.
            print("No Match!",end='')
    print()

Can someone please tell me why code is slow? Also, let me know if you want me to comment more of my code so it's easier to understand. The link to the problem description should be quite useful in understanding the problem.

Various nitpicks

range(0, x) is better written as range(x)
print(str(i) + " ", end='') is equivalent to print(i, end=' ') but slower
if matches == 0 reads better as if not matches
You compute len(V) several times, just store its value in a variable.
Given that you don't use sep, and that h should already be a string, P,sep,V = str(h).partition(' ') is better written as P, V = h.split()

Special cases aren't special enough to break the rules

Checking for equality between the patient sub-string and the virus is just a sub-case of counting the differences between them (and finding it is 0). Same for comparing the entire patient string and the virus. All are just sub-cases of:

    for i in range(0,upperlimit):
        Psub = P[i:i+len(V)]
        for j in range(0,len(V)): # If we hit this code then, we're about to see how many characters mismatch in the substring
            if Psub[j] != V[j]:
                mismatches += 1
            if mismatches>=2: # No point checking the substrings alphabet by alphabet any further, more than 1 mismatch present here.
                break

Besides, it is to be expected that there will be much more mismatch or fuzzy-matching when checking the substrings than there will be exact matches. So the exact comparison is just slowing you down.

Moreover, you can abuse the fact that booleans are integers and use the sum builtins to compute the number of differences faster:

    for i in range(upperlimit):
        Psub = P[i:i+len(V)]
        mismatches = sum(Psub[j] != V[j] for j in range(len(V)))

Once again, this can be optimized by using zip instead of retrieving letters with their indices in the string:

    for i in range(upperlimit):
        Psub = P[i:i+len(V)]
        mismatches = sum(p != v for p, v in zip(Psub, V))

The advantage of using zip being that the iteration will stop when reaching the end of the shortest string. We can use that to simplify the writting to:

    for i in range(upperlimit):
        mismatches = sum(p != v for p, v in zip(P[i:], V))

Separation of concerns

As it stand, your code does 3 things at once:

iterate over the input data
compute the index of "matching" substrings
print such indexes

You should make functions to separate concerns and make it easier to re-use/test. A basic layout could look like:

def analyze(patient, virus):
    # build indexes where there is a match

def main():
    # iterate over lines in input
    for line in input_string:
        patient, virus = line.split()
        matches = analyze(patient, virus)
        # print output based on `matches`

It makes it more clear to follow what is going on. You then just have to figure out how to return meaningful values from analyze. First thoughts should be to build a list and return it:

def analyze(patient, virus):
    matches = []
    for i in range(len(patient) - len(virus) + 1):
        if sum(p != v for p, v in zip(patient[i:], virus)) < 2:
            matches.append(i)
    return matches

def main():
    # iterate over lines in input
    for line in input_string:
        patient, virus = line.split()
        matches = analyze(patient, virus)
        if not matches:
            print('No match!')
        else:
            print(' '.join(map(str, matches)))

Or you could turn analyze into a generator and convert its computation into a list when calling it. It let you avoid explicitly calling append:

Proposed improvements

def analyze(patient, virus):
    for begin in range(len(patient) - len(virus) + 1):
        if sum(p != v for p, v in zip(patient[begin:], virus)) < 2:
            yield begin

if __name__ == '__main__':
    T = int(input())

    for _ in range(T):
        patient, virus = input().split()
        matches = list(analyze(patient, virus))
        if not matches:
            print('No Match!')
        else:
            print(' '.join(map(str, matches)))

The advantage of using a generator is that you don't even have to convert it to a list, your original prints work as well:

    for _ in range(T):
        patient, virus = input().split()
        matches = 0
        for index in analyze(patient, virus):
            print(index, end=' ')
            matches += 1
        if not matches:
            print('No match!')
        else:
            print()

Or you can convert the matches on the fly:

    for _ in range(T):
        patient, virus = input().split()
        matches = ' '.join(map(str, analyze(patient, virus)))
        if not matches:
            matches = 'No match!'
        print(matches)


Handling combinations of optional parameters for an Angular filter pipe This is a simple implementation of an ng2 filter pipe, which can currently take 2 optional facets: <string>status and <string>description. A "filter pipe" is an angular2 view decorator.

The filter receives a list of objects (values) which I want to filter by some criteria specific to the type of data object passed to it. The types of criteria are known in advance for each type of object.

Currently, I have implemented only one object type, a "maintenance event", which can be filtered by:

a status field, which is a fixed enum of strings, and
a free-text description

The filter is implemented in the view like so:

<div *ngFor="let event of maintenanceEvents | filter:'MAINTENANCE_EVENT':statusFilter:wordFilter">

where statusFilter and wordFilter are populated by a form.

The rest parameter (...filters) was used to allow any future arbitrary number of filters to be passed to the common filter module. So, in future, I could implement a filter like this:

<div *ngFor="let person of people | filter:'PEOPLE':title:permissions:locations:reports">

This is my complete pipe module:

import { Pipe, PipeTransform } from "@angular/core";

@Pipe({ name: 'filter' })
export class FilterPipe implements PipeTransform {

    transform(values: any, type: string, ...filters: Object[]): any {

        switch (type) {
            case 'MAINTENANCE_EVENT':

                const ALL = "Show all";
                const BLANK = "";
                const DESCRIPTION_FILTER = new RegExp(<string>filters[1], 'gi');

                if (filters.length !== 2) {
                    throw new SyntaxError('MAINTENANCE_EVENT requires exactly 2 filter parameters: <string>status and <string>description')
                }

                if (filters[0] === ALL && filters[1] === BLANK) {
                    return values;
                }
                else {
                    return values.filter(value => {
                        if (value.hasOwnProperty('status') && value.hasOwnProperty('part') && value.part.hasOwnProperty('description')) {
                            if (filters[0] === ALL && filters[1] !== BLANK) {
                                return DESCRIPTION_FILTER.test(value['part']['description']);
                            }
                            else if (filters[0] !== ALL && filters[1] === BLANK) {
                                return value.status === filters[0];
                            }
                            else if (filters[0] !== ALL && filters[1] !== BLANK) {
                                return value.status === filters[0] && DESCRIPTION_FILTER.test(value['part']['description']);
                            }
                        }
                        else {
                            console.error("Attempting to filter by properties not present in available <MaintenanceEvent>s");
                        }
                    });
                }

            default:
                // Return everything
                return values;
        }
    }
}

Particularly, I'm interested in an alternative to the if, else if, else if ... else if N chain, since this will become wholly unmaintainable for filters with more complex facets (we plan for as many as 9 in future).

Direction

My approach would be based on the Strategy design pattern. Each of the original if-elseif-elseif-... sections is now becoming an IRule object that represents the rule. This object does two things:

A) It defines whether it may or may not be applied to the given filter input;

B) It defines the application function (logic) itself.

In order to select a proper rule dynamically in run time, we need a registry of the rules which is implemented as a simple array.

Benefits

Implementation of a new "rule" becomes almost trivial: we'll need to create a rule object itself, and add it to a proper place in the list of registry entries, so that it's picked up.

Code

import { Pipe, PipeTransform } from "@angular/core";

const ALL = "Show all";
const BLANK = "";

/**
 * Represents a contract of a single rule.
 */
interface IRule {
    /**
     * A predicate that sayw whether the rule may or may not be applied to a value based on the filter.
     */
    isApplicableTo(filters: Object[]): boolean;

    /**
     * Implementation of rule applied to a value based on filter parameters.
     */
    applyTo(filters: Object[], value: any): boolean;
}

/**
 * The complete list of rules that your system supports.
 * NOTICE that the order of the rule is important.
 * If more than one rule is applicable to a filters object,
 * the very first applicable rule is applied.
 */
const RULES_REGISTRY: IRule[] = [
    new AllMatchingRegexRule(),
    new MatchingStatusRule(),
    new SomeMatchingRegexRule()
];

/**
 * Three sample rules based on your original code.
 */

class AllMatchingRegexRule implements IRule {
    isApplicableTo(filters: Object[]): boolean {
        const safeFilters = getSafeFilters(filters);
        return isAllFilter(safeFilters) && isNotBlankFilter(safeFilters);
    }

    applyTo(filters: Object[], value: any): boolean {
        return descriptionRegexMatched(filters[1], value);
    }
}

class MatchingStatusRule implements IRule {
    isApplicableTo(filters: Object[]): boolean {
        const safeFilters = getSafeFilters(filters);
        return isNotAllFilter(safeFilters) && isBlankFilter(safeFilters);
    }

    applyTo(filters: Object[], value: any): boolean {
        return statusMatches(filters, value);
    }
}

class SomeMatchingRegexRule implements IRule {
    isApplicableTo(filters: Object[]): boolean {
        const safeFilters = getSafeFilters(filters);
        return isNotAllFilter(safeFilters) && isNotBlankFilter(safeFilters);
    }

    applyTo(filters: Object[], value: any): boolean {
        return value.status === filters[0] && descriptionRegexMatched(filters[1], value);;
    }
}

// Reusable working horse lambdas (use in Rules)

const getSafeFilters = (filters: Object[]) => filters && filters.length >= 2 ? filters : new Object[2];

const descriptionRegexMatched = (pattern: any, value: any) => new RegExp(pattern, 'gi').test(value['part']['description']);

const statusMatches = (filters: Object[], value: any) => value.status === filters[0];

const isAllFilter = (filters: Object[]) => filters[0] === ALL;
const isNotAllFilter = (filters: Object[]) => !isAllFilter(filters);

const isBlankFilter = (filters: Object[]) => filters[1] === BLANK;
const isNotBlankFilter = (filters: Object[]) => !isBlankFilter(filters);

Consuming code (that uses the Rule registry)

Now your filter can be implemented like this:

@Pipe({ name: 'filter' })
export class FilterPipe implements PipeTransform {

    transform(values: any, type: string, ...filters: Object[]): any {
        switch (type) {
            case 'MAINTENANCE_EVENT':
                if (filters.length !== 2) {
                    throw new SyntaxError('MAINTENANCE_EVENT requires exactly 2 filter parameters: <string>status and <string>description')
                }

                if (isAllFilter(filters) && isNotBlankFilter(filters)) {
                    return values;
                } else {
                    return values.filter(value => {
                        const ruleToApply = RULES_REGISTRY.find(rule => rule.isApplicableTo(filters));

                        if (!ruleToApply) {
                            throw new Error("Attempting to filter by properties not present in available <MaintenanceEvent>s");
                        }

                        return ruleToApply.applyTo(filters, value);
                    });
                }

            default:
                return values;
        }
    }
}

Disclaimer

I am very new to TypeScript so I may be using wrong idioms. The key in my answer is the design change, rather than implementation details.

P.S.

Notice that the same approach may be applied to type == 'MAINTENANCE_EVENT' 'OTHER_EVENT' .... And, speaking more generally, Strategy design pattern is almost always a working alternative to an if or switch. Of course, to get the benefits of the pattern, we need to write some boilerplate code (IRule + Rule registry), however it's a great trade of for the scenarios where there are a lot of rules, or the rules are frequently added/modified, or the rule selection strategy may change in run time.


Is this file image upload safe? Is this image uploading script safe? I have spent my day researching this and it would be very much appreciated if you could point out any errors or ways to make it safer.

<?php

$max_file_size = 1048576; // expressed in bytes
                          // 10240 = 10 KB
                          // 102400 = 100 KB
                          // 1048576 = 1 MB
                          // 10485760 = 10 MB

$upload_errors = array(
  // http://www.php.net/manual/en/features.file-upload.errors.php
  UPLOAD_ERR_OK         => "No Errors",
  UPLOAD_ERR_INI_SIZE   => "Larger than upload_max_filesize",
  UPLOAD_ERR_FORM_SIZE  => "Larger than form MAX_FILE_SIZE",
  UPLOAD_ERR_PARTIAL    => "Partial upload",
  UPLOAD_ERR_NO_FILE    => "No file",
  UPLOAD_ERR_NO_TMP_DIR => "No temporary directory",
  UPLOAD_ERR_CANT_WRITE => "Can't write to disk",
  UPLOAD_ERR_EXTENSION  => "File upload stopped by extension"
  );

$image_types = array (
  IMAGETYPE_GIF         => "gif",
  IMAGETYPE_JPEG        => "jpg",
  IMAGETYPE_PNG         => "png"
  );

$image_whitelist = array(IMAGETYPE_JPEG, IMAGETYPE_PNG, IMAGETYPE_GIF);

$message = "";
if (isset($_POST['submit'])) {

  // This would store the actual user id
  $userId = 1;

  // store uploaded file's temp path.
  $tmp_file = $_FILES['file_upload']['tmp_name'];

  if($_FILES['file_upload']['error'] > 0){
    $error = $_FILES['file_upload']['error'];
    die($upload_errors[$error]);
  }

  // check to see if file is an image, if not die.
  if(!getimagesize($tmp_file)) {
    die('Please ensure you are uploading an image.');
  }

  // check to see if image is a valid type, if not die.
  if (!in_array(exif_imagetype($tmp_file), $image_whitelist)) {
    die('Please upload a valid image. (.jpg, .png or .gif)');
  }

  $target_file = md5($userId . date('U')) . "." . $image_types[exif_imagetype($tmp_file)];
  $upload_dir = "uploaded";

  // check to see if file already exsits if it does die.
  if (file_exists($upload_dir."/".$target_file)) {
    die('file already exists');
  }

  //move_uploaded file will return false if $tmp_file is not a valid upload file
  // or if it cannot be moved for any other reason
  if (move_uploaded_file($tmp_file, $upload_dir."/".$target_file)) {
    $message = "File uploaded successfully.";
  } else {
    die("move_uploaded_file() error.");
  }
}
?>

<html>
<head>
  <title>File Upload</title>
</head>
<body>
  <?php if(!empty($message)) { echo "<p>{$message}</p>"; } ?>
  <form action="fileupload.php" enctype="multipart/form-data" method="POST">
    <input type="hidden" name="MAX_FILE_SIZE" value="<?php echo $max_file_size; ?>">
    <input type="file" name="file_upload">
    <input type="submit" name="submit" value="Upload">
  </form>
</body>
</html>

Many Thanks

It is pretty solid for saving images on a single web server. Some optional/extra things you could do are:

Host the images on a CDN. That way if there was any PHP or JS injected in, it would not be able to access any cookies or directories on the web server you use to serve content.
Your code would block out PHP files in disguise from being uploaded because of the getimagesize() and the exif_imagetype(). You can add this bit to an htaccess file in the images directory as well to disable PHP files from being executed on their own. This wouldn't stop the file from being executed though if your code automatically included the "image" in the code somewhere.

<IfModule mod_php5.c>
php_flag engine off
</IfModule>


Reusing select statement I am checking if there are any rows in a table with some conditions.  If there are more than 1 row, it should filter again with some more conditions.

DECLARE @Matches AS int
DECLARE @Result AS int

SET @Matches =
(
    Select COUNT(*)
        FROM Houses
            INNER JOIN Houses_Access ON Houses.ID = Houses_Access.DoorKey
            FULL OUTER Join UserAccess ON  Houses_Access.GroupKey  = UserAccess.GroupKey
        WHERE Houses.ID IS NOT NULL AND Houses.ID = @DoorKey AND UserAccess.UserKey = @UserKey
)

IF(@Matches < 1)
 SET @Result = 0
ELSE IF(@Matches > 0)
    BEGIN
        SET @Matches =
        (
        Select COUNT(*)
        FROM Houses
            INNER JOIN Houses_Access ON Houses.ID = Houses_Access.DoorKey
            FULL OUTER Join UserAccess ON  Houses_Access.GroupKey  = UserAccess.GroupKey
        WHERE Houses.ID IS NOT NULL AND Houses.ID = @DoorKey AND UserAccess.UserKey = @UserKey
            AND Houses_Access.GroupKey != Houses.OwnerGroupKey
        )
        IF(@Matches < 1)
            SET @Result = 0
        ELSE
            SET @Result = 1
    END

RETURN(@Result)

END

Create a view vw_HousesAccess  which will contain following query and then

        Create View vw_HousesAccess
        as
        Select * --TODO specify columns where
        FROM Houses
            INNER JOIN Houses_Access ON Houses.ID = Houses_Access.DoorKey
            FULL OUTER Join UserAccess ON  Houses_Access.GroupKey  = UserAccess.GroupKey

        ---then in main query you can use like this.
        IF Exists(Select 1 from vw_HousesAccess  where  WHERE ID IS NOT NULL AND ID = @DoorKey AND UserKey = @UserKey)
         ---your logic


How to Unit Test a Thread which has an infinite loop? I wanted to be sure the thread being tested would complete its job by the Assert statement of the unit test, but I couldn't be sure bc of the way I designed the threads. I designed them to run forever in the background, performing a task - logging, assembling, or disassembling - at regular timed intervals. What I ended up doing was sleeping the test thread for a bit to ensure the thread being tested had enough time to complete its task. I have no idea whether this is the "right" way to handle this situation, and I'd appreciate any feedback!

@Test
// Tests whether thread removes completed tasks from in_progress and returns
// processors to available
public void testOne() {

    t.complete = true;
    d.start();

    try {
        Thread.sleep(20);
    } catch (InterruptedException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }

    Assert.assertEquals(cs.size(), 3);
    Assert.assertEquals(inProgressJobs.size(), 0);

}

Full code: https://github.com/dandelion1234/Map-Reduce

When you want to test things that happened asynchronously, one solution is to give them some time to happen as you did. A bit more practical approach is to check if the assert condition is met in a loop and sleep a bit between the checks. if the condition is met no need to continue waiting. eventually, it needs to stop after a few tries or you have an infinite loop in your test.

Usually, you need to use this approach in E2E / Integration tests and not unit tests.


Transposing characters in a string I want to create a function that takes a string and returns an array of versions of it in which always one pair of characters is transposed (swapped).

I came up with this:

fn transposes(s: &str) -> Vec<String> {
    let mut vec = vec![];

    let b = s.as_bytes();

    for i in 1..b.len() {
        let mut transpose = String::with_capacity(s.len());
        transpose.push_str(&s[..i - 1]);
        transpose.push(s.as_bytes()[i] as char);
        transpose.push(s.as_bytes()[i - 1] as char);
        transpose.push_str(&s[i + 1..]);

        vec.push(transpose);
    }

    vec
}

fn main() {
    let s = "maxi".to_owned();
    println!("{:?}", transposes(&s));
}

Output is:

["amxi", "mxai", "maix"]

That result is what I want, it does:

First swap m and a
Then swap a and x
Then swap i and x

I don't find the code super readable because of the string slicing going on. In C it would be a mere "pointer swap", but my Rust version looks a bit more involved.

Is there a better way to do what I want?

It is a fair assumption that this will only work correctly for ASCII strings.

If it's just ASCII, treat it as a byte array all the way until the end. Vec has a swap method and you can reconstruct a String from the bytes.
Use map and collect instead of allocating a Vec and manually pushing.
There's no need to allocate a String to pass to &str.

fn transposes(s: &str) -> Vec<String> {
    let bytes = s.as_bytes();

    (1..bytes.len()).map(|i| {
        let mut transpose = bytes.to_owned();
        transpose.swap(i - 1, i);
        String::from_utf8(transpose).expect("Invalid UTF-8")
    }).collect()
}

fn main() {
    println!("{:?}", transposes("maxi"));
}

Even better, if your data is ASCII, encode that in the type system:

extern crate ascii;

use ascii::{AsciiStr, AsciiString};

fn transposes(s: &AsciiStr) -> Vec<AsciiString> {
    (1..s.len()).map(|i| {
        let mut transpose = s.to_owned();
        transpose.as_mut_slice().swap(i - 1, i);
        transpose
    }).collect()
}

fn main() {
    let s = AsciiStr::from_ascii("maxi").expect("Not ASCII");
    println!("{:?}", transposes(s));
}

Note that the transposes function can no longer fail due to invalid strings.


Find maximum distance between elements of an array I have written a program in scala which actually does the below

  For every number between the maximum and minimum value of the array calculate the distance for each and every element in the array and find the element with maximum distance.

I want to master functional programming in scala. Trying to understand how I can use functional programming to this.

/*
* Author:
* Given an integer array find the distance find the index that ranges between
* minimum and maximum value of Array and that has the maximum distance from all
* the array elements.
*/

package com.scala.practice.basic

object CycleProblem {
  def main(args: Array[String]){
    val input:Array[Int]=Array(-1000,1000,-30,70,80,20,100,110,-120,-180,120)
    val minVal=input.reduceLeft(_ min _)
    val maxVal=input.reduceLeft(_ max _)
    var maxDistance=0
    var index=minVal-1;
    for(i<-minVal to maxVal){
      var distance =0
      for(j<-0 to input.length-1){
        distance=distance+Math.abs(i-input(j))
      }
      if(distance > maxDistance)
        {
        maxDistance=distance
        index=i
        }
    }
    println("Park the bicycle in this location:"+ index+ " the distance(maximum when compared to to other indices) from all other parked location is: "+maxDistance + "");

  }
}

...and welcome to Scala :)

I will show changes in several steps:

a) Almost automatic changes without changing any part of the logic:

object CycleProblem extends App {
  val input: Array[Int] = Array(-1000, 1000, -30, 70, 80, 20, 100, 110, -120, -180, 120)
  val minVal = input.min
  val maxVal = input.max
  var maxDistance = 0
  var index = minVal - 1
  for (i <- minVal to maxVal) {
    var distance = 0
    for (j <- input.indices) {
      distance = distance + Math.abs(i - input(j))
    }
    if (distance > maxDistance) {
      maxDistance = distance
      index = i
    }
  }
  println(s"Park the bicycle in this location: $index. The distance(maximum when compared to to other indices) from all other parked location is: $maxDistance")
}

Replaced the main method with extends App
Removed a couple of semicolons ;
Used string interpolation
Used plain min and max methods instead of reduce
Used .indices instead of 0 to input.length-1. You can also use the prettier 0 until input.length.

b) Important enough to get its own step: Separate your algorithm into a (stateless) function! Stateless means that the same input always produces the same output (what you intuitively expect from a function).

object CycleProblem extends App {
  val input: Array[Int] = Array(-1000, 1000, -30, 70, 80, 20, 100, 110, -120, -180, 120)

  val (index, maxDistance) = findValueInRangeWithMaxTotalDistance(input)

  println(s"Park the bicycle in this location: $index. The distance(maximum when compared to to other indices) from all other parked location is: $maxDistance")

  private def findValueInRangeWithMaxTotalDistance(input: Array[Int]): (Int, Int) = {
    val minVal = input.min
    val maxVal = input.max
    var maxDistance = 0
    var index = minVal - 1
    for (i <- minVal to maxVal) {
      var distance = 0
      for (j <- input.indices) {
        distance = distance + Math.abs(i - input(j))
      }
      if (distance > maxDistance) {
        maxDistance = distance
        index = i
      }
    }
    (index, maxDistance)
  }
}

c) Now come the real changes: The algorithm I use is exactly the same, but expressed via functional methods on collections. You can also uncomment my println comment for check the logic of your algorithm.

object CycleProblem extends App {
  val input: Array[Int] = Array(-1000, 1000, -30, 70, 80, 20, 100, 110, -120, -180, 120)
  val (maxValue, maxDistance) = findValueInRangeWithMaxTotalDistance(input)
  println(s"Park the bicycle in this location: $maxValue. The distance(maximum when compared to to others) from all other parked location is: $maxDistance")

  def findValueInRangeWithMaxTotalDistance(input: Array[Int]): (Int, Int) = {
    def distance(a: Int, b: Int) = Math.abs(a-b)
    val valueRange = input.min to input.max
    val valuesInRangeWithTotalDistance = valueRange.map { numInRange =>
      val sumOfAllDistances = input.map(itemInInput => distance(numInRange, itemInInput)).sum
      //println(s"$numInRange, $sumOfAllDistances")
      (numInRange, sumOfAllDistances)
    }
    //valuesInRangeWithTotalDistance.foreach(println)
    valuesInRangeWithTotalDistance.maxBy(_._2)//_2 is the second element, i.e. the distance
  }
}

d) When you get accustomed to functional expressions, you can collapse the code into a few neat lines of code:

object CycleProblem extends App {
  val input: Array[Int] = Array(-1000, 1000, -30, 70, 80, 20, 100, 110, -120, -180, 120)
  val (maxValue, maxDistance) = findValueInRangeWithMaxTotalDistance(input)
  println(s"Park the bicycle in this location: $maxValue. The distance(maximum when compared to to others) from all other parked location is: $maxDistance")

  def findValueInRangeWithMaxTotalDistance(input: Array[Int]): (Int, Int) = {
    def distance(a: Int, b: Int) = Math.abs(a-b)
    val valuesInRangeWithTotalDistance = (input.min to input.max).map(numInRange => (numInRange, input.map(distance(numInRange, _)).sum))
    valuesInRangeWithTotalDistance.maxBy(_._2)
  }
}

Note my very long function name: I am reducing noisy lines of code that obscure the essence of the code (don't take it personally, it is inherent in java-like code) and add long names to describe the logic of the encapsulated algorithm.

e) To highlight my point about clarity, note that the return type of our function is (Int,Int). It is not clear what is what, lets fix it by adding a case class to hold our result:

object CycleProblem extends App {
  val input: Array[Int] = Array(-1000, 1000, -30, 70, 80, 20, 100, 110, -120, -180, 120)
  val result = findValueInRangeWithMaxTotalDistance(input)
  println(s"Park the bicycle in this location: ${result.value}. The distance(maximum when compared to to others) from all other parked location is: ${result.distance}")

  case class ValueAndDistance(value: Int, distance: Int)
  def findValueInRangeWithMaxTotalDistance(input: Array[Int]): ValueAndDistance = {
    def distance(a: Int, b: Int) = Math.abs(a-b)
    val valuesInRangeWithTotalDistance = (input.min to input.max).map(numInRange => ValueAndDistance(numInRange, input.map(distance(numInRange, _)).sum))
    valuesInRangeWithTotalDistance.maxBy(_.distance)
  }
}

Again, adding code is good, as long as it makes the logic more clear and fool-proof to your colleagues or the future-you.


Count resources in each category I have the following python script like:

query_parser.py

#!/usr/bin/env python

def count_resources_per_category(resource_names):
    APPS_MAP = {
        'billing': ["paypal", "checkout", "cart", "order-service"],
        'browse': ["content", "localization", "pricing", "search",],
        'some_category': ["random-1", "random-2"],
    }
    results_map = {}
    for key, _ in APPS_MAP.items():
        results_map[key] = 0
    results_map['other'] = 0
    # now we have:
    #   results_map = {
    #     'checkout': 0,
    #     'browse': 0,
    #     'MCM': 0,
    #     'other': 0
    #   }
    for resource_name in resource_names:
        FOUND_IT = False
        for key, app_names in APPS_MAP.items():
            if FOUND_IT:
                break
            for app_name in app_names:
                if app_name in resource_name:
                    results_map[key] += 1
                    FOUND_IT = True
                    break
        if not FOUND_IT:
            results_map['other'] += 1

    return results_map

def determine_if_resource_count_issue(results_map):
    RESULT_FOUND = False
    for key, count in results_map.items():
        if count:
            if RESULT_FOUND:
                print("The script isn't gonna work as planned")
                return True
            RESULT_FOUND = True
    print("All good :)")
    return False

#--------------------------------------------------------------------

""" TESTS """

# test_count_resources_per_category():
test_input = [
    'some-prefix-pricing', # browse
    'some-prefix2-cart', # billing
    'content', # browse
    'some-random-thing-not-found' # other
]
expectation = {
    'browse': 2,
    'billing': 1,
    'some_category': 0,
    'other': 1
}
result = count_resources_per_category(test_input)
assert result == expectation

#--------------------------------------------------------------------

# test_determine_resource_count_issue_passes():
test_input = {
    'browse': 234234234,
    'billing': 0,
    'some_category': 0,
    'other': 0
}
assert False == determine_if_resource_count_issue(test_input)

#--------------------------------------------------------------------

# test_determine_resource_count_issue_fails():
test_input = {
    'browse': 2,
    'billing': 1,
    'some_category': 0,
    'other': 1
}
assert True == determine_if_resource_count_issue(test_input)

#--------------------------------------------------------------------

It has some tests at the bottom explaining how it works, but the goal is to ensure each result from running a query is only for 1 category at a time.

I was wondering what the clean, pythonish way to handle these nested loops would be without the FOUND_IT and RESULT_FOUND flags. Any improvements welcome.

You don't need to initialize the keys of results_map to 0. You can do it on the fly. Here is a generic example:

counts = {}
for value in values:
    if value in counts:
        counts[value] += 1
    else:
        counts[value] = 1

Even better, use collections.Counter instead. Generic example:

from collections import Counter

counts = Counter()
for value in values:
    counts[value] += 1

Refactor the method to check if app_name is in resource_name. Put APPS_MAP outside the functions so it can be accessed as a global variable.

def get_category(resource_name):
    for key, app_names in APPS_MAP.items()
        for app_name in app_names:
            if app_name in resource_name:
                return key
    return 'other'

Now you can rewrite the loop in count_resources_per_category() like this:

from collections import Counter

results_map = Counter()
for resource_name in resource_names:
    results_map[get_category(resource_name)] += 1
return results_map

It seems like determine_if_resource_count_issue() is checking to make sure only one field is non-zero. Here's a quick one-liner replacement. Put the print() statements outside this function.

def determine_if_resource_count_issue(results_map):
    return [v != 0 for v in results_map.values()].count(True) == 1

get_category() as implemented above is computationally expensive. Depending on your use case, you may want to consider using regex.
Don't pad your question with lorem ipsum lol.


Small socket based packet Library I'm currently writing a python script which needs some simple networking. After some playing around with sockets, this emerged, but as this is my first foray into sockets I'd really like a second opinion.

Edit: I know my script is not PEP8 compliant, as this is just an experiment. I posted it here to get Feedback about my usage of Sockets, and if there are any pitfalls or corner cases I overlooked.

Package Format:

  datalength+1 \0 data \0

import socket

sep: bytes = b'\0'

minpacketsize: int = 10

def sendpacket(sock: socket.socket, data: bytes) -> None:
    send: bytes = str(len(data) + 1).encode('utf-8') + sep + data + sep

    if len(send) < minpacketsize:
        raise ValueError("Packet too small!")

    sock.sendall(send)

def recivepacket(sock: socket.socket) -> bytes:
    datalen = 0
    header: bytearray = bytearray()
    while not datalen:
        header += sock.recv(1)
        if header.endswith(sep):
            try:
                datalen = int(header[:-1].decode('utf-8'))
            except UnicodeDecodeError or ValueError as e:
                raise ValueError("Error while decoding header!") from e

        if len(header) == 10:
            raise ValueError("Could not find Header!")

    rawdata: bytes = sock.recv(datalen)

    if rawdata.endswith(sep):
        return rawdata[:-1]
    else:
        raise ValueError("Could not find Packet end!")

Docstrings

You should include a docstring at the beginning of every method, class, and module you write. This will allow documentation to identify what your code is supposed to do. It also helps external text editors, such as VSCode, to display what types of parameters your functions accept, and a short description of your method.

Error or Error

This line

except UnicodeDecodeError or ValueError as e:

should be this

except (UnicodeDecodeError, ValueError) as e:

Your current line of code raises this warning from pylint:

  Exception to catch is the result of a binary "or" operation

If you need to handle multiple errors, you should group them into a tuple.

Method Names

According to PEP-8 Function Name Conventions, your method names sendpacket and recivepacket should really be send_packet and receive_packet.

Unnecessary else

This section of code

if rawdata.endswith(sep):
    return rawdata[:-1]
else:
    raise ValueError("Could not find Packet end!")

should be this

if rawdata.endswith(sep):
    return rawdata[:-1]
raise ValueError("Could not find Packet end!")

Since you are returning a value if the condition is satisfied, the ValueError line will not run. And if the condition is not satisfied, it will simply move to the next line. Therefore, the else is unnecessary and should be removed.

Updated Code

"""
Description of this module goes here
"""

import socket

sep: bytes = b'\0'

minpacketsize: int = 10

def send_packet(sock: socket.socket, data: bytes) -> None:
    """

    Description of this method goes here

    :param sock -> socket.socket: Socket to send the data over\n
    :param data -> bytes: Data to send over the socket

    :return: None
    """
    send: bytes = str(len(data) + 1).encode('utf-8') + sep + data + sep

    if len(send) < minpacketsize:
        raise ValueError("Packet too small!")

    sock.sendall(send)

def receive_packet(sock: socket.socket) -> bytes:
    """

    Description of this method goes here

    :param sock -> socket.socket: Socket to receive the data over

    :return: bytes
    """
    datalen = 0
    header: bytearray = bytearray()
    while not datalen:
        header += sock.recv(1)
        if header.endswith(sep):
            try:
                datalen = int(header[:-1].decode('utf-8'))
            except (UnicodeDecodeError, ValueError) as e:
                raise ValueError("Error while decoding header!") from e

        if len(header) == 10:
            raise ValueError("Could not find Header!")

    rawdata: bytes = sock.recv(datalen)

    if rawdata.endswith(sep):
        return rawdata[:-1]
    raise ValueError("Could not find Packet end!")


Follow-up: Randomizing Civilization 5 team choice This is a follow-up on my previous question about a VBA macro for randomizing a draw from a table. It can be found here:
Randomizing Civilization 5 team choice

The code has been improved with the help I got earlier and I now seek even further improvements.

Public Enum CivilizationTableColumns
        CivilizationName = 1
        CivilizationLeader = 2
End Enum
Public Enum TextColumns
        PlayerTextColumn = 1
        CivTextColumn = 2
End Enum

Public Sub Draw()
Dim ws As Worksheet
Set ws = ThisWorkbook.ActiveSheet

Dim resultsRange As Range
Set resultsRange = GetResultsRange(ws)
resultsRange.ClearContents

Dim CivilizationsTable As ListObject
Set CivilizationsTable = Worksheets("Civilizations").ListObjects("tblCivilizations")

Dim randCiv As String

For noOfPlayers = 1 To GetPlayerNum(ws)

    resultsRange.Cells(GetPlayerNameRow(ws, noOfPlayers), PlayerTextColumn).Value = GetPlayerName(noOfPlayers)

    For noOfOptions = 1 To GetOptionsNum(ws)

        Dim endOfRange As Boolean
        endOfRange = False

        While Not endOfRange

            randCiv = GetCivilizationCaption(GetRandomNum(CivilizationsTable), CivilizationsTable)
            For Z = 1 To resultsRange.Rows.Count
                If resultsRange.Cells(Z, CivTextColumn) = randCiv Then
                    Exit For
                End If
                If Z = resultsRange.Rows.Count Then
                    endOfRange = True
                End If
            Next Z
        Wend

        resultsRange.Cells(GetCivNameRow(ws, noOfPlayers, noOfOptions), CivTextColumn).Value = randCiv
    Next noOfOptions
Next noOfPlayers
End Sub

Private Function GetRandomNum(ByVal CivilizationsTable As ListObject) As Integer
    GetRandomNum = CInt(Int((CivilizationsTable.Range.Rows.Count - 1) * Rnd())) + 1
End Function

Private Function GetCivilizationCaption(ByVal index As Long, ByVal CivilizationsTable As ListObject)
    Set Row = CivilizationsTable.ListRows(index)
    civName = Row.Range(ColumnIndex:=CivilizationName)
    civLeader = Row.Range(ColumnIndex:=CivilizationLeader)
    GetCivilizationCaption = civLeader & " (" & civName & ")"
End Function

Private Function GetPlayerNum(ByVal ws As Worksheet)
    GetPlayerNum = ws.Cells(3, 3).Value
End Function

Private Function GetOptionsNum(ByVal ws As Worksheet)
    GetOptionsNum = ws.Cells(3, 7).Value
End Function

Private Function GetPlayerName(ByVal noOfPlayers As Integer) As String
    GetPlayerName = "Player " & noOfPlayers
End Function

Private Function GetCivNameRow(ByVal ws As Worksheet, ByVal noOfPlayers As Integer, ByVal noOfOptions As Integer) As Integer
    GetCivNameRow = (GetOptionsNum(ws) + 2) * (noOfPlayers - 1) + (noOfOptions + 3)
End Function

Private Function GetPlayerNameRow(ByVal ws As Worksheet, ByVal noOfPlayers As Integer) As Integer
    GetPlayerNameRow = 3 + (GetOptionsNum(ws) + 2) * (noOfPlayers - 1)
End Function

Private Function GetResultsRange(ByVal ws As Worksheet) As Range
    Set GetResultsRange = ws.Range("K1:L50")
End Function

Your indentation is generally fine, except the Draw procedure isn't indented as it should; everything in a Sub block should be indented 1 level, so that this:

Next noOfPlayers
End Sub

Becomes that:

    Next noOfPlayers
End Sub

I like that you have lots of small functions - that's very good!

However Draw could be further broken down; I'm removing the fluff to illustrate:

Public Sub Draw()
    For
        For
            While
                For
                    '...
                Next
            Wend
        Next
    Next
End Sub

That's 4 layers of nested loops! I've seen worse arrow code, but I'd extract a method out of the body of the 2nd For loop, to make it look like this:

Public Sub Draw()
    For
        For
            ExtractedMethodCall
        Next
    Next
End Sub

Other than that, looks great!


Get int array from database Cursor I find this code quite ugly, but in fact, I can't find a finer way of achieving the goal.

private static final String TABLE_NAME = "table_name";
private static final String[] allNeededColumns = {"col_id","col_one","col_two"};

public int[] getItemIds(int category_id) {
    ArrayList<Integer> ids = new ArrayList<Integer>();
    Cursor c = getDatabase().query(TABLE_NAME, allNeededColumns, CATEGORY_ID + "=" + category_id, null, null, null, null);
    c.moveToFirst();
    while(!c.isAfterLast()){
        ids.add(Integer.valueOf(c.getInt(2)));
        c.moveToNext();
    }
    return convertIntegers(ids);
}

public static int[] convertIntegers(ArrayList<Integer> integers)
{
    int[] ret = new int[integers.size()];
    Iterator<Integer> iterator = integers.iterator();
    for (int i = 0; i < ret.length; i++)
    {
        ret[i] = iterator.next().intValue();
    }
    return ret;
}

The purpose is to get out of table int array, where IDs are item IDs from category bycategory_id`.

The problem is, I need some kind of expandable collection to add DB result ID to, and return primitives array, and I find it prettier than creating new int[] variable for each iteration on Cursor.

Can you think of a prettier solution?

If you were going to use the above you could still utilise Java's autoboxing feature, but there is a shorter form as hinted at by Darth Beleg.

Untested:

int[] ret = new int[c.getCount()];
if (c.moveToFirst())
    for (int i = 0; i < c.getCount();) {
        ids[i++] = c.getInt(2);
        c.moveToNext();
    }
}

Although it is not pertinent to your case it is worth remembering that

ret[i] = iterator.next().intValue();

could throw a NPE.


Hi there! I've recently been getting into java and was wondering how I can clean up or condense the code for this problem I've been working on The program asks the user to enter values for items bought, the price of each item, the GST rate, the QST rate. The program calculates the subtotal based on these inputs. For every invalid input entered by the user the program will prompt the user to re-enter that value until it is a valid input. Items bought must be between 1 and 10. The price of each item must be between 1 and 1000. The GST must be between 0 and 14. The QST must be between 0 and 17.

Im wondering if there's a way I can make this code more efficient by including more methods or anything else. Thanks!

import java.util.Scanner;
        public class Taxes{
        public static void main(String[] args){
        double subtotal = 0;
        int errors = 0;
        Scanner scan = new Scanner(System.in);
        System.out.println("Please enter the number of items bought (1-10): ");
        int num_items = scan.nextInt();
        while (num_items < 1 || 10 < num_items){
            errors += 1;
            System.out.println("Please enter the number of items bought (1-10): ");
            num_items = scan.nextInt();
        }

        for (int i = 1; i <= num_items; i++){
            Scanner scn = new Scanner(System.in);
            System.out.println("Please enter the price of item " + i);
            Double item_cost = scn.nextDouble();
            while (item_cost < 1 || 1000 < item_cost){
                errors += 1;
                System.out.println("Please enter the price of item " + i);
                item_cost = scn.nextDouble();
            }
            subtotal += item_cost;
        }
        System.out.println("Please enter the tax rate of GST in %: ");
        double gRate = scan.nextDouble();
        while (gRate < 0 || 14 < gRate){
            errors += 1;
            System.out.println("Please enter the tax rate of GST in %: ");
            gRate = scan.nextDouble();
        }
        System.out.println("Please enter the tax rate of QST in %: ");
        double qRate = scan.nextDouble();

        while (qRate < 0 || 17 < qRate){
            errors += 1;
            System.out.println("Please enter the tax rate of QST in %: ");
            qRate = scan.nextDouble();
        }
        calculate(subtotal, gRate, qRate, errors);
}

    public static void calculate(double subtotal, double gRate, double qRate, int errors)
        {
            double gst = subtotal * (gRate/100);
            double qst = (subtotal + gst) * (qRate/100);
            double total = subtotal + gst + qst;
            System.out.println("GST: " + gst);
            System.out.println("QST: " + qst);
            System.out.println("Subtotal: "  + total);
            System.out.println("You entered " + errors + " invalid inputs");
        }

    }

Style

Please use proper indentation. It is generally considered good practice to indent the code inside a method. This will make your code more legible.
The use of curly brackets is inconsistent (have a look at the calculate-method. In Java, the most common use of curly brackets looks like this:

public static void calculate(...) {
    //Commands here
}

For variable names the lowerCamelCase is used. So num_items becomes numItems and item_cost becomes itemCost.

Input

You have been thinking about how to stop the user from making invalid inputs. You are using:

while (numItems < 1 || 10 < numItems) {
            errors += 1;
            System.out.println("Please enter the number of items bought (1-10): ");
            numItems = scan.nextInt();
        }

This will stop the user entering other numbers than intended. The problem is that your program crashes, when the user enters not even a number (for example "hi"). This can be solved with the following code (use import java.util.InputMismatchException;):

while(true) {
        try {
            numItems = scn.nextInt();
            if(numItems < 1 || 10 < numItems) {
                throw new InputMismatchException()
            }
            break;
        }
        catch(InputMismatchException e) {
            System.out.println("Invalid Input!");
            scn.nextLine();
        }
}

Also, you have initialized more than one scanner in your program. That's not necessary. Just initialize one in the beginning and use it for the whole time.

Other

I removed the variable errors, because it is not useful to tell the user how many errors he/she made. But that's just a personal opinion.
It's not necessary to restrict the number of items and their prices, but I have left it in the code so you can see how to restrict it.

All in all your code could look like this:

import java.util.Scanner;
import java.util.InputMismatchException;

public class Taxes {

    public static void main(String[] args) {

        double subtotal = 0;
        Scanner scan = new Scanner(System.in);
        System.out.println("Please enter the number of items bought (1-10): ");
        int numItems;
        while(true) {
            try {
                numItems = scan.nextInt();
                if(numItems < 1 || 10 < numItems) {
                    throw new InputMismatchException();
                }
                break;
            }
            catch(InputMismatchException e) {
                System.out.println("Invalid Input!");
                scan.nextLine();
            }
        }

        for (int i = 1; i <= numItems; i++) {
            System.out.println("Please enter the price of item " + i);
            Double itemCost;
            while(true) {
                try {
                    itemCost = scan.nextDouble();
                    if(itemCost < 1 || 1000 < itemCost) {
                        throw new InputMismatchException();
                    }
                    break;
                }
                catch(InputMismatchException e) {
                    System.out.println("Invalid Input!");
                    scan.nextLine();
                }
            }
            subtotal += itemCost;
        }
        System.out.println("Please enter the tax rate of GST in %: ");
        double gRate;
        while(true) {
            try {
                gRate = scan.nextDouble();
                if(gRate < 0 || 14 < gRate) {
                    throw new InputMismatchException();
                }
                break;
            }
            catch(InputMismatchException e) {
                System.out.println("Invalid Input!");
                scan.nextLine();
            }
        }
        System.out.println("Please enter the tax rate of QST in %: ");
        double qRate;

        while(true) {
            try {
                qRate = scan.nextDouble();
                if(qRate < 0 || 17 < qRate) {
                    throw new InputMismatchException();
                }
                break;
            }
            catch(InputMismatchException e) {
                System.out.println("Invalid Input!");
                scan.nextLine();
            }
        }

        calculate(subtotal, gRate, qRate);
    }

    public static void calculate(double subtotal, double gRate, double qRate) {
        double gst = subtotal * (gRate/100);
        double qst = (subtotal + gst) * (qRate/100);
        double total = subtotal + gst + qst;
        System.out.println("GST: " + gst);
        System.out.println("QST: " + qst);
        System.out.println("Subtotal: "  + total);
    }
}

I will leave it to you to comment the code properly.


Need Feedback on JUnit test comparing two timestamps I am writng a JUnit test case for the below methods :
public final class LoggerService implements ILoggerService {

  private Logger logger;

  private <T> LoggerService(T type){
    logger = LoggerFactory.getLogger((Class<?>) type);
  }

  public static <T> LoggerService getLoggerService(T type){
    return new LoggerService(type);
  }

  @Override
  public Long startTimeFrame() {
    return new Date().getTime();
  }

  @Override
  public Long stopTimeFrame(Long startTimeFrame) {
    return new Date().getTime() - startTimeFrame;
  }

  }

}

The simple JUnit test is :

@Test
public void testStartTimeFrame(){
   LoggerService loggerService = LoggerService.getLoggerService(LoggerServiceTest.class);
   assertEquals(new Date().getTime(), loggerService.startTimeFrame());
}

Is this an efficient test case ? Is there any chance that this test could fail sometimes ?
Please suggest how I can improve this

The problem here is the call to the new operator inside your code under test (cut). So the way to go is to inject this dependency into your cut. Of cause the (failed) implementation of the Java Singelton Pattern is a problem too.
By looking at the API of the Date class we find that it is mutable (which raised lots of discussions when it was introduced). This gives us the opportunity to define a member variable of type Date and reuse is in all the methods of the cut. Of cause in each method  Date.setTime(milliseconds) needs to be called where we pass in System.currentTimeMillis().
Of cause this is a new requirement to our cut that needs its own UnnitTest methods.
 class InjectDateTest {

    static class LoggerService {

        private Date date;

        public LoggerService(Date date) {
            this.date = date;
        }

        public Long startTimeFrame() {
            date.setTime(System.currentTimeMillis());
            return date.getTime();
        }

    }

    private LoggerService loggerService;
    private Date loggerDate = new Date();

    @BeforeEach
    void setUp() throws Exception {
        loggerService = new LoggerService(loggerDate);
    }

    @Test
    public void shouldSetTimeAfterCall() throws InterruptedException {
        long creationTime = loggerDate.getTime();
        Thread.sleep(200, 0);
        loggerService.startTimeFrame();
        assertTrue(creationTime < loggerDate.getTime(),
                String.format("time in date object %s < %s", creationTime, loggerDate.getTime())
                );
    }

    @Test
    public void testStartTimeFrame() {
        Long startTimeFrame = loggerService.startTimeFrame();
        assertEquals(loggerDate.getTime(), startTimeFrame);
    }

}


Add headers to colums and rows in Python - 2D array (list of lists) I have a 2D Python array (list of lists). I treat this as a 'table'. Now I want to replace the first row and the top row with a header row and column. The row and the header are exactly the same.
I try this using list comprehension. It works out for the 'row' part, but the column part is not very Pythonic yet.
# The header that i want to add
headers = ['foo', 'bar', 'baz', 'other']
l = len(headers) + 1  # The final matrix is one element bigger

# square matrix of random size, filled with data
array = [['xxx' for i in range(l)] for j in range(l)]

# The concise one - add headers to top row
array[0] = ['Title'] + [category for category in headers]

# The ugly one - add headers for the rows
for i in range(l):
    array[i][0] = array[0][i]

The final output should look like this (which it does):
[['Title', 'foo', 'bar', 'baz', 'other'],
 ['foo', 'xxx', 'xxx', 'xxx', 'xxx'],
 ['bar', 'xxx', 'xxx', 'xxx', 'xxx'],
 ['baz', 'xxx', 'xxx', 'xxx', 'xxx'],
 ['other', 'xxx', 'xxx', 'xxx', 'xxx']]

I'm just not so happy with the 'for' loop. How can this be done more Pythonic?

Maybe this can help (if you don't want to use numpy):
headers = ['foo', 'bar', 'baz', 'other']
l = len(headers)

arr = [["xxx" for i in range(l)] for j in range(l)]

# adding top row
arr = [headers] + arr

# adding first column
headers_mod = ['Title'] + headers
new_arr = [[headers_mod[i]]+arr[i] for i in range(l+1)]

for i in new_arr:
  print(*i)

gives you the output as:
Title foo bar baz other
foo xxx xxx xxx xxx
bar xxx xxx xxx xxx
baz xxx xxx xxx xxx
other xxx xxx xxx xxx

Otherwise, when dealing with array manipulations in python try going with numpy, pandas, as they provide better operations like by giving option for axis, transpose, etc.


CreateView set foreign key from URL parameter I want to be able to create a lorum, using a ipsum_id (ForeignKey) that has been supplied in the URL. Using Django's class based views.

Given the following URL:

url(r'^lorem/create/ipsum/(?P<ipsum_id>\d+)/$', LoremCreateView.as_view(), name='create'),

I have the following view:

class LoremCreateView(CreateView):
    """ Allow a user to create a Lorem. """
    model = Lorem
    ...

    def get_context_data(self, **kwargs):
        """ Get context variables for rendering the template. """
        ipsum = get_object_or_404(Ipsum, pk=self.kwargs['ipsum_id'])
        kwargs['ipsum'] = ipsum
        return super().get_context_data(**kwargs)

    def form_valid(self, form):
        """ Save the form instance. """
        ipsum = get_object_or_404(Ipsum, pk=self.kwargs['ipsum_id'])
        form.instance.ipsum = ipsum
        return super().form_valid(form)

This works fine, however I don't like the fact I call get_object_or_404 twice.

Is there a better way of doing this?

It might be possible to override an appropriate method and set self.ipsum - not sure I like doing this either.

Also is get_object_or_404 appropriate here?

There's no need to add the ipsum instance to the template context unless it's used in the template. Adding things to self is the cleanest way.

Using get_object_or_404 is appropriate if we require the ipsum instance to exist in order to save a lorem.

You should end up with something like this:

# urls.py

from django.urls import path

urlpatterns = [
    path('lorem/create/ipsum/<int:ipsum_id>/', LoremCreateView.as_view(), name='create'),
]

# views.py

class LoremCreateView(CreateView):
    model = Lorem

    def dispatch(self, request, *args, **kwargs):
        """
        Overridden so we can make sure the `Ipsum` instance exists
        before going any further.
        """
        self.ipsum = get_object_or_404(Ipsum, pk=kwargs['ipsum_id'])
        return super().dispatch(request, *args, **kwargs)

    def form_valid(self, form):
        """
        Overridden to add the ipsum relation to the `Lorem` instance.
        """
        form.instance.ipsum = self.ipsum
        return super().form_valid(form)