Dividing a range into N sub-ranges Introduction

The function divides a range given by a begin_iterator and an end_iterator into n sub-ranges of the same format, where the sub-ranges must be as equally sized as possible.

Example

Consider a set of size 10:

  \$\{ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 \}\$

The output would be is a set of n = 3 sub-ranges:

  \$\{ \{ 0, 3 \}, \{ 4, 6 \}, \{ 7, 9 \} \}\$

Where each sub-set is as even as possible, in the previous example the sets have sizes 4, 3, and 3, respectively; that is the most even way to distribute 10 elements into 3 sub-sets.

Review goals

Efficiency
Correctness

Implementation

The oringinal code contained an issue which I missed by pure luck. Here is the fixed working code; the changes do not invalidate the present answer.

#include <vector>   // std::vector<T>
#include <iterator> // std::distance(), std::next()

template <typename Iterator>
decltype( auto ) divide_work( Iterator begin, Iterator end, std::size_t n )
{
    using size_type = std::size_t;

    auto sz = std::distance( begin, end );
    auto element_per_group_count = sz / n;
    auto remaining_element_count = sz % n;

    std::vector<std::pair<Iterator, Iterator>> ranges;
    ranges.reserve( n );

    if ( remaining_element_count == 0 )
    {
        --element_per_group_count;

        ranges.emplace_back( std::make_pair( std::next( begin, 0 ),
            std::next( begin, element_per_group_count ) ) );

        for ( auto i = 1U; i < n; ++i )
        {
            auto first = std::next( ranges[ i - 1 ].second, 1 );
            ranges.emplace_back( std::make_pair( first,
                std::next( first, element_per_group_count ) ) );
        }
    }
    else
    {
        ranges.emplace_back( std::make_pair( std::next( begin, 0 ),
            std::next( begin, element_per_group_count ) ) );

        for ( auto i = 1U; i < remaining_element_count; ++i )
        {
            auto first = std::next( ranges[ i - 1 ].second, 1 );
            ranges.emplace_back( std::make_pair( first,
                std::next( first, element_per_group_count ) ) );
        }

        --element_per_group_count;

        for ( auto i = ranges.size(); i < n; ++i )
        {
            auto first = std::next( ranges[ i - 1 ].second, 1 );
            ranges.emplace_back( std::make_pair( first,
                std::next( first, element_per_group_count ) ) );
        }
    }
    return ranges;
}

Output

Misleading Return Type

You have:

template <typename Iterator>
decltype( auto ) divide_work( Iterator begin, Iterator end, std::size_t n )

The difference between auto and decltype(auto) is that the latter can deduce reference types as well. But you're returning a value, always, regardless of Iterator. This choice makes it seem as if you could be returning a reference. If you are going to use a placeholder return type here, use simply auto.

Even better, the return type for this function is not a complicated expression that's difficult to spell. It's simply std::vector<std::pair<Iterator, Iterator>>, so let's just use that:

template <typename Iterator>
std::vector<std::pair<Iterator, Iterator>>
divide_work( Iterator begin, Iterator end, std::size_t n )

emplace_back

The advantage of emplace_back is not having to actually construct the object to pass in. So when you write:

ranges.emplace_back( std::make_pair( std::next( begin, 0 ),
            std::next( begin, element_per_group_count ) ) );

That's inefficient (you're copy constructing a pair instead of in-place constructing it) and just a bunch of extra typing. Also next(iter, 0) is iter, so the above line is logically exactly equivalent to:

ranges.emplace_back(begin, std::next(begin, element_per_group_count));

Overly Complicated Algorithm!

I'm not sure why you have three for loops or why even division is a special case of the algorithm... Also I'm pretty sure that special case is wrong. Consider a range of size 15 dividing into 3 chunks, so it divides evenly. We'd start with element_per_group_count == 5, but then we decrement it to 4, and add the first range of:

[0, 4)

Then the next two ranges will be:

[5, 9)
[10, 14)

We successfully divided into three chunks, but we're actually missing elements! 4, 9, and 14 will not appear in any of the three chunks. The correct division would be [[0, 5), [5, 10), [10, 15)].

We're just chunking the range (begin, end) into n pieces. That implies a simple for-loop. We need n things. Each one is approximately of size distance(begin, end)/n. There will be distance(begin, end)%n remainder, which we will allocate one at a time as we go until we run out of remainder. When we already added n-1 things, we just can add the rest as the last chunk (since there's nothing else to do at that point).

This is all you need:

template <typename Iterator>
std::vector<std::pair<Iterator, Iterator>>
divide_work( Iterator begin, Iterator end, std::size_t n )
{
    std::vector<std::pair<Iterator, Iterator>> ranges;
    ranges.reserve(n);

    auto dist = std::distance(begin, end);
    auto chunk = dist / n;
    auto remainder = dist % n;

    for (size_t i = 0; i < n-1; ++i) {
        auto next_end = std::next(begin, chunk + (remainder ? 1 : 0));
        ranges.emplace_back(begin, next_end);

        begin = next_end;
        if (remainder) remainder -= 1;
    }

    // last chunk
    ranges.emplace_back(begin, end);
    return ranges;
}

Dividing up the remainder one at a time means that if we have 15 elements into 6 chunks, we end up with sizes of [3, 3, 3, 2, 2, 2] instead of any other weird alternative like the naive [2, 2, 2, 2, 2, 5].

Too many chunks? None?

What do you do in the case where n > distance(begin, end) or n == 0? I guess we should probably add some error checks:

template <typename Iterator>
std::vector<std::pair<Iterator, Iterator>>
divide_work( Iterator begin, Iterator end, std::size_t n )
{
    std::vector<std::pair<Iterator, Iterator>> ranges;
    if (n == 0) return ranges;
    ranges.reserve(n);

    auto dist = std::distance(begin, end);
    n = std::min<size_t>(n, dist);
    auto chunk = dist / n;
    auto remainder = dist % n;

    for (size_t i = 0; i < n-1; ++i) {
        auto next_end = std::next(begin, chunk + (remainder ? 1 : 0));
        ranges.emplace_back(begin, next_end);

        begin = next_end;
        if (remainder) remainder -= 1;
    }

    // last chunk
    ranges.emplace_back(begin, end);
    return ranges;
}


Uploading a .csv to a NoSQL cluster - batch faster than consumer/producer I was tasked with making a program that uploads a .csv to a NoSQL cluster. The files are larger (typically 2-17GB). My program works in batch mode and can process a 17GB file in 6 hours.

I decided to make a consumer-producer multithreading structure. This caused it to be significantly slower. I want to know why the producer-consumer construct was slower than a batch produce, batch consume method.

The batch looks like this:

int count = 0;
        //
        Row r;

        while ((r = rm.getNextRow()) != null)
        {
            RowQueue.Enqueue(r);
            while (RowQueue.Count <= ROWMAX)
            {
                if ((r = rm.getNextRow()) != null)
                    RowQueue.Enqueue(r);
                else
                    break;
            } //

            int uniqueIdentifer = -1;
            if (count > 1000)
            {
                PrintAndSavePosition(count, rm, positionQueue, true);
                count = 0;
            }
            //give it some extra room to be safe
            while (RowQueue.Count != 0)
            {
                r = RowQueue.Dequeue();
                while (uniqueIdentifer == -1)
                {
                    uniqueIdentifer = nsqw.tryPut(r);
                    if (uniqueIdentifer == -1)
                        Thread.Sleep(1);
                }
                count++;
            }

            positionQueue.Add(new Tuple<int, long>(uniqueIdentifer, rm.Position));
         }

As compared to

public void produceLoop(){
            while (true)
            {

                while (RowQueue.Count <= ROWMAX && (r = rm.getNextRow()) != null){
                    RowQueue.Enqueue(r);
                }

            }
 }

public void consumeLoop(){
     while(true){
     while (RowQueue.Count != 0)
            {
                RowQueue.TryDequeue(out r);
                while (uniqueIdentifer == -1)
                {
                    uniqueIdentifer = nsqw.tryPut(r);
                    if (uniqueIdentifer == -1)
                        Thread.Sleep(1);
                }
                count++;
            }

            positionQueue.Add(new Tuple<int, long>(uniqueIdentifer, rm.Position));
         }
 }

}

The bottom half are infinite loops for a speed test.

I'm not an expert in cluster operations, so I'll review the parts I do know.

Single-letter variable names

These are a big no-no unless the only thing you're worried about is not being fireable. Any maintenance programmer looking in the middle of a chunk of code is not going to be happy jumping back and forwards to definitions to work out what on earth r is when you could just as easily have written row.

var

You should use var when the right hand side of an assignment makes the type obvious. e.g.

int uniqueIdentifier = 1;

should be

var uniqueIdentifier = 1;

This is recommended because if you decide to change the type (e.g. to a GUID), you only have to change it in one place.

Style

In C# the general naming guideline for methods is to use PascalCase. This is, of course, optional (as with any point on style), but recommended to make code easier to read for another programmer.


Checking if a file exists then do something if it's not a directory While I'm aware that code with callbacks tends to be complex, I'm wondering if there are any patterns or something to improve my situation.

All I do here is check if a file exists and then print its name if it's not a directory:

var fs = require('fs'),
    filename = process.args[2];

fs.exists(filename, function(exists) {
  if (exists) {
    fs.stat(filename, function(err, stats) {
      if (stats.isDirectory()) {
        console.log(filename + ": is a directory");
      } else {
        // do something with file
        console.log(filename);
      }
    });
  } else {
    console.log(filename + ": no such file");
  }
});

You can actually omit fs.exists(). The fs.stat() will return an error when the item you are testing is not there. You can scavenge through the err object that fs.stat() returns to see what error caused it. As I remember, when fs.stat() stats a non-existing entry, it returns an ENOENT, no such file or directory error.

And so:

var fs = require('fs')
  , filename = process.args[2]
  ;

fs.stat(filename, function(err, stats) {
  if(err){
    //doing what I call "early return" pattern or basically "blacklisting"
    //we stop errors at this block and prevent further execution of code

    //in here, do something like check what error was returned
    switch(err.code){
      case 'ENOENT':
        console.log(filename + ' does not exist');
        break;
      ...
    }
    //of course you should not proceed so you should return
    return;
  }

  //back there, we handled the error and blocked execution
  //beyond this line, we assume there's no error and proceed

  if (stats.isDirectory()) {
    console.log(filename + ": is a directory");
  } else {
    console.log(filename);
  }
});

So essentially, we reduced the number of indents caused by callbacks and also reduced it by restructuring if-else statements


Applying timed functions recursively I am relatively new to JavaScript/jQuery, and cobbled together a script. Essentially, the script sets individual timers on specific DOM elements, and then recursively applies timed functions to them. The functions actually apply a "flip" effect to a div, showing a front and back, and then do different things on each side. The references to "Vague" are from Vague.js, and external library I didn't write that provides a blur effect to elements. The script works, but writing working code and writing good code are two separate things.

