Printing a list to a file then reassembling said list This is a GCSE practice problem, to practice for the actual coursework. Computing's my passion and I'd like to impress the examiners (note: any tips would be appreciated)

Essentially, this is simply splitting by lines into an array, printing said array into a file, then reassembling it by looping through one list by indexing the other.

uniqueword =[]
for i in range(0,len(sentencelist)):
    if not sentencelist[i] in uniqueword:#<--- creates list of unique words
        uniqueword.append(sentencelist[i])
    else:
        i+=1
print(uniqueword)
positionlist = []
for i in range(0,len(sentencelist)):
    positionlist.append(uniqueword.index(sentencelist[i]))#<-- finds the index of the unique words in the sentence and appends them to positionlist
print(positionlist)
wordvals = []
end=False
im = 0
while True:
    try:
        wordvals.append(uniqueword[im])
        im+=1
    except:
        break
print(wordvals)
text_file = open("sentencemanip.txt", "w")#prints by line on new lines
for item in positionlist:
  text_file.write("%s\n" % item)
text_file.close()
text_file = open("sentencemanip1.txt", "w")
for item in wordvals:
  text_file.write("%s\n" % item)#whee
text_file.close()
print('the sentence has been printed to the text files sentencemanip.txt and sentencemanip1.txt')

import sys
sentencelist = []
def splitsent(text):

    # split the text
    words = text.split()

    # for each word in the line:
    for word in words:
        sentencelist.append(word)

and

    import sys
    pl = []
    wv= []
    with open('sentencemanip.txt') as f:
        pl = f.read().splitlines()
    f.close()
    with open('sentencemanip1.txt') as f:
    wv=f.read().splitlines()
    f.close()
    print(pl)
    print(wv)
    for i in pl: print(wv[int(i)],'',end = '')#<- here

The question is, is there a better/more efficient/faster way to accomplish this task? Am I making any elementary errors in variable assignment? Would while or for loops be better?

Any help is appreciated on my continuing quest to become not awful at python!

Forewords

Let's break down the problem once more:

you have a text and want to extract individual words out of it;
with these words, you want to build a list of unique ones.
you replace each word by its position in the list of uniques;
and write these positions and the list of uniques into two files.

After that, an other part can read both files and rebuild the original text.

Now let's get back to the code and put everything into a function to organize things a bit and allow for better reusability and testing. One function for the text to files: def split_sentence(text, filename_indexes, filename_uniques): and one for the files to text: def build_sentence(filename_indexes, filename_uniques):. You can see that by giving meaningful names to parameters, we can get a better grasp at what they are used for.

Text to file

Let's iterate over each part of the algorithm and see what can be improved. Remember, we are defining:

def split_sentence(text, filename_indexes, filename_uniques):

Improving

Extract individual words from a text

sentencelist = []

# split the text
words = text.split()

# for each word in the line:
for word in words:
    sentencelist.append(word)

Each "thing" that you can place after in in a for loop (such as words here) is an iterable. And building a list by appending elements from an iterable into a for loop is a waste of time: the list constructor already does the exact same thing. So, at the very least, you should do words = text.split(); sentencelist = list(words).

But here, words is not only an iterable, it is also already a list. So you don't need to copy it over. The only thing you need is:

    sentence_list = text.split()

Build a list of unique words

uniqueword =[]
for i in range(0,len(sentencelist)):
    if not sentencelist[i] in uniqueword:#<--- creates list of unique words
        uniqueword.append(sentencelist[i])
    else:
        i+=1

So many things done wrong here:

The else: i+=1 part is useless as i, being the iteration variable, is overwritten at each loop iteration. And since you perform no extra operation with it after incrementing it, you can drop these two lines.
range(0, len(sentencelist)): as 0 is the default start of a range, it is common practice to not specify it.
for i in range(len(sentencelist)): ... sentencelist[i] ...: this is a code smell. Unless you have a truly good reason to do that, it is faster, more Pythonic and more readable to iterate over the elements directly rather than using their indices. So use for element in sentencelist: ... element ... instead. If you really need the index, you can still use enumerate.
uniqueword = []; for element in sentencelist: if ...: uniqueword.append(element): again this is unefficient and rather un-Pythonic. Use a list-comprehension instead. Except you can't here because you need the partially built list to check for existence of the current element. But:

All in all, we could discuss about ways to improve this for loop, but that would be missing the point: you don't need it. There is a data structure in Python that is meant for holding unique elements: set. Whenever you try to insert an elemnt in a set, if the element is already present then nothing happens. Since you will need a list for its indexes latter on, you will need to convert it back:

    unique_words = list(set(sentence_list))

Compute the position of each word in the list of uniques

positionlist = []
for i in range(0,len(sentencelist)):
    positionlist.append(uniqueword.index(sentencelist[i]))#<-- finds the index of the unique words in the sentence and appends them to positionlist

I already talked about it a bit above, but you should convert this loop into a list-comprehension; iterating over the elements rather than the indices:

    positions = [unique_words.index(word) for word in sentence_list]

You then have this code:

wordvals = []
end=False
im = 0
while True:
    try:
        wordvals.append(uniqueword[im])
        im+=1
    except:
        break

That I don't understand. You are copying uniqueword into wordvals. For one, this can be done using wordvals = list(uniqueword). For two, this is useless as you already have the list (uniqueword) and you don't modify any of these two lists. So just stick to using uniqueword and avoid unnecessary computation.

Write to files

First off, you should get into the habit of using with when you use open. You do it in your second snippet but not in the first. Second, closeing the file is done automaticaly by with so you don't need to do it manually aftewards.

The "%s\n" % item syntax is the old-style formatting syntax which is superseeded by the format function and the str.format method of strings. And the upcoming "f-strings" in Python 3.6 will make it even more easy to use. So write '{}\n'.format(item) instead for now and f'{item}\n' in Python 3.6. This has the added advantage that, if item is a tuple, your code won't crash.

But, in Python 3 (which I guess you are using, judging by your print calls), you can use the print function to write into files and use its full potential of features: automatic conversion of anything into a string and newlines at the end of each call:

    with open(filename_indexes, 'w') as f:
        for position in positions:
            print(position, file=f)
    with open(filename_uniques, 'w') as f:
        for word in unique_words:
            print(word, file=f)

Improving (again)

Now that we reduced the visual clutter of the function down to 9 lines, let's focus on the complexity of the algorithm.

Splitting the text and getting the unique words out of it can't really be reduced. But when you build the list of indices, you perform an \$O(n)\$ lookup into unique_words for each word in sentence_list: you end up with an overall complexity of \$O(n^2)\$. You can do better.

You don't really need the indices of the words. All you need is a unique value associated to each unique word. So you will need a data structure to hold such association and a way to generate enough unique values. You already know how to generate n unique values: range(n). And to associate one of them to each word, you will need a dictionary. Also the zip function will help in the process of making pairs:

unique_words = set(sentence_list)
lookup_table = dict(zip(unique_words, range(len(unique_words))))

That way, you can replace unique_words.index(word) which is \$O(n)\$ by lookup_table[word] which is \$O(1)\$. Unfortunately, this means that you will have to sort the words in lookup_table by their associated value if you want to be able to reconstruct the text properly. Luckily, sorted, the standard function to sort elements of a container, can help us doing so. It accepts a key parameter which is a function that will be called with each value that should return its "index" in the resulting sorted sequence. Here we’ll make use of the get method on dictionaries that return the element associated to the given value. In the case of lookup table, it will return the unique number associated to each word:

for word in sorted(lookup_table, key=lookup_table.get):
    print(word, file=f)

The whole code for the text to file transformation can thus become:

def split_sentence(text, filename_indexes, filename_uniques):
    words = text.split()
    uniques = set(words)
    lookup_table = dict(zip(uniques, range(len(uniques))))
    positions = [lookup_table[word] for word in words]

    with open(filename_indexes, 'w') as f:
        for position in positions:
            print(position, file=f)
    with open(filename_uniques, 'w') as f:
        for word in sorted(lookup_table, key=lookup_table.get):
            print(word, file=f)

File to text transformation

First, the line wv=f.read().splitlines() is underindented and cannot possibly work as the file will be closed by the time you try to read it.

Second, f.read().splitlines() is neat as it removes newlines characters at the end of each line but it has the disadvantage of copying the file in memory twice: once when reading it, and once when building the list of lines. You can achieve pretty much the same using a list-comprehension and half the amount of memory. List-comprehensions also have the advantage that you can perform any operation while building the list, such as converting a string into a number:

def build_sentence(filename_indexes, filename_uniques):
    with open(filename_indexes) as f:
        positions = [int(line) for line in f]
    with open(filename_uniques) as f:
        words = [line.strip() for line in f]

    for index in positions:
        print(words[index], '', end='')

But you can go further and transform the number into a word as well; granted that you already have your list of words:

def build_sentence(filename_indexes, filename_uniques):
    with open(filename_uniques) as f:
        words = [line.strip() for line in f]
    with open(filename_indexes) as f:
        sentence = [words[int(line)] for line in f]

    for word in sentence:
        print(word, '', end='')

Lastly, you shouldn't compute a value and print it in the same function: return the value, if the caller want it printed, it will print it. But if the responsibility of your functions is to compute and return a value only, it is much more reusable than if they also tried to print it.

So you will need to find a way to build a string out of each word in sentence by inserting a space between each of them. You could try to do it in a loop, starting with an empty list and concatenating words and spaces. But again, that would be terribly inneficient and completely un-Pythonic. Instead use str.join(iterable) that will do the exact same thing: concatenate every elements in iterable and inserting str between each. Here you would need ' '.join(sentence).

But you can do even better by not creating the sentence list in the first place and let join combine the words and the spaces as the lines are read from the file:

def build_sentence(filename_indexes, filename_uniques):
    with open(filename_uniques) as f:
        words = [line.strip() for line in f]
    with open(filename_indexes) as f:
        return ' '.join(words[int(line)] for line in f)


Password checker in Python Using Python 2.7.12 I wrote a simple little script, psk_validate.py, that prompts the user for a potential password and checks if it has upper and lower-case characters, numbers, special characters, and that it has a length of at least 8 characters.

From what I understand one could use the regex library to write this much more efficiently, however, I have yet to learn about regex.

The program seems to work just fine, and with a program this small I think that not using regex is also just fine.

