Models in .NET: separate settings For my models for a certain section I thought it would be better to have a base class model for both "view" and "edit" models, like so:

public abstract class Setting
{
    [Required]
    public int SettingId { get; set; }

    public string Name { get; set; }

    [Required]
    [MaxLength(255)]
    public string Value { get; set; }
    public string ModifiedBy { get; set; }
    public DateTime ModifiedDt { get; set; }
}

public class ViewSetting : Setting
{
}

public class EditSetting : Setting
{
}

So in my Service or Views I use the appropriate ViewSetting or EditSetting. Right now I don't have anything that would be different - but should I follow a pattern like this to maintain a separation of concerns here? Or should I just stick to using one Setting class? (as an aside, I'm using Entity Frameworks but in my Service layer is where I convert to/from the models for the ORM)

The Model wouldn't change regardless of whether you were viewing it or editing the records in the given model.

Let's go ahead and create a view and edit model from the information you have given us, it will look like this

Note: this doesn't work, it is here to make a point.

public abstract class Setting
{
    [Required]
    public int SettingId { get; set; }

    public string Name { get; set; }

    [Required]
    [MaxLength(255)]
    public string Value { get; set; }
    public string ModifiedBy { get; set; }
    public DateTime ModifiedDt { get; set; }
}

public class ViewSetting : Setting
{
    public int SettingId { get; }
    public string Name { get; }
    public string Value { get; }
    public string ModifiedBy { get; }
    public DateTime ModifiedDt { get; }
}

public class EditSetting : Setting
{
    [Required]
    public int SettingId { set; }

    public string Name { set; }

    [Required]
    [MaxLength(255)]
    public string Value { set; }
    public string ModifiedBy { set; }
    public DateTime ModifiedDt { set; }
}

and then you don't want to go setting the ID Field because that would change more than the properties of the data, it would be changing indexing in your database, so we would take that out too.  And if you were going to edit the Record you should also make the ModifiedBy and ModifiedDt required.

Also, because if you implement the Abstract class you implement everything anyway, it really doesn't make sense to create an abstract class, so I make them all regular classes and you can see what is really going on here.

public class Setting
{
    [Required]
    public int SettingId { get; set; }

    public string Name { get; set; }

    [Required]
    [MaxLength(255)]
    public string Value { get; set; }
    public string ModifiedBy { get; set; }
    public DateTime ModifiedDt { get; set; }
}

public class ViewSetting
{
    public int SettingId { get; }
    public string Name { get; }
    public string Value { get; }
    public string ModifiedBy { get; }
    public DateTime ModifiedDt { get; }
}

public class EditSetting
{
    public string Name { set; }

    [Required]
    [MaxLength(255)]
    public string Value { set; }
    [Required]
    public string ModifiedBy { set; }
    [Required]
    public DateTime ModifiedDt { set; }
}

you wouldn't really make separate models for editing and viewing. it isn't needed.

The properties do the getting (get = view) and setting (set = edit). so there isn't really a reason to make two separate models, one for getting and one for setting, when one class/model can do it all


jQuery form preview using the Instagram plugin How can I improve my jQuery form preview implementation? I'm currently using the jQuery Instagram Plugin to get the hashtag from Instagram.

var timeoutReference;
var element;
var tagsa;

$(document).ready(function() {
  $('input#instagram_hashtag_hashtag').keypress(function() {
    if (timeoutReference) clearTimeout(timeoutReference);

      timeoutReference = setTimeout(function() {
        doneTyping($('input#instagram_hashtag_hashtag').val());
      }, 350);

    });
    $('input#instagram_hashtag_hashtag').blur(function(){
        doneTyping($('input#instagram_hashtag_hashtag').val());
    });
});