$(document).ready( function() {
    $('.flipper').each(function(e) {
        var flipper = new myFlipper($(this));
    })
});

var myFlipper = function (flipper) {
    //console.log(flipper);
    this.flipper = flipper;
    this.image = flipper.find('img');
    this.blurStatus = true;
    this.flipStatus = true;
    this.Vague = this.image.Vague ({
        intensity: 8,
        forceSVGUrl: false
    });
    this.blurContext = $.proxy(this.myBlur, this);
    this.flipContext = $.proxy(this.myFlip, this);
    // $(this.image).on("click", this.blurContext);
    var randomNumber = Math.floor(Math.random()*10);
    var timer = randomNumber * 1000;
    setTimeout(this.blurContext, timer);
}

myFlipper.prototype.myBlur = function(e) {
    console.log(this.flipContext);
    if(this.blurStatus) {
        this.Vague.blur();
        this.blurStatus = false;
        this.flipper.find('.caption').animate({opacity: 1.0}, 3000);
        setTimeout(this.flipContext,5000);
    } else {
        this.Vague.unblur();
        this.blurStatus = true;
    }
}

myFlipper.prototype.myFlip = function(e) {
    console.log(this.flipper.attr("class"));
    console.log(this.flipStatus);
    if (this.flipStatus) {
        this.flipper.addClass('hover');
        this.flipper.find('.caption').animate({opacity: 0.0}, 1000);
        this.blurContext();
        this.flipStatus = false;
        setTimeout(this.flipContext, 20000);
    } else {
        this.flipper.removeClass('hover');
        this.flipStatus = true;
        setTimeout(this.blurContext,5000);
    }
}

Here is the JSFiddle of the whole thing.

This might not be an improvement per se, but you can change this:

$(document).ready( function() {
    $('.flipper').each(function(e) {
        var flipper = new myFlipper($(this));
    })
});

To this:

$(function() {
    $('.flipper').each(function(e) {
        var flipper = new myFlipper($(this));
    });
});

The reason this works is $() is a shortcut for $(document).ready().

With JavaScript, it is usually recommended that you keep from cluttering the global namespace. You currently have everything contained in var myFlipper, which is good. But since you're using jQuery, you might as well use a jQuery plugin, which would allow you to do something like this:

$.fn.myFlipper = function () {
    //...
};

$(function() {
    $('.flipper').myFlipper();
});

This way, you aren't introducing another variable into the global namespace. Plus, the jQuery plugin architecture affords you some additional benefits, like syntax, etc.


How can I remove this for-loop in this competitive FizzBuzz code? I'v written this code snippet that's part of a code competition.

I want to do the trick without the for-loop, or simply find a way to optimize this code for speed.

for ( i=1; i<=N; i++ )

How is it possible, if it is?  What do I need to modify to speed up this code considering a very large input file?

Here is the competition summary:

  Players generally sit in a circle. The player designated to go first
  says the number "1", and each player thenceforth counts one number in
  turn. However, any number divisible by 'A' e.g. three is replaced by
  the word fizz and any divisible by 'B' e.g. five by the word buzz.
  Numbers divisible by both become fizz buzz. A player who hesitates or
  makes a mistake is either eliminated.

  Write a program that prints out the the pattern generated by such a
  scenario given the values of 'A'/'B' and 'N' which are read from an
  input text file. The input text file contains three space delimited
  numbers i.e. A, B, N. The program should then print out the final
  series of numbers using 'F' for fizz, 'B' for 'buzz' and 'FB' for fizz
  buzz.

  Print out the series 1 through N replacing numbers divisible by 'A' by
  F, numbers divisible by 'B' by B and numbers divisible by both as
  'FB'. Since the input file contains multiple sets of values, your
  output will print out one line per set. Ensure that there are no
  trailing empty spaces on each line you print.

Input file:

3 5 10
2 7 15

Output:

1 2 F 4 B F 7 8 F B
1 F 3 F 5 F B F 9 F 11 F 13 FB 15

Here my code:

int main(int argc, const char * argv[])
{
  NSString* filename      = [NSString stringWithCString:argv[1] encoding:NSASCIIStringEncoding];
  NSString* content       = [NSString stringWithContentsOfFile:filename encoding:NSASCIIStringEncoding error:nil];
  NSScanner* scanner      = [NSScanner scannerWithString:content];
  while ( ![scanner isAtEnd] )
  {
    NSString* line;
    [scanner scanUpToString:@"\n" intoString:&line];

    NSArray *nums = [ line componentsSeparatedByString:@" " ];
    int A         = [[nums objectAtIndex:0] intValue];
    int B         = [[nums objectAtIndex:1] intValue];
    int N         = [[nums objectAtIndex:2] intValue];

    int i=0;
    for ( i=1; i<=N; i++ )
    {
      NSString *str = [ NSString stringWithFormat:@"%d", i ];

      if ( i%A == 0 ) str = @"F";
      if ( i%B == 0 ) str = @"B";
      if ( i%A == 0 && i%B == 0 ) str = @"FB";

      printf( "%s ", [str UTF8String] );
    }
    printf("\n");
  }
  return 0;
}

The title of your post puts a lot of stress on removing the for loop. Can it be removed?

No, it cannot. You need at least one loop to solve the fizz-buzz...

Additionally, the 'print' statements are by far the largest bulk of your performance problem.

I don't really believe that this code suffers from a problematic performance issue, but, if you want it to go faster, your best bet would be to accumulate all your String values and only print the line once:

NSMutableString* result;
....

for (...) {
    .....
        [result appendString:@"FB "];
    .....
        [result appendFormat:@"%d ", i];

}
printf(result);
printf("\n");

This saves a fair number of unnecessary print statements, and your performance will improve as a result.


Creating instances of all subclasses in Python I have a superclass with several subclasses. Each subclass overrides the "solve" method with some a different algorithm for estimating the solution to a particular problem. I'd like to efficiently create an instance of each subclass, store those instances in a list, and then take the sum (or average) of the estimates that the subclasses give:

class Technique():

    def __init__(self):
        self.weight = 1

    def solve(self, problem):
        pass

class Technique1(Technique):

    def __init__(self):
        super()

    def solve(self,problem):
        return 1

class Technique2(Technique):

    def __init__(self):
        super()

    def solve(self,problem):
        return 6

 class Technique3(Technique):

    def __init__(self):
        super()

    def solve(self,problem):
        return -13

testCase = 3
myTechniques = []
t1 = Technique1()
myTechniques.append(t1)
t2 = Technique1()
myTechniques.append(t2)
t3 = Technique1()
myTechniques.append(t3)

print(sum([t.solve(testCase) for t in myTechniques])

The code at the end feels very clunky, particularly as the number of subclasses increases. Is there a way to more efficiently create all of the instances?

You might try this:

myTechniques = [cls() for cls in Technique.__subclasses__()]

Note that it does not return indirect subclasses, only immediate ones. You might also consider what happens if some subclass of Technique is itself abstract. Speaking of which, you should be using ABCMeta and abstractmethod.


Task scheduler for a particular application This isn't meant as a scheduler for just any job. The job that I need to schedule here is very specific for an application I'm writing for one of my clients.

The scheduler will run as a console application because it's comparatively lightweight. The application will be started with Windows (startup items in the Registry) and should always be running - much like a Windows service but not (mostly because I don't have a project template for a Windows service in VS 2013 Community).

The task this scheduler runs should be done at 4am every day.

Does it look like the code below matches this description of the requirements?

class MyScheduledTask
{
    private static Timer _timer;

    static void Main(string[] args)
    {
        SetTime();
    }

    private static void SetTime()
    {
        DateTime currentTime = DateTime.Now;
        int interval = 0;
        DateTime scheduleTime = new DateTime(currentTime.Year, currentTime.Month, currentTime.Day, 12, 45, 0);

        if (currentTime <= scheduleTime)
        {
            interval = (int)scheduleTime.Subtract(currentTime).TotalSeconds;
        }
        else
        {
            interval = (int)scheduleTime.AddDays(1).Subtract(currentTime).TotalSeconds;
        }

        _timer = new Timer(interval);
        _timer.Elapsed += new ElapsedEventHandler(_timer_Elapsed);
        _timer.Start();
    }

    void _timer_Elapsed(object sender, System.Timers.ElapsedEventArgs e)
    {
        StartTaskWork();
        _timer.Interval = (24 * 60 * 60 * 1000);
    }
}

First, I'd mention that scheduling execution of anything in Windows is better done through the Windows Task Scheduler. Scheduling your application that way would remove the "must run everyday at 4AM" requirement from the code, although I don't see how your code is enforcing that.
That said...
Magic Numbers & Configuration

DateTime scheduleTime = new DateTime(currentTime.Year, currentTime.Month, currentTime.Day, 12, 45, 0);

"12" and "45" (and "0") are magic numbers here. These should be taken from configuration, or better yet, from command-line arguments.

The right hammer for the right nail
System.Int32 is an awesome hammer. But that doesn't make every number a nail.

int interval = 0;

You (present you - future you could be a different story) know they're seconds. But the name says nothing of it, and the type is making your life harder than it needs to be. The .NET framework has a struct specifically meant to represent a time interval, more precisely a time span - System.TimeSpan. Compare readability here:
TimeSpan interval = TimeSpan.FromSeconds(0);

Ambiguity: gone. And as a bonus, you no longer need to cast anything here:
    if (currentTime <= scheduleTime)
    {
        interval = scheduleTime.Subtract(currentTime);
    }
    else
    {
        interval = scheduleTime.AddDays(1).Subtract(currentTime);
    }

Oh, and it turns this:

_timer.Interval = (24 * 60 * 60 * 1000);

Into that:
_timer.Interval = TimeSpan.FromDays(1).TotalMilliseconds;

And that rids the code of an implicit conversion from int to double, since both TimeSpan.TotalMilliseconds and Timer.Interval are double.


Python-like enumerate and join I'm new to Rust and trying to convert some Python to Rust.  I have the following in Python:

>>> v = ((0, 0), (1,1), (2, 2))
>>> strings = "\n".join(("%s: (%s, %s)" % (i, val[0], val[1]) for i,val in enumerate(v))))
>>> print(strings)
0: (0, 0)
1: (1, 1)
2: (2, 2)

I'm trying to find the best/smartest/most rustic way of doing this:

let v = vec![(0, 0), (1,1), (2, 2)];

let strings = v
    .iter()
    .enumerate()
    .map(|(i, &(a,b))| format!("{:?}: ({}, {})", i, a, b))
    .collect::<Vec<_>>()
    .join("\n");

println!("{}", strings);

Your code is reasonable, but there some small tweaks you could do.