I'd like any and all feedback about this program. In particular, I'd like to know if a program written this simply could be used in real-world applications. I'd also like to know if there are any logical errors and/or bugs in the program.

from sys import exit

def check_upper(input):
    uppers = 0
    upper_list = "A B C D E F G H I J K L M N O P Q R S T U V W X Y Z".split()
    for char in input:
        if char in upper_list:
            uppers += 1
    if uppers > 0:
        return True
    else:
        return False

def check_lower(input):
    lowers = 0
    lower_list = "a b c d e f g h i j k l m n o p q r s t u v w x y z".split()
    for char in input:
        if char in lower_list:
            lowers += 1
    if lowers > 0:
        return True
    else:
        return False

def check_number(input):
    numbers = 0
    number_list = "1 2 3 4 5 6 7 8 9 0".split()
    for char in input:
        if char in number_list:
            numbers += 1
    if numbers > 0:
        return True
    else:
        return False

def check_special(input):
    specials = 0
    special_list = "! @ $ % ^ & * ( ) _ - + = { } [ ] | \ , . > < / ? ~ ` \" ' : ;".split()
    for char in input:
        if char in special_list:
            specials += 1
    if specials > 0:
        return True
    else:
        return False

def check_len(input):
    if len(input) >= 8:
        return True
    else:
        return False

def validate_password(input):
    check_dict = {
        'upper': check_upper(input),
        'lower': check_lower(input),
        'number': check_number(input),
        'special': check_special(input),
        'len' : check_len(input)
    }
    if check_upper(input) & check_lower(input) & check_number(input) & check_special(input) & check_len(input):
        return True
    else:
        print "Invalid password! Review below and change your password accordingly!"
        print
        if check_dict['upper'] == False:
            print "Password needs at least one upper-case character."
        if check_dict['lower'] == False:
            print "Password needs at least one lower-case character."
        if check_dict['number'] == False:
            print "Password needs at least one number."
        if check_dict['special'] == False:
            print "Password needs at least one special character."
        if check_dict['len'] == False:
            print "Password needs to be at least 8 characters in length."
        print

while True:
    password = raw_input("Enter desired password: ")
    print
    if validate_password(password):
        print "Password meets all requirements and may be used."
        print
        print "Exiting program..."
        print
        exit(0)

Concept

Obligatory XKCD comic, before I begin:

Enforcing password strength by requiring human-unfriendly characters is no longer considered good practice.  Nevertheless, I'll review the code as you have written it.

"Obvious" simplifications

Any code with the pattern if bool_expr: return True; else: return False should be written simply as return bool_expr.
Strings are directly iterable; there is no need to convert them into a list first, using .split().  In other words, the code would work the same if you just wrote:

upper_list = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"

Better yet, you could just use string.ascii_uppercase.
The uppers += 1 counting loop could be written more expressively using the sum() built-in function.  Actually, in this case, since you only care whether uppers > 0, you could just use the any() function.

With those changes, your check_upper() function becomes a one-liner:

def contains_upper(s):
    return any(c in ascii_uppercase for c in s)

I've renamed check_upper() to contains_upper() to make it clear that the function returns True or False.  Also, avoid using variable names, like input, that coincide with names of built-in functions: it could cause trouble if you ever want to use input().

Code duplication

Most of your check_something() functions are identical.  You should generalize, instead of duplicating the code.

from string import ascii_uppercase, ascii_lowercase, digits

def contains(required_chars, s):
    return any(c in required_chars for c in s)

def contains_upper(s):
    return contains(ascii_uppercase, s)

def contains_lower(s):
    return contains(ascii_lowercase, s)

def contains_digit(s):
    return contains(digits, s)

def contains_special(s):
    return contains(r"""!@$%^&*()_-+={}[]|\,.></?~`"':;""", s)

def long_enough(s):
    return len(s) >= 8

Note that I've used a raw long string to help deal with the need for backslashes in the punctuation string.

validate_password()

The check_dict isn't doing anything for you.  You'd be no worse off with five boolean variables. You are also calling each validation function twice.

The & (binary bitwise AND) operator is not quite appropriate here.  The and (boolean AND) operator would be more appropriate. Even though the results appear identical, the execution differs: the logical and allows short-circuit evaluation.

Personally, I'd write it this way, gathering up a list of all of the failure messages:

def validate_password(password):
    VALIDATIONS = (
        (contains_upper, 'Password needs at least one upper-case character.'),
        (contains_lower, 'Password needs at least one lower-case character.'),
        (contains_digit, 'Password needs at least one number.'),
        (contains_special, 'Password needs at least one special character.'),
        (long_enough, 'Password needs to be at least 8 characters in length.'),
    )
    failures = [
        msg for validator, msg in VALIDATIONS if not validator(password)
    ]
    if not failures:
        return True
    else:
        print("Invalid password! Review below and change your password accordingly!\n")
        for msg in failures:
            print(msg)
        print('')
        return False

If the function returns True in one place, then it would be good practice to return False instead of None in the other branch, for consistency.

Free-floating code

It is customary to put if __name__ == '__main__': around the statements in the module that are not inside a function.  That way, you could incorporate the functions into another program by doing import psk_validate without actually running this program.

Calling sys.exit(0) is rarely desirable or necessary, if you structure the code properly.  Here, all you needed was a break.

if __name__ == '__main__':
    while True:
        password = raw_input("Enter desired password: ")
        print()
        if validate_password(password):
            print("Password meets all requirements and may be used.\n")
            print("Exiting program...\n")
            break


Increase view counter with each page view My question is similar to before, but now the code has changed completely. I would like to understand if this code is vulnerable to mysql injection.

    <?php

// Define our querystring variables
$option = (array_key_exists('option', $_POST) ? $_POST['option'] : false);
$view   = (array_key_exists('view', $_POST) ?  $_POST['view'] : false);
$id     = (array_key_exists('id', $_POST) ?  intval($_POST['id']) : false);

// Check wheter our variables are correct in order to proceed
if ($option == 'com_content' && $view == 'video' && $id > 0) {

    // Connect to the database
    include_once("../configuration.php");
    $cg = new JConfig;
    $con = mysqli_connect($cg->host,$cg->user,$cg->password,$cg->db);
    if (mysqli_connect_errno()) {
        die('n/a');
    }

    // Add new hit: Update the `times_viewed` field corresponding to specific video id
    $query = "UPDATE " . $cg->dbprefix . "hdflv_upload
                        SET `times_viewed` = `times_viewed` + 1
                        WHERE `id` LIKE " . $id . ";";

    if (mysqli_query($con, $query) === true) {

    // Get the updated `times_viewed` of video id
        $query  = "SELECT `times_viewed`
                             FROM " . $cg->dbprefix . "hdflv_upload
                             WHERE `id` LIKE " . $id . ";";
        $result = mysqli_fetch_assoc(mysqli_query($con, $query));
    }

    // close the connection to the database
    mysqli_close($con);
    $addone = $result['times_viewed'];
    echo $addone;
}
?>

No, this code is not vulnerable to SQL injection.  The use of intval() and if (… && $id > 0) guarantee that $id is a positive integer, so the UPDATE and SELECT queries cannot be malicious.

It is very weird that you use the LIKE operator, though.  Is the id column not an INTEGER?  I would expect that that works by converting every id into a string, then performing pattern matching.  That would be slow because

Converting every id in the hdflv_upload table would be extra work.
The database cannot locate the row with the right id using an index; it has to examine every id.
Pattern matching is more complicated than a simple integer equality check.

Note that in this version, you have fixed a race condition that existed in the previous version.  In the previous version, it is possible to under-count if two pages simultaneously try to update the count, and the sequence ends up being SELECT, SELECT, UPDATE, UPDATE.  This code is better because UPDATE hdflv_upload SET times_viewed = times_viewed + 1 WHERE … should be immune to race conditions.


iOS tableView with two special rows of headers Using swift for iOS, here is my cellforRow method

func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
    switch indexPath.section {
    case 0: // Cover Image
        guard let cell = tableView.dequeueReusableCell(withIdentifier: cellIdentifierCover) as? AddBlogCoverTableViewCell else { return UITableViewCell() }
        cell.layer.backgroundColor = UIColor.clear.cgColor
        cell.selectionStyle = .none
        cell.indexPath = indexPath
        cell.delegate = self
        cell.titleTextField.delegate = self
        cell.coverImageView.image = coverImage
        cell.titleTextField.placeholder = "Title"
        cell.titleTextField?.text = articleName ?? ""
        cell.setBorder(of: 0.7, ofColor: UIColor.lightGray.cgColor)

        return cell

    case 1: // rest story
        guard let cell = tableView.dequeueReusableCell(withIdentifier: cellIdentifier) as? AddBlogTableViewCell else { return UITableViewCell() }
        cell.layer.backgroundColor = UIColor.clear.cgColor
        cell.selectionStyle = .none
        cell.indexPath = indexPath
        cell.delegate = self
        cell.captionTextView.delegate = self
        cell.captionTextView.tag = indexPath.row
        cell.tag = indexPath.section
        cell.media = cards?[indexPath.row].media
        cell.captionTextView.text = cards?[indexPath.row].caption ?? ""
        showLocationSuggestions(at: indexPath, in: cell)
        cell.poi = cards?[indexPath.row].poi
        return cell
    default:
        return UITableViewCell()
    }
    return UITableViewCell()
}

How should I avoid repeating the code that I have to provide to the cell in all cases of switch?

Defensive programming (never force unwrap, no forced cast, ...) is good
and important to handle "runtime problems" gracefully: Unexpected user
input, failed network connections, I/O errors, and many more.

But programming errors are a different category. Example 1: If

tableView.dequeueReusableCell(withIdentifier: cellIdentifierCover)

fails, or does not return a AddBlogCoverTableViewCell, then you did
not configure the table view correctly in the Storyboard. Such cases
should be detected during development. Therefore it is better to abort
with a runtime error instead of "hiding" the problem and returning
a plain UITableViewCell. This is a valid use-case for a forced cast:

 let cell = tableView.dequeueReusableCell(withIdentifier: cellIdentifierCover) as! AddBlogCoverTableViewCell

or better:

let cell = tableView.dequeueReusableCell(withIdentifier: cellIdentifierCover, for: indexPath) as! AddBlogCoverTableViewCell

Example 2: Your table view has 2 sections, so indexPath.section must be 0 or 1, every other value would be a programming error.
Again, fail early instead of hiding the problem:

