How to search for a word in a sorted text file efficiently? I want to write my own program to search for a word in a sorted text file. In the text file each word is in one line. I want to write this myself because I did not find any simple program (without libraries such as boost) which can provide suggestion word which means a word with most similarity. Here is my code but I don't know if I am in the right or wrong track. Appreciate any suggestion.

#include<cstdlib>
#include<fstream>
#include<iostream>
using namespace std;

int main(){

  ifstream fin;
  fin.open("/usr/dict/words");
  if(!fin.good()){cout<<"No dictionary file!\n"; exit(1);}

  fin.seekg(0,ios::end);
  int last = fin.tellg();

  fin.seekg(0,ios::beg);
  int first = fin.tellg();

  string word = "book";
  int middle = 0;
  string search;
  bool found = false;
  while(!fin.eof())
  {
        middle=(first+last)/2;
        cout<<first<<" "<<last<<" "<<" "<<middle;  //**
        fin.seekg(middle);
        fin>>search;

        cout<<search<<endl;  //**
        if(search<word)
        {
            first=middle+1;
            cout<<"a";  //**
        }
        else if(search>word)
        {
            last=middle-1;
            cout<<"b";  //**
        }
        else
        {
            found=true;
            break;
        }
        cout<<first<<" "<<last<<" "<<" "<<middle<<endl;  //**

  }

  return 0;
}

Your biggest problem is the code does not take into account not finding the word. If it fails your code will loop forever. You need a not found exit case.

This is always a bad sign

while(!fin.eof())

That line is nearly always wrong (in every programming language I know). Especially in your case that should never be true (since you know where the end is).

Remember that the last successful read reads up-to but not past the EOF. The eof-flag is only set when you read past EOF.

So on the last good read you will read up-to the end of file processes the data. There is no more data left in the file but the eof-flag will not be set because you have not read past the EOF yet so you will re-enter the loop. The next attempt to read data will fail (setting the eof-flag), but the read has failed and thus left the variables you just read into in a questionable state.

Luckily you don't get into that state because you move after each search. But it is a sign of the wrong type of test. This should be:

while(searching) //   Set to false when you finish searching.
                 //   Which may be when you find the word
                 //   or there are no more word to search.

You are assuming that words start on the exact boundary of middle!

    middle=(first+last)/2;
    fin.seekg(middle);
    fin>>search;

Not sure I believe that will hold. Middle is a good estimate. But you need to find the start of the word. So seek forward for the next word.

    middle=(first+last)/2;
    fin.seekg(middle);
    fin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');

    if (!fin.eof()) // That may have read past EOF
    {
    }

Correct typing in C++ is everything:

fin.seekg(0,ios::end);
int last = fin.tellg(); // This does not return an int.
                        // With todays big files this can be rather a large
                        // number.

// That line should be.
std::streamsize last = fin.tellg();

If you don't care about the actual type (you just want the correct one). Then let the compiler work it out for you.

// C++11 let the compiler deduct the correct type of last.
auto last = fin.tellg()

Why take two lines to open the file:

ifstream fin;
fin.open("/usr/dict/words");

// Much simpler to read and write:
ifstream fin("/usr/dict/words");

Sure you can test for good(). But its better to let the stream decide if it is a usable state.

if(!fin.good()){cout<<"No dictionary file!\n"; exit(1);}

// Prefer to let the stream decide:
if (fin)  // Stream used in a boolean context will convert itself into
          // something useful that is convertible to true if you can
          // use the stream or false if the stream is not usable.
{
     cout<<"No dictionary file!\n";
     exit(1);
}

If the size of the dictionary is not huge (couple of Meg). I personally would just load it into memory and let the standard data structures handle it. Alternatively You could map the file to memory and do a more standard search.

bool isWordInDict(std::string const& word);
{
    struct MyDict: std::set<std::string>
    {
        typedef std::set<std::string>::const_iterator const_iterator;

        MyDict()
        {
            std::ifstream fin("/usr/dict/words");

            std::copy(std::istream_iterator<std::string>(fin), std::istream_iterator<std::string>(),
                      std::inserter(*this, end()));
        }
    };
    static const MyDict  dict;

    MyDict::const_iterator find = dict.find(word);
    return find != dict.end();
}


custom smart pointer class template Kindly provide your review comments:

//SmartPtr.h
class RefCount {
public:
    void AddRef() {
        ++(this->_count);
    }
    int Release() {
        return --(this->_count);
    }
private:
    int _count;
};

template <class T>
class SmartPtr {

public:

    // constructor
    SmartPtr();
    SmartPtr(T* iObject);

    // copy constructor
    SmartPtr(const SmartPtr<T>& iSPtr);

    // destructor
    ~SmartPtr();

    // operators
    SmartPtr<T>& operator=(const SmartPtr<T>& iSPtr);
    T& operator*();
    T* operator->();

private:
    T* _ptr;
    RefCount* _refCount;

    void _release();
    void _copySmartPtr(const SmartPtr<T>& iSPtr);
};

//SmartPtr.cpp
#include "SmartPtr.h"

// constructor
template <class T>
SmartPtr<T>::SmartPtr():
    _refCount(nullptr),
    _ptr(nullptr)
{
}

template <class T>
SmartPtr<T>::SmartPtr(T* iObject):
    _refCount(new RefCount()),
    _ptr(iObject)
{
    this->_refCount->AddRef();
}

// copy constructor
template <class T>
SmartPtr<T>::SmartPtr(const SmartPtr<T>& iSPtr)
{
    this->_copySmartPtr(iSPtr);
}

// destructor
template <class T>
SmartPtr<T>::~SmartPtr() {
    this->_release();
}

// operators
template <class T>
SmartPtr<T>& SmartPtr<T>::operator=(const SmartPtr<T>& iSPtr) {
    if (iSPtr._ptr && (this != &iSPtr)) {
        this->_release();
        this->_copySmartPtr(iSPtr);
    }
    return *this;
}

template <class T>
T& SmartPtr<T>::operator*() {
    return *(this->_ptr);
}

template <class T>
T* SmartPtr<T>::operator->() {
    return this->_ptr;
}

template <class T>
void SmartPtr<T>::_release() {
    if (this->_refCount && this->_refCount->Release() == 0) {
        delete this->_ptr;
        delete this->_refCount;
    }
}

template <class T>
void SmartPtr<T>::_copySmartPtr(const SmartPtr<T>& iSPtr) {
    this->_ptr = iSPtr._ptr;
    this->_refCount = iSPtr._refCount;
    this->_refCount->AddRef();
}

The Big Issue

One of the biggest things about smart pointers is that they guarantee deletion of the pointer. EVEN when exceptions happen. My main problem here is that your constructor may leak the pointer if it fails to initialize correctly.

template <class T>
SmartPtr<T>::SmartPtr(T* iObject):
    _refCount(new RefCount()),       // This can throw.
    _ptr(iObject)
{
    this->_refCount->AddRef();
}

If the creation of the _refCount object fails you will leak the pointer. You need to guarantee that the pointer does not leak:

SmartPtr<T>::SmartPtr(T* iObject)
try                               // Add a try block
    :_refCount(new RefCount())
    ,_ptr(iObject)
{
    this->_refCount->AddRef();
}
catch(...)
{
    delete iObject;    // If there is an exception make sure the object is deleted.
    throw;             // Then re-throw the exception.
}

Minor Missing Functions

The main missing things I see are:

 T*  get();            // Sometimes its nice to just get the pointer.
 operator bool();      // Nice when you want to simply check the smart
                       // pointer has something in a boolean context.

 SmartPtr<Stuff>  data = getSmartPointer();
 if (data) {
     data->doWork();   // Need to check there is a pointer in the
 }                     // object before calling doWork().

Constructor Problems

Explicit

Your one parameter constructor should be explicit. Think of this situation.

 void doStuff(SmartPtr<Stuff> work)
 {
      if (work) {
          work->doWork();
      }
 }

Looks simple enough. When you call the function you get another reference counted version of the pointer and thus it is simple and safe to use.

But what happens when I do this:

int main()
{
    Stuff*  work = new Stuff;
    doStuff(work);
    work->moreActions();
    delete work;
}

This code compiles. But the call to doStuff() results in delete being called on the work object. Even though you though it was safe to call (as you are making a copy of smart pointer).

The trouble is that the compiler has converted the Stuff* to a SmartPtr which is deleted in this scope.

What About nullptr

Your object does not accept a nullptr!

 SmartPtr<Stuff>  value = nullptr;  // fails to compile.

It does not look like a major problem. But when you start using templatized code where things are initialized and your type can be swapped in then it becomes an issue as your type can not be used.

What About Derived Types.

On of the major things about C++ is derived types with more functionality.

Derived*    x = new Derived;
Base*       y = x;

The same should work with smart pointers.

SmartPtr<Derived>  x = new Derived;
SmartPtr<Base>     y = x;           // fails to compile.

This will be a common in most C++ code (not exactly like this). But this functionality is really needed.

Make Shared

One of the things the standard builders found was that each shared pointer required the allocation of TWO objects. One for the thing and one for the counter.

They remedied this by introducing std::make_shared<T>(). This does one allocation that allocates the object and the counter inside the same space thus reducing the overall overhead of the object.

Further Reading

I cover a lot of these details and more in some articles I wrote:

Series
Smart-Pointer - Unique Pointer
Smart-Pointer - Shared Pointer
Smart-Pointer - Constructors


Try Parse Multiple Types I am trying to write a method that takes in a String and returns an Object. The catch is I want to parse it to one of a few different data-types, and if that fails, return the original String. I currently have the code below. Is this the best way to handle what I want or is there something obvious that I'm missing?

private static Object tryParse(String str) {
    try {
        return Long.parseLong(str);
    } catch (NumberFormatException ex1) {
        try {
            return Double.parseDouble(str);
        } catch (NumberFormatException ex2) {
            return str;
        }
    }
}

EDIT: I made a tweak that I think should improve performance a bit since I only need to check for longs, doubles, and Strings.

private static Object tryParse(String str) {
    try {
        Double d = Double.parseDouble(str);
        if (d % 1 == 0) {
            return d.longValue();
        }
        return d;
    } catch (NumberFormatException ex1) {
        return str;
    }
}

Your second code, while might be slightly more efficient than the first due to the avoidance of the second try ... catch inside the first, has two bugs:

First, if str contains a value greater than \$2^{63}-1\$ or less than \$-2^{63}\$, while it is an integer value (d % 1 == 0 will be true), it cannot be expressed as a long, so d.longValue() will discard the upper bits.

Second, if str contains a value which can be parsed as a Long, but has a magnitude greater than \$2^{52}\$, parsing it as a Double will discard some of the low-order bits.

jshell> Double d = Double.parseDouble("10000000000000123");
d ==> 1.0000000000000124E16

jshell> d.longValue()
$2 ==> 10000000000000124

jshell> Long.parseLong("10000000000000123");
$3 ==> 10000000000000123