Check out itertools, which adds some nice extensions to iterators. Specifically, it allows joining an iterator, instead of needing to collect to an intermediate Vec.
I'm unclear why you would use the Debug formatter for an integer.
Tuples have a space after the comma, between elements (see in the closure).
Your example might just be overly minimized, but there's no reason to use a Vec here, an array would suffice.

extern crate itertools;

use itertools::Itertools;

fn main() {
    let v = [(0, 0), (1, 1), (2, 2)];

    let strings = v.iter()
        .enumerate()
        .map(|(i, &(a, b))| format!("{}: ({}, {})", i, a, b))
        .join("\n");

    println!("{}", strings);
}

If you are OK changing the semantics a little, you can move the newline into the format! and collect into a String:

fn main() {
    let v = [(0, 0), (1, 1), (2, 2)];

    let strings: String = v.iter()
        .enumerate()
        .map(|(i, &(a, b))| format!("{}: ({}, {})\n", i, a, b))
        .collect();

    println!("{}", strings);
}

Note that there is now a trailing newline on the string.

The "unfortunate" thing about these solutions is that they all require intermediate string allocations - the result of format!.

There's a less-common method in itertools to deal with this: format_with:

extern crate itertools;

use itertools::Itertools;

fn main() {
    let v = [(0, 0), (1, 1), (2, 2)];

    let formatter = v.iter()
        .enumerate()
        .format_with("\n", |(i, &(a, b)), fmt| {
            fmt(&format_args!("{}: ({}, {})", i, a, b))
        });

    let strings = format!("{}", formatter);

    println!("{}", strings);
}

I wouldn't worry about this until you know you need that extra bit of performance ^_^


Input validation for text boxes in a Form In a Win Form I'm doing initial validation in the Form. So before saving data, I want to validate whether all the required fields are filled (Text Boxes) by the user. There are about 18 such Text Boxes in the Form. Currently I'm doing it as follows. To make the code short only three fields are shown in the code.

     private void cmbSave_Click(object sender, EventArgs e)
     {
         if(IsFilled (txtApplicationNumber.Text))
         {
             if(IsFilled (txtEmployeeID.Text))
             {
                 if (IsFilled(txtNIC.Text))
                 Save(sender, e);
             }
         }
    }

    private bool IsFilled(string s)
    {
        if (s != "")
        { return true; }
        else
        { return false; }
    }

Another option would be to use the logic inside the cmbSave_Click straightaway as follows,

    private void cmbSave_Click(object sender, EventArgs e)
    {
        If (txtApplicationNumber.Text!="" & txtEmployeeID.Text!="" & txtNIC.Text="")
        Save(sender, e)
    }

Basically the code should be readable, consistent and should perform the validation properly.

Which option could be considered better? If both are not good please propose a way to do this?

Another option would be to handle the Validating event:

private void textBox_Validating(object sender, CancelEventArgs e)
{
    TextBox currenttb = (TextBox)sender;
    if(currenttb.Text == "")
        MessageBox.Show(string.Format("Empty field {0 }",currenttb.Name.Substring(3)));
        e.Cancel = true;
    else
    {
        e.Cancel = false;
    }
}

Adding the handler to the textboxes is easily done with a foreach loop in the form constructor:

foreach(TextBox tb in this.Controls.OfType<TextBox>().Where(x => x.CausesValidation == true))
{
    tb.Validating += textBox_Validating;
}

Now the user can't leave a textbox empty.  The handler returns the focus back to the textbox.  To exclude a textbox from being validated simply set the CausesValidation property to false.


Anti-pattern or acceptable way of using Promises? Whenever I need to use a Promise interface in my NodeJs code and I don't have a promise to start off with, I do this:

var someResultPromise = Q.resolve()
    .then(function () {
        var something = 'value';

        var anotherPromise = callFunctionThatReturnsPromise(something)
            .then(function (result) {
                return processing(result);
            });

        return anotherPromise;
    });

Is this a good way to do it? If no, why not?

The Q.resolve() and the outer then(function() {...}) are not necessary. Your code will simplify to ...

var something = 'value';
var someResultPromise = callFunctionThatReturnsPromise(something).then(function (result) {
    return processing(result);
});

... and in general that is what we would write.

However sometimes it's advantageous to start a promise chain with a resolved promise, even if it's not strictly necessary.

For example, we might want a chain such as ...

var promise = doSomethingAsync()
    .then(doSomethingAsync)
    .then(doSomethingAsync)
    .then(doSomethingAsync);

... but we might choose to write ...

var promise = Q.resolve()
    .then(doSomethingAsync)
    .then(doSomethingAsync)
    .then(doSomethingAsync)
    .then(doSomethingAsync);

... which is functionally identical but easier on the eye because it allows the first doSomethingAsync to be coded the same as all the others; it does not appear to be a special case.


Creating dynamic threads for a SMS text API First, I want to say that this code works, but I want to make sure this is the right way of doing it. I am writing this for a SMS text API that loops through each phone number. I sometimes gets hung up on 1 number or just takes time. It roughly takes 6-8 sec per number. So I thought I would write something that would make it multitreaded. I wrote this to allow the number of threads at one time to go up depending on the workload.

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading;

namespace TestTreading
{
    class Program
    {
        static public BlockingCollection<int> Queue = new BlockingCollection<int>();
        static public BlockingCollection<int> Finished = new BlockingCollection<int>();
        static public BlockingCollection<Thread> Threads = new BlockingCollection<Thread>();
        static public int QUCount = 0;
        static public int THCount = 0;
        static public int FNCount = 0;

        static void Main(string[] args)
        {
            BuildList();
            DateTime StartTime = DateTime.Now;

            QUCount = Queue.Count;
            THCount = Threads.Count;
            FNCount = Finished.Count;

            while (QUCount > 0)
            {
                THCount = Threads.Count;
                if (THCount < 20)
                {
                    Thread th1 = new Thread(LoopMethod);
                    Threads.Add(th1);
                    THCount = Threads.Count;
                    th1.Start();
                }
            }

            DateTime EndTime = DateTime.Now;
            Console.WriteLine(StartTime);
            Console.WriteLine(EndTime);
            Console.WriteLine(EndTime - StartTime);
            Console.ReadLine();
        }

        static void LoopMethod()
        {
            int QueueItem = Queue.Take();
            QUCount = Queue.Count;
            for (long i = 0; i < 100000; i++)
            {
                long result = i / 2;
            }
            string Display = string.Format("ID:{0} Left In Queue:{1}  Finished:{2}  Threads Running:{3}", QueueItem, QUCount, FNCount, THCount);
            Console.WriteLine(Display);
            Thread.Sleep(10);

            Finished.Add(QueueItem);
            FNCount = Finished.Count;
            Thread CurTh = Thread.CurrentThread;
            Threads.TryTake(out CurTh);
        }

        static void BuildList()
        {
            for (long i = 0; i < 5000; i++)
            {
                Random RandomNum = new Random();
                int Num = RandomNum.Next(1, 2);
                Queue.Add(Num);
            }
        }

    }
}

static public BlockingCollection<int> Queue
static public BlockingCollection<int> Finished
static public BlockingCollection<Thread> Threads
static public int QUCount
static public int THCount
static public int FNCount

There is no need to make any of these fields public. You shouldn't change the visibility from the default of private unless you have a reason to do it.

for (long i = 0; i < 5000; i++)
{
    Random RandomNum = new Random();
    int Num = RandomNum.Next(1, 2);
    Queue.Add(Num);
}

This is a very complicated way to generate 5000 1s. If you actually want to generate random numbers, you can't create a new Random for each iteration, you should instead create it once before the loop and then just call Next() inside it. That's because Random is initialized by the current time by default, and the time as measured by .Net doesn't change that often. That means you would get runs of the same number, which is not random.

THCount = Threads.Count;
FNCount = Finished.Count;

These lines don't do anything here, both collections are going to be empty at this point. In fact, the Count fields are completely useless, you should just use the Count property of each collection instead.

while (QUCount > 0)
{
    if (THCount < 20)
    {
        Thread th1 = new Thread(LoopMethod);
        th1.Start();
    }
}

This code doesn't make much sense. It's certainly possible that it will start all 20 threads even if there's just a single item in the queue, because you could create all of the threads before the first one starts.

What's much worse is that it will keep on iterating while the other threads are working. This means a whole single core of your CPU will be fully busy, doing basically nothing.

Thread th1 = new Thread(LoopMethod);

Threads are relatively heavy-weight, you should usually create a new thread only for a long running operation. If you have thousands of operations, you shouldn't create thousands of threads. Instead, you should create just a few of them and have a loop inside each.

Threads.Add(th1);

There is no need to do this. You don't actually need a collection of threads, you just need to know their count. So you should use an int field for that, and increment that value in a thread-safe manner.

Thread CurTh = Thread.CurrentThread;
Threads.TryTake(out CurTh);

This code looks like it's trying to take the current thread out of the collection. In fact, it does no such thing. Instead, it removes the first item from the collection (i.e. the oldest item that hasn't yet been removed from the collection) and assigns that to the CurTh variable. The out modifier means, among other things, that the parameter is used for output only and if it has some value before the method is called, it's going to be ignored.

int QUCount
int THCount
int FNCount
Thread th1

You shouldn't abbreviate your field and variable names like this. You should write their names in full, (e.g. QueueCount or thread), because code is read more often than it is written, so it makes sense to strive for code that is easy to read, not code that is easy to write. (And it doesn't even make much difference with autocompletion.)

Usually, it's much better to use TPL for parallel processing, instead of manually using threads. Using GetConsumingEnumerable() and Parallel.ForEach() would make your code shorter, more readable and much more efficient. (CPU efficiency usually doesn't matter much for code like this, but you're wasting CPU a lot.)

class Program
{
    static BlockingCollection<int> Queue = new BlockingCollection<int>();
    static BlockingCollection<int> Finished = new BlockingCollection<int>();

    static void Main()
    {
        BuildList();

        Parallel.ForEach(
            Queue.GetConsumingEnumerable(),
            new ParallelOptions { MaxDegreeOfParallelism = 20 },
            ProcessItem);
    }

    static void ProcessItem(int i)
    {
        int result = /* somehow calculate the result here */;
        Finished.Add(result);
    }
}

(I have also removed your logging for brevity.)


parallel.For function I am trying to add parallelism. The following function works.  For testing purposes, calculating chunckLength is fine. However, when I allocate more cores, it seems to slow down significantly which seems due to massive overhead. The visualisation shows the average core utilisation during execution. Each run, the function is iterated over 10 times with lists of 10000 elements.

I am fairly new to F# (as you can probably tell from the functions structure...), so any suggestions on how to optimise it so I can achieve some speed-up would help greatly.

let pmap_tpl_parforlb f (xs:list<_>) =
let xs_arr = xs.ToArray()
let chunkLength = xs.Length / numProc
Parallel.For(0,numProc,fun c ->
    let x = c * chunkLength
    let y = (c * chunkLength) + chunkLength
    for x in 0..y-1 do
        xs_arr.[x] <- f (xs_arr.[x])
) |> ignore