function doneTyping(tags){
  if (!timeoutReference){
    return;
  }

  tagsa = null;
  tagsa = tags.match(/(^#|\s#)([a-z0-9]+)/gi);

  timeoutReference = null;

  var length = tagsa.length,
    element = null;
  for (var i = 0; i < length; i++) {
    element = tagsa[i];
    $(function(){
      var
        insta_container = $(".instagram")
      , insta_next_url

      insta_container.instagram({
        hash: element.replace("#", "")
      , clientId: 'instagramclientid'
      , show : 4
      })
    });
  }
}

//$(document).ready(fn) can be shortened to $(fn)
//you can use it to avoid global scope pollution so we place everything in here
$(function () {

  //IDs are unique, so we drop input
  //cache values that are static and used more than once
  var instagramHashtag = $('#instagram_hashtag_hashtag');
  var pattern = /(^#|\s#)([a-z0-9]+)/gi;
  var instaContainer = $('.instagram');
  var timer;

  function doneTyping(tags) {

    //why create the timer check? besides, if the function was called and
    //tags was blank, the loop wouldn't run since there are no matches

    //these variables are only used here, so declare them in here
    var tagsa = tags.match(pattern);
    var length = tagsa.length;
    var i = 0;

    while(i < length) {
      instaContainer.instagram({
        hash: tagsa[i++].replace('#', ''),
        clientId: 'instagramclientid',
        show: 4
      });
    }
  }

  //use jQuery on, which accepts a map of events
  //that way, you write the selector part only once
  instagramHashtag.on({
    keypress: function () {
      if(timer) clearTimeout(timer);
      timer = setTimeout(function () {
        doneTyping(instagramHashtag.val());
      }, 350);
    },
    blur: function () {
      doneTyping(instagramHashtag.val());
    }
  });
});


Boost CRC example program file I'm currently looking at this Boost::CRC example code which I have also inserted below.

I always try to look for suggestions for improving my own coding style when I encounter well-written and well-formatted code.

This code definitely looks like good code, but two things puzzle me about this:

is it good practice to copy #defined constants to local const variables before using them? Why would this be a good idea?
is it actually acceptable to just omit the parenthesis around a function body if the body is a try-catch-block?

//  Boost CRC example program file  ------------------------------------------//

//  Copyright 2003 Daryle Walker.  Use, modification, and distribution are
//  subject to the Boost Software License, Version 1.0.  (See accompanying file
//  LICENSE_1_0.txt or a copy at <http://www.boost.org/LICENSE_1_0.txt>.)

//  See <http://www.boost.org/libs/crc/> for the library's home page.

//  Revision History
//  17 Jun 2003  Initial version (Daryle Walker)

#include <boost/crc.hpp>  // for boost::crc_32_type

#include <cstdlib>    // for EXIT_SUCCESS, EXIT_FAILURE
#include <exception>  // for std::exception
#include <fstream>    // for std::ifstream
#include <ios>        // for std::ios_base, etc.
#include <iostream>   // for std::cerr, std::cout
#include <ostream>    // for std::endl

// Redefine this to change to processing buffer size
#ifndef PRIVATE_BUFFER_SIZE
#define PRIVATE_BUFFER_SIZE  1024
#endif

// Global objects
std::streamsize const  buffer_size = PRIVATE_BUFFER_SIZE;

// Main program
int
main
(
    int           argc,
    char const *  argv[]
)
try
{
    boost::crc_32_type  result;

    for ( int i = 1 ; i < argc ; ++i )
    {
        std::ifstream  ifs( argv[i], std::ios_base::binary );

        if ( ifs )
        {
            do
            {
                char  buffer[ buffer_size ];

                ifs.read( buffer, buffer_size );
                result.process_bytes( buffer, ifs.gcount() );
            } while ( ifs );
        }
        else
        {
            std::cerr << "Failed to open file '" << argv[i] << "'."
             << std::endl;
        }
    }

    std::cout << std::hex << std::uppercase << result.checksum() << std::endl;
    return EXIT_SUCCESS;
}
catch ( std::exception &e )
{
    std::cerr << "Found an exception with '" << e.what() << "'." << std::endl;
    return EXIT_FAILURE;
}
catch ( ... )
{
    std::cerr << "Found an unknown exception." << std::endl;
    return EXIT_FAILURE;
}

With the manifest constant being assigned to a const variable, you now have two ways of referring to the same value. In principle, this is superfluous, and should therefore be avoided. In practice, with manifest constants always being globally visible in C++, this is a rather moot point, but it can still be used for making a decision in lack of any other point. So, if there are any other considerations not immediately evident in the code, such as the need to be able to redefine the manifest constant AND at the same time the need to have the constant strongly typed, then by all means, it should be kept, and preferably documented.

The try statement used as a function body looks mighty weird, which means that the average C++ programmer looking at it will go "WTF?".  At the same time it does not accomplish any mighty feat that will make one add "--oh, I see why it is done that way, cool!". It is okay for a coding technique to have a certain WTF factor to it, as long as it accomplishes something neat, the neatness of which is proportional to its WTFness, so as to justify it. All that the try-statement-used-as-a-function-body accomplishes is to spare us from having to type an additional --but expected-- pair of curly brackets. Therefore, it should definitely be avoided.


Custom set data structure in Ruby I wrote a set data structure with some useful functions. I'm not sure if it is a good implementation and if there are other essential functions that a set should support.

class CustomSet

  def initialize(list=[])
    @cs =[]
    list.each do |n|
      put(n)
    end
  end

  def size
    @cs.size
  end

  def each(&blk)
    @cs.each do |n|
      blk.call(n)
    end
  end

  def index(n)
    @cs.index { |x| x.eql?(n) }
  end

  def ==(other_object)
    return false unless other_object.class == self.class
    return false unless other_object.size ==self.size
    other_object.each do |n|
      return false unless index(n)
    end
    true
  end

  def eql?(other)
    self == other
  end

  def hash
    @cs.reduce(:+){ |n| n.hash }
  end

  def delete(n)
    @cs.delete(n) if index(n)
    self
  end

  def difference(other)
    CustomSet.new(@cs.select { |n| other.index(n).nil? })
  end

  def disjoint?(other)
    union(other).size == self.size + other.size
  end

  def intersection(other)
    CustomSet.new(@cs.select { |n| other.index(n) })
  end

  def subset?(other)
    self.size >= other.size && intersection(other) == other
  end

  def union(other)
    u = CustomSet.new(@cs)
    other.each { |n| u.put(n) }
    u
  end

  def member?(n)
    index(n)
  end

  def put(n)
    @cs << n unless index(n)
    self
  end

  def empty
    @cs = []
    self
  end

  def to_list
    @cs
  end

end

Set treats integers and decimals as distinct elements:

irb(main):019:0> Set.new([1,2,3]) == Set.new([1,2.0,3])
=> false
irb(main):020:0> Set.new([1,2,3]) == Set.new([1,2,3])
=> true
irb(main):021:0> 2 == 2.0
=> true
irb(main):022:0> [2] == [2.0]
=> true
irb(main):023:0> Set.new([1,2,3]) == Set.new([1,3,2])
=> true

Set accepts a range, so the initialize has to copy elements manually unless there is another way.

irb(main):029:0> Set.new(1..3) == Set.new([1,3,2])
=> true

As mentioned in the comments, Ruby already has a very nice Set class in its standard library. But, as also mentioned, there's still code to review here.

So, review-wise, a few things that caught my eye:

@cs. Sorry, but I'm a stickler for descriptive names. I'm guessing this is supposed to be short for "CustomSet", but that's the name of the class, and the variable is just an array. Something like @members might make more sense.
I'd also add a private accessor method for the variable. Call it members, and replace (almost) all instances of @cs with members. By accessing it though a method, you decouple your code better.
You constructor is unnecessarily complex. Just duplicate the input array:

  def initialize(list = [])
    @cs = list.dup
  end

#to_list. "List" isn't a thing in Ruby. Array is a thing though, and the conventional name for this sort of method is to_a. However, you don't want to return your instance variable directly! If you do, it can then be modified externally. It also allows external code to keep references, and all manner of other stuff. So return a dup instead.
#empty is self-modifying. It should either be empty! to indicate its effect, or it should be called clear (to match the so-named method on Array). The method should probably also just call clear on the internal @cs array instead of replacing it.
#member? should perhaps be aliased (or renamed) as #include? (again to match Array)
#each can be shortened to just

def each(&block)
  @cs.each(&block)
end

#put could be written more idiomatically using include?:

def put(item)
  @cs << item unless @cs.include?(item)
end

Or you can push the new item immediately, and just call uniq! afterward. Or you can use a hash (i.e. instance of Hash - not to be confused with the method discussed later), and use its built-in duplicate handling, e.g.:

def put(item)
  @cs[item.hash] = item # assumes @cs is a hash
end

Since Ruby 1.9, hashes are ordered too, so in effect it'd be little different from using an array internally.
#difference, #intersection, and #union can be handled via the internal arrays, e.g.:

def difference(other)
  self.class.new(@cs - other.to_a)
end

def intersection(other)
  self.class.new(@cs & other.to_a)
end

def union(other)
  self.class.new(@cs | other.to_a)
end

And as you can see from the above, those methods should probably be aliased (or renamed) as -, &, and | respectively.
@cs.delete(n) if index(n) - there's no need for the if guard. It just requires an extra lookup to do, and slows things down. The call to delete will work fine on its own.
#== and #eql?. Here you may again use the fact that both self and other have array representations that already support all this:

def ==(other)
  self.class == other.class && @cs == other.to_a
end

I'd include a dup method for cloning a set. E.g.:

def dup
  self.class.new(@cs)
end

I'd include the Enumerable module to get a bunch of methods "for free". All that's required on your class is that it has an #each method - which is does.

Now, about your #hash. Your method is simply flawed, because it doesn't use reduce correctly. Your method is currently equivalent to (:+).hash (or, if the set's empty: :+). You're simply using reduce wrong.

reduce takes an initial value and a block. The block, in turn, receives two arguments: The value carried over from the previous iteration (or the initial value, first time 'round), and the current element in the array. For instance:

sum = [1, 2, 3].reduce(0) do |sum_so_far, item|
  puts "Sum so far: #{sum_so_far}"
  puts "Current value: #{item}"
  sum_so_far += item # this is returned and becomes sum_so_far in the next iteration
end
puts "Final sum: #{sum}"

which'll print

Sum so far: 0
Current value: 1
Sum so far: 1
Current value: 2
Sum so far: 3
Current value: 3
Final sum: 6

As you can hopefully tell, your use of reduce is very different.

What you seem to be trying to do is either

@cs.reduce(0) { |sum, member| sum += member.hash }

or

@cs.map(&:hash).reduce(0, &:+)

which are functionally identical ways of doing the same thing: Sum the hash values of each object in the set.

However, this approach seems flawed to me, since you'd risk conflicts. A set with just one member might have the hash 7 (the member's hash), and a set with two members might also have a hash of 7 (because the members' hashes are 3 and 4). That's no good.

Honestly, I'd consider simply not implementing #hash unless you have very good reason to do so. You have ==/eql? to check for equality, so why bother with hash? Easier to let the default implementation handle that.


Safely convert dollars to cents For my project, I need to convert dollar to cents, so I wrote the following function to safely do the conversion. The input dollar is float and the output should be int.

def safe_dollar_to_cent(dollar):
    parts = str(dollar).split('.')

    msg = 'success'
    status = 0
    if len(parts) == 1:
        return status, int(parts[0])*100, msg

    decimal_part = parts[1]
    if len(decimal_part) > 2:
        decimal_part = decimal_part[0:2]
        msg = 'dollar has been truncated: {} -> {}'.\
              format(parts[1], decimal_part)
        status = 1

    ret = int(parts[0]) * 100
    multiplier = 10
    for i, s in enumerate(decimal_part):
        ret += int(s) * multiplier
        multiplier /= 10

    return status, ret, msg

I am posting here for seeking other some pythonic way of doing this job.

Update:

my input is expected to be float, the return value should be int.
The reason of this implementation is that I found the following incorrect computation.

18.90 * 100 = 1889.9999999999998

I don't like the 3 argument return you're doing. You're using status and msg to actually do the same thing. They both return a signal of success unless the dollar is truncated. You don't need both pieces of information. Personally, I'd just say that you only need to print a note about the dollar being truncated so that the function only returns the cents value.

Now, you're also overlooking a very simple formula to convert this:

cents = dollars * 100

You don't need anything more complicated than that for the basic function:

def safe_dollar_to_cent(dollar):
    cents = dollar * 100
    return cents

If you're not sure that dollar will be a number, you can try converting it:

def safe_dollar_to_cent(dollar):
    cents = float(dollar) * 100
    return cents

As for truncating, I think it's better as an option. Let the user choose whether or not something will be truncated:

def safe_dollar_to_cent(dollar, truncate=True):
    cents = float(dollar) * 100
    if truncate:
        return int(cents)
    else:
        return cents

int will turn cents into a whole number instead of a decimal, so it's effectively truncating everything after the ..


The code looks good to me as well except if you are looking for the ids of the failed logins you would not want to count the ids (SELECT count(id)) but instead just select the id (SELECT id).


Smoothen page transition for mobile devices I have next/previous buttons on my site and all they do is slide sections of the site from left to right and vice versa, it is smooth and looks good on a desktop but on mobile devices it is a bit slow and jumpy.

if (type == "prev") {
    $(this).hide('drop', { direction: 'right' }, 600, function () { $('#dv' + page).show('drop', { direction: 'left' }, 600), $("input:text:visible:not([readonly]):first").focus(); });
} else {
    $(this).hide('drop', { direction: 'left' }, 600, function () { $('#dv' + page).show('drop', { direction: 'right' }, 600), $("input:text:visible:not([readonly]):first").focus(); });
}

Uses the built in jQuery functions to slide the current page out and the next/previous page in based on the current page number, then sets the focus on the first input box.

This was as simple as I could think to do it but maybe there is a better smoother way to do left/right transitions?

It sounds like the issue isn't really the code, just the performance of the hardware: A mobile device just can't redraw everything as fast as a desktop browser. Writing stuff in single lines does not increase performance!

However, you could try the hardware acceleration "hack" shown and explained here.

You should also consider using CSS transforms to do the actual animation, rather than jQuery's position: absolute trickery. All modern browsers (with the exception of Opera Mobile) support it, and such transforms are always hardware accelerated.

Lastly, since this is Code Review, I'd advice you to get rid of the repetition in your code:

var PAGE_SWAP_DURATION = 600; // define this once

function swapPage(outgoingPage, incomingPage, type) {
  var outDirection = type === "prev" ? "right" : "left",
      inDirection  = type === "prev" ? "left" : "right";

  outgoingPage.hide('drop', { direction: outDirection }, PAGE_SWAP_DURATION, function () {
    incomingPage.show('drop', { direction: inDirection }, PAGE_SWAP_DURATION, function () {
      // wait for the animation to finish; this is a complex selector
      // so it may cause the browser to stutter
      incomingPage.find("input:text:visible:not([readonly]):first").focus();
    });
  });
}

Apropos that selector: If possible, it would be better to find the correct input ahead of time, rather than run and re-run such a complex query on each page swap.

For instance, you could map out what inputs belong to what pages when the site first loads, and link them to their respective pages with .data(). Or you could give the inputs IDs or class names, which will make the selector much simpler.


A collection of tasks and their CancellationTokens I have a task wrapper which stores tasks, their CancellationTokens and of course their names (botName). botName is important, because when I want to stop a bot/cancel task, I will have to find it in the collection.

I wonder if there is a better collection to do that, because it looks ugly, especially the CancellationToken cancellation logic.

public class BotWrapper
{
    private readonly static ConcurrentDictionary<string, Tuple<Task, CancellationTokenSource>> _bots = new ConcurrentDictionary<string, Tuple<Task, CancellationTokenSource>>();

    public void Start(string botName, string symbol, KlineInterval interval, StrategyBase strategy)
    {
        CancellationTokenSource cts = new CancellationTokenSource();

        Task task = Task.Run(() => strategy.Start(symbol, interval, cts.Token), cts.Token);

        _bots.TryAdd(botName, new Tuple<Task, CancellationTokenSource>(task, cts));
    }

    public void Stop(string botName)
    {
        foreach (var bot in _bots)
        {
            if (bot.Key.Contains(botName))
            {
                CancellationTokenSource cts = bot.Value.Item2;
                cts.Cancel();

                Thread.Sleep(2500);

                cts.Dispose(); // unmanaged

                _bots.TryRemove(botName, out _);
            }
        }
    }
}

By the way StrategyBase.Start is nothing specific:

public virtual void Start(string symbol, KlineInterval interval, CancellationToken token)
{
    ... logic

    ... and at some point

    if (token.IsCancellationRequested)
    {
        try
        {
            token.ThrowIfCancellationRequested();
        }
        catch (OperationCanceledException)
        {
            SocketClient.UnsubscribeAll();
            Console.WriteLine("Task canceled.");
        }
    }
}
```

Duplicate logic

if (token.IsCancellationRequested)
{
    try
    {
        token.ThrowIfCancellationRequested();
    }
    catch (OperationCanceledException)
    {
        SocketClient.UnsubscribeAll();
        Console.WriteLine("Task canceled.");
    }
}

This code is weird. You first check if a cancellation is requested, then you ask for an exception to be throw if a cancellation is requested (which you already know it is), and then you immediately catch that exception. It seems like you've conflated two ways of doing the same thing. You could instead use either:

try
{
    token.ThrowIfCancellationRequested();
}
catch (OperationCanceledException)
{
    SocketClient.UnsubscribeAll();
    Console.WriteLine("Task canceled.");
}

OR

if (token.IsCancellationRequested)
{
    SocketClient.UnsubscribeAll();
    Console.WriteLine("Task canceled.");
}

Which approach you use depends on whether it's meaningful to throw an exception, and it doesn't seem meaningful here (though you have actually omitted the surrounding code so I can't give you a guarantee).

Throwing an exception is an expensive operation, and its main purpose is to bubble an exception up through multiple layers without requiring manual handling at every step. Since you're throwing and catching at the same layer, the cost of the expensive throw is not worth the benefit.

Based on what I see, a simple if suffices here.

Tuples

I don't like tuples (in this particular scenario, at least). They're a honeytrap.

At first sight, they look really easy by allowing you to skip creating a custom class, but then you are stuck using unreadable Item1, Item2, ... properties which have no meaning. In any reasonably sized codebase, I'd have to look up which item is stored in which slot several times during the development cycle.

I suggest replacing this with a custom class just so you can keep the property names relevant:

public class BotTask
{
    public Task Task { get; private set; }
    public CancellationTokenSource CancellationTokenSource { get; private set; }

    public BotTask (Task task, CancellationTokenSource cancellationTokenSource)
    {
        this.Task = task;
        this.CancellationTokenSource = cancellationTokenSource;
    }
}

It's a few lines extra, but it significantly improves readability in the code that uses these objects, e.g.:

// Previously

CancellationTokenSource cts = bot.Value.Item2;
cts.Cancel();

// Now

bot.Value.CancellationTokenSource.Cancel();

The increased readability and somewhat shortened line count in the usage generally outweighs the effort of writing a simple DTO class.

Contains

It seems to me that you're conflating the Dictionary.ContainsKey and String.Contains methods:

public void Stop(string botName)
{
    foreach (var bot in _bots)
    {
        if (bot.Key.Contains(botName))
        {
            // ...
        }
    }
}

If you have a list of botTasks with names Alice, Bob, Cindy and you ask to stop the both with name ind, then you should come up dry since no bot with that exact name exists. However, in your current logic, that is not the case, since "Cindy".Contains("ind") returns true!

What you presumably want is to check if the dictionary contains the key "ind" (exactly), instead of whether the dictionary contains any key which contains "ind" in part of the key. That can be achieved like so:

public void Stop(string botName)
{
    if(_bots.ContainsKey(botName))
    {
        var bot = _bots[botName];

        // ...
    }
}

Or alternatively:

public void Stop(string botName)
{
    if(_bots.TryGetValue(botName, out bot))
    {
        // ...
    }
}

Thread.Sleep

Thread.Sleep(2500);

Since you're using async in your code, it seems counterproductive to then use Thread.Sleep instead of the asynchronous ´Task.Delay´, i.e.:

await Task.Delay(2500);

This is a tiny aside, because I'll omit this delay in the next section, but it's still important to point out in case a delay was actually needed here.

Manual disposing

I think you can skip the "sleep and dispose" logic here.

It's good to see that you are managing your resources responsibly, but in this particular case the management is causing an undesirable blocking of the thread for 2.5 seconds. That's not good. Even if you use an await Task.Delay instead, I'm still not convinced that it's necessary to manually handle this.

When nothing references the cancellation token source anymore, the garbage collector will eventually pick this up and dispose of the object itself. There are cases where you don't have the luxury of waiting on the garbage collector to kick into action, but in such cases blocking the thread for 2.5 seconds would still be a bigger evil.

You can simplify the logic:

public void Stop(string botName)
{
    if(_bots.ContainsKey(botName))
    {
        var bot = _bots[botName];

        bot.Value.CancellationTokenSource.Cancel();

        _bots.TryRemove(botName, out _);
    }
}

Instead of waiting for the task to cancel itself, your code instead entrusts that the task will eventually end itself, and your code only focuses on getting its own affairs in order, i.e. removing the task from your dictionary.

Static ConcurrentDictionary

This is more of an educated guess than a guarantee.

Your static ConcurrentDictionary comes across as a red flag to me, where you're misusing statics.

It seems to me like you're intending to spawn multiple BotWrapper instances which you expect to share the same static botlist, but that's a flawed approach. It would be better if you instead pass the same BotWrapper instance around in your codebase (whether through DI containers or manually, I don't know how your architecture is set up), and then keep the botlist as a non-static property of that instance.

The benefits of managing a single instance are a minor memory improvement (since you don't need multiple instances anymore), and it allows you to scale out your codebase in case you ever need to manage two different lists of bottasks. Using statics, it's impossible to have more than one. While it may not be necessary right now to maintain different lists, never say never, and it's good practice to allow for maximum reusability down the line.

However, you didn't actually provide enough code for me to make a final decision on this. This is just an educated guess based on the code as it is presented.


Searching for an element in a 2D array For example, given a 2D array (a matrix):

{{1, 2, 3, 4},
{5, 6, 7, 8},
{9, 10, 11, 12}}

What are the solutions to find a number, such as 6?

Here's the code I tried using binary search method:

int main(){
    int sample[3][4] = {{1, 2, 3, 4}, {5, 6, 7, 8}, {9, 10, 11, 12}};
    int target = 6;
    int lowX, highX, lowY, highY, midX, midY;

    //Note: some values are hardcoded, should not matter.
    lowX = 0;
    highX = 2;
    lowY = 0;
    highY = 3;

    if(target <= sample[0][0] || target >= sample[3 - 1][4 - 1]){
        std::cout<<"Nope";
    }

    while(lowX <= highX && lowY <= highY){
        midX = (lowX + highX)/2;
        midY = (lowY + highY)/2;

        if(target < sample[midX][midY]){
            highX = midX - 1;
            highY = midY - 1;
        }
        else if(target > sample[midX][midY]){
            lowX = highX + 1;
            lowY = highY + 1;
        }
        else {
            lowX = highX + 1;
            lowY = highY + 1;
        }
    }

    if(target == sample[midX][midY]){
        std::cout<<"Found";
    }
    else{
        std::cout<<"Nope";
    }

    return 0;
}

Is this method optimal?
What are the other possible solutions?

Have you actually tested your code (with numbers other than 6)? Because I doubt that can work.

Firstly you have lowX = highX + 1; where it most likely should be lowX = midX + 1; (dito with Y).

And more importantly, your code only adds or subtracts one from X and Y at the same time. There is no case where X and Y are changed independently from each other, thus making it impossible to navigate the whole matrix.

I would suggest two alternative solutions:

A) Run the binary search recursivly. First search the outer array for the inner element (array) that includes the sought number, then search that array.

B) Logically map the two dimensional array to a simple array and search that, e.g. an index i on a simple array maps to the indexes x = i/4 and y = x%4 of the matrix.

Questions: What is the use case for this? If the arrays are sorted like this, why use a matrix instead of a simple array? What is the logic behind using sub-arrays like this? Do you need the location of an element, or just if it's present or not?


Finding Mersenne primes An attempt to find Mersenne primes using the Lucas-Lehmer primality test:
n=int(input("Enter n to check if 2^n-1 is a prime:"))
a=4
for i in range(0,n-2):
    b=a**2
    a=b-2
    if i==n-3:
        c=(a)%((2**n) - 1)
        if c==0:
            print((2**n) - 1 ,"is a prime number")
            break
if c!=0:
    print((2**n) - 1 ,"is not a prime number")

Can you critique this code? It is apparent that it needs improvement as it doesnt work for n<=2 and computing power increases exponentially for increasing n.
I have been learning python for less than a year, I am only 15 and would love any feedback on this program.
Edit: The confusion with n-2 and n-3 is because we have to check if the (n-1)th term in the series is divisible by l = 2^n - 1 to establish that l is a Mersenne prime. But as you would have noticed the above written code considers the 0th term as 14 which is the 3rd term in the actual series. This means there is a gap of two terms between the actual series and the series procured by the program, therefore to actually study the (n-1)th term in the series , we have to consider the (n-3)th term here. Hence the mess that doesn't allow n to be <=2.
Please provide your opinion on this

The page you linked to has pseudocode, and it does s = ((s × s) − 2) mod M. And right below the pseudocode it says:

Performing the mod M at each iteration ensures that all intermediate results are at most p bits (otherwise the number of bits would double each iteration).

Do that and it'll be much faster.
And better use the same variable names as in the page you referenced, don't make up new ones, that just obfuscates the connection.


Using Regexp to select the date from a string I have a string with date details with time, but I would like to show the day information alone.  For that, I use the regexp to select the date.  Can anyone tell me the regexp, and if what is used is the correct one?

Here is my string:

2014-09-12T00:00:00

Here is the regexp, what is used:

\d.+\-\d{1,2}

I am getting the result as:

2014-09-12

I am getting correct result, but I am wondering if there may be a better approach.

The regular expression you used, although it works, it's not very specific. It matches the sequence of these:

\d = a digit
.+ = one or more of any character
\- = a literal - (btw you didn't need the \ here, it has no effect here)
\d{1,2} = one or two digits

Many non-dates can match this pattern too, for example these strings:

1abcdefg-4
2999999!@$$#$@#$-12
...

I would recommend to use a more strict pattern, for example:

\d{4}-\d{2}-\d{2}

This matches the sequence of these:

exactly 4 digits
literal -
exactly 2 digits
literal -
exactly 2 digits

Note that this is still not perfect. It won't prevent these invalid dates:

2014-02-31
9999-99-99

A bit more strict pattern would be:

2\d{3}-[01]\d-[0-3]\d

Which, of course, is still not perfect, because it will still allow 2014-19-39, for example. It's possible to be even more strict, but whether it's worth it / necessary or not depends on your use case.


Sql Server: Getting the highest value based on two uniqueidentifiers - possible to avoid union? I have two riscProfileTypeIds (both of type uniqueidentifier) of which I need the one with the highest sequence number
(the highest of the two risc profiles, at business level).
The below query does the job, but I'm not very happy with the union, I wonder if there's a way to do the same without it.

SELECT [ID]
FROM [RiscProfileType]
WHERE 1 = 1
AND [Active] = 1
AND [Sequence] = (
    SELECT MAX([Sequence])
    FROM (
        SELECT [Sequence]
        FROM [RiscProfileType]
        WHERE 1 = 1
        AND [Active] = 1
        AND [ID] = @riscProfileTypeId1
        UNION
        SELECT [Sequence]
        FROM [RiscProfileType]
        WHERE 1 = 1
        AND [Active] = 1
        AND [ID] = @riscProfileTypeId2
    ) src
)

The [RiscProfileType] table is very small (4 records), so performance isn't much of an issue but still I prefer to write the best query possible.

Put simply it contains:

[Risc]          [Sequence]
Unacceptable    4
High            3
Normal          2
Low             1

I don't think a join is possible, because there's no real link between the entities.

Any suggestions on improving this query, or is this fine?

Why don't you use IN instead of the JOIN?

SELECT MAX([Sequence])
FROM [RiscProfileType]
WHERE 1 = 1
   AND [Active] = 1
   AND [ID] IN (@riscProfileTypeId1, @riscProfileTypeId2)

Or rethink the logic:

SELECT [ID]
FROM [RiscProfileType]
WHERE 1 = 1
   AND [Active] = 1
   AND [ID] IN (@riscProfileTypeId1, @riscProfileTypeId2)
ORDER BY [Sequence] DESC
LIMIT 1


nagios plugin webdav check I have written a small plugin for nagios to check a WebDAV server. It is supposed to connect using a username and password and upload & delete a test file

This is my code:

#!/bin/bash
#Exit codes
STATE_OK=0
STATE_WARNING=1
STATE_CRITICAL=2
STATE_UNKNOWN=3

FILENAME=/tmp/test_monitor
FILE=test_monitor
HOSTNAME=$1
USERNAME=$2
PASSWORD=$3
WEBDAVDIR=$4

# Verify the type of input and number of values
# Display an error message if the (input) is not correct
# Exit the shell script with a status of 1 using exit 1 command.
[ $# -ne 4 ] && { echo "Usage: $0 <hostname> <username> <password> <webdav_dir>"; exit 1; }

# create temp file
truncate -s 1M /tmp/test_monitor

# connect to server and upload file
curl --fail -T $FILENAME -u $USERNAME:$PASSWORD http://$HOSTNAME/$WEBDAVDIR/

status=$?
case $status in
    0)
        echo "OK - File uploaded on WebDAV Server: $HOSTNAME"
        # remove test file
        curl --fail -X DELETE -u $USERNAME:$PASSWORD http://$HOSTNAME/$WEBDAVDIR/$FILE
        exit $STATE_OK
        ;;
    1)
        echo "CRITICAL - Cannot upload file on $HOSTNAME"
        exit $STATE_CRITICAL
        ;;
    6)
        echo "CRITICAL - Could not resolve host $HOSTNAME"
        exit $STATE_CRITICAL
        ;;
    26)
        echo "CRITICAL - Cannot open $FILENAME for upload"
        exit $STATE_CRITICAL
        ;;
    22)
        echo "CRITICAL - The requested URL returned error"
        exit $STATE_CRITICAL
        ;;
    127)
        echo "CRITICAL - Command not found"
        exit $STATE_CRITICAL
        ;;
    *)
        echo "UNKNOWN - "
        exit $STATE_UNKNOWN
esac

Can you suggest any improvements?

Overall, the code is clean and easy to follow.  I appreciate the consistent indentation.  I would not be upset about running your code in production.  Here are some suggestions that would make it even better:

use your own variables

With the lines

# create temp file
truncate -s 1M /tmp/test_monitor

we get a temporary file, but since we name it explicitly it isn't clear that it gets used by the curl right after.  It would be easier to follow if you did:

truncate -s 1M $FILENAME

If you want to be really careful, check the exit value of truncate or see if the file is the right size (which also implies that it exists).

check before assigning

You assign your command line arguments before checking if they are even there.  I prefer to check before using so I would like to see:

# Verify the type of input and number of values
# Display an error message if the (input) is not correct
# Exit the shell script with a status of 1 using exit 1 command.
[ $# -ne 4 ] && { echo "Usage: $0 <hostname> <username> <password> <webdav_dir>"; exit 1; }

HOSTNAME=$1
USERNAME=$2
PASSWORD=$3
WEBDAVDIR=$4

for usage exit UNKNOWN rather than WARNING

If somebody runs your plugin within nagios without enough command line arguments your script will print the helpful usage and then exit 1 which we know nagios will interpret as a warning.  That's something that could get missed in a big nagios install for a while so I'd prefer to exit $STATE_UNKNOWN so that it will be obvious in nagios that something is broken with the config.

duplicate variables

From your script:

FILENAME=/tmp/test_monitor
FILE=test_monitor

I understand that you need this in different places, one without the path, and otherwise with the path.  But we can reduce duplication by doing something like

FILE=test_monitor
FILENAME=/tmp/$FILE

I would go one step further and rename the variables as

FILENAME=test_monitor
FQFN=/tmp/$FILE

now FILENAME contains the name of the file without the path and FQFN is he "Fully Qualified File Name" and is the one you will mostly use.

tips for nagios plugin writers

For writing a nagios plugin in any language I would encourage folks to:

include a sampling of command config entries
maintain your code on github so you get a free issue tracker and such
register with the icinga registry since the nagios one has been broken lately

btw

The locals don't like it if you revise your code in the same post.  So if you want another review, please start another thread.  Dropping a link to the new one in the comments here is also nice.  :-)


Json config loader class Currently the class takes a json file and retrieves data from it, and i'd like to improve this class.
ConfigLoader.h
#ifndef LEVELEDITOR_CONFIGLOADER_H
#define LEVELEDITOR_CONFIGLOADER_H

#include <string>
#include <json/json.h>
#include <stdexcept>
#include <fstream>

namespace Editor
{
    class JsonInit
    {
    public:
        JsonInit()
        {
            std::ifstream inputFile("config.json");
            if (inputFile.bad())
            {
                throw std::runtime_error("could not load config file");
            }

            inputFile >> _root;
        }

        inline Json::Value& getRoot() { return _root; }

    private:
        Json::Value _root;
    };

    class ConfigLoader
    {
    public:
        inline static int loadInt(const std::string key) {return json.getRoot().get(key, -1).asInt(); }
        inline static std::string& loadString(const std::string key) {return json.getRoot().get(key, -1).asString();}

    private:

        static JsonInit json;
    };
}

#endif // LEVELEDITOR_CONFIGLOADER_H

ConfigLoader.cpp
#include "configloader.h"

Editor::JsonInit Editor::ConfigLoader::json;

Edit: forgot to say the "config.json" path is temporary, as i wish to make it possible to let the user choose

It appears that the file ConfigLoader.cpp is unnecessary unless you are just providing an example of usage.
The keyword inline is obsolete, it is really only a suggestion these days. Modern C++ compilers will make the decision on whether the code should be inlined or not when using the -O3 switch.
If you are going to let the user define the config file name, then pass the name into the constructor and default it at a higher level.


Python program to get all non unique elements in an array Given an array of ints as input:

non_unique([1, 2, 3, 1, 3]) returns [1, 3, 1, 3]
non_unique([1, 2, 3, 4, 5]) returns []
non_unique([5, 5, 5, 5, 5]) returns [5,5,5,5,5]
non_unique([10, 9, 10, 10, 9, 8]) returns [10, 9, 10, 10, 9]

The code itself:

def checkio(originalList: list) -> list:

    checked_list = []
    result_list = []

    for i, number in enumerate(originalList):
        if number in checked_list:
            result_list.append(number)
        else:
            if (len(originalList) - 1) - i > 0:
                if number in originalList[i+1:]:
                    result_list.append(number)
        checked_list.append(number)

    return result_list

How can be the code improved?

One thing to keep in mind is your variable checked_list, which serves almost no purpose in the function, as you could do without the else block.

The simplest thing I can think of would be just to shorten the function down to one list comprehension, instead of appending to two lists.

Revised code:

def checkio(original_list: list) -> list:
    return [x for x in original_list if original_list.count(x) > 1]

When running this function, these are the outputs:

checkio([1, 2, 3, 1, 3])  # Returns [1, 3, 1, 3]
checkio([1, 2, 3, 4, 5])  # Returns []
checkio([5, 5, 5, 5, 5])  # Returns [5,5,5,5,5]
checkio([10, 9, 10, 10, 9, 8])  # Returns [10, 9, 10, 10, 9]


Loading content into a div A while ago I used this code to load content into a div using jQuery load. I repeated the code for all the clicks to load different pages in the same div.

Is there any other way to do this?

$("#button1").click(function(){
    $('#div1').load('page1.php');
});

$("#button2").click(function(){
    $('#div1').load('page2.php');
});

<div id="div1">      </div>

Option 1:

If your buttons and pages are actually numbered like that, you can simply extract the number from the ID:

var $container = $('#div1');

$('[id^=button]').click(function() {
    $container.load('page' + this.id.match(/\d+$/)[0] + '.php');
});

Option 2:

If that was just an example, but in reality your naming scheme is not so predictable, you can keep an object map with all the appropriate URLs:

var $container = $('#div1'),
    map = {
        button1: 'page1',
        button2: 'page2'
        // and so on...
    };

$('[id^=button]').click(function() {
    $container.load( map[ this.id ] );
});

Option 3:

You could store the page URL in the HTML:

and then use that in your JavaScript:

var $container = $('#div1');

$('[id^=button]').click(function() {
    $container.load( $(this).attr('data-page') );
});

I personally dislike this method, since it puts behavioral stuff in your HTML.


Count of category occurances (multiple cateogries can fall under each row) I want to get a count of each of the category occurrences (in how many rows did it show up in under the taxonomy column). The taxonomy column has a list of categories and I have several thousand rows, and I want to get a count of the number of times the categories shows up. Please note that there could be multiple categories per row but the same category will not be repeated in the same cell.

Is there a way to condense or clean the following code when running through a dataframe? I want to combine the if statements somehow without losing the logic.

for index, row in df.iterrows():
    for key in (row['taxonomy'] ) :

        if key in technical_issues_list:
            technical_issues += 1

        if key  ==    'technical_issues' :
            technical_issues_specific += 1

        if key == 'product_not_available':
            product_not_available += 1

        if key == 'site_performance':
            site_performance += 1

        if key in checkout_list:
            checkout += 1

        if key == 'unable_to_checkout':
            unable_to_checkout += 1

        if key == 'unable_to_add_or_remove_products':
            unable_to_add_or_remove_products += 1

        if key == 'promotions':
            promotions += 1

        if key == 'payment_options':
            payment_options += 1

        if key == 'paypal':
            paypal += 1

        if key == 'shipping_cost':
            shipping_cost += 1

        if key == 'order_total_wrong':
            order_total_wrong += 1

        if key == 'backorder':
            backorder += 1

        if key == 'address_not_fitting_or_incorrect':
            address_not_fitting_or_incorrect +=1

        if key in fulfillment_list:
            fulfillment += 1

        if key == 'where_is_my_order':
            where_is_my_order += 1

        if key == 'BOPIS':
            BOPIS += 1

        if key == 'delivery_issues':
            delivery_issues += 1

        if key == 'no_communication_or_delays':
            no_communication_or_delays += 1

        if key == 'damaged_product':
            damaged_product += 1

        if key == 'unassembled_assembly_issues':
            unassembled_assembly_issues += 1

        if 'customer service' == key:
            customer_service += 1
        if 'long-wait_times' == key:
            long_wait_times +=1
        if 'contact_issues' == key:
            contact_issues +=1
        if 'canada_related'  == key:
            canada_related +=1
        if 'product + info' == key:
            product_info += 1
        if 'design issues' == key:
            design_issues += 1
        if 'gift_wrapping' in key:
            gift_wrapping +=1
        if  'stores' == key:
            store += 1
        if  'email' == key:
            email += 1
        if  'suggestions' == key:
            suggestions +=1
        if 'rewards' == key:
            rewards +=1
        if  'registry' == key:
            registry += 1
        if  'returns_cancellations' in key:
            returns_cancellations +=1

All I did was convert all if statements with exact values into a dictionary. This way will allow you to increment the values of each key without as much if/else usage. If you then want to later on pass the values of each key to a variable of your choice, you can just loop through the dictionary once, which is less time consuming and it also makes debugging easier.

I didn't touch the if statements using in. You can try placing it in the dict too.

all_dict = {'customer service':0,
    'long-wait_times':0,
    'contact_issues':0,
    'canada_related':0,
    'product + info':0,
    'design issues':0,
    'gift_wrapping':0,
    'stores':0,
    'email' :0,
    'suggestions':0,
    'rewards':0,
    'registry' :0,
    'returns_cancellations':0
    'technical_issues':0,
    'product_not_available':0,
    'site_performance':0,
    'unable_to_checkout':0,
    'unable_to_add_or_remove_products':0,
    'promotions':0,
    'payment_options':0,
    'paypal':0,
    'shipping_cost':0,
    'order_total_wrong':0,
    'backorder':0,
    'address_not_fitting_or_incorrect':0,
    'where_is_my_order':0,
    'BOPIS':0,
    'delivery_issues':0,
   'no_communication_or_delays':0,
   'damaged_product':0,
  'unassembled_assembly_issues':0
  }

for index, row in df.iterrows():
    for key in (row['taxonomy'] ) :

        if key in technical_issues_list:
            technical_issues += 1

        elif key in checkout_list:
            checkout += 1

        elif key in fulfillment_list:
            fulfillment += 1

        elif  'returns_cancellations' in key:
            returns_cancellations +=1

        else:
            all_dict[key] += 1


Calculating percentages in arbitrary number of columns Given this sample input:

ID     Sample1     Sample2      Sample3
One      10          0            5
Two      3           6            8
Three    3           4            7

I needed to produce this output using GNU AWK (awk in Linux, gawk in BSD):

ID    Sample1 Sample2 Sample3
One   62.50   0.00    25.00
Two   18.75   60.00   40.00
Three 18.75   40.00   35.00

This is how I solved it:

function percent(value, total) {
    return sprintf("%.2f", 100 * value / total)
}
{
    label[NR] = $1
    for (i = 2; i <= NF; ++i) {
        sum[i] += col[i][NR] = $i
    }
}
END {
    title = label[1]
    for (i = 2; i <= length(col) + 1; ++i) {
        title = title "\t" col[i][1]
    }
    print title
    for (j = 2; j <= NR; ++j) {
        line = label[j]
        for (i = 2; i <= length(col) + 1; ++i) {
            line = line "\t" percent(col[i][j], sum[i])
        }
        print line
    }
}

What do you think about this implementation? How would you improve it?

It's quite good. I can only slightly simplify it:

NR==1 {print; next}
{
    label[NR] = $1
    for (i=2; i<=NF; i++) { sum[i] += $i; val[NR,i] = $i }
}
END {
    OFS = "\t"
    for (nr=2; nr<=NR; nr++) {
        $1 = label[nr]
        for (i=2; i<=NF; i++) $i = sprintf("%.2f", 100*val[nr,i]/sum[i])
        print
    }
}

don't need to store the header line, just print it and move on
I find sum[i] += col[i][NR] = $i needlessly complicated
take advantage of OFS and awk can build the line itself


Searching a Word Document from Excel - v2.0 My first question:

Searching a Word Document from Excel

Since then I have implemented most of the suggestions by @RubberDuck and @Comintern. I have also added functionality for different-sized spreadsheets (horizontal and vertical), read multiple files in during a single run of the macro, and a bunch of constants.

This is what I currently have:

Global lineCount As Integer

Sub LoadWordFromExcel()

    Const ONE As Integer = 1
    Const FIRST_ROW As Integer = 2
    Const PATH_TO_FOLDER As String = "C:\Users\<user>\Desktop\Macro folder\"
    Const LAST_NAME As Integer = 3
    Const CHOP_LEFT As Integer = 21
    Const CHOP_RIGHT As Integer = 11

    Dim r As Integer
    Dim k As Integer
    Dim c As Integer
    Dim filePath As String
    Dim file As String
    Dim lastRow As Integer
    Dim lastColumn As Integer
    Dim sheet As Worksheet
    Set sheet = ActiveSheet

    'Finds last row and column with valid data
    With ActiveSheet
        lastRow = .Range("C" & .Rows.Count).End(xlUp).Row
        lastColumn = ActiveSheet.Cells(ONE, Columns.Count).End(xlToLeft).Column
    End With

    For c = ONE To lastColumn
        file = Dir$(PATH_TO_FOLDER & sheet.Cells(ONE, c).Value & ".docx")
        If (Len(file) > 0) Then
            filePath = PATH_TO_FOLDER & file
            Dim entries As Scripting.Dictionary
            Set entries = LoadWordToDictionary(filePath)
            For r = FIRST_ROW To lastRow
                For k = 1 To lineCount
                    If InStr(entries(k), sheet.Cells(r, LAST_NAME).Value) <> 0 Then
                        sheet.Cells(r, c).Value = Left(Right(entries(k), CHOP_LEFT), CHOP_RIGHT)
                        Exit For
                    Else
                        sheet.Cells(r, c).Value = ""
                    End If
                Next
            Next
        End If
    Next
    MsgBox ("I'm Finished")
End Sub

'Loads data from word document and adds it to a dictionary
Private Function LoadWordToDictionary(file As String) As Scripting.Dictionary

    Dim host As New Word.Application
    Dim doc As Word.Document
    Dim lines() As String

    'loads data from Word doc into lines'
    Set doc = host.Documents.Open(file)
    lines = Split(doc.Content.Text, vbCr)
    doc.Close
    host.Quit

    Dim output As New Scripting.Dictionary
    Dim items() As String
    Dim i As Integer

    'Loads data from lines(array) to output(scripting.dictionary)
    For i = 0 To UBound(lines)
        Call output.Add(i + 1, lines(i))
    Next i

    lineCount = UBound(lines)
    Set LoadWordToDictionary = output

End Function

Questions:

Am I still unwittingly using Hungarian Notation? I read the article but it brought up more questions than answers.
Is there anything that I'm overthinking and could simplify? I'm looking at my nested for loop and can't help but think there's another way to do it.

So far, this script is FAST. It can enter over 5,000 cells of information in under 30s instead of the 5-10 minutes for 300 cells that the first version did.

Answers

Question 1: No, this is now Hungarian-free™. The article that was referred to in your prior post differentiated between "Apps Hungarian" and "Systems Hungarian", and your previous code was using the latter (i.e. strName). You were basically indicating that yourFunction SearchWordDoc(strPath, strName)was expecting String's as parameters. The current code is both more explicit and will in fact not even compile if you try to pass it anything but a String. This is a Good Thing™.

Question 2: This one is harder to answer without some information about the layout of the spreadsheet. From the code, I would gather that row 1 consists of column headings that correspond to your Word document names and that column "C" is the last names that you are looking up. If this is the case, your first loop (the "c" loop) counter is off - instead of starting at 1, it should start at the first column that is expected to have a value. I'm not sure exactly what the inner-most loop (the "k" loop) is doing - it seems to rely on the Word document being in the same sort order as the rows in the spreadsheet.  The Dictionary key is an integer, and you're using the row as the key when you index it instead of the last name. Take a another look at my answer to your last post, you should be parsing a name out of the Word file for the Dictionary keys.

Other Observations

Finding Worksheet Size: The following code will likely return inconsistent results in many cases (at least it does for me):

'Finds last row and column with valid data
With ActiveSheet
    lastRow = .Range("C" & .Rows.Count).End(xlUp).Row
    lastColumn = ActiveSheet.Cells(ONE, Columns.Count).End(xlToLeft).Column
End With

Excel's Worksheet object has a property that is much easier to use and much more reliable and readable in your code:

With sheet.UsedRange
    lastColumn = .Columns.Count
    lastRow = .Rows.Count
End With

Using ActiveSheet: Don't do it. After you get your reference to sheet at the start of Sub LoadWordFromExcel(), don't use ActiveSheet again. If you need a reference in another function, pass it as an argument. Within the same function, always use your captured reference. To expand on my answer to your previous question, every time you call ActiveSheet, you get a reference to whatever Worksheet Excel has active.  If you are half way through your routine and the user opens another workbook, your reference can change.  This can (and will) result in read garbage cells and writing into the wrong spreadsheet.

Global Variables (other than constants): Avoid them in any way that you can, as they can lead to unexpected and hard to locate bugs. Variables should always have the minimum required scope.  In your case, you are using lineCount to track how many items you add to the Dictionary object you return, but there isn't a reason to do this.  You can just get a count directly from the object:

'instead of 1 To lineCount ...
For k = 1 To entries.Count
    ...
Next

Constants: Definately well intentioned, but there are a couple of things to mention here.  First, these should typically be placed in your module's header (after the missing Option Explicit). The primary reason to do so is so that they get a wider scope. Remember, the goal of having a constant is not just to have an easily identifiable name - it make changes a lot easier. For example, if your layout changes, you can change Const LAST_NAME As Integer = 3 to whatever value you need exactly once instead of searching all over for the number 3 and trying to figure out if it is being used as a column index. That said, Const ONE As Integer = 1 is never going to change. Use 1 instead. The other goal, easily identifiable names, is the other place I would make a couple minor changes to make them more explicit and readable.  I'd likely do something like this:

Option Explicit

Private Const FIRST_DATA_ROW As Integer = 2
Private Const WORD_DOCUMENT_PATH As String = "C:\Users\<user>\Desktop\Macro folder\"
Private Const LAST_NAME_COL As Integer = 3
Private Const PARSE_START_INDEX As Integer = 21
Private Const PARSE_LENGTH As Integer = 11

Variable Names: While you can occasionally use one letter loop counters like i when it is obvious that you are indexing something, in this case it would be better to be more descriptive in what you are looping through and how you are using the loop counters to index.  This is much more important in nested loops than in a simple array traversal.  I'd suggest something more like this:

Dim row As Integer
Dim key As Integer
Dim col As Integer

For col = 1 To lastColumn
    ...
    For row = FIRST_DATA_ROW To lastRow
        For key = 1 To entries.Count
            ...
        Next key
    Next row
Next col

I'm also a fan of the Next [Variable] syntax for readability, especially with nested loops.

Odds and Ends: The following If test will never return False:

file = Dir$(PATH_TO_FOLDER & sheet.Cells(ONE, c).Value & ".docx")
If (Len(file) > 0) Then

The length of the file variable will always be at least 5 + Len(PATH_TO_FOLDER) - it can never be zero. You should also make a habit of using Trim$() before you test a string length (unless you care about white-space).

Also, if you are going to return a the value of Left() or Right() into a String, use the string specific functions Left$() and Right$() to prevent casting back and forth from a Variant. Better than Left$(Right$(var, start), length) would be to use the Mid$() function.

This test is wildly inefficient:

If InStr(entries(k), sheet.Cells(r, LAST_NAME).Value) <> 0 Then

It takes the returned Item from the Dictionary and then uses a string comparision to see if it matches the cell contents. The Dictionary object has a method to determine if a given key exists - .Exists(). Restructuring this one If statement can likely eliminate the inner-most loop entirely (assuming you key by name).

Dim key As String
...
key = Trim$(sheet.Cells(row, LAST_NAME).Value)
If entries.Exists(key) Then
    sheet.Cells(row, col).Value = Mid$(entries(key), PARSE_START_INDEX, PARSE_LENGTH)
Else
    sheet.Cells(row, col).Value = vbNullString
End If

Finally (and this is more of a pet-peeve of mine), use the built-in constants when they exist to improve readability. "" is much harder to comprehend at a glance than vbNullString in much the same way that vbCr is easier to read than Chr$(13).


Count number of ways to construct binary search tree with n elements I had this question asked in an interview:

  In how many ways, we can construct binary search tree from \$n\$ elements?

I have written this code, but got a feedback that it could be improved. How to do it?

def count_ways(n):
    c = [0] * (n+1)
    c[0] = 1
    c[1] = 1
    for i in xrange(2, n+1):
        sum_w = 0
        for j in xrange(0, i):
            sum_w += c[j] * c[i-j-1]
        c[i] = sum_w
    return c[n]

print count_ways(4)

The number of trees can be expressed in the closed form \$\frac{\binom{2n}{n}}{n+1}\$, and due to \$\binom{2n}{n} = \frac{4n - 6}{n} \binom{2(n-1)}{n-1}\$ the result is computed in linear time.

I would not ask such question in face-to-face interview (unless the position requires in-depth knowledge of combinatoric and graph theory).


Picking a random point along the edge of an area I'm writing a Diffusion-limited aggregation simulation, and need to pick a random point along the edge of an area defined by a dimension pair of [width, height]. The point shouldn't be deep within the area; it should always be on the border of one of the sides. This is the intended behavior:

(let [rand-gen (Random.)
      dims [21 11]]

  (dotimes [_ 25]
    ; I'm casting to int so the output is legible
    ;  I actually need doubles
    (println
      (mapv int (random-edge-coord dims rand-gen)))))

[13 0]
[0 8]
[15 10]
[8 10]
[20 0]
[0 6]
[20 2]
[0 5]
[7 10]
[0 7]
[0 10]
[20 1]
[6 0]
[20 0]
[0 2]
[20 10]
[15 0]
[14 0]
[7 0]
[0 7]
[12 0]
[20 7]
[20 4]
[20 8]
[20 4]

This is the mess I came up with:

; These two functions are just for the sake of a MCVE
(defn- random-boolean [^Random rand-gen]
  (.nextBoolean rand-gen))

(defn- random-double [min, max, ^Random rand-gen]
  (let [r (.nextDouble rand-gen)
        spread (- max min)
        rand (* spread r)]
    (+ rand min)))

(defn random-edge-coord [dimensions, ^Random rand-gen]
  (let [[w h] dimensions
        vertical? (random-boolean rand-gen)
        start? (random-boolean rand-gen)

        x (if vertical?
            ; Pick either the left or right edge
            (if start?
              0
              (dec w))

            ; Else, pick a random point along the top/bottom egde
            (random-double 0 w rand-gen))

        y (if vertical?
            (random-double 0 h rand-gen)

            (if start?
              0
              (dec h)))]

    [x y]))

The redundancy is killing me though. I can't think of how to clean it up. I feel like this should be fairly straightforward to generalize, but I'm stuck. I figured I could clean it up introducing some anonymous functions:

(defn random-edge-coord [dimensions rand-gen]
  (let [[w h] dimensions
        vertical? (g/random-boolean rand-gen)
        start? (g/random-boolean rand-gen)

        edge #(if start? 0 (dec %))
        rand-edge #(g/random-double 0 % rand-gen)

        x (if vertical?
            (edge w)
            (rand-edge w))

        y (if vertical?
            (rand-edge h)
            (edge h))]

    [x y]))

But that's not a huge gain.

Any input here would be appreciated.

Easy-peasy.  Just imagine walking around the rectangle starting at the origin in a counter-clockwise direction, and keeping track of the length traversed:

(defn random-edge-coord
  [width height]
  (let [len-1 width
        len-2 (+ width height)
        len-3 (+ (* 2 width) height)
        len-4 (* 2 (+ width height))
        pos   (rand len-4)]
    (cond
      (< pos len-1) [pos 0]
      (< pos len-2) [width (- pos len-1)]
      (< pos len-3) [(- len-3 pos) height]
      :else [0 (- len-4 pos)])))

(defn print-1 [[x y]]
  (printf "[%3.1f %3.1f] \n" (double x) (double y)) )

(dotest
  (nl)
  (dotimes [i 10]
    (print-1 (random-edge-coord 2 3))))

with result:

[1.4 3.0]
[0.0 0.2]
[2.0 2.1]
[1.2 0.0]
[2.0 1.4]
[1.7 0.0]
[1.5 3.0]
[1.5 3.0]
[2.0 0.7]
[1.0 3.0]

and

[1.0 0.0]
[1.4 0.0]
[1.4 3.0]
[0.5 0.0]
[0.0 2.6]
[2.0 2.7]
[0.0 1.5]
[2.0 2.8]
[0.9 3.0]
[1.1 3.0]

For a more general shape, you could calculate the length breakpoints in a loop and use a loop to search them in order instead of a cond.  You could also use generalized line-segments, then interpolate so you don't depend on simply vertical/horizontal coordinates.


RB Tree Left and Right Rotate Functions I am writing a program to implement the Red-Black Tree data structure in java. Below is the beginning of my implementation, namely the left and right rotate functions. I want to know if these functions are correct, and if not, any tips on correcting them. I am basing my functions off of pseudocode I found. Here is a link to the pseudocode I used. If you need to see the node class let me know however I think it is self explanatory for the most part.

/*
 * To change this license header, choose License Headers in Project Properties.
 * To change this template file, choose Tools | Templates
 * and open the template in the editor.
 */
package rbtree;

/**
 *
 * @author williamnewman
 */
public class RBTree {
    node root;

   RBTree(int val){
       node r = new node();

       r.setVal(val);
       this.root = r;
   }

   void leftRotate(node x){
     node y = x.rightChild;
     x.rightChild = y.leftChild;
     if(y.leftChild != null){
         y.leftChild.parent = x;
     }
     y.parent = x.parent;
     if(x.parent == null)
         this.root = y;
     else if(x == x.parent.leftChild){
         x.parent.leftChild = y;
     }
     else{
         x.parent.rightChild = y;
     }
     y.leftChild = x;
     x.parent = y;
    }

   void rightRotate(node x){
       node y = x.leftChild;
       x.leftChild = y.rightChild;
       if(y.rightChild != null){
           y.rightChild.parent = x;
       }
       y.parent = x.parent;
       if(x.parent == null){
           this.root = y;
       }
       else if(x == x.parent.rightChild){
           x.parent.rightChild = y;
       }
       else{
           x.parent.leftChild = y;
       }
       y.rightChild = x;
       x.parent = y;
   }

}

In Java class names should start with an upper letter (node->Node).
Why all your methods have default visibility? You usually want to have public methods though if this intentional then just ignore this point.
You seem to be restricting your RBTree to have only ints. You could use generics instead to make it more flexible and make it useful in more cases
Your formatting is inconsistent (indentation).


Encryption program for a messenger app I am building an encryption program for a messenger app I am developing.

I was wondering if the code looks clean and succinct, and at the same times is safe and secure:

import random
import os
import time

key = os.urandom(64)

def encrypt(_key, message):
    letter_map_message = []
    ascii_msg_value = ''.join(str(ord(c)) for c in message)
    for c in message:
        ''.join(str(ord(c)))
        letter_map_message.append(ord(c))
    ascii_key_value = ''.join(str(ord(x)) for x in str(_key))
    new_msg_encrypted = int(ascii_msg_value) * int(ascii_key_value)
    returned = (new_msg_encrypted, letter_map_message)
    return returned;

def decrypt(_key, message, letter_map):
    ascii_key_value = ''.join(str(ord(x)) for x in str(_key))
    msg_ascii_divided = int(message)/int(ascii_key_value)
    letters = []
    for c in range(len(letter_map)):
        li = letter_map[c]
        c = int(li)
        letter = chr(c)
        letters.append(letter)
    new_msg_decrypted = ''.join(letters)
    return new_msg_decrypted;

encrypted = encrypt(key, "HEHHEHEHHE")
print(encrypted[0])
decrypted = decrypt(key, encrypted[0], encrypted[1])
print(decrypted)

This is not a secure system.

I haven't actually read your code, but unless you have a team of at least 10 security researchers who have been doing nothing other than testing this encryption scheme, you should not use it in production code. You should be using a crypto library built by someone that knows what they are doing. I would recommend cryptography. Docs are here https://cryptography.io/en/latest/. Good luck


Singleton-like pattern for a project I'm changing the inner implementation of a project.  In order to do so, I've created an interface that will be implemented in 2 different ways.

One of this ways is by class A, which is a wrapper for an outside class (class "B") from a .dll file (a product of other team).

Class B is a private a member of class A.  Class B ctor takes an argument, which does not change during the run of my application. It needed to be created only once, and to stay available for class A everywhere in the code.

Class A is an implementation of interface.  It use the capabilities of class B in order to implement it's functions, and obviously needed to be available also everywhere in the code.

I've concluded that:

Class A has to be:

explicitly created once (hence, not static)
only once, (because I need B to be created only once.)
accessible from anywhere in the code
have a member (B) that must initialized once, and only once (private member, needed only inside class A)

Concerning condition #4, ideally I would like to initialize B in the constructor.

A bit similar to singleton pattern, with the exception of wanting to initialize a member in the constructor.

I've come to a solution, and would like to get feedback.

public sealed class A
{
   private static SomeClass B = null; // the "must initialized only once" member
   // setting B:
   private void setB(int i)
   {
      B = new SomeClass(i);
   }
   // ctor
   public A(int i)
   {
      if(B == null)
         setB(i);
      else throw new Exception("An instance of A can be created only once");
   }
}

The idea is to assign the class to static object at the beginning of the code.  It's not very neat, but it compiles and seems to do the job just right.  I don't use singleton since it won't be possible to initialize the member in the ctor.  By the way, thread safety is not a concern here.

SomeClass needed only inside class A. SomeClass comes from dll, and class A kind of wrapping it.  A does answering cross-cutting concern, it is an implementation of Interface used all over the code. I've tried to implement singleton, but as much as I tried it didn't answer my needs.

If you need to wrap SomeClass, make your wrapper an abstraction:

public interface ISomeClassWrapper
{
    // expose the methods and properties you want to wrap
}

Then everywhere it's needed in the code, replace static method calls like this:

public void DoSomething()
{
    SomeClass.Xyz();
}

With something like this:

public class MyOtherClass
{
    private readonly ISomeClassWrapper _wrapper;

    public MyOtherClass(ISomeClassWrapper wrapper)
    {
        _wrapper = wrapper;
    }

    public void DoSomething()
    {
        _wrapper.Xyz();
    }
}

Now, if you're worried about how many instances of SomeClassWrapper (which implements ISomeClassWrapper but MyOtherClass doesn't need to know that), don't instantiate it. ever.

How do you ensure there's only 1 instance of it?

When you've replaced SomeClass.Xyz(); with _wrapper.Xyz();, you've done several things:

MyOtherClass is now decoupled from SomeClass, and even from SomeClassWrapper.
The ISomeClassWrapper dependency is now statically declared as such, so just by looking at MyOtherClass's constructor you know it needs an implementation for ISomeClassWrapper to carry out its tasks.
You can now provide a fake/mock implementation of ISomeClassWrapper, and write a test for DoSomething() without worrying about side-effects in code that you have no control over.
The code that's responsible for providing the ISomeClassWrapper instance/implementation, is also responsible for passing everyone that wants an ISomeClassWrapper instance, the same instance. Most IoC containers can do this very, very easily.


Transmitting/Sending big packet safely using TcpClient/Socket I am writing a TCP based client that need to send and receive data. I have used the Asynchronous Programming Model (APM) provided for Socket class by the .NET Framework.

After being connected to the socket, I start waiting for data on the socket using BeginReceive.

Now, while I am waiting for the data on the Socket, I may need to send data over the socket. And the send method can be called multiple times,

So I have make sure that

All the bytes from previous Send call is entirely sent.
The way I am sending the Data is safe considering that, while a data send is in progress, any call to send data can be made.

This is my first work on socket, So is my approach right to send data ?

    private readonly object writeLock = new object();
    public void Send(NetworkCommand cmd)
    {
        var data = cmd.ToBytesWithLengthPrefix();
        ThreadPool.QueueUserWorkItem(AsyncDataSent, data);
    }

    private int bytesSent;
    private void AsyncDataSent(object odata)
    {
        lock (writeLock)
        {
            var data = (byte[])odata;
            int total = data.Length;
            bytesSent = 0;
            int buf = Globals.BUFFER_SIZE;
            while (bytesSent < total)
            {
                if (total - bytesSent < Globals.BUFFER_SIZE)
                {
                    buf = total - bytesSent;
                }
                IAsyncResult ar = socket.BeginSend(data, bytesSent, buf, SocketFlags.None, DataSentCallback, data);
                ar.AsyncWaitHandle.WaitOne();
            }
        }
    }

How object is changed into byte[], sometimes the NetworkCommand can be as big as 0.5 MB

    public byte[] ToBytesWithLengthPrefix()
    {
        var stream = new MemoryStream();
        try
        {
            Serializer.SerializeWithLengthPrefix(stream, this, PrefixStyle.Fixed32);
            return stream.ToArray();
        }
        finally
        {
            stream.Close();
            stream.Dispose();
        }
    }

Instead of using the ToArray() method of the MemoryStream you could use GetBuffer() instead because this isn't creating a new array.

Edit based on @Iridium's comment

But this is only good if you use one of the constructor that creates a not resizable MemoryStream which isn't the case for the default constructor.

Calling Dispose() on the MemoryStream closes the stream too.

If you enclose the stream inside inside a using block the disposing of the stream will be handled automatically.

public byte[] ToBytesWithLengthPrefix()
{
    using (var stream = new MemoryStream())
    {
        Serializer.SerializeWithLengthPrefix(stream, this, PrefixStyle.Fixed32);
        return stream.ToArray();
    }
}

I needed to look twice at var data = (byte[])odata; which is a clear sign that you should make the method argument odata easier to distinguish from data, so objectData or dataToSend would be better.

int buf isn't well named either. One shouldn't use abbreviations to name variables. A much better name would be for instance dataSize or just size to indicate that it is the amount of data being sent.


Simple implementation of a generic stack I'm fairly new to C++ programming. To help I've been writing my own data structures for practice. I'd liked this to be judged as if it was written by a professional, and receive honest feedback. I already have a few concerns.

Am I managing memory correctly? I don't believe I have any leaks. A lot of advice I get when using pointers is to use smart pointers, but raw pointers are fine when the class encapsulates all pointers correct?
I'm unhappy my << operator overload has to call to peekAll inside the class. I feel like I'm going about this in the wrong way. Aside from that should I overload any other operators? I didn't feel arithmetic operators made much sense.

#include <ostream>

template <typename T>
class BBStack {
public:
BBStack(T type) {
    head = new Node(type);
}

virtual ~ BBStack() {
    Node* temp = head;
    while (head != nullptr) {
        temp = head->next;
        delete head;
        head = temp;
    }
    delete temp;
}

void push(T type) {
    Node *newNode = new Node(type, head);
    head = newNode;
}

T peek() const {
    return head->data;
}

T pop() {
    if (head == nullptr) {
        std::cout<<"Error stack is empty"<<std::endl;
        return NULL;
    } else {
        Node *temp = head;
        T result = temp->data;
        head = head->next;
        delete temp;
        return result;
    }
}

std::ostream& peekAll(std::ostream& out) const {
    if (head == nullptr) return out << "Stack is empty";
    Node* temp = head;

    while (temp != nullptr) {
        out << temp->data << " ";
        temp = temp->next;
    }

    delete temp;
    return out;
}

private:
    struct Node {
        Node(T type, Node* _next = nullptr) : data(type), next(_next) {}
        Node(Node* node) : data (node->data), next (node->next) {}

        T data;
        Node* next;
    };
    Node* head;

};

template <typename T>
std::ostream& operator<< (std::ostream& out, const BBStack<T>& stack) {
    return stack.peekAll(out);
}

Ownership

Yes raw pointers are fine if you are not transferring ownership (Is the class responsibility to delete them).

Pop function

Also I am not sure if this will compile for any data other than pointers and types convertible to 0 since you return NULL. (Also is better to use nullptr consistently).

 T pop() {
    if (head == nullptr) {
        std::cout<<"Error stack is empty"<<std::endl;
        return NULL;
    } else {
        Node *temp = head;
        T result = temp->data;
        head = head->next;
        delete temp;
        return result;
    }
}

IMO it will be better to throw a underflow exception since is the client responsibility to check that is not empty before. Also avoid returning the data since we will be throwing an exception (For more info on this check out this article http://ptgmedia.pearsoncmg.com/images/020163371x/supplements/Exception_Handling_Article.html)

 void pop() {
    if (head == nullptr)
        throw std::underflow_error("underflow");

    Node *temp = head;
    head = head->next;
    delete temp;
 }

So the client will have to call peek before pop to get the same effect.

Parameters:

In most parameter like this (T type) you should change it to (const T& type)
to avoid copying. Even better would be to provide an overload for rvalue references like this (T&&)

peekAll function

This function seams kind of weird. Why are you deleting temp? . It seams that after the while loop temp should always be nullptr.  And aren't you just writing the stack on the stream.  I would just omit this function and move it to the << operator overload.

Also you should implement an empty function.


Taking some JSON data, converting the date format and calculating a user average I'm originally a Java developer taking a stab at some Scala development. One of the main reasons I'm posting is because I know that in Scala you can write your code in a Java (OOP) way and it will run but that makes it bad Scala code.

I was hoping I could get a review of my code and maybe somebody could point out how I could be doing things a bit more in a functional way.

A method to get a set of JSON objects from a directory:

def createJsonSessionSet(filePaths: Set[String]) : Set[JSONObject] = {
    val sessions: Set[JSONObject] = Set()
    val parser: JSONParser = new JSONParser

    for (filePath <- filePaths) {
      val jsonObject: JSONObject = parser.parse(new FileReader(filePath)).asInstanceOf[JSONObject]
      sessions.add(jsonObject)
    }

    return sessions
}

A method that lists directories from a file path:

def getListOfJsonFilePaths(file: File): Set[String] = {
    val filePaths: Set[String] = Set()

    for (file <- file.listFiles) {
      filePaths.add(file.getAbsolutePath)
    }

    return filePaths
}

A method that takes a set of json objects and converts the time:

def convertTimeZone(sessions: Set[JSONObject]): Set[JSONObject] = {

val convertedSessions: Set[JSONObject] = Set()
val parser: JSONParser = new JSONParser

val utcFormat: DateFormat = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss'Z'")
utcFormat.setTimeZone(TimeZone.getTimeZone("UTC"))

val newFormat: DateFormat = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss'Z'")
newFormat.setTimeZone(TimeZone.getTimeZone("Etc/GMT+7"))

for (session <- sessions) {

  val date: Date = utcFormat.parse(session.get("UTCStartTime").toString)

  implicit val formats = Serialization.formats(NoTypeHints)

  val convertedSession = Map(
    "SessionId" -> session.get("SessionId").toString,
    "UserId" -> session.get("UserId").toString,
    "Score" -> session.get("Score").toString,
    "ETCStartTime" -> newFormat.format(date)
  )

  val jsonObject: JSONObject = parser.parse(write(convertedSession)).asInstanceOf[JSONObject]

  convertedSessions.add(jsonObject)

}
return convertedSessions
}

A method that returns a set of JSON objects based on a set of JSON objects:

def getOutputData(convertedSessions: Set[JSONObject]): Set[JSONObject] = {

val outputData: Set[JSONObject] = Set()

val distinctUserIds: Set[String] = Set()
val distinctDays: Set[String] = Set()

for (session <- convertedSessions) {
  distinctUserIds.add(session.get("UserId").toString)
  distinctDays.add(session.get("ETCStartTime").toString.split("T")(0))
}

for (day <- distinctDays) {
  for (userId <- distinctUserIds) {

    implicit val formats = Serialization.formats(NoTypeHints)

    val data = Map(
      "UserId" -> userId,
      "Day" -> day,
      "AverageScore" -> 10
    )
    val parser: JSONParser = new JSONParser

    val jsonObject: JSONObject = parser.parse(write(data)).asInstanceOf[JSONObject]

    if (!jsonObject.get("AverageScore").toString.contains("NaN"))
       outputData.add(jsonObject)

  }
}

return outputData
}

I went about this mini project by first writing it in Java which was easy enough. I had trouble with the next method that returned fine in Java, but not so well in Scala.  I think it has something to do with the fact that in Java I used ArrayLists but Scala not supporting that I used sets.

def getUserDailyAverageScore(userId: String, day: String, sessions:  Set[JSONObject]): Double = {
    var sum: Double = 0
    var sessionSize: Int = 0

    for (session <- sessions) {
      println("test")
      if (session.get("UserId").toString.equals(userId) && session.get("ETCStartTime").toString.startsWith(day)) {
        sum += session.get("Score").asInstanceOf[Double]
        sessionSize += 1
      }
    }

    return sum / sessionSize
}

From debugging, the code never steps into the if statement whilst in Java it does.

This answer is by no means complete but I would like to give you a few hints.
Use for-loops sparingly, prefer combinators - map, flatMap, filter, zip etc.
For example in the last snippet, instead of iterating over sessions using for loop, try something like:
val filtered = session.filter(session.get()...)
val sessionSize = filtered.size
val sum = filtered.map(_.get("Score").asInstanceOf[Double]).sum

Always think twice before writing for-loop, it is very likely you can achieve the same using combinators.
Of course, common sense applies, don't try to pack all your code into one long line of combinators - split it into many lines and use indentation and local variables to keep it readable.
You don't have to declare types when defining variables
It is quite typical in Scala code not to declare types for local variables, so try:
val date = utcFormat.parse(session.get("UTCStartTime").toString)

without declaring type :Date
However, if variable has wider scope or providing type would improve readability then, of course, you can still do it. I encourage you to declare return types on public methods.
Learn about different implementations of list in Scala
Most popular ones below.
Mutable:

ListBuffer - quick sequential access
ArrayBuffer - quick random access (like Java's ArrayList you mentioned)

Immutable:

List - implemented as linked list

Prefer immutable ones if you want to code in FP style. Read about "structural sharing" if you have doubts about performance of immutable data structures
Don't use return keyword - it is redundant and potentially dangerous
In your examples you can remove all occurrences of return, read more about usage of return here: Don't use return in Scala
Bonus
Here is a talk from Scala's author where he presents his definition of idiomatic Scala - Scala with Style
Hope this will help.


Solution of class design issue of generic of restricted type I am developing a statistics library in .net. I have developed a Set<T> data structure. Now, I want to derive a data structure named DescriptiveStatisticalSet<T>, and I want this set to be able to operate only on integer and double types.
Problem:

I want to implement a generic type that is able to work with integer
and double data types only.
Say, I have the following interfaces and classes:
public interface IIntegerDataType
{
    int Data { get; set; }
    int Add(int other);
}

public interface IDoubleDataType
{
    double Data { get; set; }
    double Add(double other);
}
public class IntegerDataType : IIntegerDataType
{
    public int Data { get; set; }

    public int Add(int other)
    {
        return Data + other;
    }
}
public class DoubleDataType : IDoubleDataType
{
    public double Data { get; set; }

    public double Add(double other)
    {
        return Data + other;
    }
}

Is it possible to create a generic type DataType<T> so that both
(and only) IntegerDataType and DoubleDataType could be accessed
through that generic type?

Solution:
I have devised the following solution:
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace DataTypeNamespace
{
    public interface IDataType
    {
        object Add(object other);
        void SetVal(object other);
    }
    public interface IDataType<T> where T : IDataType, new()
    {
        T Add(T other);
    }
    class IntegerDataType : IDataType
    {
        public int Data { get; set; }

        public object Add(object other)
        {
            int o = ((IntegerDataType)other).Data;
            return Data + o;
        }
        public void SetVal(object other)
        {
            Data = (int)other;
        }
    }

    class DoubleDataType : IDataType
    {
        public double Data { get; set; }

        public object Add(object other)
        {
            double o = ((DoubleDataType)other).Data;
            return Data + o;
        }

        public void SetVal(object other)
        {
            Data = (double)other;
        }
    }

    public class DataType<T> : IDataType<T> where T : IDataType, new()//https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/new-constraint
    {
        IDataType _item;
        public DataType(IDataType item)
        {
            _item = item;
        }

        public T Add(T other)
        {
            object o = _item.Add(other);

            T t = new T();

            t.SetVal(o);

            return t;
        }
    }

    public class MainClass
    {
        public static void Main(string[] args)
        {
            //IntegerDataType item1 = new IntegerDataType();
            //item1.Data = 10;

            //IntegerDataType item2 = new IntegerDataType();
            //item2.Data = 20;

            //DataType<IntegerDataType> l1 = new DataType<IntegerDataType>(item1);
            //IntegerDataType sum1 = l1.Add(item2);

            DoubleDataType item3 = new DoubleDataType();
            item3.Data = 10.5;

            DoubleDataType item4 = new DoubleDataType();
            item4.Data = 20.5;

            DataType<DoubleDataType> l2 = new DataType<DoubleDataType>(item3);
            DoubleDataType sum2 = l2.Add(item4);
        }
    }
}

Can someone review this? Or, maybe help me to improve?

As has been said in the comments, it's unclear why you feel you need this behaviour to restrict the types used.  There's an interesting discussion around constraining types here, which includes a link to example code.
So, there's a couple of questions.  Why do you need to restrict it to double / integer types?  Is it really that you want to do is restrict it to types that support certain operations?  What is the actual downside to using it for Decimal / float?
You've presented an example of IDataType, however suggested that really your goal is to put these items into a set.  So this begs the question, are you planning on creating DescriptiveStatisticalSet<IntegerDataType> or DescriptiveStatisticalSet<IDataType>?  If it's IntegerDataType, what are you gaining over Int32 for example?  If it's IDataType, then are you expecting to have both IntegerDataType  and DoubleDataType both present in the same set?  If so, what are you planning on doing if an Integer has the same value as a Double?  Do you keep both because they are different types, or keep whichever one was there first, or keep the Double because it's more flexible, or the Integer because it's faster?
Looking at your actual code, there's some aspects which are worth mentioning.

object Add(object other);

If we look at IntegerDataType, this method takes in an other of type IntegerDataType, but returns an int.  This it not at all obvious from the method signature.  You are working around this to an extent in your DataType class, which does some recasting by creating a new instance of the returned object, however this seems unnecessarily complex.  Particularly since the implementation of Add relies on casting other to the correct type.  Calling Add with a DoubleDataType for example throws a cast exception.

public int Data { get; set; }

Both your Integer and Double data types declare a Data property that has both public getter and setter.  Since you're implementing an interface to setVal, does it really make sense to declare this public setter?  It feels like it would be better for the set to be private, in order to encourage the client to use the interface method, in case you decide you need to do additional checking in the future.
Your DataType class seems to exist purely to create a new item from the object version of Add.  This seems to add unnecessary complexity over just having the initial Add return the correct datatype.  So, it's a bit unclear to me what advantage your current approach has over simply doing this:
public interface IOtherDataType<T>
{
    T Data { get;}

    IOtherDataType<T> Add(IOtherDataType<T> other);
    void SetVal(T other);
}

public class IntType : IOtherDataType<int>
{
    public IntType(int data)
    {
        Data = data;
    }

    public int Data { get; private set; }

    public IOtherDataType<int> Add(IOtherDataType<int> other)
    {
        return new IntType(Data + other.Data);
    }

    public void SetVal(int other)
    {
        Data = other;
    }
}

Which can be used in a similar way...
IntType five = new IntType(5);
IOtherDataType<int> eleven = five.Add(new IntType(6));


Basic Python OO bank account I am learning Python and in particular learning about classes. Since I'm self teaching, I'd appreciate any feedback and constructive criticism.

I have a couple specific questions:

In Python you can have multiple functions and classes in a single file (module), but is it considered bad practice to do so?
Python doesn't have private, so is there a point to having accessor methods like I do below?
My IDE suggest I put a comment beneath each function signature. Is it still good to do this when the functions are short and simple, like withdraw(x) in a bank account?

BankAccount.py

# bank account class
class BankAccount:
    def __init__(self, name, balance=0.00):
        self.name = name
        self.balance = balance

    def deposit(self, amount):
        """make a deposit"""
        self.balance += amount

    def withdraw(self, amount):
        """make a withdraw"""
        if amount > self.balance:
            raise ValueError("insufficient funds")
        self.balance -= amount

    def get_balance(self): #are accessor methods needed in Python?
        """check the balance"""
        return self.balance

def main():
    customer1 = BankAccount('Alex')
    print(customer1.get_balance())
    customer1.deposit(100)
    customer1.withdraw(30)
    print(customer1.get_balance())

    customer2 = BankAccount('Sam', 200)
    print(customer2.get_balance())

if __name__ == "__main__":
    main()

Python doesn't have private anything but we still have a need to separate inners of a class from the public interface.

By convention, attributes or methods whose name starts with a single underscore are considered internals of the class and, thus, not part of the public API. There is no mechanism in place to restrict their access, so user that know what they are doing can still use them; but if anything goes wrong, it will most likely be their fault, not yours.

Also, in the context of versionned libraries, internals are not subject to backward compatibility and can change without notice on a new release.

But instead of methods as getter or setters, we prefer properties that allows the same kind of flexibility (read-only vs. read-write) with the simplicity of attributes access.

Appart from that, I would add either __str__ or __repr__ (or both) as methods in your class to simplify printing accounts:

class BankAccount:
    def __init__(self, name, balance=0.00):
        self.name = name
        self._balance = balance

    def deposit(self, amount):
        """make a deposit"""
        self._balance += amount

    def withdraw(self, amount):
        """make a withdraw"""
        if amount > self._balance:
            raise ValueError("insufficient funds")
        self._balance -= amount

    @property
    def balance(self):
        """check the balance"""
        return self._balance

    def __repr__(self):
        return '{0.__class__.__name__}(name={0.name}, balance={0.balance})'.format(self)

    def __str__(self):
        return 'Bank account of {}, current balance: {}'.format(self.name, self.balance)

Example usage being:

customer1 = BankAccount('Alex')
print(repr(customer1))
customer1.deposit(100)
customer1.withdraw(30)
print(customer1)
customer2 = BankAccount('Sam', 200)
print(customer2.balance)

As regard to the "one class per file" rule, it may make sense in languages where a file does not bear much meaning.

But in python a file is a module. It is perfectly fine and expected to put related entities in them as they define a namespace.

Beside, where would you put simple functions and constants using a "one class per file" approach? As class attribute of some sort of "namespace" class? Yuck! Module already serve this purpose, don't reinvent the wheel.


k th smallest element in row wise and column wise sorted matrix Find the kth smallest element in row wise and column wise sorted element:

  Example:

  matrix[row][col] < matrix[row+1][col]

  matrix[row][col] < matrix[row][col+1]

  mat=

    1  2  21

    11 23 25

    31 36 41

I have written following code, How to make more readable and improve its complexity:

import heapq
def kth_smallest(k, mat):
    m = len(mat)
    n = len(mat[0])
    hp = []
    for i in xrange(n):
           heapq.heappush(hp,[mat[0][i], 0, i])
    heapq.heapify(hp)
    while k >1:
        x = heapq.heappop(hp)
        if x[1]+1 < m:
            heapq.heappush(hp,[mat[x[1]+1][x[2]], x[1]+1, x[2]])
        heapq.heapify(hp)
        k-=1

    return hp[0][0]

print kth_smallest(8, [[1,10,12],[2,11,21], [3,31,45]])

heappush and heappop maintain the heap invariant; there is no need to call heapify after them. Alternatively, you could initialize the heap from an unsorted list using heapify and skip the heappush, but as the list is already sorted in this case, it is already a heap and you don't need to apply either function.
You push the whole first row to the heap but the first k elements would suffice thanks to the rows being already sorted.
Instead of

for i in xrange(n):
       heapq.heappush(hp,[mat[0][i], 0, i])

it would be more idiomatic to use

for i, value in enumerate(mat[0]):
       heapq.heappush(hp, [value, 0, i])

though as I already advised to take only k elements, you could instead use

for i, value in zip(xrange(k), mat[0]):

More idiomatic instead of while k >1: ... k-=1 would be for _ in xrange(k - 1):
Readability of
[mat[x[1]+1][x[2]], x[1]+1, x[2]]

could be improved to [mat[row + 1][col], row + 1, col]

by unpacking value, row, col = heapq.heappop(hp)

Revised code:

def kth_smallest(k, mat):
    '''Return the kth smallest element in row wise and column wise sorted matrix.
    mat represents the matrix as list of lists.
    '''
    m = len(mat)
    hp = [(value, 0, col) for col, value in zip(xrange(k), mat[0])]
    # hp is a min heap because mat[0] is assumed to be sorted
    for _ in xrange(k - 1):
        value, row, col = heapq.heappop(hp)
        row += 1
        if row < m:
            heapq.heappush(hp, (mat[row][col], row, col))

    return hp[0][0]