The statements return Long.parseLong(str) and return Double.parseDouble(str) may be doing more work than necessary.  These methods return a long and a double, respectively, which then need to be auto-boxed into Long and Double.  You should instead use Long.valueOf(str) and Double.valueOf(str) which return already boxed values.

I would write this code like:

private static Object tryParse(String str) {
    try {
        return Long.valueOf(str);
    } catch (NumberFormatException ex) { /* no-op */ }

    try {
        return Double.valueOf(str);
    } catch (NumberFormatException ex) { /* no-op */ }

    return str;
}

... which avoids the nested try-catch blocks, so can easily be extended to several other types, possibly even looping over a collection of parsers:

private static Object tryParse(String str) {
    for (Parser parser : parsers) {
        try {
            return parser.valueOf(str);
        } catch (NumberFormatException ex) {
            /* no-op */
        }
    }

    return str;
}

or even

private static Object tryParse(String str) {
    for (Parser parser : parsers) {
        Optional<Object> value = parser.valueOf(str);
        if (!value.empty()) {
            return value.get();
        }
    }

    return str;
}

Implementation of Parser, and its instances, left as exercise to student.


Select the element out of the array if its index is a Fibonacci number I've some questions for an exam and wondered if somebody could take a look at my Fibonacci sequence that is supposed to answer this question:

  "select the element out of the array if its index is a Fibonacci number"

def fibonacci_sequence(max_num)
  sequence = [1, 2]
  loop do
    fibonacci = sequence[-2] + sequence[-1]
    break if fibonacci >= max_num
    sequence << fibonacci
  end
  sequence
end

# array size
max_num = 100
numbers = []
1.upto(max_num) { |num| numbers << num }
fibonacci = fibonacci_sequence(max_num)

# select fibonacci numbers and assign array to variable
fibonacci_numbers = numbers.select { |num| fibonacci.include?(num) }

My thoughts were that I had to work out what the Fibonacci sequence was before I could select numbers out of the array that were Fibonacci. The thing is I already answer the question by assigning these numbers to the Fibonacci variable. I've then used a select method on the original numbers array even though I already know what the output is going to be.

Have I gone about this the right way? I'm thinking no!

Some notes:

fibonacci_numbers = numbers.select { |num| fibonacci.include?(num) }. Array#include? is O(n), this will be very slow. With this approach, fibonacci should be a set.
Since you have to store a the fibonacci values, the space complexity won't be nice for big inputs.
Your algorithm only works for arrays, it would be nice if it worked with enumerables.

I'd write:

def remove_fib_indexes(xs)
  Enumerator.new do |yielder|
    fib_a, fib_b = [0, 1]
    xs.each.with_index do |x, idx|
      if idx == fib_a
        # skip the repeated 1 in the sequence [0, 1, 1, 2, 3, 5, ...]
        fib_a, fib_b = (fib_b == 1) ? [1, 2] : [fib_b, fib_a + fib_b]
      else
        yielder.yield(x)
      end
    end
  end
end

xs = (0..Float::INFINITY).lazy.map(&:to_s)
remove_fib_indexes(xs).take(10).to_a
#=> ["4", "6", "7", "9", "10", "11", "12", "14", "15", "16"]


Deleting email using mailcore with imapSession The following function works fine. However, deleting 1 mail takes about 4 seconds (from start of operation to the firing of the completion handler).

Currently, using mailcore2, copying mails on the Gmail server.

Are there any significant issues?

func deleteOnServer (indexSet: MCOIndexSet) {
     NSLog("will delete email")
     let sessionForOperation = getSessionForOperation()
     let localCopyMessageOperation = sessionForOperation.copyMessagesOperationWithFolder("INBOX", uids: indexSet, destFolder: account.trashFolderPath)
     localCopyMessageOperation!.start { (error, uidMapping) -> Void in
            if let error = error {
                NSLog("error in deleting email : \(error.userInfo!)")
            } else {
                NSLog("email deleted")
            }
        }
    }

}

My session is configured as follows:

    func createNewIMAPSessionWith(userName: String, hostname: String, oauth2Token: String) -> MCOIMAPSession {
        let retSession =  MCOIMAPSession()
        retSession.hostname = hostname
        retSession.port = 993
        retSession.username = userName
        retSession.OAuth2Token = oauth2Token
        retSession.authType = MCOAuthType.XOAuth2
        retSession.connectionType = MCOConnectionType.TLS
        retSession.maximumConnections = 2
        retSession.timeout = NSTimeInterval(60)
        retSession.allowsFolderConcurrentAccessEnabled = true

        return retSession
}

I don't know anything about mailcore2, nor have you provided any Time Profiler information to help narrow down what takes so long to complete the action (is it just a slow network, or is it something in your code?  Anyone's guess), so I can't really address the slowness issue.  I also don't have a clue which methods are yours versus what you simply get from mailcore2, but with that said...

NSLog() statements should almost always be wrapped in #if DEBUG and #endif statements.

Moreover, regardless of whether or not the deletion is successfully, we need to let the user know one way or the other, and NSLog() isn't going to cut it.

I'm not sure how the second code snippet ties to the first, but all of our methods could use better names.

deleteOnServer(indexSet:) - delete what?  on which server?  What does index set represent?

createNewIMAPSessionWith - the words New and With can both be removed from this method.  Although, what would probably be best is simply creating a factory method:

extension MCOIMAPSession {
    class func session(userName: String, hostname: String, oauth2Token: String) -> MCOIMAPSession {
        // all the code you're already doing
    }
}

And then we call it simply like this:

let imapSession = MCOIMAPSession.session(userName:"username", hostname:"hostname", oauth2Token:"token")

localCopyMessageOperation!.start