Have you tried not using explicit chunk sizes? Parallel.For does some fancy chunking (range-splitting) and load-balancing under the hood, which usually gives very good performance on it's own.

In other words, try a simpler solution first, e.g.,:

let mapInPlace (mapping : 'T -> 'T) (array : 'T[]) : unit =
    Parallel.For (0, array.Length, fun i ->
        array.[i] <- mapping array.[i])
    |> ignore

Another point -- if you're iterating over lists of 10000 elements, you should convert those lists to an array once, then call your iterative function however many times you need to. I suspect you're losing a non-trivial amount of performance from doing the list-to-array conversion repeatedly.


Generate all combinations of certain digits Related to another answer of mine on Project Euler 35, I found the need to calculate all combinations of certain digits, i.e. 1, 3, 7 and 9, below a given n. I looked around, and didn't find any really good premade solutions, but found some bits and pieces related to Cartesian products, and collated my findings into the following code:

import itertools

def all_digit_combinations(allowed_digits, maximum):
    """Yield all combinations of allowed_digits below maximum.

    For allowed_digits being a list of single digits, i.e. [1, 3, 7, 9],
    combine all variants of these digits until we pass the maximum value.
    """
    no_of_digits = 1

    while True:
        for digit_tuple in itertools.product(allowed_digits, repeat=no_of_digits):
            new_number = reduce(lambda rst, d: rst * 10 + d, digit_tuple)
            if new_number < maximum:
                yield new_number
            else:
                raise StopIteration

        no_of_digits += 1

if __name__ == '__main__':
    print ', '.join(map(str, all_digit_combinations([1, 3, 7, 9], 100)))
    print ', '.join(map(str, all_digit_combinations([3, 5], 1000)))

Which indeed prints the expected output of:

1, 3, 7, 9, 11, 13, 17, 19, 31, 33, 37, 39, 71, 73, 77, 79, 91, 93, 97, 99
3, 5, 33, 35, 53, 55, 333, 335, 353, 355, 533, 535, 553, 555

I first tried using itertools.combinations_with_replacement() and other variations from itertools, but some of those versions failed to include numbers like 31 and 73. But it I could very well simply not use the correct parameters.

Can you review this code, suggesting any optimalisations or improvements?

I think the code that you have is near best it can become.

I would however use a more functional/iterator based solution.

while True: no_of_digits += 1 can be replaced with a for loop.
This is by using itertools.count(1). (Careful infinite generator here)
all_digit_combinations would imply to me, that I get all of them, even 55555555.
Instead I would make all_digit_combinations an infinite generator.
else: raise StopIteration is kinda ugly. Personally I would use itertool.takewhile. (In this case it's better than filter, as infinite's are involved.)

As an alternate to itertools, you could just return. But due to that being implicit, it may be disliked.

And so I would re-write it into this:

def all_digit_combinations(allowed_digits):
    return (
        reduce(lambda rst, d: rst * 10 + d, digit_tuple)
        for no_of_digits in itertools.count(1)
        for digit_tuple in itertools.product(allowed_digits, repeat=no_of_digits)
    )

def digit_combinations(allowed_digits, maximum):
    return itertools.takewhile(
        lambda x: x < maximum,
        all_digit_combinations(allowed_digits)
    )

if __name__ == '__main__':
    print ', '.join(map(str, digit_combinations([1, 3, 7, 9], 100)))
    print ', '.join(map(str, digit_combinations([3, 5], 1000)))

As a performance is concern, your code is quite a bit faster than mine.
I used the following code:

import itertools
import timeit
from functools import wraps

def all_digit_combinations_original(allowed_digits, maximum):
    no_of_digits = 1
    while True:
        for digit_tuple in itertools.product(allowed_digits, repeat=no_of_digits):
            new_number = reduce(lambda rst, d: rst * 10 + d, digit_tuple)
            if new_number < maximum:
                yield new_number
            else:
                raise StopIteration
        no_of_digits += 1

def all_digit_combinations_enhanced(allowed_digits, maximum):
    for no_of_digits in itertools.count(1):
        for digit_tuple in itertools.product(allowed_digits, repeat=no_of_digits):
            new_number = reduce(lambda rst, d: rst * 10 + d, digit_tuple)
            if new_number < maximum:
                yield new_number
            else:
                raise StopIteration

def all_digit_combinations_answer(allowed_digits):
    return (
        reduce(lambda rst, d: rst * 10 + d, digit_tuple)
        for no_of_digits in itertools.count(1)
        for digit_tuple in itertools.product(allowed_digits, repeat=no_of_digits)
    )

def digit_combinations_answer(allowed_digits, maximum):
    return itertools.takewhile(
        lambda x: x < maximum,
        all_digit_combinations_answer(allowed_digits)
    )

def digit_combinations_enhanced(allowed_digits, maximum):
    return itertools.takewhile(
        lambda x: x < maximum,
        (
            reduce(lambda rst, d: rst * 10 + d, digit_tuple)
            for no_of_digits in itertools.count(1)
            for digit_tuple in itertools.product(allowed_digits, repeat=no_of_digits)
        )
    )

def timer_wrapper(*args, **kwargs):
    def wrapper(fn):
        @wraps(fn)
        def run():
            list(fn(*args, **kwargs))
        return run
    return wrapper

def timeit_(fn):
    print fn.__name__, timeit.timeit(fn)

if __name__ == '__main__':
    timer = timer_wrapper([1, 3, 7, 9], 100)
    timeit_(timer(all_digit_combinations_original))
    timeit_(timer(digit_combinations_answer))
    timeit_(timer(all_digit_combinations_enhanced))
    timeit_(timer(digit_combinations_enhanced))

    timer = timer_wrapper([3, 5], 1000)
    timeit_(timer(all_digit_combinations_original))
    timeit_(timer(digit_combinations_answer))
    timeit_(timer(all_digit_combinations_enhanced))
    timeit_(timer(digit_combinations_enhanced))

I obtained the following benchmarks.

all_digit_combinations_original 14.666793108
digit_combinations_answer 18.1881940365
all_digit_combinations_enhanced 15.228415966
digit_combinations_enhanced 17.9368011951

all_digit_combinations_original 14.4108960629
digit_combinations_answer 17.2161641121
all_digit_combinations_enhanced 14.8418960571
digit_combinations_enhanced 17.1169350147


Checking whether a host has a second-level domain from a list I have an url and need to check whether it has some second-level domain (SLD) from a list. For example for SLD foo.com I need to approve hosts foo.com, some.foo.com and reject host kungfoo.com. There is some solution:

domains_a = ['foo.com', 'bar.com', 'fur.net', 'beer.org']
domains_dot_a = ['.foo.com', '.bar.com', '.fur.net', '.beer.org']

domains_b = ['door.com', ... ]
domains_dot_b = ['.door.com', ... ]

domains_c = ['doom.com', ... ]
domains_dot_c = ['.doom.com', ... ]

def any_sld_matches(domains, domains_dot, host):
    return (host in domains) or any(host.endswith(sld) for sld in domains_dot)

def get_url_category(url):
    host = urlparse.urlparse(url.strip()).netloc
    if any_sld_matches(domains_a, domains_dot_a, host):
        return 'a'
    elif any_sld_matches(domains_b, domains_dot_b, host):
        return 'b'
    elif any_sld_matches(domains_c, domains_dot_c, host):
        return 'c'
    elif not host:
        return 'empty'
    else:
        return None

You can change your function to be much faster if you use a set.
You can also use re to reduce the amount of code too.

The regex that you can use is an optional non-capturing group of anything ending with a  ..
And a capturing group at the end of the string with one period:

(?=.+\.)?([^\.]+\.[^\.]+)$

Using this you can lop off the second-level domain, and do a single check.

def get_url_category(url):
    host = urlparse.urlparse(url.strip()).netloc
    domain = re.search(r'(?=.+\.)?([^\.]+\.[^\.]+)$', host)
    if domain is not None:
        domain = domain.group(1)
        if domain in domains_a:
            return 'a'
    if not host:
        return 'empty'
    return None

To make this faster you should change domains_a to a set:

domains_a = {'foo.com', 'bar.com', 'fur.net', 'beer.org'}

And finally to make the program easier to write you can change the domains to be a dictionary of sets.
To use this just loop through the items via domains.iteritems().
And if the host is found then you return the group.

domains = {
    'a': {'foo.com', 'bar.com', 'fur.net', 'beer.org'},
    'b': {'door.com', ...},
    'c': {'doom.com', ...}
}

def get_url_category(url):
    host = urlparse.urlparse(url.strip()).netloc
    domain = re.search(r'(?=.+\.)?([^\.]+\.[^\.]+)$', host)
    if domain is not None:
        domain = domain.group(1)
        for group, domains_ in domains.iteritems():
            if domain in domains_:
                return group
    if not host:
        return 'empty'
    return None

Finally I think returning None is a bit strange, but is fine.


Finding numbers with odd frequency in an array Below is my Code, I want to know ways to improve it.
const findOdd = (A) => {

  newObj={};

  A.filter((cur,index) =>{
    return A.indexOf(cur)===index;
  }).forEach((cur) =>{
    newObj[cur] = 0
    for(let i = 0; i<A.length; i++) if(cur === A[i]) newObj[cur] +=1;
  });

  const keys = Object.keys(newObj)
  key = keys.filter(cur => newObj[cur] % 2 !== 0)

  return parseInt(key[0]);
}

Test Case :
A = [1,1,2,-2,5,2,4,4,-1,-2,5]
Output = -1
Edit : I am trying to create a list of distinct elements at first then add them to an object with key as elements and value as their frequency, then finally finding the keys with odd frequencies.

Undeclared variables You aren't declaring your newObj variable or your key variable. This will either implicitly create properties on the global object, or throw an error in strict mode. Both should be avoided. Always declare your variables (with const, or let when const isn't usable) before using them for the first time. Eg, change newObj={}; to const newObj = {};.
Algorithm complexity Your current approach is pretty computationally expensive. .indexOf will iterate through all elements of the array until it finds a match, or until it reaches the end of the array. .filter will iterate through all elements of an array. With an .indexOf inside a .filter, the computational complexity is O(n ^ 2). (For example, given an array of 30 items, you can expect to have to carry out 900 times more operations than an array with only 1 item, worst-case.) That's not very good.
Inside the forEach, you again iterate over every element of the array, resulting in another operation of O(n ^ 2) complexity.
To reduce the overall complexity of the whole findOdd function to O(n), iterate over the input array only once, and inside the loop, either create a property on the object, or increment the existing property. (See snippet below for a demo.)
Variable names You have variable names A and newObj. At a glance, these are not very informative. Better to give them names that represent what they contain so you can understand them at a glance, such as inputNumbers and occurrencesByNumber. Also, the variable named key isn't actually a key - it's an array of keys. Better to make it plural: keys.
Semicolons Sometimes you're using semicolons, sometimes you aren't. Unless you're an expert, I'd recommend using semicolons, else you may occasionally run into hard-to-understand errors due to Automatic Semicolon Insertion. I'd highly a linter (not just for semicolons, but for many ways to prompt you to correct potential bugs before they turn into errors, and to enforce a consistent code style)
Iterating over an object You collect the keys using Object.keys, then you iterate over the value at each key by accessing the property of the object: newObj[cur]. If you want to iterate over keys and values at once, you can consider using Object.entries instead.
.find or .filter? If the objective is to