switch indexPath.section {
case 0:
    // do something ...
case 1:
    // do something ...
default:
    fatalError("Unexpected section \(indexPath.section)")
}

Note also that your final

return UITableViewCell()

is never be executed.

Now for possible simplifications. Both cell classes have some properties
in common, these can be defined in a common superclass:

class AddBlogCommonCell : UITableViewCell {
    var indexPath: IndexPath!
    var delegate: UITableViewController!
    // ...
}

class AddBlogCoverTableViewCell : AddBlogCommonCell {
    // ...
}

class AddBlogTableViewCell : AddBlogCommonCell {
    // ...
}

Then you can configure the specific properties inside the switch
statement, and the shared properties outside the switch statement,
thereby avoiding the code repetition:

override func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {

    let cell: AddBlogCommonCell
    switch indexPath.section {
    case 0:
        let coverCell = tableView.dequeueReusableCell(withIdentifier: cellIdentifierCover) as! AddBlogCoverTableViewCell
        coverCell.titleTextField.delegate = self
        coverCell.coverImageView.image = coverImage
        // ...
        cell = coverCell
    case 1:
        let storyCell = tableView.dequeueReusableCell(withIdentifier: cellIdentifier) as! AddBlogTableViewCell
        storyCell.captionTextView.delegate = self
        storyCell.captionTextView.tag = indexPath.row
        // ...
        cell = storyCell
    default:
        fatalError("Unexpected section \(indexPath.section)")
    }

    // AddBlogCommonCell properties:
    cell.delegate = self
    cell.indexPath = indexPath
    // ...

    // UITableViewCell properties:
    cell.layer.backgroundColor = UIColor.clear.cgColor
    cell.selectionStyle = .none
    // ...

    return cell
}


Four-function calculator with roots and powers I have some code for a calculator, and I was wondering what I could look at to make the code smaller, easier to read or better in any way. I'm particularly looking at my global variable x. Is there any way to change the code to completely remove that? Someone mentioned a getTwoInputs() function, but with multiple different questions (powers() input and addition() input) I assume it would take up the same space.

def Addition():
    print('Addition: What are your numbers?')
    a = float(input('First Number:'))
    b = float(input('Second Number:'))
    print('Your Answer is:', a + b)

def Subtraction():
    print('Subtraction: What are your numbers?')
    c = float(input('First Number:'))
    d = float(input('Second Number:'))
    print('Your Answer is:', c - d)

def Multiplication():
    print('Multiplication: What are your numbers?')
    e = float(input('First Number:'))
    f = float(input('Second Number:'))
    print('Your Answer is:', e * f)

def Division():
    print('Division: What are your numbers?')
    g = float(input('First Number:'))
    h = float(input('Second Number:'))
    print('Your Answer is:', g / h)

def Roots():
    print('Roots: What do you want to Root?')
    i = float(input('Number:'))
    print('Roots: By what root?')
    j = float(input('Number:'))
    k = ( 1 / j )
    print('Your Answer is:',i**k)

def Powers():
    print('Powers: What number would you like to multiply by itself?')
    l = float(input('Number:'))
    print('Powers: How many times would you like to multiply by itself?')
    m = float(input('Number:'))
    print('Your Answer is:',l**m)

x = 'test'

def Question():
        print('What would you like to do?')
        x = input('(Add, Subtract, Divide, Multiply, Powers, Roots or Quit)')
        if x.lower().startswith('a'):
            x = 'test123'
            print(Addition())
            x = 'test'
        elif x.lower().startswith('d'):
            x = 'test123'
            print(Division())
            x = 'test'
        elif x.lower().startswith('m'):
            x = 'test123'
            print(Multiplication())
            x = 'test'
        elif x.lower().startswith('s'):
            x = 'test123'
            print(Subtraction())
            x = 'test'
        elif x.lower().startswith('r'):
            x = 'test123'
            print(Roots())
            x = 'test'
        elif x.lower().startswith('p'):
            x = 'test123'
            print(Powers())
            x = 'test'
        elif x.lower().startswith('e'):
            x = 'test'
            print(exit())
        elif x.lower().startswith('q'):
            x = 'test'
            print(exit())
        else:
            print("""
Please type the first letter of what you want to do.
""")
            print(Question())

while x == 'test':
    Question()

I am not sure it is very clear to you what your different functions are returning as you seem to be trying to print their return value even though they do not return anything special.

You do not need to change your variable names in each function without special reason. a and b are decent names in your case, you don't need to use c and d later and then e and f as there is no way to get confused anyway.

The way you use your x variable is pretty cryptic to me. I guess this is just some testing/debugging but it would help if you could submit a clean version of your code for reviewing.

Most of what happens in your function is (pretty much) the same from one to another. A good thing to do is to try to avoid repetition whenever you can and to abstract common behaviors. The quick and dirty way I've used to avoid such a repetition was to notice that you ask the user for 2 numbers anyway so it doesn't really matter which operator we will use them for. My minimalist solution stores things in a dictionary (you could improve the content of the dictionary if you wanted to have more things changing from one operator to another such as user prompts).

#!/usr/bin/python
import operator

operations={
        'a':('Addition',      operator.add),
        's':('Substraction',  operator.sub),
        'm':('Multiplication',operator.mul),
        'd':('Division',      operator.truediv),
        'p':('Power',         operator.pow),
        'r':('Root',          lambda x, y:x**(1/y)),
}

while True:
        # TODO : Next line could be generated from the list if it keeps growing
        x = input('(Add, Subtract, Divide, Multiply, Powers, Roots or Quit)').lower()
        if x in operations:
                operation_name,operator=operations[x]
                print(operation_name)
                a = float(input('First Number:'))
                b = float(input('Second Number:'))
                print("Your answer is ",operator(a,b))
        elif x.startswith('q'):
                break
        else:
                print("Please type the first letter of what you want to do.")


Non-Blocking MySQL queries in Java I wrote a non-blocking MySQL interface for java (wrapped around the standard mysql java connector).

I think the code to use it looks a bit ugly however...