This is a pretty big no-no, in my opinion.  If the method we're calling to get localCopyMessageOperation returns an optional, then we should use real optional chaining, not forced unwrapping.  It's as simple as changing the exclamation point to a question mark and it prevents an "found nil when unwrapping" (or whatever it's called) exception.


Dynamically changing autocomplete source option jQuery In this scenario I've got a textbox to which I have added autocomplete functionality.  The kicker is that I want to dynamically change the source option of the AJAX call based on the value of a dropdown list.  To do this I encapsulated the differences between the AJAX calls in an object of type Resolver. The name property of the Resolver object is the same as the text value coming from the dropdown list.  I was looking for a way to make this a little cleaner.  The code works, just looking for more extensible options as the project grows.

$(document).ready(function ()
    {
        //encapsulate the URL of the AJAX call
        //the name property maps to a dropdown list value
        //so if Drugs is chosen from the dropdown menu we look for the name property which = drugs
        Resolver = function (name, url)
        {
            this.name = name;
            this.url = url;

        }
        Resolver.prototype.resolve = function (request, response)
        {
            $.ajax(
            {
                type: "POST",
                url: "Service.asmx/" + this.url,
                data: JSON.stringify({ 'param': $('#auto').val() }),
                dataType: "json",
                contentType: "application/json",
                success: function (data)
                {
                    response(data.d);
                },
                error: function (xhr)
                {
                    console.log(xhr.status);
                }
            });
        }
        //in the future I expect the Resolver object to have more properties
        //this is the simplest example I could think of
        var drugs = new Resolver('drugs', 'GetDrugNames');
        var hospitals = new Resolver('hospitals', 'GetHospitalNames');
        var resolverArray = [];
        resolverArray.push(drugs);
        resolverArray.push(hospitals);
        //on the change even of the ddl we're looking for the index of the resolverArray
        //which has the name name property as the value selected in the ddl.
        $('#ddl').change(function ()
        {
            var target = $(this).val().toLowerCase();
            var index = 0;
            $.each(resolverArray, function (ix, val)
            {
                if (resolverArray[ix].name == target)
                {
                    index = ix;
                }
            });
            $('#auto').autocomplete(
            {
                source: function (request, response)
                {
                    resolverArray[index].resolve(request, response);
                }
            });

        });
    });

One way to make it extensible is to make it reusable rather than stick it to one dropdown. You can even make it into a jQuery plugin so that it looks like this:

$('.someAutocompleteBox').dependentAutoComplete({

  /* the selector of the element to monitor changes */
  elementToMonitor : '.theElementToMonitor',

  /* value set, which correspond to the value of the previous input */
  sets : {
    drugs : DRUG_URL,
    hospitals : HOSPITAL_URL,
  }

  /* you can monitor depending on the event and which value to grab */
  triggeringEvent : 'keyup',
  propertyToCheck : 'value'
});

So in this example, I set .someAutocompleteBox to change sets when a keyup event is triggered from .theElementToMonitor. Then the plugin will grab the property defined in propertyToCheck and check it against the sets object which URL to use as source. Pretty simple isn't it?

As for jQuery plugin creation, you can use the jQuery Boilerplate to start off with a template. Everything else is self-explanatory, which is:

In the init() function of your plugin code, attach an event stated by triggeringEvent to the element stated by elementToMonitor.
When the event is fired, the handler should grab the value of the property defined in propertyToCheck.
Do a lookup from the passed sets property and grab the URL of the corresponding key (which is the value grabbed from the property earlier)
Update the current element's autocomplete source.

The plugin code (in the init) should look something like this:

init: function () {
  // nothing to check, nothing to check against
  if(!this.settings.elementToMonitor || !this.settings.sets) return;

  // Save the context
  var instance = this;

  // Attach the defined event to defined dependency
  $(this.settings.elementToMonitor)
    .on(this.settings.triggeringEvent,function(){

      // Get the value at defined property
      var value = $(this).prop(instance.settings.propertyToCheck);
      var url;

      // Check if we have that url in the set, return if not
      if(!(url = instance.set[value])) return;

      // Update the source accordingly
      $(instance.element).autocomplete({
        source = url;
      });
    });
}


Leetcode 3Sum problem using hashmap in Python For practice, I solved Leetcode 15. 3Sum question:

  Given an array nums of n integers, are there elements a, b, c in nums such that a + b + c = 0? Find all unique triplets in the array which gives the sum of zero.

My code pass all testcases from my location but get Time Limit Exceeded from leetcode. Can anyone suggest me how to improve my code?

def threeSum(nums):
    """
    :type nums: List[int]
    :rtype: List[List[int]]
    """
    if len(nums) < 3: return []
    len_n, res, n_map, target = len(nums), set(), dict(), 0
    nums.sort()
    for i, n in enumerate(nums):
        for j, c in enumerate(nums[i + 1:], i + 1):
            s = n + c
            n_map[s] = n_map.get(s, []) + [(i, j)]

    for i, n in enumerate(nums):
        s = target - n
        for k in n_map.get(s, []):
            if i > k[1]:
                res.add((nums[i], nums[k[0]], nums[k[1]]))
    return list(map(list, res))

I think the current way of solving this problem seems an acceptable, albeit naive, way to solve it, but there are some tweaks that can enhance the readability.

variables

len_n, res, n_map, target = len(nums), set(), dict(), 0

is both unclear and unnecessary.

len_n is never used, res is only used far further in the code

collections.defaultdict

You do a lot of n_map.get(s, []). Simpler would be to define n_map as a collectcions.defaultdict(list), and then for example just do n_map[s].append((i, j))

indices

You add (i, j) to n_map, only to later retrieve them as tuple k. It would be easier to use tuple unpacking:

for k, n in enumerate(nums): # i is used
    s = target - n
    for i, j in n_map[s]:
        if k > j:
            res.add((nums[k], nums[i], nums[j]))

Since you only use i and j here to retrieve a and b, why not save them in n_map in the first place?

n_map = defaultdict(list)
for i, a in enumerate(nums):
    for j, b in enumerate(nums[i + 1 :], i + 1):
        n_map[a + b].append((j, a, b))
res = set()
for k, c in enumerate(nums):
    for j, a, b in n_map[target - c]:
        result = c, a, b
        if k > j:
            ...

res and yield

Defining res as a set is a good choice. I think it is easier to only add the tuple to res if it is not present yet, and yield it at the same time, instead of returning list(map(list, res)) at the end

In total this gives:

def three_sum_maarten(nums, target=0):
    """
    :type nums: List[int]
    :rtype: List[List[int]]In total this gives
    """
    if len(nums) < 3:
        return []
    n_map = defaultdict(list)
    nums = sorted(nums)
    for i, a in enumerate(nums):
        for j, b in enumerate(nums[i + 1 :], i + 1):
            n_map[a + b].append((j, a, b))
    res = set()
    for k, c in enumerate(nums):
        for j, a, b in n_map[target - c]:
            result = c, a, b
            if k > j and result not in res:
                yield [c, a, b]
                res.add(result)

With this leetcode boilerplate:

class Solution:
    def threeSum(self, nums: 'List[int]') -> 'List[List[int]]':
        return list(three_sum_maarten(nums))

This passes all but one scenario. The scenario it fails is nums = [0] * 3000

To tackle this scenario, you can filter all numbers so only maximum 3 of each are present in nums. I do this with the help of a collections.Counter:

def prepare_nums(nums):
    counter = Counter(nums)

    for n, c in sorted(counter.items()):
        yield from [n] * min(c, 3)

and then nums = list(prepare_nums(nums)) instead of nums = sorted(nums)

Alternative approach

You make about half of all combinations of 2 numbers in nums. One extra bit of knowledge you can use to reduce this is to take into account that at least 1 negative and 1 positive number need to be present in each triplet.

counter = Counter(nums)
positives = [i for i in counter if i > 0]
negatives = [i for i in counter if i < 0]

for a, b in product(positives, negatives):
    c = -(a + b)
    if c not in counter:
        continue
    result = a, b, c

and then only yield the correct, unique results

    result = a, b, c
    if c == a:
        if counter[a] >= 2:
            yield result
    elif c == b:
        if counter[b] >= 2:
            yield result
    elif a > c > b:
        yield result

and yield 1 (0, 0, 0) triplet if there are 3 or more 0s present

if counter[0] >= 3:
    yield (0, 0, 0)

This solution is about 10 times faster, and uses 30 times less memory.


HackerRank "Lists" code I completed the following exercise on HackerRank. My program is fed a list of commands, such as:

12
insert 0 5
insert 1 10
insert 0 6
print
remove 6
append 9
append 1
sort
print
pop
reverse
print

Where the first line of the input is the number of commands, the rest of the lines commands and parameters.

Here is my code, nothing too fancy; any and all comments welcome!

def commdr(lst, instruct):
    if instruct[0] == 'insert':
        lst.insert(int(instruct[1]), int(instruct[2]))
    elif instruct[0] == 'print':
        print(lst)
    elif instruct[0] == 'remove':
        lst.remove(int(instruct[1]))
    elif instruct[0] == 'append':
        lst.append(int(instruct[1]))
    elif instruct[0] == 'sort':
        lst.sort()
    elif instruct[0] == 'reverse':
        lst.reverse()
    elif instruct[0] == 'pop':
        lst.pop()
    else:
        print("Command not recognized!")

n = int(input())
lst = []
for command in range(0,n):
    temp = [str(i) for i in input().strip().split()]
    commdr(lst, temp)

You have the right idea.  There are a few details that could be tightened up.

In the main driver:

command is not really what I would consider a command; it's just a useless counter.  By convention, unused variables should be named _.
temp is always a poor variable name.  In this case, command would be a better name than temp.  However, I don't think you need to give it a name at all — just write input().split().  The .strip() is superfluous, since .split() will discard leading and trailing whitespace anyway.  You don't need to convert strings to strings using str().

Naming and parameter-passing for the commdr() function could be improved as well.  Here's how I would write it:

def execute(lst, cmd, *args):
    if cmd == 'insert':
        lst.insert(int(args[0]), int(args[1]))
    elif cmd == 'print':
        print(lst)
    elif cmd == 'remove':
        lst.remove(int(args[0]))
    elif cmd == 'append':
        lst.append(int(args[0]))
    elif cmd == 'sort':
        lst.sort()
    elif cmd == 'reverse':
        lst.reverse()
    elif cmd == 'pop':
        lst.pop()
    else:
        print("Command not recognized!")

lst = []
for _ in range(int(input())):
    execute(lst, *input().split())

Bonus golfed solution

If you have absolutely no concern for validation or security, you could take advantage of the fact that most of the commands look just like existing methods in Python's list.

class ListInterp(list):
    def print(self):
        print(self)

    def execute(self, cmd, *args):
        getattr(self, cmd)(*map(int, args))

lst = ListInterp()
for _ in range(int(input())):
    lst.execute(*input().split())

I'm not saying that this is good code… I just thought I would mention that there is a quick-and-dirty solution.


Caffeine-Driven FizzBuzz I have been given following problem to solve:

If the integer is divisible by 3, return the string "Java".
If the integer is divisible by 3 and divisible by 4, return the string "Coffee"
If the integer is one of the above and is even, add "Script" to the
end of the string. Otherwise, return the string "mocha_missing!"

Sample Input and Output

caffeineBuzz(1) => "mocha_missing"
caffeineBuzz(3) => "Java"
caffeineBuzz(6) => "JavaScript"
caffeineBuzz(12) => "CoffeeScript"

//Class

    public class CoffieScriptGenerator {

        public String caffeineBuzzz(Integer number) {

            if(isDivisbleByThreeAndFour(number))
            {
                if(isEven(number))
                return "CoffeeScript";
                else
                return "Coffee";
            }
            if(isDivisbleByThree(number))
            {
                if(isEven(number))
                return "JavaScript";
                else
                return "Java";
            }
            return "mocha_missing!";

        }

        private boolean isEven(Integer number) {
            return number%2==0;
        }

        private boolean isDivisbleByThreeAndFour(Integer number) {

            return isDivisbleByThree(number) && number%4==0;
        }

        private boolean isDivisbleByThree(Integer number) {
            return number%3==0;
        }

    }

Please provide suggestions to refactor it and improve it and how i can improve such nesting if else with help of patterns like strategy.

Your private one-liner functions do very little to help with anything, and they're not really DRY. Might as well do this:

private boolean isDivisibleBy(Integer number, Integer divideBy) {
    return number % divideBy == 0;
}

As you can see this does nothing much more than wrap the modulo operator with a function call... which is questionable. Notice:

"Divisible" - not sure why your functions lost an i in the process.
Spacing. There's no need to cram operators and operands together into as little horizontal spaces as possible - let code breathe. Especially if the reason for having these functions in the first place, is to improve readability!

Your code doesn't comply with the specs: it's not adding "Script" to the end of the string when the number is even. In fact, your logic is redundant:

if(isDivisbleByThreeAndFour(number)) {
    ...
}

if(isDivisbleByThree(number)) {
    ...
}

Notice:

Both blocks check if number can be divided by 3.
Consistent brace position. If you're going to put scope-opening braces at the end of the line, keep 'em that way - don't switch to C#-style next-line bracing in the middle of your code!

Your bracing style not only inconsistent, the braces themselves are inconsistently present!

    if(isEven(number))
    return "CoffeeScript";
    else
    return "Coffee";

Don't do that. Use braces, and indentation. For your own health.

if(isEven(number)) {
    return "CoffeeScript";
} else {
    return "Coffee";
}

Note, when you have if (x) { return foo; } else { return bar; }, it can be rewritten with a ternary operator, like so:

return x ? foo : bar;

But you don't want to do that here.

You don't want that, because you'll need nested ternaries to remove the redundancies, and that would look terrible.

What would the correct logic be then?

Just follow the specs:

Check to see if it divides by 3 - if it does:

If it also divides by 4, we got "Coffee". Else, we got "Java".
If it also divides by 2, append "Script" to whatever we got.

Otherwise, return "mocha_missing!"

The nesting here should give you a clue: you only need to check once if the number can divide by 3.

String value;
if (number % 3 == 0) {
    if (number % 4 == 0) {
        value = "Coffee";
    } else {
        value = "Java";
    }
    if (number % 2 == 0) {
        value += "Script";
    }
} else {
    value = "mocha_missing!";
}

return value;

Strategy Pattern seems very much overkill here. If this is for an interview, I'd say don't overthink it - they're just trying to see if you can read specs.


One consumer and two producers using threads I have based this code on Producer-Consumer Threading on Real Python.

The idea is that I have 2 producers:

One producer  will give result every 0.4 to 0.7 seconds, depending on REST call.
The second producer will be a web socket that will forward filtered information to consumer.

And one consumer that needs to act on results from producers.

This code is working fine, but I'd like a second opinion on what I can improve from design/architecture side.

import concurrent.futures
import logging
import queue
import random
import threading
import time

def producer_1(queue_1, event_1):
    """Pretend we're getting a number from the network."""
    while True:
        time.sleep(1)
        message = random.randint(1, 101)
        logging.info("Producer_1 got message: %s", message)
        queue_1.put(message)
        event_1.set()

    logging.info("Producer_1 Exiting")

def producer_2(queue_2, event_2):
    """Pretend we're getting a number from the network."""
    while True:
        time.sleep(0.5)
        message = random.randint(1, 101)
        logging.info("Producer_2 got message: %s", message)
        queue_2.put(message)
        event_2.set()

    logging.info("Producer_2 Exiting")

def consumer(queue_1, queue_2, event_1):
    """Pretend we're saving a number in the database."""

    while True:
        event_1.wait()
        logging.info("Consumer START")

        if not queue_1.empty():
            message = queue_1.get()
            logging.info("Consumer storing message_1: %s (size=%d)", message, queue_1.qsize())

            if queue_1.empty():
                event_1.clear()
                logging.info("Queue_1 EMPTY")

        if not queue_2.empty():
            message = queue_2.get()
            logging.info("Consumer storing message_2: %s (size=%d)", message, queue_2.qsize())

            if queue_2.empty():
                event_1.clear()
                logging.info("Queue_2 EMPTY")

        logging.info("Consumer END")

    logging.info("Consumer Exiting")

if __name__ == "__main__":
    format = "%(asctime)s: %(message)s"
    logging.basicConfig(format=format, level=logging.INFO,
                        datefmt="%H:%M:%S")

    pipeline_1 = queue.Queue(maxsize=10)
    event_1 = threading.Event()

    pipeline_2 = queue.Queue(maxsize=10)

    with concurrent.futures.ThreadPoolExecutor(max_workers=3) as executor:
        #executor.submit(consumer, pipeline_1, event_1, pipeline_2, event_2)
        executor.submit(consumer, pipeline_1, pipeline_2, event_1)
        executor.submit(producer_1, pipeline_1, event_1)
        executor.submit(producer_2, pipeline_2, event_1)

        logging.info("Main: about to set event")

Logging after an infinite loop

The only way that this:

while True:
    time.sleep(1)
    message = random.randint(1, 101)
    logging.info("Producer_1 got message: %s", message)
    queue_1.put(message)
    event_1.set()

is going to terminate is on an exception. That in itself isn't a bad thing - stop iteration exceptions are a common pattern in Python. However, it means that this line:

logging.info("Producer_1 Exiting")

can by definition never execute. You might want to try:

try:
    while True:
        time.sleep(1)
        message = random.randint(1, 101)
        logging.info("Producer_1 got message: %s", message)
        queue_1.put(message)
        event_1.set()
finally:
    logging.info("Producer_1 Exiting")

main method

Putting code in this block:

if __name__ == "__main__":

doesn't remove it from global scope. For that, you need to add a main function.


Class that handles header and footer includes looks like a mess I tried making a class that handles my header and footer of my site. It looks like a mess, because I like to preserve the indentations as is.

But I'm wondering if there maybe is an even cleaner or better way of writing this class?

<?php
class HeadFoot {
    protected $title,
              $scripts;

    public function __construct($title, $scripts) {
        $this->title = $title;
        $this->scripts = $scripts;
    }

    public function getHeader() {
        if($this->title) {
$header = <<< EOD
<!DOCTYPE html>

        <meta charset="UTF-8">
        <title>$this->title - Domain</title>
        <link rel="shortcut icon" href="/favicon.ico">
        <link rel="stylesheet" href="/stylesheets/init.css">
        <link rel="stylesheet" href="/stylesheets/layout.css">

EOD;

if($this->scripts) {
    foreach($this->scripts as $script) {
        $header .= str_repeat(' ', 8) . $script . "\n";
    }
}

$header .= <<< EOD

Domain

EOD;

$header .= Navigation::get();
$header .= str_repeat(' ', 12) . "\n";

            return $header;
        }

        return false;
    }

    public function getFooter($ads = null) {
        if($ads) {
            return
'

Copyright © 2014 - Domain.net

Terms of Service / Privacy Policy

';
        }

        return
'

Copyright © 2014 - Domain.net

Terms of Service / Privacy Policy

';
    }
}

Separation of Concerns

You should implement Separation of Concerns. In your case, don't mix logic with your view. Follow basic MVC, where M (model) is the data source, V (view) is where your HTML is, and C (controller), where all your logic is. In this case, put your HTML in one file, the logic in the other.

Templating

One thing that messes up development is when logic is mixed to the template. It makes the template messy and hard to maintain and visualize. I suggest you take a look at the PHP implementation of Mustache templates. In your controller, simply make a function that loads the template to a variable, feeds it to Mustache, put some data in and done.

Inheritance

One use of inheritance is to hide utility functions. For instance, you can create a View class, where your future views inherit from.

End Result

You can simply do something as simple as this in the controller!

public function index(){
    $myView = new View('path/to/template');     // Create a view
    $myDataSource = new Model('MODEL_NAME');    // Create a model
    $theData = myDataSource->getData();         // Grab data
    $theResultHTML = myView->render($theData);  // Render data to template and print
}


Randomly sampling a population and keeping means: tidy up, generalize, document? This is part from an answer to a Stack Overflow question. The OP needed a way to perform calculations on samples from a population, but was hitting memory errors due to keeping samples in memory.

The function is based on part of random.sample, but only the code branch using a set is present.

If we can tidy and comment this well enough, it might be worth publishing as a recipe at the Python Cookbook.

import random

def sampling_mean(population, k, times):
    # Part of this is lifted straight from random.py
    _int = int
    _random = random.random

    n = len(population)
    kf = float(k)
    result = []

    if not 0 <= k <= n:
        raise ValueError, "sample larger than population"

    for t in xrange(times):
        selected = set()
        sum_ = 0
        selected_add = selected.add

        for i in xrange(k):
            j = _int(_random() * n)
            while j in selected:
                j = _int(_random() * n)
            selected_add(j)
            sum_ += population[j]

        # Partial result we're interested in
        mean = sum_/kf
        result.append(mean)
    return result

sampling_mean(x, 1000000, 100)

Maybe it'd be interesting to generalize it so you can pass a function that calculates the value you're interested in from the sample?

Making a generator version of random.sample() seems to be a much better idea:

from __future__ import division
from random import random
from math import ceil as _ceil, log as _log

def xsample(population, k):
    """A generator version of random.sample"""
    n = len(population)
    if not 0 <= k <= n:
        raise ValueError, "sample larger than population"
    _int = int
    setsize = 21        # size of a small set minus size of an empty list
    if k > 5:
        setsize += 4 ** _ceil(_log(k * 3, 4)) # table size for big sets
    if n <= setsize or hasattr(population, "keys"):
        # An n-length list is smaller than a k-length set, or this is a
        # mapping type so the other algorithm wouldn't work.
        pool = list(population)
        for i in xrange(k):         # invariant:  non-selected at [0,n-i)
            j = _int(random() * (n-i))
            yield pool[j]
            pool[j] = pool[n-i-1]   # move non-selected item into vacancy
    else:
        try:
            selected = set()
            selected_add = selected.add
            for i in xrange(k):
                j = _int(random() * n)
                while j in selected:
                    j = _int(random() * n)
                selected_add(j)
                yield population[j]
        except (TypeError, KeyError):   # handle (at least) sets
            if isinstance(population, list):
                raise
            for x in sample(tuple(population), k):
                yield x

Taking a sampling mean then becomes trivial:

def sampling_mean(population, k, times):
    for t in xrange(times):
        yield sum(xsample(population, k))/k

That said, as a code review, not much can be said about your code as it is more or less taking directly from the Python source, which can be said to be authoritative. ;) It does have a lot of silly speed-ups that make the code harder to read.