then finally finding the keys with odd frequencies.

then
return parseInt(key[0])

is not the right approach, since it'll return only the first element of the array. If you want only one match to be returned, use .find above instead of .filter, so that it ends as soon as a match is found. If you want more than one match to be found, return the array of keys/entries, mapped to numbers.
Demo implementing these fixes:

const findOdd = (inputNumbers) => {
  const occurrencesByNumber = {};
  for (const number of inputNumbers) {
    occurrencesByNumber[number] = (occurrencesByNumber[number] || 0) + 1;
  }
  return Object.entries(occurrencesByNumber)
    .filter(([, occurrences]) => occurrences % 2 === 1)
    .map(([numberKey]) => Number(numberKey));
};

console.log(findOdd([1, 1, 2, -2, 5, 2, 4, 4, -1, -2, 5]));
console.log(findOdd([1, 1, 2]));
console.log(findOdd([0, 1, 2]));


How to delete last element in java.util.Set efficiently? List<Route> listOfRoutes = routeStopsService.getAllRoutes();
Set<String> listOfStopNames = new TreeSet<String>();
for(Route route:listOfRoutes){
    Set<Stop> stops = routeStopsService.getStops(route);
    for(Stop stop:stops)
        listOfStopNames .add(stop.getStopName());
    listOfStopNames .remove( ((TreeSet<String>) listOfSources).last());
}

Here I am getting list of stop names for every route, and I am removing the last stop name from every route.

First of all, you should always use block statements with the for loops. The original formatting of the question was this:

for(Stop stop:stops)
 listOfStopNames .add(stop.getStopName());
 listOfStopNames .remove( ((TreeSet<String>) listOfSources).last());

It's hard to read. Readers could think that the remove call is part of the for loop since it's the same indetation level as the line before.

for (final Stop stop : stops) {
    listOfStopNames.add(stop.getStopName());
}
listOfStopNames.remove(listOfSources.last());

Hungartion notation is redundant, try to avoid it. A few new names:

listOfRoutes -> routes
listOfStopNames -> stopNames
listOfSources -> sources

As @luketorjussen already mentioned, downcasting is nasty. If the implementation of listOfSources changes it may return another type and the compiler will not warn you but you'll get exceptions at runtime. At least change the casting to SortedSet instead of the TreeSet. TreeSet implements SortedSet and SortedSet has the required last method.

I'd extract out some method for better readability. For further review you should share some code from the routeStopsService, Router and Stop classes.

private Set<String> getStopNamesWithoutLast(final Route route) {
    final Set<String> result = getStopNames(route);
    final String lastStopName = ((SortedSet<String>) sources).last();
    result.remove(lastStopName);
    return result;
}

private Set<String> getStopNames(final Route route) {
    final Set<String> result = new TreeSet<String>();
    final Set<Stop> stops = routeStopsService.getStops(route);
    for (final Stop stop : stops) {
        final String stopName = stop.getStopName();
        result.add(stopName);
    }

    return result;
}

final List<Route> routes = routeStopsService.getAllRoutes();
final Set<String> allStopName = new TreeSet<String>();
for (final Route route : routes) {
    final Set<String> stopNames = getStopNamesWithoutLast(route);
    allStopName.addAll(stopNames);
}


Wrapping a non-unique handle with additional information I have a situation where I'd like to wrap up some plain C handles into a class, with a destructor and other niceties, in C++. I would like the wrapper class to have the exact same size as the handle, so I can pass an array of the wrappers into the C api without having to allocate and translate them first.

The handle is not unique - it has some extra information, which is known at creation time - so I can't use the handle itself as a key in one big map. Instead, I'm trying to use the address of the handle as the key.

Details aren't really important on this one - it has data, savvy?

class InstanceMetadata { ... };

The header for the wrapper class:

class Instance
{
public:
    Instance(const Instance&) = delete;
    Instance(Instance&& other);
    Instance(InstanceHandle handle, InstanceMetadata metadata);
    ~Instance();

    Instance& operator=(Instance&) = delete;
    Instance& operator=(Instance&& other);

    InstanceHandle GetHandle() const;
    void ResetHandle();

private:
    InstanceHandle handle;
};

A thin wrapper around std::map, which handles synchronization and nullptrs

template<typename Handle, typename Metadata>
class MetadataCache
{
public:
    void Set(Handle& key, Metadata* value)
    {
        std::lock_guard<std::mutex> guard(mutex);
        data[&key] = value;
    }

    Metadata* Get(Handle& key)
    {
        std::lock_guard<std::mutex> guard(mutex);
        std::map<Handle*, Metadata*>::iterator it = data.find(&key);
        if (it == data.end())
            return nullptr;
        return it->second;
    }

    void Erase(Handle& key)
    {
        std::lock_guard<std::mutex> guard(mutex);
        std::map<Handle*, Metadata*>::iterator it = data.find(&key);
        if (it != data.end())
            data.erase(it);
    }

private:
    std::mutex mutex;
    std::map<Handle*, Metadata*> data;
};

The implementation for the wrapper class:

using namespace std;
using namespace vulkan;

MetadataCache<InstanceHandle, InstanceMetadata> cache;

Instance::Instance(Instance&& other) :
    handle(other.handle)
{
    other.handle = InstanceHandle();
    cache.Set(this->handle, cache.Get(other.handle));
    cache.Erase(other.handle);
}

Instance::Instance(InstanceHandle handle, InstanceMetadata metadata) :
    handle(handle)
{
    cache.Set(handle, new InstanceMetadata(metadata));
}

Instance::~Instance()
{
    delete cache.Get(handle);
    if (handle)
        vkDestroyInstance(handle, nullptr);
}

Instance& Instance::operator=(Instance&& other)
{
    delete cache.Get(handle);
    cache.Set(handle, cache.Get(other.handle));
    cache.Erase(other.handle);
    other.handle = InstanceHandle();
    return *this;
}

InstanceHandle Instance::GetHandle() const
{
    return handle;
}

void Instance::ResetHandle()
{
    delete cache.Get(handle);
    handle = InstanceHandle();
}

The default constructor for InstanceHandle creates a null handle.

I'm still fairly new to move-semantics, so the main things I would like to know are:

Is it safe to do this? (i.e. have I missed an operator or something?)
Is there another way to do this?
Is there any good reason not to do this?

Keep in mind I don't really care about performance, as these objects will not be created or destroyed very often.

Because I also had to deal with a lot of handles, used by for example the Win32 API, I really can understand your use case.

  Is it safe to do this? (i.e. have I missed an operator or something?)

I don't think so, for a lot of reasons: Let's assume the handle your library defines is an opaque type, usually a void*. So the first problem is that the C-library has no idea of C++ concepts like operator overloading. So it will not call your operator= i.e. nor will the compiler disallow to assign named instances. Furthermore the library will deference your handle, assuming it points to it's internal data structure of some kind. Also the compiler has no idea that you have given the library a instance different from handle, while translating the library code. So even compiling the library with a C++ compiler won't make a difference.

  Is there another way to do this?

Although you're desire to design a nice C++ wrapper is a good idea, I would drop the idea to slip the C-library your type. I recommend to just define a .native_handle() member function or overload the operator* but then I would suggest that the name of your type does stress the wrapper nature more.

  Is there any good reason not to do this?

Despite from the reasons above, the size would already change if someone would sub or base class it.


Wrapping bound methods I've written this small class in Python that wraps bound methods but does not prevent the deletion of self.

Do you have any thoughts on my code?
Do you think I handle errors appropriately?
Is it missing anything?
What should be done to make it more robust?
Is the documentation clear enough?

import weakref

class WeakBoundMethod:
    """
    Wrapper around a method bound to a class instance. As opposed to bare
    bound methods, it holds only a weak reference to the `self` object,
    allowing it to be deleted.

    This can be useful when implementing certain kinds of systems that
    manage callback functions, such as an event manager.

    """
    def __init__(self, meth):
        """
        Initializes the class instance. It should be ensured that methods
        passed through the `meth` parameter are always bound methods. Static
        methods and free functions will produce an `AssertionError`.

        """
        assert (hasattr(meth, '__func__') and hasattr(meth, '__self__')),\
               'Object is not a bound method.'

        self._self = weakref.ref(meth.__self__)
        self._func = meth.__func__

    def __call__(self, *args, **kw):
        """
        Calls the bound method and returns whatever object the method returns.
        Any arguments passed to this will also be forwarded to the method.

        In case an exception is raised by the bound method, it will be
        caught and thrown again to the caller of this `WeakBoundMethod` object.

        Calling this on objects that have been collected will result in
        an `AssertionError` being raised.

        """
        assert self.alive(), 'Bound method called on deleted object.'

        try:
            return self._func(self._self(), *args, **kw)
        except Exception, e:
            raise e

    def alive(self):
        """
        Checks whether the `self` object the method is bound to has
        been collected.

        """
        return self._self() is not None

class WeakBoundMethod:

I suggest making it a new-style class by inheriting from object.

    assert (hasattr(meth, '__func__') and hasattr(meth, '__self__')),\
           'Object is not a bound method.'

Don't do this. Just let the invalid parameter types raise attribute errors when you try to fetch the __func__ and __self__. You don't gain anything by checking them beforehand.

assert self.alive(), 'Bound method called on deleted object.'

Raising an Assertion here is a bad choice. Assertions are for thing that should never happen, but having the underlying self object cleaned up doesn't really count. Raise an exception like weakref.ReferenceError. That way a caller can reasonably catch the error.

The documentation is very clear, well done.

EDIT

    try:
        return self._func(self._self(), *args, **kw)
    except Exception as e:
        raise e

Why are you catching an exception only to rethrow it? That's really pointless.

I'd write the whole function as:

def __call__(self, *args, **kw):
    _self = self._self()
    if _self is None:
        raise weakref.ReferenceError()

    return self._func(_self, *args, **kw)


Flexible 2P TicTacToe I have spent hours on this code. I am a beginner and open to advice and improvements. I've put # where I feel insecure about the code.

It lets you pick your marker, checks winner, checks draw, ask player if want to play again and checks if the square is occupied.