db.rawQuery(queries.someAlreadyPreparedStatement, (new Callback(){
    public void result(ResultSet result){
        while (result.next()) {
            //Handle each row from the result and do any processing
    }
}));

How could this be improved? Is this as good as it gets?

This is as good as it gets in Java. The idea of callbacks are very popular, and are the foundation of functional languages. Even Javascript offers a cleaner approach, as JQuery shows. Unfortunately for Java, anonymous inner classes (as you have) are the shortest way to create a function like this.


Script to move images into year and month folder This following code searches for jpg files of size larger than 500 KB. Its purpose is to move/cp files, depending on their EXIF data, into folders that correspond to the files' year and month.

find . -type f -iname '*.jpg' -size +500k | while read file; do
    yr=$(stat -f '%Sm' -t '%Y' "$file")
    month=$(stat -f '%Sm' -t '%m' "$file")
    folder="/path/to/backup/folder/$yr"
    subFodler=$folder"/"$month
    [[ -d "$folder" ]] || echo mkdir "$folder"
    [[ -d "$subFodler" ]] || echo mkdir "$subFodler"
    echo mv "$file" "$folder/$month"
done

Currently it´s a lot of duplicate lines.

I see this invocation of stat is MacOS specific.

Here's how I would shrink your code

find . -type f -iname '*.jpg' -size +500k -exec sh -c '
    for file in "$@"; do
        folder="/path/to/backup/folder/$(stat -f '%Sm' -t '%Y/%m' "$file")"
        mkdir -p "$folder"
        echo mv "$file" "$folder"
    done
' sh {} +

mkdir -p will create all missing folders, and it will also suppress errors if the directory already exists.

find's -exec cmd {} + feeds several filenames to the given command.

And this looks odd sh -c 'stuff' sh file ... -- the 2nd sh will be assigned to $0 inside the -c script, and the given files will be $1, $2, etc.


Prioritizing one column over the other column of the same table I am creating an API in .net core and I just could not figure out how to put the following Linq expression into a single call:

public async Task<ActionResult<IEnumerable<MyClass>>> GetByReference(string reference)
        {
             var myClassPkColumn = _context.MyClass
                .Where(myClass => myClass.PkColumn.Equals(reference))
                .Take(1)
                .ToList();

            if (myClassPkColumn.Count > 0)
            {
                return myClassPkColumn;
            }
            else
            {
                var myClassDescription = _context.MyClass
                .Where(myClass => myClass.Description.Contains(reference))
                .Take(8)
                .ToListAsync();
                return await myClassDescription;
            }
        }

In my front-end I am creating an autocomplete function and this now serves my intended purpose but I am curious if I could have made this into a single call (I guess for performance purposes).

If not possible then could I have written this in a cleaner way?

I hope that it is understandable that both columns are of type string and the first one I am querying is the primary key column (with 2-3 characters) while the second one is a non unique one with up to 50 characters.

The issue I had with the first approach:

var myClassResult = _context.MyClass
                    .Where(myClass => myClass.PkColumn.Equals(reference) ||
                     myClass.Description.Contains(reference))
                    .Take(8)
                    .ToListAsync();

was that the result was always prioritizing the Description column over the PkColumn intead of my desired expectation which is the other way around. Even after I added orderBy I couldnt get it to show me the pkColumn result when I know there was one.

I believe there is not much to improve in here.

If you really want to make a one call to sql server, then you can move this logic to sql procedure.

You can visually improve first query and use FirstOrDefault:

var myClassPkColumn = _context.MyClass.FirstOrDefault(x => x.PkColumn == reference);

if (myClassPkColumn != null)
{
    return new MyClass[] { myClassPkColumn };
}

If you know that your PkColumn is always less or equal 3 characters, then you can add if statement and skip first query completely in case your reference has bigger length.

if (reference?.Length <= 3)
{
    // query by PkColumn column
}

If you want to combine two queries into one you can add this OrderBy:

var myClassResult = _context.MyClass
                    .Where(myClass => myClass.PkColumn.Equals(reference) ||
                        myClass.Description.Contains(reference))
                    .OrderBy(x => x.PkColumn == reference ? 0 : 1)
                    .Take(8)
                    .ToList();


Issue a citation based on speed, with lenience for your birthday You are driving a little too fast, and a police officer stops you. Write
 code to compute the result, encoded as an int value: 0=no ticket, 1=small
 ticket, 2=big ticket. If speed is 60 or less, the result is 0. If speed is
 between 61 and 80 inclusive, the result is 1. If speed is 81 or more, the
 result is 2. Unless it is your birthday -- on that day, your speed can be
 5 higher in all cases.

public int caughtSpeeding(int speed, boolean isBirthday) {
if(isBirthday) {
    if(speed <= 65) {
        return 0;
    } else if(66 <= speed && speed <= 85) {
        return 1;
    } else if(86 <=  speed) {
        return 2;
    }
}

if(speed <= 60) {
    return 0;
} else if(61 <= speed && speed <= 80) {
    return 1;
} else {
    return 2;
}}

This function should probably be static, since it does not rely on any instance variables.

On your birthday, your speed can be 5 higher in all cases — so don't make three more cases for the birthday.

If you have already determined that the speed is not <= 60, then there is no point in testing 61 <= speed — that will always be the case.

Since all three branches similarly return an expression, I would use a ternary statement.

public static int caughtSpeeding(int speed, boolean isBirthday) {
    int birthdayAdjustedSpeed = isBirthday ? speed - 5 : speed;
    return (birthdayAdjustedSpeed <= 60) ? 0 :
           (birthdayAdjustedSpeed <= 80) ? 1 :
                                           2;
}


2D tilemap collision method Is this the optimal method to check for collisions in a 2d-based game?  I put together a working demo of 2D collisions here (WSAD to move, orange blocks collide).

I currently use the following code to check for collisions:

function checkmove(x, y) {
  if(level[Math.floor(x/20)][Math.floor(y/20)] == 1 || level[Math.ceil(x/20)][Math.floor(y/20)] == 1 || level[Math.floor(x/20)][Math.ceil(y/20)] == 1 || level[Math.ceil(x/20)][Math.ceil(y/20)] == 1) {
    return false;
  } else {
    return true;
  }
}

Update function:

function update(key) {
  switch(key) {
    case "W":
      if(checkmove(pos.x, pos.y-2)) {
        pos.y -= 2;
        break;
      } else {
        break;
      }
    case "S":
      if(checkmove(pos.x, pos.y+2)) {
      pos.y += 2;
        break;
      } else {
        break;
      }
    case "A":
      if(checkmove(pos.x-2, pos.y)) {
      pos.x -= 2;
      break;
      } else {
        break;
      }
    case "D":
      if(checkmove(pos.x+2, pos.y)) {
      pos.x += 2;
       break;
      } else {
        break;
      }
    default:
      break;
  }
}

This is called before movement is applied the the 'player'.  The game is laid out as a 2D array of 1's and 0's for testing purposes.

Can I use fewer mathematical operations (less expensive for each tick) to check for a collision between the player and a '1' on my game grid?

Before making this more efficient I'd suggest making it more readable:

in your switch statement you have break in both the true and else parts of your if statement.

if(checkmove(pos.x, pos.y-2)) {
    pos.y -= 2;
    break;
} else {
    break;
}

It would be much easier to read if you just move the break out of the if statement.

if(checkmove(pos.x, pos.y-2)) {
    pos.y -= 2;
}
break;

In another question I had suggested a bit shaving optimisation that might help here. Math.floor(number) is equivalent to number >> 0 this bit shifts the number by 0 bits and converts it to an integer.

Math.floor(x/20)

becomes

(x / 20) >> 0

similarly adding almost 1 (+ 1 - Number.EPSILON) and then flooring will give you the ceiling. (number + 1 - Number.EPSILON) >> 0

The checkmove() function calculates this multiple times on the one line. I'd rewrite it to calculate once.

function checkmove(x, y) {
  var floorX = (x/20) >> 0;
  var floorY = (y/20) >> 0;
  var ceilX = ((x/20) + 1 - Number.EPSILON) >> 0;
  var ceilY = ((y/20) + 1 - Number.EPSILON) >> 0;

  return level[floorX][floorY] == 1 ||
      level[ceilX][floorY] == 1 ||
      level[floorX][ceilY] == 1 ||
      level[ceilX][ceilY] == 1;
}


Compute the next occurrence of Friday the 13th How can I get the date of the next occurrence of Friday the 13th?

Below is my attempt. It returns the correct results. I think there must be a better way of writing this without checking thirteenth > start_date for every month. It might also be possible to exploit the fact that every calendar year has a Friday 13th.

import itertools
from datetime import date

def next_fri_13(start_date):
    '''Get the next Friday 13th after the specified start_date.

    Args:
        start_date (datetime.date)

    Returns:
        datetime.date: The first Friday 13th after the start_date.
    '''
    for year in itertools.count(start_date.year):
        for month in range(1, 13):
            thirteenth = date(year, month, 13)
            # Ensure that the 13th is after the start date, and is a Friday.
            if thirteenth > start_date and thirteenth.weekday() == 4:
                return thirteenth

r = next_fri_13(date(2020, 2, 11))
print(r)  # Prints: 2020-03-13

I think this can be slightly simplified by using dateutils.relativedelta (since datetime.timedelta does not support months, unfortunately). This way you don't need to manually keep track of the year and month and if you ever need to implement something for e.g. next end of a month that is on a Sunday, it is way easier, because it takes care of the peculiarities for you, like months ending on different days of the month.

I would also make the magic constant 4 into a global constant called FRIDAY.

from datetime import date
from dateutil.relativedelta import relativedelta

FRIDAY = 4    # Monday is 0

def next_fri_13(start_date):
    '''Get the next Friday 13th after the specified start_date.

    Args:
        start_date (datetime.date)

    Returns:
        datetime.date: The first Friday 13th after the start_date.
    '''
    thirteenth = date(start_date.year, start_date.month, 13)
    if start_date.day >= 13:
        thirteenth += relativedelta(months=1)

    while thirteenth.weekday() != FRIDAY:
        thirteenth += relativedelta(months=1)
    return thirteenth

Alternatively, you could make a generator of thirteenths days and drop all that are not a Friday using filter and use the fact that relativedelta does not only take offsets, but can also replace attributes at the same time (using the keywords without a trailing "s"):

from itertools import count

def next_fri_13(start_date):
    '''Get the next Friday 13th after the specified start_date.

    Args:
        start_date (datetime.date)

    Returns:
        datetime.date: The first Friday 13th after the start_date.
    '''
    thirteenths = (start_date + relativedelta(months=n, day=13)
                   for n in count(start_date.day >= 13))
    return next(filter(lambda d: d.weekday() == FRIDAY, thirteenths))

IMO the first is easier to read, though, while this one is slightly easier to convert into a generator yielding all Friday the 13ths after start_date (just replace return next(...) with yield from ...).

In any case, there is a bit of a philosophical debate about doing

import itertools

itertools.count()

or

from itertools import count

count()

I personally tend to favor the latter, as long as it is still somewhat easy to figure out which module a function comes from, and there is no shadowing. This makes the lines a bit shorter and easier to read, especially if you use one particular module quite heavily (which the itertools module is a bit predestined for). It is also slightly faster, because you only need to do a global lookup instead of a global lookup and an attribute lookup, but that is negligible most of the time (and if it is not, you should make it a local variable anyway). But in the end the choice is yours.


Displaying an image in a lightbox Please review my first query project. It achieves the desired result, but am I doing it right?

window.onresize = centreBoxInViewport;

function displayLightBox(obj) {

    // Get the path to the image
    var path = obj.getAttribute("href");

    // Create a new image, and set its source.
    var image = new Image();

    // The onload function must come before we set the image's src, see link for explanation.
    // http://www.thefutureoftheweb.com/blog/image-onload-isnt-being-called.

    // Anonymous function set to onload event, to make sure we only proceed if the image has been loaded.
    image.onload = function() {
        $('#image').attr("src", path)
        centreBoxInViewport();
        showOverlay();

        // Bind the hide functions here, as the elements have now been loaded into the DOM.
        $('#overlay').click(hideLightBox);
    };

    // Set the src, and show the image.
    image.src = path;
}

function hideLightBox() {
    fadeBox();
}

function showBox(){
    $('#lightbox').fadeTo(200, 1);
}

function fadeBox(){
    $('#lightbox').fadeTo(200, 0, function() {
        $('#lightbox').hide();
        hideOverlay();
    });
}

function centreBoxInViewport() {
    // Get the dimensions of the viewport.
    height = $(window).height()
    width = $(window).width();

    // Position the box.
    $('#lightbox').css('top', (height/2) - parseInt($('#lightbox').height())/2);
    $('#lightbox').css('left', (width/2) - parseInt($('#lightbox').width())/2);

}

function showOverlay() {
    $('#overlay').fadeTo(200, 0.5, function() { showBox() });
}

function hideOverlay() {
    $('#overlay').fadeTo(200, 0, function() {
        $('#overlay').hide();
    });
}

HTML

CSS

div#overlay { position: fixed; background-color:#000000; top: 0px; left: 0px; width: 100%; height: 100%; z-index: 99; }
div#lightbox { background-color: white; border: 5px solid white; z-index: 100; position: fixed; }

Looks fine to me, but I do have a few recommendations:

Instead of calling the onclick handler inline, consider doing the following:

$('#close').click(function(e) { fadeBox(e); });

Then I would pass e to fadeBox() and do e.preventDefault() so that the click event doesn't throw the user to the top of the page.

Also, you do not need to create the function hideLightBox() if the only thing it is doing is calling fadeBox().


std::function implementation I've decided to make an implementation of the C++11 class function. I was checking that I have done everything correctly and have not missed anything:

template < typename > class function;

template < typename _Ret, typename... _Args > class function<_Ret(_Args...)>
{
public:
    typedef _Ret result_type;
    typedef function<result_type(_Args...)> _Myt;
    typedef result_type(*pointer)(_Args...);

    function()
        : _f_ptr(nullptr)
    {
    }

    template < typename _Fn_Ty > function(_Fn_Ty &&_Fn)
        : _f_ptr(reinterpret_cast<pointer>(_Fn))
    {
    }

    function(pointer &&_Fn)
        : _f_ptr(_Fn)
    {
    }

    function(const _Myt &_Rhs)
        : _f_ptr(_Rhs.f_ptr)
    {
    }

    function(_Myt &&_Rhs)
        : _f_ptr(_Rhs.f_ptr)
    {
    }

    ~function()
    {
    }

    _Myt &assign(pointer &&_Fn)
    {
        _f_ptr = _Fn;
        return *this;
    }

    _Myt &operator=(pointer &&_Fn)
    {
        return assign(_Fn);
    }

    template < typename _Fn_Ty > _Myt &assign(_Fn_Ty &&_Fn)
    {
        _f_ptr = reinterpret_cast<pointer>(_Fn);
        return *this;
    }

    template < typename _Fn_Ty > _Myt &operator=(_Fn_Ty &&_Fn)
    {
        return assign<_Fn_Ty>(_Fn);
    }

    result_type operator()(_Args... _Arguments)
    {
        if (_f_ptr == nullptr)
        {
            throw std::exception("nullptr found instead of function");
        }
        return _f_ptr(_Arguments...);
    }

    pointer &ptr() const
    {
        return _f_ptr;
    }

    operator pointer &() const
{
    return _f_ptr;
}
private:
    pointer _f_ptr;
};

Your code has no chance of working, because the only non-static data member of your function<void(void)> is a single pointer of type void (*)(void). There's no room to store any other kind of functor.

In other words, you've implemented the concept of "function pointer wrapped in a struct", but you haven't implemented anything like std::function yet.


Get a random event out of a set of mutually exclusive events Task:

  Get a random event from a set of collectively exhaustive and mutually
  exclusive events, with this set being represented by a list of tuples
  (name, probability).

  Reminder:

  Collectively exhaustive events means at least one of them must occur.
  Mutually exclusive events means at most 1 of them occurs.
  Collectively exhaustive and mutually exclusive means their probabilities sum up to 1 and only one of them occurs.

Code:

Events definition:

events = [
    ('Win', 0.5),
    ('Loss', 0.3),
    ('Draw', 0.2)
]

Getting the random event:

def get_random_event(events):
    random_number = uniform(0., 1.)
    lower_bound = 0
    for event, prob in events:
        if lower_bound <= random_number <= lower_bound + prob:
            return event
        else:
            lower_bound += prob

Illustration:

0 -- -- Win -- -- 0.5 -- Loss -- 0.8 - Draw - 1

Generate a random number in [0,1] and check where it falls.

Some things:

The collectively exhaustive events are not going to change or be updated. So use a tuple instead of a list:

events = (  # <<- Note: ( not [
    ('Win', 50),
    ...
)

You don't show your import of random. But I'd prefer that you not import uniform by name, and rather spell it out inside your function. You're only calling it once, so it's more efficient to the reader for you to simply say:

random_number = random.uniform(...)

than to have them search backwards for where uniform occurs. Also, of course, in this context random.uniform actually reads better than just uniform.
You have an input parameter events. You use event, prob in your loop. But event is the singular of events, so when you say return event it seems like you are returning a member of the events iterable. I'd choose a different name, like outcome, making it clear that your returning a component of the inner tuple.
You compare lower_bound <= random_number <= lower_bound + prob but you don't have to. Since your random number is pinned to 0., you could just use a comparison against the high value and get the same result.
Structurally, the lower_bound += prob shouldn't be in the else branch. Yes, it only executes when the if is not executed, but the if should not be part of the flow for that statement. I'd rather see:

if ...
    return ...

lower_bound += prob

This kind of thing comes back to bite you when you're refactoring. Or, worse yet, when someone else is refactoring. (Imagine you changed the return to a print, or something. Suddenly the code stops working.)
The Task description would (almost) make a good function docblock. Note: s/list/iterable/:

from typing import Iterable, Tuple

Name = str
Prob = float

def get_random_event(events: Iterable[Tuple[Name, Prob]]) -> Name:
    """Get a random event from a set of collectively exhaustive
    and mutually exclusive events, with this set being represented
    by an iterable of tuples (name, probability).

    """


AngularJS recursive function call with $timeout I have created a recursive function call in AngularJS and just wonder if is there a better way to solve this problem? The code works perfectly.

My code is the following:

function getLocation(query, map, object, delay) {
    var deferred = $q.defer();
    var service = new google.maps.places.PlacesService(map);
    service.textSearch({ query: query }, function (results, status) {
        if (status == google.maps.places.PlacesServiceStatus.OK) {
            var loc = results[0].geometry.location;
            object.lang = loc.lng();
            object.lat = loc.lat();
            deferred.resolve(object);
        } else {
            if (status === "OVER_QUERY_LIMIT") {
                delay += 100;
                deferred.resolve($timeout(function () {
                    var deferred = $q.defer();
                    getLocation(query, map, object, delay).then(function (object) {
                        deferred.resolve(object);
                    });
                    return deferred.promise;
                }, delay));
            } else {
                deferred.reject(status);
            }
        }
    });
    return deferred.promise;
}

I have already tried to replace the following section:

deferred.resolve($timeout(function () {
    var deferred = $q.defer();
    getLocation(query, map, object, delay).then(function (object) {
        deferred.resolve(object);
    });
    return deferred.promise;
}, delay));

with:

$timeout(getLocation(query, map, object, delay).then(function (object) {
    deferred.resolve(object);
}), delay);

but it didn't work (perform the recursive call without timeout).

Could you give me some advice to build a less ugly function to solve this recursive call?

You're almost there. But first, a few code smells:

function getLocation(query, map, object, delay) {
    ...
    service.textSearch({ query: query }, function (results, status) {
        if (status == google.maps.places.PlacesServiceStatus.OK) {
            ...
            object.lang = loc.lng();
            object.lat = loc.lat();
            deferred.resolve(object);
        }
        ...
    ...
}

Your function returns a promise that resolves with an object containing lat and long. However, you are mutating an object called object. The problem here is the unwanted side effect. If you ever want to use this function elsewhere, you have to supply it an unnecessary object. If you also pass in an existing object, you unintentionally introduce lat and long.

if (status == google.maps.places.PlacesServiceStatus.OK) {
  ...
} else {
  if (status === "OVER_QUERY_LIMIT") {
    ...
  }
}

That google.maps.places.PlacesServiceStatus.OK is very long and visually annoying. Consider putting it in a shorter variable. Also, the if statement in the else block can be collapsed as an else if of the first if statement.

Lastly, the deferred pattern is actually an anti-pattern. Wrap your operation in a promise constructor instead of having a dangling deferred.

If I understand correctly, you need to do a location request, and retry on fail. You simply call your API. Resolve and reject accordingly.

function getLocation(query, map, retryTimeout){
  var service = new google.maps.places.PlacesService(map);
  var OK = google.maps.places.PlacesServiceStatus.OK;
  var OVER_LIMIT = 'OVER_QUERY_LIMIT';

  return new Promise(function(resolve, reject){
    service.textSearch({ query: query }, function (results, status) {
      var loc = results[0].geometry.location;

      if(status === OK ){
        resolve({ lat: loc.lat(), lang: loc.lang() });
      } else if(status === OVER_LIMIT){

        // The most interesting part. Reschedule a retry.
        setTimeout(function(){

          // When the promise of this call resolves then...
          // call the resolve/reject of the enclosing (previous) promise
          getLocation(query, map, retryTimeout).then(resolve, reject);
        }, retryTimeout);

      } else {
        reject(new Error(status));
      }
    });
  });
}

// Usage:
var object= {...}
var locationPromise = getLocation(query, map, 1000);

locationPromise.then(function(locationData){
  // Only then do you mutate. Only have getLocation get the location data.
  // Mutating object is not its concern. It's the consumer's concern.
  object.lat = locationData.lat;
  object.lang = locationData.lang;
});

Now here's the trick. In order to make getLocation act like a recursive call, you simply call getLocation again, but hand over the resolve and reject of the enclosing promise as its callbacks. When the succeeding getLocation call succeeds, it will call the previous promise's callbacks.

So in terms of execution, it's not really recursive and the entire operation is still async. However, how the promises enclose each other one call after the other and pass on their resolve and reject as callbacks to the next makes it act recursive.

Of course we cannot escape the wrath of setTimeout since we need it for delay.

Since Promises are not widely supported yet, you can use Angular's built-in $q which does the same thing, and probably more.


Hacky Haskell monadic testing Edit for posterity: The library you're looking for is Hspec.

I recently started writing Haskell tests with HUnit.
My general setup looks something like this:

import Test.HUnit

import Widget (foo, bar)

tests = TestList [ "foo" ~: testFoo
                 , "bar" ~: testBar
                 ]

testFoo :: Test
testFoo = TestList
  [ "with even numbers" ~:
    4 ~=? foo 4
  , "with odd numbers" ~:
    0 ~=? foo 5
  ]

testBar :: Test
testBar = TestList [ {- omitted -} ]

The semantics of this are fine with me: I'm trying to do expected–actual testing, not property testing like Quickcheck does.
However, there are two things that I don't like about the syntax:

the list syntax feels really clunky, and
I have to write "foo" once and testFoo two times (three counting the type annotations); while I could inline this, that would make the lists even clunkier.

My goal was to be able to write tests like this:

import Test.HUnit (Test, (~=?))
import Describe (toTests, (...), (~:))

import Widget (foo, bar)

tests :: Test
tests = toTests $ do
  "foo" ... do
    "with even numbers" ~:
      4 ~=? foo 4
    "with odd numbers" ~:
      0 ~=? foo 5
  "bar" ... do
    "with true" ~:
      10 ~=? bar True
    "with false" ~:
      -10 ~=? bar False

I managed to accomplish just that!
But to do so I had to resort to what I consider to be a pretty ugly monad.
Here's Describe.hs:

module Describe(group, describe, toTests, (~:), (...)) where

import qualified Test.HUnit as H

data LeftList l r = LeftList [l] ()
                    deriving (Show)

instance Monad (LeftList l) where
  (>>=) = error "LeftList does not support binding; use (>>) instead"
  (LeftList xs a) >> (LeftList ys b) = LeftList (xs ++ ys) b
  return x = LeftList [] ()

group :: String -> LeftList H.Test () -> LeftList H.Test ()
group s (LeftList xs ()) = LeftList [s H.~: xs] ()

(...) = group
infixr 9 ...

describe :: String -> H.Test -> LeftList H.Test ()
describe s x = LeftList [s H.~: x] ()

(~:) = describe
infixr 0 ~:

toTests :: LeftList H.Test () -> H.Test
toTests (LeftList xs _) = H.TestList xs

Obviously, the ugly part is creating this data type that has an unnecessary second type parameter and isn't actually a monad!
That is, my implementation of the do-notation is incomplete; sequencing works fine, but if I wrote x <- "foo" ... do { } I would get the error here.

I don't really mind that I'm shadowing HUnit.~:, although it's not optimal.

So, my question: what's the cleanest way to get the desired test case syntax without making the monads cry?

Firstly, I would advise you to look at how blaze-html implements their monads for HTML templating. They are doing something very similar to what you want to do.

You can simplify your definitions a bit by removing the second field from the LeftList constructor:

data LeftList a r = LeftList [a]

Then every place where you use the constructor LeftList you can omit the (now) extraneous (), e.g. the >> definition simplifies to:

  (LeftList xs) >> (LeftList ys) = LeftList (xs ++ ys)

You should also make group, describe, and toTests more general by using a type variable instead of ():

group :: String -> LeftList H.Test r -> LeftList H.Test r

This makes these functions valid for any return type r - not just ().
Of course, you don't care what the return type is anyway. But GHC cares - and allowing a general return type might help with type checking.

It actually is possible to define bind for the LeftList monad -- just pass in undefined or error "...":

  (>>=) (LeftList xs) f = let LeftList ys = f (error "LeftList does not support binding")
                          in LeftList (xs++ys)

An error will occur only if the parameter to f is actually evaluated.

For instance, this will not throw an error:

"foo" ... do
   x <- "with even numbers" ~:
           4 ~=? foo 4
   "with odd numbers" ~:
     0 ~=? foo 5

because the value x is never evaluated.

Finally, with GHC 7.10 you will also have to define Functor and Applicative instances for your monad:

instance Functor (LeftList a) where
  fmap f left = left

instance Applicative (LeftList a) where
  pure _ = LeftList []  -- should be same as return
  (<*>)  = undefined


DI container with auto-wiring feature So I decided to write my own DI container for educational purposes and would like some feedback on how I can improve the quality of my code.

I'm not sure about the documentation as most of my experience has been with closed source personal code, so I would like some feedback on that.

Another area is where I am using exit(), maybe an exception would be better? Although my experience is minimal here, maybe Exception or RuntimeException?

I would also prefer responses about common good practices over personal preferences.

<?php declare (strict_types = 1);

namespace Rosa;

use RuntimeException;

class Rosa {
    private $objects = [];

    /**
     * Register an instantiated object to the container.
     *
     * @param object $object
     */
    public function register(object $object) : void {
        $this->objects[get_class($object)] = $object;
    }

    /**
     * Fetch a cached object from the container.
     *
     * @param string $objectName
     * @return object
     */
    public function fetch(string $objectName) : object {
        if (array_key_exists($objectName, $this->objects)) {
            return $this->objects[$objectName];
        }

        return $this->make($objectName);
    }

    /**
     * Creates an object from its name and auto-wires constructor arguments.
     *
     * @param string $objectName
     * @return object
     * @throws \ReflectionException
     */
    private function make(string $objectName) : object {
        $reflection = new \ReflectionClass($objectName);

        if (!$reflection->isInstantiable()) {
            exit($reflection->getName() . ' can\'t be instantiated.');
        }

        $arguments = $this->resolveArguments($reflection);

        if (count($arguments) < 1) {
            return $reflection->newInstance();
        }
        else {
            return $reflection->newInstanceArgs($arguments);
        }
    }

    /**
     * Creates an array of arguments from a reflection class.
     * Uses default value if there is one, auto-wires the object if not.
     *
     * @param $reflection
     * @return array
     */
    private function resolveArguments($reflection) : array {
        $constructor = $reflection->getConstructor();
        $parameters = $constructor->getParameters();

        if (!$parameters) {
            return $reflection->newInstance();
        }

        $arguments = [];

        foreach ($parameters as $parameter) {
            if ($parameter->isDefaultValueAvailable()) {
                $arguments[] = $parameter->getDefaultValue();
                continue;
            }

            if ($parameter->getClass() == null) {
                exit($parameter->name . ' on ' . $reflection->getName() . ' needs a default value');
            }

            $arguments[] = $this->fetch($parameter->getClass()->getName());
        }

        return $arguments;
    }
}

Naming

class Rosa tells me nothing. The name should scream out that it is a dependency injection container.

PSR

There is a standard interface for DI containers.
https://github.com/php-fig/container/blob/master/src/ContainerInterface.php

You might want to have you container implement it.

Exit vs. Exception

Implementing the PSR interface would automatically answer your question about exit vs. exception. In short, yes throw exception. Don't call exit generaly from anywhere.

Shared vs one-time services

Your container keeps references of services that you have registered but will instantiate a new one every time if not registered explicitly.

This is rather unintuitive. Consumers of the container should either be able to choose whether a service is shared or should get new instance every time. Or they should always receive consitent behaviour for all services (only one of them is supported).

On demand instantiation

Assuming you have chosen to support shared services, it is common that only a recipe for the service is provided and the actual service object is instantiated on first demand.

Configuration of Services

It would be more precise to say that your container does not just have autowiring feature, but that the autowiring is almost unevitable. You either register instantiated service (for which you have had to resolve dependencies manually) or it autowires all constructor arguments (potentialy recursively).

This is somewhat related to the previous section. If you had recipes for service instantiation, you could define which values parameters should be autowired and which should receive value defined by the consumer.

Services Dependant on Interfaces

You assume that a constructor parameter is a class or a primitive type with default value. Recipes could again help with primitives without default.

If it is not a primitive it still does not mean it is a class, it can be an interface. But you dont have any mechanism autowire interfaces because you dont keep track of interfaces implemented by services.

Service Identification

You save services by class name, making it impossible to have multiple services of the same class. It is common that it is possible to set a name for a service. Again this could be property of a service recipe.

Single Responsibility Principle

Autowiring is quite a big topic and it could be useful in conjunction with other things that a DI container. Therefore it makes sense to separate the autowiring mechanism to a separate class and have the container just depend on it.


Permutation index Python Here is my Python code for finding the permutation index of a given digits and a target number. And by permutation index I mean, for example, given digits [1, 2, 3, 4]. find the index of 3124, meaning the position of 3124 in all permutations. This code finds that. However I have suspicions about it efficiency and implementation. Please review my code.

def perm_ind(numbers, target, length):
    """
    Finding the position of a single permutation in all permutations.
    Using the mathematic formula.
    """

    ranks = sorted(numbers) # For keeping the index even after some numbers are placed and removed.
    ind = 0
    for step in reversed(xrange(length)):

        num = (target / 10 ** step) # These two lines are used to get
        target %= 10 ** step        # the left-most digit in each iteration

        times = ranks.index(num) # Find the rank of the currect digit.

        ind += factorial(length - 1) * times # Add to result
        ranks.remove(num) # Remove the used(Placed) number
        length -= 1 # Decrease length

    return ind

# Example usages
print perm_ind([1, 2, 3], 123, 3)
print perm_ind([1, 2, 3], 132, 3)
print perm_ind([1, 2, 3], 213, 3)

Interface

The parameters passed to the function are not orthogonal; they are subject to a number of restrictions, such as

length == len(numbers)
length == len(str(target))
target is composed of precisely the elements of numbers

If any restriction is violated, the function fails with an exception, or produces the wrong result. You may want to validate the input, or revise the interface to infer as much as possible from as little as necessary.

For example, it is reasonable to assume that length is len(numbers), and don't pass it at all. It is also reasonable to assume that target is well formed, and infer the alphabet from it.

By passing target as an integer you severely restrict the utility of a function. I highly recommend to pass it as a list of (comparable) elements.

Efficiency

First obvious inefficiency is recalculation of factorial, which drives the complexity quadratic by length. I recommend precompute factor = factorial(length) before the loop, and do

    factor /= length
    ind += factor * times
    length -= 1

to reduce the complexity to linear.

Second, extraction of leading digit also involves heavyweight computations. Even if the target is passed as integer, you'd be better off by converting it to an iterable, e.g.

    target = str(target)

and access individual digits positionally. And let me reiterate, it is better to pass target as iterable in the first place.


Writing lists to .csv file I'm teaching myself programming, using Python as my initial weapon of choice.

I have learnt a few basics and decided to set myself the challenge of asking the user for a list of names, adding the names to a list and then finally writing the names to a .csv file.

What would you do differently, i.e. how could this code be improved for readability and efficiency? Would you approach the situation differently, structure it differently, call different functions? I am interested in, and would appreciate a great deal, the feedback from more experienced programmers.

In particular, I find certain parts clunky; such as having to specify to the user the required format for data entry. If I were to simply request the data (name age location) without the commas however, then each record, when written to .csv, would simply end up as one record per cell (Excel) – this is not the desired result.

guestNames = ""
guestList = []
number = 0
guestNumber = ["second","third","fourth","fifth","sixth"]
finalList = ""

#Requesting user input.
guestNames = input("Please enter the names of your guests, one at a time.\n"\
    "Once you have finished entering the information, please type the word \"Done\".\n"\
    "Please enter your names in the following format (Name, Age, Location). ").capitalize()

guestList.append(guestNames)

while guestNames.lower() != "done".lower() :
    guestNames = input("Please enter the name of your " + guestNumber[number] + " guest: ").capitalize()
    guestList.append(guestNames)
    number += 1

#Sorting the list.
guestList.sort()
guestList.remove("Done")

#Creating .csv file.
guestFile = open("guestList.csv","w")
guestFile.close()

#Writing to file.
for entries in guestList :
    guestFile = open("guestList.csv","a")
    guestFile.write(entries)
    guestFile.write("\n")
    guestFile.close()

Here is just a quick response to your code:

Look into the csv module – This module handles csv-files rather nicely
Look into using functions to separate concerns – At all the places you've added comments, you should/could use functions. And the comments should rather be docstrings on line after function definition like in the following:

def write_list_to_file(guest_list, filename):
    """Write the list to csv file."""

    with open(filename, "w") as outfile:
        for entries in guest_list:
            outfile.write(entries)
            outfile.write("\n")

Variable and function names are usually snake_case – See example above, which is according to PEP8
Use the with construct to encapsulate file handling – This is called a context manager, and it handles error cases, opening and closing of the file. See example above.
Why do you create the file, and not write to it directly? – I don't quite see why you do the # Creating .csv file, when you can both create and write to it directly.
Change text to avoid guestNumber – Having the guestNumber seems unneccessary. If you change the text, you don't need it. Using something similar to "Add another guest (Name, age, location) or 'Done': ".
Doing capitalize() is somewhat "dangerous" – What if "John McEnroe" comes along, or "Jean von Luxembourg"?  They wouldn't like their name changed into "John Mcenroe" or "Jean Von Luxembourg". The same applies to the location part.
In general, don't add stuff you're going to remove immediately afterwards – That is I would most likely not add the Done guest for later removal, instead of adding it first and then removing it. Add a simple if in front of the append and the issue is avoided.
Avoid lengthy input prompts – Maybe personal preference, but I would rather have a lengthy text in front of the input prompt, instead of a lengthy prompt text. This way you could possibly also give even more examples of legal input.
Change into separate inputs – If you change to using the csv module, I would also change to three separate input statements, and possible do some simple validation on each of the inputs before adding them into the guest list.
Careful think about variable names – Some of your names are good, and some not so good. I.e. guestNames when it actually is both the name, age and location, is misleading. And in for entires in guestList, it would possibly read easier as for guest in guest_list:.

The latter is a typical pattern by the way, that is to use the singular of something plural, i.e. for guest in guests:, for number in numbers:, and so on. In general, plural version indicates list of some sort, whilst the singular version can be used within loops and similar.

I'll end this hasty review with a very useful construct which could cleanup your code (when you've created functions):

def main():
    """Main method used when called as a script."""

    guests = enter_guests()
    write_list_to_file(sorted(guests), "guestlist.csv")

if __name__ == '__main__':
    main()

Using this construct would allow for your code to be reused as a module later on, whilst still allowing it to be run directly from the command line. It also makes it very easy to understand what happens just skimming the code.


Excel to JSON parser with http download I've been working on a project (link) to download a spreadsheet of known ransomware and properties and turn it into json so I can better consume the information within early detection projects.

I'm new to python - what can I be doing better? The destination json I'm converting can be found here.

update_json.py (entry point)

from excel_to_json import excel_to_json
from download_file import download_file

SOURCESHEET = 'https://docs.google.com/spreadsheets/d/1TWS238xacAto-fLKh1n5uTsdijWdCEsGIM0Y0Hvmc5g/pub?output=xlsx'
OUTPUTSHEET = '../RansomwareOverview.xlsx'
JSONFILE = '../ransomware_overview.json'

def write_json_file(json_data, filename):
    output = open(filename, 'w')
    output.writelines(json_data)

def generate_json(source_file, download_destination, json_file):
    download_file(source_file, download_destination)
    write_json_file(excel_to_json(download_destination), json_file)

generate_json(SOURCESHEET, OUTPUTSHEET, JSONFILE)

download_file.py

import urllib.request

def download_file(source, destination):
    try:
        urllib.request.urlretrieve(source, destination)
    except IOError:
        print('An error occured trying to write an updated spreadsheet. Do you already have it open?')
    except urllib.error.URLError:
        print('An error occured trying to download the file. Please check the source and try again.')

excel_to_json.py

import simplejson as json
import xlrd
from collections import OrderedDict

def excel_to_json(filename):
    wb = xlrd.open_workbook(filename)
    sh = wb.sheet_by_index(0)
    mw = wb.sheet_by_index(2)

    # List to hold dictionaries
    c_list = []

    # Iterate through each row in worksheet and fetch values into dict
    for rownum in range(1, sh.nrows):
        wares = OrderedDict()
        row_values = sh.row_values(rownum)

        if row_values[6]=="":
            name = row_values[0]
            gre=[name]
        elif "," in row_values[6]:
            e=row_values[6].split(",")
            ge = [row_values[0]]
            gre=e+ge
        else:
            gre=[row_values[0],row_values[6]]

        wares['name'] = gre
        wares['extensions'] = row_values[1]
        wares['extensionPattern'] = row_values[2]
        wares['ransomNoteFilenames'] = row_values[3]
        wares['comment'] = row_values[4]
        wares['encryptionAlgorithm'] = row_values[5]
        wares['decryptor'] = row_values[7]
        if row_values[8]=="":
            wares['resources'] = [row_values[9]]
        elif row_values[9]=="":
            wares['resources']=[row_values[8]]
        else:
            wares['resources'] = [row_values[8], row_values[9]]
        wares['screenshots'] = row_values[10]

        for r in range(1, mw.nrows):
            rowe = mw.row_values(r)

            if row_values[0] == rowe[0]:
                wares['microsoftDetectionName']=rowe[1]
                wares['microsoftInfo'] = rowe[2]
                wares['sandbox'] = rowe[3]
                wares['iocs'] = rowe[4]
                wares['snort'] = rowe[5]

        c_list.append(wares)

    # Serialize the list of dicts to JSON
    return json.dumps(c_list, indent=4)

Performance tips:

ujson can bring more speed
since both simplejson and xlrd are pure-python, you may get performance improvements "for free" by switching to PyPy
you may (or not) see speed and memory usage improvements if switching to openpyxl and especially in the "read-only" mode
in the excel_to_json function, you are accessing the same values from row_values by index multiple times. Defining intermediate variables (e.g. defining name = row_values[6] and using name variable later on) and avoiding accessing an element by index more than once might have a positive impact
I'm not sure I completely understand the inner for r in range(1, mw.nrows) loop. Can you break once you get the if row_values[0] == rowe[0] evaluated to True?
are you sure you need the OrderedDict and cannot get away with a regular dict? (there is a serious overhead for CPythons prior to 3.6)
instead of .dumps() and a separate function to dump a JSON string to a file - use .dump() method to dump to a file directly - make sure to use with context manager when opening a file

Code Style notes:

follow PEP8 guidelines in terms of whitespace usage in expressions and statements
properly organize imports
if row_values[6]=="": can be simplified to if not row_values[6]: (similar for some other if conditions later on)
the generate_json() call should be put into the if __name__ == '__main__': to avoid it being executed on import
the excel_to_json() function is not quite easy to grasp - see if you can add a helpful docstring and/or comments to improve on clarity and readability

Other notes:

improve variable naming. Variables like sh, mw, rowe are very close to being meaningless. I would also replace wb with a more explicit workbook
have you considered using pandas.read_excel() to read the contents into the dataframe and then dumping it via .to_json() (after applying the desired transformations)?


Class that only creates an instance of a new game I am creating a game. I am at the final phase for the underlying basis and I am wondering whether what I am doing is recommended- if it's convention.

All this class does is create an instance of the game. That's it. The game instance takes care of the rest (i.e attaching the menu screen to the screenManager (i.e displaying the menu screen))

As a result, this class complains that we're not using the value. But that's exactly what we want, we HAVE no use of this result. I just want to know, is this OK?

    public final class RunGame
{
    private RunGame() {}

    public static void main(String[] args){
       Game game = new Game(); // We want to start a new instance of the game, but we have no use with the result!
    }
}

Being pedantic about the terminology: Not the class, but the IDE, complains about the unused value. As hinted by markspace, I agree that first creating an instance of the game and then starting it is the natural way to think about it. Don't add to many side effects to the constructor that the user might not expect.

To get rid of the warning about the unused value, don't assign the instance to a variable. Just do new Game() or new Game().start().

Here is my recommendation, with fixed formatting and changed class name (since RunGame sounds like a method name). I also removed the constructor from the main class; I don't think you need it, since main() is static.

public final class Main
{
    public static void main(String[] args) {
        new Game().start();
    }
}


Filtering product ids As regarding the following question I have completed the code as below.

But I am curious about other effective solutions. I am also curious about if there is any solution that could be applicable by using java stream API.

It is not mandatory to keep records at int[][] matrix. Any data structure can be used.

My solution has O(n) time complexity. If you have a better to solve the question please share your solution.

For a given retailer "100" function only needs to return 4 because product 1 exists in other retailers.

Thanks.

public class InterviewQuestion {

 public static void main(String[] args) {
    int[][] itemSeller =       { {1, 100},
                                 {2, 200},
                                 {3, 300},
                                 {4, 100},
                                 {1, 200},
                                 {2, 300} };

    int target = 100;

    getUniqueProduct(itemSeller,target);

}

private static void getUniqueProduct(int[][] itemSeller, int target) {

    int count[] = new int[itemSeller.length];

    for (int i = 0; i < itemSeller.length; i++) {
        count[itemSeller[i][0]]++;
    }

    for (int i = 0; i < itemSeller.length; i++) {
        if (target == itemSeller[i][1]) {

            int item = itemSeller[i][0];
            if (count[item] > 1) {
                continue;
            } else
                System.out.println(item);
        }
    }
}
}

Your class is InterviewQuestion, if it is for an interview, then there's a few other things to consider, in addition to whether or not you get a reasonably efficient solution.  Whilst a bad solution will probably rule you out, a non-optimal solution may be acceptable depending on other factors.

Seperation of concerns

Finding the list of unique product items seems like it's a different concern to printing the list out to the console.  Rather than printing the items out directly, it would be better to return the list of items and then print them from the caller.  Not only does this demonstrate that you're thinking about reuse, but it also makes the code easier to write automated tests (for a given set of inputs, you can test against the expected results).

Naming

Naming is fairly important for readability.  You variable names are mostly appropriate, but a few small things.  Your method getUniqueProduct doesn't return anything, which is misleading.  productRetailer is a collection of mappings, rather than a single product retailer.  Something like productRetailerMappings might be more descriptive.  target refers to a retailer, so targetRetailer would make this clearer, otherwise given the context it might be expected that it's the targetProductId.

Consistency

Modern IDE's can auto-format your code for you.  Inconsistencies suggest that either you don't use your IDE effectively, or that you lack attention to detail.  Both of these are things that interviewers are going to consider.  Two obvious things that stand out are sometimes there's ) { and sometimes there's ){.  A missing space may seem minor, but it is noticeable.  Another is blank line after at the top of the if block.  Sometimes you have one, sometimes you don't.

To continue...or not

if(count[item]>1){
    continue;
}else
    System.out.println(item);

It's almost always preferable to use {} around your if/else clauses.  Doing it for one side of an if, and not for the else, when they're both one-line is unnecessarily confusing.  The else is also redundant in this case, since if the if is triggered, the following code won't be executed anyway, you don't need the else.  Where possible you want to avoid introducing unnecessary nesting.  Since there's nothing else after the println that's executed, inverting the if would also make the code cleaner, since you wouldn't need the branch condition:

if(count[item]==1) {
    System.out.println(item);
}

Constants can help readability

You're using a two dimensional array, with a fixed size.  Have two constants, one for the ITEM_COUNT and one for the RETAILER_ID would help in understanding.  It's not obvious from this line:

if(target == productRetailer[i][1]) {

What '1' is referring to (retailer or item).


Query of consultants and their expected payments for the month I have the following SQL query which takes ~3 seconds to run, maybe ~4. This populates a consultants table with their expected payments for the month.

Unfortunately, the page only loads when this query is finished. Is there any way to make it quicker?

Is there anything you notice that I could do better or any best practices I could follow?

DECLARE @lastMonthDate VARCHAR(10)
DECLARE @currentMonthDate VARCHAR(10)
DECLARE @Year VARCHAR(10)

SET @lastMonthDate = CONVERT(VARCHAR(4), DATEPART(YEAR, DATEADD(MONTH, -1, GETDATE())))+'-'+CONVERT(VARCHAR(2), DATEPART(MONTH, DATEADD(MONTH, -1, GETDATE())))+'-21'
SET @currentMonthDate = CONVERT(VARCHAR(4), DATEPART(YEAR, GETDATE()))+'-'+CONVERT(VARCHAR(2), DATEPART(MONTH, GETDATE()))+'-20'
SET @Year = DATEPART(YEAR, GETDATE())

SELECT Comp_Name, comp_idnr, oppo_date17_1sent,datename(month, oppo_date17_1sent), budg_tempDODate1, CONVERT(DECIMAL(10,2), budg_do1_total) AS budg_do1_total, budg_paymentMethodM_R,  v_ClientTransactionSummary.HyphenStatus, v_ClientTransactionSummary.TransactionDate FROM Company

JOIN Opportunity on Company.Comp_CompanyId = Opportunity.Oppo_PrimaryCompanyId
JOIN Budget on  Opportunity.Oppo_OpportunityId = Budget.budg_OpportunityId
JOIN Users on company.Comp_PrimaryUserId = User_UserId
Join Channel on User_PrimaryChannelId = Channel.Chan_ChannelId
JOIN [Server2].PayTrader.dbo.v_ClientTransactionSummary on Company.comp_idnr COLLATE SQL_Latin1_General_CP1_CI_AS = v_ClientTransactionSummary.IDNumber COLLATE SQL_Latin1_General_CP1_CI_AS AND budg_do1_total = v_ClientTransactionSummary.ExpectedAmount AND CONVERT(Date, budg_tempDODate1, 105) = CONVERT(Date, v_ClientTransactionSummary.TransactionDate, 105)
WHERE CONVERT(DATE, budg_tempDODate1) BETWEEN CONVERT(DATE, @lastMonthDate) AND CONVERT(DATE, @currentMonthDate) AND User_FirstName = @Name AND User_LastName = @Lastname AND Oppo_deleted IS NULL
Order by budg_tempDODate1 ASC

you are doing a lot of CONVERTing in this query.

Instead of converting the Date variables inside the WHERE clause you should do that before you even start the query, otherwise you are running this convert for every row in the SELECT because the WHERE decides if a row should be kept or discarded from the returned table.

Are the date fields not already in DateTime format?

get rid of all the Converts that are not needed.

also you should format your code with some indentation so that it is easier to read, for you and for others who will maintain the code you write.


How to dynamically Generate Sort strings for Data Layer from Controller I need help refactoring this code to dynamically generate a sort string that I can send to my data layer so that my database does the sorting instead of it happening in memory. I am using MVC4 with EF5 as my data layer.

public ActionResult InstanceSearch(int? page, string sortOrder, string computerName, string instanceName,
                                       string productName, string version)
    {
        int pageSize = 10;

        int pageNumber = (page ?? 1);
        if (pageNumber < 1)
        {
            pageNumber = 1;
        }

        if (string.IsNullOrEmpty(sortOrder))
        {
            sortOrder = "Computer";
        }

        ViewBag.CurrentSort = sortOrder;
        ViewBag.ComputerSort = sortOrder == "Computer" ? "ComputerDesc" : "Computer";
        ViewBag.InstanceSort = sortOrder == "Instance" ? "InstanceDesc" : "Instance";
        ViewBag.VersionSort = sortOrder == "Version" ? "VersionDesc" : "Version";
        ViewBag.ProductSort = sortOrder == "Product" ? "ProductDesc" : "Product";

        //IEnumerable<Instance> instances = ecuWebDataContext.Instances;

        IQueryable<Instance> instances
        = string.IsNullOrEmpty(computerName) == true ? ecuWebDataContext.Instances : ecuWebDataContext.Instances.Where(i => i.Computer.Name == computerName);

        if (string.IsNullOrEmpty(instanceName) == false)
        {
            instances = instances.Where(i => i.Name == instanceName);
        }

        if (!string.IsNullOrEmpty(productName))
        {
            ProductName product = (ProductName)Enum.Parse(typeof(ProductName), productName);
            instances = instances.Where(i => i.Product.Name == product);
        }

        if (!string.IsNullOrEmpty(version))
        {
            instances = instances.Where(i => i.Version == version);
        }

        switch (sortOrder)
        {
            case "Computer":
                instances = instances.OrderBy(i => i.Computer.Name);
                break;

            case "ComputerDesc":
                instances = instances.OrderByDescending(i => i.Computer.Name);
                break;

            case "Instance":
                instances = instances.OrderBy(i => i.Name);
                break;

            case "InstanceDesc":
                instances = instances.OrderByDescending(i => i.Name);
                break;

            case "Version":
                instances = instances.OrderBy(i => i.Version);
                break;

            case "VersionDesc":
                instances = instances.OrderByDescending(i => i.Version);
                break;

            case "Product":
                instances = instances.OrderBy(i => i.Product.Name);
                //instances = instances.OrderBy(i => Enum.Parse(typeof(ProductName), i.Product.Name));
                break;

            case "ProductDesc":
                instances = instances.OrderByDescending(i => i.Product.Name);
                //instances = instances.OrderByDescending(i => Enum.Parse(typeof(ProductName), i.Product.Name));
                break;
        }
        ViewBag.SortOrder = sortOrder;

        var instanceSearchModel = new InstanceSearchModel { ComputerName = computerName, InstanceName = instanceName };
        ViewBag.ComputerName = computerName;
        ViewBag.InstanceName = instanceName;
        ViewBag.ProductName = productName;
        ViewBag.InstanceCount = instances.Count();
        ViewBag.Version = version;
        return View(instances.ToPagedList(pageNumber, pageSize));
    }

I tried returning an IQueryable from the data layer, but then I run into connection problems because I never close it.

The data layer that appears to be causing your problem is the root cause.

You're abstracting over an abstraction, and in the process having to marshall sort orders around as a result. Prefer injecting your context in a request scope and remove the abstraction.

You can always create an abstraction over the specific query, and inject the query, which has a dependency on the context, if you really need to.


Split string into 160-character chunks while adding text to each part The send_sms_message method takes text as an argument. The length of text is unknown and can be 0 or as large as 1070 (after that we will have no space for the actual text, because multi-part message text will be so long).

The trick is that if text is larger then 160 characters, we need to split it into parts and to each part add text part 1 of 2.

The problem is that we don't know the length of the extra text. It can be 14 characters for part 1 of 2 or 16 for part 11 of 99 and so on.

This code can handle up to 99 parts. How can I make it generic to handle more parts without adding more conditions?

SMS_LENGTH = 160
MPM_SIZE_LONG = 16
MPM_SIZE_SHORT = 14
MPM_SHORT_LIMIT = 1314

def send_sms_message(text, to, from)
  unless text.length > SMS_LENGTH
    deliver_message_via_carrier(text, to, from)
  else
    parts = text.scan(/.{1,#{SMS_LENGTH - (text.length > MPM_SHORT_LIMIT ? MPM_SIZE_LONG : MPM_SIZE_SHORT)}}/)
    parts.to_enum.with_index(1) do |message_part, index|
      deliver_message_via_carrier("#{message_part} - Part #{index} of #{parts.length}", to, from)
    end
  end
end

Some notes:

Is your code inside a module or a class, I guess? You can include it in the question.
As a general rule, favour the user of affirmative statements (if instead of unless).
If you spend enough time with a pencil and a paper, maybe you'd get to a nice formula get_total_parts(message_size, sms_size, parts_message_min_size), but it's not trivial, that's for sure. A pre-computing of the max_size (along with some extra info you'll need) is a bit tedious to write but fast. For example, what's the maximum size if you have a total count with exactly 3 digits (100-999)? (SMS_SIZE - PARTS_MSG_MIN_SIZE - 2) * 9 + (SMS_SIZE - PARTS_MSG_MIN_SIZE - 3) * 90 + (SMS_SIZE - PARTS_MSG_MIN_SIZE - 4) * 900. You get the idea.

I'd write:

module SMS
  SMS_LENGTH = 160

  PARTS_MESSAGE = " - Part %{n} of %{total}"

  SPLIT_INFO = (1..70).map do |ndigits|
    base_size = (PARTS_MESSAGE % {n: "1", total: "1"}).size
    parts_per_digit = (0...ndigits).map do |n|
      (SMS_LENGTH - base_size - (ndigits + n - 1)) * (9 * (10 ** n))
    end
    {
      max_size: parts_per_digit.reduce(0, :+),
      size_of_first_parts: parts_per_digit[0...-1].reduce(0, :+),
      min_parts: 10**(ndigits - 1) - 1,
      msgsize_for_last_parts: (SMS_LENGTH - base_size - 2 * (ndigits - 1))
    }
  end

  def self.get_total_parts_for_long_message(text)
    info = SPLIT_INFO.detect { |h| text.size <= h[:max_size] } or
      raise ValueError("Message text too large")
    info[:size_of_first_parts] +
      Rational(text.size - info[:min_size], info[:msgsize_for_last_parts]).ceil
  end

  def self.send_sms_message(text, to, from)
    if text.length <= SMS_LENGTH
      deliver_message_via_carrier(text, to, from)
    else
      total_parts = get_total_parts_for_long_message(text)
      idx = 0

      (1..total_parts).each do |part_index|
        split_message = PARTS_MESSAGE % {n: part_index, total: total_parts}
        user_message_size = SMS_LENGTH - split_message.size
        message_text = text[idx, SMS_LENGTH - user_message_size]
        deliver_message_via_carrier(message_text + split_message, to, from)
        idx += user_message_size
      end
    end
  end

  def self.deliver_message_via_carrier(text, to, from)
    puts("Sending #{from} -> #{to}: #{text} - #{text.size} bytes")
  end
end