LINQ or Lambda for two foreach query I have a Document Name Table in my SQL database and Files in the Content folder.
I have two lists: ListOfFileNamesSavedInTheDB and ListOfFileNamesInTheFolder.

Basically, I am getting all file names saved in the Database and checking if it exists in the Folder, and if not, delete the file name from the database.

 var clientDocList = documentRepository.Documents.Where(c => c.ClientID == clientID).ToList();

 if (Directory.Exists(directoryPath))
            {
                string[] fileList = Directory.GetFiles(directoryPath).Select(Path.GetFileName).ToArray();
                bool fileNotExist = false;
                foreach (var file in fileList)
                {
                    foreach(var clientDoc in clientDocList)
                    {
                        if (clientDoc.DocFileName.Trim().ToUpper()==file.ToUpper().Trim())
                        {
                            fileNotExist = false;
                            break;
                        }
                    }

                    if (fileNotExist)
                    {
                        //Delete file name from Database
                    }
                }
            }

The code I have written works fine, this inquiry being purely for educational purposes. I want to know how others would do this better and cleaner. I especially hate the way I use two for loops to get data. There has to be a more efficient way.

First thing I'd suggest is make the 2 lists the same type(string).  Now the file list can be an exclusion list that contains only the filenames that aren't in the database.  Something like this should work:

    var clientDocList = documentRepository
        .Documents
        .Where(c => c.ClientID == clientID)
        .Select(d => d.DocFileName
        .Trim()
        .ToUpper()).ToList();

    var fileExclusionList = Directory
        .GetFiles(directoryPath)
        .Select(x => Path.GetFileName(x)
        .Trim()
        .ToUpper())
        .Except(clientDocList).ToArray();


Using Grand Central Dispatch to fetch data from Parse.com and update UI asynchronously This works, and the UI is snappy in the simulator, but since this is my first time really using GCD, I'd just like this code to be reviewed before I start using it everywhere.  Note that this is inside a PFQueryTableViewController.

My function:

  func tableRefresh() {

        // get quality of service (high level)
        let qos = Int(QOS_CLASS_USER_INITIATED.value)

        // get global queue
        dispatch_async(dispatch_get_global_queue(qos, 0)) { () -> Void in

            //execute slow task
            self.queryForTable()

        // get main queue, do UI update
            dispatch_async(dispatch_get_main_queue()) {
                self.loadObjects()
            }

        }

    }

My function in action:

@IBAction func done(segue: UIStoryboardSegue) {

        let addPersonViewController = segue.sourceViewController as! AddPersonViewController

        // If I get a person back from APVC
        if let person = addPersonViewController.person {

            // If that person has a name
            if let name = person.name {

            // Note: the reference to current user creates the pointer
            let newPerson = Person(name: name, user: PFUser.currentUser()!)

            // Save
            newPerson.saveInBackgroundWithBlock() { succeeded, error in
                if succeeded {

                    println("\(newPerson) was Saved")

                    self.tableRefresh()

                } else {

                    if let errorMessage = error?.userInfo?["error"] as? String {
                        self.showErrorView(error!)
                        }
                    }
                }
            }

        }

    }

First off, I don't know Swift.  These are generic issues that should be addressed in all written code, and I'll leave the Swift-specific stuff to the experts.

Comments

//execute slow task

Comments should say why the code is the way it is, not a generic comment that doesn't tell us anything constructive.  If there is an issue with the code being slow, document why it is slow, and maybe state why you haven't been able to fix it for future reference.

Spacing

    }

}

We definitely don't need that much whitespace hogging our screens.  It forces code off the screen, it increases scrolling, and generally slows us down when reading and understanding the code.

Indentation

} else {

    if let errorMessage = error?.userInfo?["error"] as? String {
        self.showErrorView(error!)
        }
    }

Your indentation is off there.  Each closing brace should have the same level indentation as the opening brace, unless they are on the same line, which is rare.

Naming

Your names should state what the function does or variable is, and only what the function does or variable is, and the variable/function shouldn't be/do anything but what the name says it is/does.

done() tells me nothing about what the function does.

qos doesn't tell me what the variable does, what it contains, or what it is used for.

self.loadObjects() appears to do UI updates, according to the comment.  Loading an object is not the same as displaying it, and loading and displaying from a single method violates the Single Responsibility Principle.


General static value required by multiple classes I have a general value called Seed which is required to multiple class. But those class aren't dependent of the class which declare it.

class Game {
    class Settings {
       public static float Seed = 1337;
    }
}

And some class need a Seed value to work. I take the class Noise as example here.

class Noise {
    private static float _seed;
}

A solution would be to use Game.Settings.Seed to know the value into the class Noise. But what if one day I decide to take all the Noise class to use it in an other project. The code won't compile because there is probably not a Game.Settings.Seed declared in the other project obviously.