#TicTacToe
def drawBoard(board):
      print('   |   |')
      print(' ' + board[7] + ' | ' + board[8] + ' | ' + board[9])
      print('   |   |')
      print('-----------')
      print('   |   |')
      print(' ' + board[4] + ' | ' + board[5] + ' | ' + board[6])
      print('   |   |')
      print('-----------')
      print('   |   |')
      print(' ' + board[1] + ' | ' + board[2] + ' | ' + board[3])
      print('   |   |')

game_board = ['#', ' ', ' ',' ', ' ',' ', ' ',' ', ' ',' ']

player1=''
player2=''

def setplayersmove(playermarker):
    print("Select your marker: ")
    playermarker = input(">  ")
    return playermarker

def makemove(someone):
    print("Enter a number 1-9 for your move: ")
    movenum = int(input('> '))
    if movenum < 1 or movenum > 9:
        print("Number between 1 and 9 only.")
        makemove(someone)
    else:#makes sure the square is not occupied by ANYONE
        if game_board[movenum] != player1 and game_board[movenum] != player2:
            game_board[movenum] = someone
        else:
            print("Move already occupied. Try again: ")
            makemove(someone)

def tryagain():
    print("Try again?: Y/N")
    ans = input('> ').lower()
    if ans == 'y':
        gamestart()
    else:
        exit(0)

def checkwinner():
    if game_board[1] == game_board[2] == game_board[3] and game_board[1] != ' ':
        print("We have a winner!")
        tryagain()
    elif game_board[4] == game_board[5] == game_board[6] and game_board[4] != ' ':
        print("We have a winner!")
        tryagain()
    elif game_board[7] == game_board[8] == game_board[9] and game_board[7] != ' ':
        print("We have a winner!")
        tryagain()
    elif game_board[1] == game_board[4] == game_board[7] and game_board[1] != ' ':
        print("We have a winner!")
        tryagain()
    elif game_board[1] == game_board[5] == game_board[9] and game_board[1] != ' ':
        print("We have a winner!")
        tryagain()
    elif game_board[2] == game_board[5] == game_board[8] and game_board[2] != ' ':
        print("We have a winner!")
        tryagain()
    elif game_board[1] == game_board[2] == game_board[3] and game_board[1] != ' ':
        print("We have a winner!")
        tryagain()
    elif game_board[3] == game_board[5] == game_board[7] and game_board[3] != ' ':
        print("We have a winner!")
        tryagain()

def checkdraw():#checking draw. Advice welcome here...
    count = 0
    for item in game_board:
        if item != ' ':
            count += 1
    if count == 10:
        print("---Game Drawn---")
        drawBoard(game_board)
        tryagain()

def gamestart():

    global player1#ways to avoid using global?
    global player2
    global game_board

    game_board = ['#', ' ', ' ',' ', ' ',' ', ' ',' ', ' ',' ']
    drawBoard(game_board)
    print("---Player I--- ")
    player1 = setplayersmove(player1)
    print("---Player II--- ")
    player2 = setplayersmove(player2)

    gameon1 = True

    while gameon1:
        print("---Player I's Turn---")
        makemove(player1)
        drawBoard(game_board)
        gameon2 = True
        checkwinner()
        checkdraw()

        gameon1 = False

        while gameon2:
            print("---Player II's Turn---")
            makemove(player2)
            drawBoard(game_board)
            gameon1 = True
            checkwinner()
            checkdraw()
            gameon2 = False

gamestart()

You should code to make sure that user input doesn't break any assumptions you are making about the game. For example, I like the gimmick that the players can pick a symbol. However, you should make sure they pick a sensible one. What happens if they pick the same? What about if they pick space?

I would suggest that in the code you use fixed indicators, and replace them when you display them. If not, do some input validation to block weird choices.

As to your question about the draw detection, counting is standard. You could keep a variable to hold the count, and update it each time someone plays. That avoids most of the work recalculating each time.

Your technique for swapping between the two players works and is quite creative. However, I would strongly recommend that you have one outside loop to keep the game going and two distinct inner loops to get player input. This makes it clearer that the behaviour of the players is symmetrical.

Building on that symmetry, you could then have just one input loop and keep a variable recording whose turn it is.


API request and database insert This is a pretty big undertaking for me with my basic skills, and I was kind of flying blind. It's basic in its intent: get a bunch of case info from a helpdesk app and put it into a database. There are >10k records, so I had to overcome some pagination challenges. It works, and I would love your opinions regarding best practices or making it more pythonic.

import time
import collections
from desk_auth import BASE_URL, service, session
import MySQLdb
import mysql.connector
from mysql.connector import errorcode
from db_info import uname,pw,host,db
import dateutil.parser

PAGER = 100

   def get_all_cases():
        """
    pages through items,
    returns a dict of dicts of case, cust, date
    #cases[case] = {cust:date}
    """
        i = 1
        all_cases = []
        is_next = ""

        while is_next is not None:
            endpoint_url = '/api/v2/cases/search'
            endpoint = ''.join([BASE_URL, endpoint_url])
            query_params = {
                'page':i,
                'per_page': PAGER,
                'status': 'new,open,pending', #,resolved,closed',
                'fields': 'created_at,status,labels,subject,updated_at'
                }
            response = session.get(endpoint, params=query_params)
            items = response.json()
            data = items['_embedded']['entries']
            is_next = items['_links']['next']
            for item in data:
                #case id, customer id
                case = get_clean_id(item['_links']['self']['href'])
                cust = get_clean_id(item['_links']['customer']['href'])

                #create date
                raw_date = item['created_at']
                p = dateutil.parser.parse(raw_date)
                date = '%s-%s-%s %s:%s:%s' % (p.year, p.month, p.day, p.hour, p.minute, '00')

                #last update
                raw_update = item['updated_at']
                l = dateutil.parser.parse(raw_update)
                last_update = '%s-%s-%s %s:%s:%s' % (l.year, l.month, l.day, l.hour, l.minute, '00')

                #subject, status
                subj = item['subject']
                status = item['status']

                #get plan from labels
                plan = get_plan(item['labels'])

                #get group id or set to 0
                if item['_links']['assigned_group'] is not None:
                    group = get_clean_id(item['_links']['assigned_group']['href'])
                else:
                    group = '0'

                #get user id or set to 0
                if item['_links']['assigned_user'] is not None:
                    assigned_to = get_clean_id(item['_links']['assigned_user']['href'])
                else:
                    assigned_to = 0

                cases = (case, cust, date, last_update, subj, status, plan, group, assigned_to)
                all_cases.append(cases)
            i += 1

            #prevents rate limiting over 60 calls/minute
            time.sleep(1)
        return all_cases

This inserts into the database:

    def add_cases(cases_dict):
        """
    Takes the list of known kanji and inserts into database.
    """
        cnx = mysql.connector.connect(user=uname, password=pw, host=host, database=db, charset='utf8')
        cursor = cnx.cursor()
        try:
            for data in cases_dict:
                print data
                sql = """INSERT INTO cases(id, customer_id, date, last_update, subject, status, plan, group_id, assigned_id)
    VALUES (%s, %s, %s , %s, %s, %s, %s, %s, %s);"""
                cursor.execute(sql, data)
                cnx.commit()

        except MySQLdb.Error, e:
            print "Error %d: %s" % (e.args[0], e.args[1])
            cnx.rollback()
        cursor.close()
        cnx.close()

Your code doesn't seem to be the complete module (e.g. get_clean_id() is referred to but not defined or imported, and I don't know what desk_auth.session does), so I won't try and understand the whole thing. I'll stick to comments about the general coding style.

Starting with your imports:

import time
import collections
from desk_auth import BASE_URL, service, session
import MySQLdb
import mysql.connector
from mysql.connector import errorcode
from db_info import uname,pw,host,db
import dateutil.parser

The first thing that comes to mind when I see this is that it doesn't look organised. Different people organise their imports differently, but to me it's a bad sign if there doesn't seem to be any order at all. The PEP-8 (coding standards for Python) suggest the following order:

standard library imports
related third party imports
local application/library specific imports

I generally put a blank line between each of these sections.

Use judgement about when to use import ... and when to use from ... import .... I won't go into the full details here, but in short I avoid this when the imported names are very short or generic. Names like pw or db could easily get confused with variables elsewhere in your code.

   def get_all_cases():
        """
    pages through items,
    returns a dict of dicts of case, cust, date
    #cases[case] = {cust:date}
    """

You have an indentation issue here. It may just be an issue with the StackExchange markup, but if not you should fix it. The function should be declared at the beginning of the line (assuming it's not a class method for which you've cut out the definition of the class).

The name get_all_cases() is not very specific. Whether this is a problem will depend on how the function is going to be called. If it were a class method, then the context might be enough to make it meaningful (e.g. doctor.get_all_cases() is quite clear). In Python, free (non-method) functions are often called by reference to the module they live in, which can make it clear what it means in the same way as an object name would. However, you need to ask yourself whether this is clear enough. My instinct is that this function name needs to be clearer.

i = 1

Avoid single-character variable names for anything but the control variable on a short for loop (e.g. for i in range(10): print x[i]). For something that is used over a large function like this, give it a proper name. It looks like this should be called page.

Generally, I think the get_all_cases() function is too long. Breaking it into smaller functions will make the logic easier to follow.

If I'm reading this right, you have quite a simple flow:

have_next = True
while have_next:
    items, have_next = get_data()
    all_items += items

return items