A other way would be to set as public field in the Noise class.

class Noise {
    public static float Seed;
}

So in this way, Game which know he need to use Noise class, will set Seed field before doing anything with this class.

But I don't like it too, in this case this is about a float, so that not really a problem, but what about an object that need to be declared, which means Noise class need to implement a verification about itself.

class Noise {

    private static float _seed;
    private static bool _isSeedDeclared;

    public static void SetSeed(int seed){
        _seed = seed;
        _isSeedDeclared = true;
    }

    public int GetValue(int x, int z){
        CheckSettings();
        return computedValue; //to calculate computedValue, I need Seed value
    }

    private void CheckSettings(){
        if(!_isSeedDeclared)
            throw new Exception("You have to define Seed value.");
    }
}

This solution works but I'm not pleased about it. For non-nullable type I have to define an other variable to check, for a nullable class I can compare it to null. But anyway, I don't have only one parameter and I don't have only one class. I'm looking for an better way to implement it.

If the seed parameter is required for the Noise class to work properly, it must go to the constructor as a required parameter. If an invalid seed passed to the constructor, it should throw an ArgumentException explaining what's wrong with the seed and (ideally) what the expected value could look like (e.g. if there are some range rules take place in your domain).

Consumers of the Noise class will have to inject the proper value (likely taking it from the settings).


Multi Producer - Single Consumer I have written multi producer single consumer code with bounded buffer. Please review the code for any improvements or mistakes.

#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>
#include <unistd.h>
#include <semaphore.h>
#include <string.h>
#define SIZE 50
#define PRODUCERS 5
#define TOTAL 2
#define ENTER printf("\n-- %s -- %d --", __func__, __LINE__);

char (*buf_write)[20], (*buf_read)[20];
char act_buf[2][SIZE][20];

pthread_mutex_t  lock = PTHREAD_MUTEX_INITIALIZER;
pthread_mutex_t     mutex = PTHREAD_MUTEX_INITIALIZER;
pthread_cond_t      cond  = PTHREAD_COND_INITIALIZER;

pthread_barrier_t barrier;

int write_index = 0, writer_cnt = 0;
void init();

/*
 * Consumer
 */

void *consumer(void *vargp)
{

    int i = 0, index = 0;

    while (1) {

        pthread_mutex_lock(&mutex);
        if (writer_cnt != PRODUCERS)
            pthread_cond_wait(&cond, &mutex);
        pthread_mutex_unlock(&mutex);

        writer_cnt = 0;
        write_index = 0;
        buf_read = &act_buf[index];
        index = index + 1;
        index = index % TOTAL;

        buf_write = &act_buf[index];
        pthread_barrier_wait(&barrier);

        i = 0;
        while (i < SIZE) {
            printf("%s\t",buf_read[i]);
            i++;
            if (i%10 == 0)
                printf("\n");
        }
        printf("\n");
        memset(buf_read, 0, sizeof(act_buf) / 2);
    }
}

/*
 * Producer
 */
void *producer(void *vargp)
{
    int i = 0;
    char *str = (char *) vargp;

    while (1) {

          i = 0;
        while (i < 10) {
            pthread_mutex_lock(&lock);
            str[0] = '0' + i;
            strcpy(buf_write[write_index], str);
            ++write_index;
            pthread_mutex_unlock(&lock);
            ++i;
        }

        pthread_mutex_lock(&mutex);
            writer_cnt++;
            if (writer_cnt == PRODUCERS)
                pthread_cond_signal(&cond);
        pthread_mutex_unlock(&mutex);

        pthread_barrier_wait(&barrier);
    }
}

int main()
{

    int i = 0;
    pthread_t prouder_tid[PRODUCERS], consumer_tid;

    char str[5][20] = {" -producer1"," -producer2"," -producer3"," -producer4"," -producer5"};

    buf_write = &act_buf[0];

    init();

    for (i = 0; i < PRODUCERS; i++) {
        pthread_create(&prouder_tid[i], NULL, producer, &str[i]);
    }

    pthread_create(&consumer_tid, NULL, consumer, NULL);

    pthread_join(consumer_tid, NULL);

    for (i = 0; i < PRODUCERS; i++) {
        pthread_join(prouder_tid[i], NULL);
    }

    return 0;
}

void init()
{
    //pthread_mutex_init(&lock, NULL);
    //pthread_mutex_init(&writer_lock, NULL);
    int s = 0;

    s = pthread_barrier_init(&barrier, NULL, PRODUCERS + 1);

    if (s != 0) {
            printf("pthread_barrier_init failied");
        exit(-1);
    }
}

Tabs Vs Spaces