(this is just a simple sketch; I'm not suggesting you use variable or function names like this).

Moving the API call into one function and the work to create the cases out of the raw API result into another function would make this structure more obvious.

            cases = (case, cust, date, last_update, subj, status, plan, group, assigned_to)
            all_cases.append(cases)

Using tuples to represent entities when you have more than two or three elements quickly gets unmaintainable. If you want to get the status out of this, you have to remember to do case[5], which isn't very clear to read. If you end up wanting to add another field to this, then you have to insert it at the end or risk breaking any piece of code that refers to the case elements.

One alternative to using a tuple is to declare a class type and then use instances of this class type to represent your cases, but this might be unnecessary overhead in this case (it depends how widely you're using these objects). A simpler alternative is to use a namedtuple. That way you can refer to case.subj, case.status etc. in your later code.


Tic Tac Toe code could probably be optimized more I recently got some help from Stack Overflow on my Tic-Tac-Toe code for the win conditions, but I think it could be optimized a little more. Any tips?

moves = [["1", "2", "3"],
         ["1", "2", "3"],
         ["1", "2", "3"]]
acc = 0
def win(letter):
  for i in range(3):
      if ((moves[i][0] == moves[i][1] == moves[i][2] == letter) or
          (moves[0][i] == moves[1][i] == moves[2][i] == letter)):
        print("~~~ " + letter + " WON!!! CONGRATS!!!! ~~~")
        quit()
  if (set(moves[i][i] for i in range(3)) == set([letter])):
      print("~~~ " + letter + " WON!!! CONGRATS!!!! ~~~")
      quit()
  if (set(moves[i][2-i] for i in range(3)) == set([letter])):
      print("~~~ " + letter + " WON!!! CONGRATS!!!! ~~~")
      quit()
  if (acc == 5):
      print("TIE YOU BOTH LOSE")
      quit()
def playerInput():
  global acc
  player1 = input("Where do you want to place your X, player 1? (row number, space, number)")
  moves[int(player1[0]) - 1][int(player1[2]) - 1] = "X"
  acc += 1
  win("X")
  player2 = input("Where do you want to place your O, player 2? (row number, space, number)")
  moves[int(player2[0]) - 1][int(player2[2]) - 1] = "O"
  boardDraw()
def boardDraw():
  print("1| "+moves[0][0]+" | "+moves[0][1]+" | "+moves[0][2]+" |")
  print(" |---+---+---|")
  print("2| "+moves[1][0]+" | "+moves[1][1]+" | "+moves[1][2]+" |")
  print(" |---+---+---|")
  print("3| "+moves[2][0]+" | "+moves[2][1]+" | "+moves[2][2]+" |")
  win("X")
  win("O")
  playerInput()
print("OK SO....\nPlayer 1 is X\nPlayer 2 is O\nGOOOOO!!")
boardDraw()

Recently, "Tic-tac-toe" game is gaining popularity on CR.It may even deserve a separate playlist like "Tic-tac-toe" Refactorings on various programming languages.You may find how other guys on CR already succeeded with different implementations (some on pure Python, some - involving numpy functionality).

But let's return to your current variation.

moves list. It's better named as board (as a game field).Instead of typing all board items it could be easily initiated with board = [['1','2','3'] for _ in range(3)]
win function. It sounds like affirmative but it's actually testing for a win.Thus a better name would be check_win
set(moves[i][i] for i in range(3)) == set([letter]) condition. Instead of generating 2 set objects - use a convenient all function:

all(board[i][i] == letter for i in range(3))

for loop (in check_win function) that checks for crossed row can be replaced with any call to serve a more wide consolidation.
2 statements:

print("~~~ " + letter + " WON!!! CONGRATS!!!! ~~~")
quit()

are duplicated across 3 different conditions. That's an explicit case for Consolidate conditional expression technique - we'll combine the conditions that share the same "body" with logical operator.Eventually the optimized check_win function would look as:

def check_win(letter):
    if any((board[i][0] == board[i][1] == board[i][2] == letter) or
            (board[0][i] == board[1][i] == board[2][i] == letter) for i in range(3)) \
            or all(board[i][i] == letter for i in range(3)) \
            or all(board[i][2 - i] == letter for i in range(3)):
        print(f"~~~ {letter} WON!!! CONGRATS!!!! ~~~")
        quit()
    if acc == 5:
        print("TIE YOU BOTH LOSE")
        quit()

playerInput and boardDraw should be renamed to player_input and board_draw to follow naming conventions
printing game's board in board_draw function looks really sad and painful.Python provides a good string formatting features.Just use them, as shown below:

def board_draw():
    board_fmt = '''
1| {} | {} | {} |
 |---+---+---|
2| {} | {} | {} |
 |---+---+---|
3| {} | {} | {} |
    '''
    print(board_fmt.format(*[v for row in board for v in row]))

    check_win("X")
    check_win("O")
    player_input()


Java Exception Message I have a few custom exception classes that I created simply for the sake of having my own exception message:

public class DivideByZeroException extends Exception
{
   @Override
   public String toString()
   {
       return "ERROR: Expression cannot divide by 0";
   }
}

I realize that this can be done by throwing an exception like:

throw new Exception("ERROR: Expression cannot divide by 0");

but this contains a prefix of java.lang.Exception: in the string.

I think it's a bit ugly to remove the unwanted prefix from the string, but creating a class just for the exception message seems a bit excessive to me. Is there a different way to do this?

Don't throw raw Exceptions, use always a subclass (as explained by MrLore). If you are throwing raw exceptions you also have to catch them, which might mask other exception or runtime exceptions. You would add a instanceof to any of your catch blocks.


jQuery File Tree Toggle I am making a web app that will have a "my files" area and to do so I am writing my own html and jQuery to control the opening and closing of each folder (or ul li). I want to make sure that this web app is as efficient as I can make it as it may have to deal with a large amount of hierarchy.

jsFiddle

<!doctype html>

        <meta charset="UTF-8" />
        <meta name="viewport" content="width=device-width" />
        <title>Title</title>

                    Animals

                            Birds

                            Mammals

Elephant

Mouse

                            Reptiles

Plants

                                Flowers

Rose

Tulip

Trees

Step 1: Fix your markup. You have the "plants"-list nested inside the link, which is invalid

Step 2: Fix your JS. You correctly add the click handler after the document's ready, but you set the folderToggle variable when the script's run, regardless of whether the document's been loaded. It still works because you have the script at the end of your page, but still. Keep everything inside the $(...) - it'll also keep the folderToggle var out of the global scope.

Step 2: Cache jQuery objects. Don't write $(this) twice.

Step 3: Don't use find() for this. While I can't speak to an exact performance difference, it's more precise to use next(), as the nested list will be the next element/sibling relative to the link.

$(function () {
    $(".tree li > a").click(function(e) {
        var link = $(this);
        e.preventDefault();
        link.next().toggle("slow");
        link.toggleClass("active");
    });
});

Step 4: If you're going to be adding more stuff to the tree via ajax or something, you'll probably want to rewrite the click handler to handle clicks for links added later:

$(".tree").on("click", "li > a", function (event) {
    ...
});

Step 5: You'll probably want to add a class to the "branch" links in the tree. Otherwise "leaf" links will also attempt to open a nested list, which doesn't make much sense, as there won't be one.

Step 6: Use OL (ordered list) unless your lists are actually "unordered". But if they're file lists, they're probably ordered somehow. Minor thing, but semantics matter.

All that said, there's nothing terribly inefficient in your code. If anything's going to lag, it's the toggle animation, but that can't really be sped up, as it's a single function call to jQuery. It's either there or not there.


Pidfile single process instance mechanism I'm writing a system daemon in Rust that may be started by the user manually. Only one instance of the daemon should be running at a time
so I have created a pidfile locking mechanism to prevent multiple instances. This is my first time doing any kind of unix style file locking
and I would appreciate another pair of eyes to check for anything I may have missed.
Style pointers would also be great as I am new to Rust.

use nix::fcntl::{flock, FlockArg};
use nix::sys::signal::kill;
use nix::unistd::Pid;
use std::fs::File;
use std::fs::OpenOptions;
use std::io;
use std::io::{Error, ErrorKind, Read, Seek, SeekFrom, Write};
use std::os::unix::io::AsRawFd;
use std::path;
use std::process;

fn write_pid(mut file: &File) -> Result<(), io::Error> {
    let id = process::id().to_string() + "\n";
    file.write_all(id.as_bytes())?;
    Ok(())
}

fn lock_pidfile(file: &File, pidfile: &path::Path) -> Result<(), io::Error> {
    flock(file.as_raw_fd(), FlockArg::LockExclusiveNonblock).map_err(|_| {
        Error::new(
            ErrorKind::Other,
            format!("Failed to lock pidfile: {}", pidfile.display()),
        )
    })
}

pub fn exclusive(pidfile: &path::Path) -> Result<bool, io::Error> {
    let pf = OpenOptions::new()
        .write(true)
        .create_new(true)
        .open(pidfile);

    match pf {
        Ok(file) => {
            lock_pidfile(&file, pidfile)?;
            write_pid(&file)?;
            Ok(true)
        }
        Err(err) => {
            match err.kind() {
                ErrorKind::AlreadyExists => {
                    let mut file = OpenOptions::new().read(true).write(true).open(pidfile)?;
                    lock_pidfile(&file, pidfile)?;
                    let mut id_str = String::new();
                    file.read_to_string(&mut id_str)?;
                    let id: u32 = id_str.trim().parse().map_err(|_| {
                        Error::new(
                            ErrorKind::Other,
                            format!("Failed to parse pidfile: {}", pidfile.display()),
                        )
                    })?;
                    // Kill None just checks if process exists.
                    // Same as kill(pid, 0); in C
                    if kill(Pid::from_raw(id as i32), None).is_ok() {
                        Ok(false)
                    } else {
                        file.seek(SeekFrom::Start(0))?;
                        write_pid(&file)?;
                        Ok(true)
                    }
                }
                _ => Err(err),
            }
        }
    }
}
```

I don't know much about file locking, but I can guide you in Rust.

I'd suggest changing your write_pid to use writeln!. I think you can simplify it to this.

fn write_pid(mut file: &File) -> Result<(), io::Error> {
    writeln!(file, "{}", process::id())
}

I'd also prefer to reduce the nesting in exclusive, perhaps something like this:

pub fn exclusive(pidfile: &path::Path) -> Result<bool, io::Error> {
    let pf = OpenOptions::new()
        .write(true)
        .create_new(true)
        .open(pidfile);

    match pf {
        Ok(file) => {
            lock_pidfile(&file, pidfile)?;
            write_pid(&file)?;
            return Ok(true);
        }
        Err(ref e) if e.kind() == ErrorKind::AlreadyExists => {}
        Err(e) => return Err(e),
    }

    let mut file = OpenOptions::new().read(true).write(true).open(pidfile)?;
    lock_pidfile(&file, pidfile)?;
    let mut id_str = String::new();
    file.read_to_string(&mut id_str)?;
    let id: u32 = id_str.trim().parse().map_err(|_| {
        Error::new(
            ErrorKind::Other,
            format!("Failed to parse pidfile: {}", pidfile.display()),
        )
    })?;
    // Kill None just checks if process exists.
    // Same as kill(pid, 0); in C
    if kill(Pid::from_raw(id as i32), None).is_ok() {
        Ok(false)
    } else {
        file.seek(SeekFrom::Start(0))?;
        write_pid(&file)?;
        Ok(true)
    }
}


Move update_attributes into class and check for non-nil params to update only There are a number of things wrong with the code here:

 @cust = Customer.find_by_id(params[:customer_id])
        upd = @cust.update_attributes({:city_id => params[:city],
                                       :zip_code => params[:zip],
                                       :first_name => params[:first_name],
                                       :last_name => params[:last_name],
                                       :phone => params[:phone].each { |s| s.gsub!(/\D+/,'')},
                                       :street1 => params[:street1],
                                       :street2 => params[:street2],
                                       :email => params[:email]
    })

It's currently in a controller action - I should move it to the Customer class
My params don't exactly match the keys in my collection (MongoDB) - but it's a bit too late for me to deal with that...
I use the .each with a block inside the update, I feel like I should separate that into it's own method
While I don't explicitly call save, it does actually save.  The documentation on update_attributes says that I should call save explicitly or it won't...

Here are some questions:

What is the right way to move this into the model such that it will return TRUE or FALSE so that my controller can then set a flash[:success] or flash[:error] ?
How do I normalize my params so that I can write ONE UNIVERSAL update method like @cust.update_attributes(params) which will intelligently update only non-nil params ?
Is it absolutely worth the time to go back to the view and normalize the name attributes of each input to match the param?

I have other methods that update more or less params depending on the view, and I want to just have a single method "update_customer" that will take any old params hash with any values and only update the values that exist.

class Customer
  def update_customer params
    update_attributes(:city_id => params[:city],
                      :zip_code => params[:zip],
                      :first_name => params[:first_name],
                      :last_name => params[:last_name],
                      :phone => phone_number_digest(params[:phone]),
                      :street1 => params[:street1],
                      :street2 => params[:street2],
                      :email => params[:email])
  end

  def phone_number_digest *numbers
    numbers.map{ |s| s.gsub!(/\D+/,'')}
  end

end

In your controller

@cust = Customer.find(params[:customer_id])
@cust.update_customer(params)

This keeps it relatively clean and logically separates your persistence logic.

Also to answer your questions.

@cust = Customer.find(params[:customer_id])
if @cust.update_customer(params)
  #do something
else
  #do something else
end

Ruby executes the code in conditionals, so you could just have it like I have above.
you would probably want to do this logic in the controller, as only allow the keys with non nil params to work.  This one is bit trickier, but definitely can be done.  Perhaps you want to build your sanitized hash of params then pass it in.  in that case, perhaps the ** operator(double splat for keyword args) would be useful.
Definitely worth your time. Naming is quite possibly one of the most important ideas in programming.  You want to read code that you worked on a year or 2 ago without having to dive into the internals, and if you properly name you classes and methods, it becomes much simpler.


Dynamically call lambda based on stream input: Try 2 Originally asked here: Dynamically call lambda based on stream input

The following version is based heavily on the answer provided by @iavr, though I hope that I have done enough more to make it worth reviewing again.

A way to clean parameters of all declarations:

template<typename Decorated>
struct CleanType
{
    typedef typename std::remove_reference<Decorated>::type     NoRefType;
    typedef typename std::remove_const<NoRefType>::type         BaseType;
};

Get Caller traits of a functor:

template <typename T>
struct CallerTraits
    : public CallerTraits<decltype(&T::operator())>
{};

template<typename C, typename ...Args>
struct CallerTraits<void (C::*)(Args...) const>
{
    static constexpr int                                        size = sizeof...(Args);
    typedef typename std::make_integer_sequence<int, size>      Sequence;
    typedef std::tuple<typename CleanType<Args>::BaseType...>   AllArgs;
};
// Notice the call to `CleanType` to get the arguments I want.
// So that I can create objects of the correct type before passing
// them to the functor:

The actual calling of the functor based on reading data from a stream (via ResultSetRow (but I am sure that can be easily generalized)).

Done in 2 parts:

This is the part you call:

template<typename Action>
void callFunctor(Action action, Detail::ResultSetRow& row)
{
    // Get information about the action.
    // And retrieve how many parameters myst be read from
    // the stream before we call `action()`
    typedef CallerTraits<decltype(action)>  Trait;
    typedef typename Trait::Sequence        Sequence;

    doCall2(action, row, Sequence());
}

Part 2:

Here we extract the parameters from the stream then call the action.

template<typename Action, int... S>
void doCall2(Action action, Detail::ResultSetRow& row, std::integer_sequence<int, S...> const&)
{
    // Create a tupple that holds all the arguments we need
    // to call the functior `action()`
    typedef CallerTraits<decltype(action)>  Trait;
    typedef typename Trait::AllArgs         ArgumentTupple;

    // Use the template parameter pack expansion
    // To read all the values from the stream.
    // And place them in the tuple.
    ArgumentTupple  arguments(row.getValue1<typename std::tuple_element<S, ArgumentTupple>::type>()...);

    // Use the template parameter pack expansion
    // To call the function expanding the tupple into
    // individual parameters.
    action(std::get<S>(arguments)...);
}

Example usage:

// Using `ResultRowSet` from below.
int main()
{
    std::stringstream    stream("1 Loki 12.3 2.2");
    Detail::ResultRowSet row(stream);
    callFunctor([](int ID, std::string const& person, double item1, float item2){
        std::cout << "Got Row:"
                  << ID     << ", "
                  << person << ", "
                  << item1  << ", "
                  << item2  << "\n";
    }, row);
}

Here it will read an int (ID), a std::string (person) a double (item1) and a float (item2) from the stream (represented by row), then call the lambda provided.

This is not the actual implementation of Detail::ResultSetRow. But for code review purposes you can think of it as:

namespace Detail
{
    class ResultRowSet
    {
        std::istream&   stream;

        public:
            ResultRowSet(std::istream& s)
                : stream(s)
            {}
            template<typename T>
            T getValue1()
            {
                  T  val;
                  stream >> val;
                  return val;
            }
    };
}

It is becoming really good, but there are still a few things that can be done:

There already is a standard way to "clean" a type: std::decay. It does a little bit more than just removing the reference and the const qualification though, but it does basically what you need. Therefore, you can totally get rid of CleanType and use std::decay instead:

using AllArgs = std::tuple<typename std::decay<Args>::type...>;

And since you are using C++14 (I assume this because of std::integer_sequence), you can even use std::decay_t instead:

using AllArgs = std::tuple<std::decay_t<Args>...>;

N3887 also made its way to the C++14 standard. Therefore, if you actually use C++14, you will be able to use the alias template std::tuple_element_t instead of the metafunction std::tuple_element:

ArgumentTupple arguments(row.getValue1<std::tuple_element_t<S, ArgumentTupple>>()...);

This line looks kind of wrong:

typedef typename std::make_integer_sequence<int, size>      Sequence;

When I tried to compile your example, it didn't compile at first because of the typename in this line. std::make_integer_sequence is an alias template, there shouldn't be a typename before it.
You sometimes use ResultRowSet and use ResultSetRow at some other places. It's probably a typo or something like that :p
ArgumentTupple also feels like a typo. It should probably be ArgumentTuple.
std::integer_sequence does not contain anything. Therefore, there is no need to pass is by const&, you can simply pass it by value.


Removing whitespace and space in list that contains different types of elements My goal is to remove whitespaces from string elements of my list, and to remove empty string elements altogether. My code:

def modifylist(lst):
        lst = [elt.strip() if type(elt) is str else elt for elt in lst]
        while '' in lst:
                lst.remove('')
        return lst

Is there any other method that python provides? more elegant and hopefully more fast than using this while loop?

Some suggestions

Follow pep8
Use duck-typing.  So, for example, use elt.strip() if hasattr(elt, 'strip')
It will be much more efficient to filter items in another list comprehension, since it only requires one search through the list rather than many.
The simplest approach would probably be to change the first list comprehension to a generator expression, then feed that to a list comprehension that filters out empty strings.
This could conceivably be a single list comprehension, but I think it would be too verbose.  Further, it would require stripping twice.

So I would do something like this:

def strip_list_noempty(mylist):
    newlist = (item.strip() if hasattr(item, 'strip') else item for item in mylist)
    return [item for item in newlist if item != '']

You could also duck-type the filtering, like so:

def strip_list_noempty(mylist):
    newlist = (item.strip() if hasattr(item, 'strip') else item for item in mylist)
    return [item for item in newlist if item or not hasattr(item, 'strip')]


Search for subsets within a 250k line text dataset The code below works pretty well - it takes about 4 seconds (this seems too slow for me) to find a group that's located in the tail area of the text file I'm searching. The text file has ≃ 250,000 lines with 20 elements per line. I cut my teeth on other programming languages and picked up Python just for this current project I'm working on, so I really am a neophyte when it comes to python efficiency.

with open(file) as infile:
    datadictionary = csv.DictReader(infile, dialect='excel-tab', quoting=csv.QUOTE_NONE)
    for key, group in itertools.groupby(datadictionary, key=lambda x:x[patient_number_field_header] == patient_id):
        if key:
            super_list = group
            break

'patient_id' is a string of digits
file is a text file

I'm wondering what you think - how can I make this more efficient? Am I "doing it wrong"?

I'm assuming your code does some background processing, because otherwise the patient data would be stored in a database, not in a text file. In that scenario, 4 seconds is probably ok.

Instead of grouping the records, you could simply filter them. That saves two lines of code, but will probably not be much faster.


Calculating primes: Could this be any faster? I've ported a prime-number calculation program from primes.pyx to C++ for benchmark purpose.
Since I wrote it in C++, I thought that my program would be faster than the original. However, mine took 25.8 ms at the fastest, while the original took only 1.45 ms on the same machine. I tested 10 times each, but got similar results (25.8~51.7ms vs 1.45~1.47ms). But, why?
Here's my code:
#include <iostream>
#include <vector>
#include <chrono>
using namespace std;

vector<int> primes(size_t nb_primes)
{
    int n;
    vector<int> p;
    p.reserve(nb_primes);

    n = 2;
    while (p.size() < nb_primes)
    {
        bool other = true;
        for (size_t i = 0; i < p.size(); i++)
        {
            if (n % p[i] == 0)
            {
                other = false;
                break;
            }
        }
        if (other)
            p.push_back(n);
        n += 1;
    }
    return p;
}

int main()
{
    auto start = std::chrono::high_resolution_clock::now();

    vector<int> p = primes(1000);
    //for (auto i : p)
    //    cout << i << ' ';

    auto finish = std::chrono::high_resolution_clock::now();

    std::chrono::duration<double> elapsed = finish - start;
    std::cout << "Elapsed Time: " << elapsed.count() << " s\n";
}

These algorithms are exactly the same, I believe.
Please don't limit the check up to sqrt(n), to achieve a sieve of Eratosthenes.
I need to compare with the original.
One thing I'm worried is the for ... else statement in the original.
I borrowed the idea to use the other flag from Username: haccks.
If you can apply another for ... else method, please go ahead.
My Windows 10 machine (i5) spec:
Clock Frequency: 1.60GHz 1.80GHz
Memory: 8.00GB
I wrote the original version on Anaconda Prompt/Python 3.8.
I write the C++ version on Visual Studio 2019.
If you need more information, please ask me.
Thank you in advance.

There are some optimizations that can make your code better, even with compiler optimizations turned on:
Pre-allocate the vector and treat it like an array.
Use a variable to keep track of the length.
Putting these together, I found about a 10% increase in speed:
vector<int> primes(size_t nb_primes)
{
    vector<int> p(nb_primes,2);
    int n = 2;
    size_t len_p = 0;
    while (len_p < nb_primes)
    {
        bool other = true;
        for (size_t i = 0; i < len_p; i++)
        {
            if (n % p[i] == 0)
            {
                other = false;
                break;
            }
        }
        if (other)
            p[len_p++] = n;
        n += 1;
    }
    return p;
}