Your code formatting isn't consistent.  Looking at your code, this appears to be because you're using both tabs and spaces for white space.  Which is why you end up with code that looks like this (notice the printf doesn't line up with the exit):

if (s != 0) {
        printf("pthread_barrier_init failied");
    exit(-1);
}

Different people prefer one or the other, but don't mix and match pick one and stick with it.

Termination

It's unclear to me how you're expecting your threads to exit.  Both your consumers and your producers go into while(true) loops, which don't have any break conditions in them.  This is inconsistent with your main, which attempts to join with the consumer and producer threads.

Pointer Assignment

My compiler isn't a huge fan of this line:

buf_write = &act_buf[index]

buf_write is char(*buf_write)[20] and you're trying to assign it to char*[50][20].  I think you should probably be doing this:

buf_write = &act_buf[index][0]

Synchronisation

I think overall, the way you've synchronised between the consumer and producers is fairly tidy, although your variable names could be improved to describe what their function is.  mutex is clearly a mutex, but what's it being used for, how is it different from lock?  For the scenario you've built, having the consumer wait until all of the producers have completed seems to work ok, however in the general case, where producers might take different amounts of time it might make more sense for the consumer to handle information from each producer as it is complete.


Hangman program w/ Python and JSON What improvements can I make? Should I organize my functions based on A-Z? For example, find_letter function comes before guessing function.

import json
import random
import re

def find_letter(user_input, vocab, i=None):
    return [i.start() for i in re.finditer(user_input, vocab)]

def guessing(tries,vocab,blank_line):
    while tries > 0:
        user_input = input("Guess:")

        if user_input in vocab:
            if not(user_input in blank_line):
                index = find_letter(user_input=user_input,vocab=vocab)
                i = 0
                while i < len(index):
                    blank_line[index[i]] = user_input
                    i += 1

                if not('_' in blank_line):
                    return "you won!!!"
            else:
                print("YOU ALREADY TRIED THAT LETTER!")
        else:
            tries -= 1

        print(str(blank_line) + "  Number of Tries : " + str(tries))

    return "you lost"

def hangman_game(vocab):
    'Preliminary Conditions'
    print(introductory(vocab=vocab))
    tries = num_of_tries(vocab=vocab)
    blank_line = produce_blank_lines(vocab=vocab)
    print(blank_line)

    print(guessing(tries=tries,vocab=vocab,blank_line=blank_line))

def introductory(vocab):
    return "HANGMAN GAME. So...what you gotta do is guess and infer what the word might be. " \
           "WORD COUNT : " + str(len(vocab))

def num_of_tries(vocab):
    if len(vocab) < 7:
        return 8
    elif 7 <= len(vocab) < 10:
        return 10
    else:
        return 14

def produce_blank_lines(vocab):
    blank_line = []
    for i in vocab:
        if i.isalpha():
            blank_line += "_"
        elif i == " ":
            blank_line += " "
        else:
            blank_line += i + " "

    return blank_line

if __name__ == '__main__':
    data = json.load(open("vocabulary.json"))
    vocab = random.choice(list(data.keys())).lower()
    print(vocab)
    print(hangman_game(vocab=vocab))

Snippet of the JSON File

{"abandoned industrial site": ["Site that cannot be used for any purpose, being contaminated by pollutants."], "abandoned vehicle": ["A vehicle that has been discarded in the environment, urban or otherwise, often found wrecked, destroyed, damaged or with a major component part stolen or missing."], "abiotic factor": ["Physical, chemical and other non-living environmental factor."], "access road": ["Any street or narrow stretch of paved surface that leads to a specific destination, such as a main highway."], "access to the sea": ["The ability to bring goods to and from a port that is able to harbor sea faring vessels."], "accident": ["An unexpected, unfortunate mishap, failure or loss with the potential for harming human life, property or the environment.", "An event that happens suddenly or by chance without an apparent cause."], "accumulator": ["A rechargeable device for storing electrical energy in the form of chemical energy, consisting of one or more separate secondary cells.\\n(Source: CED)"], "acidification": ["Addition of an acid to a solution until the pH falls below 7."], "acidity": ["The state of being acid that is of being capable of transferring a hydrogen ion in solution."], "acidity degree": ["The amount of acid present in a solution, often expressed in terms of pH."], "acid rain": ["Rain having a pH less than 5.6."]

data = json.load(open("vocabulary.json"))

Unless Python 3 has changed this, I think you're leaking the open file handle here. What you want is more like

with open("vocabulary.json") as f:
    data = json.load(f)

which IMHO is even a bit easier to read because it breaks up the mass of nested parentheses.

vocab = random.choice(list(data.keys())).lower()

Same deal with the nested parentheses here. We can save one pair by eliminating the unnecessary materialization into a list. [EDIT: Graipher points out that my suggestion works only in Python 2. Oops!]

Also, vocab (short for vocabulary, a set of words) isn't really what this variable represents. data.keys() is our vocabulary; what we're computing on this particular line is a single vocabulary word. So:

word = random.choice(data.keys()).lower()
print(word)
print(hangman_game(word))

I can tell you're using Python 3 because of the ugly parentheses on these prints. ;) But what's this? Function hangman_game doesn't contain any return statements! So it basically returns None. Why would we want to print None to the screen? We shouldn't be printing the result of hangman_game; we should just call it and discard the None result.

word = random.choice(data.keys()).lower()
print(word)
hangman_game(word)

def hangman_game(vocab):

Same comment about vocab versus word here.

'Preliminary Conditions'

This looks like you maybe forgot a print? Or maybe you're trying to make a docstring? But single quotes don't make a docstring AFAIK, and even if they did, "Preliminary Conditions" is not a useful comment. I'd just delete this useless line.

print(introductory(vocab=vocab))

In all of these function calls, you don't need to write the argument's name twice. It's a very common convention that if you're calling a function with only one argument, the argument you pass it is precisely the argument it needs. ;) Also, even if the function takes multiple parameters, the reader will generally expect that you pass it the right number of arguments in the right order. You don't have to keyword-argument-ize each one of them unless you're doing something tricky that needs calling out. So:

print(introductory(word))

And then you can inline introductory since it's only ever used in this one place and it's a one-liner already:

print(
    "HANGMAN GAME. So...what you gotta do is guess and infer what the word might be. "
    "WORD COUNT : " + str(len(vocab))
)

Personally, I would avoid stringifying and string-concatenation in Python, and write simply

print(
    "HANGMAN GAME. So...what you gotta do is guess and infer what the word might be. "
    "WORD COUNT : %d" % len(vocab)
)

I know plenty of working programmers who would write

print(
    "HANGMAN GAME. So...what you gotta do is guess and infer what the word might be. "
    "WORD COUNT : {}".format(len(vocab))
)

instead. (I find that version needlessly verbose, but it's definitely a popular alternative.)

tries = num_of_tries(vocab=vocab)
blank_line = produce_blank_lines(vocab=vocab)
print(blank_line)
print(guessing(tries=tries,vocab=vocab,blank_line=blank_line))

It's odd that you abbreviated number_of_tries to num_..., but then left in the word of. I would expect either number_of_tries or num_tries (or ntries for the C programmers in the audience); num_of_tries is in a weird no-man's-land.

It's weird that you have a function named produce_blank_lines that produces actually a single blank_line. It's even weirder that the value of blank_line is not a constant "\n"! This suggests that everything here is misnamed. You probably meant underscores = produce_underscores(word) or even blanks = shape_of(word).

Finally, it is weird that blank_line is not even a string, but rather a list of characters.

I would replace this entire dance with something like

blanks = ''.join('_' if c.isalpha() else ch for ch in word)

or

blanks = re.sub('[A-Za-z]', '_', word)

This would lose your addition of a blank space after each non-space-non-alpha character, but IMHO that's actually an improvement over your version. (Your sample JSON doesn't show any examples of words containing non-space-non-alpha characters. My version certainly performs better for words like non-space or Route 66.)

if not(user_input in blank_line):

This would be idiomatically expressed (both in Python and in English) as

if user_input not in blank_line:

index = find_letter(user_input=user_input,vocab=vocab)
i = 0
while i < len(index):
    blank_line[index[i]] = user_input
    i += 1

You have another singular/plural problem here (just like with blank_line[s]). It doesn't make sense to get the len of a singular index. Since the variable actually represents a list of indices, we should name it [list_of_]indices.

Also, this style of for-loop is very un-Pythonic. In Python we can say simply

for i in find_letter(user_input, word):
    blank_line[i] = user_input

And then we can inline the single-use find_letter:

for m in re.finditer(user_input, word):
    blank_line[m.start()] = user_input

(Using m as the conventional name for a match object.)
And then we can eliminate the regex and just loop over the word directly:

for i, ch in enumerate(word):
    if ch == user_input:
        blank_line[i] = user_input

or

for i in range(len(word)):
    if word[i] == user_input:
        blank_line[i] = user_input

In fact, by not using regexes, we fix a bug in your code: if the chosen vocabulary word contains . as one of its non-space-non-alpha characters, a user who enters . as their guess will win the game immediately! :D

Anyway, that's enough for one answer, I think.


Generating a table of differences between two dictionaries for (int i = 0; i < keyList.Count; i++)
{
    if (oldDic.ContainsKey(keyList[i].ToString()))
    {
        if (newDic[keyList[i].ToString()].ToString() == oldDic[keyList[i].ToString()].ToString())
        {
            //ReminderBackupLog("Same");
        }
        else
        {
            isChnagedSectionFields = "Yes";
            string oldValue = oldDic[keyList[i].ToString()].ToString();
            string newValue = newDic[keyList[i].ToString()].ToString();
            table = table + "
" + colNames[keyList[i].ToString()].ToString() + "	" + oldValue + "	" + newValue + "
";
        }
    }
    else
    {
        isChnagedSectionFields = "Yes";
        string newValues = newDic[keyList[i].ToString()].ToString();
        table = table + "
" + colNames[keyList[i].ToString()].ToString() + "	" + " " + "	" + newValues + "
";
    }
}

You should separate the logic from your representation and first collect those differences in a separate list. Also, what are the types of your dictionaries? All those ToString()s make it really noisy and are likely not necessary. Furthermore, use a foreach loop to iterate through your key list, this will also automatically get rid of all those keyList[i] accesses. And finally, instead of appending to a string directly, use a StringBuilder to make it more efficient:

// Use a separate type to collect your diffs
class DiffEntry
{
    public string ColumnName { get; set; }
    public string OldValue { get; set; }
    public string NewValue { get; set; }
}

// collect all diffs
List<DiffEntry> diffs = new List<DiffEntry>();
foreach (string key in keyList)
{
    if (oldDic.ContainsKey(key))
    {
        if (newDic[key] != oldDic[key])
        {
            diffs.Add(new DiffEntry()
            {
                ColumnName = colNames[key],
                OldValue = oldDic[key],
                newValue = newDic[key]
            });
        }
    }
    else
    {
        diffs.Add(new DiffEntry()
        {
            ColumnName = newDic,
            NewValue = newDic[key]
        });
    }
}

// generate the actual table output
StringBuilder tableContent = new StringBuilder();
foreach (DiffEntry diff in diffs)
{
    tableContent.Append("
");
    tableContent.Append("");
    tableContent.Append(diff.ColumnName);
    tableContent.Append("	");
    tableContent.Append(diff.OldValue);
    tableContent.Append("	");
    tableContent.Append(diff.NewValue);
    tableContent.Append("
");
}
table += tableContent.ToString();


jQuery menu state is-active I have this menu in jQuery that works perfectly, but I think it would be better if, instead of using class="btn-accordion" for each li, I just had a single class="accordion-menu".

Is that possible or is the code OK?

my html:

jquery

  $('.btn-accordion').on('click', function() {
        $(this).closest('li').toggleClass('is-active');
  });

scss:

.btn-accordion {
  color: red;
  background-color: blue;
  .fa {
    color: white;
  }
  &.is-active {
    color: blue;
    background-color: red;
    .fa {
      color: yellow;
    }
  }
}

This is ok since it's valid CSS anyways. However...

The problem with both approaches (.btn-accordion.is-active and .accordion-menu .is-active) is that they're least 0020 in specificity. Should you ever want to override, the override must at least have a specificity of 0020.

.btn-accordion {
  color: red;
}
.btn-accordion.is-active {
  color: blue;
}

/* overriding selector needs to be at least the same specificity */
.btn-accordion.is-active {
  color: green;
}

In the case where you can't control order of the styles, you can raise the specificity. However, this would equate to longer selectors and/or harder-to-override selectors.

.btn-accordion {
  color: red;
}

/* An override appearing earlier in the styles */
/* Specificity: 0021 */
li.btn-accordion.is-active {
  color: blue;
}

/* Specificity: 0020 */
.btn-accordion.is-active {
  color: green;
}

CSS tightly coupled to the HTML structure are fragile. Accidentally removing/renaming the class name, the selector becomes useless. If the HTML structure changes, the selectors also become useless.

Long chains of selectors are also hard to maintain. You do not know how deep a selector is relative to its parent/descendant selector. It's also hard to figure out if a combination of selectors is even used on the page, leading to excess fat on the CSS.

One way to avoid specificity climb is to use BEM naming scheme. This allows you to write selectors with just one class while being able to specify the containing block, block contents and modifiers.

With the following code, it allows me to know the accordion-menu__button belongs to accordion-menu set of elements and that it has a accordion-menu__button--is-active state. Since the selectors are just 1 class name, modifier selectors can predictably be placed after the selectors they modify. It also allows me to easily deduce that a style is applied since it doesn't depend on the HTML nesting structure.

/* Default state of the button */
.accordion-menu__button{
  color: red;
}

/* Override the color when the button is active */
.accordion-menu__button--is-active{
  color: green;
}

However, this would mean that every target element will have a class. But in most cases, especially in large apps, you will have templating mechanisms that will do all this repetitiveness for you and the tradeoff of writing this way vs manageability of the code is justifiable.


Remove every occurence of the number 13 and the next number from a list, then sum the result This function takes a list, deletes every occurrence of the number 13 and deletes the number present after 13 in every instance and then prints the sum of the list.

How can I shorten it?

def average(nums):
  thirteen_idx = []
  bad_idx = []
  final_list = []
  num_length = len(nums)-1

  #list of indexes where 13 resides
  for i in range(len(nums)):
    if nums[i] == 13:
      thirteen_idx.append(i)

  #make list of indexes where 13 resides and the index after it (bad_idx)
  for i in range(len(thirteen_idx)):
    bad_idx.append(thirteen_idx[i])
    bad_idx.append(thirteen_idx[i]+1)

  #delete any index from bad_idx that is greater than the highest index in nums
    if bad_idx[-1] > num_length:
        bad_idx.pop(len(bad_idx)-1)

  #we now have a list of all bad indexes (bad_idx)

  #iterate thru nums and move any non bad index into a final_list
  for i in range(len(nums)):
    if i not in bad_idx:
      final_list.append(nums[i])

  print (sum(final_list))

Your output is simply a sum of certain numbers. You don't need to build intermediary lists at all here, just select the numbers you want to sum, by looping. Your criteria are that the current number should not be 13, and the preceding number should not be 13.

To achieve this, you could just use a loop that remembers the preceding number:

initialize a preceding variable to a value you can easily test for. Here, any value other than 13 would do, and None is probably a good sentinel value.
In a loop over the numbers, if the current number is not 13 and the preceding number is not 13, add it to the running total.
At the end of the loop, set preceding to the current number. That way, when you go back to the top of the loop body, preceding will still contain the number from the preceding iteration.

So, given an input of [17, 13, 3, 9], you'd want to sum the first and the last number. We start with preceding set to None, result is set to 0 and we iterate 4 times:

preceding = None, set current to the first value 17.

preceding != 13 and current != 13 is true, so add the current value to the result -> result = 0 + 17 = 17.
set preceding = current, so 17

preceding = 17, set current to the second value 13.

preceding != 13 and current != 13 is false (current is set to 13), so skip this value.
set preceding = current, so 13

preceding = 13, set current to the third value 3.

preceding != 13 and current != 13 is false (preceding is set to 13), so skip this value.
set preceding = current, so 3

preceding = 3, set current to the forth value 9.

preceding != 13 and current != 13 is true, so add the current value to the result -> result = 17 + 9 = 26.
set preceding = current, so 9

and you end up with result = 26.

Generally speaking, when you find yourself wanting to look ahead in a for loop, turn the problem around and look behind instead. It is much simpler, as your loop will already have processed the items behind.

Translating those steps above to Python code looks like this:

def sum_without_13s(nums):
    """Sum all values that are not equal to 13 or directly follow 13"""

    # the running total of qualifying numbers
    result = 0
    # the preceding value in the loop, set at the end of the loop body
    preceding = None  # any value that is not 13 would do

    for value in nums:
        # only add to the sum if this value and the preceding value are both
        # *not* equal to 13.
        if value != 13 and preceding != 13:
            result += value

        # for the next iteration, remember the current value so we can
        # skip any values that followed 13.
        preceding = value

    return result

That's the most readable method of implementing your algorithm. The function produces the sum of integers not 13, and not directly following 13:

>>> sum_without_13s([13, 99, 3])  # only 3 fits criteria, so should produce 3
3
>>> sum_without_13s([99, 3])  # no 13s, so sum should be 102
102
>>> sum_without_13s([1, 2, 13, 13, 3, 4])  # only 1, 2, and 4 should be summed == 7
7

Note that the function also returns the result, rather than print the result to the console. If you want to see the output of your function, you can always print the returned value:

>>> theanswer = sum_without_13s([17, 13, 19, 18, 13, 13, 3, 7])
>>> print(theanswer)
42

Other looping constructs that give you access to the preceding value

You could in principle make use of itertools.chain() and zip() to produce an iterator of (preceding, current) tuples to loop over, and so make it possible to filter and sum in a generator expression.

The above 'preceding value' approach is a variant of the pairwise() function from the itertools recipes section. But where pairwise() starts with (nums[0], nums[1]) and ends at (nums[-2], nums[-1]),  we really need to be starting with (None, nums[0]) first because we wouldn't want to skip either the first or the last value in your input from consideration.

So here's a with_preceding() function, with a configurable default value for that first element of the first tuple:

from itertools import chain, tee

def with_preceding(iterable, default=None):
    "s -> (default, s0), (s0,s1), (s1,s2), (s2, s3), ..."
    a, b = tee(iterable)
    return zip(chain((default,), a), b)

which then can be used in an alternate implementation using a generator expression, like this:

def sum_without_13s_iter(nums):
    """Sum all values that are not equal to 13 or directly follow 13"""
    return sum(
        current
        for preceding, current in with_preceding(nums)
        if preceding != 13 and current != 13
    )

However, readability suffers, in my eyes, and for little gain as the above is actually slower than the first version. See the small benchmark at the very end.

Performance considerations

I tried to construct what I hoped would be a faster function using iterators, the itertools and operator modules and the sum(), map() and zip() functions to try and avoid the Python interpreter loop altogether, but this was simply not any faster; at best I could get it to be about 5-10% slower than the above first version of the function.

If performance is a concern, then you should really be using numpy arrays anyway. With a numpy array of integers as the input, the function can be expressed as:

import numpy as np

def array_sum_without_13s(numarray):
    """Sum all values in an array that are not equal to 13 or directly follow 13"""
    # boolean array with True where the input array has 13
    is13 = numarray == 13
    # boolean array with True where the input array is preceded by 13
    follows13 = np.concatenate(([False], is13[:-1]))
    # sum of all values that don't fit either of the above criteria
    return np.sum(numarray[~(is13 | follows13)])

As stated, this function expects a numpy array as the input:

>>> import numpy as np
>>> array_sum_without_13s(np.array([13, 99, 3]))
3
>>> array_sum_without_13s(np.array([99, 3]))
102
>>> array_sum_without_13s(np.array([1, 2, 13, 13, 3, 4]))
7
>>> array_sum_without_13s(np.array([17, 13, 19, 18, 13, 13, 3, 7]))
42

Given an input of 1 million integers, the numpy version takes ~3.5 milliseconds versus the pure-Python version taking ~89 milliseconds. Numpy is the numeric processing champion, hands down:

In [1]: import random
   ...: import numpy as np
   ...: from codereview237899 import sum_without_13s, array_sum_without_13s
   ...: testdata = random.choices(range(1_000), k=1_000_000)
   ...: testarray = np.array(testdata)
   ...:

In [2]: %timeit sum_without_13s(testdata)
88.7 ms ± 2.12 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

In [3]: %timeit array_sum_without_13s(testarray)
3.55 ms ± 81.7 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

In [4]: %timeit sum_without_13s_iter(testdata)
119 ms ± 13.5 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

The last test there is for the with_preceding() and generator expression variant above; it's a disappointing 35% slower.


Synchronous and asynchronous motor movement I am working on an API that is mostly intended to be used interactively/little scripts. There are some classes with methods that could act slightly differently depending on the intention of the user, I have explored several options and would like to get some feedback on caveats, and alternative options.

I will use a simple example:

class Motor(object):
    def __init__(self, name):
        self.name = name
        self._blocking = True

    def move_to(self, position):
        self.block_until_idle() # protects from conflict in any case
        #internal move call here
        if self._blocking:
            self.block_until_idle()
        else:
            self._blocking = True

    def block_until_idle(self):
        #internal status query loop

I want to use an instance of Motor in two ways (dictated by the value of _blocking attribute)

The movement call blocks until idle, because I want to take a picture at that position.
The movement call returns ASAP, because I want to do other things meanwhile (for example move a completely different motor).

There are many different calls (not only move_to) that would follow this pattern.

The wish: Usage like:

motor = Motor("A")
motor2 = Motor("B")

motor.move_to(20)
# > wait until done

noblock(motor.move_to(50))
# > returns immediately so I can move another
motor2.move_to(30)

So, in summary, the desire is that it is quick and fast to switch between behaviors, preferably within the same line of code and without lasting side effects.

Explored but discarded options:

Manually handling the flag variable: potentially dangerous (easy to forget), three liner every time.

Duplicate the methods into blocking and not blocking: API duplicates in size.

Context manager: Taking care of the flag and cleaning up the logic in the move_to method. Still a bit too verbose for my taste, specially for command line (I skip the context manager code). Note that the advantage of the context manager of being able to unblock many calls is not useful here, since the pre-blocking safety would kick in and only the last call would be affected. Also heavy to combine several unblocked motors.

with noblock(motor) as nbmotor:
    nbmotor.move_to(25)

Extra parameter: requires changes on each method, polluting call signature, still the best of the trivial options

def move_to(self, position, blocking = True):

Decoration / wrapper: I did not make it work so far in a way that would not scare users:

# syntax error, decorators do not work this way, and besides two liner
@noblock
motor.move_to(20)

# hi is evaluated before polite is applied
noblock(motor.move_to(15))

# almost, but problems to extract self inside the wrapper, the syntax starts to get tricky
noblock(motor.move_to) (20)

# this works, but awful!
noblock(Motor.move_to) (motor2, 15)

My best solution so far:

Some twisty implementation, but can be invoked in a single line.

class Motor(object):
    def __init__(self, name):
        self.name = name
        self._blocking = True
        self.noblock = Noblock(self)

    def move_to(self, position):
       self.block_until_idle() # protects from conflict in any case
       #internal move call here
       if self._blocking:
           self.block_until_idle()
       else:
           self._blocking = True

class Noblock(object):
    def __init__(self, other):
        self.other = other

    def __getattribute__(self, name):
        if name in ["move_to"]: #list of methods we intercept
            self.other._blocking = False
            return self.other.__getattribute__(name)
        else:
            return object.__getattribute__(self, name)

motor = Motor("A")

motor.move_to(20)
# > wait
motor.noblock.move_to(20)
# > returns ASAP

Thoughts? Any wrapper/decoration strategy that I missed and does the same?

My suggestion would be to create a special result value (similar to Future) that would represent an asynchronous computation:

class Future(object):
  def result(self):
    raise NotImplementedError()

where result waits until the underlying computation finishes and returns the reuslt.

Then let all your methods return instances of Future asynchronously. If you want a synchronous invocation, just call

motor.move_to(position).result()

The actual implementation in Motor would depend on your needs, in particular:

When calling two subsequent asynchronous computations, should the second block or should it be queued?
Should result() block only until its corresponding call has finished, or until the motor is idle? The former makes more sense, but would require something different than block_until_idle.
Should the class be thread-safe? (Your example code isn't.)


I would explicitly refer the constants in class variables :

class Elipsoids:
    airy = Elipsoid(6377563.396, 299.3249646, 'airy', 'Airy 1830', 'Britain', 7001),
    mod_airy = Elipsoid(6377340.189, 299.3249646, 'mod_airy', 'Airy Modified 1849', 'Ireland', 7002),
    ...

This way :

You get rid of global variables : you simply do module.Elipsoids.airy .
You get rid of dirty assigning code
You can get rid of alias if it's not used anywhere else in the code
You get more readable access to variables


simple strcmp() asm64 I have implemented a simple strcmp() in asm64. I'm very new to assembly so it must be awful ! I would like to know what errors were made.

    global  mstrc
mstrc:
        mov     r15b, [rsi]
        cmp     [rdi], r15b;compare two characters
        jne stop;if they are different, return
        cmp     byte [rdi], 0
        je      stop;else if end of string 1, return
        cmp     byte [rsi], 0
        je      stop;else if end of string 2, return
        ;increment both string adresses then loop
        add     rdi, 1;
        add     rsi, 1;
        jmp     mstrc;
stop:
;setup return value (s1[i] - s2[i]) then return
            mov     rax, [rdi]
            sub     rax, [rsi]
            ret

Technically there are no errors, however there is some redundancy in your code. You only need be concerned with the terminator of one string. Therefore you can test for NULL in this manner;

mstrc:     cmp     byte [rsi], 0          ; Are we at EOS (end of string)
           jz      stop
           cmpsb                          ; Lookup why this works
           jz      mstrc
stop:

Those 4 instructions almost accomplish the exact functionality your looking for except your code expects source and destination to be exactly the same.  Mine however determines that as many characters in destination are equal to source.

Conformity to strcmp

mstrc:    lodsb
          or      al,  al
          jz      stop
          sub     al, [esi]
          jnz     stop
          inc     esi
          jmp     mstrc
stop:


Unittests for CashFlow object I'm just writing a small unittest to test for a hypothetical library and object, as a way of predefining the behavior, perhaps both as an acceptance test and for unittesting as well. As I'm writing this, I'm wondering about the value of BDD constructs.

import unittest

def pv(data, rate):
    '''simple reference implementation for present value of a cash flow'''
    return sum(v/((1+rate)**i) for i, v in enumerate(data))

class CashFlowTestCase(unittest.TestCase):
    @classmethod
    def setUpClass(cls):
        cls.data1 = [-10,1,2,3,4,5,6,7,8,9,10]
        cls.rate1 = .05
        cls.cf1 = CashFlow(cls.data1, rate=cls.rate1)
    def test_PV(self):
        self.assertEqual(pv(self.data1, self.rate1), self.cf1.PV)

Such an object then might be created like this:

import numpy as np

class CashFlow(object):
    def __init__(self, data, rate):
        self.data = numpy.array(data) if isinstance(data, (list, tuple)) else data
        self.rate = rate
    def present_value(self):
        rates = np.ones_like(self.data) + self.rate
        return (self.data/(rates**np.arange(len(self.data)))).sum()
    PV = property(present_value)

I would intend to write more unittests for other aspects of cashflows and various ways of handling cashflows, for example, IRR's, number of periods per year/rate, etc...

So I just ran this until the test passed, and went ahead and saved it. Thoughts? Criticisms? Is it bad to calculate both sides of the test? Should I use assertAlmostEqual() since there could be rounding errors?

I'm wondering about the value of BDD constructs.

The value / non-value should become more apparent after implementing more and more test cases. Keep going!

  Thoughts? Criticisms?

Be careful with test cases that use things defined outside of them, for example:

def test_PV(self):
    self.assertEqual(pv(self.data1, self.rate1), self.cf1.PV)

Ideally, it's best when a test cases is self-contained,
and includes everything inside about the test,
including the expected value and the input values.
Here, we have to trust that self.data1, self.rate1, self.cf1.PV are all correctly setup to make this work, and that they were not tampered with afterwards.
That's a bit too much trust required.

The test case would be easier to understand and safer from side effects if you included all values needed to validate the calculation:

def test_PV(self):
    data = (-10, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10)
    rate = .05
    cf = CashFlow(data, rate)
    self.assertEqual(pv(data, rate), cf.PV)

This looks a lot less magical,
as the connection between data, rate and cf is clearly visible.
Although the definition of pv is still outside,
I could live with that,
because I see the intuition that whatever is pv,
the goal of this test is to verify that CashFlow(data, rate).PV behaves like pv(data, rate).

Note also that in this implementation you can no for a fact that no other test case could mess with data, rate and cf.
You cannot mistakenly add another test case with side effects to break this one.
I know such mistakes seem unlikely to do (why would anyone ever edit cf1?),
but minimizing potential side-effecty things is a good practice in general,
and especially critical in unit tests.

I understand you did this because you anticipate many tests based on this data:

@classmethod
    cls.data1 = [-10,1,2,3,4,5,6,7,8,9,10]
    cls.rate1 = .05
    cls.cf1 = CashFlow(cls.data1, rate=cls.rate1)

However, right now you only have a single test,
which makes this look a bit speculative.
Especially because you named the variables data1, rate1, cf1,
suggesting that there might be soon data2, rate2, and so on.

You might rightfully ask,
should you duplicate the initial data in multiple test cases,
violating the DRY principle?
The DRY principle is less important in unit tests than in implementation.
It's ok to duplicate, but it can become inconvenient if the specification changes later.
But the thing is, if the specification changes that's usually a big problem anyway,
with no good solution.
So yes, it's ok to duplicate the initial data.
If later you realize that a different initial data would be more interesting to test,
don't edit the previous values,
but add new tests with the new initial data.
If the previous values should still pass,
there's no need to remove them, who knows, they might still catch some bugs.

  Is it bad to calculate both sides of the test?

It's not great, but it can be acceptable.
You might want to add some test cases to verify pv itself so you can trust it.

  Should I use assertAlmostEqual() since there could be rounding errors?

When, you experience rounding errors, then you should add.
If you don't have rounding errors in your current test case,
then no need to speculate.
If you do have rounding errors already, then it's not even a choice.


Efficient implemetation of AlertDialog callback in Android One day I realised that my Android project utilizes AlertDialogs here and there in very ineffective way. The heavily duplicated portion of code looked like this (actually, copied and pasted from some documentation):

AlertDialog.Builder builder = new AlertDialog.Builder(context);
builder.setMessage("Are you sure you want to close application?")
  .setPositiveButton("Yes", new DialogInterface.OnClickListener()
  {
    public void onClick(DialogInterface dialog, int id)
    {
      dialog.dismiss();
      System.runFinalizersOnExit(true);
      System.exit(0);
    }
  })
  .setNegativeButton("No", new DialogInterface.OnClickListener()
  {
    public void onClick(DialogInterface dialog, int id)
    {
      dialog.dismiss();
    }
  });
builder.create().show();

Apparently, this code fragment should be transformed into a function accepting 2 parameters: a question to ask, and an action to perform.

After several iterations of code optimizations (each one of which seemed as the last and most minimal one), I came to the following class:

public abstract class DialogCallback implements DialogInterface.OnClickListener,Runnable
{
  DialogCallback(Context c, String q)
  {
    AlertDialog.Builder builder = new AlertDialog.Builder(c);
    builder.setMessage(q)
      .setPositiveButton("Yes", this)
      .setNegativeButton("No", this);
    builder.create().show();
  }

  public void onClick(DialogInterface dialog, int which)
  {
    dialog.dismiss();
    if(which == DialogInterface.BUTTON_POSITIVE) run();
  }
}

with the following usage:

new DialogCallback(context, "are you sure?")
{
  public void run()
  {
    // some action
  }
};

Is there a way of further optimization? What other methods for effective dialog callbacks can be used?

for a confirmation dialog use setCancelable(true) to make the user able to cancel the dialog using the back button. This is the native way and you don't have to implement anything.

Note: in Android you are not supposed to close the application. The operating system will close it when it feels like to do so.


Adding filtering to an existing query I need to simplify this code as well as a Linq query:

 var data = from linq_row in trend_data.AsEnumerable()
                       group linq_row by linq_row["questionnaire_uniquename"] into g

                       select new
                       {
                           questionnaire_uniquename = g.First<DataRow>()["questionnaire_uniquename"].ToString(),
                           question_name = g.First<DataRow>()["question_name"].ToString(),
                           question_score_type = g.First<DataRow>()["question_score_type"].ToString(),
                           sparkline_data = g.Select(s => s["trend_score"].ToString()).ToArray()
                       }; ;
            int rowspan = 2;

Based on some condition, I am adding just adding filtering to an existing query and modifying the selected query here. Instead of selecting question_name, I want to select response_label from a data table.

if (criteria.CorporateSummaryExpand == "1")
{
    data =
       from linq_row in trend_data.AsEnumerable()
       group linq_row by linq_row["questionnaire_uniquename"] into g
       where g.First<DataRow>()["questionnaire_uniquename"].ToString() != "[Questionnaire].[Questionnaire].[Question].&[OFSP]&[OFSP5000]"
       && g.First<DataRow>()["questionnaire_uniquename"].ToString() != "[Questionnaire].[Questionnaire].[Question].&[OFSP]&[OFSP5008]"
       && g.First<DataRow>()["questionnaire_uniquename"].ToString() != "[Questionnaire].[Questionnaire].[Question].&[OFSP]&[OFSP1130]"

       select new
       {
           questionnaire_uniquename = g.First<DataRow>()["questionnaire_uniquename"].ToString(),
           question_name = g.First<DataRow>()["response_label"].ToString(),
           question_score_type = g.First<DataRow>()["question_score_type"].ToString(),
           sparkline_data = g.Select(s => s["trend_score"].ToString()).ToArray()
       };
    rowspan = 4;
}

From what you have provided you can combine  the two queries into one. You should also consider removing constants like "1", 2, 4 with meaningful names.

var yourCondition = criteria.CorporateSummaryExpand == "1";
var data = from linq_row in trend_data.AsEnumerable()
           group linq_row by linq_row["questionnaire_uniquename"] into g
           where (criteria.CorporateSummaryExpand != "1") || (yourCondition  && g.First<DataRow>()["questionnaire_uniquename"].ToString() != "[Questionnaire].[Questionnaire].[Question].&[OFSP]&[OFSP5000]"
                   && g.First<DataRow>()["questionnaire_uniquename"].ToString() != "[Questionnaire].[Questionnaire].[Question].&[OFSP]&[OFSP5008]"
                   && g.First<DataRow>()["questionnaire_uniquename"].ToString() != "[Questionnaire].[Questionnaire].[Question].&[OFSP]&[OFSP1130]")
           select new
           {
                questionnaire_uniquename = g.First<DataRow>()["questionnaire_uniquename"].ToString(),
                question_name =yourCondition  ? g.First<DataRow>()["response_label"].ToString() : g.First<DataRow>()["question_name"].ToString(),
                question_score_type = g.First<DataRow>()["question_score_type"].ToString(),
                sparkline_data = g.Select(s => s["trend_score"].ToString()).ToArray()
           };
int rowspan = yourCondition ? 4 : 2;


Convert generic class into XML string of specific structure There is a need to have a specific structure of XML string when interacting with 3rd party system. Here are two examples:

<DocumentElement>
 <Country>
  <CountryCode>UA</CountryCode>
  <Name>Ukraine</Name>
 </Country>
</DocumentElement>

and

<DocumentElement>
 <Currency>
  <CurrencyCode>UAH</CurrencyCode>
  <Description>Hryvnia</Description>
 </Currency>
</DocumentElement>

Like you can see it is a common wrapping tag DocumentElement.
So, here is approach #1:

Have a wrapper class:

[XmlRoot("DocumentElement")]
public class DocumentElement<T>
{
    public T InnerObject { get; set; }

    public DocumentElement(){}
    public DocumentElement(T some)
    {
        InnerObject = some;
    }
}

And replace InnerObject after conversion:

public static string GetXml<T>(T instance) where T : class
{
    var wrapper = new DocumentElement<T>(instance);
    var writer = new StringWriter();
    var serializer = new XmlSerializer(typeof(DocumentElement<T>));
    serializer.Serialize(writer, wrapper);
    return writer.ToString().Replace("InnerObject", instance.GetType().Name);
}

Or, approach #2:

Throw away DocumentElement and just concat a needed string:

public static string GetXml<T>(T instance) where T : class
{
    var writer = new StringWriter();
    var serializer = new XmlSerializer(typeof(T));
    serializer.Serialize(writer, instance);
    return "<DocumentElement>" + writer.ToString() + "</DocumentElement>";
}

Both approaches produce a needed format but neither looks clean enough.
Maybe, someone can suggest a better approach to this?

Take a look at  How to: Specify an Alternate Element Name for an XML Stream.
Although it is more code, I would use the mentioned XmlAttributeOverrides like so

public static string GetXml<T>(T instance, string sourceName = "InnerObject") where T : class
{
    var wrapper = new DocumentElement<T>(instance);
    var writer = new StringWriter();
    var overrides = CreateOverrides(typeof(DocumentElement<T>), instance.GetType().Name, sourceName);

    var serializer = new XmlSerializer(typeof(DocumentElement<T>), overrides);
    serializer.Serialize(writer, wrapper);
    return writer.ToString();
}
private static XmlAttributeOverrides CreateOverrides(Type type, string destinationName, string sourceName)
{
    var elementAttribute = new XmlElementAttribute() { ElementName = destinationName };
    var attributes = new XmlAttributes();
    attributes.XmlElements.Add(elementAttribute);
    var overrides = new XmlAttributeOverrides();
    overrides.Add(type, sourceName, attributes);
    return overrides;
}


Extracting information from files I have a file, the content is in the format of

41545
0
0
1
2
3
3
3
...

Following is the code

X=[]
Y=[]
total=0
count=0
for line in open(file,'r'):
    count+=1
    line=line.strip()
    if(line.strip()):
        if(count==1):
            total=int(line)
        else:
            X.append(count-1)
            Y.append(int(line))

the output for the above code is

total=41545 #first line
X=[1,2,3,4,5,6,7] #count of second line onwards
Y=[0,0,1,2,3,3,3] #second line onwards information

Is there a shorter and hopefully more efficient code that can achieve the same purpose?

Build a generator over the file to return just ints, then transpose the enumeration to get your X and Y, eg:

with open('testing.txt') as fin:
    lines = (int(line) for line in fin)
    total = next(lines)
    X, Y = zip(*enumerate(lines, 1))

Note X and Y will be tuples instead of lists, but you can convert them after if you wish.